From aeeaf73507c28ef45de8773d0445bdc9eea3419c Mon Sep 17 00:00:00 2001
From: Afreen Misbah <afreen@ibm.com>
Date: Thu, 17 Apr 2025 21:05:39 +0530
Subject: [PATCH] mgr/dashboard: Fix pool update on edit

Issue: Pool field was blank on editing namespace form and user needs to type out pool
Reason: `image` field is no longer supported in form yet edit function trying to fetch it and failing on that, hence no following updates for pool
Fix: Removed stale `image` field

Additional changes:
- included unit tests for edit to capture such errors
- enhanced unit tests to sue ActivatedRouteStub and `router.url`
- pre populating pool form on create with first rbd pool in the list

Fixes https://tracker.ceph.com/issues/70966

Regression by: https://tracker.ceph.com/issues/69900

Signed-off-by: Afreen Misbah <afreen@ibm.com>
---
 .../nvmeof-namespaces-form.component.html     |   7 +-
 .../nvmeof-namespaces-form.component.spec.ts  | 138 +++++++++++++++---
 .../nvmeof-namespaces-form.component.ts       |   8 +-
 .../src/testing/activated-route-stub.ts       |  19 ++-
 4 files changed, 140 insertions(+), 32 deletions(-)

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.html b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.html
index f030e17dd94..bdd3830ffea 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.html
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.html
@@ -19,9 +19,10 @@
             <input *ngIf="edit"
                    class="form-control"
                    type="text"
+                   id="pool-edit"
                    formControlName="pool">
             <select *ngIf="!edit"
-                    id="pool"
+                    id="pool-create"
                     class="form-select"
                     formControlName="pool">
               <option *ngIf="rbdPools === null"
@@ -46,7 +47,8 @@
         </div>
         <!-- Namespace Count -->
         <div *ngIf="!edit"
-             class="form-group row">
+             class="form-group row"
+             id="namespace-count">
           <label class="cd-col-form-label"
                  for="nsCount">
             <span [ngClass]="{'required': !edit}"
@@ -93,6 +95,7 @@
                 <ng-container i18n>This field is required</ng-container>
               </span>
               <span class="invalid-feedback"
+                    id="image-size-invalid"
                     *ngIf="edit && invalidSizeError">
                 <ng-container i18n>Enter a value above than previous. A block device image can be expanded but not reduced.</ng-container>
               </span>
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.spec.ts
index 856fc19389d..5227f28dd03 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.spec.ts
@@ -16,32 +16,68 @@ import { NvmeofService } from '~/app/shared/api/nvmeof.service';
 import { of } from 'rxjs';
 import { PoolService } from '~/app/shared/api/pool.service';
 import { NumberModule } from 'carbon-components-angular';
+import { ActivatedRoute, Router } from '@angular/router';
+import { By } from '@angular/platform-browser';
+import { ActivatedRouteStub } from '~/testing/activated-route-stub';
 
