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

[IMPLEMENT] Use Terms Modal #5242

Merged
merged 57 commits into from
Mar 17, 2023
Merged

[IMPLEMENT] Use Terms Modal #5242

merged 57 commits into from
Mar 17, 2023

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Nov 15, 2022 •
edited

Description
Implementation of the new User Terms logic.

  • The new user will not be able to create a new wallet or import an existing one while the new user while do not accept the Terms and Conditions
  • The existing user will not be able to do anything if do not accept the Terms and Conditions

Screenshots/Recordings
Behaviour with fresh install:
Importing SRP
https://recordit.co/wjQpTaIMds
Create new wallet
https://recordit.co/RmtOhH9gih

With existing users:
https://recordit.co/4h8bTKbUl5

Test Cases
Case 1:

  • Fresh install
  • accept user terms
  • close app
  • open app
  • expected not to appear the user terms again

Case2:

  • Already installed app
  • accept user terms
  • close app
  • open app
  • expected not to appear the user terms again

Case3:
Need to be tested the events on Mix panel
Key: 'Terms of Use'
value1: 'ToU Displayed'
value2: 'ToU Accepted'

Case4:

  • Small screen devices

Issue

Progresses #???

Checklist

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

@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 marked this pull request as ready for review January 4, 2023 11:39
@tommasini tommasini requested a review from a team as a code owner January 4, 2023 11:39
@tommasini tommasini added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Jan 4, 2023
@tommasini tommasini changed the title [DRAFT][IMPLEMENT] Use Terms Modal [IMPLEMENT] Use Terms Modal Jan 4, 2023
Cal-L
Cal-L reviewed Jan 6, 2023
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left some comments.

app/components/Nav/Main/index.js Outdated Show resolved Hide resolved
app/components/Views/Onboarding/index.js Outdated Show resolved Hide resolved
app/components/UI/ModalUseTerms/ModalUseTerms.tsx Outdated Show resolved Hide resolved
app/component-library/components/Modals/ModalMandatory/ModalMandatory.tsx Outdated Show resolved Hide resolved
app/component-library/components/Modals/ModalMandatory/ModalMandatory.tsx Outdated Show resolved Hide resolved
@Cal-L
Copy link
Contributor

Cal-L commented Jan 6, 2023

It also looks like the implemented UI is slightly different than what is presented in the Finalized section in Figma.

  • The checkbox isn't aligned relative to the agreement description
  • The circle with the down caret doesn't exist in the implementation

I left a few comments in Figma asking for clarification on some pieces. Will keep an eye out for a response. @tommasini Who should we reach out to as the designer for this UI?
Figma for reference - https://www.figma.com/file/j3T1H0Xiuke9NhFJ3yOxBU/Updated-Terms-of-Use?node-id=0%3A1&t=VX7fTz82hEXlbNiO-0

@chrisleewilcox
Copy link
Contributor

Hi all. Tomas had looped me in on this thread because I am testing the ToU and I have concerns about the accessibility icon at it's current position on the ToU content in mobile app. Currently the accessibility icon is too close to the agree to terms checkbox and if a user mistaps on the accessibility icon they can get confused with the different accessibility content being displayed and have a hard time agreeing to terms.
Can we move the accessibility icon? I'm thinking to the upper right of the ToU content. If so I think this would have to be done on the website side.

@SamuelSalas
E2e appium/5783 tou onboarding new app installs (#5951)
0cfb06b 
* Webdriverio and Detox test scripts for Term of use feature. Test scripts updated to have the term of use steps

* Small changes to fit browserstack

* Remove static-logos.js file changes
@chrisleewilcox
Copy link
Contributor

@tommasini I am unable to see the ToU events. I can see other events for mobile app but not ToU events. Can you take a look?
https://recordit.co/5fDI0enmj3

@chrisleewilcox chrisleewilcox added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels Mar 13, 2023
@tommasini
Copy link
Contributor Author

@chrisleewilcox It's fixed, the ToU modal was moved to before importing the SRP or create a new wallet screens and right after the Metrics agreed screen, this way news users, if they agree to participate on the metrics, the mix panel events should appear

Cal-L
Cal-L approved these changes Mar 14, 2023
View reviewed changes
Copy link
Contributor

@Cal-L Cal-L left a comment •
edited

Choose a reason for hiding this comment

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

Code LGTM. @tommasini Could you verify Chris's question about the ToU events? LMK if you need support

@tommasini tommasini added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Mar 15, 2023
@tommasini
Copy link
Contributor Author

They are addressed on this PR, the reason why they were not appearing it was that it was a new user and still did not accept to participate in the metrics

@tommasini tommasini added the release-6.3.0 PR for release 6.3.0 label Mar 15, 2023
@SamuelSalas
Copy link
Member

Webdriver.IO Browserstack test results:
/wdio/features/Accounts/CreatingWalletAccount.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/521d7390f4c43a28130de11107f32ed6e3e0dbf3?auth_token=60b1b6a1021fde7802e00eb31d6661ce751543e08f10ccd54179e78d4b1e7f96

/wdio/features/Accounts/ImportingAccount.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/0538249f59ed6e471203e5fe6dc6374cfc756a63?auth_token=08f7b5c143a11dac3e620766ad1a7b9ae8453ff098e0586fc4dbbeaf6416b5d7

/wdio/features/Onboarding/CreateNewWallet.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/94d198ecd44e786cbebca558bdceb6fc664b1e8f?auth_token=79b49c5ed157b59418bc8552c2aa1ac70245791b02a65a3a16240c7c699b8178

/wdio/features/Onboarding/ImportWallet.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/f11b33be46545b268bda4ef17fafdd9564a7fe97?auth_token=6225c8675703479047c579bc2247ec0538291aaa37e162030c51b27018497a1a

/wdio/features/Onboarding/ImportWalletRegression.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/c505a6675f2ed1cee046e0a07d8b856b109f66b1?auth_token=074accdd04692188d52c7827ef2f8b24ab85f13edc2d22e81b44233dcf36f53b

/wdio/features/Onboarding/TermsOfUse.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/983f14e98a39a833d93081dc78256c64caee0a56?auth_token=0db9e909a06dea0bb1f1ecb359120fbe24e8404570562a85fc9042ad734e22d7

/wdio/features/Performance/AppLaunchTime.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/bd21a47bd0d37590b1d7a86dccf8272df9e284b3?auth_token=b56a04ef64aa0f4b9a5e9789ea63f49e59e64cb68cb085c11a2049d389d2d96f

/wdio/features/AddressFlow.feature -FAILED

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/dab2b9214e3a086d22693cb1d491cbabe6a7a9c4?auth_token=e307fc8ab0f4d18518404bcde4880cf8ec77a1c55b427302a1b07caf0ecf923a

/wdio/features/ExploringWizard.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/82c6107fc9010e5edb2228ebe3c49c1521fa2bd9?auth_token=5d64907aeef8250126fe209b90b1ad858bbfde11ce21642b6f2326ca4eab091f

/wdio/features/LockResetWallet.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/cad6bbb66e42f768de7de0e5b2153556d50b4f87?auth_token=c82d1299a0c53345edcf17bc24e5960b4e59b364d71699c66c08b40fae12502f

/wdio/features/ImportCustomToken.feature - FAILED

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/dff5cc6163cb422e5e29574749341db528e513c7?auth_token=b9d8253686a519b8a72cfaeec70f479854ea28ee82f8f3d90959984295d72de1

/wdio/features/NetworkFlow.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/55375aa04ef4df19ce81b923559e40dfa27e3827?auth_token=ea4acf3f6b62566e9b139115b923a75958173eacb4b65cd5ced4364673cdd6cf

/wdio/features/RequestTokenFlow.feature - FAILED

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/f8a137cd9a02ba2193eda3b83979c4b2834244a4?auth_token=7b0959e392fc12b47c39c6712c646ca4ce88019771126bd3fa5355919333c88c

/wdio/features/SecurityPrivacyRememberMe.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/0a3f6b8a0d732e514589a839cd89d4fa84c34414?auth_token=69da264a969521f146005f481532e08e63a2d5f2d7b3c5e4aa205692614116e2

/wdio/features/SendToken.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/b40047a26355496f1faa64e0f3920eefeb80a5c2?auth_token=be1b837cc2d560066000120cd29c2ff50a91cfb2cc3fb42d42ed32152069ad5a

/wdio/features/Onboarding/OnboardingCarousel.feature

https://app-automate.browserstack.com/builds/8921ccc14ac9135ca38531b8fc30448c28a5e573/sessions/c1283403ff6c2c0ad81e7652c8b3f1292831311a?auth_token=5f5275f58e2601d72999b3e66a3f62e6837c32e70a1b5b0cb6c1a154cb807aa9

@SamuelSalas
Copy link
Member

Detox test results:
Test Suites: 4 failed, 9 passed, 13 total
Tests: 16 failed, 120 passed, 136 total

Failed:
e2e/specs/confirmations/send-eth-flow.spec.js
e2e/specs/confirmations/advanced-gas-fees.spec.js
e2e/specs/wallet-tests.spec.js
e2e/specs/deeplinks.spec.js

@SamuelSalas
Solve lint issues
9efea64
@SamuelSalas
Merge branch 'main' into implement/466-modal-user-terms
8ddffc4 
 Please enter a commit message to explain why this merge is necessary,
chrisleewilcox
chrisleewilcox approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

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

LGTM

@chrisleewilcox chrisleewilcox merged commit bb239f3 into main Mar 17, 2023
@chrisleewilcox chrisleewilcox deleted the implement/466-modal-user-terms branch March 17, 2023 20:27
@github-actions github-actions bot locked and limited conversation to collaborators Mar 17, 2023
@chrisleewilcox chrisleewilcox added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 20, 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

@chrisleewilcox chrisleewilcox chrisleewilcox approved these changes

@sethkfman sethkfman sethkfman approved these changes

@Cal-L Cal-L Cal-L approved these changes

@jpcloureiro jpcloureiro Awaiting requested review from jpcloureiro

Assignees

@chrisleewilcox chrisleewilcox

@SamuelSalas SamuelSalas

Labels
QA Passed A successful QA run through has been done release-6.3.0 PR for release 6.3.0 team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@tommasini @Cal-L @chrisleewilcox @SamuelSalas @sethkfman @gauthierpetetin @cortisiko