Perform the WPCOM email+password login in the background by hypest · Pull Request #6878 · wordpress-mobile/WordPress-Android

hypest · 2017-11-17T14:10:37Z

This PR introduces a Service to handle logging in to WPCOM when the email+password flow is used. That's the flow mentioned in the ticket and this PR is on-purpose limited to that flow so the changeset is smaller and more manageable.

There is some code duplication with LoginBaseFormFragment (mainly the onAccountChanged and onSiteChanged listeners) which should be lifted when all login flows switch to using a Service.

This PR builds on top of the Service implementation introduced with #6825. See the FAQ section below for more information.

F.A.Q

Why use a service?

A Service is used so the login procedure can continue even if the app is backgrounded. The FluxC messages that are now missed by the screen Fragments are properly received and handled in the Service without interruption.

AutoForeground class. What black magic is that??

This class monitors whether the Service has bound "clients" or not. If bound clients exist, Service's state updates are delivered via an EventBus message. If no clients are bound, the Service gets promoted to foreground and its updates are forwarded to a status bar notification. As clients get bound or unbound, this class automatically toggles the Service foreground-ness and hides or shows the notifications.

Why is the Service promoted to a foreground one?

The Service is promoted to a foreground one to make sure the system doesn't kill it, even on Android 8.0.

Why is a client (LoginEmailPasswordFragment) attaching and detaching in `onResume` and `onPause`?

The client (the app UI essentially) attaches or detaches to denote that it wants to start or stop listening to the Service updates. That's similar to how other Fragments register/de-register from EventBus. The added functionality here is that when detached, updates are forwarded to the status bar notification.

Sticky EventBus messages? What's the deal there?

The Service is designed to start and then stop itself on success or failure. The app may be in the background when the Service stops and the sticky message makes sure the latest state of the Service is available to pick when the app resumes. The sticky message also plays the role of state retention and the UI doesn't need to save it in onSaveInstanceState or similar.

Any downsides to using a sticky message?

Yes, someone has to cleanup or initialise the state. See usages of the clearLoginServiceState() method.

Any downsides to using a sticky message?

Yes, someone has to cleanup or initialise the state. See usages of the clearLoginServiceState() method.

Why not always `clearLoginServiceState()` on the Fragment's start?

We want the client (Fragment in this case) to pick up the current state of the Service. Clearing it always when starting breaks the pattern.

Why is the "logging in" notification sticky?

That notification is active when the Service is in the foreground and its work hasn't finished yet. Android API demands the notification to be sticky under those conditions.

Why are the error or success notifications dismissible?

The Service is finished by that time. Failure or success, nothing is running in the background anymore and the sticky notification doesn't make sense. User can dismiss the notification and resume the app whenever they prefer to continue.

To test:

With the app data cleaned, launch the app
Tap on "LOG IN", input your WPCOM email in the email screen and tap NEXT
Tap on "Enter your password instead" link to drop to the password input screen
Put in your password, tap NEXT and immediately hit "Home" to exit the app
A notification should appear. It would show "Logging in..." or a success/failure message depending on how the login process went. For example, try to put in a wrong password in the previous step.
If you try this on a 2FA enabled account, the notification will inform you of the fact that a 2FA is needed to continue.
Tap on the notifications or perhaps resume the app from the launcher as normal. In any case the notification should hide.

The following flow is "touched" by this PR and should continue to work as normal:

Social login to a 2FA enabled WPCOM account that was not connected before (web nor mobile)

/cc @mzorz you might want to have a look at this. It's based on the solution we reviewed last week and builds on top of it. Thanks!

/cc @theck13 there are some Social login things touched here so, I would appreciate a pass to make sure nothing broke there. Thanks!

The fragment attaches, detaches and spawns the email+password login Service, updating its UI to reflect the process's stage.

mzorz · 2017-11-17T19:52:28Z

+        // connect to the Service. We'll receive updates via EventBus.
+        mServiceEventConnection = new AutoForeground.ServiceEventConnection(getContext(), LoginWpcomService.class, this);
+
+        // install the change listener as late as possible so the UI can be setup before triggering the


the sentence in the comment is ended abruptly "before triggering the".

🤦‍♂️ what a cliffhanger!
Fixed with c8287d3.

mzorz · 2017-11-17T21:13:00Z

