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

[PM-9693] Refresh - LoginDecryptionOptionsComponent #11782

Merged
merged 37 commits into from
Nov 21, 2024

Conversation

rr-bw
Copy link
Contributor

@rr-bw rr-bw commented Oct 29, 2024

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-9693

📔 Objective

Creates a refreshed and consolidated LoginDecryptionOptionsComponent for use on all visual clients, which will be used when the UnauthenticatedExtensionUIRefresh is on.

Notes

📸 Screenshots

Web - Admin Approval

web-admin-approval.mov

Browser - Admin Approval

extension-admin-approval.mov

Desktop - Admin Approval

desktop-admin-approval.mov

Web - Approve with MP

Uploading web-use-master-password.mov…

Web - Approve from other device

web-approve-with-other-device.mov

Web - New User

web-new-user.mov

Error Case - User Email Could Not Be Retrieved

desktop-email-not-found-error.mov

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 16.37427% with 143 lines in your changes missing coverage. Please review.

Project coverage is 33.42%. Comparing base (5873f0e) to head (29e0745).
Report is 4 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...tion-options/login-decryption-options.component.ts 0.00% 123 Missing ⚠️
libs/auth/src/angular/index.ts 0.00% 3 Missing ⚠️
...n-options/login-decryption-options-v1.component.ts 0.00% 2 Missing ⚠️
apps/browser/src/popup/services/services.module.ts 0.00% 2 Missing ⚠️
...n-options/login-decryption-options-v1.component.ts 0.00% 2 Missing ⚠️
...n-options/login-decryption-options-v1.component.ts 0.00% 2 Missing ⚠️
apps/browser/src/popup/app-routing.module.ts 0.00% 1 Missing ⚠️
apps/browser/src/popup/app.module.ts 0.00% 1 Missing ⚠️
apps/desktop/src/app/app-routing.module.ts 0.00% 1 Missing ⚠️
apps/desktop/src/auth/login/login.module.ts 0.00% 1 Missing ⚠️
... and 5 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11782      +/-   ##
==========================================
- Coverage   33.45%   33.42%   -0.03%     
==========================================
  Files        2865     2871       +6     
  Lines       89650    89808     +158     
  Branches    17070    17103      +33     
==========================================
+ Hits        29993    30021      +28     
- Misses      57295    57425     +130     
  Partials     2362     2362              

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


🚨 Try these New Features:

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Logo
Checkmarx One – Scan Summary & Detailsdf8acc56-9399-4b3a-a8b7-7770bff301f0

Fixed Issues

Severity Issue Source File / Package
LOW Client_JQuery_Deprecated_Symbols /apps/cli/src/service-container/service-container.ts: 876

@rr-bw rr-bw marked this pull request as ready for review November 7, 2024 23:31
@rr-bw rr-bw requested a review from a team as a code owner November 7, 2024 23:31
@rr-bw rr-bw requested a review from a team as a code owner November 7, 2024 23:31
dani-garcia
dani-garcia previously approved these changes Nov 8, 2024
Copy link
Member

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

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

Platform changes LGTM

ike-kottlowski
ike-kottlowski previously approved these changes Nov 8, 2024
@rr-bw rr-bw dismissed stale reviews from dani-garcia and ike-kottlowski via aab0f60 November 19, 2024 21:30
@rr-bw
Copy link
Contributor Author

rr-bw commented Nov 21, 2024

@ike-kottlowski

Since your last review:

  • aab0f60
    • I added better fallback error handling for a case where the active user email cannot be retrieved. After conversation with Product, we'll now show a toast and then log the user out via messagingService so that we can route them properly. I included a video of this scenario in the PR description.
      • We need to log them out first because otherwise our route guards would get in the way of navigating them elsewhere, like / or /login.
      • 224b1d8 - added a setTimeout() of 5 seconds before logging out to give the user time to see the error toast before process reload kicks in.
    • Browser has some extra logout logic, hence the client-specific service.
  • 58f6243 - added simple tests for default service
  • 7e73fe3 - added tests for extension service
  • I also re-recorded videos of the flows in the PR description.

@rr-bw rr-bw merged commit 9f99454 into main Nov 21, 2024
72 checks passed
@rr-bw rr-bw deleted the auth/pm-9693/refresh-login-decryption-options-component branch November 21, 2024 21:31
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.

4 participants