Skip to content

Commit

Permalink
fix: do not throw exception if content provider id exists (#7633)
Browse files Browse the repository at this point in the history
* fix: do not throw exception if content provider id exists
instead it will overwrite existing content provider

---------

Signed-off-by: Yulong Ruan <ruanyl@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit f08dbdf)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
1 parent d7f3765 commit 5ba4b64
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 25 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/7633.yml
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))
Original file line number Diff line number Diff line change
Expand Up @@ -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' },
Expand All @@ -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,
Expand All @@ -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' },
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,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] = {

Check warning on line 105 in src/plugins/content_management/public/components/section_input.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/content_management/public/components/section_input.ts#L105

Added line #L105 was not covered by tests
gridData: { ...panel.gridData, i: reference.id },
type: reference.type,
explicitInput: {
id: panel.panelIndex,
id: reference.id,
savedObjectId: reference.id,
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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' },
},
]);
});
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand All @@ -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;

Check warning on line 43 in src/plugins/content_management/public/services/content_management/page.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/content_management/public/services/content_management/page.ts#L43

Added line #L43 was not covered by tests
}
sectionContents.push(content);
// sort content by order
Expand All @@ -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,
Expand Down

0 comments on commit 5ba4b64

Please sign in to comment.