Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -103,11 +103,11 @@
}
const reference = references.find((ref) => ref.name === panel.panelRefName);
if (reference) {
panels[panel.panelIndex] = {
gridData: panel.gridData,
panels[reference.id] = {

Check warning on line 106 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#L106

Added line #L106 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 @@
}

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 @@
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 @@
}

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
Loading