feat: Email opt-in.

[shane-tomlinson]

ref #2443

[rfk]

Looks like this takes an existing accessToken. What scopes does this token have? ISTM we may want to use a special-purpose scope for this, e.g. basket instead of the usual profile scope.

[shane-tomlinson]

It will, I'll scope the token when I create it over here.

[shane-tomlinson]

@zaach - Except for fxa-client, I don't think any of these clients save any state. Instead of creating all these clients on startup and passing them around, we could just pass in a list of service urls and have the callers of the clients create them when needed? Dunno.

[zaach]

👍 👍 We could allow both ways (since passing clients is still useful for testing) and default to using the URL list.

[shane-tomlinson]

I dig it.

[shane-tomlinson]

@rfk - is this correct?

I've been waffling whether email marketing related functionality should live here and an email-marketing-client is passed in, or whether the email marketing client should be passed to the views and its functions accept an account. This seems like the the more natural of the two because it means account callers do not have to worry about the email marketing client, but I could be convinced otherwise.

[rfk]

We might consider "basket:write" to allow modification of email marketing preferences, by analogy with "profile:write". We don't have much precedent for distinguishing read-only from read/write in scopes, but it seems worthwhile to follow the only precedent we have.

[rfk]

I've been waffling whether email marketing related functionality should live here and an email-marketing-client is
passed in, or whether the email marketing client should be passed to the views and its functions accept an account.

Tough call.

In isolation, I'd slightly prefer to have an "email marketing client" that takes the account to operate on as an argument, either in the constructor or in each method call. But I have a strong default preference for keeping a low method count on each class. People have criticized my style in the past for preferring "manager objects" a little to much...

But given that the account already handles profile information by exposing profile-manipulation methods that call through to an underlying profile client, I think you made the right call here by keeping the approach consistent. We can consider factoring both of these out if the account object becomes too unweildly under the weight of all its special-purpose methods.

(It's not just about cognitive load either - one concrete downside of making these all methods on the Account, is that we must load the code for all of them even if the user never sees anything to do with their profile or email marketing prefs. Something to think about in the future if we ever want to chunk up our downloads a little more).

[rfk]

is this correct?

I realized I didn't answer this explicitly: yes, this looks correct, modulo potentially changing it to "basket:write".

An alternative approach would be to request a single accessToken with scope "profile:write basket:write" that we can persist to the account and use for both purposes. I can't think of compelling reasons to favour one over the other though.

[shane-tomlinson]

(It's not just about cognitive load either - one concrete downside of making these all methods on the Account, is that we must load the code for all of them even if the user never sees anything to do with their profile or email marketing prefs. Something to think about in the future if we ever want to chunk up our downloads a little more).

I had a similar concern when putting together this patch. app-start.js creates several clients that are passed down through user.js to account.js, or passed into views. Except for fxa-js, none of these clients keep any state AFAICT, though they rely on information stored in the URLs returned from a call to the /config endpoint. It seems to me a more natural approach would be to pass around a "ServiceURLs" or "Configuration" object into User/Account, which could then create the clients as needed, reducing the number of objects instantiated on starutp.

[pdehaan]

This should probably be xhr (lower) for consistency with other files above.

[shane-tomlinson]

👍

[rfk]

The production value for newsletter id will be "firefox-accounts-journey".

[rfk]

alphabetization, nice :-)

[rfk]

If this token is not persisted anywhere, please attempt to destroy it after you're done with it. Like session tokens, these currently live forever unless explicitly destroyed.

[shane-tomlinson]

Ah, I did not know.

[rfk]

Is it worth checking (and can we easily check?) the expected request body as part of this test?

[rfk]

The body here will need to be { "newsletters": NEWSLETTER_ID }

[rfk]

I did a bit of testing on these endpoints in production. They seem to expect a trailing slash, like "/subscribe/" rather than "/subscribe". The body here will need to be { "newsletters": NEWSLETTER_ID, "optin": "Y" }, with the assumption that the email will be pulled out of the authz header automatically.

[rfk]

@shane-tomlinson this is looking great!

[shane-tomlinson]

For whatever reason, one thing that only occurred to me today is that I should probably call the client once to get all of the initial marketing email data, store that off into a model (either the account, or a separate email-preferences model that is related to the account), and use that one model to populate multiple fields on the /settings/communications_preferences page. Otherwise, two calls will be made to the backend to display that page, once for the user's opt-in preference, once for the user's Basket management URL.

[shane-tomlinson]

Extract the magic string.

[shane-tomlinson]

Add the real link.

[shane-tomlinson]

move this up below ajax

[zaach]

Shouldn't this also pass in an email address? Same for the other API calls.

[zaach]

Ah, nevermind. I was looking at the current API docs. I assume basket's new API won't use that. (related: I noticed the oauth docs were out of date wrgt the email in the /verify response .)

[zaach]

Do we need to pass in lang? Also optin=Y.

[rfk]

For FxA users, their "lang" setting in basket is supposed to come from their FxA account data, and is populated by a background process that transfers FxA account data over to basket. There's some corner-cases here if we try to process the opt-in before the background transfer has completed.

I think the safest course of action will be to leave out "lang" on this request, and only enable the opt-in for en-US on initial release. That way if there's a race, the worst that happens is they get default english newsletters until we set their actual language preference, which is what they wanted anyway.

We'll need to sanity-check this with @pmclanahan.

[zaach]

This port is taken by one of the profile server helpers! How about 3031?

[shane-tomlinson]

This needs to come from configuration.

[pdehaan]

Not sure if this little _ variable guy will cause confusion if we end up requiring underscore above in the future. 💥

[pdehaan]

Are we still planning on merging this for Train-38, or should we bump milestone to Train-39?

[jrgm]

So, basket-proxy-server.js uses request, so that module needs to be promoted from devDependencies to dependencies in package.json

[rfk]

This logic seems to be getting over-ridden by a re-render of the view. When I click "subscribe" I see the following sequence of button text:

• transitions to spinner while doing the request
• briefly changes to "Unsubscribe"
• changes back to "Subscribe" almost immediately

[shane-tomlinson]

It's the progress indicator overwriting the text. grrr.

[zaach]

Yeah; I turned on the debugger and I'm not quite sure how the second render gets trigger after the form submit. I put a breakpoint in context but it doesn't get triggered in the render after the submit.

[shane-tomlinson]

a dirty hack is to wrap this line in a setTimeout w/ 100ms delay. dirty. (I'm not online)

[shane-tomlinson]

@zaach - perhaps that's a better solution, re-render with a success ephemeral message. It looks a bit goofy because the success message closes and re-opens every time the user clicks the button, but the button state is correct.

diff --git a/app/scripts/views/settings/communication_preferences.js b/app/scripts/views/settings/communication_preferences.js
index 50712cd..8a13780 100644
--- a/app/scripts/views/settings/communication_preferences.js
+++ b/app/scripts/views/settings/communication_preferences.js
@@ -90,13 +90,8 @@ function (Cocktail, Xss, Constants, MarketingEmailErrors, BaseView, FormView,
           var successMessage = isOptedIn ?
                                   t('Subscribed successfully') :
                                   t('Unsubscribed successfully');
-          self.displaySuccess(successMessage);
-
-          var buttonText = isOptedIn ?
-                                  t('Unsubscribe') :
-                                  t('Subscribe');
-
-          self.$('button[type=submit]').text(buttonText);
+          self.ephemeralMessages.set('success', successMessage);
+          self.render();
         }, function (err) {
           self.displayError(err);
         });

[zaach]

Ahh. Maybe slightly cleaner is to have the progress indicator skip restoring the contents of the button if it has changed since the progress indicator was initiated.

[zaach]

@shane-tomlinson I like that 👍

[shane-tomlinson]

+          self.render();

And if you add a return on the front of that, the unit tests still pass.

+           return self.render();

[zaach]

The logic is much nicer, but now there's another flicker issue:

Is there a way to prevent the progress indicator from transitioning?

[zaach]

@shane-tomlinson thanks! returning self.render() takes away the button flicker

[zaach]

I'm getting all 200 responses from the proxy, but the button doesn't toggle from Subscribe to Unsubscribe like it should:

[zaach]

If the user is verifying in a different browser than they signed up with, this will be undefined. We may have to use the resume token if we want to reliably honor the opt-in.

[rfk]

In initial discussions in @shane-tomlinson we decided that it's fine to punt on the different-browser case for the initial version; we'll lose some percentage of people who might otherwise have opted-in, but at least it fails "safely" in that everything else still works.

[rfk]

(we should of course follow up on it for subsequent versions)

[shane-tomlinson]

An OAuth token needs to be generated for opt-in, in a 2nd browser the user will not have a sessionToken that can be used to generate an OAuth token. We can get around this in a couple of ways:

1. Have the auth server fetch an oauth token and opt in on behalf of the user.
2. Have the user authenticate in the 2nd browser, an oauth token can then be created, and the user opts in.

There are probably other ways, but of those two, I'm partial to the 2nd.

[zaach]

We could attempt to opt-in from the original confirmation tab also, as a stop gap. But I wonder how many people verify in a different browser? I didn't see any explicit metrics being collected around that– but it would be useful to know.

[rfk]

I'm seeing mis-alignment of the "email preferences" link in the latest version:

[rfk]

Hmm, actually I have one browser window where the mis-alignment only triggers if I shrink the browser window to a certain size and goes away when I make it bigger, and another window where it's always like that across window resizes and page reloads.

[zaach]

Merging now. We'll follow up with PRs to fix the remaining issues.

[vladikoff]

Awesome!! Will keep an eye on TeamCity