Conversation
| <Menu class="sapUiSizeCompact"> | ||
| <MenuItem press="onKeyTableDeletePress" text="{i18n>delete}" | ||
| icon="sap-icon://delete"> | ||
| icon="sap-icon://delete" |
There was a problem hiding this comment.
two points here:
- Lets move the conditions to a formatter function. There we can check the conditions against ENUM and it will make it more testable and remove the use of strings (as in the XML).
- I am not super aware of it, but just wondering, cant we check the User Group role instead of name. We do have some specific ENUMs for the role right to identify the whether it is
Tenant_AdminorTenant_AuditorUser groups.
There was a problem hiding this comment.
- formatter added
- we need to check for name as it is not all tenant admins roles but the autogenerated groups with those names
There was a problem hiding this comment.
So does that mean there can be Tenant admin or Tenant auditor user groups which have the role "TENANT_ADMINISTRATOR" or "TENANT_AUDITOR" but will not have the relevant name. And those should be allowed to be edited?
| <m:Button icon="sap-icon://edit" text="{i18n>edit}" | ||
| type="Ghost" | ||
| visible="{= !${oneWay>/editMode} && ${roleBasedAccess>/userGroups/canManage} }" | ||
| enabled="{= ${oneWay>/groupData/name} !== 'TenantAdministrator' && ${oneWay>/groupData/name} !== 'TenantAuditor' }" |
There was a problem hiding this comment.
+1, request for formatter logic.
| const newIamIdentifier = event.getParameter('value') ?? ''; | ||
| const iamIdentifier = this.oneWayModel.getProperty('/groupData/iamIdentifier') as string; | ||
| if (newIamIdentifier === iamIdentifier) { | ||
| this.oneWayModel.setProperty('/groupValid', false); |
There was a problem hiding this comment.
nit: maybe we should think about renaming groupValid-> groupEdited or something. Name seems misleading as it controls the enable state of the SAVE groups button.
neilmcclelland
force-pushed
the
improvement/KMS20-3862-add-iam-identifier-edit
branch
from
2b39d95 to
80505fb
Compare
neilmcclelland
force-pushed
the
improvement/KMS20-3862-add-iam-identifier-edit
branch
from
80505fb to
c202690
Compare
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces protection for auto-generated tenant groups (TenantAdministrator and TenantAuditor) by preventing their deletion and editing. Additionally, the IAM identifier field is now editable through a live input binding, with controller validation to track changes during group editing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/controller/groups/Detail.controller.ts`:
- Around line 125-144: The current onGroupIamIdentifierChanged only toggles
/groupValid and sets /newIamIdentifier when different, but it leaves a stale
/newIamIdentifier when a user reverts to the original value and only tests IAM
to enable save — causing onGroupSavePress to include stale IAM values; update
onGroupIamIdentifierChanged to clear /newIamIdentifier (set to '') and set
/groupValid to false when newIamIdentifier ===
this.oneWayModel.getProperty('/groupData/iamIdentifier'), otherwise set
/newIamIdentifier and /groupValid true; additionally ensure any other change
handlers for name/description (the inputs that set /newGroupName and
/newGroupDescription) set /groupValid true when they differ from
this.oneWayModel.getProperty('/groupData/...') so onGroupSavePress only sends
fields that are actually changed and no stale IAM values remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c86c3aa-b726-41ba-b524-02e977cab188
📒 Files selected for processing (6)
webapp/common/Enums.tswebapp/common/Formatters.tswebapp/controller/groups/Detail.controller.tswebapp/i18n/i18n_en.propertieswebapp/resources/fragments/groups/UserGroupsTable.fragment.xmlwebapp/view/groups/GroupDetail.view.xml
| public onGroupIamIdentifierChanged(event: Input$LiveChangeEvent): void { | ||
| const newIamIdentifier = event.getParameter('value') ?? ''; | ||
| const iamIdentifier = this.oneWayModel.getProperty('/groupData/iamIdentifier') as string; | ||
| if (newIamIdentifier === iamIdentifier) { | ||
| this.oneWayModel.setProperty('/groupValid', false); | ||
| } | ||
| else { | ||
| this.oneWayModel.setProperty('/groupValid', true); | ||
| this.oneWayModel.setProperty('/newIamIdentifier', newIamIdentifier); | ||
| } | ||
| }; | ||
|
|
||
| public async onGroupSavePress(): Promise<void> { | ||
| this.getView()?.setBusy(true); | ||
|
|
||
| const payload: GroupPayload = { | ||
| name: this.oneWayModel.getProperty('/newGroupName') as string, | ||
| description: this.oneWayModel.getProperty('/newGroupDescription') as string | ||
| description: this.oneWayModel.getProperty('/newGroupDescription') as string, | ||
| IAMIdentifier: this.oneWayModel.getProperty('/newIamIdentifier') as string | ||
| }; |
There was a problem hiding this comment.
/groupValid and /newIamIdentifier handling can produce incorrect PATCH behavior.
At Line 129/132, save enablement is derived only from IAM field changes. At Line 133, reverted IAM values are not cleared. Combined with Line 143, stale IAM values can be sent when another field is edited.
Proposed fix
public onGroupIamIdentifierChanged(event: Input$LiveChangeEvent): void {
const newIamIdentifier = event.getParameter('value') ?? '';
const iamIdentifier = this.oneWayModel.getProperty('/groupData/iamIdentifier') as string;
- if (newIamIdentifier === iamIdentifier) {
- this.oneWayModel.setProperty('/groupValid', false);
- }
- else {
- this.oneWayModel.setProperty('/groupValid', true);
- this.oneWayModel.setProperty('/newIamIdentifier', newIamIdentifier);
- }
+ this.oneWayModel.setProperty(
+ '/newIamIdentifier',
+ newIamIdentifier === iamIdentifier ? undefined : newIamIdentifier
+ );
+
+ const currentName = this.oneWayModel.getProperty('/groupData/name') as string;
+ const currentDescription = this.oneWayModel.getProperty('/groupData/description') as string;
+ const hasNameChange =
+ ((this.oneWayModel.getProperty('/newGroupName') as string | undefined) ?? currentName) !== currentName;
+ const hasDescriptionChange =
+ ((this.oneWayModel.getProperty('/newGroupDescription') as string | undefined) ?? currentDescription) !== currentDescription;
+ const hasIamChange =
+ ((this.oneWayModel.getProperty('/newIamIdentifier') as string | undefined) ?? iamIdentifier) !== iamIdentifier;
+
+ this.oneWayModel.setProperty('/groupValid', hasNameChange || hasDescriptionChange || hasIamChange);
};-const payload: GroupPayload = {
- name: this.oneWayModel.getProperty('/newGroupName') as string,
- description: this.oneWayModel.getProperty('/newGroupDescription') as string,
- IAMIdentifier: this.oneWayModel.getProperty('/newIamIdentifier') as string
-};
+const payload: Partial<GroupPayload> = {};
+const newName = this.oneWayModel.getProperty('/newGroupName') as string | undefined;
+const newDescription = this.oneWayModel.getProperty('/newGroupDescription') as string | undefined;
+const newIamIdentifier = this.oneWayModel.getProperty('/newIamIdentifier') as string | undefined;
+if (newName !== undefined) payload.name = newName;
+if (newDescription !== undefined) payload.description = newDescription;
+if (newIamIdentifier !== undefined) payload.IAMIdentifier = newIamIdentifier;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webapp/controller/groups/Detail.controller.ts` around lines 125 - 144, The
current onGroupIamIdentifierChanged only toggles /groupValid and sets
/newIamIdentifier when different, but it leaves a stale /newIamIdentifier when a
user reverts to the original value and only tests IAM to enable save — causing
onGroupSavePress to include stale IAM values; update onGroupIamIdentifierChanged
to clear /newIamIdentifier (set to '') and set /groupValid to false when
newIamIdentifier === this.oneWayModel.getProperty('/groupData/iamIdentifier'),
otherwise set /newIamIdentifier and /groupValid true; additionally ensure any
other change handlers for name/description (the inputs that set /newGroupName
and /newGroupDescription) set /groupValid true when they differ from
this.oneWayModel.getProperty('/groupData/...') so onGroupSavePress only sends
fields that are actually changed and no stale IAM values remain.
spusala-sap
force-pushed
the
improvement/KMS20-3862-add-iam-identifier-edit
branch
from
c202690 to
a11a17b
Compare
| <Menu class="sapUiSizeCompact"> | ||
| <MenuItem press="onKeyTableDeletePress" text="{i18n>delete}" | ||
| icon="sap-icon://delete"> | ||
| icon="sap-icon://delete" |
There was a problem hiding this comment.
So does that mean there can be Tenant admin or Tenant auditor user groups which have the role "TENANT_ADMINISTRATOR" or "TENANT_AUDITOR" but will not have the relevant name. And those should be allowed to be edited?
| icon="sap-icon://delete"> | ||
| icon="sap-icon://delete" | ||
| enabled="{path: 'oneWay>name', formatter: 'commonFormatter.isGroupDeletable'}" | ||
| tooltip="{parts: ['oneWay>name', 'i18n>delete', 'i18n>autoGeneratedGroupCannotBeDeleted'], formatter: 'commonFormatter.getGroupDeleteTooltip'}"> |
There was a problem hiding this comment.
can you recheck if commonFormatter.getGroupDeleteTooltip is working fine? Because I don't see the tooltip. I HAVE switched the language to German, but anyhow it should fallback to English translations. But see no tool tip on the delete button
| <m:Button icon="sap-icon://edit" text="{i18n>edit}" | ||
| type="Ghost" | ||
| visible="{= !${oneWay>/editMode} && ${roleBasedAccess>/userGroups/canManage} }" | ||
| enabled="{= ${oneWay>/groupData/name} !== 'TenantAdministrator' && ${oneWay>/groupData/name} !== 'TenantAuditor' }" |
There was a problem hiding this comment.
+1, request for formatter logic.
| visible="{= !${oneWay>/editMode} && ${roleBasedAccess>/userGroups/canManage} }" | ||
| enabled="{= ${oneWay>/groupData/name} !== 'TenantAdministrator' && ${oneWay>/groupData/name} !== 'TenantAuditor' }" | ||
| tooltip="{= ${oneWay>/groupData/name} === 'TenantAdministrator' || ${oneWay>/groupData/name} === 'TenantAuditor' ? ${i18n>autoGeneratedGroupCannotBeEdited} : ${i18n>edit} }" | ||
| press="onGroupEditDetailsPress"> |
There was a problem hiding this comment.
same as above. I don't see the tooltip in the UI currently.
| TENANT_AUDITOR = 'TENANT_AUDITOR' | ||
| } | ||
|
|
||
| export enum ProtectedGroups { |
There was a problem hiding this comment.
nit:suggestion: Can we change the name to ProtectedGroupsName, so we differentiate to from roles.
3 participants
Fixes #22
Summary by CodeRabbit