Merge pull request #24823 from Devp00l/wip-issue-36615

[RFC] mgr/dashboard: Cleanup of OSD list methods

Reviewed-by: Alfonso Martínez <almartin@redhat.com>
Reviewed-by: Patrick Nawracay <pnawracay@suse.com>
Reviewed-by: Tiago Melo <tmelo@suse.com>

src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-list/osd-list.component.spec.ts

@@ -35,6 +35,36 @@ describe('OsdListComponent', () => {
     }
   };
 
+  const getTableAction = (name) => component.tableActions.find((action) => action.name === name);
+
+  const setFakeSelection = () => {
+    // Default data and selection
+    const selection = [{ id: 1 }];
+    const data = [{ id: 1 }];
+
+    // Table data and selection
+    component.selection = new CdTableSelection();
+    component.selection.selected = selection;
+    component.selection.update();
+    component.osds = data;
+  };
+
+  const openActionModal = (actionName) => {
+    setFakeSelection();
+    getTableAction(actionName).click();
+  };
+
+  /**
+   * The following modals are called after the information about their
+   * safety to destroy/remove/mark them lost has been retrieved, hence
+   * we will have to fake its request to be able to open those modals.
+   */
+  const mockSafeToDestroy = () => {
+    spyOn(TestBed.get(OsdService), 'safeToDestroy').and.callFake(() =>
+      of({ 'safe-to-destroy': true })
+    );
+  };
+
   configureTestBed({
     imports: [
       HttpClientTestingModule,
@@ -59,18 +89,6 @@ describe('OsdListComponent', () => {
     modalServiceShowSpy = spyOn(TestBed.get(BsModalService), 'show').and.stub();
   });
 
-  const setFakeSelection = () => {
-    // Default data and selection
-    const selection = [{ id: 1 }];
-    const data = [{ id: 1 }];
-
-    // Table data and selection
-    component.selection = new CdTableSelection();
-    component.selection.selected = selection;
-    component.selection.update();
-    component.osds = data;
-  };
-
   it('should create', () => {
     fixture.detectChanges();
     expect(component).toBeTruthy();
@@ -142,72 +160,70 @@ describe('OsdListComponent', () => {
   describe('tests if all modals are opened correctly', () => {
     /**
      * Helper function to check if a function opens a modal
-     * @param fn
+     *
      * @param modalClass - The expected class of the modal
      */
-    const expectOpensModal = (fn, modalClass): void => {
-      setFakeSelection();
-      fn();
+    const expectOpensModal = (actionName: string, modalClass): void => {
+      openActionModal(actionName);
 
       expect(modalServiceShowSpy.calls.any()).toBe(true, 'modalService.show called');
-      expect(modalServiceShowSpy.calls.first()).toBeTruthy();
       expect(modalServiceShowSpy.calls.first().args[0]).toBe(modalClass);
 
       modalServiceShowSpy.calls.reset();
     };
 
-    it('opens the appropriate modal', () => {
-      expectOpensModal(() => component.reweight(), OsdReweightModalComponent);
-      expectOpensModal(() => component.markOut(), ConfirmationModalComponent);
-      expectOpensModal(() => component.markIn(), ConfirmationModalComponent);
-      expectOpensModal(() => component.markDown(), ConfirmationModalComponent);
-
-      // The following modals are called after the information about their
-      // safety to destroy/remove/mark them lost has been retrieved, hence
-      // we will have to fake its request to be able to open those modals.
-      spyOn(TestBed.get(OsdService), 'safeToDestroy').and.callFake(() =>
-        of({ 'safe-to-destroy': true })
-      );
+    it('opens the reweight modal', () => {
+      expectOpensModal('Reweight', OsdReweightModalComponent);
+    });
 
-      expectOpensModal(() => component.markLost(), CriticalConfirmationModalComponent);
-      expectOpensModal(() => component.remove(), CriticalConfirmationModalComponent);
-      expectOpensModal(() => component.destroy(), CriticalConfirmationModalComponent);
+    it('opens all confirmation modals', () => {
+      const modalClass = ConfirmationModalComponent;
+      expectOpensModal('Mark Out', modalClass);
+      expectOpensModal('Mark In', modalClass);
+      expectOpensModal('Mark Down', modalClass);
+    });
+
+    it('opens all critical confirmation modals', () => {
+      const modalClass = CriticalConfirmationModalComponent;
+      mockSafeToDestroy();
+      expectOpensModal('Mark Lost', modalClass);
+      expectOpensModal('Remove', modalClass);
+      expectOpensModal('Destroy', modalClass);
     });
   });
 
   describe('tests if the correct methods are called on confirmation', () => {
-    const expectOsdServiceMethodCalled = (fn: Function, osdServiceMethodName: string): void => {
-      setFakeSelection();
+    const expectOsdServiceMethodCalled = (
+      actionName: string,
+      osdServiceMethodName: string
+    ): void => {
       const osdServiceSpy = spyOn(TestBed.get(OsdService), osdServiceMethodName).and.callFake(
         () => EMPTY
       );
-
-      modalServiceShowSpy.calls.reset();
-      fn(); // calls show on BsModalService
-      // Calls onSubmit given to `bsModalService.show()`
+      openActionModal(actionName);
       const initialState = modalServiceShowSpy.calls.first().args[1].initialState;
-      const action = initialState.onSubmit || initialState.submitAction;
-      action.call(component);
+      const submit = initialState.onSubmit || initialState.submitAction;
+      submit.call(component);
 
       expect(osdServiceSpy.calls.count()).toBe(1);
       expect(osdServiceSpy.calls.first().args[0]).toBe(1);
-      modalServiceShowSpy.calls.reset();
+
+      // Reset spies to be able to recreate them
       osdServiceSpy.calls.reset();
+      modalServiceShowSpy.calls.reset();
     };
 
-    it('calls the corresponding service methods', () => {
-      // Purposely `reweight`
-      expectOsdServiceMethodCalled(() => component.markOut(), 'markOut');
-      expectOsdServiceMethodCalled(() => component.markIn(), 'markIn');
-      expectOsdServiceMethodCalled(() => component.markDown(), 'markDown');
-
-      spyOn(TestBed.get(OsdService), 'safeToDestroy').and.callFake(() =>
-        of({ 'safe-to-destroy': true })
-      );
+    it('calls the corresponding service methods in confirmation modals', () => {
+      expectOsdServiceMethodCalled('Mark Out', 'markOut');
+      expectOsdServiceMethodCalled('Mark In', 'markIn');
+      expectOsdServiceMethodCalled('Mark Down', 'markDown');
+    });
 
-      expectOsdServiceMethodCalled(() => component.markLost(), 'markLost');
-      expectOsdServiceMethodCalled(() => component.remove(), 'remove');
-      expectOsdServiceMethodCalled(() => component.destroy(), 'destroy');
+    it('calls the corresponding service methods in critical confirmation modals', () => {
+      mockSafeToDestroy();
+      expectOsdServiceMethodCalled('Mark Lost', 'markLost');
+      expectOsdServiceMethodCalled('Remove', 'remove');
+      expectOsdServiceMethodCalled('Destroy', 'destroy');
     });
   });
 });

src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/osd/osd-list/osd-list.component.ts

@@ -83,42 +83,55 @@ export class OsdListComponent implements OnInit {
       {
         name: 'Mark Out',
         permission: 'update',
-        click: () => this.markOut(),
+        click: () => this.showConfirmationModal('out', this.osdService.markOut),
         disable: () => this.isNotSelectedOrInState('out'),
         icon: 'fa-arrow-left'
       },
       {
         name: 'Mark In',
         permission: 'update',
-        click: () => this.markIn(),
+        click: () => this.showConfirmationModal('in', this.osdService.markIn),
         disable: () => this.isNotSelectedOrInState('in'),
         icon: 'fa-arrow-right'
       },
       {
         name: 'Mark Down',
         permission: 'update',
-        click: () => this.markDown(),
+        click: () => this.showConfirmationModal('down', this.osdService.markDown),
         disable: () => this.isNotSelectedOrInState('down'),
         icon: 'fa-arrow-down'
       },
       {
-        name: 'Mark lost',
+        name: 'Mark Lost',
         permission: 'delete',
-        click: () => this.markLost(),
+        click: () =>
+          this.showCriticalConfirmationModal(
+            'Mark',
+            'OSD lost',
+            'marked lost',
+            this.osdService.markLost
+          ),
         disable: () => this.isNotSelectedOrInState('up'),
         icon: 'fa-unlink'
       },
       {
         name: 'Remove',
         permission: 'delete',
-        click: () => this.remove(),
+        click: () =>
+          this.showCriticalConfirmationModal('Remove', 'OSD', 'removed', this.osdService.remove),
         disable: () => this.isNotSelectedOrInState('up'),
         icon: 'fa-remove'
       },
       {
         name: 'Destroy',
         permission: 'delete',
-        click: () => this.destroy(),
+        click: () =>
+          this.showCriticalConfirmationModal(
+            'destroy',
+            'OSD',
+            'destroyed',
+            this.osdService.destroy
+          ),
         disable: () => this.isNotSelectedOrInState('up'),
         icon: 'fa-eraser'
       }
@@ -238,18 +251,6 @@ export class OsdListComponent implements OnInit {
     });
   }
 
-  markOut() {
-    this.showConfirmationModal('out', this.osdService.markOut);
-  }
-
-  markIn() {
-    this.showConfirmationModal('in', this.osdService.markIn);
-  }
-
-  markDown() {
-    this.showConfirmationModal('down', this.osdService.markDown);
-  }
-
   reweight() {
     const selectedOsd = this.osds.filter((o) => o.id === this.selection.first().id).pop();
     this.modalService.show(OsdReweightModalComponent, {
@@ -285,16 +286,4 @@ export class OsdListComponent implements OnInit {
       });
     });
   }
-
-  markLost() {
-    this.showCriticalConfirmationModal('Mark', 'OSD lost', 'marked lost', this.osdService.markLost);
-  }
-
-  remove() {
-    this.showCriticalConfirmationModal('Remove', 'OSD', 'removed', this.osdService.remove);
-  }
-
-  destroy() {
-    this.showCriticalConfirmationModal('destroy', 'OSD', 'destroyed', this.osdService.destroy);
-  }
 }