Adjust user messages around the Session Pause feature #2858

avazirna · 2024-09-18T21:35:12Z

Summary

Copy of #2851

cross-request: cross-request: dimagi/commcare-core#1437

avazirna · 2024-09-18T23:06:00Z

@damagatchi retest this please

shubham1g5 · 2024-09-19T03:31:06Z

@avazirna It doesn't look like the comments on the original PR are addressed in this PR, let me know if my understanding is wrong.

avazirna · 2024-09-19T21:46:59Z

@shubham1g5 I have entertained a few design alternatives over this, walking the fine line between structural changes and the bare minimum to achieve the goals of this PR. I'm more inclined to leave the structural work for 2.55 where we can have a proper QA and properly assess all the risks. I also don't think that deep changes should be rolled out in a hotfix, which I believe you agree with me. So in terms of future work, the main points are:

InterruptedFormState should evolve into InterruptedSessionState - we are now redundantly saving the SessionStateDescriptorId, individually and in the InterruptedFormState - this means using this model for both Session expiration (and suspension) and Session Pause;
- This requires a few change in the way we restore expired sessions
- There may be challenges to ensure backward compatibility, considering that users would be migrating from version with both
Stop storing the SSDId in SharedPreferences
Review the workflow around the is-restart-after-session-expiration - currently, it implies catering only for Session expiration but the reality is that with the changes we introduced, it's also serving the Session Pause feature

shubham1g5

Overall approach looks good to me, left some minor comments around code clarity and readability as optional but nice to have feedback.

shubham1g5 · 2024-09-20T08:36:29Z

app/src/org/commcare/activities/FormEntryActivity.java

        // note that we have started saving the form
        customFormSaveCallback = listener;
-        interruptAndSaveForm(exit);
+        interruptAndSaveForm(true, false);


it is not clear to me from any of the semantics around this method that formSaveCallback necessarily means that sessionExpiredOrSustended is true and userTriggered is false. I suggest that we should either let these values get passed as a paramenter to formSaveCallback OR rename formSaveCallback to make it clear that it's only every utilised by auto-save calls which are not a result of explicit user action and that the save is a result of session lifecycle.

done 3ade9b2

shubham1g5 · 2024-09-20T08:47:33Z

app/src/org/commcare/activities/FormEntryActivity.java

    }

-    private void interruptAndSaveForm(boolean exit) {
+    private void interruptAndSaveForm(boolean sessionExpiredOrSustended, boolean userTriggered) {


nit: rename sessionExpiredOrSustended -> sessionExpired (assiming session expiry and suspensions are same things here)

shubham1g5 · 2024-09-20T08:50:32Z

app/src/org/commcare/activities/FormEntryActivity.java

-    private void saveIncompleteFormToDisk(boolean exit) {
-        saveDataToDisk(exit, false, null, true);
+    private void saveIncompleteFormToDisk(boolean sessionExpiredOrSuspended, boolean userTriggered) {
+        boolean shouldExit = sessionExpiredOrSuspended ? FormEntryConstants.EXIT : FormEntryConstants.DO_NOT_EXIT;


would prefer if exit comes directly from caller of the save methods as before instead of us deriving it on the basis of sessionExpiry. (More scalable, would cause less changes in future if we decide to save due to another reason where we want user to exit as well)

done 3ade9b2

shubham1g5 · 2024-09-20T08:58:26Z

app/src/org/commcare/logic/AndroidFormController.java

@@ -20,13 +21,17 @@ public class AndroidFormController extends FormController implements PendingCall
    private boolean wasPendingCalloutCancelled;
    private FormIndex formIndexToReturnTo = null;
    private boolean formCompleteAndSaved = false;
+    private InterruptedFormState restoredFormSession;


nit: probably better to call it interrruptedFormState

shubham1g5 · 2024-09-20T09:00:07Z

app/src/org/commcare/logic/AndroidFormController.java


    private FormAnalyticsHelper formAnalyticsHelper;

-    public AndroidFormController(FormEntryController fec, boolean readOnly, FormIndex formIndex) {
+    public AndroidFormController(FormEntryController fec, boolean readOnly, InterruptedFormState savedFormSession) {


nit: inconsistent variable naming. would keep them all as interruptedFormState

shubham1g5 · 2024-09-20T09:01:10Z

app/src/org/commcare/logic/AndroidFormController.java

+        if (savedFormSession !=null ){
+            formIndexToReturnTo = savedFormSession.getFormIndex();
+        }


think we can remove global reference to formIndexToReturnTo and just derive it from restoredFormSession when needed

formIndexToReturnTo is nullified after its first use, which makes sense because we are supposed to return to that FormIndex only when loading the form after a session expiration or app kill. Am I missing something here?

makes sense. no need for a change here.

app/src/org/commcare/models/database/InterruptedFormState.java

shubham1g5 · 2024-09-20T09:08:31Z

@avazirna Can you create a ticket describing the pending list of items and make sure it gets prioritised for 2.55.

This reverts commit 2c3b7bb.

shubham1g5 · 2024-10-22T16:43:42Z

app/src/org/commcare/logic/AndroidFormController.java


    private FormAnalyticsHelper formAnalyticsHelper;

-    public AndroidFormController(FormEntryController fec, boolean readOnly, FormIndex formIndex) {
+    public AndroidFormController(FormEntryController fec, boolean readOnly, InterruptedFormState savedFormSession) {


can we mark savedFormSession and restoredFormSession as @Nullable

shubham1g5 · 2024-10-22T16:44:12Z

app/src/org/commcare/logic/AndroidFormController.java

        super(fec, readOnly);
        formAnalyticsHelper = new FormAnalyticsHelper();
-        formIndexToReturnTo = formIndex;
+        this.restoredFormSession = savedFormSession;
+        if (savedFormSession !=null ){


nit:formatting

shubham1g5 · 2024-10-23T08:29:26Z

app/src/org/commcare/models/database/InterruptedFormState.java

@@ -59,6 +63,10 @@ public FormIndex getFormIndex() {
        return formIndex;
    }

+    public boolean getInterruptedDueToSessionExpiration(){


nit: rename to isInterrupted..

shubham1g5 · 2024-10-23T08:35:22Z

app/src/org/commcare/activities/FormEntryActivity.java

@@ -793,7 +796,7 @@ protected void onExternalAttachmentUpdated() {
        }
        String formStatus = formRecord.getStatus();
        if (FormRecord.STATUS_INCOMPLETE.equals(formStatus)) {
-            saveDataToDisk(false, false, null, true);
+            saveDataToDisk(false, false, null, true, true);


not sure if this save isuserTriggered as it happens in response to updating an attachment.

true, good catch

shubham1g5 · 2024-10-23T08:37:26Z

app/src/org/commcare/activities/FormEntryActivity.java

@@ -1063,7 +1066,8 @@ private void loadForm() {
            }

            mFormLoaderTask = new FormLoaderTask<FormEntryActivity>(symetricKey, instanceIsReadOnly,
-                    formEntryRestoreSession.isRecording(), FormEntryInstanceState.mFormRecordPath, this, lastFormIndex) {
+                    formEntryRestoreSession.isRecording(), FormEntryInstanceState.mFormRecordPath, this, 


this looks weird to have a comma just before method call close. No compilation errors here ?

Noticed that too, bad conflict resolution. Redid it locally, pushing it in a few.

app/src/org/commcare/activities/FormEntryActivity.java

shubham1g5 · 2024-10-23T08:41:14Z

app/src/org/commcare/models/AndroidSessionWrapper.java

        if (sessionStateRecordId != -1) {
            SqlStorage<SessionStateDescriptor> sessionStorage =
                    CommCareApplication.instance().getUserStorage(SessionStateDescriptor.class);
            SessionStateDescriptor current = sessionStorage.read(sessionStateRecordId);
-            InterruptedFormState interruptedFormState = new InterruptedFormState(current.getID(), formIndex, current.getFormRecordId());
+
+            InterruptedFormState interruptedFormState = new InterruptedFormState(current.getID(), formIndex, current.getFormRecordId(), sessionExpired);


nit: not sure on formatting here.

…ving-session-form

avazirna · 2024-10-23T15:42:47Z

@damagatchi retest this please

avazirna requested a review from shubham1g5 September 18, 2024 21:35

avazirna added 8 commits September 19, 2024 14:33

Add interruption reason related flag to InterruptedFormState

e3acf14

Remove exit flag from formSaveCallback

2c3b7bb

Replace exit flag with sessionExpiredOrSuspended flag

3023424

Save interruption reason flag

37e044e

Pass InterruptionFormState to form controller

1836d2f

Show message when restoring session after session pause

d643cca

Add userTriggered flag when saving forms

312ade0

Don't show message when auto saving

2b994db

avazirna force-pushed the adjust-user-messages-when-autosaving-session-form branch from 7d90d94 to 2b994db Compare September 19, 2024 20:56

shubham1g5 approved these changes Sep 20, 2024

View reviewed changes

avazirna added 2 commits September 27, 2024 16:03

Refactor

296dc38

Revert "Remove exit flag from formSaveCallback"

6aaf0ff

This reverts commit 2c3b7bb.

shubham1g5 reviewed Oct 22, 2024

View reviewed changes

Refactor

3ade9b2

avazirna force-pushed the adjust-user-messages-when-autosaving-session-form branch from 5b4b1be to 3ade9b2 Compare October 22, 2024 16:53

Make class field nullable

7218aba

shubham1g5 requested changes Oct 23, 2024

View reviewed changes

Merge branch 'feature/givewell' into adjust-user-messages-when-autosa…

a776076

…ving-session-form

avazirna force-pushed the adjust-user-messages-when-autosaving-session-form branch from 15b4fea to a776076 Compare October 23, 2024 10:58

avazirna added 3 commits October 23, 2024 13:33

Nit

a27ab98

Correct userTriggered flag when updating attachments

019cd2d

Refactor

dec7568

avazirna requested a review from shubham1g5 October 23, 2024 11:42

shubham1g5 approved these changes Oct 23, 2024

View reviewed changes

avazirna merged commit aebd94e into feature/givewell Oct 23, 2024
1 check passed

avazirna mentioned this pull request Oct 23, 2024

Feature/givewell #2805

Merged

3 tasks

avazirna deleted the adjust-user-messages-when-autosaving-session-form branch October 23, 2024 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust user messages around the Session Pause feature #2858

Adjust user messages around the Session Pause feature #2858

avazirna commented Sep 18, 2024 •

edited

Loading

avazirna commented Sep 18, 2024

shubham1g5 commented Sep 19, 2024

avazirna commented Sep 19, 2024

shubham1g5 left a comment

shubham1g5 Sep 20, 2024

avazirna Oct 22, 2024

shubham1g5 Sep 20, 2024

shubham1g5 Sep 20, 2024

avazirna Oct 22, 2024

shubham1g5 Sep 20, 2024

shubham1g5 Sep 20, 2024

shubham1g5 Sep 20, 2024

avazirna Oct 22, 2024

shubham1g5 Oct 22, 2024

shubham1g5 commented Sep 20, 2024

shubham1g5 Oct 22, 2024

avazirna Oct 22, 2024

shubham1g5 Oct 22, 2024

shubham1g5 Oct 23, 2024

avazirna Oct 23, 2024

shubham1g5 Oct 23, 2024

avazirna Oct 23, 2024

avazirna Oct 23, 2024

shubham1g5 Oct 23, 2024

avazirna Oct 23, 2024

shubham1g5 Oct 23, 2024

avazirna commented Oct 23, 2024

Adjust user messages around the Session Pause feature #2858

Adjust user messages around the Session Pause feature #2858

Conversation

avazirna commented Sep 18, 2024 • edited Loading

Summary

avazirna commented Sep 18, 2024

shubham1g5 commented Sep 19, 2024

avazirna commented Sep 19, 2024

shubham1g5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shubham1g5 commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avazirna commented Oct 23, 2024

avazirna commented Sep 18, 2024 •

edited

Loading