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.

Sign up for GitHub

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

Jump to bottom

[Dashboard] Add Readonly State For Managed Dashboards #166204

Merged
merged 16 commits into from Sep 21, 2023
Merged

[Dashboard] Add Readonly State For Managed Dashboards #166204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0216597
Add managed state to Dashboard
ThomThomson Sep 11, 2023
a5556ca
fix types
ThomThomson Sep 11, 2023
c2e78eb
fix jest test
ThomThomson Sep 12, 2023
f555386
fix jest test again
ThomThomson Sep 13, 2023
76acf9d
Merge branch 'main' into dashboard/managedState
kibanamachine Sep 13, 2023
3d25eb1
read only for managed Dashboards in table list view.
ThomThomson Sep 13, 2023
ae8210f
omit managed Dashboards from dashboard picker
ThomThomson Sep 13, 2023
bcfc7af
block dashboard from entering edit mode when managed
ThomThomson Sep 13, 2023
f51b9a7
lock down edit mode when write controls are off. Rename and combine i…
ThomThomson Sep 14, 2023
9d28907
undo module level showWriteControls
ThomThomson Sep 14, 2023
ab07806
Merge branch 'main' into dashboard/managedState
kibanamachine Sep 18, 2023
7029844
default isEditable to true
ThomThomson Sep 18, 2023
c895a57
check for managed state inside table list view
ThomThomson Sep 18, 2023
4679bb1
make managed state override itemIsEditable
ThomThomson Sep 20, 2023
5cbd79c
Update comment to mention that managed items will always be non-edita…
ThomThomson Sep 20, 2023
ed2555d
Merge remote-tracking branch 'upstream/main' into dashboard/managedState
ThomThomson Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/content-management/table_list_view/src/table_list_view.tsx
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ export type TableListViewProps<T extends UserContentCommonSchema = UserContentCo
| 'contentEditor'
| 'titleColumnName'
| 'withoutPageTemplateWrapper'
| 'showEditActionForItem'
| 'itemIsEditable'
> & {
title: string;
description?: string;
Expand Down Expand Up @@ -73,6 +73,7 @@ export const TableListView = <T extends UserContentCommonSchema>({
titleColumnName,
additionalRightSideActions,
withoutPageTemplateWrapper,
itemIsEditable,
}: TableListViewProps<T>) => {
const PageTemplate = withoutPageTemplateWrapper
? (React.Fragment as unknown as typeof KibanaPageTemplate)
Expand Down Expand Up @@ -118,6 +119,7 @@ export const TableListView = <T extends UserContentCommonSchema>({
id={listingId}
contentEditor={contentEditor}
titleColumnName={titleColumnName}
itemIsEditable={itemIsEditable}
withoutPageTemplateWrapper={withoutPageTemplateWrapper}
onFetchSuccess={onFetchSuccess}
setPageDataTestSubject={setPageDataTestSubject}
Expand Down
29 changes: 21 additions & 8 deletions packages/content-management/table_list_view_table/src/table_list_view_table.tsx
View file
Open in desktop
Expand Up @@ -90,10 +90,12 @@ export interface TableListViewTableProps<
* Edit action onClick handler. Edit action not provided when property is not provided
*/
editItem?(item: T): void;

/**
* Handler to set edit action visiblity per item.
* Handler to set edit action visiblity, and content editor readonly state per item. If not provided all non-managed items are considered editable. Note: Items with the managed property set to true will always be non-editable.
*/
showEditActionForItem?(item: T): boolean;
itemIsEditable?(item: T): boolean;
Copy link
Contributor

@nreese nreese Sep 18, 2023 •
edited

Choose a reason for hiding this comment

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

Why is itemIsEditable dashboard implemenation checking for item.managed? Shouldn't that information already be in item? So instead of this change, just check that item.managed here instead of making itemIsEditable implemenation do it. This will reduce code duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It made sense to me to unify the callback into one function that determined if the itemIsEditable for both the edit action + the inspect item action.

  1. If the table list view gets any more edit functionality in the future, the same function can be reused.
  2. The generic type UserContentCommonSchema isn't necessarily a saved object. We could require UserContentCommonSchema to have a managed?: boolean field
  3. It allows the implementor to determine for themselves what makes an item readonly or managed etc. For instance, Visualizations have a readonly attribute.

We could leave the function as itemIsEditable, and also have a default implementation that checks managed state.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with leaving managed behavior up to each implementation is that will result in inconsistent behavior across Kibana and potential bugs as teams forget to add checks for managed.

I would be in favor of adding managed to UserContentCommonSchema. Then the table component can provide consistent behavior for managed flag in a single location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add managed to UserContentCommonSchema and check it within the table list view, but it's important to note that it won't automatically make any managed saved objects non-editable. This is because the implementor is also in charge of actually fetching the objects, and mapping them to UserContentCommonSchema.

Copy link
Contributor

@nreese nreese Sep 18, 2023 •
edited

Choose a reason for hiding this comment

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

Would it make sense to make managed required? That why tslint would fail when findItems does not provide a value?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer option 2.

Wasn't there mention of 'by reference' saved objects in some of the managed dashboards? That would require support for managed saved objects for other types as well. Having a consistent behavior across listing pages would be ideal.

Copy link
Contributor

Choose a reason for hiding this comment

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

there will be a pattern for this managed state

Are you saying that this is a new top level SO property handled by core available on all saved object? That's great to hear 👍

Sorry about mentioning Drew in my comment above. I went too quick when looking at the line change in the IDE

Screenshot 2023-09-20 at 15 36 30

Regarding the showEditActionForItem, looking at it again it seems to be a tech debt that we have. It will have to be removed in favor of using the existing rowItemActions (which need to support the edit action).

Thanks for clarifying, I agree then to go with option 2. Could you update the comments on top of the showEditActionForItem and editItem props to indicate that those will not have any effect if the content item has the managed prop set to true?

Slightly off topic:
Do all SO that have the managed prop set to true automatically get the managed tag (in their references array)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've made the managed state override the itemIsEditable callback, and have added a comment to explain that in 4679bb1.

Yes, the managed state is a new top level SO property handled by core, thanks to @TinaHeiligers for that! And I'm not quite sure how the managed tag appears, but I assume it's more of a holdover from before the new property. Tina would know more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. What I meant for the comments is to add that information to the TableListViewTableProps interface. We want to warn consumers that those props won't have any effect if the SO has managed set to true.

// packages/content-management/table_list_view_table/src/table_list_view_table.tsx

...
  /**
   * Edit action onClick handler. Edit action not provided when property is not provided
   */
  editItem?(item: T): void;
  /**
   * Handler to set edit action visiblity per item.
   */
  showEditActionForItem?(item: T): boolean;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5cbd79c


/**
* Name for the column containing the "title" value.
*/
Expand Down Expand Up @@ -144,6 +146,7 @@ export interface State<T extends UserContentCommonSchema = UserContentCommonSche
export interface UserContentCommonSchema {
id: string;
updatedAt: string;
managed?: boolean;
references: SavedObjectsReference[];
type: string;
attributes: {
Expand Down Expand Up @@ -264,7 +267,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
findItems,
createItem,
editItem,
showEditActionForItem,
itemIsEditable,
deleteItems,
getDetailViewLink,
onClickTitle,
Expand Down Expand Up @@ -451,6 +454,15 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
items,
});

const isEditable = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

(item: T) => {
// If the So is `managed` it is never editable.
if (item.managed) return false;
return itemIsEditable?.(item) ?? true;
},
[itemIsEditable]
);

const inspectItem = useCallback(
(item: T) => {
const tags = getTagIdsFromReferences(item.references).map((_id) => {
Expand All @@ -466,6 +478,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
},
entityName,
...contentEditor,
isReadonly: contentEditor.isReadonly || !isEditable(item),
onSave:
contentEditor.onSave &&
(async (args) => {
Expand All @@ -476,7 +489,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
}),
});
},
[getTagIdsFromReferences, openContentEditor, entityName, contentEditor, fetchItems]
[getTagIdsFromReferences, openContentEditor, entityName, contentEditor, isEditable, fetchItems]
);

const tableColumns = useMemo(() => {
Expand Down Expand Up @@ -550,7 +563,7 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
),
icon: 'pencil',
type: 'icon',
available: (v) => (showEditActionForItem ? showEditActionForItem(v) : true),
available: (item) => isEditable(item),
enabled: (v) => !(v as unknown as { error: string })?.error,
onClick: editItem,
'data-test-subj': `edit-action`,
Expand Down Expand Up @@ -598,16 +611,16 @@ function TableListViewTableComp<T extends UserContentCommonSchema>({
customTableColumn,
hasUpdatedAtMetadata,
editItem,
contentEditor.enabled,
listingId,
getDetailViewLink,
onClickTitle,
searchQuery.text,
addOrRemoveIncludeTagFilter,
addOrRemoveExcludeTagFilter,
addOrRemoveIncludeTagFilter,
DateFormatterComp,
contentEditor,
isEditable,
inspectItem,
showEditActionForItem,
]);

const itemsById = useMemo(() => {
Expand Down
2 changes: 2 additions & 0 deletions packages/kbn-content-management-utils/src/msearch.ts
View file
Open in desktop
Expand Up @@ -43,11 +43,13 @@ function savedObjectToItem<Attributes extends object>(
error,
namespaces,
version,
managed,
} = savedObject;

return {
id,
type,
managed,
updatedAt,
createdAt,
attributes: pick(attributes, allowedSavedObjectAttributes),
Expand Down
2 changes: 2 additions & 0 deletions packages/kbn-content-management-utils/src/saved_object_content_storage.ts
View file
Open in desktop
Expand Up @@ -69,11 +69,13 @@ function savedObjectToItem<Attributes extends object>(
error,
namespaces,
version,
managed,
Copy link
Contributor

Choose a reason for hiding this comment

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

By default, managed is false. The only way to change to true is to use SO import, with an option with conditions

} = savedObject;

return {
id,
type,
managed,
updatedAt,
createdAt,
attributes: pick(attributes, allowedSavedObjectAttributes),
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-content-management-utils/src/types.ts
View file
Open in desktop
Expand Up @@ -200,6 +200,7 @@ export interface SOWithMetadata<Attributes extends object = object> {
statusCode: number;
metadata?: Record<string, unknown>;
};
managed?: boolean;
attributes: Attributes;
references: Reference[];
namespaces?: string[];
Expand Down
11 changes: 11 additions & 0 deletions src/plugins/dashboard/public/dashboard_app/_dashboard_app_strings.ts
View file
Open in desktop
Expand Up @@ -25,6 +25,17 @@ export const dashboardReadonlyBadge = {
}),
};

export const dashboardManagedBadge = {
getText: () =>
i18n.translate('dashboard.badge.managed.text', {
defaultMessage: 'Managed',
}),
getTooltip: () =>
i18n.translate('dashboard.badge.managed.tooltip', {
defaultMessage: 'This dashboard is system managed. Clone this dashboard to make changes.',
}),
};

/**
* @param title {string} the current title of the dashboard
* @param viewMode {DashboardViewMode} the current mode. If in editing state, prepends 'Editing ' to the title.
Expand Down
24 changes: 19 additions & 5 deletions src/plugins/dashboard/public/dashboard_app/top_nav/dashboard_top_nav.tsx
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ import {
leaveConfirmStrings,
getDashboardBreadcrumb,
unsavedChangesBadgeStrings,
dashboardManagedBadge,
} from '../_dashboard_app_strings';
import { UI_SETTINGS } from '../../../common';
import { useDashboardAPI } from '../dashboard_app';
Expand Down Expand Up @@ -67,7 +68,7 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
navigation: { TopNavMenu },
embeddable: { getStateTransfer },
initializerContext: { allowByValueEmbeddables },
dashboardCapabilities: { saveQuery: showSaveQuery },
dashboardCapabilities: { saveQuery: showSaveQuery, showWriteControls },
} = pluginServices.getServices();
const isLabsEnabled = uiSettings.get(UI_SETTINGS.ENABLE_LABS_UI);
const { setHeaderActionMenu, onAppLeave } = useDashboardMountContext();
Expand All @@ -82,6 +83,7 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
const fullScreenMode = dashboard.select((state) => state.componentState.fullScreenMode);
const savedQueryId = dashboard.select((state) => state.componentState.savedQueryId);
const lastSavedId = dashboard.select((state) => state.componentState.lastSavedId);
const managed = dashboard.select((state) => state.componentState.managed);

const viewMode = dashboard.select((state) => state.explicitInput.viewMode);
const query = dashboard.select((state) => state.explicitInput.query);
Expand Down Expand Up @@ -237,9 +239,8 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
});

const badges = useMemo(() => {
if (viewMode !== ViewMode.EDIT) return;
const allBadges: TopNavMenuProps['badges'] = [];
if (hasUnsavedChanges) {
if (hasUnsavedChanges && viewMode === ViewMode.EDIT) {
allBadges.push({
'data-test-subj': 'dashboardUnsavedChangesBadge',
badgeText: unsavedChangesBadgeStrings.getUnsavedChangedBadgeText(),
Expand All @@ -251,7 +252,7 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
} as EuiToolTipProps,
});
}
if (hasRunMigrations) {
if (hasRunMigrations && viewMode === ViewMode.EDIT) {
allBadges.push({
'data-test-subj': 'dashboardSaveRecommendedBadge',
badgeText: unsavedChangesBadgeStrings.getHasRunMigrationsText(),
Expand All @@ -264,8 +265,21 @@ export function DashboardTopNav({ embedSettings, redirectTo }: DashboardTopNavPr
} as EuiToolTipProps,
});
}
if (showWriteControls && managed) {
allBadges.push({
'data-test-subj': 'dashboardSaveRecommendedBadge',
badgeText: dashboardManagedBadge.getText(),
title: '',
color: 'primary',
iconType: 'glasses',
toolTipProps: {
content: dashboardManagedBadge.getTooltip(),
position: 'bottom',
} as EuiToolTipProps,
});
}
return allBadges;
}, [hasRunMigrations, hasUnsavedChanges, viewMode]);
}, [hasUnsavedChanges, viewMode, hasRunMigrations, showWriteControls, managed]);

return (
<div className="dashboardTopNav">
Expand Down
5 changes: 3 additions & 2 deletions src/plugins/dashboard/public/dashboard_app/top_nav/use_dashboard_menu_items.tsx
View file
Open in desktop
Expand Up @@ -56,6 +56,7 @@ export const useDashboardMenuItems = ({
const lastSavedId = dashboard.select((state) => state.componentState.lastSavedId);
const dashboardTitle = dashboard.select((state) => state.explicitInput.title);
const viewMode = dashboard.select((state) => state.explicitInput.viewMode);
const managed = dashboard.select((state) => state.componentState.managed);
const disableTopNav = isSaveInProgress || hasOverlays;

/**
Expand Down Expand Up @@ -265,7 +266,7 @@ export const useDashboardMenuItems = ({
const labsMenuItem = isLabsEnabled ? [menuItems.labs] : [];
const shareMenuItem = share ? [menuItems.share] : [];
const cloneMenuItem = showWriteControls ? [menuItems.clone] : [];
const editMenuItem = showWriteControls ? [menuItems.edit] : [];
const editMenuItem = showWriteControls && !managed ? [menuItems.edit] : [];
return [
...labsMenuItem,
menuItems.fullScreen,
Expand All @@ -274,7 +275,7 @@ export const useDashboardMenuItems = ({
resetChangesMenuItem,
...editMenuItem,
];
}, [menuItems, share, showWriteControls, resetChangesMenuItem, isLabsEnabled]);
}, [isLabsEnabled, menuItems, share, showWriteControls, managed, resetChangesMenuItem]);

const editModeTopNavConfig = useMemo(() => {
const labsMenuItem = isLabsEnabled ? [menuItems.labs] : [];
Expand Down
7 changes: 5 additions & 2 deletions src/plugins/dashboard/public/dashboard_container/embeddable/api/run_save_functions.tsx
View file
Open in desktop
Expand Up @@ -33,10 +33,11 @@ export function runSaveAs(this: DashboardContainer) {

const {
explicitInput: currentState,
componentState: { lastSavedId },
componentState: { lastSavedId, managed },
} = this.getState();

return new Promise<SaveDashboardReturn | undefined>((resolve) => {
if (managed) resolve(undefined);
const onSave = async ({
newTags,
newTitle,
Expand Down Expand Up @@ -132,9 +133,11 @@ export async function runQuickSave(this: DashboardContainer) {

const {
explicitInput: currentState,
componentState: { lastSavedId },
componentState: { lastSavedId, managed },
} = this.getState();

if (managed) return;

const saveResult = await saveDashboardState({
lastSavedId,
currentState,
Expand Down
15 changes: 15 additions & 0 deletions src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.test.ts
View file
Open in desktop
Expand Up @@ -94,6 +94,21 @@ test('pulls state from dashboard saved object when given a saved object id', asy
expect(dashboard!.getState().explicitInput.description).toBe(`wow would you look at that? Wow.`);
});

test('passes managed state from the saved object into the Dashboard component state', async () => {
pluginServices.getServices().dashboardContentManagement.loadDashboardState = jest
.fn()
.mockResolvedValue({
dashboardInput: {
...DEFAULT_DASHBOARD_INPUT,
description: 'wow this description is okay',
},
managed: true,
});
const dashboard = await createDashboard({}, 0, 'what-an-id');
expect(dashboard).toBeDefined();
expect(dashboard!.getState().componentState.managed).toBe(true);
});

test('pulls state from session storage which overrides state from saved object', async () => {
pluginServices.getServices().dashboardContentManagement.loadDashboardState = jest
.fn()
Expand Down
18 changes: 15 additions & 3 deletions src/plugins/dashboard/public/dashboard_container/embeddable/create/create_dashboard.ts
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@ import { syncUnifiedSearchState } from './unified_search/sync_dashboard_unified_
import { DEFAULT_DASHBOARD_INPUT, GLOBAL_STATE_STORAGE_KEY } from '../../../dashboard_constants';
import { startSyncingDashboardControlGroup } from './controls/dashboard_control_group_integration';
import { startDashboardSearchSessionIntegration } from './search_sessions/start_dashboard_search_session_integration';
import { DashboardPublicState } from '../../types';

/**
* Builds a new Dashboard from scratch.
Expand Down Expand Up @@ -86,16 +87,27 @@ export const createDashboard = async (
// --------------------------------------------------------------------------------------
// Build and return the dashboard container.
// --------------------------------------------------------------------------------------
const initialComponentState: DashboardPublicState = {
lastSavedInput: savedObjectResult?.dashboardInput ?? {
...DEFAULT_DASHBOARD_INPUT,
id: input.id,
},
hasRunClientsideMigrations: savedObjectResult.anyMigrationRun,
isEmbeddedExternally: creationOptions?.isEmbeddedExternally,
animatePanelTransforms: false, // set panel transforms to false initially to avoid panels animating on initial render.
hasUnsavedChanges: false, // if there is initial unsaved changes, the initial diff will catch them.
managed: savedObjectResult.managed,
lastSavedId: savedObjectId,
};

const dashboardContainer = new DashboardContainer(
input,
reduxEmbeddablePackage,
searchSessionId,
savedObjectResult?.dashboardInput,
savedObjectResult.anyMigrationRun,
dashboardCreationStartTime,
undefined,
creationOptions,
savedObjectId
initialComponentState
);
dashboardContainerReady$.next(dashboardContainer);
return dashboardContainer;
Expand Down
9 changes: 7 additions & 2 deletions src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.test.tsx
View file
Open in desktop
Expand Up @@ -167,10 +167,15 @@ test('Container view mode change propagates to new children', async () => {

test('searchSessionId propagates to children', async () => {
const searchSessionId1 = 'searchSessionId1';
const sampleInput = getSampleDashboardInput();
const container = new DashboardContainer(
getSampleDashboardInput(),
sampleInput,
mockedReduxEmbeddablePackage,
searchSessionId1
searchSessionId1,
0,
undefined,
undefined,
{ lastSavedInput: sampleInput }
);
const embeddable = await container.addNewEmbeddable<
ContactCardEmbeddableInput,
Expand Down