-const mockPools = [
+const MOCK_POOLS = [
   Mocks.getPool('pool-1', 1, ['cephfs']),
   Mocks.getPool('rbd', 2),
   Mocks.getPool('pool-2', 3)
 ];
-class MockPoolService {
+class MockPoolsService {
   getList() {
-    return of(mockPools);
+    return of(MOCK_POOLS);
   }
 }
 
+const MOCK_NS_RESPONSE = {
+  nsid: 1,
+  uuid: '185d541f-76bf-45b5-b445-f71829346c38',
+  bdev_name: 'bdev_185d541f-76bf-45b5-b445-f71829346c38',
+  rbd_image_name: 'nvme_rbd_default_sscfagwuvvr',
+  rbd_pool_name: 'rbd',
+  load_balancing_group: 1,
+  rbd_image_size: '1073741824',
+  block_size: 512,
+  rw_ios_per_second: '0',
+  rw_mbytes_per_second: '0',
+  r_mbytes_per_second: '0',
+  w_mbytes_per_second: '0',
+  trash_image: false
+};
+
+const MOCK_ROUTER = {
+  editUrl:
+    'https://192.168.100.100:8443/#/block/nvmeof/subsystems/(modal:edit/nqn.2001-07.com.ceph:1744881547418.default/namespace/1)?group=default',
+  createUrl: 'https://192.168.100.100:8443/#/block/nvmeof/subsystems/(modal:create)?group=default'
+};
+
 describe('NvmeofNamespacesFormComponent', () => {
   let component: NvmeofNamespacesFormComponent;
   let fixture: ComponentFixture<NvmeofNamespacesFormComponent>;
   let nvmeofService: NvmeofService;
+  let router: Router;
   let form: CdFormGroup;
   let formHelper: FormHelper;
-  const mockRandomString = 1720693470789;
-  const mockSubsystemNQN = 'nqn.2021-11.com.example:subsystem';
-  const mockGWgroup = 'default';
+  let activatedRouteStub: ActivatedRouteStub;
+  const MOCK_RANDOM_STRING = 1720693470789;
+  const MOCK_SUBSYSTEM = 'nqn.2021-11.com.example:subsystem';
+  const MOCK_GROUP = 'default';
+  const MOCK_NSID = String(MOCK_NS_RESPONSE['nsid']);
 
   beforeEach(async () => {
+    activatedRouteStub = new ActivatedRouteStub(
+      { subsystem_nqn: MOCK_SUBSYSTEM, nsid: MOCK_NSID },
+      { group: MOCK_GROUP }
+    );
     await TestBed.configureTestingModule({
       declarations: [NvmeofNamespacesFormComponent],
-      providers: [NgbActiveModal, { provide: PoolService, useClass: MockPoolService }],
+      providers: [
+        NgbActiveModal,
+        { provide: PoolService, useClass: MockPoolsService },
+        { provide: ActivatedRoute, useValue: activatedRouteStub }
+      ],
       imports: [
         HttpClientTestingModule,
         NgbTypeaheadModule,
@@ -54,30 +90,36 @@ describe('NvmeofNamespacesFormComponent', () => {
     }).compileComponents();
     fixture = TestBed.createComponent(NvmeofNamespacesFormComponent);
     component = fixture.componentInstance;
-    component.ngOnInit();
-    form = component.nsForm;
-    formHelper = new FormHelper(form);
-    fixture.detectChanges();
   });
 
-  it('should create', () => {
+  it('should create component', () => {
     expect(component).toBeTruthy();
   });
-
-  describe('should test form', () => {
+  describe('should test create form', () => {
     beforeEach(() => {
-      component.subsystemNQN = mockSubsystemNQN;
-      component.group = mockGWgroup;
+      router = TestBed.inject(Router);
       nvmeofService = TestBed.inject(NvmeofService);
       spyOn(nvmeofService, 'createNamespace').and.stub();
-      spyOn(component, 'randomString').and.returnValue(mockRandomString);
+      spyOn(component, 'randomString').and.returnValue(MOCK_RANDOM_STRING);
+      Object.defineProperty(router, 'url', {
+        get: jasmine.createSpy('url').and.returnValue(MOCK_ROUTER.createUrl)
+      });
+      component.ngOnInit();
+      form = component.nsForm;
+      formHelper = new FormHelper(form);
+    });
+    it('should have set create fields correctly', () => {
+      expect(component.rbdPools.length).toBe(2);
+      fixture.detectChanges();
+      const poolEl = fixture.debugElement.query(By.css('#pool-create')).nativeElement;
+      expect(poolEl.value).toBe('rbd');
     });
     it('should create 5 namespaces correctly', () => {
       component.onSubmit();
       expect(nvmeofService.createNamespace).toHaveBeenCalledTimes(5);
-      expect(nvmeofService.createNamespace).toHaveBeenCalledWith(mockSubsystemNQN, {
-        gw_group: mockGWgroup,
-        rbd_image_name: `nvme_rbd_default_${mockRandomString}`,
+      expect(nvmeofService.createNamespace).toHaveBeenCalledWith(MOCK_SUBSYSTEM, {
+        gw_group: MOCK_GROUP,
+        rbd_image_name: `nvme_rbd_default_${MOCK_RANDOM_STRING}`,
         rbd_pool: 'rbd',
         rbd_image_size: 1073741824
       });
@@ -93,4 +135,60 @@ describe('NvmeofNamespacesFormComponent', () => {
       formHelper.expectError('image_size', 'min');
     });
   });
+  describe('should test edit form', () => {
+    beforeEach(() => {
+      router = TestBed.inject(Router);
+      nvmeofService = TestBed.inject(NvmeofService);
+      spyOn(nvmeofService, 'getNamespace').and.returnValue(of(MOCK_NS_RESPONSE));
+      spyOn(nvmeofService, 'updateNamespace').and.stub();
+      Object.defineProperty(router, 'url', {
+        get: jasmine.createSpy('url').and.returnValue(MOCK_ROUTER.editUrl)
+      });
+      fixture.detectChanges();
+    });
+    it('should have set edit fields correctly', () => {
+      expect(nvmeofService.getNamespace).toHaveBeenCalledTimes(1);
+      const poolEl = fixture.debugElement.query(By.css('#pool-edit')).nativeElement;
+      expect(poolEl.disabled).toBeTruthy();
+      expect(poolEl.value).toBe(MOCK_NS_RESPONSE['rbd_pool_name']);
+      const sizeEl = fixture.debugElement.query(By.css('#size')).nativeElement;
+      expect(sizeEl.value).toBe('1');
+      const unitEl = fixture.debugElement.query(By.css('#unit')).nativeElement;
+      expect(unitEl.value).toBe('GiB');
+    });
+    it('should not show namesapce count ', () => {
+      const nsCountEl = fixture.debugElement.query(By.css('#namespace-count'));
+      expect(nsCountEl).toBeFalsy();
+    });
+    it('should give error with no change in image size', () => {
+      component.onSubmit();
+      expect(component.invalidSizeError).toBe(true);
+      fixture.detectChanges();
+      const imageSizeInvalidEL = fixture.debugElement.query(By.css('#image-size-invalid'));
+      expect(imageSizeInvalidEL).toBeTruthy();
+    });
+    it('should give error when size less than previous (1 GB) provided', () => {
+      form = component.nsForm;
+      formHelper = new FormHelper(form);
+      formHelper.setValue('unit', 'MiB');
+      component.onSubmit();
+      expect(component.invalidSizeError).toBe(true);
+      fixture.detectChanges();
+      const imageSizeInvalidEL = fixture.debugElement.query(By.css('#image-size-invalid'))
+        .nativeElement;
+      expect(imageSizeInvalidEL).toBeTruthy();
+    });
+    it('should have edited namespace successfully', () => {
+      component.ngOnInit();
+      form = component.nsForm;
+      formHelper = new FormHelper(form);
+      formHelper.setValue('image_size', 2);
+      component.onSubmit();
+      expect(nvmeofService.updateNamespace).toHaveBeenCalledTimes(1);
+      expect(nvmeofService.updateNamespace).toHaveBeenCalledWith(MOCK_SUBSYSTEM, MOCK_NSID, {
+        gw_group: MOCK_GROUP,
+        rbd_image_size: 2147483648
+      });
+    });
+  });
 });
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.ts
index a3644836606..d2a136f3119 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/block/nvmeof-namespaces-form/nvmeof-namespaces-form.component.ts
@@ -87,12 +87,10 @@ export class NvmeofNamespacesFormComponent implements OnInit {
       .subscribe((res: NvmeofSubsystemNamespace) => {
         const convertedSize = this.dimlessBinaryPipe.transform(res.rbd_image_size).split(' ');
         this.currentBytes = res.rbd_image_size;
-        this.nsForm.get('image').setValue(res.rbd_image_name);
         this.nsForm.get('pool').setValue(res.rbd_pool_name);
         this.nsForm.get('unit').setValue(convertedSize[1]);
         this.nsForm.get('image_size').setValue(convertedSize[0]);
         this.nsForm.get('image_size').addValidators(Validators.required);
-        this.nsForm.get('image').disable();
         this.nsForm.get('pool').disable();
       });
   }
@@ -100,10 +98,10 @@ export class NvmeofNamespacesFormComponent implements OnInit {
   initForCreate() {
     this.poolService.getList().subscribe((resp: Pool[]) => {
       this.rbdPools = resp.filter(this.rbdService.isRBDPool);
+      if (this.rbdPools?.length) {
+        this.nsForm.get('pool').setValue(this.rbdPools[0].pool_name);
+      }
     });
-    if (this.rbdPools?.length) {
-      this.nsForm.get('pool').setValue(this.rbdPools[0].pool_name);
-    }
   }
 
   ngOnInit() {
diff --git a/src/pybind/mgr/dashboard/frontend/src/testing/activated-route-stub.ts b/src/pybind/mgr/dashboard/frontend/src/testing/activated-route-stub.ts
index e217838600c..a1556f16a50 100644
--- a/src/pybind/mgr/dashboard/frontend/src/testing/activated-route-stub.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/testing/activated-route-stub.ts
@@ -3,24 +3,33 @@ import { ActivatedRoute } from '@angular/router';
 import { ReplaySubject } from 'rxjs';
 
 /**
- * An ActivateRoute test double with a `params` observable.
+ * An ActivateRoute test double with a `params` and `queryParams` observable.
  * Use the `setParams()` method to add the next `params` value.
+ * Use the `setQueryParams()` method to add the next `params` value.
  */
 export class ActivatedRouteStub extends ActivatedRoute {
   // Use a ReplaySubject to share previous values with subscribers
   // and pump new values into the `params` observable
-  private subject = new ReplaySubject<object>();
+  private paramSubject = new ReplaySubject<object>();
+  private queryParamSubject = new ReplaySubject<object>();
 
-  constructor(initialParams?: object) {
+  constructor(initialParams?: object, initialQueryParams?: object) {
     super();
     this.setParams(initialParams);
+    this.setQueryParams(initialQueryParams);
   }
 
   /** The mock params observable */
-  readonly params = this.subject.asObservable();
+  readonly params = this.paramSubject.asObservable();
+  /** The mock queryParams observable */
+  readonly queryParams = this.queryParamSubject.asObservable();
 
   /** Set the params observables's next value */
   setParams(params?: object) {
-    this.subject.next(params);
+    this.paramSubject.next(params);
+  }
+  /** Set the queryParams observables's next value */
+  setQueryParams(queryParams?: object) {
+    this.queryParamSubject.next(queryParams);
   }
 }
-- 
2.39.5