Fix #3327: Create app layer mechanism for saving checkpoints. #3402

MaskedCarrot · 2021-07-03T05:30:28Z

Explanation

Fixes #3327

This PR is stacked on top of PR #3403

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The PR follows the style guide.
The PR does not contain any unnecessary auto-generated code from Android Studio.
The PR is made from a branch that's not called "develop".
The PR is made from a branch that is up-to-date with "develop".
The PR's branch is based on "develop" and not on any other branch.
The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

Testing the changes made in this PR

Checkpointing is disabled by default until a mechanism to resume checkpoints is also implemented. However it can be enabled by changing the value of the variable isCheckpointingEnabled and shouldSavePartialProgresss to true in TopicLessonsFragment.
Since size limit imposed on checkpoint database is set to 2MB, therefore the ProgressDatabaseFull dialog won’t be visible until checkpoint database exceeds 2MB in size So to view the ProgressDatabaseFull dialog box, enable checkpointing and then set the size limit to 0 in ExplorationStorageModule.

Tests

Test	Espresso test result
ExplorationActivityTest

Dialog boxes after this PR

This PR introduces the following three dialog boxes:

StopExplorationDialog: Visible to the user when the exit an exploration that is saved and size of checkpoint database has not exceeded.
UnsavedProgressDialog: Visible to the user when the progress is not saved ( this can happen if checkpointing fails or the lesson was previously completed). This dialog box is also visible to the user when they exit a practice session.
ProgressDatabaseFullDialog: Visible to the user when the progress is saved but the checkpoint database has exceeded the size limit.

…/oppia/oppia-android into add-controller-for-checkpointing

MaskedCarrot

These are the methods whose names can be a bit misleading in questionPlayerActivity

MaskedCarrot · 2021-07-15T17:05:55Z

app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivity.kt

+
+  override fun deleteCurrentProgressAndStopSession() {
+    // No progress is saved in training sessions.
+    questionPlayerActivityPresenter.stopTrainingSession()
+  }
+
+  override fun deleteOldestProgressAndStopSession() {
+    // This function is not needed because their is no progress that is being saved in training
+    // sessions.
+  }


These names can be a bit misleading.

So I think my concern here is that the training session experience shouldn't even show the flow that results in progress being deleted (i.e. I'd expect the old dialog to show up & neither of these methods to get called).

The user still sees the old dialog box with every thing is exactly the same as it was from the user's viewpoint.
But the code for this has changed since from explorations progress also has to be deleted.

Since all the existing dialog boxes have progress associated with them (saved, unsaved and databaseFull), so should I file a new issue that after milestone 1 the training session should have a new dialog box?

BenHenning

Thanks @MaskedCarrot. I had a few concerns left, but otherwise the PR LGTM.

app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivity.kt

BenHenning · 2021-07-16T01:31:55Z

app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivity.kt

+
+  override fun deleteCurrentProgressAndStopSession() {
+    // No progress is saved in training sessions.
+    questionPlayerActivityPresenter.stopTrainingSession()
+  }
+
+  override fun deleteOldestProgressAndStopSession() {
+    // This function is not needed because their is no progress that is being saved in training
+    // sessions.
+  }


So I think my concern here is that the training session experience shouldn't even show the flow that results in progress being deleted (i.e. I'd expect the old dialog to show up & neither of these methods to get called).

app/src/main/res/values/strings.xml

scripts/assets/test_file_exemptions.textproto

…heckpointing

…ing' into app-layer-mech-for-checkpointing

BenHenning

Thanks @MaskedCarrot! LGTM for reviewed files.

app/src/main/res/values/strings.xml

scripts/assets/test_file_exemptions.textproto

BenHenning · 2021-07-16T09:32:54Z

Given that past tests have passed, I'm fine with enabling auto-merge here since Gradle tests should be sufficient for the latest changes.

oppiabot · 2021-07-16T09:35:20Z

Unassigning @BenHenning since they have already approved the PR.

oppiabot · 2021-07-16T09:35:24Z

Hi @MaskedCarrot, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

MaskedCarrot and others added 22 commits June 17, 2021 10:44

add controller and tests

51148fd

NIT

89b44ba

added tests for constant

ee7a8ab

moved enum to a different file

47ed878

fixed lint

51100bb

Merge branch 'develop' into add-controller-for-checkpointing

14f2805

fixed comments

79f430b

Merge branch 'add-controller-for-checkpointing' of https://github.com…

a46bed3

…/oppia/oppia-android into add-controller-for-checkpointing

saving

b62ec7f

added mech to save checkpoints

f4b2380

lint fix

372b786

added module to UI-tests

72bf728

added exploration storage module to ui tests

894ef7d

nit

e5f0818

Merge branch 'add-module-to-tests' into saving-checkpoints

c263975

added module in missed files

ebd7ec5

lint fix

5825b72

fixed saving

ee26301

added app layer saving mechanism

8cb5dea

added app layer saving mechanism

026a25f

lint fix

8ecd952

fix lint in domain layer

6c32da0

MaskedCarrot requested a review from aggarwalpulkit596 July 3, 2021 05:30

MaskedCarrot assigned aggarwalpulkit596 and MaskedCarrot Jul 3, 2021

MaskedCarrot requested review from anandwana001, rt4914 and vinitamurthi as code owners July 3, 2021 05:30

MaskedCarrot changed the base branch from add-controller-for-checkpointing to add-module-to-tests July 3, 2021 05:30

lint fix

5994f0a

MaskedCarrot commented Jul 15, 2021

View reviewed changes

BenHenning reviewed Jul 16, 2021

View reviewed changes

BenHenning assigned MaskedCarrot and unassigned BenHenning Jul 16, 2021

MaskedCarrot added 3 commits July 16, 2021 08:32

nit

3b6ee57

Merge branch 'saving-checkpoints-alternate' into app-layer-mech-for-c…

da201e4

…heckpointing

nit

76bbc25

MaskedCarrot assigned BenHenning and aggarwalpulkit596 and unassigned MaskedCarrot Jul 16, 2021

MaskedCarrot requested a review from BenHenning July 16, 2021 03:11

aggarwalpulkit596 closed this Jul 16, 2021

aggarwalpulkit596 deleted the branch oppia:develop July 16, 2021 04:48

MaskedCarrot reopened this Jul 16, 2021

MaskedCarrot changed the base branch from saving-checkpoints-alternate to develop July 16, 2021 04:52

MaskedCarrot and others added 4 commits July 16, 2021 10:23

Merge branch 'develop' into app-layer-mech-for-checkpointing

1996f29

fixed dialog box

d1947d6

Merge remote-tracking branch 'localrepo/app-layer-mech-for-checkpoint…

3a6b22c

…ing' into app-layer-mech-for-checkpointing

disabled checkpointing, it was enabled for testing locally

ca1ba6e

BenHenning approved these changes Jul 16, 2021

View reviewed changes

app/src/main/res/values/strings.xml Show resolved Hide resolved

scripts/assets/test_file_exemptions.textproto Show resolved Hide resolved

BenHenning enabled auto-merge (squash) July 16, 2021 09:32

oppiabot bot unassigned BenHenning Jul 16, 2021

oppiabot bot added the PR: LGTM label Jul 16, 2021

oppiabot bot assigned MaskedCarrot Jul 16, 2021

BenHenning merged commit b5697a0 into oppia:develop Jul 16, 2021

MaskedCarrot deleted the app-layer-mech-for-checkpointing branch July 16, 2021 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3327: Create app layer mechanism for saving checkpoints. #3402

Fix #3327: Create app layer mechanism for saving checkpoints. #3402

MaskedCarrot commented Jul 3, 2021 •

edited

MaskedCarrot left a comment

MaskedCarrot Jul 15, 2021 •

edited

BenHenning Jul 16, 2021

MaskedCarrot Jul 16, 2021 •

edited

BenHenning left a comment

BenHenning Jul 16, 2021

BenHenning left a comment

BenHenning commented Jul 16, 2021

oppiabot bot commented Jul 16, 2021

oppiabot bot commented Jul 16, 2021

Fix #3327: Create app layer mechanism for saving checkpoints. #3402

Fix #3327: Create app layer mechanism for saving checkpoints. #3402

Conversation

MaskedCarrot commented Jul 3, 2021 • edited

Explanation

Checklist

Testing the changes made in this PR

Tests

Dialog boxes after this PR

MaskedCarrot left a comment

Choose a reason for hiding this comment

MaskedCarrot Jul 15, 2021 • edited

Choose a reason for hiding this comment

BenHenning Jul 16, 2021

Choose a reason for hiding this comment

MaskedCarrot Jul 16, 2021 • edited

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Jul 16, 2021

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Jul 16, 2021

oppiabot bot commented Jul 16, 2021

oppiabot bot commented Jul 16, 2021

MaskedCarrot commented Jul 3, 2021 •

edited

MaskedCarrot Jul 15, 2021 •

edited

MaskedCarrot Jul 16, 2021 •

edited