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

Profile management #2472

Merged
merged 41 commits into from
Oct 10, 2023
Merged

Profile management #2472

merged 41 commits into from
Oct 10, 2023

Conversation

JillieBeanSim
Copy link
Contributor

@JillieBeanSim JillieBeanSim commented Sep 21, 2023

Proposed changes

This is an update for profile management that we can build from to eliminate issues we have seen users face with different authentication methods

right-click menu
Screen Shot 2023-10-02 at 6 51 05 PM

basic auth:
Screen Shot 2023-10-02 at 7 05 23 PM

token auth:
Screen Shot 2023-10-02 at 6 52 53 PM

Screen Shot 2023-10-02 at 6 53 43 PM

no auth specified:
Screen Shot 2023-10-02 at 6 51 21 PM

Screen Shot 2023-10-02 at 6 51 39 PM

Release Notes

Milestone: 2.12.0

Changelog: Introduce a new user interface for managing profiles via right-click action "Manage Profile".

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)

Further comments

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Files Coverage Δ
packages/zowe-explorer/src/globals.ts 98.61% <100.00%> (ø)
packages/zowe-explorer/src/utils/ProfilesUtils.ts 91.48% <100.00%> (+0.15%) ⬆️
packages/zowe-explorer/src/shared/init.ts 96.59% <50.00%> (-1.31%) ⬇️
packages/zowe-explorer/src/Profiles.ts 84.16% <80.00%> (-0.03%) ⬇️
...kages/zowe-explorer/src/utils/ProfileManagement.ts 96.59% <96.59%> (ø)

📢 Thoughts on this report? Let us know!.

@JillieBeanSim JillieBeanSim self-assigned this Sep 21, 2023
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
@JillieBeanSim JillieBeanSim marked this pull request as ready for review September 22, 2023 21:30
@zFernand0
Copy link
Member

zFernand0 commented Sep 30, 2023

Dumb question:
Should "all things profile" be inside the "Manage Profile" option?
i.e. should we move "Hide Profile" and Disable/Enable Validation" inside the quick-pick menu?

I know it is an extra click, and some people may find it a bit annoying, but it might help clean up the Context menu even further. 😋

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.

  • Should we remove the login option for service profiles that have the "user" and "password" in the secure array?
    May sound odd, but I was sort of expecting that option not to be there.

  • When I click on the login option, and I cancel out of the user/password prompts, the "login successful message shows up... Should that be removed?
    image

  • Should the logout operation be removed when there is no tokenValue stored in the vault? otherwise the user might get this error message.
    image

  • This one might be on the main branch already and does not cause any error messages (other than the "Element with ID XYZ is already registered).

    • When we refresh the extension via the command palette, should we collapse all profiles?
      image

@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Oct 2, 2023

@zFernand0 addressed feedback except the element already registered error that exists on main and is way out of scope for this PR. The concern about the logout option being presented, that option is only presented when the tokenType exists since that exists only when logged in and removed upon logout. The tokenValue in the vault check is reused from Rudy's work to determine if the profile should have login option and no basic auth options.

@JillieBeanSim JillieBeanSim dismissed stale reviews from zFernand0 and t1m0thyj October 2, 2023 23:00

changes made

@JillieBeanSim
Copy link
Contributor Author

A heads up with moving the validation and hiding options into menu. These options only work in the treeview the action is initiated in, same as the main branch. We could do follow up work to make these work across all 3 trees in the future.

@traeok
Copy link
Member

traeok commented Oct 3, 2023

addressed feedback except the element already registered error that exists on main and is way out of scope for this PR.

Just for awareness, this error was resolved in #2481 and will go away once maintenance changes are merged into main 👍

zFernand0
zFernand0 previously approved these changes Oct 3, 2023
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.

Changes LGTM! 😋
Thanks for addressing the comments.


The concern about the logout option being presented, that option is only presented when the tokenType exists since that exists only when logged in and removed upon logout.

  • Do we plan to possibly update this if we decide not to remove tokenType on logout?

The tokenValue in the vault check is reused from Rudy's work to determine if the profile should have login option and no basic auth options.

  • Would it be possible to hook into that work to determine whether or not to should the logout option?

I found something kind of odd (which also happens on main and the marketplace)

When clicking the search icon on an APIML profile, it invalidates the profile with the following errors
image

And when clicking a ZOSMF profile, it asks for username and password. And if you cancel out of the prompt, you still get prompted for the search filter (dataset pattern, ...)

@JillieBeanSim
Copy link
Contributor Author

JillieBeanSim commented Oct 3, 2023

Hey @zFernand0 thanks for re-testing. Would you mind creating a bug issue for the errors mentioned that are pre-existing?
For the question about us having to adjust for behavior changes, of course we will have to update for new behavior but the behavior change will have to come with v3 or another major release if it's a breaking change.

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Description on some rows ends with a period, but not for other rows. Can we make this consistent? IMO without periods looks better 😋
image

Also curious if we should move "Disable Profile Validation" down further in the list? Although it's good that we have it, I don't think this option is used very often compared to other ones like "Edit Profile".

Signed-off-by: Billie Simmons <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 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
1.4% 1.4% Duplication

Copy link
Contributor

@rudyflores rudyflores 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 for this enhancement @JillieBeanSim love the new icons :))

@JillieBeanSim JillieBeanSim merged commit ccccc65 into main Oct 10, 2023
24 checks passed
@JillieBeanSim JillieBeanSim deleted the profile-management branch October 10, 2023 11:14
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.

5 participants