1384385 - Update basket when a user changes their primary email

Status: VERIFIED FIXED

(Cloud Services :: Server: Firefox Accounts, enhancement)

Description

[Ryan Kelly [:rfkelly]]

(Apologies if this is a dupe, I recall some discussion of this previously but couldn't find the bug)

FxA will soon ship and exciting new feature where the user can change the primary email address on their account.

We currently send every new account creation over to SMFC via basket, and manage opt-in to the Firefox newsletter via the user's Firefox Account.  What is the expected behavior inside basket/SFMC when a user changes their email with FxA?

I suspect it to be something along the lines of:

* If a user changes their email address, their record in the big list of all FxA users in SMFC, is updated to reflect the new email address
* If a user is subscribed to the Firefox Newsletter, and they change their email address, they remain subscribed to the newsletter under the new email address

But I'll need some help figuring out how that maps to the underlying data model, and how to make it so in practice.

Ben, :pmac, thoughts?

Comment 1

[Ben Niolet]

Within Salesforce, we'd need to be able to know that the change occurred since we copy email addresses to multiple tables to use them for journeys and programs. it's not a huge deal, but would need to be added to all FxA-related programs we have running. 

I can handle making sure those queries are updated when the time comes.

Comment 2

[Ryan Kelly [:rfkelly]]

My proposal for signalling this from the FxA side, would look very similar to how we currently message new account creations over to basket:

* FxA server publishes an "email change" message into an SQS queue with the account uid and the new email address
* Some process inside basket pulls items off this queue and updates the necessary bits in SFMC

I'd like to avoid having to send the *old* email address as part of the message just to simplify data handling, but we can probably send it if we need it.

Comment 3

[Paul [:pmac] McLanahan]

I don't think we'll need the old address since we should already have it associated with the FxA_ID. This proposal sounds perfect to me. We'll just need to do a lot of thinking about what all to change in SF{M,D}C, but that's for us once we get the signal.

Comment 4

[Ryan Kelly [:rfkelly]]

Tracking the fxa side of this in https://github.com/mozilla/fxa-auth-server/issues/2042

Comment 5

[Ryan Kelly [:rfkelly]]

This should be up and running now in production, using the same event queue as creations and deletion.  There will be 'primaryEmailChanged' events with "uid" and "email" fields giving the uid and the updated email address.

Comment 6

[Ryan Kelly [:rfkelly]]

As I noted over in https://github.com/mozilla/addons-server/issues/6451#issuecomment-335048886 there's a potential gotcha here - standard SQS queues do not guarantee order of delivery.  So if a user happens to change their primary address multiple times in a short timeframe, it's possible that basket might receive the notifications out-of-order and wind up with incorrect state.

:pmac, it might be safer to have basket reach out to FxA to fswtchthe latest email address for the user, rather than using the value included in the notification message.  What do you think?

Comment 7

[Paul [:pmac] McLanahan]

(In reply to Ryan Kelly [:rfkelly] from comment #6)
> :pmac, it might be safer to have basket reach out to FxA to fswtchthe latest
> email address for the user, rather than using the value included in the
> notification message.  What do you think?

I agree that does sound like the best way to be sure we're right. What would the API call look like to get that info from FxA? Is that something that is already possible?

Comment 8

[Jon Buckley [:jbuck]]

We could also switch to an SQS FIFO queue which guarantees ordering

Comment 9

[Ryan Kelly [:rfkelly]]

Another suggestion from https://github.com/mozilla/addons-server/issues/6451#issuecomment-335099369 was to just include a timestamp (or some other incrementing counter) in the message.  You'd have to store the most recent timestamp on your side and discard anything below that high-water-mark, but it would remove the need for a callback to FxA.

> What would the API call look like to get that info from FxA? Is that something that is already possible?

It's possible; ni? myself to come back here with details

Comment 10

[Paul [:pmac] McLanahan]

(In reply to Ryan Kelly [:rfkelly] from comment #9)
> Another suggestion from
> https://github.com/mozilla/addons-server/issues/6451#issuecomment-335099369
> was to just include a timestamp (or some other incrementing counter) in the
> message.  You'd have to store the most recent timestamp on your side and
> discard anything below that high-water-mark, but it would remove the need
> for a callback to FxA.

That could work. I don't think I'd have to permanently store the timestamp, just maybe for a few hours or a day, since our concern here is rapid change. So I could potentially use the cache for this. I'm reluctant to keep any PII around in a DB table in basket for this kind of thing, but if it were really temporary the cache is likely sufficient. Failing that I could keep a SHA256 hash of the FxA_ID with a timestamp in the DB for this... hmmm... many options. And it would be nice to avoid having to call back to FxA for this. Let's try a timestamp for now. Easiest thing would be just a Unix Epoch integer as a timestamp in the message, then it's just a very quick "greater than" comparison. Sound worth trying?

Comment 11

[Ryan Kelly [:rfkelly]]

Epoch timestamp works for me.  I filed a bug to track this on our side: https://github.com/mozilla/fxa-auth-server/issues/2160

Comment 12

[Ryan Kelly [:rfkelly]]

:pmac, just to loop back here, is this ready to go once we do the cutover to retire fxa-basket-proxy in Bug 1417298?  Does it need to block on that work, or is basket already consuming the new events from the SQS queue?

Also /cc :vbudhram, to note that we need to close the loop here before enabling the feature for all users.

Comment 13

[Paul [:pmac] McLanahan]

Sorry Ryan. I need to get caught back up on where we are with all of our changes. Let me get back to you tomorrow. Leaving NI for now.

Comment 14

[Ryan Kelly [:rfkelly]]

> I need to get caught back up on where we are with all of our changes.

Me too :-)

Comment 15

[Paul [:pmac] McLanahan]

Okay. I see that the timestamps are done on your side. I do still need to do work for this to use those timestamps to ensure that we use the correct address. I'll get on that as soon as I can. Apologies for the delay on this one.

Comment 16

[Ryan Kelly [:rfkelly]]

> I do still need to do work for this to use those timestamps to ensure that we use the correct address

To clarify, are you already consuming the events and just need to add the race-detecting-via-timestamps thing?

Comment 17

[Paul [:pmac] McLanahan]

We are not already consuming these messages. I've not gotten word from my people on what to do with these changes on the Salesforce side. I'll see what I can do to get this rolling again. Thanks for the ping.

Comment 18

[Ryan Kelly [:rfkelly]]

Thanks.

In the interests of visibility and prioritization: this bug will soon be the sole blocker for preffing on the change-email feature in FxA (Bug 1364676).  That's not intended as a hurry-up, but I thought you might find it useful as an additional motivator to include in driving conversations forward on the SFMC :-)

Comment 19

[Paul [:pmac] McLanahan]

Recording the decision on what basket should do which I recieved from bniolet in email (thanks Ben!):

Basket should add the new email address and FxA_ID to a new Data Extension in SFMC:

ExternalKey: FXA_EmailUpdated

Fields:
- FXA_ID (text-256--primary key)
- NewEmailAddress (emailaddress)


I should have a PR for this soon.

Comment 20

[Paul [:pmac] McLanahan]

Attachment: pr 60: Fix bug 1384385: Handle FxA primaryEmailChanged messages

Comment 21

[[github robot]]

Commit pushed to master at https://github.com/mozmeao/basket

https://github.com/mozmeao/basket/commit/fa6f1617bff28f2bb3464cf5d26415aab652ddbc
Fix bug 1384385: Handle FxA primaryEmailChanged messages

Comment 22

[Paul [:pmac] McLanahan]

This was just deployed to production.