963442 - Firefox Accounts (Sync) - surface notifications when Account needs user intervention

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

When the account needs to be verified, or needs a new password, or needs an upgraded Fennec, we should surface a persistent notification to inform the user of that fact.  For starters we'll drive towards the status activity, which should show what is wrong with the account and provide UI to deal with any problems.

Bonus -- I think this could close 709393 :)

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: github PR

Still lots that could be improved here.  We could make sure that the various status and update activities clean any pending notifications when they change state, deliver notifications themselves, etc.

Comment 2

[Nick Alexander :nalexander [he/him]]

Lots of questions for UX here.  Copy, icon, desired action...

Comment 3

[Richard Newman [:rnewman]]

Feedback:

* Title might want to mention "Firefox", or provide a call-to-action. Currently

  Couldn't sign in
  [e-mail]

Google's error is:

  Sign-in error for mozfennec@…
  Touch to sign in to your account.

* Add longtext?

* Re-connect hangs the UI for a second or two before the spinner starts.
01-24 15:26:24.726 I/Choreographer(28693): Skipped 229 frames!  The application may be doing too much work on its main thread.

Comment 4

[Nick Alexander :nalexander [he/him]]

> * Re-connect hangs the UI for a second or two before the spinner starts.
> 01-24 15:26:24.726 I/Choreographer(28693): Skipped 229 frames!  The
> application may be doing too much work on its main thread.

Known: Bug 963184.  I think I'll be fixing it as part of the 401/120 email normalization retry.

Comment 5

[Nick Alexander :nalexander [he/him]]

For posterity.  From the github review:

rnewman wondered if we might do `state.showOrClearNotification(NOTIFICATION_ID)`, saying:

I think it makes sense for the state to decide if it should take action, and even to take it. Strong opinion, weakly held, though.

nalexander replied:

I think we have different philosophies.  I like to write plain Java with a controlling Android App around it; you are happier to mix Android code into the plain Java pieces.  So, I'd rather the ugly adapter handle all the Android plumbing, while you'd prefer the states to know how to signal things about themselves.  I'm going to keep this as is for now, with the understanding that we might go hard down the path towards `State` and `Transition` instances know a lot about the Android world and take more action in future.  CCing this to the ticket, too.

Comment 6

[Nick Alexander :nalexander [he/him]]

Landed, backed out due to bad commit message, relanded:

https://hg.mozilla.org/services/services-central/rev/c975f01f9483
https://hg.mozilla.org/services/services-central/rev/ef2ac1589655
https://hg.mozilla.org/services/services-central/rev/4f0723488d5a

Comment 7

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8364876 [details] [review]
github PR

Reviewed on github.

Comment 8

[Francesco Lodolo [:flod]]

Is it possible to use more meaningful entity/variable names?

fxaccount_sync_sign_in_error_notification_text = Tap to sign in to &formatS;

I undestand there's a localization comment, but it wouldn't hurt to have something like loginaddress, loginemail, or something similar.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Francesco Lodolo [:flod] from comment #8)
> Is it possible to use more meaningful entity/variable names?
> 
> fxaccount_sync_sign_in_error_notification_text = Tap to sign in to &formatS;
> 
> I undestand there's a localization comment, but it wouldn't hurt to have
> something like loginaddress, loginemail, or something similar.

flod, do you mean the string name (fxaccount_sync_sign_in_...) or the &formatS?  I think the long name is pretty clear (well, if you speak English), and I tried to follow what Fennec did for the format strings.  E.g.,

<!-- Localization note (num_tabs2) : Number of tabs is always more than one.
     We can't use android plural forms, sadly. See bug #753859. -->
<!ENTITY num_tabs2 "&formatD; tabs">

I am happy to define

<!ENTITY loginEmail "&formatS;">

and then

<!ENTITY fxaccount_sync_sign_in_error_notification_text "Tap to sign in to &loginEmail;">

if that is what you prefer.

Comment 10

[Francesco Lodolo [:flod]]

I meant the name of the variable used inside the string ("formatS"), string ID is good.

If that requires creating a new string just to hold the variable's name, please ignore my request, had no idea this is needed on Android.

Comment 11

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/4f0723488d5a

Comment 12

[Aaron Train [:aaronmt]]

Saw a notification reminder to sign-in after account creation and prior to verification