Skip to content

OBPIH-6020 Preference Type Modal#4480

Merged
awalkowiak merged 3 commits intofeature/product-supplier-list-redesignfrom
OBPIH-6020-3
Jan 31, 2024
Merged

OBPIH-6020 Preference Type Modal#4480
awalkowiak merged 3 commits intofeature/product-supplier-list-redesignfrom
OBPIH-6020-3

Conversation

@alannadolny
Copy link
Collaborator

I included fixes after @awalkowiak's review in the product source feature branch in this PR

Comment on lines 66 to 78
roles: [RoleType.ROLE_PRODUCT_MANAGER],
minimumRequiredRole: isAdmin,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these need to be separated? Could ROLE_ADMIN be moved to the roles, and then just checked if all required roles are there?

Copy link
Collaborator Author

@alannadolny alannadolny Jan 29, 2024

Choose a reason for hiding this comment

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

@awalkowiak I think it's better to have it separated because it's a minimum required role, so when I pass the admin role, I can also be a superuser, and it still should be true. When I have to check the admin role within the roles argument I have to include the superuser here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@alannadolny Ok, but then call it supplementalRoles as it is on the backend, and maybe change the ordering of these params. So we have "required roles" first, and then "supplemental" ones.

@awalkowiak
Copy link
Collaborator

@alannadolny plus fix the conflicts

@awalkowiak awalkowiak merged commit 30c342b into feature/product-supplier-list-redesign Jan 31, 2024
@awalkowiak awalkowiak deleted the OBPIH-6020-3 branch January 31, 2024 12:01
awalkowiak pushed a commit that referenced this pull request Mar 12, 2024
* OBPIH-6020 Change font size to smaller

* OBPIH-6020 Fixes after product source review

* OBPIH-6020 Change naming of the argument in hasPermission function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants