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

Enhance UX for managing credentials #2445

Closed
wants to merge 11 commits into from
Closed

Conversation

t1m0thyj
Copy link
Member

@t1m0thyj t1m0thyj commented Sep 6, 2023

Proposed changes

This PR refactors multiple context menu items:

  • Update Credentials
  • Log In to Authentication Service
  • Log Out of Authentication Service

into a single menu item "Manage Credentials" which shows a quickpick for users to choose an authentication method.

image

Release Notes

Milestone: 2.12.0

Changelog:

Replaced multiple options for updating credentials with a single "Manage Credentials" option that prompts for preferred authentication type. #2263

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

In addition to improving UX, this should make it easier to support certificates (#2373).

@t1m0thyj t1m0thyj force-pushed the feat/enhance-credentials-ux branch from 974fdd5 to 8c473af Compare September 7, 2023 14:40
@JillieBeanSim JillieBeanSim added this to the v2.12.0 milestone Sep 8, 2023
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Patch coverage is 97.72% of modified lines.

Files Changed Coverage
packages/zowe-explorer/src/utils/ProfilesUtils.ts 97.67%
...kages/zowe-explorer/src/dataset/ZoweDatasetNode.ts 100.00%

📢 Thoughts on this report? Let us know!.

@t1m0thyj t1m0thyj linked an issue Sep 11, 2023 that may be closed by this pull request
@t1m0thyj t1m0thyj force-pushed the feat/enhance-credentials-ux branch from 1ae2c08 to 502fee5 Compare September 11, 2023 21:38
@t1m0thyj t1m0thyj marked this pull request as ready for review September 11, 2023 21:46
traeok
traeok previously approved these changes Sep 14, 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, thanks @t1m0thyj 😁

@JillieBeanSim JillieBeanSim self-requested a review September 15, 2023 20:32
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.

this is looking great @t1m0thyj!!
I was wondering if the logout option should be a radio button too? maybe not since the others are showing what is in use. Should we use a separator line for logout then similar to add/create profile?
Screen Shot 2023-09-15 at 4 44 22 PM

I also notice that the quickpick didn't close after choosing the logout option, I have to escape it.
Screen Shot 2023-09-15 at 4 38 30 PM

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

No Coverage information No Coverage information
34.7% 34.7% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@JillieBeanSim JillieBeanSim self-requested a review September 18, 2023 15:21
JillieBeanSim
JillieBeanSim previously approved these changes Sep 18, 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 for pushing the latest updates, everything works great! Thanks @t1m0thyj for this enhancement

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.

When performing a token auth login, seems to be working well, I do have a concern in terms of the UI/UX for logging out in this change, should we show instead just a "logout" button or show the option for logout as a radio button too? i'm curious why it has a different UI for the logout.

Another thing I wanted to mention is that when clicking "logout" the pop up window with the option doesn't close but remains open, but the bottom right show that logout was done successfully, my feedback on this is I felt like maybe there is no feedback when clicking on the button that will tell the user that they logged out unless they look at the bottom right, I think maybe closing the pop up selection menu will help with this!

Screenshot 2023-09-18 at 11 53 03 AM

@rudyflores
Copy link
Contributor

One more thing @t1m0thyj

if you perform the following on a z/OSMF or extender profile:

  1. Log in with token auth
  2. click manage credentials and choose basic auth
  3. type in credentials for basic auth

This causes now the profile to be using both basic and token auth which now doesn't allow the user to logout unless the secure array properties with user and password are removed, I think we may not want to mix credential management types here but may be something we can discuss as well.

@t1m0thyj
Copy link
Member Author

@rudyflores Thanks for the feedback! 😍

When performing a token auth login, seems to be working well, I do have a concern in terms of the UI/UX for logging out in this change, should we show instead just a "logout" button or show the option for logout as a radio button too? i'm curious why it has a different UI for the logout.

The radio buttons are for authentication options (User and Password, Authentication Token, and in the future Certificate File). Since logout is an action separate from these, I don't think it should be a radio button. I agree a "logout" button would be nice but unfortunately the UI options provided by VS Code for quickpicks are limited so we can't show a button with text on its own row. I've mocked up some alternative ideas:

  1. Logout icon shown on the right of the quickpick - this is the only style of button they support. The button can show a tooltip when hovered, but I'm not sure if its obvious enough for users to easily find the "logout" option.
    image
  2. Adding an icon to the existing "logout" option - this is a small change but I think it looks better.
    image

Another thing I wanted to mention is that when clicking "logout" the pop up window with the option doesn't close but remains open, but the bottom right show that logout was done successfully, my feedback on this is I felt like maybe there is no feedback when clicking on the button that will tell the user that they logged out unless they look at the bottom right, I think maybe closing the pop up selection menu will help with this!

Agree that the popup should close after logout, did you test with the latest commit? There was an issue with the popup not being closed that Billie pointed out which should be fixed in 5a6fc3a

@JillieBeanSim JillieBeanSim dismissed their stale review September 21, 2023 14:49

due to concerns

@JillieBeanSim
Copy link
Contributor

Hey @t1m0thyj due to concerns with the authentication methods and pre-existing issues with offering basic auth info gathering while token auth in place and other issues I have started work on a POC for profile management based off of these ideas, keep in mind this isn't complete yet. We can continue discussions on how we would like to proceed

@t1m0thyj t1m0thyj mentioned this pull request Sep 26, 2023
16 tasks
@t1m0thyj
Copy link
Member Author

t1m0thyj commented Sep 26, 2023

Closing in favor of #2472

@t1m0thyj t1m0thyj closed this Sep 26, 2023
@JillieBeanSim JillieBeanSim deleted the feat/enhance-credentials-ux branch November 20, 2023 19:29
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.

Prevent users from storing wrong credential type for session
4 participants