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

Hide remember me #4305

Merged
merged 18 commits into from
Aug 17, 2022
Merged

Hide remember me #4305

merged 18 commits into from
Aug 17, 2022

Conversation

owencraston
Copy link
Contributor

@owencraston owencraston commented May 13, 2022

Description

  • Remove remember me as an option for new users
  • The key here is new users. Users who have already enabled remember me should not be affected by this change.
  • This simply hides the front end that allows for remember me to be toggled on.
  • Based on the feedback below, remember me will be something the user can turn on in settings and is now intended more for developers
  • I also made a LoginOptionsSwitch that handles if what login option should be rendered into its own component since it was repeated in several files
  • biometrics is always going to be the higher priority option for login. we will only show remember me as an option if its enabled in settings and biometrics are disabled

Future improvements

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

#4194
Progresses #4194

@github-actions
Copy link
Contributor

github-actions bot commented May 13, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@owencraston owencraston force-pushed the hide-remember-me branch 3 times, most recently from 08ca010 to 9f03037 Compare May 16, 2022 19:21
@owencraston owencraston marked this pull request as ready for review May 16, 2022 21:12
@owencraston owencraston requested a review from a team as a code owner May 16, 2022 21:12
Copy link
Contributor

@blackdevelopa blackdevelopa left a comment

Choose a reason for hiding this comment

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

I didn't run this code and my comments are simply nitpicking.

app/components/UI/LoginWithBiometricsSwitch/styles.ts Outdated Show resolved Hide resolved
app/components/Views/ChoosePassword/index.js Outdated Show resolved Hide resolved
app/components/Views/ImportFromSeed/index.js Outdated Show resolved Hide resolved
app/components/Views/Login/index.js Outdated Show resolved Hide resolved
@blackdevelopa blackdevelopa added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 17, 2022
@gantunesr
Copy link
Member

Some general notes on the PR,

  • If this has an impact in the UI/UX, you should add a screenshot/recording of the before and after for QA and future references
  • Don't use the keyword Resolves, GH closes the issue related to the PR when it is merged, and this can lead to some confusion on our product boards. Use Progresses instead.

Regarding the code, it looks good to me in the scope of my knowledge about the "Remember Me" epic. I'm not sure if you want @sethkfman to review it since he has more context on the Auth Refactor. If not, and @blackdevelopa agrees, we can approve it and pass it to QA after the failing test (LoginWithBiometricsSwitch) is solved.

@gantunesr gantunesr added needs-qa Any New Features that needs a full manual QA prior to being added to a release. Mobile QA board team-accounts labels May 18, 2022
gantunesr
gantunesr previously approved these changes May 18, 2022
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM. @blackdevelopa let me know if you have any objection.

@owencraston will solve the failing test before moving it to QA

@gantunesr gantunesr removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label May 18, 2022
@owencraston owencraston force-pushed the hide-remember-me branch 2 times, most recently from 93da857 to 474be3f Compare May 18, 2022 17:13
@AlexJupiter AlexJupiter added the Priority - High Task with high priority label Jun 2, 2022
@plasmacorral plasmacorral removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Jun 7, 2022
@AlexJupiter
Copy link

AlexJupiter commented Jun 7, 2022

Suggested route forward to still allow this feature to be used by developers.

  1. Still hide this feature for new users, but provide the ability for someone to turn Remember Me on via a toggle on the Security & Privacy settings section of the app.
  2. This toggle should be placed underneath the "Unlock with Face ID?" toggle and be named "Unlock with Remember Me?". It should also have the subtitle "Turn on at your own risk, since this feature gives anyone with access to your phone also access to MetaMask without any additional security."
  3. This toggle should be turned off by default on new installations
  4. We should record an event when this toggle is turned on Remember Me Toggled On and an event for when the toggle is turned off Remember Me Toggled Off

@jakehaugen could you briefly check my copy suggestion please? Thank you :).

cc. @plasmacorral @gantunesr @sethkfman @owencraston

@AlexJupiter
Copy link

@owencraston slight improvement to the copy @jakehaugen and I have discussed:
Title: Allow "Remember me"
Subtitle: Turn on at your own risk. When "Remember me" is on, it gives anyone access to your MetaMask if they can access your phone.

Since this should just enable the checkbox again, and not actually turn it on, we think this copy improvement is necessary. Any thought let us know!

@owencraston owencraston force-pushed the hide-remember-me branch 2 times, most recently from 1559390 to 8826d97 Compare June 13, 2022 14:29
@owencraston
Copy link
Contributor Author

owencraston commented Aug 10, 2022

I ripped the translations out of this crowdin PR

@owencraston owencraston force-pushed the hide-remember-me branch 3 times, most recently from dc9ea47 to 5e32eda Compare August 10, 2022 16:30
android/app/build.gradle Outdated Show resolved Hide resolved
@owencraston owencraston force-pushed the hide-remember-me branch 3 times, most recently from f12b621 to cb4d6f3 Compare August 16, 2022 15:15
@plasmacorral plasmacorral added the QA Passed A successful QA run through has been done label Aug 17, 2022
@plasmacorral plasmacorral merged commit 0ea0413 into main Aug 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Priority - High Task with high priority QA Passed A successful QA run through has been done release-5.7.0 PRs for release 5.7.0 team-accounts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants