From e0e467a7a2011c991e487ab848750b59d79adc3f Mon Sep 17 00:00:00 2001
From: Afreen Misbah <afreen23.git@gmail.com>
Date: Wed, 11 Sep 2024 21:33:31 +0530
Subject: [PATCH] mgr/dashboard: Adding group and pool name to service name

- Pre-populating the service name field with the format `nvmeof.<pool_name>.<group_name>`.
- This can be changed by user but by default this value will be there.
- This will help user to quickly fill form and proceed hence improving usability.
- cephadm also uses this format as of now this convention so it will make UI aligned with CLI experience

- updates unit tests to improve coverage

- hides`count` values as that is not needed for 'nvmeof' only hosts and labels required.

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

Signed-off-by: Afreen Misbah <afreen23.git@gmail.com>
---
 .../service-form/service-form.component.html  |   2 +-
 .../service-form.component.spec.ts            | 101 +++++++++++++++---
 .../service-form/service-form.component.ts    |  39 ++++---
 .../frontend/src/testing/unit-test-helper.ts  |   3 +-
 4 files changed, 116 insertions(+), 29 deletions(-)

diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html
index 4e31c133f78..641d25a8dc2 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.html
@@ -305,7 +305,7 @@
         </div>
 
         <!-- Count -->
-        <div *ngIf="!serviceForm.controls.unmanaged.value"
+        <div *ngIf="!serviceForm.controls.unmanaged.value && serviceForm.controls.service_type.value !== 'nvmeof'"
              class="form-group row">
           <label class="cd-col-form-label"
                  for="count">
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.spec.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.spec.ts
index 7d3a57fad23..0e931d485d0 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.spec.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.spec.ts
@@ -13,8 +13,21 @@ import { CephServiceService } from '~/app/shared/api/ceph-service.service';
 import { PaginateObservable } from '~/app/shared/api/paginate.model';
 import { CdFormGroup } from '~/app/shared/forms/cd-form-group';
 import { SharedModule } from '~/app/shared/shared.module';
-import { configureTestBed, FormHelper } from '~/testing/unit-test-helper';
+import { configureTestBed, FormHelper, Mocks } from '~/testing/unit-test-helper';
 import { ServiceFormComponent } from './service-form.component';
+import { PoolService } from '~/app/shared/api/pool.service';
+
+// for 'nvmeof' service
+const mockPools = [
+  Mocks.getPool('pool-1', 1, ['cephfs']),
+  Mocks.getPool('rbd', 2),
+  Mocks.getPool('pool-2', 3)
+];
+class MockPoolService {
+  getList() {
+    return of(mockPools);
+  }
+}
 
 describe('ServiceFormComponent', () => {
   let component: ServiceFormComponent;
@@ -25,7 +38,7 @@ describe('ServiceFormComponent', () => {
 
   configureTestBed({
     declarations: [ServiceFormComponent],
-    providers: [NgbActiveModal],
+    providers: [NgbActiveModal, { provide: PoolService, useClass: MockPoolService }],
     imports: [
       HttpClientTestingModule,
       NgbTypeaheadModule,
@@ -389,22 +402,54 @@ x4Ea7kGVgx9kWh5XjWz9wjZvY49UKIT5ppIAWPMbLl3UpfckiuNhTA==
 
     describe('should test service nvmeof', () => {
       beforeEach(() => {
+        component.serviceType = 'nvmeof';
         formHelper.setValue('service_type', 'nvmeof');
-        formHelper.setValue('service_id', 'svc');
-        formHelper.setValue('pool', 'xyz');
-        formHelper.setValue('group', 'abc');
+        component.ngOnInit();
+        fixture.detectChanges();
       });
 
-      it('should submit nvmeof', () => {
-        component.onSubmit();
-        expect(cephServiceService.create).toHaveBeenCalledWith({
-          service_type: 'nvmeof',
-          service_id: 'svc',
-          placement: {},
-          unmanaged: false,
-          pool: 'xyz',
-          group: 'abc'
-        });
+      it('should set rbd pools correctly onInit', () => {
+        expect(component.pools.length).toBe(3);
+        expect(component.rbdPools.length).toBe(2);
+      });
+
+      it('should set default values correctly onInit', () => {
+        expect(form.get('service_type').value).toBe('nvmeof');
+        expect(form.get('group').value).toBe('default');
+        expect(form.get('pool').value).toBe('rbd');
+        expect(form.get('service_id').value).toBe('rbd.default');
+      });
+
+      it('should reflect correct values on group change', () => {
+        // Initially the group value should be 'default'
+        expect(component.serviceForm.get('group')?.value).toBe('default');
+        const groupInput = fixture.debugElement.query(By.css('#group')).nativeElement;
+        // Simulate input value change
+        groupInput.value = 'foo';
+        // Trigger the input event
+        groupInput.dispatchEvent(new Event('input'));
+        // Trigger the change event
+        groupInput.dispatchEvent(new Event('change'));
+        fixture.detectChanges();
+        // Verify values after change
+        expect(form.get('group').value).toBe('foo');
+        expect(form.get('service_id').value).toBe('rbd.foo');
+      });
+
+      it('should reflect correct values on pool change', () => {
+        // Initially the pool value should be 'rbd'
+        expect(component.serviceForm.get('pool')?.value).toBe('rbd');
+        const poolInput = fixture.debugElement.query(By.css('#pool')).nativeElement;
+        // Simulate input value change
+        poolInput.value = 'pool-2';
+        // Trigger the input event
+        poolInput.dispatchEvent(new Event('input'));
+        // Trigger the change event
+        poolInput.dispatchEvent(new Event('change'));
+        fixture.detectChanges();
+        // Verify values after change
+        expect(form.get('pool').value).toBe('pool-2');
+        expect(form.get('service_id').value).toBe('pool-2.default');
       });
 
       it('should throw error when there is no service id', () => {
@@ -418,6 +463,23 @@ x4Ea7kGVgx9kWh5XjWz9wjZvY49UKIT5ppIAWPMbLl3UpfckiuNhTA==
       it('should throw error when there is no group', () => {
         formHelper.expectErrorChange('group', '', 'required');
       });
+
+      it('should hide the count element when service_type is "nvmeof"', () => {
+        const countEl = fixture.debugElement.query(By.css('#count'));
+        expect(countEl).toBeNull();
+      });
+
+      it('should submit nvmeof', () => {
+        component.onSubmit();
+        expect(cephServiceService.create).toHaveBeenCalledWith({
+          service_type: 'nvmeof',
+          service_id: 'rbd.default',
+          placement: {},
+          unmanaged: false,
+          pool: 'rbd',
+          group: 'default'
+        });
+      });
     });
 
     describe('should test service smb', () => {
@@ -650,6 +712,15 @@ x4Ea7kGVgx9kWh5XjWz9wjZvY49UKIT5ppIAWPMbLl3UpfckiuNhTA==
         const poolId = fixture.debugElement.query(By.css('#pool')).nativeElement;
         expect(poolId.disabled).toBeTruthy();
       });
+
+      it('should not edit groups for nvmeof service', () => {
+        component.serviceType = 'nvmeof';
+        formHelper.setValue('service_type', 'nvmeof');
+        component.ngOnInit();
+        fixture.detectChanges();
+        const groupId = fixture.debugElement.query(By.css('#group')).nativeElement;
+        expect(groupId.disabled).toBeTruthy();
+      });
     });
   });
 });
diff --git a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts
index ac4578cbf22..27477a845f7 100644
--- a/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/services/service-form/service-form.component.ts
@@ -212,7 +212,7 @@ export class ServiceFormComponent extends CdForm implements OnInit {
         ]
       ],
       group: [
-        null,
+        'default',
         CdValidators.requiredIf({
           service_type: 'nvmeof'
         })
@@ -855,7 +855,6 @@ export class ServiceFormComponent extends CdForm implements OnInit {
         this.serviceForm.get('count').setValue(2);
         break;
       case 'iscsi':
-      case 'nvmeof':
       case 'cephfs-mirror':
       case 'nfs':
       case 'grafana':
@@ -962,16 +961,29 @@ export class ServiceFormComponent extends CdForm implements OnInit {
     );
   }
 
-  setNvmeofServiceId(): void {
-    const defaultRbdPool: string = this.rbdPools?.find((p: Pool) => p.pool_name === 'rbd')
-      ?.pool_name;
-    if (defaultRbdPool) {
-      this.serviceForm.get('pool').setValue(defaultRbdPool);
-    }
+  onNvmeofGroupChange(groupName: string) {
+    const pool = this.serviceForm.get('pool').value;
+    if (pool) this.serviceForm.get('service_id').setValue(`${pool}.${groupName}`);
+    else this.serviceForm.get('service_id').setValue(groupName);
   }
 
-  onNvmeofGroupChange(groupName: string) {
-    this.serviceForm.get('service_id').setValue(groupName);
+  getDefaultBlockPool(): string {
+    // returns 'rbd' pool otherwise the first block pool
+    return (
+      this.rbdPools?.find((p: Pool) => p.pool_name === 'rbd')?.pool_name ||
+      this.rbdPools?.[0].pool_name
+    );
+  }
+
+  setNvmeofServiceIdAndPool(): void {
+    const defaultBlockPool: string = this.getDefaultBlockPool();
+    const group: string = this.serviceForm.get('group').value;
+    if (defaultBlockPool && group) {
+      this.serviceForm.get('pool').setValue(defaultBlockPool);
+      this.serviceForm.get('service_id').setValue(`${defaultBlockPool}.${group}`);
+    } else {
+      this.serviceForm.get('service_id').setValue(null);
+    }
   }
 
   requiresServiceId(serviceType: string) {
@@ -981,7 +993,7 @@ export class ServiceFormComponent extends CdForm implements OnInit {
   setServiceId(serviceId: string): void {
     const requiresServiceId: boolean = this.requiresServiceId(serviceId);
     if (requiresServiceId && serviceId === 'nvmeof') {
-      this.setNvmeofServiceId();
+      this.setNvmeofServiceIdAndPool();
     } else if (requiresServiceId) {
       this.serviceForm.get('service_id').setValue(null);
     } else {
@@ -1019,7 +1031,10 @@ export class ServiceFormComponent extends CdForm implements OnInit {
 
   onBlockPoolChange() {
     const selectedBlockPool = this.serviceForm.get('pool').value;
-    if (selectedBlockPool) {
+    const group = this.serviceForm.get('group').value;
+    if (selectedBlockPool && group) {
+      this.serviceForm.get('service_id').setValue(`${selectedBlockPool}.${group}`);
+    } else if (selectedBlockPool) {
       this.serviceForm.get('service_id').setValue(selectedBlockPool);
     } else {
       this.serviceForm.get('service_id').setValue(null);
diff --git a/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts b/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts
index ac1e9e5ea25..21e94e9996c 100644
--- a/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts
+++ b/src/pybind/mgr/dashboard/frontend/src/testing/unit-test-helper.ts
@@ -423,9 +423,10 @@ export class Mocks {
     return { name, type, type_id, id, children, device_class };
   }
 
-  static getPool = (name: string, id: number): Pool => {
+  static getPool = (name: string, id: number, application_metadata: string[] = ['rbd']): Pool => {
     return _.merge(new Pool(name), {
       pool: id,
+      application_metadata,
       type: 'replicated',
       pg_num: 256,
       pg_placement_num: 256,
-- 
2.39.5