Closed Bug 1004251 Opened 5 years ago Closed 5 years ago

Implement Resend Verification Email command in FxA's Settings UI

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S4 (20june)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: spenrose, Assigned: _6a68)

References

Details

(Keywords: late-l10n, Whiteboard: [fxa4fxos2.0])

Attachments

(4 files)

This will require a button in the Settings UI that tells the system app to send a mozFxAccountsContentEvent with method "resendVerificationEmail" down to Gecko.
Priority: -- → P3
Whiteboard: [fxa4fxos2.0]
Priority: P3 → P2
Summary: Implement Resend Verification Email command in FxA's Setting's UI → Implement Resend Verification Email command in FxA's Settings UI
Attached file 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
Attachment #8432905 - Flags: review?(ferjmoreno)
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 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.
Attachment #8432905 - Flags: review?(ferjmoreno) → review+
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
Flags: needinfo?(ferjmoreno)
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
Attachment #8432905 - Flags: review?(arthur.chen)
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
Attachment #8432905 - Flags: feedback?(francesco.lodolo)
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.
Attachment #8432905 - Flags: review?(arthur.chen)
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.
Attachment #8432905 - Flags: feedback?(francesco.lodolo) → feedback+
Comment on attachment 8432905 [details] [review]
Github PR 19903

I left a comment in the PR.
Attachment #8432905 - Flags: review+
Flags: needinfo?(ferjmoreno)
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
Attachment #8432905 - Flags: review?(ferjmoreno)
Attachment #8432905 - Flags: review?(arthur.chen)
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
Attachment #8432905 - Flags: review?(ferjmoreno) → review+
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
Flags: needinfo?(arthur.chen)
Flagging as late-l10n since Monday is approaching.
Keywords: late-l10n
OK, I'll do the review this morning.
Flags: needinfo?(arthur.chen)
Comment on attachment 8432905 [details] [review]
Github PR 19903

I left a few comments in github, please check them, thanks!
Attachment #8432905 - Flags: review?(arthur.chen)
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
Attachment #8432905 - Flags: review?(arthur.chen)
Comment on attachment 8432905 [details] [review]
Github PR 19903

Thank you for the effort, r=me.
Attachment #8432905 - Flags: review?(arthur.chen) → review+
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?
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S4 (20june)
(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 :-\
blocking-b2g: --- → 2.0?
BLocking as this looks like a new feature miss..
blocking-b2g: 2.0? → 2.0+
Duplicate of this bug: 1023599
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?
Flags: needinfo?(bbajaj)
Reverted in master: https://github.com/mozilla-b2g/gaia/commit/a10c9ab609a045fa71d327d5b98630d2605838c8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file 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
Attachment #8438691 - Flags: review?(arthur.chen)
(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 :)
Flags: needinfo?(bbajaj)
(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 on attachment 8438691 [details] [review]
Github PR 20383

r=me. Please land the patch when travis or gaia-try is green. Thanks!
Attachment #8438691 - Flags: review?(arthur.chen) → review+
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.
Alright! Tree reopened, merged to master.

Master: https://github.com/mozilla-b2g/gaia/commit/1d4d176ec6563ad5ae47f04a82a06db73a151e27
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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?
Flags: needinfo?(arogers)
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 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.
Attachment #8438691 - Flags: approval-gaia-v2.0?
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
Attachment #8438691 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Flags: needinfo?(nojun.park)
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.
Flags: needinfo?(nojun.park)
Duplicate of this bug: 1027307
Clearing ni
Flags: needinfo?(arogers)
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
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
Attached image Verify_Flame_Resend.png
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) {
Flags: needinfo?(6a68)
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
Flags: needinfo?(6a68)
You need to log in before you can comment on or make changes to this bug.