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

Fix #4366, #3611, #4323: Add app data reset flow to facilitate being able to reset the admin pin #4418

Merged
merged 5 commits into from
Sep 7, 2022

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Jul 3, 2022

Explanation

Fixes #4366
Fixes #3611
Fixes #4323

This PR introduces a data reset flow for cases when the user fails to remember their administrator pin. Since there's no remote profile support currently, the only option is to clear all profiles on the device. This is done using a two-step dialog since the action is permanent (at least for now--in the future we could consider making a local backup that could be restored, but that could become pretty complicated).

While this isn't a great solution, it's better than not having an option at all since users can currently get permanently stuck (see #3611). While this doesn't directly fix #3611, it mitigates it by providing a proper in-app means of resetting app data.

#4323 seems unrelated, but resetting the app required routing back to SplashActivity which means this PR needed to introduce a robustness mechanism to not set the app locale unless it wasn't already initialized (which bypasses the bug mentioned by that issue).

Mechanically, the deletion process involves:

  • Deleting all profiles on the device
  • Updating the in-memory cache (which causes a few UI glitches)
  • "Restarting" the app by ending the activity and routing back to SplashActivity

This replaces the existing "forgot PIN" flow for admins, and it technically does not reset non-profile data (such as the device installation ID, onboarding status, analytics/crash events, and some device-wide preferences).

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

This PR is using the app's existing alert dialog theme, so it's not particularly interesting to show landscape/tablet screenshots as the UI isn't fundamentally different. The same is the case for special internationalization or accessibility concerns.

In terms of a demonstration of the new flow, see the following video:

reset_oppia_data.mp4

#4567 is tracking introducing tests, and the PR addressing it for these changes will include demonstrations of changed Espresso tests.

@oppiabot
Copy link

oppiabot bot commented Jul 10, 2022

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 10, 2022
@oppiabot oppiabot bot closed this Jul 17, 2022
@BenHenning BenHenning reopened this Jul 18, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 18, 2022
@oppiabot
Copy link

oppiabot bot commented Jul 25, 2022

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 25, 2022
@oppiabot oppiabot bot closed this Aug 1, 2022
@BenHenning BenHenning reopened this Aug 2, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 2, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 9, 2022

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 9, 2022
@oppiabot oppiabot bot closed this Aug 16, 2022
@BenHenning BenHenning reopened this Aug 25, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 25, 2022
@BenHenning
Copy link
Member Author

@seanlip what are your thoughts on the video demonstrating the new flow? Happy to take easy suggestions (such as wording), or feedback on if you think there's something especially problematic with the new flow.

@BenHenning
Copy link
Member Author

BenHenning commented Aug 25, 2022

Remaining tasks before force-merge:

  • Passing CI
  • Finished description
  • Sign-off from product (done, just need to make an adjustment)
  • Fix caching issue wherein new profiles temporarily show their previous profile slot's progress
  • Change forgot pin wording per Sean's suggestion below

@seanlip
Copy link
Member

seanlip commented Aug 25, 2022

Just to check, you've verified that profiles actually get deleted on reset, and that if they are recreated with the same profile name, the old data no longer shows? The video doesn't show that so I wanted to double-check.

Otherwise, the only suggestion I have is to change "I forgot my pin" to "Forgot PIN?"; I think the rest seems OK to me.

Thanks!

@seanlip
Copy link
Member

seanlip commented Aug 25, 2022

Also @BenHenning, just a note for next time, please assign me explicitly if you want me to take action in any way (I depend on this somewhat to manage the incoming PR queue). Thanks!

@BenHenning
Copy link
Member Author

Thanks @seanlip. The name doesn't matter, but thanks for bringing up that flow. There's an in-memory cache inconsistency here for saved progress where a new profile will have the same transient progress as the previous one (for that slot--name doesn't matter here). Restarting the app shows the correct reset progress, so I need to look into why this is happening.

@oppiabot
Copy link

oppiabot bot commented Sep 1, 2022

Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 1, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Sep 7, 2022
@BenHenning
Copy link
Member Author

So digging into the issue a bit more, the main problem is that all of the caches throughout the app that depend on profiles are not considering the files being removed like they are in this use case. We can certainly add cataloging to fix this, but I'm not sure it's worth the long-term maintenance cost (as it will affect all future cache stores, too).

The workaround is to forcibly close the app and instruct the user to reopen it manually. This isn't a great option, but automatically opening the app is tricky (Android provides mechanisms, but they are either inexact in when they'll run, or they will eventually require user-revokable permissions in later versions of Android which is just a bit messy). Given how rare this case is expect to happen, having the user reopen the app seems reasonable I think.

@BenHenning
Copy link
Member Author

Due to the importance of getting this PR in ASAP for Beta MR1, I will be force-merging this once the CI checks come back passing. #4567 is tracking finishing the PR's tests, and ensuring the PR is reviewed later.

@BenHenning BenHenning merged commit ca2660f into develop Sep 7, 2022
@BenHenning BenHenning deleted the add-admin-pin-reset-option branch September 7, 2022 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants