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

feat: [contentManagement] allow to update section input after page rendered #7651

Merged
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/7651.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- [contentManagement] allow to update section input after page rendered ([#7651](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7651))
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,8 @@ export interface CardExplicitInput {
}

export type CardContainerInput = ContainerInput<CardExplicitInput> & { columns?: number };

/**
* The props which allow to be updated after card container was created
*/
export type CardContainerExplicitInput = Partial<Pick<CardContainerInput, 'title' | 'columns'>>;
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { coreMock } from '../../../../core/public/mocks';
import { Content, Section } from '../services';
import { createCardInput, createDashboardInput } from './section_input';

test('it should create input for card section', () => {
test('it should create card section input', () => {
const section: Section = { id: 'section1', kind: 'card', order: 10 };
const content: Content = {
id: 'content1',
Expand Down Expand Up @@ -36,6 +36,41 @@ test('it should create input for card section', () => {
});
});

test('it should create card section input with explicit input specified', () => {
const section: Section = {
id: 'section1',
kind: 'card',
order: 10,
input: { title: 'new title', columns: 4 },
};
const content: Content = {
id: 'content1',
kind: 'card',
order: 0,
title: 'content title',
description: 'content description',
};
const input = createCardInput(section, [content]);
expect(input).toEqual({
id: 'section1',
title: 'new title',
hidePanelTitles: true,
viewMode: 'view',
columns: 4,
panels: {
content1: {
type: 'card_embeddable',
explicitInput: {
description: 'content description',
id: 'content1',
onClick: undefined,
title: 'content title',
},
},
},
});
});

test('it should not allow to create card input with non-card section', () => {
const section: Section = { id: 'section1', kind: 'dashboard', order: 10 };
const content: Content = {
Expand Down Expand Up @@ -79,7 +114,12 @@ test('it should throw error if creating dashboard input with non-dashboard secti
});

test('it should create dashboard input', async () => {
const section: Section = { id: 'section1', kind: 'dashboard', order: 10 };
const section: Section = {
id: 'section1',
kind: 'dashboard',
order: 10,
input: { timeRange: { from: 'now-1d', to: 'now' } },
};
const staticViz: Content = {
id: 'content1',
kind: 'visualization',
Expand Down Expand Up @@ -112,6 +152,9 @@ test('it should create dashboard input', async () => {
savedObjectsClient: clientMock,
});

// with explicit section input
expect(input.timeRange).toEqual({ from: 'now-1d', to: 'now' });

expect(input.panels).toEqual({
content1: {
explicitInput: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
viewMode: ViewMode.VIEW,
columns: section.columns,
panels,
...section.input,
};

contents.forEach((content) => {
Expand Down Expand Up @@ -74,7 +75,7 @@
const h = 15;
const counter = new BehaviorSubject(0);

contents.forEach(async (content, i) => {
contents.forEach(async (content) => {

Check warning on line 78 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#L78

Added line #L78 was not covered by tests
counter.next(counter.value + 1);
try {
if (content.kind === 'dashboard') {
Expand Down Expand Up @@ -164,29 +165,27 @@
}
});

/**
* TODO: the input should be hooked with query input
*/
const input: DashboardContainerInput = {
viewMode: ViewMode.VIEW,
panels,
id: section.id,
title: section.title ?? '',
viewMode: ViewMode.VIEW,
useMargins: true,
isFullScreenMode: false,
filters: [],
useMargins: true,
id: section.id,
timeRange: {
to: 'now',
from: 'now-7d',
},
title: section.title ?? 'test',
query: {
query: '',
language: 'lucene',
language: 'kuery',
Copy link
Member

@SuZhou-Joe SuZhou-Joe Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between the language here? I guess it will be overwritten by ...section.input?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it can be override by section.input. Here it just changed the default language from lucene to kuery(DQL).

},
refreshConfig: {
pause: true,
value: 15,
},
...section.input,
};

return new Promise<DashboardContainerInput>((resolve) => {
Expand Down
10 changes: 10 additions & 0 deletions src/plugins/content_management/public/components/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { DashboardContainerInput } from '../../../dashboard/public';

export type DashboardContainerExplicitInput = Partial<
Pick<DashboardContainerInput, 'filters' | 'timeRange' | 'query'>
>;
1 change: 1 addition & 0 deletions src/plugins/content_management/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ export class ContentManagementPublicPlugin
this.contentManagementService.start();
return {
registerContentProvider: this.contentManagementService.registerContentProvider,
updatePageSection: this.contentManagementService.updatePageSection,
renderPage: (id: string) => {
const page = this.contentManagementService.getPage(id);
if (page) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,25 @@ test('it should throw error when register content provider with invalid target a
})
).toThrowError();
});

test('it should throw error if update page section with invalid target area', () => {
const cms = new ContentManagementService();
cms.registerPage({ id: 'page1', sections: [{ id: 'section1', kind: 'card', order: 0 }] });
expect(() => cms.updatePageSection('invalid_target_area', jest.fn())).toThrowError();
});

test('it should update page section', () => {
const cms = new ContentManagementService();
cms.registerPage({ id: 'page1', sections: [{ id: 'section1', kind: 'card', order: 0 }] });
cms.updatePageSection(
'page1/section1',
jest.fn().mockReturnValue({ id: 'section1', kind: 'card', input: { title: 'new title' } })
);
expect(cms.getPage('page1')?.getSections()).toHaveLength(1);
expect(cms.getPage('page1')?.getSections()[0]).toEqual({
id: 'section1',
kind: 'card',
order: 0,
input: { title: 'new title' },
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import { Page } from './page';
import { ContentProvider, PageConfig } from './types';
import { ContentProvider, PageConfig, Section } from './types';

export class ContentManagementService {
contentProviders: Map<string, ContentProvider> = new Map();
Expand Down Expand Up @@ -48,6 +48,22 @@
}
};

updatePageSection = (

Check warning on line 51 in src/plugins/content_management/public/services/content_management/content_management_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/content_management/public/services/content_management/content_management_service.ts#L51

Added line #L51 was not covered by tests
targetArea: string,
callback: (section: Section | null, err?: Error) => Section | null
) => {
const [pageId, sectionId] = targetArea.split('/');

Check warning on line 55 in src/plugins/content_management/public/services/content_management/content_management_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/content_management/public/services/content_management/content_management_service.ts#L55

Added line #L55 was not covered by tests

if (!pageId || !sectionId) {
throw new Error('getTargetArea() should return a string in format {pageId}/{sectionId}');

Check warning on line 58 in src/plugins/content_management/public/services/content_management/content_management_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/content_management/public/services/content_management/content_management_service.ts#L58

Added line #L58 was not covered by tests
}

const page = this.getPage(pageId);

Check warning on line 61 in src/plugins/content_management/public/services/content_management/content_management_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/content_management/public/services/content_management/content_management_service.ts#L61

Added line #L61 was not covered by tests
if (page) {
page.updateSectionInput(sectionId, callback);

Check warning on line 63 in src/plugins/content_management/public/services/content_management/content_management_service.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/content_management/public/services/content_management/content_management_service.ts#L63

Added line #L63 was not covered by tests
}
};

setup() {
return {};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,3 +161,106 @@ test('it should only allow to add one dashboard to a section', () => {
},
]);
});

test('it should update dashboard section with new input', () => {
const page = new Page({ id: 'page1' });
page.createSection({
id: 'section_id',
kind: 'dashboard',
order: 1000,
input: { timeRange: { from: 'now-7d', to: 'now' } },
});
expect(page.getSections()).toHaveLength(1);
expect(page.getSections()[0]).toEqual({
id: 'section_id',
kind: 'dashboard',
order: 1000,
input: { timeRange: { from: 'now-7d', to: 'now' } },
});

page.updateSectionInput('section_id', (section) => {
if (section?.kind === 'dashboard') {
return { ...section, input: { timeRange: { from: 'now-1d', to: 'now' } } };
}
return section;
});

expect(page.getSections()[0]).toEqual({
id: 'section_id',
kind: 'dashboard',
order: 1000,
input: { timeRange: { from: 'now-1d', to: 'now' } },
});
});

test('it should update card section with new input', () => {
const page = new Page({ id: 'page1' });
page.createSection({
id: 'section_id',
kind: 'card',
order: 1000,
input: {},
});
expect(page.getSections()).toHaveLength(1);
expect(page.getSections()[0]).toEqual({
id: 'section_id',
kind: 'card',
order: 1000,
input: {},
});

page.updateSectionInput('section_id', (section) => {
if (section?.kind === 'card') {
return { ...section, input: { title: 'new title' } };
}
return section;
});

expect(page.getSections()[0]).toEqual({
id: 'section_id',
kind: 'card',
order: 1000,
input: { title: 'new title' },
});
});

test('it should not allow to update section property other than `input`', () => {
const page = new Page({ id: 'page1' });
page.createSection({
id: 'section_id',
kind: 'dashboard',
order: 1000,
input: { timeRange: { from: 'now-7d', to: 'now' } },
});
expect(page.getSections()).toHaveLength(1);
expect(page.getSections()[0]).toEqual({
id: 'section_id',
kind: 'dashboard',
order: 1000,
input: { timeRange: { from: 'now-7d', to: 'now' } },
});

// update the section with new kind: custom and new id: section_id_new
page.updateSectionInput('section_id', (section) => {
if (section?.kind === 'dashboard') {
return { ...section, id: 'section_id_new', kind: 'custom', render: jest.fn() };
}
return section;
});

// section should not changed as it only allows to update `section.input` field
expect(page.getSections()[0]).toEqual({
id: 'section_id',
kind: 'dashboard',
order: 1000,
input: { timeRange: { from: 'now-7d', to: 'now' } },
});
});

test('it should callback with error if section not exist', () => {
const page = new Page({ id: 'page1' });
const callbackMock = jest.fn();
page.updateSectionInput('section_id_not_exist', callbackMock);
expect(callbackMock.mock.calls[0][0]).toBe(null);
expect(callbackMock.mock.calls[0][1]).toBeInstanceOf(Error);
});
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@
this.config = pageConfig;
}

createSection(section: Section) {
private setSection(section: Section) {
this.sections.set(section.id, section);
this.sections$.next(this.getSections());
}

createSection(section: Section) {
this.setSection(section);

Check warning on line 27 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#L27

Added line #L27 was not covered by tests
}

getSections() {
return [...this.sections.values()].sort((a, b) => a.order - b.order);
}
Expand All @@ -31,6 +35,28 @@
return this.sections$;
}

updateSectionInput(
sectionId: string,
callback: (section: Section | null, err?: Error) => Section | null
) {
const section = this.sections.get(sectionId);

Check warning on line 42 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#L42

Added line #L42 was not covered by tests
if (!section) {
callback(null, new Error(`Section id ${sectionId} not found`));
return;

Check warning on line 45 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#L44-L45

Added lines #L44 - L45 were not covered by tests
}

const updated = callback(section);

Check warning on line 48 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#L48

Added line #L48 was not covered by tests
if (updated) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you feel about doing

if (updated.kind !== section.kind) return;

... and then a switch-case for dashboard, card, and custom?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I will adopt this change in the upcoming PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image Seems TS compiler is not happy with that ;D

if (updated.kind === 'dashboard' && section.kind === 'dashboard') {
this.setSection({ ...section, input: updated.input });

Check warning on line 51 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#L51

Added line #L51 was not covered by tests
}
if (updated.kind === 'card' && section.kind === 'card') {
this.setSection({ ...section, input: updated.input });

Check warning on line 54 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#L54

Added line #L54 was not covered by tests
}
// TODO: we may need to support update input of `custom` section
}
}

addContent(sectionId: string, content: Content) {
const sectionContents = this.contents.get(sectionId);
if (sectionContents) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { CardContainerExplicitInput } from '../../components/card_container/types';
import { DashboardContainerExplicitInput } from '../../components/types';

export interface PageConfig {
id: string;
title?: string;
Expand All @@ -25,13 +28,15 @@ export type Section =
order: number;
title?: string;
description?: string;
input?: DashboardContainerExplicitInput;
}
| {
kind: 'card';
id: string;
order: number;
title?: string;
columns?: number;
input?: CardContainerExplicitInput;
};

export type Content =
Expand Down
Loading
Loading