[Port to dtq-dev] 117 better control of user deletion in user administration page#1335
Open
kosarko wants to merge 1 commit into
Open
[Port to dtq-dev] 117 better control of user deletion in user administration page#1335kosarko wants to merge 1 commit into
kosarko wants to merge 1 commit into
Conversation
* Disable self-delete and add delete warnings Prevent administrators from deleting their own account by disabling the delete button for the current authenticated user and showing a tooltip. Add a warningLabel input to the confirmation modal and render it when present. Implement logic in EPeopleRegistryComponent to determine the current user, compute a contextual delete warning (submitter, admin, or both) by querying workspace/workflow submissions, search, and group membership, and open the confirmation modal with that warning. Handle delete responses to show a friendly notification for backend 400 self-delete errors and use a generic failure notification otherwise. Update templates, component imports and helper methods (isCurrentUser, getDeleteWarningLabel, hasSubmittedItems, isAdministrator, isSelfDeletionError, showSelfDeleteNotification), and extend unit tests to cover disabled self-delete UI, composed warning labels, and notification behavior. * Prevent self-delete and show delete warnings Disable direct self-deletion and surface contextual warnings when deleting an EPerson. Template: render the delete button as disabled for the currently authenticated user and show a tooltip explaining self-delete restrictions; otherwise show the normal delete button. Component: track the current authenticated user, compute a combined warning label based on whether the EPerson is an administrator and/or has submitted items (checks workspace, workflow and archived submissions), present that warning in the confirmation modal, and handle deletion results including a friendly notification for backend self-delete errors. Added helper methods (isCurrentUser, getDeleteWarningLabel, hasSubmittedItems, isAdministrator, isSelfDeletionError, showSelfDeleteNotification) and adjusted delete flow to update canDelete$ appropriately. Tests: updated and added specs and mocks to cover the disabled self-delete UI, combined warning label usage, and the friendly self-delete error notification; added required service mocks and test setup changes. * Add EPerson delete warnings and self-delete message Add four i18n keys to en.json5 and cs.json5 for EPerson deletion flows: a forbidden self-delete message and warnings for deleting users who are submitters, administrators, or both. Provides localized English and Czech strings to surface these messages in the admin access-control UI. * Guard EPerson delete until auth ID resolved Prevent delete actions from running before the current authenticated user id is available. Add a template condition to hide the delete button until currentAuthenticatedUserId is set, add an early-return guard in deleteEPerson when the id is missing, and add a unit test ensuring the modal is not opened and delete is not called before the id is resolved. * Hide delete button until auth user ID resolved Prevent delete actions before the current authenticated user ID is available. Add *ngIf to the delete button in the template, add an early return guard in the component's delete flow when currentAuthenticatedUserId is not set, and include unit tests verifying the button is hidden and the delete/modal are not invoked until the ID is resolved. * Restrict delete button visibility and update tests Template: Wrap self-delete tooltip/button in a guard that checks epersonDto.ableToDelete and currentAuthenticatedUserId so delete UI is only rendered when deletion is allowed; simplify the enabledDeleteButton markup by removing a duplicated *ngIf on the inner button. Spec: remove unused DebugElement import, rename the test from 'should be disabled' to 'should be hidden', and update assertions to expect no delete buttons to be present (reflecting the new visibility behavior). * Detect Administrator group across paginated pages Add support for detecting Administrator group membership across paginated group lists and a unit test for it. - Introduce hasAdministratorGroupOnPage(groupsHref, currentPage) which fetches group pages (elementsPerPage=100) and recursively checks subsequent pages when the Administrator group is not found on the current page. - isAdministrator now delegates to hasAdministratorGroupOnPage starting at page 1. - Preserve error handling to return false on failures. - Add a unit test that simulates a two-page group response (administrator present on the second page) and verifies the component detects administrator membership and displays the correct warning label. * Detect Administrator group across pages Refactor group membership check to traverse paginated group lists until an "Administrator" group is found or pages are exhausted. Introduces a recursive hasAdministratorGroupOnPage that requests pages, validates payload/pageInfo, checks for the admin group, and requests the next page when needed. Adds a unit test covering detection on later pages and a minor spec setup tweak (setting currentAuthenticatedUserId) to correctly exercise deletion behavior. * Fix Czech typo in delete warning Corrects a spelling mistake in src/assets/i18n/cs.json5 for key admin.access-control.epeople.delete.warning.submitter: changed 'repositáři' to 'repozitáři' to improve Czech translation accuracy. * Make disabled delete button unfocusable Add tabindex="-1" to the disabled delete button in the epeople registry template so it cannot receive keyboard focus. This ensures the parent span (which provides the tooltip) remains the sole focus target for the current-user case, improving accessibility and preventing duplicate focusable elements. * Make disabled self-delete button unfocusable Add tabindex="-1" to the disabled delete button in eperson-form.component.html so the button cannot receive keyboard focus when showing the current user. The tooltip remains on the parent span, improving keyboard navigation and accessibility for the self-delete warning. * Remove stray 'after' attribute from eperson template Remove an unintended "after" attribute from a <span> in eperson-form.component.html. The change cleans up the template by leaving only the intended *ngIf binding for conditional rendering of the delete/impersonation UI, preventing potential template parsing or linting issues. * code cleanup - remove duplicities * cleanup * copilot comments * copilot's comment --------- Co-authored-by: Ondrej Kosarko <kosarko@ufal.mff.cuni.cz> (cherry picked from commit d0b5ecf)
📝 WalkthroughWalkthroughIntroduces ChangesEPerson Self-Delete Guard Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 3
🧹 Nitpick comments (1)
src/app/access-control/epeople-registry/epeople-registry.component.ts (1)
245-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a doc comment for
isCurrentUser().This new public method is missing TypeDoc/JSDoc.
As per coding guidelines,**/*.ts: All new/modified public methods and classes require TypeDoc/JSDoc comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/access-control/epeople-registry/epeople-registry.component.ts` around lines 245 - 247, Add a TypeDoc/JSDoc comment for the public isCurrentUser(ePerson: EPerson) method in EpeopleRegistryComponent. Briefly describe that it checks whether the provided EPerson matches the currently authenticated user, and keep the comment aligned with the existing public API documentation style used in this component.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/access-control/epeople-registry/epeople-registry.component.html`:
- Around line 81-89: The focusable self-delete wrapper is missing the accessible
name and disabled semantics because the `tabindex="0"` span is the real keyboard
target while the nested button is removed from tab order. Update the wrapper in
the epeople registry template so the translated label and disabled state are
applied to the outer element instead of the inner button, and make the same
change in the matching self-delete control in eperson-form so both use the
wrapper as the accessible target.
In
`@src/app/access-control/epeople-registry/eperson-form/eperson-form.component.ts`:
- Around line 553-555: Add a JSDoc/TypeDoc comment for the public isCurrentUser
method in EpersonFormComponent. Document that it checks whether the given
EPerson matches the currently authenticated user, and keep the comment directly
above isCurrentUser so the new public API is documented per the coding
guidelines.
- Around line 515-525: Restore the authorization-backed canDelete$ stream in
eperson-form.component by avoiding permanent replacement with constant
observableOf(true/false) values inside the delete flow. Update the logic around
the delete/confirm handling so the original isAuthorized(FeatureID.CanDelete,
...) observable is preserved or re-created after the modal closes, and only use
temporary disabled/enabled state during the action. Ensure the finalize block
and the delete pipeline in the component keep canDelete$ tied to authorization
rather than leaving it as a static observable.
---
Nitpick comments:
In `@src/app/access-control/epeople-registry/epeople-registry.component.ts`:
- Around line 245-247: Add a TypeDoc/JSDoc comment for the public
isCurrentUser(ePerson: EPerson) method in EpeopleRegistryComponent. Briefly
describe that it checks whether the provided EPerson matches the currently
authenticated user, and keep the comment aligned with the existing public API
documentation style used in this component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dcd07e2-6997-4719-9fc7-11bb7962d6fc
📒 Files selected for processing (14)
src/app/access-control/access-control.module.tssrc/app/access-control/epeople-registry/epeople-registry.component.htmlsrc/app/access-control/epeople-registry/epeople-registry.component.spec.tssrc/app/access-control/epeople-registry/epeople-registry.component.tssrc/app/access-control/epeople-registry/eperson-delete-guard.service.spec.tssrc/app/access-control/epeople-registry/eperson-delete-guard.service.tssrc/app/access-control/epeople-registry/eperson-form/eperson-form.component.htmlsrc/app/access-control/epeople-registry/eperson-form/eperson-form.component.spec.tssrc/app/access-control/epeople-registry/eperson-form/eperson-form.component.tssrc/app/shared/confirmation-modal/confirmation-modal.component.htmlsrc/app/shared/confirmation-modal/confirmation-modal.component.spec.tssrc/app/shared/confirmation-modal/confirmation-modal.component.tssrc/assets/i18n/cs.json5src/assets/i18n/en.json5
Comment on lines
+81
to
+89
| <span tabindex="0" [ngbTooltip]="selfDeleteWarningLabel | translate"> | ||
| <button [dsBtnDisabled]="true" | ||
| tabindex="-1" | ||
| [attr.aria-label]="selfDeleteWarningLabel | translate" | ||
| class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton" | ||
| type="button"> | ||
| <i class="fas fa-trash-alt fa-fw"></i> | ||
| </button> | ||
| </span> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== epeople-registry.component.html slice ==\n'
sed -n '70,100p' src/app/access-control/epeople-registry/epeople-registry.component.html
printf '\n== eperson-form.component.html slice ==\n'
sed -n '1,140p' src/app/access-control/epeople-registry/eperson-form/eperson-form.component.html
printf '\n== search for similar wrapper pattern ==\n'
rg -n --no-heading 'tabindex="0"|ngbTooltip|dsBtnDisabled="true"|aria-label=.*selfDeleteWarningLabel' src/app/access-control/epeople-registryRepository: dataquest-dev/dspace-angular
Length of output: 7821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path("src/app/access-control/epeople-registry/epeople-registry.component.html"),
Path("src/app/access-control/epeople-registry/eperson-form/eperson-form.component.html"),
]
for p in paths:
print(f"\n## {p}")
text = p.read_text(encoding="utf-8")
for i, line in enumerate(text.splitlines(), 1):
if "tabindex=\"0\"" in line or "ngbTooltip" in line or "dsBtnDisabled=\"true\"" in line or "selfDeleteWarningLabel" in line or "delete-button" in line:
start = max(1, i - 3)
end = min(len(text.splitlines()), i + 4)
print(f"-- lines {start}-{end}")
for j in range(start, end + 1):
print(f"{j:4}: {text.splitlines()[j-1]}")
PYRepository: dataquest-dev/dspace-angular
Length of output: 4918
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --no-heading 'selfDeleteWarningLabel|currentAuthenticatedUserId|isCurrentUser|tabindex="0"|ngbTooltip|dsBtnDisabled="true"' src/app/access-control/epeople-registryRepository: dataquest-dev/dspace-angular
Length of output: 4805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether the wrapper is intended to be keyboard-focusable tooltip trigger,
# and whether there is any existing accessibility pattern in nearby templates.
python3 - <<'PY'
from pathlib import Path
for rel in [
"src/app/access-control/epeople-registry/epeople-registry.component.html",
"src/app/access-control/epeople-registry/eperson-form/eperson-form.component.html",
]:
p = Path(rel)
if not p.exists():
print(rel, "MISSING")
continue
lines = p.read_text(encoding="utf-8").splitlines()
print(f"\nFILE: {rel}")
for idx, line in enumerate(lines, 1):
if any(tok in line for tok in ["ngbTooltip", "tabindex=\"0\"", "aria-label", "aria-disabled", "dsBtnDisabled", "delete-button", "selfDeleteWarningLabel"]):
a = max(1, idx - 4)
b = min(len(lines), idx + 4)
print(f"\n--- around line {idx} ---")
for j in range(a, b + 1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: dataquest-dev/dspace-angular
Length of output: 8383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,100p' src/app/access-control/epeople-registry/epeople-registry.component.html
printf '\n---\n'
sed -n '1,140p' src/app/access-control/epeople-registry/eperson-form/eperson-form.component.html
printf '\n---\n'
rg -n --no-heading 'selfDeleteWarningLabel|currentAuthenticatedUserId|isCurrentUser|tabindex="0"|ngbTooltip|dsBtnDisabled="true"|aria-label|aria-disabled' src/app/access-control/epeople-registryRepository: dataquest-dev/dspace-angular
Length of output: 12995
Label the focusable self-delete wrapper. The tabindex="0" <span> is the actual keyboard target, but it has no accessible name or disabled state; the translated label is only on the nested button, which is removed from the tab order. Move the label/disabled semantics to the wrapper here and in src/app/access-control/epeople-registry/eperson-form/eperson-form.component.html.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/access-control/epeople-registry/epeople-registry.component.html`
around lines 81 - 89, The focusable self-delete wrapper is missing the
accessible name and disabled semantics because the `tabindex="0"` span is the
real keyboard target while the nested button is removed from tab order. Update
the wrapper in the epeople registry template so the translated label and
disabled state are applied to the outer element instead of the inner button, and
make the same change in the matching self-delete control in eperson-form so both
use the wrapper as the accessible target.
Comment on lines
+515
to
+525
| this.canDelete$ = observableOf(false); | ||
| return this.epersonService.deleteEPerson(eperson).pipe( | ||
| getFirstCompletedRemoteData(), | ||
| map((restResponse: RemoteData<NoContent>) => ({ restResponse, eperson, attempted: true })) | ||
| ); | ||
| } | ||
|
|
||
| return observableOf({ restResponse: null, eperson, attempted: false }); | ||
| }), | ||
| finalize(() => this.canDelete$ = observableOf(true)) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the authorization-backed canDelete$ stream after the modal closes.
Lines 515-524 replace canDelete$ with constant observables. After the first cancel/confirm cycle, the original isAuthorized(FeatureID.CanDelete, ...) check is gone, so the delete UI can stay enabled even for states that were not authorized.
💡 Minimal fix
+ private createCanDeleteStream(): Observable<boolean> {
+ return this.activeEPerson$.pipe(
+ switchMap((eperson) => this.authorizationService.isAuthorized(FeatureID.CanDelete, hasValue(eperson) ? eperson.self : undefined))
+ );
+ }
+
initialisePage() {
@@
- this.canDelete$ = this.activeEPerson$.pipe(
- switchMap((eperson) => this.authorizationService.isAuthorized(FeatureID.CanDelete, hasValue(eperson) ? eperson.self : undefined))
- );
+ this.canDelete$ = this.createCanDeleteStream();
@@
- finalize(() => this.canDelete$ = observableOf(true))
+ finalize(() => this.canDelete$ = this.createCanDeleteStream())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| this.canDelete$ = observableOf(false); | |
| return this.epersonService.deleteEPerson(eperson).pipe( | |
| getFirstCompletedRemoteData(), | |
| map((restResponse: RemoteData<NoContent>) => ({ restResponse, eperson, attempted: true })) | |
| ); | |
| } | |
| return observableOf({ restResponse: null, eperson, attempted: false }); | |
| }), | |
| finalize(() => this.canDelete$ = observableOf(true)) | |
| ); | |
| this.canDelete$ = observableOf(false); | |
| return this.epersonService.deleteEPerson(eperson).pipe( | |
| getFirstCompletedRemoteData(), | |
| map((restResponse: RemoteData<NoContent>) => ({ restResponse, eperson, attempted: true })) | |
| ); | |
| } | |
| return observableOf({ restResponse: null, eperson, attempted: false }); | |
| }), | |
| finalize(() => this.canDelete$ = this.createCanDeleteStream()) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/access-control/epeople-registry/eperson-form/eperson-form.component.ts`
around lines 515 - 525, Restore the authorization-backed canDelete$ stream in
eperson-form.component by avoiding permanent replacement with constant
observableOf(true/false) values inside the delete flow. Update the logic around
the delete/confirm handling so the original isAuthorized(FeatureID.CanDelete,
...) observable is preserved or re-created after the modal closes, and only use
temporary disabled/enabled state during the action. Ensure the finalize block
and the delete pipeline in the component keep canDelete$ tied to authorization
rather than leaving it as a static observable.
Comment on lines
+553
to
+555
| isCurrentUser(ePerson: EPerson): boolean { | ||
| return this.deleteGuard.isCurrentUser(ePerson, this.currentAuthenticatedUserId); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add JSDoc for isCurrentUser.
This new public method is undocumented. As per coding guidelines, **/*.ts: All new/modified public methods and classes require TypeDoc/JSDoc comments.
📝 Suggested comment
+ /**
+ * Whether the given EPerson is the currently authenticated user.
+ */
isCurrentUser(ePerson: EPerson): boolean {
return this.deleteGuard.isCurrentUser(ePerson, this.currentAuthenticatedUserId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| isCurrentUser(ePerson: EPerson): boolean { | |
| return this.deleteGuard.isCurrentUser(ePerson, this.currentAuthenticatedUserId); | |
| } | |
| /** | |
| * Whether the given EPerson is the currently authenticated user. | |
| */ | |
| isCurrentUser(ePerson: EPerson): boolean { | |
| return this.deleteGuard.isCurrentUser(ePerson, this.currentAuthenticatedUserId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/access-control/epeople-registry/eperson-form/eperson-form.component.ts`
around lines 553 - 555, Add a JSDoc/TypeDoc comment for the public isCurrentUser
method in EpersonFormComponent. Document that it checks whether the given
EPerson matches the currently authenticated user, and keep the comment directly
above isCurrentUser so the new public API is documented per the coding
guidelines.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of ufal#140 by @amadulhaxxani to
dtq-dev.Summary by CodeRabbit
New Features
Bug Fixes
Documentation