1004251 - Implement Resend Verification Email command in FxA's Settings UI

Status: VERIFIED FIXED

(Firefox OS Graveyard :: FxA, defect, P2)

Description

[Sam Penrose]

This will require a button in the Settings UI that tells the system app to send a mozFxAccountsContentEvent with method "resendVerificationEmail" down to Gecko.

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 19903

Hi Fernando,

This is the last fxa feature bug that's open, it would be great to get it reviewed before the 9th. I have not yet added the resend verification button to the settings app, but wanted to get your review of the parts that affect system. 

Thanks!

Jared

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

To add a little detail, the resend verification UX is all in the Settings app. The system dialog doesn't have to be displayed at all--that's why I have treated it like the getAccounts() call in the fxa_manager and fxa_client code.

See 10.3 in the latest spec[1] for UX: after clicking resend, we show the user an alert telling them we've sent the email. I suppose we'll just console.error the errors, since there is no UX for that.

[1] https://bug960130.bugzilla.mozilla.org/attachment.cgi?id=8427186

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Thanks Jared. LGTM!

You just need to update fxa_iac_client_test.js and make Travis happy.

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Hey Fernando,

Unfortunately, there's a bug in Gecko that requires one small workaround inside the fxa_iac_client (the resend response is formatted differently than other responses). Would you mind taking a look? It's a separate commit, https://github.com/6a68/gaia/commit/1579eeb9.

Thanks,

Jared

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Hi Arthur,

This is the last fxa feature bug for 2.0, I would really appreciate review this week if possible.

We are adding a 'resend email' feature, Fernando has already reviewed the parts inside system, and the pull request has a separate commit containing changes needed inside Settings.

Thanks very much,

Jared

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Hi Francesco,

We've added 3 new fxa strings in this patch. Do you think you could find time to take a look this week? This should be the last string additions for 2.0 for fxa. I think I finally understand how to do l10n (thanks for all your help on 987418), hopefully this will be a quick review.

Thanks!

Jared

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Overall the settings part looks good to me, but there are a few things need to be addressed before merging. And please also add unit tests for the added function.

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8432905 [details] [review]
Github PR 19903

