Fix #3325: Domain layer mechanism for saving checkpoints. #3408

MaskedCarrot · 2021-07-05T11:35:20Z

Explanation

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.

Tests

test class	Screenshot
ExplorationActivityTest (Tests ignored here are due to an existing issue.)

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

BenHenning

Thanks @MaskedCarrot. I haven't been focusing as much on the specifics of the controllers or even code quality (though I've commented on things when I notice them), and instead have been focused on the actual data flow mechanisms. My remaining comments cover all of my remaining concerns; please address & respond to all of them (some older ones haven't yet been responded to by you--please make sure to respond to these also). Once all are addressed, I'm happy to approve this.

Also, please follow up in chat regarding the bit about not needing a DataProvider in ExplorationActivity if we should discuss that further.

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt

BenHenning · 2021-07-15T00:49:24Z

domain/build.gradle

@@ -26,6 +26,9 @@ android {

  kotlinOptions {
    jvmTarget = JavaVersion.VERSION_1_8
+    freeCompilerArgs += [
+        "-Xuse-experimental=kotlinx.coroutines.ExperimentalCoroutinesApi"


Reminder to address this comment fully @prayutsu.

BenHenning · 2021-07-15T00:49:50Z

domain/src/main/java/org/oppia/android/domain/exploration/ExplorationDataController.kt

+   * @param shouldSavePartialProgress the boolean that indicates if partial progress has to be saved
+   *     for the current exploration
+   * @return a one-time [LiveData] to observe whether initiating the play request succeeded.
+   *     The exploration may still ail to load, but this provides early-failure detection.


Typo in word 'fail'

Do you mean these words: early-failure ?

may still fail to load

domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgress.kt

domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressController.kt

vinitamurthi

LGTM thanks!

BenHenning

Thanks @MaskedCarrot. I have 4 nits and 1 reminder comment--please make sure to address all of these. The PR otherwise LGTM.

domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressController.kt

model/src/main/proto/exploration.proto

BenHenning · 2021-07-16T01:44:07Z

domain/src/main/java/org/oppia/android/domain/exploration/ExplorationDataController.kt

+   * @param shouldSavePartialProgress the boolean that indicates if partial progress has to be saved
+   *     for the current exploration
+   * @return a one-time [LiveData] to observe whether initiating the play request succeeded.
+   *     The exploration may still ail to load, but this provides early-failure detection.


may still fail to load

domain/src/main/java/org/oppia/android/domain/state/StateDeck.kt

BenHenning · 2021-07-16T01:49:01Z

domain/build.gradle

@@ -26,6 +26,9 @@ android {

  kotlinOptions {
    jvmTarget = JavaVersion.VERSION_1_8
+    freeCompilerArgs += [
+        "-Xuse-experimental=kotlinx.coroutines.ExperimentalCoroutinesApi"


@prayutsu reminder.

Disregarding the review as all the changes have been approved and we are running low on time.

MaskedCarrot and others added 25 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

lint fix

5994f0a

nit changes

3cfca4d

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

5fc512d

nit changes

30a2817

fixed function names

09bf7d7

fixed failing domain tests and some nits

12cc209

completed saving approach

04a3695

MaskedCarrot requested a review from aggarwalpulkit596 July 5, 2021 11:35

MaskedCarrot assigned aggarwalpulkit596 Jul 5, 2021

MaskedCarrot mentioned this pull request Jul 5, 2021

Fix #3325: Add domain layer mechanism to save checkpoints. #3403

Closed

8 tasks

MaskedCarrot added 2 commits July 5, 2021 17:44

added more tests and fixed failing tests

0cf493a

checkpointing disabled by default

1cdb4b9

added more tests

1ea247c

MaskedCarrot assigned vinitamurthi Jul 14, 2021

BenHenning previously requested changes Jul 15, 2021

View reviewed changes

BenHenning assigned MaskedCarrot and unassigned BenHenning Jul 15, 2021

vinitamurthi approved these changes Jul 15, 2021

View reviewed changes

vinitamurthi removed their assignment Jul 15, 2021

changed approach to stop exploration and nit changes

567998e

MaskedCarrot requested a review from BenHenning July 15, 2021 05:08

MaskedCarrot added 2 commits July 15, 2021 10:45

nit

d38002d

nit

6e4ee29

MaskedCarrot assigned BenHenning and unassigned MaskedCarrot Jul 15, 2021

MaskedCarrot added 3 commits July 15, 2021 12:04

nit

96f448d

nits in comments

46b989a

applied suggestion in stateDeck

16b3cf4

BenHenning reviewed Jul 16, 2021

View reviewed changes

BenHenning assigned MaskedCarrot and unassigned BenHenning Jul 16, 2021

nit

3b6ee57

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

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

aggarwalpulkit596 enabled auto-merge (squash) July 16, 2021 04:20

aggarwalpulkit596 merged commit c325fc8 into develop Jul 16, 2021

aggarwalpulkit596 deleted the saving-checkpoints-alternate branch July 16, 2021 04:47

MaskedCarrot restored the saving-checkpoints-alternate branch July 16, 2021 04:50

MaskedCarrot deleted the saving-checkpoints-alternate branch July 16, 2021 04:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3325: Domain layer mechanism for saving checkpoints. #3408

Fix #3325: Domain layer mechanism for saving checkpoints. #3408

MaskedCarrot commented Jul 5, 2021 •

edited

BenHenning left a comment

BenHenning Jul 15, 2021

BenHenning Jul 15, 2021

MaskedCarrot Jul 15, 2021 •

edited

BenHenning Jul 16, 2021

MaskedCarrot Jul 16, 2021

vinitamurthi left a comment

BenHenning left a comment

BenHenning Jul 16, 2021

BenHenning Jul 16, 2021

Fix #3325: Domain layer mechanism for saving checkpoints. #3408

Fix #3325: Domain layer mechanism for saving checkpoints. #3408

Conversation

MaskedCarrot commented Jul 5, 2021 • edited

Explanation

Checklist

Tests

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning Jul 15, 2021

Choose a reason for hiding this comment

BenHenning Jul 15, 2021

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

Choose a reason for hiding this comment

vinitamurthi left a comment

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 Jul 16, 2021

Choose a reason for hiding this comment

MaskedCarrot commented Jul 5, 2021 •

edited

MaskedCarrot Jul 15, 2021 •

edited