134 - Improve UX of crosswalk to webview migration #162

mrsarm · 2021-03-11T15:09:19Z

Improve the UX after the migration restarting the app.

Issue: #134

TO DO:

Restart the app after the migration to avoid the login page
Add a nice activity / layout following the mockups while the user wait for the migration
~~Add also a better UI with activities / layout when the connection is lost~~ → Moved to Improve connection errors UX, especially for the XWalk-Webview migration process #163

Other improvements

Improved logging in the app (sorry if it adds noise, I started just with a few corrections and then I couldn't stop fixing it).
Upgrade build library com.android.tools.build:gradle ~~4.1.1~~ → 4.1.2, Android Studio marks the older version as insecure.

- Remove redundant DEBUG check performed by MedicLog.* methods - Better and more homogeneous logging messages - Add caller reference to Medic.log

Upgrade due security concerns highlighted by Android Studio, though there is no release notes with the issues fixed in the library

src/webview/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java

garethbowen · 2021-03-11T21:18:42Z

src/webview/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java

@@ -78,6 +80,8 @@ public void onReceiveValue(String result) {
 	private PhotoGrabber photoGrabber;
 	private SmsSender smsSender;

+	private boolean isMigrationRunning = false;


With this as a boolean it'll reset to false on restart. This may be an issue if it crashes or the user closes it. Can we store it somewhere persistent instead?

Yes, but solve that means make the migration process more resilient, with the ability to resume in the cases you mention, but it is out of scope I think for this ticket that only intent to improve the UI so users are not presented to the login page which cause many problems described in the ticket.

Currently, If the app crashes but the migration was finished, there won't be a problem, because the app will be restarted and with our without this patch the UI will load the CHT dashboard because a that point the cookies will be available again. If the crash happens during the migration, is likely the data will be corrupted and the user will need to reinstall the app losing the data, but again this PR cannot address that now.

I'm worried about edge cases like... What if the user spawns a second instance of this Activity? What happens if two migrations run in parallel, or one after the other?

However adding some sort of persistent flag is also risky... what if it gets set to migrating = true and then the migration fails? It may never recover.

This is going to have to be tested really thoroughly to avoid bricking or corrupting phones. Are you confident this is the safest approach?

garethbowen

I know you hadn't marked this for review, but thought it might help to add my two cents earlier rather than later!

I've added a couple of suggestions inline.

Additionally, the if (DEBUG) statements are there as a slight performance boost so in production the app doesn't have to initialise and format the parameters only for them to be GCed again almost immediately. This is best practice as far as I'm aware.

mrsarm · 2021-03-12T12:54:42Z

Additionally, the if (DEBUG) statements are there as a slight performance boost so in production the app doesn't have to initialise and format the parameters only for them to be GCed again almost immediately. This is best practice as far as I'm aware.

@garethbowen yes actually I removed it not just to make the code more readable without so many if (DEBUG) statements but also because performance, due that the trace method already do that check, and it does not perform the string formatting if DEBUG is true. I only kept the if (DEBUG) in the cases where the formatting is done outside the trace method because it requires more complex concatenations.

…gin page even if it is a fraction of seconds

mrsarm · 2021-03-12T20:16:01Z

Ready for review again, comments about the changes here: #134 (comment)

mrsarm · 2021-03-13T15:55:31Z

Quick test instructions

Not yet in AT but here are some observations of how to test the PR.

Because the XWalk version does not work in Android >= 10, and the Webview version is compiled to work only in Android >= 10 for now in the master branch and this branch, the way to test the PR would be install the XWalk version in an Android <10 phone, then upgrade the phone to Android >=10 and finally install the version on this PR, which is is too much for testing, and may not be an option because the device used for testing does not support Android upgrades to the version 10.

What I do for test is:

Get a phone with Android <10 and >=5 (Webview doesn't work with Android 4.4).
Install in the phone the unbranded XWalk version of the app using the latest available from our releases: armeabi-v7a or arm64-v8a APK.
Setup the app with one of the CHT environments suggested by the app, I could reproduce the error easily with the Gamma dev environment.
Log in with an offline user to sync the app and some user data.
Then to be able to install the Webview version of this PR over the version installed in your phone, you need to downgrade the minimal SDK supported in your local development installation of this branch, so go to build.gradle#L164 and set a API version supported by your phone or below, eg.:
```
minSdkVersion 23     // Android 6
```
Run the unbrandedWebviewDebug flavor of the app of this branch with adb, gradle or Android Studio over the same phone where the Xwalk version was installed.
You will see the app starts, and then after a few seconds the splash screen with the upgrade notification while the migration process happens, as described here in the ticket. Then instead of being presented with the login page of the CHT you should see the normal dashboard of the app, with the data already synchronized there.

garethbowen

The video walkthrough looks awesome. Great work!

I've left some comments inline. Because of the risk of data loss or bricking phones I'm being even more pedantic than usual...

garethbowen · 2021-03-14T20:27:17Z

src/webview/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java

@@ -78,6 +80,8 @@ public void onReceiveValue(String result) {
 	private PhotoGrabber photoGrabber;
 	private SmsSender smsSender;

+	private boolean isMigrationRunning = false;


I'm worried about edge cases like... What if the user spawns a second instance of this Activity? What happens if two migrations run in parallel, or one after the other?

However adding some sort of persistent flag is also risky... what if it gets set to migrating = true and then the migration fails? It may never recover.

This is going to have to be tested really thoroughly to avoid bricking or corrupting phones. Are you confident this is the safest approach?

garethbowen · 2021-03-14T20:28:47Z

src/webview/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java

+						log(this, "onPageStarted() :: Migration process in progress, and " +
+								"cookies were not loaded, restarting ...");
+						Context context = view.getContext();
+						Intent intent = new Intent(context, StartupActivity.class);
+						intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TASK);
+						context.startActivity(intent);
+						Runtime.getRuntime().exit(0);


This section is really important, and quite difficult to understand. Consider pulling out a private function that can be named clearly. In addition it probably needs some commenting about what this is doing and why it works.

garethbowen · 2021-03-14T20:31:27Z

src/webview/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java

+					isMigrationRunning = false;
+					CookieManager cookieManager = CookieManager.getInstance();
+					String cookie = cookieManager.getCookie(appUrl);
+					if (cookie == null) {


What happens if a user's cookies have expired (or be wiped manually)? I think it'll work... migration will pick up the IndexedDB, then on restart isMigrationRunning will be false, and the user will go to the login page (as expected). Is that right? Have you tested this case?

Yes, I tried to install this version over a XWalk installation that has loaded the webapp but logged out, and the migration runs and then the app display the login page as expected.

Also installed the app without previous installations of Xwalk to ensure the migration process is not started. Same if it was a XWalk installation but with no data previously loaded.

… in the arguments

…Started()`

src/main/res/values-fr/strings.xml

garethbowen

Awesome!

Approved - the build is failing but I'll let you dig in to that.

Feel free to create a separate issue/PR for the service worker cache so we can start the AT process on this one.

mrsarm · 2021-03-15T18:09:07Z

Awesome!

Approved - the build is failing but I'll let you dig in to that.

Thanks @garethbowen ! Yes the build error is due an issue in the jCenter servers that are responding with a HTTP 403 error when CI tries to pull one of the dependencies. I'll try to rebuild in a few minutes.

Feel free to create a separate issue/PR for the service worker cache so we can start the AT process on this one.

Yes, going to do that to move forward with testing.

Co-authored-by: Marc Abbyad <[email protected]>

mrsarm added 3 commits March 11, 2021 10:31

Logging improvements

d80d705

- Remove redundant DEBUG check performed by MedicLog.* methods - Better and more homogeneous logging messages - Add caller reference to Medic.log

Upgrade com.android.tools.build:gradle to 4.1.2

754e95b

Upgrade due security concerns highlighted by Android Studio, though there is no release notes with the issues fixed in the library

After XWalk → Webview migration restart app to avoid login page

a81bfa3

garethbowen reviewed Mar 11, 2021

View reviewed changes

src/webview/java/org/medicmobile/webapp/mobile/EmbeddedBrowserActivity.java Show resolved Hide resolved

garethbowen reviewed Mar 11, 2021

View reviewed changes

mrsarm added 2 commits March 12, 2021 13:51

Use onPageStarted event instead onPageFinished to avoid seeing the lo…

9f31159

…gin page even if it is a fraction of seconds

Add splash screen with spinner while the migration is happening

7c72508

mrsarm force-pushed the 134-imp-ux-migration branch from 38ba520 to 7c72508 Compare March 12, 2021 19:44

mrsarm marked this pull request as ready for review March 12, 2021 20:14

mrsarm mentioned this pull request Mar 12, 2021

Improve UX of crosswalk to webview migration #134

Closed

mrsarm requested a review from garethbowen March 12, 2021 20:14

garethbowen reviewed Mar 14, 2021

View reviewed changes

mrsarm added 2 commits March 15, 2021 13:58

Set if(DEBUG) again in the cases were there is a string computation…

3e49c59

… in the arguments

Refactor code and add comments of why the app is restarted in `onPage…

e378c7a

…Started()`

mrsarm commented Mar 15, 2021

View reviewed changes

src/main/res/values-fr/strings.xml Outdated Show resolved Hide resolved

garethbowen approved these changes Mar 15, 2021

View reviewed changes

Fix french translations

9958c97

Co-authored-by: Marc Abbyad <[email protected]>

mrsarm merged commit 2749d4d into master Mar 17, 2021

mrsarm mentioned this pull request Mar 19, 2021

163 - Improve connection errors UX (Webview) #165

Merged

mrsarm deleted the 134-imp-ux-migration branch March 29, 2021 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

134 - Improve UX of crosswalk to webview migration #162

134 - Improve UX of crosswalk to webview migration #162

mrsarm commented Mar 11, 2021 •

edited

Loading

garethbowen Mar 11, 2021

mrsarm Mar 12, 2021

garethbowen Mar 14, 2021

garethbowen left a comment

mrsarm commented Mar 12, 2021

mrsarm commented Mar 12, 2021

mrsarm commented Mar 13, 2021

garethbowen left a comment

garethbowen Mar 14, 2021

garethbowen Mar 14, 2021

garethbowen Mar 14, 2021

mrsarm Mar 15, 2021

garethbowen left a comment

mrsarm commented Mar 15, 2021

134 - Improve UX of crosswalk to webview migration #162

134 - Improve UX of crosswalk to webview migration #162

Conversation

mrsarm commented Mar 11, 2021 • edited Loading

TO DO:

Other improvements

garethbowen Mar 11, 2021

Choose a reason for hiding this comment

mrsarm Mar 12, 2021

Choose a reason for hiding this comment

garethbowen Mar 14, 2021

Choose a reason for hiding this comment

garethbowen left a comment

Choose a reason for hiding this comment

mrsarm commented Mar 12, 2021

mrsarm commented Mar 12, 2021

mrsarm commented Mar 13, 2021

Quick test instructions

garethbowen left a comment

Choose a reason for hiding this comment

garethbowen Mar 14, 2021

Choose a reason for hiding this comment

garethbowen Mar 14, 2021

Choose a reason for hiding this comment

garethbowen Mar 14, 2021

Choose a reason for hiding this comment

mrsarm Mar 15, 2021

Choose a reason for hiding this comment

garethbowen left a comment

Choose a reason for hiding this comment

mrsarm commented Mar 15, 2021

mrsarm commented Mar 11, 2021 •

edited

Loading