From 29e8c29adec9814dc1aa18f329343f57ac6b262a Mon Sep 17 00:00:00 2001
From: Jukka Lipka <3710455+jlipka@users.noreply.github.com>
Date: Fri, 23 May 2025 17:20:31 +0200
Subject: [PATCH] fix: confirm dialog for group deletion

Introduced a confirmation modal before deleting groups in both the Group Registry and Comcol Role components. This ensures users explicitly confirm deletion, reducing accidental data loss. Updated relevant tests and added new translations for modal text.

(cherry picked from commit 57bf254bec793800ba6c8d1ccd00b5513b5e528a)
---
 .../groups-registry.component.html            |  2 +-
 .../groups-registry.component.spec.ts         |  2 +
 .../groups-registry.component.ts              | 34 +++++++++++++-
 .../comcol-role/comcol-role.component.html    |  2 +-
 .../comcol-role/comcol-role.component.spec.ts | 44 +++++++++++++++----
 .../comcol-role/comcol-role.component.ts      | 32 ++++++++++++++
 src/assets/i18n/en.json5                      | 16 +++++++
 7 files changed, 121 insertions(+), 11 deletions(-)

diff --git a/src/app/access-control/group-registry/groups-registry.component.html b/src/app/access-control/group-registry/groups-registry.component.html
index 510a156476..2c6c2590e2 100644
--- a/src/app/access-control/group-registry/groups-registry.component.html
+++ b/src/app/access-control/group-registry/groups-registry.component.html
@@ -85,7 +85,7 @@
                         }
                         @if (!groupDto.group?.permanent && groupDto.ableToDelete) {
                           <button
-                            (click)="deleteGroup(groupDto)" class="btn btn-outline-danger btn-sm btn-delete"
+                            (click)="confirmDelete(groupDto)" class="btn btn-outline-danger btn-sm btn-delete"
                             title="{{messagePrefix + 'table.edit.buttons.remove' | translate: {name: dsoNameService.getName(groupDto.group) } }}">
                             <i class="fas fa-trash-alt fa-fw"></i>
                           </button>
diff --git a/src/app/access-control/group-registry/groups-registry.component.spec.ts b/src/app/access-control/group-registry/groups-registry.component.spec.ts
index 81af022055..673d650723 100644
--- a/src/app/access-control/group-registry/groups-registry.component.spec.ts
+++ b/src/app/access-control/group-registry/groups-registry.component.spec.ts
@@ -381,6 +381,8 @@ describe('GroupsRegistryComponent', () => {
       deleteButton.click();
       fixture.detectChanges();
 
+      (document as any).querySelector('.modal-footer .confirm').click();
+
       expect(groupsDataServiceStub.delete).toHaveBeenCalledWith(mockGroups[0].id);
     });
   });
diff --git a/src/app/access-control/group-registry/groups-registry.component.ts b/src/app/access-control/group-registry/groups-registry.component.ts
index 9a7924f210..e846c5bbb9 100644
--- a/src/app/access-control/group-registry/groups-registry.component.ts
+++ b/src/app/access-control/group-registry/groups-registry.component.ts
@@ -9,7 +9,10 @@ import {
   UntypedFormBuilder,
 } from '@angular/forms';
 import { RouterLink } from '@angular/router';
-import { NgbTooltipModule } from '@ng-bootstrap/ng-bootstrap';
+import {
+  NgbModal,
+  NgbTooltipModule,
+} from '@ng-bootstrap/ng-bootstrap';
 import {
   TranslateModule,
   TranslateService,
@@ -27,6 +30,7 @@ import {
   defaultIfEmpty,
   map,
   switchMap,
+  takeUntil,
   tap,
 } from 'rxjs/operators';
 
@@ -57,6 +61,7 @@ import {
 } from '../../core/shared/operators';
 import { PageInfo } from '../../core/shared/page-info.model';
 import { BtnDisabledDirective } from '../../shared/btn-disabled.directive';
+import { ConfirmationModalComponent } from '../../shared/confirmation-modal/confirmation-modal.component';
 import { hasValue } from '../../shared/empty.util';
 import { ThemedLoadingComponent } from '../../shared/loading/themed-loading.component';
 import { NotificationsService } from '../../shared/notifications/notifications.service';
@@ -142,6 +147,7 @@ export class GroupsRegistryComponent implements OnInit, OnDestroy {
               private paginationService: PaginationService,
               public requestService: RequestService,
               public dsoNameService: DSONameService,
+              private modalService: NgbModal,
   ) {
     this.currentSearchQuery = '';
     this.searchForm = this.formBuilder.group(({
@@ -314,4 +320,30 @@ export class GroupsRegistryComponent implements OnInit, OnDestroy {
     this.paginationService.clearPagination(this.config.id);
   }
 
+  confirmDelete(group: GroupDtoModel): void {
+    const modalRef = this.modalService.open(ConfirmationModalComponent);
+    modalRef.componentInstance.name = this.dsoNameService.getName(group.group);
+    modalRef.componentInstance.headerLabel = 'admin.access-control.epeople.table.edit.buttons.remove.modal.header';
+    modalRef.componentInstance.infoLabel = 'admin.access-control.epeople.table.edit.buttons.remove.modal.info';
+    modalRef.componentInstance.cancelLabel = 'admin.access-control.epeople.table.edit.buttons.remove.modal.cancel';
+    modalRef.componentInstance.confirmLabel = 'admin.access-control.epeople.table.edit.buttons.remove.modal.confirm';
+    modalRef.componentInstance.brandColor = 'danger';
+    modalRef.componentInstance.confirmIcon = 'fas fa-trash';
+
+    const modalSub: Subscription = modalRef.componentInstance.response.pipe(
+      takeUntil(modalRef.closed),
+    ).subscribe((result: boolean) => {
+      if (result === true) {
+        this.deleteGroup(group);
+      }
+    });
+
+    void modalRef.result.then().finally(() => {
+      modalRef.close();
+      if (modalSub && !modalSub.closed) {
+        modalSub.unsubscribe();
+      }
+    });
+  }
+
 }
diff --git a/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.html b/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.html
index b136f83dc5..0570691479 100644
--- a/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.html
+++ b/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.html
@@ -51,7 +51,7 @@
         @if (hasCustomGroup$ | async) {
           <button
             class="btn btn-danger delete"
-            (click)="delete()">
+            (click)="confirmDelete(dsoNameService.getName(group))">
             <i class="fas fa-trash" aria-hidden="true"></i> {{'comcol-role.edit.delete' | translate}}
           </button>
         }
diff --git a/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.spec.ts b/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.spec.ts
index 2972cf1656..ff5435c7a1 100644
--- a/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.spec.ts
+++ b/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.spec.ts
@@ -10,6 +10,7 @@ import {
 import { By } from '@angular/platform-browser';
 import { NoopAnimationsModule } from '@angular/platform-browser/animations';
 import { RouterTestingModule } from '@angular/router/testing';
+import { NgbModule } from '@ng-bootstrap/ng-bootstrap';
 import { TranslateModule } from '@ngx-translate/core';
 import { of } from 'rxjs';
 
@@ -36,17 +37,22 @@ describe('ComcolRoleComponent', () => {
   let comcolRole;
   let notificationsService;
 
-  const requestService = { hasByHref$: () => of(true) };
+  const requestService = {
+    hasByHref$: () => of(true),
+    setStaleByHrefSubstring: () => of(true),
+  };
 
   const groupService = {
     findByHref: jasmine.createSpy('findByHref'),
     createComcolGroup: jasmine.createSpy('createComcolGroup').and.returnValue(of({})),
     deleteComcolGroup: jasmine.createSpy('deleteComcolGroup').and.returnValue(of({})),
+    clearGroupsRequests: jasmine.createSpy('clearGroupsRequests'),
   };
 
   beforeEach(waitForAsync(() => {
     TestBed.configureTestingModule({
       imports: [
+        NgbModule,
         RouterTestingModule.withRoutes([]),
         TranslateModule.forRoot(),
         NoopAnimationsModule,
@@ -174,17 +180,18 @@ describe('ComcolRoleComponent', () => {
         name: 'custom group name',
       };
       statusCode = 200;
-      comp.comcolRole = comcolRole;
+      comp.comcolRole = {
+        name: 'test role name' + Math.random(),
+        href: 'test role link',
+      };
+      comp.roleName$ = of(comcolRole.name);
       fixture.detectChanges();
     });
 
     it('should have a delete button but no create or restrict button', (done) => {
-      expect(de.query(By.css('.btn.create')))
-        .toBeNull();
-      expect(de.query(By.css('.btn.restrict')))
-        .toBeNull();
-      expect(de.query(By.css('.btn.delete')))
-        .toBeTruthy();
+      expect(de.query(By.css('.btn.create'))).toBeNull();
+      expect(de.query(By.css('.btn.restrict'))).toBeNull();
+      expect(de.query(By.css('.btn.delete'))).toBeTruthy();
       done();
     });
 
@@ -194,7 +201,16 @@ describe('ComcolRoleComponent', () => {
         de.query(By.css('.btn.delete')).nativeElement.click();
       });
 
+      afterEach(() => {
+        const modal = document.querySelector('ds-confirmation-modal');
+        if (modal) {
+          modal.remove();
+        }
+      });
+
       it('should call the groupService delete method', (done) => {
+        (document as any).querySelector('.modal-footer .confirm').click();
+        fixture.detectChanges();
         expect(groupService.deleteComcolGroup).toHaveBeenCalled();
         done();
       });
@@ -204,12 +220,24 @@ describe('ComcolRoleComponent', () => {
       beforeEach(() => {
         groupService.deleteComcolGroup.and.returnValue(createFailedRemoteDataObject$());
         de.query(By.css('.btn.delete')).nativeElement.click();
+        fixture.detectChanges();
+      });
+
+      afterEach(() => {
+        const modal = document.querySelector('ds-confirmation-modal');
+        if (modal) {
+          modal.remove();
+        }
       });
 
       it('should show an error notification', (done) => {
+        (document as any).querySelector('.modal-footer .confirm').click();
+        fixture.detectChanges();
+
         expect(notificationsService.error).toHaveBeenCalled();
         done();
       });
     });
+
   });
 });
diff --git a/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.ts b/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.ts
index c0ddbd6cc2..0519760019 100644
--- a/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.ts
+++ b/src/app/shared/comcol/comcol-forms/edit-comcol-page/comcol-role/comcol-role.component.ts
@@ -5,6 +5,7 @@ import {
   OnInit,
 } from '@angular/core';
 import { RouterLink } from '@angular/router';
+import { NgbModal } from '@ng-bootstrap/ng-bootstrap';
 import {
   TranslateModule,
   TranslateService,
@@ -12,11 +13,13 @@ import {
 import {
   BehaviorSubject,
   Observable,
+  Subscription,
 } from 'rxjs';
 import {
   filter,
   map,
   switchMap,
+  takeUntil,
 } from 'rxjs/operators';
 
 import { getGroupEditRoute } from '../../../../../access-control/access-control-routing-paths';
@@ -34,6 +37,7 @@ import {
   getFirstCompletedRemoteData,
 } from '../../../../../core/shared/operators';
 import { AlertComponent } from '../../../../alert/alert.component';
+import { ConfirmationModalComponent } from '../../../../confirmation-modal/confirmation-modal.component';
 import {
   hasNoValue,
   hasValue,
@@ -115,6 +119,7 @@ export class ComcolRoleComponent implements OnInit {
     protected notificationsService: NotificationsService,
     protected translateService: TranslateService,
     public dsoNameService: DSONameService,
+    private modalService: NgbModal,
   ) {
   }
 
@@ -215,4 +220,31 @@ export class ComcolRoleComponent implements OnInit {
 
     this.roleName$ = this.translateService.get(`comcol-role.edit.${this.comcolRole.name}.name`);
   }
+
+  confirmDelete(groupName: string): void {
+    const modalRef = this.modalService.open(ConfirmationModalComponent);
+
+    modalRef.componentInstance.name = groupName;
+    modalRef.componentInstance.headerLabel = 'comcol-role.edit.delete.modal.header';
+    modalRef.componentInstance.infoLabel = 'comcol-role.edit.delete.modal.info';
+    modalRef.componentInstance.cancelLabel = 'comcol-role.edit.delete.modal.cancel';
+    modalRef.componentInstance.confirmLabel = 'comcol-role.edit.delete.modal.confirm';
+    modalRef.componentInstance.brandColor = 'danger';
+    modalRef.componentInstance.confirmIcon = 'fas fa-trash';
+
+    const modalSub: Subscription = modalRef.componentInstance.response.pipe(
+      takeUntil(modalRef.closed),
+    ).subscribe((result: boolean) => {
+      if (result === true) {
+        this.delete();
+      }
+    });
+
+    void modalRef.result.then().finally(() => {
+      modalRef.close();
+      if (modalSub && !modalSub.closed) {
+        modalSub.unsubscribe();
+      }
+    });
+  }
 }
diff --git a/src/assets/i18n/en.json5 b/src/assets/i18n/en.json5
index c262850745..8488b8f373 100644
--- a/src/assets/i18n/en.json5
+++ b/src/assets/i18n/en.json5
@@ -305,6 +305,14 @@
 
   "admin.access-control.epeople.table.edit.buttons.remove": "Delete \"{{name}}\"",
 
+  "admin.access-control.epeople.table.edit.buttons.remove.modal.header": "Delete Group \"{{ dsoName }}\"",
+
+  "admin.access-control.epeople.table.edit.buttons.remove.modal.info": "Are you sure you want to delete Group \"{{ dsoName }}\" and all its associated policies?",
+
+  "admin.access-control.epeople.table.edit.buttons.remove.modal.cancel": "Cancel",
+
+  "admin.access-control.epeople.table.edit.buttons.remove.modal.confirm": "Delete",
+
   "admin.access-control.epeople.no-items": "No EPeople to show.",
 
   "admin.access-control.epeople.form.create": "Create EPerson",
@@ -1523,6 +1531,14 @@
 
   "comcol-role.edit.delete.error.title": "Failed to delete the '{{ role }}' role's group",
 
+  "comcol-role.edit.delete.modal.header": "Delete Group \"{{ dsoName }}\"",
+
+  "comcol-role.edit.delete.modal.info": "Are you sure you want to delete Group \"{{ dsoName }}\" and all its associated policies?",
+
+  "comcol-role.edit.delete.modal.cancel": "Cancel",
+
+  "comcol-role.edit.delete.modal.confirm": "Delete",
+
   "comcol-role.edit.community-admin.name": "Administrators",
 
   "comcol-role.edit.collection-admin.name": "Administrators",