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: add support for deleting profiles #689

Merged
merged 21 commits into from
Apr 27, 2020
Merged

feat: add support for deleting profiles #689

merged 21 commits into from
Apr 27, 2020

Conversation

crawr
Copy link
Contributor

@crawr crawr commented Apr 15, 2020

Addresses:

Fix #332 - Support deleting a profile

Details:

  • New feature: adds deleteProfile() under Profiles which can be executed via context menu and command palette
  • The function makes use of the CliProfileManager to perform the delete
  • Via the command palette, the user has to choose from the list of all profiles
  • All the trees are updated to remove the successfully deleted profile
  • Favorites related to the profile deleted are cleaned up
  • "Remove Profile" context menu option is now called "Hide Profile" to avoid confusion

Release Notes:

Milestone: v1.5
Changelog: Provide ability to delete profiles via the explorer.

crawr and others added 9 commits April 14, 2020 10:53
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: jellypuno <jessielaine.punongbayan@broadcom.com>
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: jellypuno <jessielaine.punongbayan@broadcom.com>
@crawr crawr added this to the 1.5 Release milestone Apr 15, 2020
@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #689 into master will decrease coverage by 1.94%.
The diff coverage is 93.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
- Coverage   92.17%   90.22%   -1.95%     
==========================================
  Files          47       44       -3     
  Lines        4764     3847     -917     
  Branches     1003      658     -345     
==========================================
- Hits         4391     3471     -920     
- Misses        370      373       +3     
  Partials        3        3              
Impacted Files Coverage Δ
src/extension.ts 86.28% <55.55%> (-8.40%) ⬇️
src/Profiles.ts 93.79% <96.15%> (-1.63%) ⬇️
src/PersistentFilters.ts 98.95% <100.00%> (+0.01%) ⬆️
src/dataset/actions.ts 85.63% <0.00%> (-3.31%) ⬇️
src/api/ZoweExplorerApiRegister.ts 91.37% <0.00%> (-2.86%) ⬇️
src/dataset/DatasetTree.ts 89.36% <0.00%> (-2.65%) ⬇️
src/uss/actions.ts 80.13% <0.00%> (-2.03%) ⬇️
src/job/ZosJobsProvider.ts 87.14% <0.00%> (-1.49%) ⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aedefcf...0d6cb4d. Read the comment docs.

crawr added 2 commits April 16, 2020 14:54
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
@crawr crawr marked this pull request as ready for review April 16, 2020 18:59
crawr and others added 3 commits April 16, 2020 21:14
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: jellypuno <jessielaine.punongbayan@broadcom.com>
@crawr crawr added the Profiles label Apr 17, 2020
@crawr crawr self-assigned this Apr 17, 2020
Copy link
Member

@Colin-Stone Colin-Stone left a comment

Choose a reason for hiding this comment

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

Neat function and made a lot of sense changing the remove to hide. Can you just check that settings.json is being cleaned up completely please?.
I noticed some traces of a deleted profile in the sessions and recall section but the views were fine. Thank you.

@katelynienaber
Copy link
Contributor

Yes, works great for me :) Except for what Colin mentioned ofc

Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
@katelynienaber
Copy link
Contributor

katelynienaber commented Apr 23, 2020

Heyoo, 3 things V:

  1. The recall list isn't updated when I delete a profile
  2. If I delete my default profile, there are a bunch of errors related to not having a default ZOSMF profile. Is there a way to set the default through the graphical interface?
  3. If I delete my default/all profiles, the + button doesn't work to add a profile back again. I had to use the CLI to do it.

Colin-Stone
Colin-Stone previously approved these changes Apr 23, 2020
Copy link
Member

@Colin-Stone Colin-Stone left a comment

Choose a reason for hiding this comment

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

Thanks Richelle this all seems to working neatly now. Thank you for cleaning up the recall section. The sessions that were left over was a throwback to not having cleaned up some old sessions that had been left behind.
It may be worth quickly running npm run vscode:prepublish and committing Package.json and the i18n file get changed slightly. I don't know if the linter on the pipeline would flag it otherwise

@VitGottwald
Copy link
Contributor

@crawr I am just testing this branch and I am not able to list files in a filter. After a long wait I get a 500 error.

Screenshot 2020-04-24 at 11 30 34

Copy link
Contributor

@VitGottwald VitGottwald left a comment

Choose a reason for hiding this comment

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

Made some stylistic comments to make stuff easier to read. When the problem with fetching datasets is fixed, will be happy to approve. If you want some help with the debugging, plese, let me know!

__tests__/__unit__/Profiles.unit.test.ts Show resolved Hide resolved
__tests__/__unit__/extension.unit.test.ts Outdated Show resolved Hide resolved
i18n/sample/package.i18n.json Show resolved Hide resolved
src/Profiles.ts Outdated Show resolved Hide resolved
src/Profiles.ts Show resolved Hide resolved
src/Profiles.ts Outdated Show resolved Hide resolved
src/Profiles.ts Outdated

// Delete from Data Set Favorites
datasetTree.mFavorites.forEach((ses) => {
const findNode = ses.label.substring(1, ses.label.indexOf("]")).trim();
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like trouble going forward 😁 But I do not know how to do it better at this point.

src/Profiles.ts Outdated Show resolved Hide resolved
src/Profiles.ts Outdated Show resolved Hide resolved
src/Profiles.ts Outdated Show resolved Hide resolved
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
src/Profiles.ts Outdated Show resolved Hide resolved
src/Profiles.ts Outdated Show resolved Hide resolved
src/Profiles.ts Outdated Show resolved Hide resolved
@crawr
Copy link
Contributor Author

crawr commented Apr 27, 2020

Thanks for the reviews!

  • If I delete my default profile, there are a bunch of errors related to not having a default ZOSMF profile. Is there a way to set the default through the graphical interface?
  • If I delete my default/all profiles, the + button doesn't work to add a profile back again. I had to use the CLI to do it.

This looks like a regression in the handling of the Profile loader. I fixed it so that the extension will still load even if the default profile is not set. An informational message will appear instead.
Note that this does not have impact on the case for new users that do not have any profiles at all.
In both cases, the next profile the user will create will be set as default in the meta yaml file.

It may be worth quickly running npm run vscode:prepublish and committing Package.json and the i18n file get changed slightly. I don't know if the linter on the pipeline would flag it otherwise

Thanks for the tip, I ran the script and included the i18n file as a result.

I am just testing this branch and I am not able to list files in a filter. After a long wait I get a 500 error.

I tested listing with my just my HLQ using regular DS filter search and Ctrl + Alt + P and it loaded the entire list without any problems. Please let me know if you were able to replicate it after the new commit.

@VitGottwald
Copy link
Contributor

Checked the latest commit d4ae34d9609494fb354 and was able to list datasets. Then tried remove the (only) profile by right-clicking it and selecting Delete Profile
Screenshot 2020-04-27 at 12 42 05.

Got an error message as a result
Screenshot 2020-04-27 at 12 36 13

@jellypuno
Copy link
Contributor

Checked the latest commit d4ae34d9609494fb354 and was able to list datasets. Then tried remove the (only) profile by right-clicking it and selecting Delete Profile
Screenshot 2020-04-27 at 12 42 05.

Got an error message as a result
Screenshot 2020-04-27 at 12 36 13

@VitGottwald Are you sure? I just tried it now, I am not experiencing this error.

@VitGottwald
Copy link
Contributor

Checked the latest commit d4ae34d9609494fb354 and was able to list datasets. Then tried remove the (only) profile by right-clicking it and selecting Delete Profile
Screenshot 2020-04-27 at 12 42 05.
Got an error message as a result
Screenshot 2020-04-27 at 12 36 13

@VitGottwald Are you sure? I just tried it now, I am not experiencing this error.

Thanks @jellypuno . Looks like it was a problem on my end. After recompiling and deleting/adding profile from CLI I am not getting the error anymore. 👍

crawr added 2 commits April 27, 2020 16:25
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
Signed-off-by: Richelle Anne Craw <richelleanne.craw@broadcom.com>
@jellypuno jellypuno self-requested a review April 27, 2020 15:12
jellypuno
jellypuno previously approved these changes Apr 27, 2020
Copy link
Contributor

@jellypuno jellypuno left a comment

Choose a reason for hiding this comment

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

Demo'ed it today in Zowe Playback. Both commands works great!

@@ -153,6 +153,16 @@ export async function activate(context: vscode.ExtensionContext): Promise<ZoweEx
}
});
}
if (datasetProvider || ussFileProvider || jobsProvider) {
Copy link
Member

Choose a reason for hiding this comment

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

Not worth adding this so close to v1.5 but the three trees have a superclass ZoweTreeProvider so you could have ZoweTreeProvider.
No action required.

Colin-Stone
Colin-Stone previously approved these changes Apr 27, 2020
Copy link
Member

@Colin-Stone Colin-Stone 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 this functionality Richelle

src/Profiles.ts Outdated
Comment on lines 527 to 553
// Delete from USS Sessions list
const ussSetting: any = {...vscode.workspace.getConfiguration().get(this.ussSchema)};
let sessUSS: string[] = ussSetting.sessions;
let faveUSS: string[] = ussSetting.favorites;
sessUSS = sessUSS.filter( (element) => {
return element.trim() !== deleteLabel;
});
faveUSS = faveUSS.filter( (element) => {
return element.substring(1, element.indexOf("]")).trim() !== deleteLabel;
});
ussSetting.sessions = sessUSS;
ussSetting.favorites = faveUSS;
await vscode.workspace.getConfiguration().update(this.ussSchema, ussSetting, vscode.ConfigurationTarget.Global);

// Delete from Jobs Sessions list
const jobsSetting: any = {...vscode.workspace.getConfiguration().get(this.jobsSchema)};
let sessJobs: string[] = jobsSetting.sessions;
let faveJobs: string[] = jobsSetting.favorites;
sessJobs = sessJobs.filter( (element) => {
return element.trim() !== deleteLabel;
});
faveJobs = faveJobs.filter( (element) => {
return element.substring(1, element.indexOf("]")).trim() !== deleteLabel;
});
jobsSetting.sessions = sessJobs;
jobsSetting.favorites = faveJobs;
await vscode.workspace.getConfiguration().update(this.jobsSchema, jobsSetting, vscode.ConfigurationTarget.Global);
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a lot of knowledge duplication here. How about this:

Suggested change
// Delete from USS Sessions list
const ussSetting: any = {...vscode.workspace.getConfiguration().get(this.ussSchema)};
let sessUSS: string[] = ussSetting.sessions;
let faveUSS: string[] = ussSetting.favorites;
sessUSS = sessUSS.filter( (element) => {
return element.trim() !== deleteLabel;
});
faveUSS = faveUSS.filter( (element) => {
return element.substring(1, element.indexOf("]")).trim() !== deleteLabel;
});
ussSetting.sessions = sessUSS;
ussSetting.favorites = faveUSS;
await vscode.workspace.getConfiguration().update(this.ussSchema, ussSetting, vscode.ConfigurationTarget.Global);
// Delete from Jobs Sessions list
const jobsSetting: any = {...vscode.workspace.getConfiguration().get(this.jobsSchema)};
let sessJobs: string[] = jobsSetting.sessions;
let faveJobs: string[] = jobsSetting.favorites;
sessJobs = sessJobs.filter( (element) => {
return element.trim() !== deleteLabel;
});
faveJobs = faveJobs.filter( (element) => {
return element.substring(1, element.indexOf("]")).trim() !== deleteLabel;
});
jobsSetting.sessions = sessJobs;
jobsSetting.favorites = faveJobs;
await vscode.workspace.getConfiguration().update(this.jobsSchema, jobsSetting, vscode.ConfigurationTarget.Global);
// Delete from Session List
const isNotDeletedSession = (element) => element.trim() !== deleteLabel;
const isNotDeletedFavorite = (element) =>
element.substring(1, element.indexOf("]")).trim() !== deleteLabel;
const deleteFromSessionList = async (schema) => {
const setting: {
settings: string[];
favorites: string[];
} = vscode.workspace.getConfiguration().get(schema);
await vscode.workspace.getConfiguration().update(
schema,
{
...setting,
session: setting.favorites.filter(isNotDeletedSession),
favorites: setting.favorites.filter(isNotDeletedFavorite),
},
vscode.ConfigurationTarget.Global
);
};
deleteFromSessionList(this.dsSchema);
deleteFromSessionList(this.ussSchema);
deleteFromSessionList(this.jobsSchema);

VitGottwald
VitGottwald previously approved these changes Apr 27, 2020
Copy link
Contributor

@VitGottwald VitGottwald left a comment

Choose a reason for hiding this comment

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

I think there is room to extract pieces of knowledge like checking if an item is to be removed into functions and then reusing it. That would make the code more concise and "dry". But I think that is not a reason to block this PR.

Great job identifying all the places that need to be updated for a delete!

…-zowe into delete-profiles

Signed-off-by: zFernand0 <fernando.rijocedeno@broadcom.com>

# Conflicts:
#	__mocks__/@zowe/imperative.ts
#	__tests__/__unit__/Profiles.unit.test.ts
#	__tests__/__unit__/extension.unit.test.ts
#	src/Profiles.ts
@zFernand0 zFernand0 dismissed stale reviews from VitGottwald, Colin-Stone, and jellypuno via a886ca2 April 27, 2020 18:25
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Nice work!
Initially was getting a few timeouts on jest hooks. Maybe we should increment the default timeout on before/after hooks if we expect to perform some zowe action on them?

@zFernand0
Copy link
Member

It had 2 or more approvals before resolving merge conflicts.
Merging!

@zFernand0 zFernand0 merged commit 33d8e87 into master Apr 27, 2020
@zFernand0 zFernand0 deleted the delete-profiles branch April 27, 2020 19:23
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.

Support deleting a profile
6 participants