Closed Bug 963442 Opened 8 years ago Closed 8 years ago

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

Categories

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

All
Android
defect
Not set
normal

Tracking

(fennec29+)

VERIFIED FIXED
Firefox 29
Tracking Status
fennec 29+ ---

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file)

57 bytes, text/x-github-pull-request
nalexander
: review+
Details | Review
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 :)
Blocks: 709393
Attached file 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.
Attachment #8364876 - Flags: review?(rnewman)
Lots of questions for UX here.  Copy, icon, desired action...
tracking-fennec: --- → ?
Whiteboard: [qa+]
Feedback:

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

  Couldn't sign in
  foobar@baz.com

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.
> * 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.
Blocks: 963833
No longer blocks: 951304
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 on attachment 8364876 [details] [review]
github PR

Reviewed on github.
Attachment #8364876 - Flags: review?(rnewman) → review+
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.
(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.
Flags: needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)
tracking-fennec: ? → 29+
Assignee: nobody → nalexander
https://hg.mozilla.org/mozilla-central/rev/4f0723488d5a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [qa+][fixed in services] → [qa+]
Target Milestone: --- → Firefox 29
Saw a notification reminder to sign-in after account creation and prior to verification
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(fennec)
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.