1250758 - Pass utm_ campaign parameters to Mozilla.UITour.showFirefoxAccounts for campaign tracking.

Status: RESOLVED FIXED

(www.mozilla.org :: Bedrock, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

In bug 1244929, we've landed the ability to pass utm_ params to the showFirefoxAccounts command. These params will be passed to the Firefox accounts server.

The parameters must all start with "utm_" and contain only alphanumeric characters and dash or underscore - see that bug for more details. The call would look something like:

Mozilla.UITour.showFirefoxAccounts({ utm_campaign: "some-value", utm_somethingelse: "something-else"});

Comment 1

[Jonathan Petto [:jpetto]]

:cmore - Two questions on the implementation:

1. Do we want any and all "utm_" parameters sent to showFirefoxAccounts?

2. Do we want granular/page-level control over when "utm_" params are sent, or do we want to say that any time showFirefoxAccounts is called, all "utm_" params are automatically included?

Comment 2

[Jonathan Petto [:jpetto]]

:garethc - Can you chime in on comment 1?

Comment 3

[Jonathan Petto [:jpetto]]

From a conversation with :garethc in IRC:

- All utm_ params present in the URL should be passed along to showFirefoxAccounts()

- utm_content={pagepath} should be added to the params sent to showFirefoxAccounts(), where {pagepath} equals everything in the URL after the locale

- utm_campaign, if not present in the URL, should be set to 'page referral - not part of a campaign'

Comment 4

[Jonathan Petto [:jpetto]]

:markh - After looking at the patch [1], it appears spaces are not allowed in names or values of utm_ params sent to showFirefoxAccounts. Can you confirm?

If true, then our proposed default value for utm_campaign wont work. I'm guessing changing the default is preferable over another patch to showFirefoxAccounts that allows spaces.

:garethc - Does the ban on spaces for all utm_ keys and values work on your end, or do you see potential issues down the road? Could we change 'page referral - not part of a campaign' to 'pagereferral-nocampaign' (or something else without spaces)?

[1] https://hg.mozilla.org/integration/fx-team/rev/800b8a648489#l2.68

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jon Petto [:jpetto] from comment #4)
> :markh - After looking at the patch [1], it appears spaces are not allowed
> in names or values of utm_ params sent to showFirefoxAccounts. Can you
> confirm?

I can confirm, but I can't see a good reason that spaces can't be allowed - we will escape them. MattN, what do you think?

Comment 6

[Jonathan Petto [:jpetto]]

There's also a request to pass the current page's path in a utm_ parameter, e.g.

{ 'utm_content': '/firefox/sync/' }

Would it be possible to also whitelist the / character?

Apologies to keep asking for inches - I know we need to draw the line somewhere.

Comment 7

[Matthew N. [:MattN]]

(In reply to Mark Hammond [:markh] from comment #5)
> (In reply to Jon Petto [:jpetto] from comment #4)
> > :markh - After looking at the patch [1], it appears spaces are not allowed
> > in names or values of utm_ params sent to showFirefoxAccounts. Can you
> > confirm?
> 
> I can confirm, but I can't see a good reason that spaces can't be allowed -
> we will escape them. MattN, what do you think?

I think allowing spaces in the param names would probably not be needed, allowing them in the value would be okay if that's something we need.

(In reply to Jon Petto [:jpetto] from comment #6)
> There's also a request to pass the current page's path in a utm_ parameter,
> e.g.
> 
> { 'utm_content': '/firefox/sync/' }
> 
> Would it be possible to also whitelist the / character?

Sure.
 
> Apologies to keep asking for inches - I know we need to draw the line
> somewhere.

No worries, I suggested making this overly strict by default but I don't have a problem adding characters that we will use/need.

Comment 8

[Matthew N. [:MattN]]

(In reply to Jon Petto [:jpetto] from comment #3)
> - utm_content={pagepath} should be added to the params sent to
> showFirefoxAccounts(), where {pagepath} equals everything in the URL after
> the locale

Does this mean:
a) everything in the *path* after the locale
or
b) everything in the path + query string after the locale
?

For (b) we would need to allow many more characters like [?&=] plus probably [%] for encoding in query params.

Comment 9

[Jonathan Petto [:jpetto]]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #8)
> (In reply to Jon Petto [:jpetto] from comment #3)
> > - utm_content={pagepath} should be added to the params sent to
> > showFirefoxAccounts(), where {pagepath} equals everything in the URL after
> > the locale
> 
> Does this mean:
> a) everything in the *path* after the locale
> or
> b) everything in the path + query string after the locale
> ?
> 
> For (b) we would need to allow many more characters like [?&=] plus probably
> [%] for encoding in query params.

I was just about to comment on that. I began coding to just allow the path, but got struck with the thought that we would probably want at least some of the query string - e.g. to pass a page variation (?v=2). If you feel comfortable white-listing the related characters, we will almost definitely use them.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jon Petto [:jpetto] from comment #9)
> > For (b) we would need to allow many more characters like [?&=] plus probably
> > [%] for encoding in query params.
> 
> I was just about to comment on that. I began coding to just allow the path,
> but got struck with the thought that we would probably want at least some of
> the query string - e.g. to pass a page variation (?v=2). If you feel
> comfortable white-listing the related characters, we will almost definitely
> use them.

If we are allowing [/?&=], we are probably running out of characters in the value that we were guarding against - ie, we should just consider removing the restriction on the value (but not the name) - they are all going to be escaped anyway...

Comment 11

[Jonathan Petto [:jpetto]]

Yeah, sounds like we're getting to the point where it's more manageable to open up and sanitize the value. Names should be fine with the existing restrictions though.

Comment 12

[Jonathan Petto [:jpetto]]

:garethc - For the utm_content value, do you want just the page path (e.g. /firefox/sync/), or the page path + any existing query params (e.g. /firefox/sync/?v=2&foo=bar)? If the latter, should the utm_ params be removed, or included/repeated there as well?

Comment 13

[Jonathan Petto [:jpetto]]

Chatted with :garethc on IRC. He confirmed utm_content should only hold the page path, and not page path + querystring.

Still probably a good idea to allow more than alphanumeric characters in utm_ values for future flexibility.

Comment 14

[Gareth Cull [:garethc]]

(In reply to Jon Petto [:jpetto] from comment #4)

> :garethc - Does the ban on spaces for all utm_ keys and values work on your
> end, or do you see potential issues down the road? Could we change 'page
> referral - not part of a campaign' to 'pagereferral-nocampaign' (or
> something else without spaces)?

Sure. I'm fine with that.

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jon Petto [:jpetto] from comment #13)
> Still probably a good idea to allow more than alphanumeric characters in
> utm_ values for future flexibility.

Note that bug 1253120 allows exactly this and is already on fx-team - it should be on mozilla-central withing 24 hours.

Comment 16

[[github robot]]

Commits pushed to master at https://github.com/mozilla/bedrock

https://github.com/mozilla/bedrock/commit/f44b8df443910905a70adb8e3c3aeb3373853d8c
[fix bug 1250758] Add utm_ params to about:accounts.

https://github.com/mozilla/bedrock/commit/28c7cffeba4025260340ccf673df92ce191c38bb
Merge pull request #3934 from mozilla/bug-1250758-pass-utm-params-to-about-accounts

[fix bug 1250758] Add utm_ params to about:accounts.