Skip to content

[Port to dtq-dev] 117 better control of user deletion in user administration page #1335

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.

Sign up for GitHub

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

Open
kosarko wants to merge 1 commit into
base: dtq-dev
Choose a base branch
from
Open

[Port to dtq-dev] 117 better control of user deletion in user administration page #1335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/app/access-control/access-control.module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { SharedModule } from '../shared/shared.module';
import { AccessControlRoutingModule } from './access-control-routing.module';
import { EPeopleRegistryComponent } from './epeople-registry/epeople-registry.component';
import { EPersonFormComponent } from './epeople-registry/eperson-form/eperson-form.component';
import { EPersonDeleteGuardService } from './epeople-registry/eperson-delete-guard.service';
import { GroupFormComponent } from './group-registry/group-form/group-form.component';
import { MembersListComponent } from './group-registry/group-form/members-list/members-list.component';
import { SubgroupsListComponent } from './group-registry/group-form/subgroup-list/subgroups-list.component';
Expand Down Expand Up @@ -57,6 +58,7 @@ export const ValidateEmailErrorStateMatcher: DynamicErrorMessagesMatcher =
provide: DYNAMIC_ERROR_MESSAGES_MATCHER,
useValue: ValidateEmailErrorStateMatcher
},
EPersonDeleteGuardService,
]
})
/**
Expand Down
25 changes: 20 additions & 5 deletions src/app/access-control/epeople-registry/epeople-registry.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,26 @@ <h2 id="search" class="border-bottom pb-2">
title="{{labelPrefix + 'table.edit.buttons.edit' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
<i class="fas fa-edit fa-fw"></i>
</button>
<button *ngIf="epersonDto.ableToDelete" (click)="deleteEPerson(epersonDto.eperson)"
class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
title="{{labelPrefix + 'table.edit.buttons.remove' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
<i class="fas fa-trash-alt fa-fw"></i>
</button>
<ng-container *ngIf="epersonDto.ableToDelete && currentAuthenticatedUserId">
<ng-container *ngIf="isCurrentUser(epersonDto.eperson); else enabledDeleteButton">
<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>
Comment on lines +81 to +89

@coderabbitai coderabbitai Bot Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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-registry

Repository: 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]}")
PY

Repository: 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-registry

Repository: 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]}")
PY

Repository: 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-registry

Repository: 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.

</ng-container>
</ng-container>
<ng-template #enabledDeleteButton>
<button (click)="deleteEPerson(epersonDto.eperson)"
class="delete-button btn btn-outline-danger btn-sm access-control-deleteEPersonButton"
title="{{labelPrefix + 'table.edit.buttons.remove' | translate: { name: dsoNameService.getName(epersonDto.eperson) } }}">
<i class="fas fa-trash-alt fa-fw"></i>
</button>
</ng-template>
</div>
</td>
</tr>
Expand Down
176 changes: 164 additions & 12 deletions src/app/access-control/epeople-registry/epeople-registry.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,42 @@
import { Router } from '@angular/router';
import { Observable, of as observableOf } from 'rxjs';
import { Observable, of as observableOf, throwError as observableThrowError } from 'rxjs';
import { CommonModule } from '@angular/common';
import { DebugElement, NO_ERRORS_SCHEMA } from '@angular/core';
import { NO_ERRORS_SCHEMA } from '@angular/core';
import { ComponentFixture, fakeAsync, TestBed, tick, waitForAsync } from '@angular/core/testing';
import { FormsModule, ReactiveFormsModule } from '@angular/forms';
import { BrowserModule, By } from '@angular/platform-browser';
import { NgbModule } from '@ng-bootstrap/ng-bootstrap';
import { TranslateLoader, TranslateModule, TranslateService } from '@ngx-translate/core';
import { buildPaginatedList, PaginatedList } from '../../core/data/paginated-list.model';
import { RemoteData } from '../../core/data/remote-data';
import { AuthService } from '../../core/auth/auth.service';
import { EPersonDataService } from '../../core/eperson/eperson-data.service';
import { EPerson } from '../../core/eperson/models/eperson.model';
import { PageInfo } from '../../core/shared/page-info.model';
import { DSONameService } from '../../core/breadcrumbs/dso-name.service';
import { FormBuilderService } from '../../shared/form/builder/form-builder.service';
import { NotificationsService } from '../../shared/notifications/notifications.service';
import { EPeopleRegistryComponent } from './epeople-registry.component';
import { EPersonMock, EPersonMock2 } from '../../shared/testing/eperson.mock';
import { createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
import { createFailedRemoteDataObject$, createSuccessfulRemoteDataObject$ } from '../../shared/remote-data.utils';
import { getMockFormBuilderService } from '../../shared/mocks/form-builder-service.mock';
import { getMockTranslateService } from '../../shared/mocks/translate.service.mock';
import { TranslateLoaderMock } from '../../shared/mocks/translate-loader.mock';
import { NotificationsServiceStub } from '../../shared/testing/notifications-service.stub';
import { RouterStub } from '../../shared/testing/router.stub';
import { AuthorizationDataService } from '../../core/data/feature-authorization/authorization-data.service';
import { FeatureID } from '../../core/data/feature-authorization/feature-id';
import { EPersonDeleteGuardService } from './eperson-delete-guard.service';
import { RequestService } from '../../core/data/request.service';
import { PaginationService } from '../../core/pagination/pagination.service';
import { PaginationServiceStub } from '../../shared/testing/pagination-service.stub';
import { WorkspaceitemDataService } from '../../core/submission/workspaceitem-data.service';
import { WorkflowItemDataService } from '../../core/submission/workflowitem-data.service';
import { FindListOptions } from '../../core/data/find-list-options.model';
import {BtnDisabledDirective} from '../../shared/btn-disabled.directive';
import { SearchService } from '../../core/shared/search/search.service';
import { DSpaceObject } from '../../core/shared/dspace-object.model';
import { SearchObjects } from '../../shared/search/models/search-objects.model';
import { BtnDisabledDirective } from '../../shared/btn-disabled.directive';

describe('EPeopleRegistryComponent', () => {
let component: EPeopleRegistryComponent;
Expand All @@ -38,10 +47,35 @@ describe('EPeopleRegistryComponent', () => {
let mockEPeople;
let ePersonDataServiceStub: any;
let authorizationService: AuthorizationDataService;
let authService: jasmine.SpyObj<AuthService>;
let workspaceItemDataService: jasmine.SpyObj<WorkspaceitemDataService>;
let workflowItemDataService: jasmine.SpyObj<WorkflowItemDataService>;
let searchService: jasmine.SpyObj<SearchService>;
let notificationsService: NotificationsServiceStub;
let modalService;
let modalRef;

let paginationService;

const buildRemoteList = <T>(items: T[], totalElements = items.length) => createSuccessfulRemoteDataObject$(
buildPaginatedList(new PageInfo({
elementsPerPage: items.length || 1,
totalElements,
totalPages: 1,
currentPage: 1
}), items)
);

const buildSearchObjects = (totalElements: number) => Object.assign(
new SearchObjects<DSpaceObject>(),
buildPaginatedList(new PageInfo({
elementsPerPage: 1,
totalElements,
totalPages: 1,
currentPage: 1
}), [])
);

beforeEach(waitForAsync(() => {
jasmine.getEnv().allowRespy(true);
mockEPeople = [EPersonMock, EPersonMock2];
Expand Down Expand Up @@ -119,8 +153,17 @@ describe('EPeopleRegistryComponent', () => {
authorizationService = jasmine.createSpyObj('authorizationService', {
isAuthorized: observableOf(true)
});
authService = jasmine.createSpyObj('authService', ['getAuthenticatedUserFromStore']);
authService.getAuthenticatedUserFromStore.and.returnValue(observableOf(EPersonMock2));
workspaceItemDataService = jasmine.createSpyObj('workspaceItemDataService', ['searchBy']);
workspaceItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
workflowItemDataService = jasmine.createSpyObj('workflowItemDataService', ['searchBy']);
workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
searchService = jasmine.createSpyObj('searchService', ['search']);
searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
builderService = getMockFormBuilderService();
translateService = getMockTranslateService();
notificationsService = new NotificationsServiceStub();

paginationService = new PaginationServiceStub();
TestBed.configureTestingModule({
Expand All @@ -135,12 +178,20 @@ describe('EPeopleRegistryComponent', () => {
declarations: [EPeopleRegistryComponent, BtnDisabledDirective],
providers: [
{ provide: EPersonDataService, useValue: ePersonDataServiceStub },
{ provide: NotificationsService, useValue: new NotificationsServiceStub() },
{ provide: NotificationsService, useValue: notificationsService },
{ provide: AuthorizationDataService, useValue: authorizationService },
{ provide: AuthService, useValue: authService },
{ provide: FormBuilderService, useValue: builderService },
{ provide: WorkspaceitemDataService, useValue: workspaceItemDataService },
{ provide: WorkflowItemDataService, useValue: workflowItemDataService },
{ provide: SearchService, useValue: searchService },
EPersonDeleteGuardService,
{ provide: Router, useValue: new RouterStub() },
{ provide: RequestService, useValue: jasmine.createSpyObj('requestService', ['removeByHrefSubstring']) },
{ provide: PaginationService, useValue: paginationService }
{ provide: PaginationService, useValue: paginationService },
{ provide: DSONameService, useValue: jasmine.createSpyObj('dsoNameService', {
getName: (dso: any) => dso?.name ?? dso?.email ?? dso?.id,
}) },
],
schemas: [NO_ERRORS_SCHEMA]
}).compileComponents();
Expand All @@ -150,7 +201,8 @@ describe('EPeopleRegistryComponent', () => {
fixture = TestBed.createComponent(EPeopleRegistryComponent);
component = fixture.componentInstance;
modalService = (component as any).modalService;
spyOn(modalService, 'open').and.returnValue(Object.assign({ componentInstance: Object.assign({ response: observableOf(true) }) }));
modalRef = Object.assign({ componentInstance: Object.assign({ response: observableOf(true) }) });
spyOn(modalService, 'open').and.returnValue(modalRef);
fixture.detectChanges();
});

Expand Down Expand Up @@ -227,6 +279,109 @@ describe('EPeopleRegistryComponent', () => {
});
});
});

it('should render the self delete button as disabled', () => {
const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));

expect(deleteButtons.length).toBe(2);
expect(deleteButtons[0].nativeElement.getAttribute('aria-disabled')).toBeNull();
expect(deleteButtons[1].nativeElement.getAttribute('aria-disabled')).toBe('true');
expect(deleteButtons[1].nativeElement.classList.contains('disabled')).toBeTrue();
});

it('should call submitter checks and compose the combined warning label', fakeAsync(() => {
workspaceItemDataService.searchBy.and.returnValue(buildRemoteList([{} as any], 1));
workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
// isAuthorized returns true by default -> the target is treated as an administrator
modalRef.componentInstance.response = observableOf(false);

const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
deleteButtons[0].triggerEventHandler('click', null);
tick();

expect(workspaceItemDataService.searchBy).toHaveBeenCalledWith('findBySubmitter', jasmine.any(FindListOptions));
expect(workflowItemDataService.searchBy).toHaveBeenCalledWith('findBySubmitter', jasmine.any(FindListOptions));
expect(searchService.search).toHaveBeenCalled();
expect(authorizationService.isAuthorized).toHaveBeenCalledWith(FeatureID.AdministratorOf, undefined, EPersonMock.id);
expect(modalRef.componentInstance.warningLabel).toBe('admin.access-control.epeople.delete.warning.submitterAndAdmin');
}));

it('should detect administrator via the authorization feature', fakeAsync(() => {
workspaceItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
// admin -> true, all submitter probes empty -> admin-only warning
(authorizationService.isAuthorized as jasmine.Spy).and.callFake((featureId: FeatureID) => observableOf(featureId === FeatureID.AdministratorOf));
modalRef.componentInstance.response = observableOf(false);

const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
deleteButtons[0].triggerEventHandler('click', null);
tick();

expect(authorizationService.isAuthorized).toHaveBeenCalledWith(FeatureID.AdministratorOf, undefined, EPersonMock.id);
expect(modalRef.componentInstance.warningLabel).toBe('admin.access-control.epeople.delete.warning.admin');
}));

it('should still open the delete modal when a submitter probe errors (centralised catchError)', fakeAsync(() => {
workspaceItemDataService.searchBy.and.returnValue(observableThrowError(() => new Error('boom')));
workflowItemDataService.searchBy.and.returnValue(buildRemoteList([], 0));
searchService.search.and.returnValue(createSuccessfulRemoteDataObject$(buildSearchObjects(0)));
// CanDelete stays true so the button renders; AdministratorOf false so the only warning could come from submitter probes
(authorizationService.isAuthorized as jasmine.Spy).and.callFake((featureId: FeatureID) => observableOf(featureId !== FeatureID.AdministratorOf));
modalRef.componentInstance.response = observableOf(false);

const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
deleteButtons[0].triggerEventHandler('click', null);
tick();

expect(modalService.open).toHaveBeenCalled();
expect(modalRef.componentInstance.warningLabel).toBeUndefined();
}));

it('should show a friendly self-delete notification on backend 400 self-delete errors', fakeAsync(() => {
modalRef.componentInstance.response = observableOf(true);
ePersonDataServiceStub.deleteEPerson = jasmine.createSpy('deleteEPerson').and.returnValue(
createFailedRemoteDataObject$('You, as admin user, cannot delete yourself', 400)
);

const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
deleteButtons[0].triggerEventHandler('click', null);
tick();

expect(notificationsService.error).toHaveBeenCalled();
let translatedKey: string;
notificationsService.error.calls.mostRecent().args[0].subscribe((value) => translatedKey = value);
expect(translatedKey).toBe('admin.access-control.epeople.notification.deleted.forbidden.self');
}));

it('should use the deleted.failure key for generic delete failures', fakeAsync(() => {
modalRef.componentInstance.response = observableOf(true);
ePersonDataServiceStub.deleteEPerson = jasmine.createSpy('deleteEPerson').and.returnValue(
createFailedRemoteDataObject$('server error', 500)
);

const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
deleteButtons[0].triggerEventHandler('click', null);
tick();

expect(notificationsService.error).toHaveBeenCalled();
let translatedKey: string;
notificationsService.error.calls.mostRecent().args[0].subscribe((value) => translatedKey = value);
expect(translatedKey).toBe('admin.access-control.epeople.notification.deleted.failure');
}));

it('should not open delete modal before authenticated user id is resolved', fakeAsync(() => {
component.currentAuthenticatedUserId = undefined;
const deleteSpy = spyOn(ePersonDataServiceStub, 'deleteEPerson').and.callThrough();

const deleteButtons = fixture.debugElement.queryAll(By.css('.access-control-deleteEPersonButton'));
deleteButtons[0].triggerEventHandler('click', null);
tick();

expect(modalService.open).not.toHaveBeenCalled();
expect(deleteSpy).not.toHaveBeenCalled();
}));
});

describe('delete EPerson button when the isAuthorized returns false', () => {
Expand All @@ -237,12 +392,9 @@ describe('EPeopleRegistryComponent', () => {
fixture.detectChanges();
});

it('should be disabled', () => {
it('should be hidden', () => {
ePeopleDeleteButton = fixture.debugElement.queryAll(By.css('#epeople tr td div button.delete-button'));
ePeopleDeleteButton.forEach((deleteButton: DebugElement) => {
expect(deleteButton.nativeElement.getAttribute('aria-disabled')).toBe('true');
expect(deleteButton.nativeElement.classList.contains('disabled')).toBeTrue();
});
expect(ePeopleDeleteButton.length).toBe(0);
});
});
});
Loading
Loading