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

feat: extend the time we resume the session without showing OTP #7212

Merged
merged 33 commits into from
Sep 27, 2023

Conversation

omridan159
Copy link
Contributor

@omridan159 omridan159 commented Sep 13, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add the appropiate QA label when dev review is completed
    • needs-qa: PR requires manual QA.
    • No QA/E2E only: PR does not require any manual QA effort. Prior to merging, ensure that you have successful end-to-end test runs in Bitrise.
    • Spot check on release build: PR does not require feature QA but needs non-automated verification. In the description section, provide test scenarios. Add screenshots, and or recordings of what was tested.
  5. Add QA Passed label when QA has signed off (Only required if the PR was labeled with needs-qa)
  6. Add your team's label, i.e. label starting with team- (or external-contributor label if your not a MetaMask employee)

Description

Extended the session persistence time to one hour without showing OTP.

For the first hour after connecting, the session will remain active without requesting an OTP.

All of the examples SDK apps that use the OTP modal tested on IOS and Android with a new wallet build that includes the new OTP changes.

The cases that have been checked were when the channel was active recently and when the channel was NOT active recently in order to test both flows.

Everything appears to be working as expected.
Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

fixes #???

Checklist

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

@omridan159 omridan159 requested a review from a team as a code owner September 13, 2023 14:31
@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

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.

Copy link
Contributor Author

@omridan159 omridan159 left a comment

Choose a reason for hiding this comment

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

I have read the CLA Document and I hereby sign the CLA

abretonc7s
abretonc7s previously approved these changes Sep 14, 2023
Copy link
Contributor

@abretonc7s abretonc7s left a comment

Choose a reason for hiding this comment

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

LGTM

app/core/SDKConnect/SDKConnect.ts Outdated Show resolved Hide resolved
abretonc7s
abretonc7s previously approved these changes Sep 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2023

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Comparison is base (481412d) 34.60% compared to head (a58d6bd) 34.57%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7212      +/-   ##
==========================================
- Coverage   34.60%   34.57%   -0.04%     
==========================================
  Files        1017     1017              
  Lines       27104    27125      +21     
  Branches     2197     2204       +7     
==========================================
- Hits         9379     9378       -1     
- Misses      17236    17258      +22     
  Partials      489      489              
Files Coverage Δ
app/core/DeeplinkManager.js 1.27% <ø> (ø)
app/components/Nav/App/index.js 5.83% <0.00%> (-0.09%) ⬇️
app/core/SDKConnect/SDKConnect.ts 1.88% <0.00%> (-0.30%) ⬇️

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

Copy link
Contributor Author

@omridan159 omridan159 left a comment

Choose a reason for hiding this comment

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

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor Author

@omridan159 omridan159 left a comment

Choose a reason for hiding this comment

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

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor Author

@omridan159 omridan159 left a comment

Choose a reason for hiding this comment

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

I have read the CLA Document and I hereby sign the CLA

@andreahaku andreahaku added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-sdk SDK team labels Sep 14, 2023
@omridan159
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

abretonc7s
abretonc7s previously approved these changes Sep 18, 2023
Copy link
Contributor

@abretonc7s abretonc7s left a comment

Choose a reason for hiding this comment

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

LGTM

@omridan159 omridan159 added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Sep 18, 2023
@abretonc7s abretonc7s force-pushed the feat_updating-OTP-flow branch from 79c9ba3 to d6f7931 Compare September 18, 2023 13:16
abretonc7s
abretonc7s previously approved these changes Sep 18, 2023
andreahaku
andreahaku previously approved these changes Sep 19, 2023
Copy link
Contributor

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

LGTM

@abretonc7s abretonc7s force-pushed the feat_updating-OTP-flow branch from f9657c0 to d3e36e3 Compare September 22, 2023 07:46
@socket-security
Copy link

Updated dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/sdk-communication-layer 0.5.2...0.7.0 None +0/-0 16.7 MB metamaskbot

@christopherferreira9 christopherferreira9 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. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Sep 26, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
2.7% 2.7% Duplication

Copy link
Contributor

@andreahaku andreahaku left a comment

Choose a reason for hiding this comment

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

LGTM

@christopherferreira9 christopherferreira9 merged commit ce22421 into main Sep 27, 2023
@christopherferreira9 christopherferreira9 deleted the feat_updating-OTP-flow branch September 27, 2023 13:12
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2023
@christopherferreira9 christopherferreira9 added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Sep 27, 2023
@metamaskbot metamaskbot added the release-7.9.0 Issue or pull request that will be included in release 7.9.0 label Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.9.0 Issue or pull request that will be included in release 7.9.0 team-sdk SDK team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants