Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#55299] Project attributes: Fix phrasing of "required" and "visible" fields #16334

Conversation

dombesz
Copy link
Contributor

@dombesz dombesz commented Aug 5, 2024

What are you trying to accomplish?

Update custom field attribute names and descriptions for required and visible. Change the visible boolean field to admin_only and flip the logic around the flag.

What approach did you choose and why?

  • Rename the text in the translation files.
  • Create a migration that renames visible to admin_only and flip the boolean values also flipping the default value from true to false.
  • Replace the visible occurences in the code with admin_only.

Ticket

https://community.openproject.org/wp/55299
Also addresses https://community.openproject.org/wp/56897

Merge checklist

  • Added/updated tests
  • Added/updated documentation in Lookbook (patterns, previews, etc)
  • Tested major browsers (Chrome, Firefox, Edge, ...)

Copy link

github-actions bot commented Aug 5, 2024

1 Warning
⚠️ This PR has migration-related changes on a release branch. Ping @opf/operations

Generated by 🚫 Danger

@dombesz dombesz force-pushed the bug/55299-project-attributes-fix-phrasing-of-required-and-visible-options branch from 3aa3034 to a2819bb Compare August 5, 2024 15:21
@dombesz dombesz requested a review from a team August 5, 2024 18:34
@dombesz dombesz requested a review from a team August 5, 2024 18:34
@dombesz dombesz marked this pull request as ready for review August 5, 2024 18:35
@dombesz dombesz force-pushed the bug/55299-project-attributes-fix-phrasing-of-required-and-visible-options branch from a2819bb to 4442592 Compare August 5, 2024 18:35
@cbliard cbliard requested review from cbliard and removed request for a team August 6, 2024 08:47
Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok if renaming also happens for other custom fields?

"Required for all projects" sounds a bit off: for instance for user custom fields and groups custom fields, the custom field is not related to any projects. Same for Time Spent custom fields.

image

For work package custom fields, it's also strange to have "Required for all projects" and "For all projects" at the same time, as it's possible to have it required and being disabled for some projects.

And the description for "Searchable" feels wrong too as it is specific to project attributes / project custom fields.

Maybe the renaming should be limited to project attributes?

@cbliard
Copy link
Member

cbliard commented Aug 6, 2024

It also has an impact on the table view of work package custom fields

image

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The admin-only attribute can not be saved.

admin-only.attribute.not.kept.webm

:admin_only is missing in permitted params in app/models/permitted_params.rb:462.

Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

It seems to work well, except the field admin_only which can not be saved. Other than that it matches the spec.

The renaming seems confusing for custom fields that are not project custom fields. I'd like to have @psatyal point of view about it.

@dombesz
Copy link
Contributor Author

dombesz commented Aug 6, 2024

Thanks.

It seems to work well, except the field admin_only which can not be saved. Other than that it matches the spec.

The renaming seems confusing for custom fields that are not project custom fields. I'd like to have @psatyal point of view about it.

Thanks @cbliard , I agree the renaming seems to be tailored to the project attributes. One option could be to apply the renaming only on the project attributes, and not all the custom fields. cc @psatyal .

@cbliard
Copy link
Member

cbliard commented Aug 6, 2024

I agree the renaming seems to be tailored to the project attributes. One option could be to apply the renaming only on the project attributes, and not all the custom fields.

Yep, it looks like a good option. Parimal is back tomorrow.

@cbliard
Copy link
Member

cbliard commented Aug 7, 2024

I agree the renaming seems to be tailored to the project attributes. One option could be to apply the renaming only on the project attributes, and not all the custom fields.

@dombesz I talked about it during today's frontend meeting. We agreed upon this:

  • Apply renaming of "required" and "visible" attribute label and description only for project attributes (so it should be reverted back for all other custom fields)
  • Apply renaming of "visible" attribute for all custom fields (which is user and project, so it's 👌 like it is right now). This will also address and fix https://community.openproject.org/projects/openproject/work_packages/56897/activity
  • Target 14.5 instead of 14.4, because it's not a regression introduced in 14.4, and it would be too late for QA-ing for 14.4 before the release.

I'll update the work package description to reflect this.

@dombesz dombesz changed the base branch from release/14.4 to dev August 7, 2024 12:00
@dombesz dombesz added this to the 14.5.x milestone Aug 7, 2024
@dombesz dombesz force-pushed the bug/55299-project-attributes-fix-phrasing-of-required-and-visible-options branch from 597ec57 to e56ca49 Compare August 7, 2024 12:38
@dombesz dombesz merged commit f0dc5c9 into dev Aug 7, 2024
8 checks passed
@dombesz dombesz deleted the bug/55299-project-attributes-fix-phrasing-of-required-and-visible-options branch August 7, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants