185: Network connectivity warning and error handling by eliserichards · Pull Request #286 · mozilla-lockwise/lockwise-android

eliserichards · 2018-12-17T22:44:54Z

Fixes #185

Testing and Review Notes

Note: there are a number of followup stories to implement thorough retry logic, etc.

To Do

review todo

change naming in NetworkStore etc
get rid of duplicate code in the NetworkErrorHelper
check what happens on release build with FxALoginFragment
file followup issue for retry button logic
-- Network error retry logic #373
Talk about this point with James: "Thirdly, you could just put the snackbar over the view, not doing the harder job of fitting it in between the squashed view and either the toolbar or the top of the screen."

engineering

~~- [x] add tests for NetworkStore~~
~~- [x] change tests for views to add new observers~~
~~- [x] finish visible/invisible error~~
~~- [x] implement sync/ ~~retest network~~ on "Retry"~~
~~-- when the network is down on one page and you navigate to another, Android "network unvailable" page shows up in addition to the message??~~
~~- [x] figure out what's going on with FxAlogin fragment~~
~~- [x] file issue in A-C for network availability and applicable followups for our board~~
-- #355
-- #356
-- #369

reviews

add “WIP” to the PR title if pushing up but not complete nor ready for review
double check the original issue to confirm it is fully satisfied
request the "UX" team perform a design review (if/when applicable)
make sure CI builds are passing (e.g.: fix lint and other errors)
check on the accessibility of any added UI

jhugman

Quick drive by!

I think that the store can be doing more, and we can remove the need for presenters to explicitly request a connectivity check.

Otherwise, looking good so far!

jhugman

So three areas of concern/in need of attention:

The NetworkStore api should be less reliant on CheckConnectivity actions, and more reliant on doOnSubscribe and doOnDispose of the isConnected observable.
I don't know what the retry button is supposed to do. This is presenter dependent, and should be more than displaying or hiding the error message, but I don't know what.
The NetworkErrorHelper could be made a little smaller. How can we delete some of this code by changing the XML in someway.

eliserichards · 2019-01-10T19:13:23Z

@jhugman :

The NetworkStore api should be less reliant on CheckConnectivity actions, and more reliant on doOnSubscribe and doOnDispose of the isConnected observable.

✅

I don't know what the retry button is supposed to do. This is presenter dependent, and should be more than displaying or hiding the error message, but I don't know what.

Right now the retry button is simply checking the connection status again. The thought being that, if the network is not connected, the user can recheck the connection once they reconnect to wifi or what have you. This should be addressed in a followup issue, making the retry logic more explicit (e.g. re-sync, reload a web page, etc). Also see @linuxwolf 's input on retry logic: #185 (comment)

The NetworkErrorHelper could be made a little smaller. How can we delete some of this code by changing the XML in someway.

I agree that the fragments should be made more consistent. I addressed this in a comment above, but I found that it was taking up too much time and abandoned it for now. Maybe address this in a followup issue as well? I'm not sure what the priority of doing a refactor like that would be.

eliserichards · 2019-01-14T23:32:15Z

@jhugman, question about doOnSubscribe()...

Currently, in my Presenters, I have the following:

networkStore.isConnected
             .doOnSubscribe { dispatcher.dispatch(NetworkAction.CheckConnectivity) }
             .subscribe(view::handleNetworkError).addTo(compositeDisposable)

We discussed this morning that .doOnSubscribe{ ... } should be moved to the NetworkStore instead of being called in every Presenter. I am stuck with where/how to implement this. My own theory makes me think that we should have something in the NetworkStore, where theinit accesses the Observable isConnected like this:

init {             
             ... 
             isConnected
                     .doOnSubscribe { dispatcher.dispatch(NetworkAction.CheckConnectivity) }
                     .addTo(compositeDisposable)
}

Is this the right path to go down? Or am I going down a rabbit hole and missing the point of the doOnSubscribe?

jhugman · 2019-01-15T11:46:10Z

@jhugman, question about doOnSubscribe()...

Currently, in my Presenters, I have the following:
networkStore.isConnected
             .doOnSubscribe { dispatcher.dispatch(NetworkAction.CheckConnectivity) }
             .subscribe(view::handleNetworkError).addTo(compositeDisposable)
My own theory makes me think that we should have something in the NetworkStore, where theinit accesses the Observable isConnected like this:
init {             
             ... 
             isConnected
                     .doOnSubscribe { dispatcher.dispatch(NetworkAction.CheckConnectivity) }
                     .addTo(compositeDisposable)
}
Is this the right path to go down? Or am I going down a rabbit hole and missing the point of the doOnSubscribe?

Yes, this looks like the right way to go about it.

The doOnSubscribe method is registering a closure to be run each time a new subscription is made on isConnected. We're putting it in the store so you can subscribe in the presenters, and immediately have it come back with an up-to-date state; without the presenters having to worry how that all happens.

If you think: presenters are subscribing to the store and the store is looking after the subscriptions; then the store will want to do something each time it receives a subscription.

--

Another thing you may consider is eliminating the dispatcher.dispatch step, and just call the checkConectivity() method directly. Other reviewers may have other opinions about whether to miss out the dispatch step or not.

changecourse · 2019-01-15T18:25:26Z

Hey @eliserichards I've pulled our 'needs-review' request for now. The latest builds have failed, so I figured I'd wait for you and @jhugman to sort out some of the functionality before we come in to test the experience. Feel free to throw us back on once complete. Let me know if you want to discuss further.

Addressed comments

nickbrandt

@eliserichards I'm having some issues with this build. I first tried signing in without an internet connection, and received a dialog that there was No Network. Are we unable to display this to the user as designed (toast message at top of screen under navbar, which we plan to use in other situations without network connection)?

Also, when I connect back to the internet, and try to sign in, the app crashes every time. I tried deleting the app and reinstalling, but still when I go to sign in the app crashes, therefore I am unable to test the other use cases.

Let me know once this is fixed and I'll retest. Thanks!

Insert warning into fragment Add message to webview Add store and action to check connection Add actions to webview fragment and presenter. Passing network availability to fragment Successfully sending network connection status\! Adding fragment to item list, item detail, and fxa login. Still commented out in FxA login. Added logic for retry button. NetworkStore tests AppWebPagePresenter test Fixing network store tests All errors visible. Next step: make them disappear. Making fxa default invisible Finishing warning visibility. Setup for presenter network tests Retry button functionality and error helper Tests, rebase, lint Add network checks to presenter tests Fix presenter tests Rebase & cleanup

eliserichards · 2019-01-19T00:43:52Z

@nickbrandt there is a followup issue for the crash you're seeing. That was the "temporary" fix that we implemented before we released. The issue #356 is On Deck right now, hopefully to be fixed soon.

My PR covers once you get to the FxA login screen and then turn the network off/on. Could you try it again and let me know if you're still getting crashes?

jhugman

👏🏼👏🏼👏🏼👏🏼👏🏼👏🏼

This is looking really really nice now. Well done.

There are things you should look at/do before landing:

file a followup to rationalize the methods and apparatuses for testing if we're running in a [unit-] testing environment.
fix the the tiny amount of nits left on this PR.

You've worked hard on this PR. Well done.

jhugman · 2019-01-17T19:07:55Z

    private fun leakCanary(): Boolean {
-        if (LeakCanary.isInAnalyzerProcess(this)) {
+        // disable LeakCanary when unitTesting
+        if (unitTesting) return false


We have at least one method to detect if we're unit testing. See isTesting and isUnitTesting. We probably shouldn't have two, but pick one of those, and use that; that'll likely do the same job as using a separate Application object.

jhugman · 2019-01-21T12:28:43Z

+        dispatcher.register.subscribe(dispatcherObserver)

-        subject = FxALoginPresenter(view, accountStore = accountStore)
+        networkStore.connectivityManager = connectivityManager


Not sure I understand this. The networkStore is a mock, and you're giving it mock connectivityManager?

you can't create an instance of a ConnectivityManager, it has to be mocked. And the network store's connectivity manager has to be instantiated or it will break

jhugman · 2019-01-21T12:30:12Z

+    }
+
+    override val unitTesting = true
+}


I'm not sure this is useful, given that we have isTesting methods.

The application was failing on Leak Canary, even before it reached the isTesting method. I'm going to file a followup for this

nickbrandt · 2019-01-22T15:59:16Z

@eliserichards I tried downloading the build again this morning, and I'm still experiencing crashes on going to the FxA screen with my network available.

linuxwolf · 2019-01-22T18:20:48Z

I just tried the release binary, from scratch (uninstall Lockbox, install from bitrise, launch). It crashes when I try to "Get Started". It appears, for some reason, that ConstraintLayout is expected but missing:

    --------- beginning of crash
2019-01-22 11:18:43.653 19065-19065/mozilla.lockbox E/AndroidRuntime: FATAL EXCEPTION: main
    Process: mozilla.lockbox, PID: 19065
    android.view.InflateException: Binary XML file line #20: Binary XML file line #7: Error inflating class android.support.constraint.ConstraintLayout
    Caused by: android.view.InflateException: Binary XML file line #7: Error inflating class android.support.constraint.ConstraintLayout
    Caused by: java.lang.ClassNotFoundException: Didn't find class "android.support.constraint.ConstraintLayout" on path: DexPathList[[zip file "/system/framework/org.apache.http.legacy.boot.jar", zip file "/data/app/mozilla.lockbox--1x_wLv-dOodeaFwv1R4sw==/base.apk"],nativeLibraryDirectories=[/data/app/mozilla.lockbox--1x_wLv-dOodeaFwv1R4sw==/lib/arm64, /data/app/mozilla.lockbox--1x_wLv-dOodeaFwv1R4sw==/base.apk!/lib/arm64-v8a, /system/lib64]]
        at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:134)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:379)
        at java.lang.ClassLoader.loadClass(ClassLoader.java:312)
        at android.view.LayoutInflater.createView(LayoutInflater.java:606)
        at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:790)
        at android.view.LayoutInflater.parseInclude(LayoutInflater.java:965)
        at android.view.LayoutInflater.rInflate(LayoutInflater.java:859)
        at android.view.LayoutInflater.rInflateChildren(LayoutInflater.java:824)
        at android.view.LayoutInflater.inflate(LayoutInflater.java:515)
        at android.view.LayoutInflater.inflate(LayoutInflater.java:423)
        at mozilla.lockbox.view.FxALoginFragment.onCreateView(FxALoginFragment.kt:45)
        at androidx.fragment.app.Fragment.performCreateView(Fragment.java:2440)
        at androidx.fragment.app.FragmentManagerImpl.moveToState(FragmentManagerImpl.java:885)
        at androidx.fragment.app.FragmentManagerImpl.addAddedFragments(FragmentManagerImpl.java:2078)
        at androidx.fragment.app.FragmentManagerImpl.executeOpsTogether(FragmentManagerImpl.java:1852)
        at androidx.fragment.app.FragmentManagerImpl.removeRedundantOperationsAndExecute(FragmentManagerImpl.java:1808)
        at androidx.fragment.app.FragmentManagerImpl.execPendingActions(FragmentManagerImpl.java:1709)
        at androidx.fragment.app.FragmentManagerImpl$1.run(FragmentManagerImpl.java:147)
        at android.os.Handler.handleCallback(Handler.java:873)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:193)
        at android.app.ActivityThread.main(ActivityThread.java:6718)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

linuxwolf · 2019-01-22T18:29:19Z

+  ~ License, v. 2.0. If a copy of the MPL was not distributed with this
+  ~ file, You can obtain one at http://mozilla.org/MPL/2.0/.
+  -->
+<android.support.constraint.ConstraintLayout


This should now be androidx.constraintlayout.widget.ConstraintLayout.

yep! already fixed 😄

Fixed crash, time to try again :)

nickbrandt

@eliserichards this looks great! Thanks for your hard work on this. 👏 🎉

The Retry button/link doesn't currently work, but I noticed you already have a follow-up issue for that 👍

✅ FxA Webviews

✅ Entry List (with Entries)

✅ Entry List (No Entries) - Note: Assuming this won't change anything, but will want to test this again once our Empty State PR (#397 ) is merged.

✅ FAQ/Ask Question/Feedback webviews

✅ Even added a nice dialog informing user they are not connected to the internet on Welcome/Get Started screen

* Message bar Insert warning into fragment Add message to webview Add store and action to check connection Add actions to webview fragment and presenter. Passing network availability to fragment Successfully sending network connection status\! Adding fragment to item list, item detail, and fxa login. Still commented out in FxA login. Added logic for retry button. NetworkStore tests AppWebPagePresenter test Fixing network store tests All errors visible. Next step: make them disappear. Making fxa default invisible Finishing warning visibility. Setup for presenter network tests Retry button functionality and error helper Tests, rebase, lint Add network checks to presenter tests Fix presenter tests Rebase & cleanup * Remove consumers from fragments * Adding string descriptions * Change subscriptions * Naming changes, lint errors * Refactor network error helper * fix lint error * Finalizing network store subscriptions * Presenter tests * Fixing tests and lint * Fixing rebase

eliserichards requested a review from a team as a code owner December 17, 2018 22:44

ghost assigned eliserichards Dec 17, 2018

ghost added the in progress label Dec 17, 2018

eliserichards force-pushed the 185-network-issues branch 4 times, most recently from 79b2715 to 1c84c1f Compare December 21, 2018 00:04

jhugman reviewed Dec 21, 2018

View reviewed changes

eliserichards force-pushed the 185-network-issues branch from 2a3e033 to 98e294d Compare December 21, 2018 21:56

eliserichards force-pushed the 185-network-issues branch 4 times, most recently from 5d236a1 to f00faf5 Compare January 8, 2019 19:45

eliserichards mentioned this pull request Jan 8, 2019

Add listener for network ConnectivityManager #355

Open

devinreams mentioned this pull request Jan 9, 2019

The login page is not displayed if turning the Internet ON after launching the app without Internet #361

Closed

eliserichards force-pushed the 185-network-issues branch from f00faf5 to eae36be Compare January 9, 2019 23:17

jhugman previously requested changes Jan 10, 2019

View reviewed changes

eliserichards force-pushed the 185-network-issues branch from 2fdf729 to 3d01cc2 Compare January 10, 2019 23:40

eliserichards changed the title ~~[WIP] 185: Network connectivity warning and error handling~~ 185: Network connectivity warning and error handling Jan 11, 2019

eliserichards force-pushed the 185-network-issues branch 2 times, most recently from f4ebb46 to eaeda6d Compare January 14, 2019 22:50

eliserichards requested a review from a team January 14, 2019 22:50

changecourse removed the request for review from a team January 15, 2019 18:24

linuxwolf mentioned this pull request Jan 15, 2019

Upgrade to A-C 0.38.0 #377

Merged

5 tasks

eliserichards force-pushed the 185-network-issues branch from d0c7ece to b00e69c Compare January 16, 2019 20:06

eliserichards requested a review from jhugman January 16, 2019 20:07

eliserichards force-pushed the 185-network-issues branch 2 times, most recently from 293a21d to 815d108 Compare January 18, 2019 02:24

eliserichards requested a review from a team January 18, 2019 02:38

nickbrandt previously requested changes Jan 18, 2019

View reviewed changes

eliserichards added 10 commits January 18, 2019 13:45

Remove consumers from fragments

1e7542d

Adding string descriptions

f5b9c65

Change subscriptions

5e971f2

Naming changes, lint errors

2d5d151

Refactor network error helper

d50afc0

fix lint error

a84368d

Finalizing network store subscriptions

e4d9335

Presenter tests

3f6bcb4

Fixing tests and lint

95b08c0

eliserichards force-pushed the 185-network-issues branch from 815d108 to 95b08c0 Compare January 18, 2019 21:45

jhugman approved these changes Jan 21, 2019

View reviewed changes

linuxwolf reviewed Jan 22, 2019

View reviewed changes

Fixing rebase

10cb416

nickbrandt self-requested a review January 22, 2019 20:35

nickbrandt approved these changes Jan 22, 2019

View reviewed changes

eliserichards merged commit ffe51ae into master Jan 22, 2019

ghost removed the in progress label Jan 22, 2019

sandysage mentioned this pull request Jan 23, 2019

"No network issue" error although on network #350

Closed

eliserichards deleted the 185-network-issues branch February 8, 2019 21:31

Conversation

eliserichards commented Dec 17, 2018 • edited by devinreams Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing and Review Notes

To Do

review todo

engineering

reviews

Uh oh!

jhugman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhugman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliserichards commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eliserichards commented Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhugman commented Jan 15, 2019

Uh oh!

changecourse commented Jan 15, 2019

Uh oh!

nickbrandt left a comment

Choose a reason for hiding this comment

Uh oh!

eliserichards commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhugman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhugman Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhugman Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

eliserichards Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhugman Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

eliserichards Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nickbrandt commented Jan 22, 2019

Uh oh!

linuxwolf commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linuxwolf Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

eliserichards Jan 22, 2019

Choose a reason for hiding this comment

eliserichards commented Dec 17, 2018 •

edited by devinreams

Loading

eliserichards commented Jan 10, 2019 •

edited

Loading

eliserichards commented Jan 14, 2019 •

edited

Loading

eliserichards commented Jan 19, 2019 •

edited

Loading

linuxwolf commented Jan 22, 2019 •

edited

Loading

nickbrandt left a comment •

edited

Loading