+    public static void clearLoginServiceState() {
+        OnLoginStateUpdated onLoginStateUpdated = EventBus.getDefault().removeStickyEvent(OnLoginStateUpdated.class);
+        if (onLoginStateUpdated != null && onLoginStateUpdated.state.isTerminal()) {
+            EventBus.getDefault().removeStickyEvent(OnLoginStateUpdated.class);


I'm not sure why removeStickyEvent is called twice on the OnLoginStateUpdated.class here - I believe in some other part of the code there's the assumption that 2 sticky events are posted when the last sticky one is isTerminal()? but I'm only guessing here. If we need to call removeStickyEvent twice let's add a comment to clarify what's happening here (i.e. what the first removal does, and what the second one does, and when that's needed)

Good catch! This was actually a bad merge situation where extra code was left in. Sorry for the confusion! Sticky event just needs to be removed once. Fixed with 3277502.

mzorz · 2017-11-17T21:19:55Z

+        mDispatcher.dispatch(AccountActionBuilder.newFetchAccountAction());
+    }
+
+    private EventBus getEventBus() {


this method looks unnecessary - we can just call EventBus.getDefault()

Right! I had grand plans for the getEventBus() method but when the plans changed I forgot to remove it. Fixed with d4f65a6.

mzorz · 2017-11-17T23:56:14Z

+    private static Intent getPendingIntent(Context context) {
+        Intent resumeIntent = context.getPackageManager().getLaunchIntentForPackage(context.getPackageName());
+
+        // apparently, we need to null out the package name to make the intent resume the app


wow didn't know this. Interesting! Do we have a reference or explanation as to why this is like that? Something we can put in here to explain the code a bit more

That's something I noticed as the difference between getLaunchIntentForPackage() and the simple Intent I manually constructed (like this). The Intent returned from getLaunchIntentForPackage() fails to just resume the app with the current Task in it. I don't have a reference to this behaviour.

Can we add a comment explaining that what this getPendingIntent actually tries to do is to recover the same intent that was launched in the first place, and uses it for making the Notification tap action "resume" the original intent?

Sure thing, makes sense. Did it with b8243bc. I've also renamed the method with ac72de2 for closer name to its functionality.

👍 I've been considering this for a while to check if there's an alternative, "cleaner" way of resuming the app (and also possibly have better code instead of a comment clarifying why we are practically defeating the purpose of calling getLaunchIntent**ForPackage** and then nullifying the intent's pacakage immediately after). I would have liked to use explicit intents but that also means we should be able to tell which Activity the user was on, thus making the whole process more convoluted. Just wanted to clarify why I thought a longer comment was good to have here.

Totally, I'm there with you. I think that getLaunchIntentForPackage() had just one job to do but yeah, falls short apparently.

mzorz · 2017-11-18T00:13:30Z

+
+    private static NotificationCompat.Builder getNotificationBuilder(Context context, @StringRes int title,
+            @StringRes int content) {
+        NotificationCompat.BigTextStyle bigTextStyle = new NotificationCompat.BigTextStyle();


I think we need to set the normal notification text as well here, otherwise when the notification is collapsed a blank notification is shown:

Once you drag the bottom edge down to expand it, the bigstyle text appears:

So, we'd need to add

notificationBuilder.setContentTitle(title); notificationBuilder.setContentText(content);

to the NotificationCompat.Builder(context) concatenation right below

Good call. I opted (with 01b1449) to setting the standard title only so the collapsed notification can look, well, collapsed :). Let me know if this is OK or you'd prefer the standard contentText to be set as well. Thanks!

In the meantime, I removed the big icon anyway to match other notification styles in the app and this has the added benefit that the collapsed notification looks less empty.

I think I would rather have both the title and the text on the notification so the user can know the reasons why "Login stopped" and which actions are required right at a glance. Otherwise they'd only see the title i..e "Login stopped" with no further reasons or actions to take (the "Please provide an authentication code to continue" is fully informative in this regard, you know what's about to happen after tapping, as opposed to not knowing what to expect with just the "login stopped" title).

Hmm, I see your point @mzorz . I think that the standard content text doesn't allow for multiple lines though and the informative texts you mention actually get clipped. It depends on the screen size too of course but, that's basically the reason I employed the BigStyle notification as well.

I made the change with 398faac.

I know, it's tricky to have all possibilities in mind. From my experience, best way to keep it with high chances of showing for everybody / giving people chance to just expand without opening to try and see what happened, is to set title and content on both normal and BigStyle, as it is now 👍

mzorz · 2017-11-18T00:29:58Z

Nice @hypest ! This one looks good for setting the base for next steps where we have other Dialogs like this one as explained in #6700. I made a comment about the notification text that should be addressed, and then it's ok to go for me.

Let's also wait for @theck13 's input and we'll be ready to roll! 👍

That better matches other notifications in the app.

This way, a collapsed notification can still display useful information.

hypest · 2017-11-18T01:05:40Z

Thanks for the the review @mzorz ! I've addressed all your feedback and this is ready for another pass.

... looks good for setting the base for next steps where we have other Dialogs like this one as explained in #6700

Yes, there are dialogs like this one in several screens in the new Login and those screens too will need to be gradually ported to using the Service. This PR was limited to just one screen to keep it small and lay the groundwork.

theck13

I tested the social login scenarios and they pass. It looks good to me.

hypest · 2017-11-27T10:46:00Z

Thanks for looking into this @theck13 !

@mzorz , this seems clear for another pass or merge. Let me know if something more is needed. Thanks!

hypest · 2017-11-27T14:17:25Z

@mzorz I've pushed a couple of commits to add some Tracks of the LoginWpcomService state to help us gain insight of the cases where the app does get backgrounded while on the email/password login screen.

I've also updated the PR with latest develop. Ready for another pass, thanks!

mzorz · 2017-11-27T14:53:02Z

+
+    private void track() {
+        Map<String, Object> props = new HashMap<>();
+        props.put("login_phase", mLoginPhase.name());


can we just make sure mLoginPhase is not null here?

Makes sense. Added a null check with f83dd70.

mzorz · 2017-11-27T14:54:47Z

Just commented a minor nit and we'll be ready to go @hypest 👍

hypest · 2017-11-27T14:58:35Z

Addressed the feedback @mzorz , ready for another pass 👍

mzorz · 2017-11-27T15:01:54Z

    private void track() {
        Map<String, Object> props = new HashMap<>();
-        props.put("login_phase", mLoginPhase.name());
+        props.put("login_phase", mLoginPhase == null ? "null" : mLoginPhase.name());


mzorz · 2017-11-27T15:02:29Z

LGTM 🎉

hypest added 23 commits November 17, 2017 02:46

First stab at a login AutoForeground Service

06a36b4

The fragment attaches, detaches and spawns the email+password login Service, updating its UI to reflect the process's stage.

Introduce a Service state for social login

390ea2d

Notify and resume for 2FA

d434308

Small code optimization

d0e8a35

Non cancellable email+password login

0d78db4

Separate class for the login notification builder

458bce7

Simplify the login progress notification message

7d1e7fa

Sticky EventBus message to hold login Service state

cccace6

Clear sticky login state when leaving email/pass screen

05cdbcb

Remove notifications when client gets bound

50113dc

Use the provided onLoginFinished(boolean)

4583596

Handle some more failure states in the UI

d375d72

Minor cleanup

8b39c8b

Code cleanup to remove AS warnings

b228183

Initialise the login state sticky event

9386143

SUCCESS doesn't need a progress percentage

8ed3321

Missing cases from isError()

fe8369b

String resources, nicer login notifications

f386f95

LoginWpcomService is not involved in this flow

a7c3773

clearLoginServiceState belongs in LoginWpcomService

0efd828

More specific name for the method

8db8efd

No need to start the reader/notif services yet

bc0c976

Contain clearLoginServiceState's usage

e102757

mzorz reviewed Nov 17, 2017

View reviewed changes

hypest added 4 commits November 18, 2017 01:23

Big title and big content for all login notifications

ec41e9e

Merge branch 'develop' into issue/6700-login-progress-dialog-forever

622f480

Finish the sentence

c8287d3

Just clear the sticky event. Leftovers from bad merge.

3277502

Minor code optimization

d4f65a6

mzorz reviewed Nov 17, 2017

View reviewed changes

mzorz reviewed Nov 18, 2017

View reviewed changes

hypest added 2 commits November 18, 2017 02:53

Don't set Large icon in login notifications

e4512f8

That better matches other notifications in the app.

Set title for standard notification style

01b1449

This way, a collapsed notification can still display useful information.

hypest added 3 commits November 20, 2017 15:11

Rename, better comments for getResumeIntent

b8243bc

Rename the method for better description

ac72de2

Set the standard content text along with the title

398faac

theck13 self-requested a review November 24, 2017 01:53

theck13 approved these changes Nov 24, 2017

View reviewed changes

hypest added 3 commits November 27, 2017 15:34

Add Tracks of the service phase updates

d3124f8

Track early, before foregroundness changes

4e6526c

Merge branch 'develop' into issue/6700-login-progress-dialog-forever

7959b98

mzorz reviewed Nov 27, 2017

View reviewed changes

Null check for mLoginPhase

f83dd70

mzorz reviewed Nov 27, 2017

View reviewed changes

mzorz merged commit 9bcd8ba into develop Nov 27, 2017

hypest mentioned this pull request Nov 28, 2017

Update SiteCreationService from latest AutoForeground #6916

Merged

hypest deleted the issue/6700-login-progress-dialog-forever branch November 29, 2017 07:01

Uh oh!

Conversation

hypest commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

F.A.Q

Why use a service?

AutoForeground class. What black magic is that??

Why is the Service promoted to a foreground one?

Why is a client (LoginEmailPasswordFragment) attaching and detaching in onResume and onPause?

Sticky EventBus messages? What's the deal there?

Any downsides to using a sticky message?

Any downsides to using a sticky message?

Why not always clearLoginServiceState() on the Fragment's start?

Why is the "logging in" notification sticky?

Why are the error or success notifications dismissible?

To test:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzorz commented Nov 18, 2017

Uh oh!

hypest commented Nov 18, 2017

Uh oh!

theck13 left a comment

Choose a reason for hiding this comment

Uh oh!

hypest commented Nov 27, 2017

Uh oh!

hypest commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzorz commented Nov 27, 2017

Uh oh!

hypest commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzorz commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

hypest commented Nov 17, 2017 •

edited

Loading

Why is a client (LoginEmailPasswordFragment) attaching and detaching in `onResume` and `onPause`?

Why not always `clearLoginServiceState()` on the Fragment's start?

hypest Nov 18, 2017 •

edited

Loading

hypest Nov 20, 2017 •

edited

Loading