554936 - Make statusbar text "Set Up Weave..." until Weave has been configured

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

We're getting far fewer accounts created than our downloads would suggest we should, and there's a concern that people won't remember how to get back, especially since we've dropped the first-run page.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Sets up the string, shows it if you haven't synced (more specifically, if you don't have a username / password / passphrase set). Also makes it so clicking the status bar when you're in the "needs setup" state brings up the prefs window instead of the menu.

Also made a slight reordering of flow to my patch in bug 539057 (which should go there), which makes it so when you "start over" the status bar updates to the "set up" message instead of "not signed in"

Comment 2

[Ed Lee :Mardak]

Will this also show "Set up Weave..." if logging in fails on a bad password/passphrase?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 436287 [details] [diff] [review]
Patch v0.1

(apart from the XXX comment, this isn't a WIP, autocompleted wrong)

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 436287 [details] [diff] [review]
Patch v0.1


>+  _needsSetup: function() {
>+    // return !Weave.Service.username;
>+    //XXXzpao For a more complicated & thorough approach...

kill the // return line
please explain the logic a little... and file a bug targeting 1.3 on making mpLocked a part of the Utils API.

>   startOver: function() {
>-    this.logout();
>     // Reset all engines
>     this.resetClient();
>     // Reset Weave prefs
>     Svc.Prefs.resetBranch("");
>+    this.logout();

logout() sets autoconnect to false, so the reset needs to come last.  Just clear the username explicitly before logout (this.username = "").

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Bah, this isn't quit ready. It interacts poorly with the case from bug 545958 comment 1. We end up not showing anything at all in the status bar because the error isn't getting caught. But even catching the error (and saying that needs setup) puts us in a weird state because the wrong page in the prefs is shown and you can continue forward without having a passphrase - which then sets the status bar to "set up..." which is technically right but doesn't do the right thing...

I'll try to get this updated ASAP.

(In reply to comment #2)
> Will this also show "Set up Weave..." if logging in fails on a bad
> password/passphrase?

It appears to, yes.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #4)
> file a bug targeting 1.3 on making mpLocked a part of the Utils API.

Filed bug 556710.

Also, making this work really required a bunch more plumbing so I did that and made opening the pref pane more awesome in the case of not having a password/passphrase. I think I've tested to death the edge cases, so should be in better shape than the previous patch. Expect it shortly.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Like I said ^^^, it's better. The big change from before is the pref pane opens the right page if there weren't explicit errors (when autoconnect = false). I somehow got into that state locally, which caused issues.

The only problem with this is that if you get into the state of missing a password/passphrase, you have to put it in if you want to change accounts or change sync options. But that seems like the edge of an edge case, so that's probably ok.

Comment 8

[Ed Lee :Mardak]

Comment on attachment 436653 [details] [diff] [review]
Patch v0.2

>+++ b/source/chrome/content/preferences/fx-prefs.js
>+    // We can open the prefs window before login was attempted (autoconnect == false),
>+    // so we should do some initial double checking before determing which page to show.
>+    // We only want to do this when we have a username so we don't trigger an error
>+    // case instead of the full setup case.
>+    // Case 1: No password:
>+    if (Weave.Service.username && !Weave.Service._mpLocked() &&
>+        !Weave.Service.password)
>+      Weave.Status.login = Weave.LOGIN_FAILED_NO_PASSWORD;
>+    // Case 2: No passphrase:
>+    else if (Weave.Service.username && !Weave.Service._mpLocked() &&
>+             !Weave.Service.passphrase)
>+      Weave.Status.login = Weave.LOGIN_FAILED_NO_PASSPHRASE;
Seems a bit odd to do this here. These login bits can be set in service.js Service.onStartup perhaps before notifying service:ready. It can check for username/mplocked+password/mplocked+passphrase and set LOGIN_FAILED_NO_USERNAME, etc.

Could put that in a separate function and reuse that from login().

Then perhaps needssetup would check for FAILED_NO_USERNAME/PASSWORD/PASSPHRASE. startOver would need to be updated to set the login status.

We'll see if mconnor had anything particular in mind.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #8)
> (From update of attachment 436653 [details] [diff] [review])
> >+++ b/source/chrome/content/preferences/fx-prefs.js
> >+    // We can open the prefs window before login was attempted (autoconnect == false),
> >+    // so we should do some initial double checking before determing which page to show.
> >+    // We only want to do this when we have a username so we don't trigger an error
> >+    // case instead of the full setup case.
> >+    // Case 1: No password:
> >+    if (Weave.Service.username && !Weave.Service._mpLocked() &&
> >+        !Weave.Service.password)
> >+      Weave.Status.login = Weave.LOGIN_FAILED_NO_PASSWORD;
> >+    // Case 2: No passphrase:
> >+    else if (Weave.Service.username && !Weave.Service._mpLocked() &&
> >+             !Weave.Service.passphrase)
> >+      Weave.Status.login = Weave.LOGIN_FAILED_NO_PASSPHRASE;
> Seems a bit odd to do this here. These login bits can be set in service.js
> Service.onStartup perhaps before notifying service:ready. It can check for
> username/mplocked+password/mplocked+passphrase and set
> LOGIN_FAILED_NO_USERNAME, etc.

I agree, but was sort of a tack-on somewhere approach. I also think this should only run when Weave.Status.login is falsey. Otherwise it could override an already set status.

Comment 10

[Ed Lee :Mardak]

(In reply to comment #9)
> only run when Weave.Status.login is falsey. Otherwise it could override an
> already set status.
Yeah. This would only get run at startup and for normal login().

Comment 11

[Ed Lee :Mardak]

Curious, what's "configured"? Configured with the right password/passphrase or just has password/passphrase?

If I "set up a second computer" and type in the wrong password, it switches to Not Signed In.

Also, this might be related to mconnor's patch in bug 545725. But after providing the right password, it switches to Not Signed In and the /!\ when I'm about to enter a passphrase.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #11)
> Curious, what's "configured"? Configured with the right password/passphrase or
> just has password/passphrase?

Just has password/passphrase (if no login attempt has been made) or configured with the right ones if a login attempt has been made.

> If I "set up a second computer" and type in the wrong password, it switches to
> Not Signed In.
> 
> Also, this might be related to mconnor's patch in bug 545725. But after
> providing the right password, it switches to Not Signed In and the /!\ when I'm
> about to enter a passphrase.

I think those both fall under the same thing. That notifies "weave:service:login:error" which I wasn't updating with "set up ...". Updated that now.

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

Updated per above. Moved detection to purely error based & set errors as appropriate. Also cleaned up the password->passphrase entry so it shows "Set up ..." instead of "not signed in"

Comment 14

[Ed Lee :Mardak]

Comment on attachment 436794 [details] [diff] [review]
Patch v0.3

>+++ b/source/modules/service.js
>+        // verifyLogin sets the failure states here. They might also be set by
>+        // _setBasicLoginStatus for ERROR_FAILED_NO_*
nit: not all the errors have NO_

Comment 15

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/labs/weave/rev/e5f0b4c479af
* Added a "Set Up Weave..." status message which is determined by Status.login states
* The "Set Up Weave..." message opens the pref window directly instead of showing the menu
* Added an additional error state for NO_PASSPHRASE
* Added checks to onStartup to check some status (no username/password/passphrase) for when autoconnect=false
* When the prefpane is opened and there's an error, it will open to the correct page so that can be corrected
* If using a custom server, that will be reflected when shown that page in the prefpane

(didn't address nit from comment 14 because all the errors from _setBasicLoginStatus are ERROR_FAILED_NO_*) per our chat.

Comment 16

[Tracy Walker [:tracy]]

verified with Weave 1.2b4 on Fx 3.6.3