I left a few comments on Github: strings per se look good (that's the f+), minus a minor error in the comment, but the code doesn't seem to match the strings.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8432905 [details] [review]
Github PR 19903

I left a comment in the PR.

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Hi guys,

Thanks very much for the feedback. I've updated the PR and responded in Github to your comments.

The current wait for Travis is 4-5 hours, so I'm going to set r? before it turns green. I'll make sure it's green when I merge.

Thanks again!

Jared

Comment 11

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

FWIW I opened a pull request against my fork, and the tests are green (except marionette, which seems to be timing out):

https://travis-ci.org/6a68/gaia/builds/26889505

Comment 12

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Arthur,

I'm sure you are very busy with final reviews for the 2.0 feature deadline, but I was hoping to get one more review of this patch.

Do you think you'll have time prior to Monday to do another review? If not, could you suggest another peer who might have time?

Thanks very much,

Jared

Comment 13

[Francesco Lodolo [:flod]]

Flagging as late-l10n since Monday is approaching.

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

OK, I'll do the review this morning.

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8432905 [details] [review]
Github PR 19903

I left a few comments in github, please check them, thanks!

Comment 16

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Hi Arthur,

Thanks so much for the review! I've responded in github, please take a look when you can.

Cheers,

Jared

Comment 17

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8432905 [details] [review]
Github PR 19903

Thank you for the effort, r=me.

Comment 18

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Master: https://github.com/mozilla-b2g/gaia/commit/0750f66a0004870773c9a743fa6bdbe124379336

Verifying this works locally, then I'll set the flag to ask for uplift.

Hey Ryan - I'm not sure what flags to set for this commit. We want it to ship with 2.0, and I understand that I'll need to set approval-gaia-2.0. What other flags should I set for now?

Comment 19

[Francesco Lodolo [:flod]]

(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #18)
> Hey Ryan - I'm not sure what flags to set for this commit. We want it to
> ship with 2.0, and I understand that I'll need to set approval-gaia-2.0.
> What other flags should I set for now?

Setting blocking-b2g: 2.0? flag on the bug, it's already marked as late-l10n for the strings :-\

Comment 20

[bhavana bajaj [:bajaj]]

BLocking as this looks like a new feature miss..

Comment 22

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

To be clear, I need to revert the fix for this bug, then fix the bug discussed in bug 1023599, get r+ from arthur overnight, merge to master and verify working, and *then* we will finally be ready to uplift.

Bhavana, any concerns about having strings uplifted tomorrow PDT?

Comment 23

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Reverted in master: https://github.com/mozilla-b2g/gaia/commit/a10c9ab609a045fa71d327d5b98630d2605838c8

Comment 24

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Github PR 20383

Hi Arthur,

Here's a new branch with the resubmitted patch, calling .bind instead of .call in the correct spot.

I haven't added more tests, because this bug needs uplift and has late strings, so there's a rush at this point; also, as you mentioned in bug 1023599, sometimes you just miss a typo, and there's no easy way to test for it.

If we really, really need additional tests here, I'd prefer to file as a separate followup bug and avoid delaying uplift another day.

Thanks,

Jared

Comment 25

[bhavana bajaj [:bajaj]]

(In reply to Jared Hirsch [:_6a68] [@6a68] from comment #22)
> To be clear, I need to revert the fix for this bug, then fix the bug
> discussed in bug 1023599, get r+ from arthur overnight, merge to master and
> verify working, and *then* we will finally be ready to uplift.
> 
> Bhavana, any concerns about having strings uplifted tomorrow PDT?

Thanks for checking, not something I would keep pushing for, but its ok :)

Comment 26

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #25)
> (In reply to Jared Hirsch [:_6a68] [@6a68] from comment #22)
> > To be clear, I need to revert the fix for this bug, then fix the bug
> > discussed in bug 1023599, get r+ from arthur overnight, merge to master and
> > verify working, and *then* we will finally be ready to uplift.
> > 
> > Bhavana, any concerns about having strings uplifted tomorrow PDT?
> 
> Thanks for checking, not something I would keep pushing for, but its ok :)

If the landing is happening elsewhere please tag the bug by late-l10n keyword...

Comment 27

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8438691 [details] [review]
Github PR 20383

r=me. Please land the patch when travis or gaia-try is green. Thanks!

Comment 28

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

OK, to be clear: now that I've got r+, I'm waiting for the tree to reopen, then I'll merge, verify fixed in master, and ask for uplift. Barring more tree closures, this should happen early today, or at least by end of day.

All the tests were green on the first try except the gaia-ui-tests, which are practically perma-red at this point :-\. I opened a PR against my repo to re-run the tests while waiting for the tree to reopen.

Comment 29

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Alright! Tree reopened, merged to master.

Master: https://github.com/mozilla-b2g/gaia/commit/1d4d176ec6563ad5ae47f04a82a06db73a151e27

Comment 30

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

OK, confirmed fixed. Ready for uplift.

Aha, I was having trouble reproducing because the server, by default, does not resend verification emails within 15 minutes of creating an account.

arog, does this seem like something we should change on the server?

Comment 31

[Sam Penrose]

I believe we should not have the client making promises it can't deliver on, which implies that we should either change it on the server or not show the resend capability until it can be fulfilled.

Comment 32

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 8438691 [details] [review]
Github PR 20383

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):

Feature bug (user story is bug 974999)

[User impact] if declined:

Users won't be able to resend their firefox accounts verification email after creating their account. If they've lost or deleted that email, they will be unable to use that account.

[Testing completed]:

Verified fixed/working on the flame. (Note for QA: you cannot trigger a resend within 15 minutes of creating an account. The phone cannot detect this. Seems like something we might want to change on the server.)

[Risk to taking this patch] (and alternatives if risky):

Low-to-Medium risk:
- No integration concerns with other gaia apps.
- The corresponding gecko functionality landed long ago, so we aren't worrying about new code landing there.
- We have added a new IAC signal to the FxA system app. All FxA features seem to be working fine on my device, but if there were a bug that affected postmessage behavior, it could potentially impact any/all of the FxA functionality (since FxA depends on message passing to function). However, unit tests and manual tests would have found such a high-impact bug.
- Changes inside the settings app are confined to one state (unverified) in the FxA panel. Low potential impact to the settings app.

The alternative would be to not allow users to resend verification emails for the 2.0 release. This could have significant impact on the user experience.

[String changes made]:

Two new strings added. Already f+ by :flod.

Comment 33

[bhavana bajaj [:bajaj]]

Comment on attachment 8438691 [details] [review]
Github PR 20383

Thanks for the detailed and clear comments on the approval request comment ! This is a blocker bug so ideally would not have needed an approval for uplift but given the risk here requesting approvals on such bugs is helpful to get the needed info out.

I am flagging :nojun from QA to verify this bug and do some exploratory testing here around this code path in FxA

Comment 34

[No-Jun Park [:njpark]]

I just verified this feature.

Once the account is created, and after 15 minutes has elapsed, pressing resend button sends the verification link email.  The link is disabled for a minute, and when it is enabled, pressing it again send the e-mail again.

Clicking the link in the email correctly validates the user.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/cd34292c6d814d07785c7001df875be0ce304311

Comment 37

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Clearing ni

Comment 38

[William Hsu [:whsu]]

Thanks for your help!
Verified it.

Attach the screenshot. (FxA_Resend_Button.png)

* Build information:
 - Gaia      7edd3b0b9f65c3dde235c732d270e43e055a1254
 - Gecko     https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/13e04ab68621
 - BuildID   20140914162307
 - Version   32.0

Comment 39

[William Hsu [:whsu]]

Attachment: FxA_Resend_Button.png

Comment 40

[Coler]

This issue has been verified successfully on Flame2.1.
Reproducing rate: 0/5
See attachment: Verify_Flame_Resend.png
STRs:
1. Launch Settings->"Firefox Accounts".
2. Tap "Create account or sign in".
3. Input a new account.
**It will switch to "Age Verification" view.
4. Select an option and tap "Next".
5. Input password and tap "Next"->"Done".

Excepted Result:
5. You'll find the "Resend" button.

Flame2.1 build version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0

Comment 41

[Coler]

Attachment: Verify_Flame_Resend.png

Comment 42

[Chaochao Huang]

apps/system/js/fxa_manager.js

case 'resendVerificationEmail':
        (function(methodName) {
          LazyLoader.load('js/fxa_client.js', function() {
            FxAccountsClient[methodName](function(data) {
              self.sendPortMessage({ methodName: methodName, data: data });
            }, function(error) {
              self.sendPortMessage({ methodName: methodName, error: error });
            });
          });
        })(methodName);
        break;

apps/system/js/fxa_client.js

var resendVerificationEmail = function resendVerificationEmail(email,
                                                       successCb, errorCb) {
    sendMessage({
      method: 'resendVerificationEmail',
      email: email
    }, successCb, errorCb);
  };

We will find  that email = function(data) {
              self.sendPortMessage({ methodName: methodName, data: data });
            }
successCb = function(error) {
              self.sendPortMessage({ methodName: methodName, error: error });
            }

errorCb = undefined.....

Obviously， it isnot right.


Could we change it as below?

--- a/apps/system/js/fxa_manager.js
+++ b/apps/system/js/fxa_manager.js
@@ -79,7 +79,6 @@ var FxAccountsManager = {
     switch (methodName) {
       case 'getAccounts':
       case 'logout':
-      case 'resendVerificationEmail':
         (function(methodName) {
           LazyLoader.load('js/fxa_client.js', function() {
             FxAccountsClient[methodName](function(data) {
@@ -90,6 +89,23 @@ var FxAccountsManager = {
           });
         })(methodName);
         break;
+      case 'resendVerificationEmail':
+        (function(methodName) {
+          var email = event.detail.email;
+          if (!email) {
+            self.sendPortMessage({ methodName: methodName,
+                                   error: 'NO_VALID_EMAIL' });
+            return;
+          }
+          LazyLoader.load('js/fxa_client.js', function() {
+            FxAccountsClient[methodName](email, function(data) {
+              self.sendPortMessage({ methodName: methodName, data: data });
+            }, function(error) {
+              self.sendPortMessage({ methodName: methodName, error: error });
+            });
+          });
+        })(methodName);
+        break;
       case 'openFlow':
         (function(methodName) {
           FxAccountsUI.login(function(data) {

Comment 43

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hi Chaochao,

This bug is closed. If you'd like to change the fxa code, open a new bug, attach the patch, and flag ferjm for the review.

Cheers,

Jared