Closed Bug 642969 Opened 9 years ago Closed 9 years ago

"i have lost my other device" link doesn't do anything at all, unless you've typed username and password

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla6

People

(Reporter: dholbert, Assigned: emtwo)

References

Details

(Whiteboard: [good first bug][verified in services])

Attachments

(1 file, 4 obsolete files)

STEPS TO REPRODUCE:
 1. new firefox profile, for simplicity
 2. Tools | Set up Sync
 3. Click "Connect" button under "I already have a sync account"
 4. Click "I don't have the device with me" at the bottom
   ( 4a. If you like, enter your username. )
 5. Click "I have lost my other device"

ACTUAL RESULTS: Nothing happens

If you type in a password (even gibberish), then you get some response from the UI, but otherwise, there's no visible response from step 5.
Summary: "i have lost my other device" UI doesn't work, unless you've typed username and password → "i have lost my other device" link doesn't do anything at all, unless you've typed username and password
Agreed, we should provide visual response in all cases.
Thanks, philikon.

To be clearer about current behavior: if you enter *either* your password *or* your username (or neither), then the link has no visible effect.

There's only a visible response right now if you've typed in *both* the username and password.
Whiteboard: [good first bug]
Assignee: nobody → msamuel
Attached patch A patch (obsolete) — Splinter Review
Wanted to get something submitted for this before you left :)
Attachment #530215 - Flags: review?(philipp)
(In reply to comment #3)
> Created attachment 530215 [details] [diff] [review] [review]
> A patch
> 
> Wanted to get something submitted for this before you left :)

:philikon
Comment on attachment 530215 [details] [diff] [review]
A patch

>   resetPassphrase: function resetPassphrase() {
>+    //set feedback messages to blank to make room for new feedback messages

Nit: Please leave a space after the //, capitalize the first letter and add punctuation where appropriate.

>+
>+      //display a feedback message for info missing
>+      if(Weave.LOGIN_FAILED_NO_USERNAME == Weave.Status.login ||
>+         Weave.LOGIN_FAILED_NO_PASSWORD == Weave.Status.login) {

I don't think we should restrict the displaying of the error message just to those two. If somehow login failed due to a network error, it'd be nice to show that too rather than continue to fail silently.

>+        let feedback = document.getElementById("missingInfoFeedbackRow");
>+        this._setFeedbackMessage(feedback, false, Weave.LOGIN_FAILED_MISSING_INFO);

Why define and use Weave.LOGIN_FAILED_MISSING_INFO if you already know the reason (Weave.Status.login)? You could just do 

  this._setFeedbackMessage(feedback, false, Weave.Status.login);

That would automatically choose the right error message depending on what info is missing.

>           <row id="existingPasswordFeedbackRow" align="center" hidden="true">
>             <spacer/>
>             <hbox>
>               <image class="statusIcon"/>
>               <label class="status" value=" "/>
>             </hbox>
>           </row>
>+          <row id="missingInfoFeedbackRow" align="center" hidden="true">
>+            <spacer/>
>+            <hbox>
>+              <image class="statusIcon"/>
>+              <label class="status" value=" "/>
>+            </hbox>
>+          </row>

Why the second row? Can't you just use "existingPasswordFeedbackRow"?

>+error.login.reason.missing    = Missing account name or password
...
>+LOGIN_FAILED_MISSING_INFO:             "error.login.reason.missing",

Don't think these are necessary (see above).
Attachment #530215 - Flags: review?(philipp) → review-
(In reply to comment #5)
> (In reply to comment #3)
> > Created attachment 530215 [details] [diff] [review] [review] [review]
> > A patch
> > 
> > Wanted to get something submitted for this before you left :)
> 
> :philikon

:)
(In reply to comment #6)
> Comment on attachment 530215 [details] [diff] [review] [review]
> A patch
> 
> >   resetPassphrase: function resetPassphrase() {
> >+    //set feedback messages to blank to make room for new feedback messages
> 
> Nit: Please leave a space after the //, capitalize the first letter and add
> punctuation where appropriate.
> 

will do

> >+
> >+      //display a feedback message for info missing
> >+      if(Weave.LOGIN_FAILED_NO_USERNAME == Weave.Status.login ||
> >+         Weave.LOGIN_FAILED_NO_PASSWORD == Weave.Status.login) {
> 
> I don't think we should restrict the displaying of the error message just to
> those two. If somehow login failed due to a network error, it'd be nice to show
> that too rather than continue to fail silently.

In toggleLoginFeedback in syncSetup.js, the network, server, and invalid passphrase failures are handled.  However, this method only gets called upon an attempt to login.  An attempt to login only happens if both username & password != "". So the reason I'm doing a check for only those 2 is because they are the only case which isn't checked for.  I do think it is possible, however, to allow login to be attempted even when username or password == "" and then place this error check within toggleLoginFeedback.  I think I may do that instead if you agree it's better.


> 
> >+        let feedback = document.getElementById("missingInfoFeedbackRow");
> >+        this._setFeedbackMessage(feedback, false, Weave.LOGIN_FAILED_MISSING_INFO);
> 
> Why define and use Weave.LOGIN_FAILED_MISSING_INFO if you already know the
> reason (Weave.Status.login)? You could just do 
> 
>   this._setFeedbackMessage(feedback, false, Weave.Status.login);
> 
> That would automatically choose the right error message depending on what info
> is missing.

Weave.Status.login for the no password case has a string of 

error.login.reason.no_password = No saved password to use

which I didn't think made sense in this case, though I may be wrong. Should I still use it anyways?

also, error.login.reason.no_username doesn't have a string defined for its error message in errors.properties. So I could define that instead of error.login.reason.missing



> 
> >           <row id="existingPasswordFeedbackRow" align="center" hidden="true">
> >             <spacer/>
> >             <hbox>
> >               <image class="statusIcon"/>
> >               <label class="status" value=" "/>
> >             </hbox>
> >           </row>
> >+          <row id="missingInfoFeedbackRow" align="center" hidden="true">
> >+            <spacer/>
> >+            <hbox>
> >+              <image class="statusIcon"/>
> >+              <label class="status" value=" "/>
> >+            </hbox>
> >+          </row>
> 
> Why the second row? Can't you just use "existingPasswordFeedbackRow"?


each of network, server & invalid passphrase seem to have their own row defined as well within the existingAccount wizard page, so I figured that was the preferred way of doing it (for clarity maybe?).  I do agree though that it's possible and better to allow all errors to use the same row.


> 
> >+error.login.reason.missing    = Missing account name or password
> ...
> >+LOGIN_FAILED_MISSING_INFO:             "error.login.reason.missing",
> 
> Don't think these are necessary (see above).


true, I can do what's mentioned above
(In reply to comment #8)
> > >+
> > >+      //display a feedback message for info missing
> > >+      if(Weave.LOGIN_FAILED_NO_USERNAME == Weave.Status.login ||
> > >+         Weave.LOGIN_FAILED_NO_PASSWORD == Weave.Status.login) {
> > 
> > I don't think we should restrict the displaying of the error message just to
> > those two. If somehow login failed due to a network error, it'd be nice to show
> > that too rather than continue to fail silently.
> 
> In toggleLoginFeedback in syncSetup.js, the network, server, and invalid
> passphrase failures are handled.  However, this method only gets called upon an
> attempt to login.  An attempt to login only happens if both username & password
> != "". So the reason I'm doing a check for only those 2 is because they are the
> only case which isn't checked for.  I do think it is possible, however, to
> allow login to be attempted even when username or password == "" and then place
> this error check within toggleLoginFeedback.  I think I may do that instead if
> you agree it's better.

That's actually a really good suggestion. A bit more indirection, but fits better into the current setup. Just explain it in a comment in resetPassphrase.

> Weave.Status.login for the no password case has a string of 
> 
> error.login.reason.no_password = No saved password to use
> 
> which I didn't think made sense in this case, though I may be wrong. Should I
> still use it anyways?

I see. Indeed, that's not a great string for this purpose. I think the original purpose was that if somebody had wiped their passwords in their profile, we'd tell them that Sync no longer had an account password to use. I'm pretty sure we don't actually thing we get as far to display this string every anymore, though. Because if Weave.Status.login == Weave.LOGIN_FAILED_NO_PASSWORD, we set Weave.Status.sync to Weave.CLIENT_NOT_CONFIGURED and disable Sync altogether.

So how about we change the string to simply say "Missing password". Note that you'd have to change the string ID here (e.g. to error.login.reason.no_password2) and the value of Weave.LOGIN_FAILED_NO_PASSWORD accordingly.

> also, error.login.reason.no_username doesn't have a string defined for its
> error message in errors.properties. So I could define that instead of
> error.login.reason.missing

Yes. Could be just "Missing account name".

> > Why the second row? Can't you just use "existingPasswordFeedbackRow"?
> 
> each of network, server & invalid passphrase seem to have their own row defined
> as well within the existingAccount wizard page, so I figured that was the
> preferred way of doing it (for clarity maybe?).

They have their own row because they're in different places. Here we want to display the error message in the same place where the "wrong password or username" error message would appear, so reusing "existingPasswordFeedbackRow" is perfectly fine.
Attached patch Patch for bug-642969 (obsolete) — Splinter Review
this._notify("verify-login", "", function() {})(); in service.js doesn't actually pass the verifyLogin() function as a parameter since this would fall into the 404 case resulting in a LOGIN_FAILED_LOGIN_REJECTED overwriting the LOGIN_FAILED_NO_PASSWORD status.  When the status is LOGIN_FAILED_NO_USERNAME or LOGIN_FAILED_NO_PASSWORD, we only want to call toggleLoginFeedback() and not verifyLogin().  philikon: if you see a better way around this, please let me know.
Attachment #530215 - Attachment is obsolete: true
Attachment #530216 - Attachment is obsolete: true
Attachment #531153 - Flags: review?(philipp)
Comment on attachment 531153 [details] [diff] [review]
Patch for bug-642969

>--- a/services/sync/locales/en-US/errors.properties
>+++ b/services/sync/locales/en-US/errors.properties
>@@ -1,12 +1,13 @@
> error.login.reason.network    = Failed to connect to the server
> error.login.reason.synckey    = Wrong Sync Key
> error.login.reason.account    = Incorrect account name or password
>-error.login.reason.no_password= No saved password to use
>+error.login.reason.no_username= Missing username
>+error.login.reason.no_password2=Missing password

The string should mention "account name" rather than "username" because the form field is actually labeled "Account". (Also see error.login.reason.account)

Nit: please add spaces around = operator. Feel free to reindent the whole block to make the equal signs line up.

>-      if (this._checkSetup() == CLIENT_NOT_CONFIGURED)
>+      if (this._checkSetup() == CLIENT_NOT_CONFIGURED) {
>+        // Since CLIENT_NOT_CONFIGURED then password, username or passphrase missing.
>+        // Notify the verify-login observer which prepares the error message.
>+        this._notify("verify-login", "", function() {})();
>         throw "aborting login, client not configured";
>+      }

I believe we can avoid this if gSyncSetup would simply listen to weave:service:login:* rather than weave:service:verify-login:*.

Once upon a time the wizard would call verifyLogin() directly, rather than through login(). That's why it subscribed to weave:service:verify-login:*. But it looks like that's no longer the case, so we can just listen to weave:service:login:* and not require this hack.

r- for this, but the rest looks good. Almost there! :)
Attachment #531153 - Flags: review?(philipp)
Attachment #531153 - Flags: review-
> The string should mention "account name" rather than "username" because the
> form field is actually labeled "Account". (Also see
> error.login.reason.account)

What did you want me to see about error.login.reason.account?
Attached patch Patch for bug-642969 (obsolete) — Splinter Review
philikon: Is this what you meant? It looks a lot better to me.
Attachment #531153 - Attachment is obsolete: true
Attachment #531234 - Flags: review?(philipp)
Comment on attachment 531234 [details] [diff] [review]
Patch for bug-642969

Yup, that's exactly what I meant, except for this change:

>-  verifyLogin: function verifyLogin()
>-    this._notify("verify-login", "", function() {
>+  verifyLogin: function verifyLogin() {
...
>-    })(),
>+    },

please revert this. r=me with that.
Attachment #531234 - Flags: review?(philipp) → review+
(In reply to comment #12)
> > The string should mention "account name" rather than "username" because the
> > form field is actually labeled "Account". (Also see
> > error.login.reason.account)
> 
> What did you want me to see about error.login.reason.account?

Oh just that it mentions "account name" rather than "username". I see your latest patch already fixes this, so well done :)
philikon: here's the last patch.
Attachment #531234 - Attachment is obsolete: true
http://hg.mozilla.org/services/services-central/rev/0433e3d31759
Whiteboard: [good first bug] → [good first bug][fixed in services]
Verified fix on s-c: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110516 Firefox/6.0a1
Whiteboard: [good first bug][fixed in services] → [good first bug][verified in services]
http://hg.mozilla.org/mozilla-central/rev/0433e3d31759
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Verified fix on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0a1) Gecko/20110518 Firefox/6.0a1
Status: RESOLVED → VERIFIED
Blocks: 615950, 684536
No longer blocks: 615950
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.