When the Hello panel is opened via showMenu("loop"), the web will register a page ID with the convention: hello-tour_OpenPanel_<source> Where <source> will be values based on the page the user is on (e.g. "snippet4", "snippet37", "productpage"). This bug is requesting that the <source> value is appended to the URLs set in the Get Started button (bug 1099462), and the Gear menu (bug 1101286). The key for the parameter will be "&hello-tour-src=<source>" The expiration of this page ID will be short (1-3 minutes). + MattN to check this bug for accuracy

Per our discussion with MattN, the resuming of the tour will be handled in a separate stream (ref bug 1117898).

After discussion with cmore and garethc, we'd like to add change the URL parameter from &hello-tour-src=<source> to &utm_content=<source> Thanks

Comment on attachment 8555393 [details] [diff] [review] Patch v1.1 Review of attachment 8555393 [details] [diff] [review]: ----------------------------------------------------------------- Neat that we can up with the same idea :) ::: browser/components/uitour/UITour.jsm @@ +376,5 @@ > + break; > + } > + this.currentPageID = data.pageID; > + > + // The rest is only relevant if Telemtry is enabled. Nit: fix the existing typo in 'Telemtry'

Comment on attachment 8555393 [details] [diff] [review] Patch v1.1 Review of attachment 8555393 [details] [diff] [review]: ----------------------------------------------------------------- Just wanted to get a quick clarification. Also, I hope it's okay that I'm adding notes here. If not, I'm happy to just add as bug comments in the future. ::: browser/components/loop/MozLoopService.jsm @@ +1543,5 @@ > url.searchParams.set("utm_campaign", aSrc); > } > + > + if (UITour.currentPageID) { > + url.searchParams.set("utm_content", UITour.currentPageID); This is saying that whatever ID is passed in registerPageID will be the value ultimately set in utm_content=. In comment #0, we discussed that the pageID would have an identifying string preceding the <source>. That's fine if this is no longer the case/an identifier isn't necessary, but I just want to make sure we're clear here.

Thanks for the feedback Cory. I appreciate the feedback, that's perfectly fine how you commented. Redirecting the review to Matt since we talked about this approach together.

Comment on attachment 8555541 [details] [diff] [review] Patch v2 Review of attachment 8555541 [details] [diff] [review]: ----------------------------------------------------------------- Cory wants this to work even when telemetry isn't enabled. ::: browser/components/uitour/test/browser_UITour_loop.js @@ +14,4 @@ > const { LoopRooms } = Components.utils.import("resource:///modules/loop/LoopRooms.jsm", {}); > > function test() { > + UITelemetry._enabled = true; Remove the lines above in your new patch so we make sure it works with this being false. @@ +20,5 @@ > > let tests = [ > + taskify(function* test_gettingStartedClicked_linkOpenedWithExpectedParams() { > + Services.prefs.setBoolPref("loop.gettingStarted.seen", false); > + Services.prefs.setCharPref("loop.server", "http://localhost:5001"); Is this needed instead of the default value from prefs_general? @@ +39,5 @@ > + let gettingStartedButton = loopDoc.getElementById("fte-button"); > + ok(gettingStartedButton, "Getting Started button should be found"); > + > + let newTabPromise = waitForConditionPromise(() => { > + return gBrowser.currentURI.path.contains("utm_content=hello-tour_OpenPanel_testPage"); This reminded me that nsIURI have the query parameters in .path (unlike location.pathname) @@ +46,5 @@ > + gettingStartedButton.click(); > + yield newTabPromise; > + yield gBrowser.removeCurrentTab(); > + > + loopPanel.hidePopup(); This should happen from the click already… @@ +52,5 @@ > + return !loopButton.open; > + }, "Menu should be hidden after hideMenu()"); > + > + checkLoopPanelIsHidden(); > + gContentAPI.registerPageID(""); I'm not sure what this line is for

Per a conversation with :jaws in IRC > <jaws> ckprice: how do page IDs expire within a couple minutes? what if about:home is left open for hours before being interacted with? The 3 minute requirement is no longer valid for this bug. I do think it makes sense from a high level to extend this limit out to the length of the user's session. Or if that's not possible, 1 hour. From a high level, if the user pulled down the Hello panel from snippet01, then closes it, and then opens it again on a different tab an hour later, it's still appropriate to attribute the engagement of the Get Started CTA to that original snippet referral. > <jaws> MattN: if a pageID is supplied, then hours pass, and then the user clicks the button, what should we send as the content_source? nothing? If there is no matching pageID, do not append the utm_content parameter.

Using a 1-hour timeout in this patch. If we want to do a "session"-length timeout, we can just use the newest value in the in-memory array.

Comment on attachment 8557339 [details] [diff] [review] Patch v3 Review of attachment 8557339 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Cory Price [:ckprice] from comment #9) > Per a conversation with :jaws in IRC > > > <jaws> ckprice: how do page IDs expire within a couple minutes? what if about:home is left open for hours before being interacted with? > > The 3 minute requirement is no longer valid for this bug. I do think it > makes sense from a high level to extend this limit out to the length of the > user's session. Or if that's not possible, 1 hour. > > From a high level, if the user pulled down the Hello panel from snippet01, > then closes it, and then opens it again on a different tab an hour later, > it's still appropriate to attribute the engagement of the Get Started CTA to > that original snippet referral. Well, it's impossible to tell whether it should truly be attributed to the snippet or not without talking to the user. Perhaps the user wasn't interested after seeing the Hello panel open from the snippet but hours later reads an article on a news site that talks about how Hello is so cool. I would want to attribute the usage to the new article and not to the snippet in that case. It's really a matter of which way you want to skew the data (more attribution or less). > > <jaws> MattN: if a pageID is supplied, then hours pass, and then the user clicks the button, what should we send as the content_source? nothing? > > If there is no matching pageID, do not append the utm_content parameter. That's not answering the question though since a pageID was supplied in this case and you're saying to extend for the session. ::: browser/components/loop/MozLoopService.jsm @@ +1543,5 @@ > url.searchParams.set("utm_campaign", aSrc); > } > + > + // Find the most recent pageID that has the Loop prefix. > + let pageIDs = UITour.inMemorySeenPageIDs; It doesn't seem necessary to alias this. @@ +1544,5 @@ > } > + > + // Find the most recent pageID that has the Loop prefix. > + let pageIDs = UITour.inMemorySeenPageIDs; > + let mostRecentLoopPageID = [null, null]; An object would make the code below more readable as I wouldn't have to guess what index 0 and 1 are for. e.g. {id: null, data: null} @@ +1553,5 @@ > + mostRecentLoopPageID[1] = pageID[1].lastSeen; > + } > + } > + > + const PAGE_ID_EXPIRATION_MS = 60 * 60 * 1000; I personally think an hour is still too long (I preferred the 3 minutes) but it's not really up to me. An idea I had was to send the time since viewing along as another query parameter so the page can do it's own bucketing or shorter expiration but we don't need to block on it. ::: browser/components/uitour/UITour.jsm @@ +75,5 @@ > url: null, > seenPageIDs: null, > + // This map is not persisted and is used for > + // building the content source of a potential tour. > + inMemorySeenPageIDs: new Map(), Nit: pageIDsForSession? For some reason I find the "inMemory" prefix kinda funny.

(In reply to Matthew N. [:MattN] from comment #11) > Perhaps the user wasn't interested after seeing the Hello panel open from > the snippet but hours later reads an article on a news site that talks about > how Hello is so cool. IMO the volume of delayed referrals (users who simply decide to take the tour later) will far outweigh unconvinced users who stumble upon a different Hello source within 60 minutes and are swayed to engage with the tour. I still recommend 60 minutes (session is probably too long), but don't feel super strong about it. > That's not answering the question though since a pageID was supplied in this > case and you're saying to extend for the session. I assumed there was an implication in his question that the expiry had been met, given I had not replied with my "session" suggestion yet. If the pageID has expired based on the duration we determine here, the utm_content parameter should not be appended.

Pushed to tryserver: remote: You can view the progress of your build at the following URL: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=143edd53159c remote: Alternatively, view them on TBPL (soon to be deprecated): remote: https://tbpl.mozilla.org/?tree=Try&rev=143edd53159c

Landed a follow-up to trigger the re-render during the first subtest that looks for the Getting Started button, https://hg.mozilla.org/integration/fx-team/rev/3e1f409373e4

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > Landed a follow-up to trigger the re-render during the first subtest that > looks for the Getting Started button, > https://hg.mozilla.org/integration/fx-team/rev/3e1f409373e4 sorry didn't fix this :( still failed with https://treeherder.mozilla.org/logviewer.html#?job_id=1856199&repo=fx-team and so i had to this out

Changesets for backout are: https://hg.mozilla.org/integration/fx-team/rev/a955ce63f809 https://hg.mozilla.org/integration/fx-team/rev/245249caa177

Relanded with the test disabled on Linux due to uitour issues with the panel on Linux, https://hg.mozilla.org/integration/fx-team/rev/21a0484b3fa7

Backed out in https://hg.mozilla.org/integration/fx-team/rev/62a3a2a5110c for frequent-but-not-constant bc1 failures: https://treeherder.mozilla.org/logviewer.html#?job_id=1864614&repo=fx-team

Pushed a speculative fix to tryserver that changes how the tests wait to get confirmation from the UITour that the page has been registered, https://treeherder.mozilla.org/#/jobs?repo=try&revision=80a544822e1b

Mike, can you help me here with figuring out why this test is failing intermittently on Linux?

We figured this out the heartbeat test failure on IRC.

This still need to be uplifted.

remote: You can view the progress of your build at the following URL: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3474ca254dad remote: Alternatively, view them on TBPL (soon to be deprecated): remote: https://tbpl.mozilla.org/?tree=Try&rev=3474ca254dad

Comment on attachment 8565519 [details] [diff] [review] Patch for beta Approval Request Comment [Feature/regressing bug #]: needed for part of the new UI Tour for Loop [User impact if declined]: some parts of the analysis of how effective the Snippets campaign will not be possible without this [Describe test coverage new/current, TreeHerder]: pushed to tryserver (see above) [Risks and why]: low-risk, only affects UI Tour code [String/UUID change made/needed]: none

Can we confirm that this bug made the 36 release candidate?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #27) > Approval Request Comment > [Feature/regressing bug #]: needed for part of the new UI Tour for Loop > [User impact if declined]: some parts of the analysis of how effective the > Snippets campaign will not be possible without this The fact that we will be unable to test snippets messaging is a big problem. We've planned/resourced to test a variety of styles/messaging on the snippets channel based on the completion of this bug. Those plans will no longer be valid. In addition to the hard blocker of snippets messaging, we've also depended on this bug to track users through the /whatsnew and /firstrun tours which have completed development. These pages will need to be recoded. There are also analytics dashboards in progress that are assuming these sources will come through as described here.

[Tracking Requested - why for this release]: Tracking this for 37 so we get uplift to current Aurora - which will be going out to Beta users late next week. That might be as far as this can go on such short notice. Will check in with Sylvestre about RC timing to see if there's a possibility of getting this into 36 RC.

Comment on attachment 8565519 [details] [diff] [review] Patch for beta Taking because it seems very important for the feature but I am a bit sad to see that the patch landed a week ago... It could have make it in beta 9 ...

Comment on attachment 8565519 [details] [diff] [review] Patch for beta [Triage Comment] We merged beta => release.

Per IRC: https://hg.mozilla.org/releases/mozilla-beta/rev/1a2baaf50371

remote: You can view the progress of your build at the following URL: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ccc5ce56d92 remote: Alternatively, view them on TBPL (soon to be deprecated): remote: https://tbpl.mozilla.org/?tree=Try&rev=2ccc5ce56d92 Approval Request Comment [Feature/regressing bug #]: needed for part of the new UI Tour for Loop [User impact if declined]: see comment #29 [Describe test coverage new/current, TreeHerder]: pushed to tryserver (see above) [Risks and why]: low-risk, only affects UI Tour code [String/UUID change made/needed]: none

=> affected due to: (Quoting Sylvestre Ledru [:sylvestre] from comment #32) > We merged beta => release.

I verified this bug using the following environment: FF 38 Build Id:20150219070749 Test Environment: https://www-demo2.allizom.org OS: Win 7 x64, Mac Os X 10.10, Ubuntu 12.04 x86

Setting as qe-verify- since this was verified once on the demo environment.