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

Status: RESOLVED FIXED

(Firefox :: Translations, defect)

Description

[Marco Mucci [:MarcoM]]

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.

Comment 1

[Florian Quèze [:florian]]

Mass move of translation bugs to the new Translation component.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please update the backlog and assign this bug to Mano? Steven is out for some training in Toronto this week. Thanks!

Comment 4

[Marco Mucci [:MarcoM]]

Iteration 32.3 Backlog updated with Mano assigned to Bug 977774.

Comment 5

[Marco Mucci [:MarcoM]]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

I'll land bug 978158 later today.

Comment 7

[Asaf Romano (gone)]

Attachment: add the metric and test it

Comment 8

[:Felipe Gomes (needinfo for replies!)]

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)

Comment 9

[Asaf Romano (gone)]

(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.

Comment 10

[Asaf Romano (gone)]

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 11

[:Felipe Gomes (needinfo for replies!)]

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

Comment 12

[:Felipe Gomes (needinfo for replies!)]

(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

Comment 13

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/61c817d378b2

Comment 14

[Asaf Romano (gone)]

Attachment: the rest of it

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 16

[Asaf Romano (gone)]

Attachment: comments addressed

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/61c817d378b2

Comment 18

[:Felipe Gomes (needinfo for replies!)]

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

Comment 19

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/5e0288a4e398

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Asaf Romano (gone)]

Relanded along with the patch for bug 1029363 that should fix that failure.
https://hg.mozilla.org/integration/fx-team/rev/b37e754928cb

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b37e754928cb

Comment 23

[:Felipe Gomes (needinfo for replies!)]

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

Comment 24

[Asaf Romano (gone)]

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).

Comment 25

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aa4cb3562ef1
https://hg.mozilla.org/releases/mozilla-aurora/rev/7717946aeb52