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

App learning progress reset when app moved from alpha to beta #4632

Closed
partha-basu opened this issue Sep 29, 2022 · 7 comments · Fixed by #4696
Closed

App learning progress reset when app moved from alpha to beta #4632

partha-basu opened this issue Sep 29, 2022 · 7 comments · Fixed by #4696
Assignees

Comments

@partha-basu
Copy link

partha-basu commented Sep 29, 2022

Describe the bug
The learning progress - Story/Lesson completion - was lost when the app moved from alpha to beta channel.
I received a notification that I've been upgraded and then taken to the home/landing screen with profile selection options.
Once the profile is chosen it shows the Learning Home Screen.
Generally when I go to a topic/Lesson I have a "continue" option but it went back to the starter screen.

question: I would expect the app not to lose progress whenever there is an update to the app.

To Reproduce
Can't reproduce the behavior anymore since it's a one-time thing.
Expected behavior
I would expect the app not to lose progress whenever there is an update to the app.

Demonstration
N.A.

Environment

  • Device/emulator being used: Pixel 5
  • Android or SDK version (e.g. Android 5 or SDK 21): Android 13
  • App version (you can get this through system app settings or via the admin controls menu in-app): 0.9-beta-3f935261e0

Additional context
Add any other context about the problem here.

@BenHenning
Copy link
Member

Thanks for filing @partha-basu. For clarity on a couple of points: do you still see a 'Continue' button for the lesson that you tried to continue playing? If so, does it successfully resume the lesson for you?

Also, are any complete lessons no longer marked as completed?

@partha-basu
Copy link
Author

@BenHenning - I restarted the journey. Now the behavior is same as before - able to leave lesson midway and then continue from drop-off point. The progress reset was a one-time thing post update. Flagging as wanted to make sure it doesn't happen with each update.

@BenHenning
Copy link
Member

Thanks for clarifying @partha-basu. This is actually expected for partially completed lessons (fully completed lessons should never reset). It's something that we definitely want to fix by introducing manual downloading support in the app, but perhaps we should investigate stop-gaps in the meantime.

Sending this over to @seanlip for product weigh-in, and since he's also looking through the original checkpointing PRD to consider different approaches. We've also talked about some potential mitigation factors here, but they're not entirely trivial.

@partha-basu
Copy link
Author

@BenHenning - thanks for the confirmation. If that is the expected behavior for all updates - having some kind of call-out/warning (something like - partially completed lesson progress would be lost!) in the playstore page or before updating would help (I did get a pop-up that I'm being upgraded to beta channel but that did not call out what would specifically change for me).

@BenHenning
Copy link
Member

BenHenning commented Oct 4, 2022

Well noted @partha-basu. I'm not sure if Play Store description is correct place for this, or if we need to consider other options (such as bundling multiple lesson versions within the app).

@seanlip
Copy link
Member

seanlip commented Oct 4, 2022

Had an offline discussion with @BenHenning and noting that this is important to fix (but let's try to fix with as minimal contortions as possible). We don't want users to be scared of updating the app due to possible loss of progress.

@seanlip seanlip assigned BenHenning and unassigned seanlip Oct 4, 2022
@BenHenning BenHenning changed the title App learning progress reset when app moved from aplha to beta App learning progress reset when app moved from alpha to beta Oct 13, 2022
@BenHenning
Copy link
Member

So thinking about this a bit more, I suspect the most correct and reliable way to attempt this would be to, upon attempting to load a checkpoint, "replay" that checkpoint against the newest version of the exploration (only if the version doesn't match). There are some properties of the current checkpoint that we would unconditionally overwrite:

  • Exploration title & version would be updated to the latest
  • The hint state would be dropped (rather than trying to figure out if it's compatible)

Otherwise, all previous answers (for both completed and the top pending state) will be "replayed" against their corresponding states in the new version of the exploration. In order to "pass", the following must be met:

  1. The state needs to exist
  2. The state needs to be reachable via the previous card's state using the correctly submitted answer for that state
  3. All answers must be type-compatible with their corresponding states

The feedbacks will then be replaced with the latest versions given each answer (as if the user replayed the whole lesson).

This is the most robust way to try and avoid needing to throw away the checkpoint, and I think works against all possible exploration transformations in a fairly organic way (since it simulates the learner re-playing everything).

BenHenning added a commit that referenced this issue Nov 11, 2022
## Explanation
Fixes #4632
Fixes #3779

Adds support for upgrading checkpoints to support the latest versions of
explorations if the checkpoint was saved for an older version, in cases
where it's very likely reasonable to upgrade the checkpoints. The
methodology applied is to simulate "replaying" the full lesson using the
past answers that were submitted in the checkpointing, and verifying
that the same outcome would more or less be achieved by ensuring:
- The content IDs for received feedback haven't changed (but the HTML
might, e.g. for typos).
- The destination of a state hasn't changed.
- The pending & all completed states still exist.

The cases that currently can't be handled (but may be reasonable to in
the future):
- State renaming (these are treated as failures).
- Reconstituting a compatible hint/solution state (the state is just
reset for now so the user can re-trigger the hints if they get stuck
after returning to the lesson).

As part of testing this:
- A bunch of new assets were introduced to directly demonstrate
upgrading flows that lead to both compatibilities and incompatibilities
(these were generated).
- A fake ``ExplorationRetriever`` needed to be introduced in order to
support proxying exploration loading (so that newer versions can be
force-loaded). This required splitting the existing ExplorationRetriever
into an interface & implementation.
- ``ExplorationStorageModule`` has been moved one package up for better
organization (which led to a bunch of other files being changed).
- Test-only checkpoint database storage sizes have been 150 bytes in all
related tests, so this has been made as the test-specific constant in
the new ExplorationStorageTestModule.

Separately, ExplorationRetriever was updated to mostly force background
thread interaction by using a suspend method for actual exploration
loading (which led to some small test changes).

#3779 was also fixed (since I was changing this code, anyway) by
returning a default details object instead of failing whenever a
checkpoint can't be found for a given exploration. Note that no test was
added for this solution since doing so would be a bit painful (as it
requires loading and verifying an exploration at the activity level
where a bunch of the lower-level tools from StateFragment{Local}Test
aren't available), and the failure case here is something that will
definitely be caught by e2e tests so the risk is low for a serious
regression.

## Essential Checklist
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
While this PR does affect UI flows for checkpoints, I'm opting to not
include a walkthrough since it's difficult and time consuming to
simulate the local upgrade flow. We'll be testing this specially during
manual QA testing for this release to ensure it works as expected.

Similarly, app module tests haven't changed in a major way so I'm
skipping showing Espresso results here. The Robolectric tests results
ought to be a sufficiently good proxy for correctness.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants