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

Feature copy datasets #2051

Merged
merged 58 commits into from
Mar 1, 2023
Merged

Feature copy datasets #2051

merged 58 commits into from
Mar 1, 2023

Conversation

KutluOzel-b
Copy link
Contributor

@KutluOzel-b KutluOzel-b commented Dec 20, 2022

Please see this comment for context: #2051 (comment)


Proposed changes

add feature copy datasets ( pds, sequential, members across pds) with multi select capabilities.

Release Notes

#1550

Changelog:

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)

Please see this comment for context: #2051 (comment)

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Patch coverage: 83.63% and project coverage change: +0.05 🎉

Comparison is base (98d51c5) 90.70% compared to head (035b7dc) 90.75%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2051      +/-   ##
==========================================
+ Coverage   90.70%   90.75%   +0.05%     
==========================================
  Files          86       86              
  Lines        8109     8245     +136     
  Branches     1712     1751      +39     
==========================================
+ Hits         7355     7483     +128     
- Misses        753      761       +8     
  Partials        1        1              
Impacted Files Coverage Δ
...ackages/zowe-explorer-api/src/globals/Constants.ts 100.00% <ø> (ø)
packages/zowe-explorer-api/src/globals/Gui.ts 97.01% <ø> (ø)
packages/zowe-explorer-api/src/globals/index.ts 100.00% <ø> (ø)
packages/zowe-explorer-api/src/index.ts 100.00% <ø> (ø)
...ckages/zowe-explorer-api/src/logger/IZoweLogger.ts 100.00% <ø> (ø)
packages/zowe-explorer-api/src/logger/index.ts 100.00% <ø> (ø)
...es/zowe-explorer-api/src/profiles/ProfilesCache.ts 99.55% <ø> (+0.44%) ⬆️
...ges/zowe-explorer-api/src/profiles/UserSettings.ts 100.00% <ø> (ø)
...-explorer-api/src/profiles/ZoweExplorerZosmfApi.ts 96.96% <0.00%> (-1.50%) ⬇️
packages/zowe-explorer-api/src/profiles/index.ts 100.00% <ø> (ø)
... and 78 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

Thanks for working on this feature @KutluOzel-b ! Just a couple things I've found while testing:

  • Since the "Copy Data Set" option brings up a dialog for the new dataset(s) names and doesn't require the user to initiate the paste operation, should the feature be renamed to something like "Duplicate data set(s)" ? Also, would it be possible to add a title to the input box to describe its intent, e.g. "New name for copy of 'DATASET1'" ? This would help provide some clarity to the end-user when the input box appears.
  • When right-clicking on data set members (PDS), the "Copy Member" option now says "Copy Data Set".
  • Trying to copy one PS data set, and providing the name of an existing PS data set as the new name, results in a z/OSMF REST API error:
    image

In this situation we could probably disregard z/OSMF API errors, or just reject the operation before making the API request if the new name already matches one of the existing datasets.

@KutluOzel-b
Copy link
Contributor Author

Thanks for working on this feature @KutluOzel-b ! Just a couple things I've found while testing:

  • Since the "Copy Data Set" option brings up a dialog for the new dataset(s) names and doesn't require the user to initiate the paste operation, should the feature be renamed to something like "Duplicate data set(s)" ? Also, would it be possible to add a title to the input box to describe its intent, e.g. "New name for copy of 'DATASET1'" ? This would help provide some clarity to the end-user when the input box appears.
  • When right-clicking on data set members (PDS), the "Copy Member" option now says "Copy Data Set".
  • Trying to copy one PS data set, and providing the name of an existing PS data set as the new name, results in a z/OSMF REST API error:
    image

In this situation we could probably disregard z/OSMF API errors, or just reject the operation before making the API request if the new name already matches one of the existing datasets.

Hey @traeok , Thanks for review .

I had a purpose while huddling copy features together which was make it more simple and let user control over the operation he/she does. I'm thinking having "copy member", "copy dataset", "duplicate datasets", "copy sequential datasets" is a not a good way of achieving good ux.
At this point it is maybe good to have more feedbacks from the community/team to continue.

Regarding the attempt of copying already existing a datasets ; I'm not sure there is an api for checking if a dataset with selected name is existing or not and dismissing api errors might be confusing considering operation is done or not.

@traeok
Copy link
Member

traeok commented Jan 3, 2023

I had a purpose while huddling copy features together which was make it more simple and let user control over the operation he/she does. I'm thinking having "copy member", "copy dataset", "duplicate datasets", "copy sequential datasets" is a not a good way of achieving good ux. At this point it is maybe good to have more feedbacks from the community/team to continue.

I agree that having too many different "Copy" operations would be redundant and confusing for the user - we could always stick with "Copy" here as the user is intentionally right-clicking on a dataset node anyway.

Regarding the attempt of copying already existing a datasets ; I'm not sure there is an api for checking if a dataset with selected name is existing or not and dismissing api errors might be confusing considering operation is done or not.

In hindsight it is probably more complicated to ensure there isn't an overlap in existing/new data set names, so we could always leave that up to the user.

Going to do some quick tests to make sure it is working as intended and then I will update the review 👍

@traeok traeok self-requested a review January 3, 2023 15:36
traeok
traeok previously approved these changes Jan 3, 2023
@JillieBeanSim JillieBeanSim added this to the v2.6.0 milestone Jan 9, 2023
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.

Thanks @KutluOzel-b this is working good. I do have a couple things I wanted to mention.
were we changing the Copy Dataset to just Copy? I remember the discussion and see paste was changed to just Paste. Also, I have to click on the data set (closing & reopening) to see the newly added files. Is there a way we can have this automatically refresh after paste is finished?

@zFernand0
Copy link
Member

Seems like the Theia tests broke after merging from main. 😢
Looking into it 👀

t1m0thyj
t1m0thyj previously approved these changes Feb 24, 2023
packages/zowe-explorer/src/dataset/actions.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/dataset/actions.ts Outdated Show resolved Hide resolved
@JillieBeanSim
Copy link
Contributor

Seems like the Theia tests broke after merging from main. 😢 Looking into it 👀

It could be from the Theia version update to 1.35.0

@zFernand0
Copy link
Member

Seems like the Theia tests broke after merging from main. 😢 Looking into it 👀

It could be from the Theia version update to 1.35.0

Seems like that's the case since replaying a successful run from main now fails
https://github.com/zowe/vscode-extension-for-zowe/actions/runs/4235493965/jobs/7420836048

traeok
traeok previously approved these changes Feb 27, 2023
Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

LGTM!

title: localize("paste.dataSet.InPrg", "Copying File(s)"),
},
() => {
return ZoweExplorerApiRegister.getMvsApi(nodes[0].getProfile()).copyDataSet(lbl, dsname, null, replace === "replace");
Copy link
Contributor

Choose a reason for hiding this comment

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

If copyDataSet hasn't been picked up by an extender yet we should have a check for this otherwise user will see messages like below.
We shouldn't expect extenders to add 'Not Supported' like I see in the FTP package here, it should be handled by the ZE extension like we have in the past where if the particular API didn't exist we either

  1. handle it the old way if this is a new way
  2. return the error of this action not being supported if the new api doesn't exist

Screen Shot 2023-02-27 at 11 40 19 AM

Copy link
Member

Choose a reason for hiding this comment

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

And error message will appear indicating that Copying datasets is not supported
However, that leaves the question of whether we want to prevent the allocateLike from happening or not.

As it stands, I believe the new Copy/Paste experience is a little better than before.

Do we want to address the AllocateLike (and possibly other misbehaviors that we haven't found yet) in this PR?

(Personal opinion)
I have considered going with a similar approach as in #2082 where we rely on the clipboard instead of performing a complex, multi-step, error-prone, user interaction for such functionality.
However, some design decisions need to be discussed. For example, the USS file system may not require allocation of resources.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Allocate like can be a future PR

traeok
traeok previously approved these changes Feb 28, 2023
Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

LGTM, check was added for the API function 👍 thanks @zFernand0

Signed-off-by: zFernand0 <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

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.

thanks for the updates @zFernand0 LGTM

@JillieBeanSim JillieBeanSim merged commit 32e0d07 into main Mar 1, 2023
@JillieBeanSim JillieBeanSim deleted the feature/copy/dsMulti branch March 1, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy Paste Multiple Datasets
5 participants