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.

Sign up for GitHub

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

Jump to bottom

[FIX] Fix SRP reveal screen #5980

Merged
merged 5 commits into from
Mar 17, 2023
Merged

[FIX] Fix SRP reveal screen #5980

merged 5 commits into from
Mar 17, 2023

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Mar 17, 2023 •
edited
Loading

Description

This PR fixes both the padding issue + interaction of the SRP reveal screen bug found in 6.2.0 regression
https://app.zenhub.com/workspaces/mobile-release-regression-6249e5242464b70013315a98/issues/gh/metamask/mobile-planning/720

Screenshots/Recordings

Reach out to @Cal-L for videos

Testing
There are three main scenarios that the reveal credentials screen affects (the first 3 scenarios. 3 and 4 are essentially the same scenario)

  • Scenario: Should be able to recover SRP when something errors out on the Root boundary

    • Given I am on the Login screen
    • And I enter the password and log in
    • When the app errors out and displays the Root error boundary
    • Then I should be able to follow the flow to reveal the SRP

  • Scenario: Should be able to recover SRP when something errors out on the Activity boundary

    • Given I am on the Wallet view
    • And I open the drawer and tap on Activity
    • When the app errors out and displays the ActivityView error boundary
    • Then I should be able to follow the flow to reveal the SRP

  • Scenario: Should be able to reveal SRP from the settings flow

    • Given I am on the Settings view
    • And I open Security & Privacy
    • When I tap on Reveal Secret Recovery Phrase
    • And I pass the quiz
    • Then I should be able to follow the flow to reveal the SRP

  • Scenario: Should be able to reveal private key from the settings flow

    • Given I am on the Settings view
    • And I open Security & Privacy
    • When I tap on Show private key
    • Then I should be able to follow the flow to reveal my private key

Checklist

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

@Cal-L Cal-L added type-bug Something isn't working needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead team-mobile-client release-6.2.0 Issue or pull request that will be included in release 6.2.0 labels Mar 17, 2023
@Cal-L Cal-L requested a review from a team as a code owner March 17, 2023 03:45
@github-actions
Copy link
Contributor

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.

tommasini
tommasini approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@tommasini tommasini 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 pinging me to review this!

Just reinforced the need for testing on outside the navigation object screen

LGTM!!

app/components/Views/ErrorBoundary/index.js Show resolved Hide resolved
sethkfman
sethkfman approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@Cal-L Cal-L force-pushed the fix/720-srp-confirm-screen branch from f937a69 to 98e89c3 Compare March 17, 2023 17:49
@Cal-L
Copy link
Contributor Author

Cal-L commented Mar 17, 2023

Just rebased

@Cal-L Cal-L removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 17, 2023
@cortisiko cortisiko removed the regression-RC DEPRECATED: Please use "regresssion-RC-x.y.z" label instead label Mar 17, 2023
@Cal-L
Copy link
Contributor Author

Cal-L commented Mar 17, 2023

@chrisleewilcox @cortisiko This issue might also be related to another one that Kat posted in Slack. If we have the bandwidth, could we quickly verify it as well?
https://consensys.slack.com/archives/G8RSKCNCD/p1676917311594709

@sethkfman sethkfman added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 17, 2023
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 17, 2023
cortisiko
cortisiko approved these changes Mar 17, 2023
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

🌮 🌮 🌮 🌮

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Mar 17, 2023
@cortisiko
Copy link
Member

@Cal-L for the issue you linked, we need to triage it in our next bug triaging session. The ticket for the issue you linked is here

@cortisiko cortisiko merged commit d484d36 into release/6.2.0 Mar 17, 2023
@cortisiko cortisiko deleted the fix/720-srp-confirm-screen branch March 17, 2023 23:38
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tommasini tommasini tommasini approved these changes

@cortisiko cortisiko cortisiko approved these changes

@sethkfman sethkfman sethkfman approved these changes

Assignees
No one assigned
Labels
QA Passed A successful QA run through has been done release-6.2.0 Issue or pull request that will be included in release 6.2.0 team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead type-bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Cal-L @cortisiko @sethkfman @tommasini @gauthierpetetin