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

Show users with passwordless accounts a meaningful error message #8663

Merged
merged 14 commits into from
Jan 2, 2024

Conversation

jswinarton
Copy link
Contributor

@jswinarton jswinarton commented Nov 27, 2023

Description

Prior to this PR, when a user attempted to log in using either an FxA "stub" account (i.e. an account without a password) or an SSO-linked account without a password, the client would display a generic error message. This PR adds views that show the users actionable messages that guide them through the steps necessary to set a password on their FxA account.

Reference

Fixes VPN-5089 and VPN-5871

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas (N/A)
  • I have added thorough tests where needed

@jswinarton jswinarton force-pushed the 5089-handle-passwordless branch 5 times, most recently from fbc73a4 to 8a4dd91 Compare December 5, 2023 21:54
@jswinarton jswinarton force-pushed the 5089-handle-passwordless branch 12 times, most recently from 95a00e8 to 062fac6 Compare December 18, 2023 16:40
@jswinarton jswinarton force-pushed the 5089-handle-passwordless branch from 062fac6 to bea9410 Compare December 18, 2023 18:49
@jswinarton jswinarton marked this pull request as ready for review December 18, 2023 20:07
@jswinarton jswinarton requested a review from flodolo as a code owner December 18, 2023 20:07
src/translations/strings.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,34 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
Copy link
Contributor

Choose a reason for hiding this comment

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

This view highly resembles ViewErrorFullScreen.qml, any opportunity to reuse that here and consolidate MZInAppAuthenticationThirdParty into ViewAuthenticationSsoAccount since MZInAppAuthenticationThirdParty is not super reusable? I only suggest this as I think it would save us some complexity within in-app auth. What do you think?

Copy link
Contributor

@MattLichtenstein MattLichtenstein Dec 21, 2023

Choose a reason for hiding this comment

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

a little cut off on mobile... maybe something to do with the ios safe area?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, the buttons should be higher on desktop, according to the mockups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re: factoring this into ViewErrorFullScreen.qml -- you are correct that it is a close copy, it was actually what I originally copied to create the base view. The only reason I didn't put more work into factoring this out is because the current plan for in-app auth is to tear it out and replace it with FxA browser based auth in Q1. We are still trying to figure out exactly what this will look like, which is why we are adding these screens as a stop gap to prevent user issues in the short term. Since this will almost certainly go away soon I feel it's probably not worth the effort.

Still working on the layout issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed the button height issue by adding a vertical spacer in the same manner as in ViewSubscriptionNeeded.qml.

tests/functional/testAuthenticationInApp.js Outdated Show resolved Hide resolved

MZButton {
objectName: "buttonSignIn"
text: "Continue to sign in"
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to go in strings.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

this is marked as resolved, but a hardcoded string is still being used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about this, not sure how that happened. Should be up to date now.

@jswinarton jswinarton force-pushed the 5089-handle-passwordless branch from bea9410 to 7172cbd Compare December 27, 2023 13:54
@jswinarton jswinarton force-pushed the 5089-handle-passwordless branch from 13a206f to 210ddf0 Compare January 2, 2024 14:14
Copy link
Collaborator

@flodolo flodolo left a comment

Choose a reason for hiding this comment

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

Missed the typo

src/translations/strings.yaml Outdated Show resolved Hide resolved
@jswinarton jswinarton enabled auto-merge (squash) January 2, 2024 21:34
@github-actions github-actions bot added the 🛬 Landing This PR is marked as "auto-merge" label Jan 2, 2024
@jswinarton jswinarton merged commit e1262f1 into main Jan 2, 2024
126 checks passed
@jswinarton jswinarton deleted the 5089-handle-passwordless branch January 2, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛬 Landing This PR is marked as "auto-merge"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants