Skip to content

Conversation

@amstone326
Copy link
Contributor

@amstone326 amstone326 commented Nov 10, 2017

Addresses https://manage.dimagi.com/default.asp?247211 by using the stored SessionStateDescriptor to re-load an incomplete form after it is interrupted by session expiration.

@ctsims: This is a slightly different way of doing this than we discussed, but I think it's much cleaner and more reliable. When I started trying to utilize the session-saving infrastructure for this, the workflows I had to use felt both messy and brittle. I then noticed the paradigm that was being used in AndroidSessionWrapper.getExistingIncompleteCaseDescriptor() to detect when the session for which the user is trying to load a form already has an incomplete form in storage, and realized that a similar strategy could be used here. The strategy I adopted does a check upon login for a stored SessionStateDescriptor that has been marked as "interrupted" by a session expiration, and re-loads it and its corresponding FormRecord if so.

1 question I have: By doing it this way, the default result is that the user gets dumped into the FormHierarchyActivity for their saved form, rather than directly into form entry. It is easy enough to change that if that seems undesirable, but I didn't have a strong opinion either way. Let me know if you do.

@amstone326
Copy link
Contributor Author

Product Note: This will make it so that when form entry is interrupted by the automatic user session expiration, the user will be entered directly back into that form when they log back in, rather than having to figure out what happened and navigate to re-opening the saved form themselves.

@amstone326 amstone326 requested a review from ctsims November 13, 2017 17:03
@amstone326
Copy link
Contributor Author

@damagatchi retest this please

@amstone326
Copy link
Contributor Author

amstone326 commented Nov 15, 2017

@ctsims Ok I added the notion of wiping flags that shouldn't get attempted later if they don't happen on first login. Implementing this made me realize that the post-update sync flag was another good candidate for this paradigm, and accordingly belonged more in doLoginLaunchChecksInOrder() than in checkForPendingAppHealthActions(), so I moved it and included it in the flag-clearing functionality.

Copy link

@wpride wpride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me; unfortunately looks like another migration 22 has been merged :/

@amstone326
Copy link
Contributor Author

@wpride Thanks for the heads up, resolved conflicts. @ctsims Would still like you to take a last look at this before it's merged.

Copy link
Member

@ctsims ctsims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important change in the update class.

Other than that this looks good. One quick note that I still think there could be an alternative where instead of using the SSD to store the interrupted state, we could store, say, "interrupted_ssd_id" in shared preferences, since we only anticipate having one of those in-play at a time.

SessionStateDescriptor.class,
new ConcreteAndroidDbHelper(c, db));

for (SessionStateDescriptorV1 ssd : oldStorage) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amstone326

Important note: Within this file. all SSD references before this migration occurs (which is all of them for now) need to be on SSDV1, otherwise a Db update from, say, v5 to v22 could try to load the SSD it's manipulating as an SSDV2 and crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ctsims Very good catch, thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..Ah, I just realized your other suggestion makes this irrelevant. I think that is a better way to go so I'll do that refactor now

@amstone326
Copy link
Contributor Author

ok @ctsims went with your suggestion of storing the interrupted SSD's id in prefs instead of using the flag. Definitely simpler, and also has the advantage of allowing us to be more precise about which SSD we intend to retrieve

@amstone326
Copy link
Contributor Author

@damagatchi retest this please

Copy link
Member

@ctsims ctsims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of quick changes.

, "org.javarosa.core.model.actions.SendAction"

// Added in 2.41
, "org.commcare.android.database.user.models.SessionStateDescriptorV1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer exists and is causing a test failure now

}
SqlStorage<SessionStateDescriptor> sessionStorage =
CommCareApplication.instance().getUserStorage(SessionStateDescriptor.class);
SessionStateDescriptor interrupted = sessionStorage.read(idOfInterrupted);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two quick things:

  1. I don't know precisely why this could fail after fixing Added support to media files validation which are not url encoded. #2 below (no record exists), but I think that if this record isn't in session storage we shouldn't fail.
  2. Shared preferences are per-app, not per-user, so right now this can bleed across userdbs (which could cause Automatic GPS capture #1, but could also just cause bizarre behavior). Easiest fix is to append the user ID to the shared preferences key in the HiddenPreferences changes

@amstone326 amstone326 merged commit 34eb2b2 into master Dec 1, 2017
@amstone326 amstone326 deleted the return-to-form-after-session-expiration branch December 1, 2017 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants