-
Notifications
You must be signed in to change notification settings - Fork 987
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(onboarding): Present Terms to users upgrading from v1 or those who need to accept updated Terms #21487
Conversation
@@ -17,6 +18,15 @@ | |||
(fn [{:keys [customization-color]}] | |||
(or customization-color constants/profile-default-color))) | |||
|
|||
;; A profile can only be created without accepting terms in Status v1. In Status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused by my own comment in the original commit, so after cherry-picking I reworded things a bit and renamed the subscription.
Jenkins Builds
|
75% of end-end tests have passed
Failed tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
75% of end-end tests have passed
Failed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Passed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
|
@ilmotta thanks for the PR. Please take a look at the issue below ISSUE 1 Terms screen appears after removing one of the existing Profiles despite terms have been already accepted beforeSteps:
Actual result: Terms screen is displayed despite terms have been already accepted. telegram-cloud-document-2-5244689624340649849.mp4 |
Thank you for promptly testing @pavloburykh 💯 Unfortunately this is the behavior by "design" in the code. It's a known shortcoming of the implementation in status-go. The state about whether the user accepted the terms is stored in the first profile ever created. If you created two profiles, but deleted the first one, then the incorrect screen will appear. If you remove profile B instead of A, it work correctly because the state would still be stored in the local db by status-go. In release 2.32 we will make same pretty significant simplifications to onboarding. One of the changes under consideration is to remove this mandatory checkbox and use something like "By proceeding you agree with the terms" and possibly removing this extra screen saying "Explore new Status" because most users would have migrated from v1 anyway (according to app stores). It will be a matter of aligning with legal too, so let's see. Right now we can minimize our efforts regarding the privacy policy and terms of use UX because we may change everything soon. For now I would say this is a minor limitation we can live with until 2.32 is published. What do you think? |
Agree @ilmotta! PR is ready for merge. Thank you! |
This commit is essentially a cherry-pick from d45eb5e. We now show the onboarding intro requesting the user to accept the Terms of Use & Privacy Policy with the new button "Explore the new Status". status-go PR status-im/status-go#5977 In practice, this means: - Users coming from Status v1 who had at least one profile will see the modified onboarding intro screen and will need to accept the terms to proceed. - Users who already installed v2, are upgrading, and Status has a privacy policy changes will need to accept the terms to proceed. Areas that may be impacted - Onboarding Steps to test: The criteria used during development: 1. Given that user Alice had installed v1 and had one or more profiles. 2. When she installs v2 and opens it, she sees the new onboarding intro and must agree to the terms to enable the button "Explore the new Status". 3. After pressing the button, she can login as usual in any of her profiles. 1. Given that user Alice already upgraded from v1 and accepted the terms. 2. When she reopens the app she does not need to accept terms again and can immediately sign-in with any of her profiles. 1. Given that user Alice already upgraded from v1 and accepted the terms. 2. When she deletes all profiles, she sees the onboarding intro for users who have not upgraded, i.e. she has to agree to terms and she sees the usual two buttons "Create profile" and "Sync or recover profile". 1. Given that user Alice never installed Status. 2. When she installs v2, she sees the normal onboarding intro screen, where she has to accept the terms and she sees two buttons "Create profile" and "Sync or recover profile". 3. When she reopens the app, she doesn't see anymore the screen to accept terms.
61ac890
to
c47f317
Compare
…ho need to accept updated Terms (#21487) Cherry-pick d45eb5e. Fixes #21113 Related status-go PR: status-im/status-go#5977
…ho need to accept updated Terms (#21487) Cherry-pick d45eb5e. Fixes #21113 Related status-go PR: status-im/status-go#5977
Hey @ilmotta! Also, it turned out that Terms screen is appearing for synced account. ISSUE 2 Terms screen appears after syncing the accountSteps:
Actual result: user needs accept Terms telegram-cloud-document-2-5280758248591349646.mp4 |
@pavloburykh, But to answer your question, onboarding simplifications have been defined and the user won't have to accept the checkbox (per screenshot). This means the existing (2.30 & 2.31) solution won't work well in 2.32 and we'll need to adjust. The business problem is: how do we tell the user the Terms of Service or Privacy Policy changed between releases? I believe whatever we do we will do in the least obtrusive way and the simplest way, therefore different than 2.30 & 2.31. Long story short, I think we shouldn't fix ISSUE 1 or ISSUE 2 now, but they will be attacked/solved in 2.32 once we define & implement what I shared above. I even have the impression we might completely remove the code from this PR (and from status-go), but let's see. |
Yes. it exists in 2.30, wondering how we (QAs) have missed it. |
@pavloburykh I didn't forget about this issue :) But in 2.32 we will go with what we have right now and not cause further modifications because the onboarding simplifications are still in progress. In release 2.33 we will likely have time to get back to this issue. My intuition tells me we will just remove the entire feature because users of 2.32+ will accept the terms implicitly. |
Fixes #21113
Summary
This PR is 99% identical to #21124. We are cherry-picking from the 2.30 release branch revision d45eb5e and applying to the status-go and status-mobile
develop
branches in preparation for the 2.31 release.Related status-go PR: status-im/status-go#5977
We will update the release branches once the develop branches are merged.
Steps to test
Steps to test are identical to the original PR #21124.
I re-tested and everything seems to work as well as in 2.30, where the original PR was already tested. I tested a scenario that's difficult for a QA to test, where I simulated a user who had Status v2 installed, had already accepted the Terms, then I forced a new migration in status-go to simulate an update to the Terms, and finally checked the user would need to re-accept terms before proceeding.
status: ready