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

[MD] Update delete credentials method to be in parallel #2076

Conversation

noCharger
Copy link
Contributor

@noCharger noCharger commented Aug 4, 2022

Signed-off-by: Louis Chu [email protected]

Description

  1. Update delete credentials method to be in parallel rather than sequentially.
  2. Add error handling and toast logic on listing page

Screen Shot 2022-08-04 at 7 03 36 PM

Issues Resolved

Part 1 of #2055

Check List

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

@noCharger noCharger requested a review from a team as a code owner August 4, 2022 23:31
@noCharger noCharger changed the title Delete credentials in parallel [MD] Delete credentials in parallel Aug 4, 2022
@noCharger noCharger self-assigned this Aug 4, 2022
@noCharger noCharger marked this pull request as draft August 4, 2022 23:32
@noCharger noCharger marked this pull request as ready for review August 5, 2022 00:28
@seraphjiang seraphjiang requested a review from a team August 5, 2022 00:31
@seraphjiang seraphjiang added the multiple datasource multiple datasource project label Aug 5, 2022
@noCharger noCharger force-pushed the cm-delete-credentials-in-parallel branch from ff553c9 to bba6d1f Compare August 5, 2022 02:05
@noCharger noCharger requested a review from seraphjiang August 5, 2022 02:08
@noCharger noCharger added the ux / ui Improvements or additions to user experience, flows, components, UI elements label Aug 5, 2022
@KrooshalUX
Copy link

@noCharger
Copy link
Contributor Author

The existing delete placement pattern for the button that appears after selection is to the left of the search field within a table.

Please see: https://playground.opensearch.oss.aws.dev/app/dashboards#/list?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now)) https://playground.opensearch.oss.aws.dev/app/visualize#/?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))

Screen Shot 2022-08-05 at 10 53 56 AM

To provide additional context, the saved object management portal looks like this, complete with a delete button on right side. I'm looking forward to the decision.

Screen Shot 2022-08-05 at 11 03 37 AM

@KrooshalUX
Copy link

Thank you for pointing out this additional implementation of delete.
The key commonality here is that the delete is in alignment with the search bar, separate from any sort of create / primary action.

To your point, lets go for consistency within the category of Stack Management, so following the pattern for Saved Objects is a good path forward.

Therefore, it would look like this, so a correction to the placement is still needed so it is not next to the Create button:
(This is a rough quick mock, please ignore spacing issues)
Screen Shot 2022-08-05 at 11 26 03 AM

@noCharger
Copy link
Contributor Author

(This is a rough quick mock, please ignore spacing issues)

LGTM, @yibow98 is working on the button alignment on #2055

@kgcreative
Copy link
Member

Note:
Singular: Delete 1 credential
Plural: Delete 3 credentials

Copy should be sentence cased.

@seraphjiang
Copy link
Member

@noCharger
the batch execution will take time more than seconds, have we added code to handle in progress spin?
image

Copy link
Member

@seraphjiang seraphjiang left a comment

Choose a reason for hiding this comment

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

  1. add confirmation before deletion
  2. refactory code to be more clear, make sure decouple ui and code logical for testibility.

@noCharger noCharger changed the title [MD] Delete credentials in parallel [MD] Method for delete credentials in parallel Aug 5, 2022
@noCharger noCharger changed the title [MD] Method for delete credentials in parallel [MD] Update delete credentials method to be in parallel Aug 5, 2022
@noCharger
Copy link
Contributor Author

Note: Singular: Delete 1 credential Plural: Delete 3 credentials

Copy should be sentence cased.

Tracked on #2055

@noCharger
Copy link
Contributor Author

@noCharger the batch execution will take time more than seconds, have we added code to handle in progress spin? image

Tracked on #2055

@noCharger
Copy link
Contributor Author

2. refactory code to be more clear, make sure decouple ui and code logical for testibility.

Could you please explain which parts of the changed files require refactoring?

@noCharger noCharger requested a review from seraphjiang August 5, 2022 21:41
@seraphjiang
Copy link
Member

  1. refactory code to be more clear, make sure decouple ui and code logical for testability.

Could you please explain which parts of the changed files require refactoring?

expected an end to end production ready solution for deletion. or rewrite this to meet expectation on code quality.

@noCharger noCharger closed this Aug 5, 2022
@noCharger noCharger reopened this Aug 5, 2022
@noCharger noCharger closed this Aug 5, 2022
Comment on lines +47 to +51
await Promise.all(
selectedCredentials.map(async (selectedCredential) => {
await savedObjectsClient.delete('credential', selectedCredential.id);
})
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multiple datasource multiple datasource project ux / ui Improvements or additions to user experience, flows, components, UI elements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants