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

[#57264] change available user callbacks for custom fields #16909

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Kharonus
Copy link
Member

@Kharonus Kharonus commented Oct 7, 2024

Ticket

OP#57264

What are you trying to accomplish?

  • custom fields of type user should show the correct list of assignable users
  • this was wrong for work package custom fields of type user, i.e. they did not show users with permissions from work package shares

What approach did you choose and why?

  • project and work package custom fields of type user now use the endpoints available_assignees to get a list of values

- https://community.openproject.org/work_packages/57264
- project and work package custom fields of type user now use the
  endpoints `available_assignees` to get a list of values
@Kharonus Kharonus self-assigned this Oct 7, 2024
@ulferts ulferts self-requested a review October 8, 2024 08:01
Copy link
Contributor

@ulferts ulferts left a comment

Choose a reason for hiding this comment

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

I believe the PR introduces an unwanted limitation, @Kharonus (see below).

if represented_is_existent_instance.(represented, Project)
api_v3_paths.available_assignees_in_project(represented.id)
elsif represented_is_existent_instance.(represented, WorkPackage)
api_v3_paths.available_assignees_in_work_package(represented.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change reduces the possible values to those that have the work_package_assigned permission. This includes those that have the work package shared with them but excludes those that are members of the project but have a different set of permissions. Before, all project members where possible values. On top of this, only if the work package is shared with the edit permission (which includes work_package_assigned), will the shared with user show up in the list.

The intended outcome would be to allow all project members and on top of those also allow users which have a work package membership.

To that end, a specific end point would probably be the best solution.

What is unsettling about the code change is that it indicates a lack of validations when actually creating a custom value for the custom field. There doesn't appear to be any constraint. This weakness existed before the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahoi @ulferts ,

should we take that discussion into a synchronous call? I'd argue, that you shouldn't be able to assign users to a work package custom field of type user, if this user does not have the work_package_assigned permission.

So, this is in the end a product decision: Is the assignee set the same for the default Assignee field and any custom field of type user (easy solution)? Or are there differences and if yes, what kind?

Copy link
Member Author

@Kharonus Kharonus Oct 8, 2024

Choose a reason for hiding this comment

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

What is unsettling about the code change is that it indicates a lack of validations when actually creating a custom value for the custom field. There doesn't appear to be any constraint. This weakness existed before the PR.

How does it indicate that? The change only changes the available_values in the schema response of the APIv3. It does not change the actual update process/validation. Is it true, that there is no validation? Could you assign any user?

EDIT: I investigated it, and indeed, the validation is apart from what is rendered in the schema. I.e. any user, that is not a member triggers a validation failure. But, I could e.g. pass a user currently, that does not have the work_package_assigned permission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants