Closed Bug 1115227 Opened 5 years ago Closed 5 years ago

Loop: add part of the UITour PageID to the Hello tour URLs as a query parameter

Categories

(Firefox :: Tours, defect)

36 Branch
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox35 --- unaffected
firefox36 + fixed
firefox37 + fixed
firefox38 --- fixed

People

(Reporter: ckprice, Assigned: jaws)

References

Details

(Whiteboard: [UITour:P1])

Attachments

(3 files, 4 obsolete files)

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
Flags: needinfo?(MattN+bmo)
Per our discussion with MattN, the resuming of the tour will be handled in a separate stream (ref bug 1117898).
Flags: needinfo?(MattN+bmo)
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
Whiteboard: [UITour:P1]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 38.2 - 9 Feb
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Patch (obsolete) — Splinter Review
Attachment #8555389 - Flags: review?(mdeboer)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #8555389 - Attachment is obsolete: true
Attachment #8555389 - Flags: review?(mdeboer)
Attachment #8555393 - Flags: review?(mdeboer)
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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8555393 - Attachment is obsolete: true
Attachment #8555393 - Flags: review?(mdeboer)
Attachment #8555541 - Flags: review?(MattN+bmo)
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
Attachment #8555541 - Flags: review?(MattN+bmo) → review-
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.
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #8555541 - Attachment is obsolete: true
Attachment #8557339 - Flags: review?(MattN+bmo)
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.
Attachment #8557339 - Flags: review?(MattN+bmo) → review+
(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
Attachment #8557339 - Attachment is obsolete: true
Attachment #8557538 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/2f80566d1106
Points: --- → 3
Flags: qe-verify+
Flags: in-testsuite+
Keywords: checkin-needed
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
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
Flags: needinfo?(jaws)
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?
Flags: needinfo?(mdeboer)
Flags: needinfo?(MattN+bmo)
We figured this out the heartbeat test failure on IRC.
Flags: needinfo?(mdeboer)
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/8893782614f6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
This still need to be uplifted.
Flags: needinfo?(jaws)
Attached patch Patch for betaSplinter Review
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
Flags: needinfo?(jaws)
Attachment #8565519 - Flags: approval-mozilla-beta?
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 ...
Attachment #8565519 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8565519 [details] [diff] [review]
Patch for beta

[Triage Comment]
We merged beta => release.
Attachment #8565519 - Flags: approval-mozilla-beta+ → approval-mozilla-release+
Attached patch Patch for auroraSplinter Review
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
Attachment #8566553 - Flags: approval-mozilla-aurora?
Attachment #8566553 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Contact: catalin.varga
=> 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.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.