Count the number of times a user opts out of Instant Translation

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: MarcoM, Assigned: mano)

Tracking

unspecified
Firefox 33
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox32 fixed, firefox33 fixed)

Details

(Whiteboard: [translation])

Attachments

(2 attachments, 1 obsolete attachment)

Breaking down Story into multiple smaller, easier-to-estimate bugs.  These individual bugs are dependencies which block the completion of the whole story.  The team provides point estimates to each of the individual bugs instead of the entire story.
Whiteboard: p=0 [qa-] → [translation] p=0 [qa-]
Summary: Story Breakdown - Count the number of times a user opts out of Instant Translation → Count the number of times a user opts out of Instant Translation
Status: NEW → ASSIGNED
Whiteboard: [translation] p=0 [qa-] → [translation] p=1 s=it-30c-29a-28b.3 [qa-]
Assignee: nobody → smacleod
Whiteboard: [translation] p=1 s=it-30c-29a-28b.3 [qa-] → [translation] p=1 s=it-31c-30a-29b.1 [qa-]
Whiteboard: [translation] p=1 s=it-31c-30a-29b.1 [qa-] → [translation] p=1 s=it-31c-30a-29b.2 [qa-]
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: [translation] p=1 s=it-31c-30a-29b.2 [qa-] → [translation] p=1 s=it-31c-30a-29b.3 [qa-]
Whiteboard: [translation] p=1 s=it-31c-30a-29b.3 [qa-] → [translation] p=1 s=it-32c-31a-30b.1 [qa-]
Mass move of translation bugs to the new Translation component.
Component: Firefox Operations → Translation
Product: Tracking → Firefox
Version: --- → unspecified
Whiteboard: [translation] p=1 s=it-32c-31a-30b.1 [qa-] → [translation] p=1 s=it-32c-31a-30b.2 [qa-]
Whiteboard: [translation] p=1 s=it-32c-31a-30b.2 [qa-] → [translation] p=1 s=it-32c-31a-30b.3 [qa-]
Depends on: 978158
No longer blocks: 977734
Depends on: 1015525
This won't be fixed by (and thus doesn't depend on) bug 1015525 as that only records the state of the translation prefs. This bug is about recording when a user clicks the "No, thanks" button when we offer to translate.
No longer depends on: 1015525
Hardware: x86_64 → All
Marco, can you please update the backlog and assign this bug to Mano? Steven is out for some training in Toronto this week. Thanks!
Assignee: smacleod → mano
Flags: needinfo?(mmucci)
Iteration 32.3 Backlog updated with Mano assigned to Bug 977774.
Flags: needinfo?(mmucci)
Hi Tim, based on today's update meeting you will confirm if this bug should be removed from the current iteration based on the progress made on the dependent Bug 978158.
Flags: needinfo?(ttaubert)
I'll land bug 978158 later today.
Flags: needinfo?(ttaubert)
Whiteboard: [translation] p=1 s=it-32c-31a-30b.3 [qa-] → [translation] p=1 s=33.1 [qa-]
Comment on attachment 8437486 [details] [diff] [review]
add the metric and test it

Review of attachment 8437486 [details] [diff] [review]:
-----------------------------------------------------------------

Is the caller for recordDeniedTranslationOffer part of a different patch?

just a note in advance: if the user clicks "Not Now", and then retrieves the infobar through the urlbar icon, and clicks not now again, I think we should not count that twice. (i.e, count only one dismissal per page)
Flags: needinfo?(mano)
(In reply to :Felipe Gomes from comment #8)
> Comment on attachment 8437486 [details] [diff] [review]
> add the metric and test it
> 
> Review of attachment 8437486 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is the caller for recordDeniedTranslationOffer part of a different patch?
> 

Yes, that was my plan (in that patch, I'm also adding a test for the UI part). Please review this part of the work.

> just a note in advance: if the user clicks "Not Now", and then retrieves the
> infobar through the urlbar icon, and clicks not now again, I think we should
> not count that twice. (i.e, count only one dismissal per page)

Unfortunately, the TranslationUI object is designed in such a way that it's browser-centric, not document-centric. By resetting some flag in documentStateReceived for STATE_OFFER data, I could introduce some "document-initialization" mechanism. That would make this edge-case fixable. However, a more obvious and common case seems un-fixable in the current design: (1) Not Now a translation offer; (2) Click on some link; (3) Go back; (4) Not Now again.

So, unless you think that this edge case is important enough to fix (leaving the back & foward issue in place), I'm inclined to keep it simple and just track not-now commands activation.

Moving forward, probably in a new bug, I think that the content script should send a distinct message when a translation "session" beings. It could also report back & forward/whole-now state.
Flags: needinfo?(mano)
One more thing: the generic [x] button is in place in the "offer" state alongside the "Not Now" button. In practice, they do the same thing. Should it count for this metric? One may argue that if the user closed the info-bar this way, s/he might have closed the notification without reading its content at all.
Comment on attachment 8437486 [details] [diff] [review]
add the metric and test it

Review of attachment 8437486 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/healthreport/docs/dataformat.rst
@@ +1553,5 @@
>      Integer count of the number of times the user manually adjusted the detected
>      language after having first translated the page.
> +deniedTranslationOffer
> +    Integer count of the numbers of times the user opted-out offered
> +    page translation.

Please expand this comment by saying it's by clicking the Not Now or X close button in the translation infobar
Attachment #8437486 - Flags: review?(felipc) → review+
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #9)
> So, unless you think that this edge case is important enough to fix (leaving
> the back & foward issue in place), I'm inclined to keep it simple and just
> track not-now commands activation.

Makes sense, let's keep it simple.
 
> Moving forward, probably in a new bug, I think that the content script
> should send a distinct message when a translation "session" beings. It could
> also report back & forward/whole-now state.

I didn't understand exactly what you mean by a translation "session", but let's leave it for another bug

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #10)
> One more thing: the generic [x] button is in place in the "offer" state
> alongside the "Not Now" button. In practice, they do the same thing. Should
> it count for this metric? One may argue that if the user closed the info-bar
> this way, s/he might have closed the notification without reading its
> content at all.

I think it should count. Even if they didn't read it they are dismissing the offer, so it's still meaningful
https://hg.mozilla.org/integration/fx-team/rev/61c817d378b2
Whiteboard: [translation] p=1 s=33.1 [qa-] → [translation] p=1 s=33.1 [qa-] [keep open]
Comment on attachment 8444099 [details] [diff] [review]
the rest of it

Review of attachment 8444099 [details] [diff] [review]:
-----------------------------------------------------------------

The test looks great to me, excluding some nits marked below. The rest of the patch lgtm as well but I'd like to let Felipe give the final review.

::: browser/components/translation/test/browser_translation_fhr.js
@@ +9,5 @@
>  
> +let MetricsChecker = {
> +  _metrics: { pageCount: 0, charCount: 0, deniedOffers: 0 },
> +  _metricsTime: new Date(),
> +  _midnightError: new Error("Cannot get metrics on midnight"),

Nit: Maybe "Getting metrics around midnight may fail sometimes"?

@@ +31,5 @@
> +
> +    // .get() may return `undefined`, which we can't compute.
> +    this._metrics = { pageCount: day.get("pageTranslatedCount") || 0,
> +                      charCount: day.get("charactersTranslatedCount") || 0,
> +                      deniedOffers: day.get("deniedTranslationOffer") || 0 };

Formatting nit:

this._metrics = {
  pageCount: day.get("pageTranslatedCount") || 0,
  charCount: day.get("charactersTranslatedCount") || 0,
  deniedOffers: day.get("deniedTranslationOffer") || 0
};

@@ +34,5 @@
> +                      charCount: day.get("charactersTranslatedCount") || 0,
> +                      deniedOffers: day.get("deniedTranslationOffer") || 0 };
> +    this._metricsTime = metricsTime;
> +  }),
> +  

Nit: white space

@@ +35,5 @@
> +                      deniedOffers: day.get("deniedTranslationOffer") || 0 };
> +    this._metricsTime = metricsTime;
> +  }),
> +  
> +  checkAdditions: function* (additions) {

checkAdditions: Task.async(function* (additions) { ...

@@ +40,5 @@
> +    let prevMetrics = this._metrics, prevMetricsTime = this._metricsTime;
> +    try {
> +      yield this._updateMetrics();
> +    }
> +    catch(ex if ex == this._midnightError) {

Nit: } catch (ex ...) {

@@ +48,5 @@
> +    // Check that it's still the same day of the month as when we started. This
> +    // prevents intermittent failures when the test starts before and ends after
> +    // midnight.
> +    if (this._metricsTime.getDate() != prevMetricsTime.getDate()) {
> +      for (let metric in prevMetrics) {

for (let metric of Object.keys(prevMetrics)) {

@@ +53,5 @@
> +        prevMetrics[metric] = 0;
> +      }
> +    }
> +
> +    for (let metric in additions) {

Object.keys() ...

@@ +130,5 @@
> +  return Task.spawn(function* task_accept_translation_offer() {
> +    let browser = tab.linkedBrowser;
> +    getInfobarElement(browser, "toLanguage").value = to;
> +    getInfobarElement(browser, "toLanguage").doCommand();
> +    yield waitForMessage(tab.linkedBrowser, "Translation:Finished");

Nit: yield waitForMessage(browser, ...);

@@ +140,5 @@
> +    let tab = yield offerTranslatationFor(text, from);
> +    yield acceptTranslationOffer(tab, to);
> +    if (closeTab)
> +      gBrowser.removeTab(tab);
> +    return tab;

if (closeTab) {
  gBrowser.removeTab(tab);
} else {
  return tab;
}

Returning the tab after closing it feels weird :)
Attachment #8444099 - Flags: feedback+
Attachment #8444099 - Attachment is obsolete: true
Attachment #8444099 - Flags: review?(felipc)
Attachment #8444311 - Flags: review?(felipc)
Comment on attachment 8444311 [details] [diff] [review]
comments addressed

Review of attachment 8444311 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with renaming the new function in TranslationUI to infobarClosed and moving the state check inside it
Attachment #8444311 - Flags: review?(felipc) → review+
Iteration: --- → 33.2
Points: --- → 1
QA Whiteboard: [qa-]
Whiteboard: [translation] p=1 s=33.1 [qa-] [keep open] → [translation] [keep open]
sorry had backout this change in https://tbpl.mozilla.org/?tree=Fx-Team&onlyunstarred=1&rev=0b7d0cbaa2bd since one of this 2 patches seems to have caused test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=42331284&tree=Fx-Team
https://hg.mozilla.org/mozilla-central/rev/b37e754928cb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment on attachment 8444311 [details] [diff] [review]
comments addressed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This bug is part of the automatic translation feature, which we want to A/B with a subset of Aurora 32 users.
User impact if declined: Data about users clicking the "Not now" button won't be collected
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): small
String or IDL/UUID changes made by this patch: none
Attachment #8444311 - Flags: approval-mozilla-aurora?
Felipe, note that you must land https://bugzilla.mozilla.org/show_bug.cgi?id=1029363 along with this patch (when I filed that one, I didn't realize tinderbox builds testing is actually in that on-its-own configuration, thus thought the patch here wouldn't be backed out).
Attachment #8444311 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.