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

Multiple delete of datasets/members #1323

Merged
merged 15 commits into from Jun 24, 2021
Merged

Multiple delete of datasets/members #1323

merged 15 commits into from Jun 24, 2021

Conversation

katelynienaber
Copy link
Contributor

@katelynienaber katelynienaber commented May 19, 2021

Proposed changes

Does what it says on the tin.

  • Select a bunch of data sets & members
  • Hit your Delete button on the keyboard
  • Confirm you want to delete
  • The data sets/members you selected are gone
  • If you selected favorites or profiles, they won't disappear

Release Notes

Milestone: Backlog

Changelog: Added multi-delete capability for data sets and members

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber katelynienaber added this to the Backlog milestone May 19, 2021
@katelynienaber katelynienaber self-assigned this May 19, 2021
Also rearranged some integration tests, to match new refresh function
location & name of Dataset actions file

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber katelynienaber changed the title Multiple delete for datasets/members Multiple delete of datasets/members May 20, 2021
katelynienaber and others added 3 commits May 20, 2021 15:39
love of potato

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber katelynienaber marked this pull request as ready for review May 25, 2021 09:11
Copy link
Contributor

@crawr crawr left a comment

Choose a reason for hiding this comment

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

The delete works well for me with the keyboard option, but I noticed that it did not work when I tried to use the context menu Delete Data Set to delete multiple datasets and members.
Is this as expected ? Can we wrap the existing delete command to also perform multiple deletion?

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber
Copy link
Contributor Author

@crawr I forgot to check this! It should be fixed now.

@crawr crawr requested a review from lauren-li June 1, 2021 14:39
@JillieBeanSim JillieBeanSim self-requested a review June 2, 2021 19:58
Copy link
Contributor

@JillieBeanSim JillieBeanSim left a comment

Choose a reason for hiding this comment

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

@katelynienaber I am in love with this new functionality!!! I have needed to cleanup my data sets list for some time now but deleting them 1 by 1 seemed so tedious lol.
I do have a simple request about the information pop up where it says node, could we change the wording of that to maybe The following were deleted ... or something similar?
Also, we have some failing integration tests that deal with deleting data sets that should get fixed with this PR
image

I am also seeing the following integration tests fail on this branch and not master so I wonder if the CLI version bump is causing another regression that will need to be addressed in a different High Priority issue.
UPDATE/EDIT: I got confused with PRs I was testing, this PR shouldn't have the CLI bump correct? Wonder why the following are failing.
image

Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber
Copy link
Contributor Author

katelynienaber commented Jun 3, 2021

@JillieBeanSim For the deletion text, is The following items were deleted acceptable?

Also I removed those 2 failing deleteDataset integration tests. There is no more confirmation dialog in function deleteDataset, I have moved it into the new function deleteDatasetPrompt. That function already has unit tests covering the cancellation dialog.

@katelynienaber
Copy link
Contributor Author

@JillieBeanSim I am still getting the 2nd integration test errors, is it a problem for you as well?

@JillieBeanSim
Copy link
Contributor

JillieBeanSim commented Jun 3, 2021

@katelynienaber I am still seeing those tests that deal with patterns fail and only on this branch do I see this issue. These tests directly follow the delete test you have and they use the same testTree so I wonder if the delete is altering the testTree for the following tests.
extension.integration.test.ts

crawr
crawr previously approved these changes Jun 4, 2021
Copy link
Contributor

@crawr crawr left a comment

Choose a reason for hiding this comment

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

Great work on this @katelynienaber, thanks!

packages/zowe-explorer/src/dataset/actions.ts Show resolved Hide resolved
@crawr crawr removed this from the Backlog milestone Jun 7, 2021
@katelynienaber
Copy link
Contributor Author

@JillieBeanSim I've fixed the integration tests, please take a look if you have time ^^

@JillieBeanSim
Copy link
Contributor

@katelynienaber I am still getting the failures on this branch, maybe someone else could run them also and make sure it's not just my. I know these tests can be finnicky at times.
image

@jellypuno
Copy link
Contributor

I have them as well.

@katelynienaber
Copy link
Contributor Author

@JillieBeanSim @jellypuno Could you give me more info about the failures? Because they are passing for me:
image

@jellypuno
Copy link
Contributor

Can you show me your testProfileData.ts? Mask your credentials

@zowe zowe deleted a comment from katelynienaber Jun 9, 2021
@jellypuno
Copy link
Contributor

@katelynienaber I'm having an error here const childrenFromTree = await sessionNode.getChildren(); 404 error for CREATE.DATASET

@jellypuno
Copy link
Contributor

I am running the pre-test and post-test before executing integration

@jellypuno
Copy link
Contributor

@JillieBeanSim Katelyn and I have been investigating this one and it is a really tricky issue. I think we would need more time to investigate. If it is okay with you, Can we merge this PR and then follow-up on a fix for that integration test?

@JillieBeanSim
Copy link
Contributor

@JillieBeanSim Katelyn and I have been investigating this one and it is a really tricky issue. I think we would need more time to investigate. If it is okay with you, Can we merge this PR and then follow-up on a fix for that integration test?

@jellypuno @katelynienaber that should be fine. I will test functionality again this afternoon and if none of that changed from before throw my approval on it. Should an issue be created for handling the integration tests or just wing it?

@jellypuno
Copy link
Contributor

@katelynienaber please open an issue for the integration for tracking.

@katelynienaber
Copy link
Contributor Author

katelynienaber commented Jun 10, 2021

@jellypuno @JillieBeanSim here is the issue for the integration tests. I will try to get to them in that weird 4-day week I have after this sprint, before I go on vacation. If nothing else comes up.

@JillieBeanSim
Copy link
Contributor

@katelynienaber deleting multiple data sets works well with right-click and keyboard , but multiple delete of members doesn't remove the members from the tree until the data set is closed then re-opened. I see these errors below in the debug console for each member
image

image

@crawr crawr removed this from the 1.16.0 Release milestone Jun 10, 2021
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
Signed-off-by: Katelyn Nienaber <katelyn.nienaber@broadcom.com>
@katelynienaber
Copy link
Contributor Author

@JillieBeanSim I added a fix for this, please take a look ^^

@JillieBeanSim
Copy link
Contributor

Thanks @katelynienaber! All LGTM with the update to handle the members being removed from the tree as well during the deletion. This multi-delete action is so awesome!! Thanks for getting this done for us

@crawr crawr added this to the 1.17.0 Release milestone Jun 24, 2021
Copy link
Contributor

@crawr crawr left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @katelynienaber

@crawr crawr merged commit d4068ed into master Jun 24, 2021
@crawr crawr deleted the multi-delete branch June 24, 2021 13:25
@katelynienaber katelynienaber restored the multi-delete branch July 27, 2021 13:59
@zFernand0 zFernand0 deleted the multi-delete branch February 11, 2022 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants