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: duplicate selected/all saved objects UI #305

Merged
merged 28 commits into from
Apr 16, 2024

Conversation

yubonluo
Copy link

@yubonluo yubonluo commented Mar 21, 2024

Description

duplicate selected/all saved objects UI

Screenshot

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

gaobinlong and others added 6 commits March 21, 2024 11:56
…ject#53)

* Add copy saved objects among workspaces functionality

Signed-off-by: gaobinlong <[email protected]>

Signed-off-by: gaobinlong <[email protected]>

* Fix bug

Signed-off-by: gaobinlong <[email protected]>

* Fix bug

Signed-off-by: gaobinlong <[email protected]>

---------

Signed-off-by: gaobinlong <[email protected]>
# Conflicts:
#	src/core/server/saved_objects/routes/copy.ts
#	src/plugins/saved_objects_management/public/constants.ts
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/header.tsx
#	src/plugins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
* fix typo

Signed-off-by: yuye-aws <[email protected]>

* adjust copy modal

Signed-off-by: yuye-aws <[email protected]>

* list workspace with write permission on copy modal

Signed-off-by: yuye-aws <[email protected]>

* add copy icon and move getcopyworkspaces function from copy_modal to saved_object table

Signed-off-by: yuye-aws <[email protected]>

* fix duplicate error in public workspace and change copy to duplicate all in header

Signed-off-by: yuye-aws <[email protected]>

* bug fix: create saved objects in public workspace

Signed-off-by: yuye-aws <[email protected]>

* update snapshots

Signed-off-by: yuye-aws <[email protected]>

* remove unused import

Signed-off-by: yuye-aws <[email protected]>

* change validate schema

Signed-off-by: yuye-aws <[email protected]>

* behavior subject bug fix for workspace plugin

Signed-off-by: yuye-aws <[email protected]>

---------

Signed-off-by: yuye-aws <[email protected]>
# Conflicts:
#	src/core/public/chrome/ui/header/__snapshots__/header.test.tsx.snap
#	src/core/server/saved_objects/permission_control/acl.test.ts
#	src/core/server/saved_objects/permission_control/client.ts
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/copy_modal.tsx
#	src/plugins/workspace/server/plugin.ts
#	src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
* hide import for application home page

Signed-off-by: Hailong Cui <[email protected]>

* add workpspace into gotoApp link

Signed-off-by: Hailong Cui <[email protected]>

* remove special logic for management workspace

Signed-off-by: Hailong Cui <[email protected]>

* variable name change and more UTs

Signed-off-by: Hailong Cui <[email protected]>

---------

Signed-off-by: Hailong Cui <[email protected]>
# Conflicts:
#	src/plugins/saved_objects_management/public/management_section/objects_table/__snapshots__/saved_objects_table.test.tsx.snap
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/__snapshots__/header.test.tsx.snap
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/table.test.tsx
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/table.tsx
#	src/plugins/saved_objects_management/public/management_section/objects_table/saved_objects_table.test.tsx
#	src/plugins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
* implement all duplicate copy modal

Signed-off-by: yuye-aws <[email protected]>

* add spacer after checkbox list

Signed-off-by: yuye-aws <[email protected]>

* add fail message for copy saved objects

Signed-off-by: yuye-aws <[email protected]>

* change title wording to manage library

Signed-off-by: yuye-aws <[email protected]>

* single duplicate

Signed-off-by: yuye-aws <[email protected]>

* change wording

Signed-off-by: yuye-aws <[email protected]>

* remove comment

Signed-off-by: yuye-aws <[email protected]>

* bug fix: keep selected saved objects info when cancel duplicate all

Signed-off-by: yuye-aws <[email protected]>

* fix typo

Signed-off-by: yuye-aws <[email protected]>

* use icu syntax in copy message

Signed-off-by: yuye-aws <[email protected]>

* bug fix: keep selected saved objects info when cancel duplicate single

Signed-off-by: yuye-aws <[email protected]>

* set current workspace as the first option

Signed-off-by: yuye-aws <[email protected]>

* update snapshot

Signed-off-by: yuye-aws <[email protected]>

* resolve conflict

Signed-off-by: yuye-aws <[email protected]>

* update snapshot

Signed-off-by: yuye-aws <[email protected]>

* bug fix for saved object table

Signed-off-by: yuye-aws <[email protected]>

* update snapshot

Signed-off-by: yuye-aws <[email protected]>

* remove unused file

Signed-off-by: yuye-aws <[email protected]>

* change i18n constant

Signed-off-by: yuye-aws <[email protected]>

* remove empty push

Signed-off-by: yuye-aws <[email protected]>

* hide duplicate when workspace is disabled

Signed-off-by: yuye-aws <[email protected]>

* update snapshots

Signed-off-by: yuye-aws <[email protected]>

---------

Signed-off-by: yuye-aws <[email protected]>
# Conflicts:
#	src/plugins/saved_objects_management/public/constants.ts
#	src/plugins/saved_objects_management/public/management_section/objects_table/__snapshots__/saved_objects_table.test.tsx.snap
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/copy_modal.tsx
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/header.tsx
#	src/plugins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
#	src/plugins/saved_objects_management/public/plugin.ts
* rename copy to duplicate

Signed-off-by: yuye-aws <[email protected]>

* duplicate in visualization

Signed-off-by: yuye-aws <[email protected]>

* duplicate in dashboard

Signed-off-by: yuye-aws <[email protected]>

* resolve conflict

Signed-off-by: yuye-aws <[email protected]>

* update test and snapshots

Signed-off-by: yuye-aws <[email protected]>

* re-duplicate if some objects cannot be duplicated

Signed-off-by: yuye-aws <[email protected]>

* remove clone for dashboard

Signed-off-by: yuye-aws <[email protected]>

* rename duplicateState to duplicateMode

Signed-off-by: yuye-aws <[email protected]>

* change workspace prop to currentWorkspace in SavedObjectsDuplicateModal

Signed-off-by: yuye-aws <[email protected]>

* change wording

Signed-off-by: yuye-aws <[email protected]>

* move duplicate modal to saved_objects for reuse

Signed-off-by: yuye-aws <[email protected]>

* move duplicate modal to saved objects management for reuse

Signed-off-by: yuye-aws <[email protected]>

* remove minimal duplicate modal props logic

Signed-off-by: yuye-aws <[email protected]>

* refactor duplicate modal props for dashboard and visualization

Signed-off-by: yuye-aws <[email protected]>

* Update getDuplicateWorkspaces function

Co-authored-by: Yulong Ruan <[email protected]>

* update function onDuplicate for dashboard

Signed-off-by: yuye-aws <[email protected]>

* Update doDuplicate for visualization

Co-authored-by: Yulong Ruan <[email protected]>

* refactor function getDuplicateWorkspaces

Signed-off-by: yuye-aws <[email protected]>

* add i18n context to saved objects table duplicate modal

Signed-off-by: yuye-aws <[email protected]>

* refactor duplicate modal logic in saved object table

Signed-off-by: yuye-aws <[email protected]>

* add error message for partial duplicate failed

Signed-off-by: yuye-aws <[email protected]>

* merge commits

Signed-off-by: yuye-aws <[email protected]>

* add type info for dashboard and visualization

Signed-off-by: yuye-aws <[email protected]>

* remote create vis reference logic

Signed-off-by: yuye-aws <[email protected]>

* Revert "remove clone for dashboard"

This reverts commit 84f77fb.

* hide duplicate when workspace disabled in dashboard

Signed-off-by: yuye-aws <[email protected]>

* feat: skip permission validate when no workspaces and permissions attributes (opensearch-project#163)

* feat: skip permission validate when saved object without workspaces and permissions attributes

Signed-off-by: Lin Wang <[email protected]>

* feat: add annontation to skip permission check

Signed-off-by: Lin Wang <[email protected]>

* refactor: remove bind and simplify validate logic

Signed-off-by: Lin Wang <[email protected]>

* feat: remove library write for object based ACL

Signed-off-by: Lin Wang <[email protected]>

---------

Signed-off-by: Lin Wang <[email protected]>

* remove get workspaces with write permission logic and add readonly props to workspace attribute

Signed-off-by: yuye-aws <[email protected]>

* change type definition logic

Signed-off-by: yuye-aws <[email protected]>

* Fix typo (opensearch-project#176)

---------

Signed-off-by: Yulong Ruan <[email protected]>

* remove exit workspace logic (opensearch-project#179)

Signed-off-by: yuye-aws <[email protected]>

* rename copy to duplicate

Signed-off-by: yuye-aws <[email protected]>

* duplicate in visualization

Signed-off-by: yuye-aws <[email protected]>

* duplicate in dashboard

Signed-off-by: yuye-aws <[email protected]>

* resolve conflict

Signed-off-by: yuye-aws <[email protected]>

* update test and snapshots

Signed-off-by: yuye-aws <[email protected]>

* re-duplicate if some objects cannot be duplicated

Signed-off-by: yuye-aws <[email protected]>

* remove clone for dashboard

Signed-off-by: yuye-aws <[email protected]>

* rename duplicateState to duplicateMode

Signed-off-by: yuye-aws <[email protected]>

* change workspace prop to currentWorkspace in SavedObjectsDuplicateModal

Signed-off-by: yuye-aws <[email protected]>

* change wording

Signed-off-by: yuye-aws <[email protected]>

* move duplicate modal to saved_objects for reuse

Signed-off-by: yuye-aws <[email protected]>

* move duplicate modal to saved objects management for reuse

Signed-off-by: yuye-aws <[email protected]>

* remove minimal duplicate modal props logic

Signed-off-by: yuye-aws <[email protected]>

* refactor duplicate modal props for dashboard and visualization

Signed-off-by: yuye-aws <[email protected]>

* Update getDuplicateWorkspaces function

Co-authored-by: Yulong Ruan <[email protected]>

* update function onDuplicate for dashboard

Signed-off-by: yuye-aws <[email protected]>

* Update doDuplicate for visualization

Co-authored-by: Yulong Ruan <[email protected]>

* refactor function getDuplicateWorkspaces

Signed-off-by: yuye-aws <[email protected]>

* add i18n context to saved objects table duplicate modal

Signed-off-by: yuye-aws <[email protected]>

* refactor duplicate modal logic in saved object table

Signed-off-by: yuye-aws <[email protected]>

* add error message for partial duplicate failed

Signed-off-by: yuye-aws <[email protected]>

* merge commits

Signed-off-by: yuye-aws <[email protected]>

* add type info for dashboard and visualization

Signed-off-by: yuye-aws <[email protected]>

* remote create vis reference logic

Signed-off-by: yuye-aws <[email protected]>

* Revert "remove clone for dashboard"

This reverts commit 84f77fb.

* hide duplicate when workspace disabled in dashboard

Signed-off-by: yuye-aws <[email protected]>

* remove get workspaces with write permission logic and add readonly props to workspace attribute

Signed-off-by: yuye-aws <[email protected]>

* change type definition logic

Signed-off-by: yuye-aws <[email protected]>

* rename variable and function name

Signed-off-by: yuye-aws <[email protected]>

* change permission mode to get target workspaces when duplicate

Signed-off-by: yuye-aws <[email protected]>

---------

Signed-off-by: yuye-aws <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: Yulong Ruan <[email protected]>
Co-authored-by: Yulong Ruan <[email protected]>
Co-authored-by: Lin Wang <[email protected]>
Co-authored-by: Yulong Ruan <[email protected]>
# Conflicts:
#	src/core/public/workspace/workspaces_service.ts
#	src/plugins/saved_objects_management/public/constants.ts
#	src/plugins/saved_objects_management/public/management_section/objects_table/components/duplicate_modal.tsx
#	src/plugins/saved_objects_management/public/management_section/objects_table/saved_objects_table.tsx
#	src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
src/core/server/saved_objects/routes/index.ts Outdated Show resolved Hide resolved
* modal can still recover (e.g. the name of the saved object was already taken).
*/

export function showDuplicateModal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is used for?

Copy link
Author

Choose a reason for hiding this comment

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

This file is used to reuse the duplicate logic in subsequent dashboard and visualize plugins.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we need to show a modal, we can embed it as a React component. Why do we make it as a util?

Copy link
Author

Choose a reason for hiding this comment

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

The file has been deleted.

src/plugins/workspace/public/workspace_client.ts Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.09%. Comparing base (efe6254) to head (8114a6f).
Report is 2 commits behind head on workspace-pr-integr.

Additional details and impacted files
@@                   Coverage Diff                   @@
##           workspace-pr-integr     #305      +/-   ##
=======================================================
+ Coverage                67.05%   67.09%   +0.03%     
=======================================================
  Files                     3345     3349       +4     
  Lines                    65045    65156     +111     
  Branches                 10505    10525      +20     
=======================================================
+ Hits                     43617    43717     +100     
- Misses                   18849    18860      +11     
  Partials                  2579     2579              
Flag Coverage Δ
Linux_ 66.44% <100.00%> (+0.04%) ⬆️
_1 32.07% <5.04%> (-0.53%) ⬇️
_2 55.34% <ø> (-0.05%) ⬇️
_3 ?
_4 35.37% <100.00%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Any modifications Copyright OpenSearch Contributors. See
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code in copy.ts and copy.test.ts are old, please use the code in the workspace-pr-integr branch.

Copy link
Author

Choose a reason for hiding this comment

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

Have reverted these codes

Comment on lines 1 to 10
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Any modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/*
* SPDX-License-Identifier: Apache-2.0
*
* The OpenSearch Contributors require contributions made to
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*
* Any modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

Copy link
Author

Choose a reason for hiding this comment

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

Done

if (duplicateMode === DuplicateMode.All) {
selectedObjects = selectedObjects.filter((item) => this.isSavedObjectTypeIncluded(item.type));
}
// If the target workspace is selected, all saved objects will be retained.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// If the target workspace is selected, all saved objects will be retained.
// If the target workspace is not selected, all saved objects will be retained.

Should be not selected?

Copy link
Author

Choose a reason for hiding this comment

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

Have modified it according to your suggestion.

>
<>
<EuiText size="s" color="subdued">
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems the text is not wrapped by i18n

Copy link
Collaborator

Choose a reason for hiding this comment

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

400 lines are a too large for a single component, can we split it?
Some recommendations:

  1. Move enum or types to separate files.
  2. Split some render logic into a small Component. -> renderDuplicateObjectCategory renderDuplicateObjectCategories.
  3. Consider to move some data transform function to util.

Copy link
Author

Choose a reason for hiding this comment

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

The code has been split to two files.

  1. The enum file has been moved to types.ts;
  2. The renderDuplicateObjectCategory and renderDuplicateObjectCategories function has been moved to a new tsx file.

@@ -31,19 +31,59 @@
import React from 'react';
import { shallow } from 'enzyme';
import { Header } from './header';
import { any } from 'bluebird';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { any } from 'bluebird';

Copy link
Author

Choose a reason for hiding this comment

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

Have modified it according to your suggestion.

const newIndexPatternUrl = applications.getUrlForApp('management', {
path: 'opensearch-dashboards/indexPatterns',
});
const newIndexPatternUrl = applications.getUrlForApp('indexPatterns');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const newIndexPatternUrl = applications.getUrlForApp('indexPatterns');
const newIndexPatternUrl = applications.getUrlForApp('management', {
path: 'opensearch-dashboards/indexPatterns',
});

Please do not change the index pattern url in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Have modified it according to your suggestion.

import { DataPublicPluginStart } from '../../../../../plugins/data/public';
import { Header, Table, Flyout, Relationships, SavedObjectsDuplicateModal } from './components';
import { DataPublicPluginStart } from '../../../../data/public';

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Copy link
Author

Choose a reason for hiding this comment

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

Have modified it according to your suggestion.

import { Header, Table, Flyout, Relationships, SavedObjectsDuplicateModal } from './components';
import { DataPublicPluginStart } from '../../../../data/public';

import { DuplicateMode } from './';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should import from ./components instead of ./index. Import from ./index makes the dependency chain confusing.

Comment on lines 238 to 239
jest.clearAllMocks();
jest.resetModules();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am thinking maybe we should only clear the needed mocks instead of clearAll.

Copy link
Author

Choose a reason for hiding this comment

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

It seems that we don't any afterEach code.

Copy link
Author

Choose a reason for hiding this comment

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

Have deleted the logic

@@ -78,14 +80,15 @@ export interface TableProps {
items: SavedObjectWithMetadata[];
itemId: string | (() => string);
totalItemCount: number;
onQueryChange: (query: any, filterFields: string[]) => void;
onQueryChange: (query: any, filterFields?: string[]) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems unrelated changes.

Copy link
Author

Choose a reason for hiding this comment

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

Have deleted it according to your suggestion.

import { Header, Table, Flyout, Relationships } from './components';
import { DataPublicPluginStart } from '../../../../../plugins/data/public';
import { Header, Table, Flyout, Relationships, SavedObjectsDuplicateModal } from './components';
import { DataPublicPluginStart } from '../../../../data/public';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import { DataPublicPluginStart } from '../../../../data/public';
import { DataPublicPluginStart } from '../../../../plugins/data/public';

better to avoid unnecessary change.

Copy link
Author

Choose a reason for hiding this comment

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

Have modified it according to your suggestion.

Signed-off-by: yubonluo <[email protected]>
Signed-off-by: yubonluo <[email protected]>
Copy link
Collaborator

@SuZhou-Joe SuZhou-Joe left a comment

Choose a reason for hiding this comment

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

Looks much better than the first version, good job!

@@ -38,12 +38,51 @@ describe('Header', () => {
onExportAll: () => {},
onImport: () => {},
onRefresh: () => {},
totalCount: 4,
onDuplicate: () => {},
title: 'Saved Objects',
Copy link
Collaborator

Choose a reason for hiding this comment

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

title is not a required props for header I think.

Copy link
Author

Choose a reason for hiding this comment

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

Have deleted the useless code.

Comment on lines 158 to 169
const targetWorkspaceId = targetWorkspaceOption?.at(0)?.key;
let selectedObjects = allSelectedObjects;
if (duplicateMode === DuplicateMode.All) {
selectedObjects = selectedObjects.filter((item) => this.isSavedObjectTypeIncluded(item.type));
}
// If the target workspace is not selected, all saved objects will be retained.
// If the target workspace has been selected, filter out the saved objects that belongs to the workspace.
const includedSelectedObjects = selectedObjects.filter((item) =>
!!targetWorkspaceId && !!item.workspaces ? !item.workspaces.includes(targetWorkspaceId) : true
);

const ignoredSelectedObjectsLength = selectedObjects.length - includedSelectedObjects.length;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extract to a method of the component maybe? It is hard to understand what these lines are doing.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, have extracted the logic to a method.

Copy link
Collaborator

@SuZhou-Joe SuZhou-Joe left a comment

Choose a reason for hiding this comment

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

LGTM

@SuZhou-Joe SuZhou-Joe merged commit 1a353cc into ruanyl:workspace-pr-integr Apr 16, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants