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

fix: formatting of 401 error message #3319

Merged
merged 7 commits into from
Nov 21, 2024
Merged

fix: formatting of 401 error message #3319

merged 7 commits into from
Nov 21, 2024

Conversation

traeok
Copy link
Member

@traeok traeok commented Nov 14, 2024

Proposed changes

This won't need ported for 3.0.3 as this was a side effect of the error correlator

How to test

Same steps as in #3302:

  • Checkout this branch in the repo with VS Code opened
  • pnpm install && pnpm build
  • After the build has finished, click on the Run & Debug panel on the VS Code Side Bar
  • Select "Run VS Code Extension" from the tasks list at the top and select the Play ▶ button
  • Once opened, make sure that you have (or set) valid credentials on a profile in ZE
  • Execute a search on that profile and expand a folder so that a file/PDS member/spool item is visible
  • Before opening the file, right click on your profile -> "Manage Profile" -> "Update Credentials"
  • Enter in invalid credentials. I recommend using a fake username to prevent lockouts
  • Once finished, try to open the file. Notice the error in the editor appears but you are prompted to update your credentials.

Except this time, you'll see a nicer error message 😋

Release Notes

Milestone: 3.0.3

Changelog: No changelog since fix hasn't been published yet

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds or improves functionality)
  • Breaking change (a change that would cause existing functionality to not work as expected)
  • Documentation (Markdown, README updates)
  • Other (please specify above in "Proposed changes" section)

Checklist

General

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • All PR dependencies have been merged and published (if applicable)
  • A GIF or screenshot is included in the PR for visual changes
  • The pre-publish command has been executed:
    • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
    • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

  • There is coverage for the code that I have added
  • I have added new test cases and they are passing
  • I have manually tested the changes

Deployment

  • I have added developer documentation (if applicable)
  • Documentation should be added to Zowe Docs
    • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
    • Otherwise, please check with the rest of the squad about any needed documentation before merging.
  • These changes may need ported to the appropriate branches (list here):

Further comments

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.02%. Comparing base (9f5c87e) to head (c6716f1).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3319   +/-   ##
=======================================
  Coverage   93.02%   93.02%           
=======================================
  Files         116      116           
  Lines       12047    12049    +2     
  Branches     2730     2759   +29     
=======================================
+ Hits        11207    11209    +2     
  Misses        839      839           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@traeok traeok marked this pull request as ready for review November 14, 2024 23:37
Copy link

📅 Suggested merge-by date: 11/28/2024

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 looking into this issue some more @traeok. I am not seeing the 401 pop up message with these changes. I am getting an action bar across the top of the text editor file that just continues to run like the request is hanging.

@traeok
Copy link
Member Author

traeok commented Nov 19, 2024

thanks for looking into this issue some more @traeok. I am not seeing the 401 pop up message with these changes. I am getting an action bar across the top of the text editor file that just continues to run like the request is hanging.

No problem @JillieBeanSim, thanks for testing. Do you happen to have the steps for reproducing this issue? I know that there are still some scenarios where the 401 pop up will not occur, but per our discussion about prompting in standup, that will need resolved separately as part of #3329. This PR only changes the error message and not the behavior of the prompt itself.

@traeok traeok added the no-changelog Add to PR's that don't require a CHANGELOG update label Nov 20, 2024
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.

LGTM, thanks @traeok!

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! 😋

I do agree that there may be some specific scenarios where "401 pop-up prompts" don't show (#3329) which we can address

@traeok traeok merged commit 8cbc49b into main Nov 21, 2024
22 checks passed
@traeok traeok deleted the fix/401-err-msg branch November 21, 2024 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Add to PR's that don't require a CHANGELOG update size/S
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants