1002888 - Cannot correct email address of Firefox Account after creating an account, and unable to verify it

Status: RESOLVED FIXED

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

Description

[dynamis (Tomoya ASAI)]

bug 576462 (need a way to change the email address for an account) is WONTFIX and 
no user interface to change mail address of Firefox Account (Sync Service).

But when user input wrong mail address in the Firefox Account sign-up page, user cannot change the address and cannot use Sync service.
# sync error notification will show up forever...

We don't have mail confirmation form and not a few mobile user will miss-type their address. I think we should support to correct email address at least before verifying it.
# ex: add "sing-up with another email address" under the resend verify mail button

Note: Actually user can delete Firefox Account from Account management window in Android's Settings apps but no link from Firefox UI and user will not notice about it.

Comment 1

[Richard Newman [:rnewman]]

(In reply to dynamis (Tomoya ASAI) from comment #0)

> We don't have mail confirmation form and not a few mobile user will
> miss-type their address. I think we should support to correct email address
> at least before verifying it.
> # ex: add "sign-up with another email address" under the resend verify mail
> button

That doesn't seem like an unreasonable idea -- "I'm not going to be able to verify this, get me out of here".

Comment 2

[Nick Alexander :nalexander [he/him]]

I think this is a good idea.  Our UX team have previously asked for a single large button on each page, so this would look like adding a small link underneath the button at or around

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/fxaccount_confirm_account.xml#43

Then, generalize the onClick handler configured at

 http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountConfirmAccountActivity.java#64

to handle the new link.  Finally, the interesting part is deleting the existing account (using http://developer.android.com/reference/android/accounts/AccountManager.html#removeAccount%28android.accounts.Account,%20android.accounts.AccountManagerCallback%3Cjava.lang.Boolean%3E,%20android.os.Handler%29), finishing ConfirmActivity, and sending the user back to the getting started activity.

Comment 4

[swaroop.rao]

I'd like to take this up if nobody's working on it already. I've got a build up and running and I've managed to reproduce this defect on my Nexus 5.

Comment 5

[Ryan Feeley [:rfeeley]]

This is the state that should appear on desktop (though testing now, it appears it's broken there too!)
https://www.dropbox.com/s/gbhdffre8ztefq9/Desktop%20-%20Preferences%20-%20Sync%20-%20Unverified.pdf

Comment 6

[swaroop.rao]

Thanks, Ryan. I see a "Forget this Email" link below the "Verify Email" button in the screenshot that you posted. Are you saying that the link does not function as it is supposed to? Can I try to implement the solution as laid out by Nick above?

Comment 7

[Ryan Feeley [:rfeeley]]

Hi Swaroop, I'm saying that the "Forget this Email" should be an option on desktop, but is not working well (it works slightly better on release than nightly).

> But when user input wrong mail address in the Firefox Account 
> sign-up page, user cannot change the address and cannot use 
> Sync service.

If the "Forget this Email" option is present, the user can remove it.

On Android, would the user just remove the account?

Hope I'm not muddying the waters here.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Ryan Feeley from comment #7)
> Hi Swaroop, I'm saying that the "Forget this Email" should be an option on
> desktop, but is not working well (it works slightly better on release than
> nightly).
> 
> > But when user input wrong mail address in the Firefox Account 
> > sign-up page, user cannot change the address and cannot use 
> > Sync service.
> 
> If the "Forget this Email" option is present, the user can remove it.
> 
> On Android, would the user just remove the account?

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1002888#c2, yes, on Android we just delete the Account and go try to set up a new one.  There are now two places to put "Forget this Email":

1) the Confirm Account activity (see Comment 2 for the link);
2) the Confirm Account sub-panel of the Remote Tabs home panel [1].

Both places will do the same thing: delete the Android Account.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/remote_tabs_needs_verification.xml#38

Comment 9

[swaroop.rao]

I found an option to remove the account from the Firefox 'Settings' screen. Here's a screenshot from my Nexus 5 to illustrate this. https://drive.google.com/file/d/0B7yRBgcEY7eoQzQtcDdvSDIzeDA/view?usp=sharing

I built this Fennec version earlier today. I tested it and it seems to do the job as expected. Isn't this sufficient?

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #9)
> I found an option to remove the account from the Firefox 'Settings' screen.
> Here's a screenshot from my Nexus 5 to illustrate this.
> https://drive.google.com/file/d/0B7yRBgcEY7eoQzQtcDdvSDIzeDA/view?usp=sharing
> 
> I built this Fennec version earlier today. I tested it and it seems to do
> the job as expected. Isn't this sufficient?

It might be, but you have to do a bit (and know some things) to get there.  I like adding the nice option.

Comment 11

[swaroop.rao]

OK, I've started making the changes. I added a link below the 'Resend confirmation email' link, modified the listener with an if condition depending on which link was clicked. Now, I have to write the code to delete the account and send the user back to the 'Getting Started' activity. The account object that I currently have in FxAccountConfirmAccountActivity is of type AndroidFxAccount not the Android Account object, so I don't understand how I can implement what Nick has suggested at the end of comment #2. AndroidFxAccount is not a derived class of the Android Account class, so I cannot use its methods. The AndroidFxAccount class has a makeDeletedAccountIntent() method. Should I use that instead or do you have any other suggestions/advice?

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #11)
> OK, I've started making the changes. I added a link below the 'Resend
> confirmation email' link, modified the listener with an if condition
> depending on which link was clicked. Now, I have to write the code to delete
> the account and send the user back to the 'Getting Started' activity. The
> account object that I currently have in FxAccountConfirmAccountActivity is
> of type AndroidFxAccount not the Android Account object, so I don't
> understand how I can implement what Nick has suggested at the end of comment
> #2. AndroidFxAccount is not a derived class of the Android Account class, so
> I cannot use its methods. The AndroidFxAccount class has a
> makeDeletedAccountIntent() method. Should I use that instead or do you have
> any other suggestions/advice?

AndroidFxAccount is a wrapper that *composes* (rather than inherits) Android's Account object.  You can get the underlying account using [1].

There's an example of code that deletes the Account already at [2].  It shows a confirmation dialog and then finish()es the current activity.  Try to duplicate or extract that method into a helper that you can use to show a confirmation dialog when you click the "forget this email" and that deletes the account on success.  Rather than calling finish(), launch the FxAccountGetStartedActivity, just like [3].

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/authenticator/AndroidFxAccount.java#107

[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/fxa/activities/FxAccountStatusActivity.java#116

[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/StartPane.java#53

Comment 13

[swaroop.rao]

Thanks, Nick. It's much clearer now. I'll get started on the changes you recommended. Is there a specific way I should be implementing helpers? For example, should I create a separate class with a static method? Or, is there a helper class already to which I can just add this method?

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #13)
> Thanks, Nick. It's much clearer now. I'll get started on the changes you
> recommended. Is there a specific way I should be implementing helpers? For
> example, should I create a separate class with a static method? Or, is there
> a helper class already to which I can just add this method?

There are lots of different places.  In general, just throw a static method wherever it's convenient -- close to the consuming code and don't worry too much about it.  We can choose a final location as different things use it.

Comment 15

[swaroop.rao]

I'm working on both this defect as well as defect #1040572. I've got fixes ready for both and tested them. Can I submit both simultaneously? I don't know how to post work in progress, but I can post the output of 'hg diff' if that will help. The code is not yet optimized in the sense that I haven't pulled out some common code into a helper method. I've just copy-pasted the code in a different classs to see if it works. Before submitting the patch, I will make the modifications and test it.

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to swaroop.rao from comment #15)
> I'm working on both this defect as well as defect #1040572. I've got fixes
> ready for both and tested them. Can I submit both simultaneously? I don't
> know how to post work in progress, but I can post the output of 'hg diff' if
> that will help. The code is not yet optimized in the sense that I haven't
> pulled out some common code into a helper method. I've just copy-pasted the
> code in a different classs to see if it works. Before submitting the patch,
> I will make the modifications and test it.

Please break the changes into two different patches, and attach them to the two separate tickets.  Make sure to flag me (feedback or review) so that it appears in my queue.

Comment 17

[swaroop.rao]

I'm submitting a patch containing the changes for this bug. I have converted the maybeDeleteAndroidAccount() method in FxAccountStatusActivity to a generic utility (read static) method. I had to add a new parameter to it to pass the object in context because there were calls to non-static functions that were throwing compilation errors. I'm not sure if this is really an elegant solution but it seems to work correctly. I checked the entire code base to ensure that the method is not called by anybody else, so there should not be any regressions. Please review and let me know if I need to change the approach.

Comment 18

[swaroop.rao]

Attachment: Patch contains changes to a couple of class files and changes to layout/language/string files

Changes were made to the following files.

mobile/android/base/fxa/activities/FxAccountConfirmAccountActivity.java
mobile/android/base/fxa/activities/FxAccountStatusActivity.java
mobile/android/base/locales/en-US/sync_strings.dtd
mobile/android/base/resources/layout/fxaccount_confirm_account.xml
mobile/android/services/strings.xml.in

Please review and let me know if these changes are correct.

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8504754 [details] [diff] [review]
Patch contains changes to a couple of class files and changes to layout/language/string files

Review of attachment 8504754 [details] [diff] [review]:
-----------------------------------------------------------------

Hey, sorry for the delay, but I expect to get to this Tuesday or Wednesday next week.

Comment 20

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8504754 [details] [diff] [review]
Patch contains changes to a couple of class files and changes to layout/language/string files

Review of attachment 8504754 [details] [diff] [review]:
-----------------------------------------------------------------

The approach is looking good.  I have some requests, and some trivial nits, but this is looking good.  Great work!

::: mobile/android/base/fxa/activities/FxAccountConfirmAccountActivity.java
@@ +19,4 @@
>  import org.mozilla.gecko.sync.setup.activities.ActivityUtils;
>  
>  import android.accounts.Account;
> +import android.accounts.AccountManager;

nit: clean up these imports.  Lots seem unused.

@@ +174,5 @@
>    }
>  
>    @Override
>    public void onClick(View v) {
> +    if (v.equals (resendLink)) {

nit: no space before parens.

@@ +177,5 @@
>    public void onClick(View v) {
> +    if (v.equals (resendLink)) {
> +        FxAccountCodeResender.resendCode(this, fxAccount);
> +    } else if (v.equals (changeEmail)) {
> +      Account account = fxAccount.getAndroidAccount();

nit: final when possible.

@@ +178,5 @@
> +    if (v.equals (resendLink)) {
> +        FxAccountCodeResender.resendCode(this, fxAccount);
> +    } else if (v.equals (changeEmail)) {
> +      Account account = fxAccount.getAndroidAccount();
> +      FxAccountStatusActivity.maybeDeleteAndroidAccount(account, this); 

nit: trailing whitespace.

@@ +179,5 @@
> +        FxAccountCodeResender.resendCode(this, fxAccount);
> +    } else if (v.equals (changeEmail)) {
> +      Account account = fxAccount.getAndroidAccount();
> +      FxAccountStatusActivity.maybeDeleteAndroidAccount(account, this); 
> +    }

In this case, we really want to go back to the "getting started" activity.  Perhaps add a callback/delegate to maybeDeleteAndroidAccount?  Or, since you provide an Activity to finish(), include an Intent (possibly null) to launch after the finish()?  I like the latter, I think.

::: mobile/android/base/fxa/activities/FxAccountStatusActivity.java
@@ +113,5 @@
>    /**
>     * Helper function to maybe remove the given Android account.
>     */
>    @SuppressLint("InlinedApi")
> +  public static void maybeDeleteAndroidAccount(final Account account, final Activity thisActivity) {

Make the Activity parameter come first (Android convention is to have any Context come first), and just call it 'activity'.

::: mobile/android/base/locales/en-US/sync_strings.dtd
@@ +166,5 @@
>  <!ENTITY fxaccount_confirm_account_header 'Confirm your account'>
>  <!-- Localization note: &formatS; is the Firefox Account's email address. -->
>  <!ENTITY fxaccount_confirm_account_verification_link 'A verification link has been sent to &formatS;'>
>  <!ENTITY fxaccount_confirm_account_resend_email 'Resend email'>
> +<!ENTITY fxaccount_confirm_account_change_email 'Change email address'>

Hmm, this is misleading, since we're not actually changing an email address.  Maybe "Stop trying to verify this email address?"  "Forget account with this email address"?

I think I like "Forget this email address?", which prompts the user to remove the account and goes to the getting started page, most.

Comment 21

[swaroop.rao]

Sure, I will work on these comments tonight.

Comment 22

[swaroop.rao]

Attachment: Patch v2 changes suggested in review feedback

Removed stray whitespace
Modified ordering of method parameters
Added an Intent parameter to method that deletes the email address
Taking the user back to FxAccountGetStartedActivity after email address is deleted
Fixed a bug that prevented the 'Next' button from getting enabled after a configuration change (usually portrait->landscape and vice versa)

Comment 23

[swaroop.rao]

Nick, did you get a chance to review my patch?

Comment 24

[swaroop.rao]

Nick, will you be able to review the patch some time this week? Or, can I get somebody else to review it so I can move on to other defects?

Comment 25

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8510086 [details] [diff] [review]
Patch v2 changes suggested in review feedback

Review of attachment 8510086 [details] [diff] [review]:
-----------------------------------------------------------------

Hey! Sorry that this took so long to review (twice)!  This looks great.  You did an excellent job implementing my suggestions and the patch read exactly like it would if I had written it, which made my review really easy.  My device testing worked perfectly.

For next time, we always format our bug commit messages like:

Bug XXX - Short description. r=nalexander

Longer description of the problem, motivation, and solution.

This needs to land upstream in android-sync, which I will do now.  Again, sorry for the long delay, thanks for your patience, and congrats on a well-crafted patch.

Comment 26

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/d4882c2599f7

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4882c2599f7

Comment 28

[swaroop.rao]

Thanks, Nick! I have a follow-up question about your comment about the format of the bug commit message. Since I'm not actually committing any code changes to Mercurial, what do you mean by a 'commit message'? Is it the subject line of the message I write to accompany my patch? Or, is it something else? I've got a patch ready for bug 1040572 and I want to ensure that I do it correctly.

Comment 29

[swaroop.rao]

Ah, I think I know what you mean. It's probably the -m argument to 'hg qrefresh' or 'hg qnew'. I'll give that a shot and upload the patch in a few minutes.