fix: do not throw exception if content provider id exists (#7633) (#7662

) * fix: do not throw exception if content provider id exists instead it will overwrite existing content provider --------- (cherry picked from commit f08dbdf) Signed-off-by: Yulong Ruan <ruanyl@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
opensearch-project · Aug 9, 2024 · 3c90c37 · 3c90c37
1 parent 5b1e0f0
commit 3c90c37
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 25 deletions.
diff --git a/changelogs/fragments/7633.yml b/changelogs/fragments/7633.yml
@@ -0,0 +1,2 @@
+feat:
+- Register section and content with the same id will not throw error but overrides the exist one ([#7633](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7633))
diff --git a/src/plugins/content_management/public/components/section_input.test.ts b/src/plugins/content_management/public/components/section_input.test.ts
@@ -246,7 +246,7 @@ test('it should create section with a dashboard as content', async () => {
       id: 'dashboard-id-static',
       attributes: {
         panelsJSON:
-          '[{"version":"3.0.0","gridData":{"x":0,"y":0,"w":48,"h":5,"i":"debc95ec-7d43-49ee-84c8-95ad7b0b03ea"},"panelIndex":"debc95ec-7d43-49ee-84c8-95ad7b0b03ea","embeddableConfig":{"hidePanelTitles":true},"panelRefName":"panel_0"}]',
+          '[{"version":"3.0.0","gridData":{"x":0,"y":0,"w":48,"h":5,"i":"i"},"panelIndex":"1","embeddableConfig":{"hidePanelTitles":true},"panelRefName":"panel_0"}]',
       },
       references: [
         { id: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2', name: 'panel_0', type: 'visualization' },
@@ -258,14 +258,14 @@ test('it should create section with a dashboard as content', async () => {
     savedObjectsClient: clientMock,
   });
   expect(input.panels).toEqual({
-    'debc95ec-7d43-49ee-84c8-95ad7b0b03ea': {
+    'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2': {
       explicitInput: {
-        id: 'debc95ec-7d43-49ee-84c8-95ad7b0b03ea',
+        id: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2',
         savedObjectId: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2',
       },
       gridData: {
         h: 5,
-        i: 'debc95ec-7d43-49ee-84c8-95ad7b0b03ea',
+        i: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2',
         w: 48,
         x: 0,
         y: 0,
@@ -292,7 +292,7 @@ test('it should create section with a dynamic dashboard as content', async () =>
       id: 'dashboard-id-static',
       attributes: {
         panelsJSON:
-          '[{"version":"3.0.0","gridData":{"x":0,"y":0,"w":48,"h":5,"i":"debc95ec-7d43-49ee-84c8-95ad7b0b03ea"},"panelIndex":"debc95ec-7d43-49ee-84c8-95ad7b0b03ea","embeddableConfig":{"hidePanelTitles":true},"panelRefName":"panel_0"}]',
+          '[{"version":"3.0.0","gridData":{"x":0,"y":0,"w":48,"h":5,"i":"1"},"panelIndex":"1","embeddableConfig":{"hidePanelTitles":true},"panelRefName":"panel_0"}]',
       },
       references: [
         { id: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2', name: 'panel_0', type: 'visualization' },
@@ -304,14 +304,14 @@ test('it should create section with a dynamic dashboard as content', async () =>
     savedObjectsClient: clientMock,
   });
   expect(input.panels).toEqual({
-    'debc95ec-7d43-49ee-84c8-95ad7b0b03ea': {
+    'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2': {
       explicitInput: {
-        id: 'debc95ec-7d43-49ee-84c8-95ad7b0b03ea',
+        id: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2',
         savedObjectId: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2',
       },
       gridData: {
         h: 5,
-        i: 'debc95ec-7d43-49ee-84c8-95ad7b0b03ea',
+        i: 'ce24dd10-eb8a-11ed-8e00-17d7d50cd7b2',
         w: 48,
         x: 0,
         y: 0,

diff --git a/src/plugins/content_management/public/components/section_input.ts b/src/plugins/content_management/public/components/section_input.ts
@@ -103,11 +103,11 @@ export const createDashboardInput = async (
                 }
                 const reference = references.find((ref) => ref.name === panel.panelRefName);
                 if (reference) {
-                  panels[panel.panelIndex] = {
-                    gridData: panel.gridData,
+                  panels[reference.id] = {
+                    gridData: { ...panel.gridData, i: reference.id },
                     type: reference.type,
                     explicitInput: {
-                      id: panel.panelIndex,
+                      id: reference.id,
                       savedObjectId: reference.id,
                     },
                   };

diff --git a/...ugins/content_management/public/services/content_management/content_management_service.ts b/...ugins/content_management/public/services/content_management/content_management_service.ts
@@ -33,10 +33,6 @@ export class ContentManagementService {
   };
 
   registerContentProvider = (provider: ContentProvider) => {
-    if (this.contentProviders.get(provider.id)) {
-      throw new Error(`Cannot register content provider with the same id ${provider.id}`);
-    }
-
     this.contentProviders.set(provider.id, provider);
 
     const targetArea = provider.getTargetArea();

diff --git a/src/plugins/content_management/public/services/content_management/page.test.ts b/src/plugins/content_management/public/services/content_management/page.test.ts
@@ -17,12 +17,12 @@ test('it should create sections', () => {
   expect(page.getSections()).toHaveLength(2);
 });
 
-test('it should not create section with existing id', () => {
+test('creating section with the same id should override the previous section', () => {
   const page = new Page({ id: 'page1' });
   page.createSection({ id: 'section1', kind: 'dashboard', order: 2000 });
-  expect(() =>
-    page.createSection({ id: 'section1', kind: 'dashboard', order: 1000 })
-  ).toThrowError();
+  page.createSection({ id: 'section1', kind: 'card', order: 1000 });
+  expect(page.getSections()).toHaveLength(1);
+  expect(page.getSections()[0]).toEqual({ id: 'section1', kind: 'card', order: 1000 });
 });
 
 test('it should return sections in order', () => {
@@ -102,3 +102,62 @@ test('it should return contents in order', () => {
     },
   ]);
 });
+
+test('it should only allow to add one dashboard to a section', () => {
+  const page = new Page({ id: 'page1' });
+  page.createSection({ id: 'dashboard-section', kind: 'dashboard', order: 1000 });
+
+  page.addContent('dashboard-section', {
+    id: 'dashboard-content-1',
+    kind: 'dashboard',
+    order: 10,
+    input: { kind: 'static', id: 'dashboard-id-1' },
+  });
+
+  // add another dashboard to the same section
+  page.addContent('dashboard-section', {
+    id: 'dashboard-content-1',
+    kind: 'dashboard',
+    order: 10,
+    input: { kind: 'static', id: 'dashboard-id-1' },
+  });
+
+  // but it should only have one dashboard content
+  expect(page.getContents('dashboard-section')).toHaveLength(1);
+  expect(page.getContents('dashboard-section')).toEqual([
+    {
+      id: 'dashboard-content-1',
+      kind: 'dashboard',
+      order: 10,
+      input: { kind: 'static', id: 'dashboard-id-1' },
+    },
+  ]);
+
+  // add non-dashboard content to a section which already has a dashboard will override the dashboard
+  page.addContent('dashboard-section', {
+    id: 'vis-content-1',
+    kind: 'visualization',
+    order: 10,
+    input: { kind: 'static', id: 'vis-id-1' },
+  });
+  page.addContent('dashboard-section', {
+    id: 'vis-content-2',
+    kind: 'visualization',
+    order: 20,
+    input: { kind: 'static', id: 'vis-id-2' },
+  });
+  expect(page.getContents('dashboard-section')).toEqual([
+    {
+      id: 'vis-content-1',
+      kind: 'visualization',
+      order: 10,
+      input: { kind: 'static', id: 'vis-id-1' },
+    },
+    {
+      id: 'vis-content-2',
+      kind: 'visualization',
+      order: 20,
+      input: { kind: 'static', id: 'vis-id-2' },
+    },
+  ]);
+});
diff --git a/src/plugins/content_management/public/services/content_management/page.ts b/src/plugins/content_management/public/services/content_management/page.ts
@@ -19,9 +19,6 @@ export class Page {
   }
 
   createSection(section: Section) {
-    if (this.sections.has(section.id)) {
-      throw new Error(`Section id exists: ${section.id}`);
-    }
     this.sections.set(section.id, section);
     this.sections$.next(this.getSections());
   }
@@ -37,8 +34,13 @@ export class Page {
   addContent(sectionId: string, content: Content) {
     const sectionContents = this.contents.get(sectionId);
     if (sectionContents) {
-      if (content.kind === 'dashboard' && sectionContents.length > 0) {
-        throw new Error('Section type "dashboard" can only have one content type of "dashboard"');
+      /**
+       * `dashboard` type of content is exclusive, one section can only hold one `dashboard`
+       * if adding a `dashboard` to an existing section, it will replace the contents of section
+       * if adding a non-dashboard content to an section with `dashboard`, it will replace the dashboard
+       */
+      if (content.kind === 'dashboard' || sectionContents.some((c) => c.kind === 'dashboard')) {
+        sectionContents.length = 0;
       }
       sectionContents.push(content);
       // sort content by order
@@ -48,7 +50,7 @@ export class Page {
     }
 
     if (this.contentObservables.get(sectionId)) {
-      this.contentObservables.get(sectionId)?.next(this.contents.get(sectionId) ?? []);
+      this.contentObservables.get(sectionId)?.next([...(this.contents.get(sectionId) ?? [])]);
     } else {
       this.contentObservables.set(
         sectionId,
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		feat:
		- Register section and content with the same id will not throw error but overrides the exist one ([#7633](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7633))