-
-
Notifications
You must be signed in to change notification settings - Fork 392
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
OBPIH-5799 / OBPIH-5800 Add purchase approver and request approver permissions #4279
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me although I am not sure why was ROLE_APPROVER
not included in the list before and should we include ROLE_PURCHASE_APPROVER
there now?
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a translation for the new role type and rename the translation for approver
@@ -195,7 +195,7 @@ class UserService { | |||
Boolean hasRoleApprover(User u) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be also renamed, since we will have two "approver" roles now hence it can be misleading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a translation for the new role type and rename the translation for approver
I didn't do that because I can't see any translations for the previous ROLE_APPROVER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see other translations for similar roles, so I think it's not needed
@drodzewicz hmmm, you asked a really good question. I think it should be added, but maybe there is a reason why it wasn't added. @awalkowiak Do you know something about that? |
grails-app/i18n/messages.properties
Outdated
@@ -998,6 +998,8 @@ enum.RoleType.ROLE_ADMIN=Administrator | |||
enum.RoleType.ROLE_MANAGER=Manager | |||
enum.RoleType.ROLE_ASSISTANT=Assistant | |||
enum.RoleType.ROLE_BROWSER=Browser | |||
enum.RoleType.ROLE_PURCHASE_APPROVER=Purchase approver | |||
enum.RoleType.ROLE_REQUEST_APPROVER=Request approver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename REQUEST to REQUISITION throughout.
@@ -159,4 +159,6 @@ | |||
<include file="0.8.x/changelog-2023-04-14-1200-create-table-product-merge-logger.xml" /> | |||
<include file="0.8.x/changelog-2023-05-17-1453-add-quantity-counted-column.xml" /> | |||
<include file="0.8.x/changelog-2023-05-22-1800-add-requisition-template-document-type.xml" /> | |||
<include file="0.8.x/changelog-2023-09-13-1233-rename-approver-role-to-purchase-approver.xml" /> | |||
<include file="0.8.x/changelog-2023-09-13-1252-add-request-approver-role.xml" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include filenames in the renaming i.e. add-requisition-approver-role.xml.
…rmissions (#4279) * OBPIH-5799 Add request approver role * OBPIH-5799 Add changelog for request approver * OBPIH-5800 Rename ROLE_APPROVER to ROLE_PURCHASE_APPROVER * OBPIH-5800 Add changelog for renaming approver to purchase approver * OBPIH-5799 Fix after review * OBPIH-5799 Fix method name in tests * OBPIH-5799 Rename request approver to requisiton approver
As I know we decided to do OBPIH-5800 within OBPIH-5799, so I have pushed changes from both of them to this PR.