1066062 - Add "version" (and "channel"?), nix "source" in about:feedback submission

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Will Kahn-Greene [:willkg] ET needinfo? me]

+++ This bug was initially created as a clone of Bug #799562 +++

The changes in bug #799562 that switch the feedback submission mechanism to use the Input API missed something (totally my fault). The previous submission mechanism did an HTTP POST and Input would infer the Firefox for Android version from the User Agent. The changes in bug #799562 switch it to the Input API and doesn't send a User Agent, so Input can't infer the version.

Because of that, feedback responses will have version set to "Unknown" and we lose out on being able to match feedback to versions.

Here's an example I posted just now using nightly (2014-09-10):

https://input.mozilla.org/en-US/dashboard/response/4594582

It lists the version as "Unknown".


The code changes should be around here:

--- a/mobile/android/chrome/content/aboutFeedback.js
+++ b/mobile/android/chrome/content/aboutFeedback.js
@@ -92,42 +95,44 @@ function sendFeedback(aEvent) {
   let sectionElement = document.getElementById(section);
   let descriptionElement = sectionElement.querySelector(".description");
 
   // Bail if the description value isn't valid. HTML5 form validation will take care
   // of showing an error message for us.
   if (!descriptionElement.validity.valid)
 	return;
 
-  let data = new FormData();
-  data.append("description", descriptionElement.value);
-  data.append("_type", 2);
+  let data = {};
+  data["happy"] = false;
+  data["description"] = descriptionElement.value;
+  data["product"] = FEEDBACK_PRODUCT_STRING;


Need to add a line along these lines:

    data["version"] = some-way-to-get-the-version;


Further, it'd be super great if we could add the channel, too:

    data["channel"] = some-way-to-get-the-channel;

Both "version" and "channel" are free form, so any helpful values are good.

Comment 1

[Kevin Brosnan [:kbrosnan]]

[Tracking Requested - why for this release]:

Comment 2

[Kevin Brosnan [:kbrosnan]]

Regression from bug 799562. This needs to be a forward fix as the input site has changed.

Comment 3

[Will Kahn-Greene [:willkg] ET needinfo? me]

One other thing needs to change. The "source" field is for non-organic feedback. If it's the case that Firefox for Android has prompted the user for feedback, then it should have a source. However, if the user is leaving feedback of their own accord, then it should not have a source.

We use source and campaign to distinguish between organic (the user has sought us out to leave feedback) from non-organic (we have asked the user for feedback) feedback.

The easy fix is to remove the line in question.

Sorry about that. I'll clarify this in the API documentation.

Comment 4

[:Margaret Leibovic]

Hey Vivek, I know you've been working really hard on bug 1007436, but we should really try to land a patch for this bug sooner rather than later. Have you had a chance to take a look at this? I think it should be a pretty easy fix.

Comment 5

[Vivek Balakrishnan[:vivek]]

Margaret, 
I'll take this a priority and look into this now.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 1066062.patch

A new patch that posts version & channel data during negative feedback submission.

The patch also includes locale and platform information.

Source has been removed as this is an organic feedback.

An sample data posted from this patch : https://input.mozilla.org/en-GB/dashboard/response/4606934

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

Vivek: The feedback on Input looks good to me. Thank you for working on this!

Comment 8

[:Margaret Leibovic]

Comment on attachment 8492149 [details] [diff] [review]
1066062.patch

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

This looks good! I'd just like to see us use Services.appinfo and Services.sysinfo instead of doing the XPCOM stuff ourselves.

I'd also like rnemwan to take a look at the locale line to make sure it's correct.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +117,5 @@
>      data["url"] = urlElement.value;
>    }
>  
>    let sysInfo = Cc["@mozilla.org/system-info;1"].getService(Ci.nsIPropertyBag2);
> +  let appInfo = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).QueryInterface(Ci.nsIXULRuntime);

It would be cleaner to just uses Services.appinfo instead of creating this local variable. And while you're here you could replace sysInfo with Services.sysinfo.

@@ +124,5 @@
>    data["device"] = sysInfo.get("device");
>    data["manufacturer"] = sysInfo.get("manufacturer");
> +  data["platform"] = appInfo.OS;
> +  data["version"] = appInfo.version;
> +  data["locale"] = chromeRegistry.getSelectedLocale("global");

I'd like to get feedback from rnemwan to make sure this is the right way to get the selected locale.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8492149 [details] [diff] [review]
1066062.patch

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

::: mobile/android/chrome/content/aboutFeedback.js
@@ +124,5 @@
>    data["device"] = sysInfo.get("device");
>    data["manufacturer"] = sysInfo.get("manufacturer");
> +  data["platform"] = appInfo.OS;
> +  data["version"] = appInfo.version;
> +  data["locale"] = chromeRegistry.getSelectedLocale("global");

This will return the correct value *the first time it is loaded*.

If you switch locales, the value is stale.

Unfortunately, the only way to refresh this is to call .reloadChrome(), which we don't want to do.

The right answer here is to use the locale service directly, or cheat and rely on Fennec's own locale prefs.

Land this for now, if you're OK that it will sometimes be wrong, and please file a follow up to change this to not get stale.

Comment 10

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #9)
> Comment on attachment 8492149 [details] [diff] [review]
> 1066062.patch
> 
> Review of attachment 8492149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/aboutFeedback.js
> @@ +124,5 @@
> >    data["device"] = sysInfo.get("device");
> >    data["manufacturer"] = sysInfo.get("manufacturer");
> > +  data["platform"] = appInfo.OS;
> > +  data["version"] = appInfo.version;
> > +  data["locale"] = chromeRegistry.getSelectedLocale("global");
> 
> This will return the correct value *the first time it is loaded*.
> 
> If you switch locales, the value is stale.
> 
> Unfortunately, the only way to refresh this is to call .reloadChrome(),
> which we don't want to do.
> 
> The right answer here is to use the locale service directly, or cheat and
> rely on Fennec's own locale prefs.
> 
> Land this for now, if you're OK that it will sometimes be wrong, and please
> file a follow up to change this to not get stale.

I don't think this is a big issue, since we only make this call when we submit feedback, and we only ever prompt users for feedback once. They would have to submit feedback, change the locale, and then manually open about:feedback themselves afterwards. I imagine this is quite an edge case.

Comment 11

[Vivek Balakrishnan[:vivek]]

Attachment: 1066062.patch

A new patch that corrects the review comments.

Source has been re-enabled as most probably the user would give feedback when fennec prompts them.

A follow-up bug would be filed for organic feedback

@rnewman: Can you please review if the code to get the locale is correct?

A sample data from the patch: https://input.mozilla.org/en-GB/dashboard/response/4607525

Comment 12

[Richard Newman [:rnewman]]

Locale fetching is equivalent to the previous mechanism, so fine.

I'll file a bug to invalidate/correct the locale service when the app locale changes.

Comment 13

[Richard Newman [:rnewman]]

Filed Bug 1070211.

Comment 14

[:Margaret Leibovic]

Comment on attachment 8492395 [details] [diff] [review]
1066062.patch

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

Looks good!

Comment 15

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/101da1df6c95

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/101da1df6c95

Comment 17

[Will Kahn-Greene [:willkg] ET needinfo? me]

(In reply to vivek from comment #11)
> Created attachment 8492395 [details] [diff] [review]
> 1066062.patch
> 
> A new patch that corrects the review comments.
> 
> Source has been re-enabled as most probably the user would give feedback
> when fennec prompts them.
> 
> A follow-up bug would be filed for organic feedback

Do we know for a fact that most feedback from fennec is coming from the feedback ask and not someone navigating the settings menus or is this a hunch?

I think as long as the new bug lands in this dev cycle, this is ok. Otherwise we should be hedging towards the existing behavior which is not to provide the source. Messing this up makes the User Advocacy metrics more complicated.

Comment 18

[:Margaret Leibovic]

(In reply to Will Kahn-Greene [:willkg] from comment #17)
> (In reply to vivek from comment #11)
> > Created attachment 8492395 [details] [diff] [review]
> > 1066062.patch
> > 
> > A new patch that corrects the review comments.
> > 
> > Source has been re-enabled as most probably the user would give feedback
> > when fennec prompts them.
> > 
> > A follow-up bug would be filed for organic feedback
> 
> Do we know for a fact that most feedback from fennec is coming from the
> feedback ask and not someone navigating the settings menus or is this a
> hunch?

We do have telemetry probes that will let us know how often users choose to submit feedback through the settings menu, but mostly yes, this is a hunch.

> I think as long as the new bug lands in this dev cycle, this is ok.
> Otherwise we should be hedging towards the existing behavior which is not to
> provide the source. Messing this up makes the User Advocacy metrics more
> complicated.

In the patch that landed, I actually don't see us setting the source:
https://hg.mozilla.org/mozilla-central/rev/101da1df6c95

vivek, did you purposefully omit this from this final version of the patch? Also, did you file that follow-up bug?

Comment 19

[Will Kahn-Greene [:willkg] ET needinfo? me]

(In reply to :Margaret Leibovic from comment #18)
> 
> In the patch that landed, I actually don't see us setting the source:
> https://hg.mozilla.org/mozilla-central/rev/101da1df6c95

It's setting the source here:

https://hg.mozilla.org/mozilla-central/rev/101da1df6c95#l1.35

Comment 20

[:Margaret Leibovic]

(In reply to Will Kahn-Greene [:willkg] from comment #19)
> (In reply to :Margaret Leibovic from comment #18)
> > 
> > In the patch that landed, I actually don't see us setting the source:
> > https://hg.mozilla.org/mozilla-central/rev/101da1df6c95
> 
> It's setting the source here:
> 
> https://hg.mozilla.org/mozilla-central/rev/101da1df6c95#l1.35

Oh, yeah. I was confused because that was added in bug 799562, not this bug :)

Perhaps we should just remove that line for the time being then, and re-land it when we can actually properly track whether or not we prompted the user for feedback.

Comment 21

[Will Kahn-Greene [:willkg] ET needinfo? me]

That sounds good to me!

Comment 22

[Vivek Balakrishnan[:vivek]]

Attachment: 1066062_remove_source.patch

A follow up patch that removes the source field.

As discussed over irc, filed a follow up bug #1071117 to set source field when the user is prompted.

Comment 23

[:Margaret Leibovic]

Comment on attachment 8493228 [details] [diff] [review]
1066062_remove_source.patch

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

Thanks, I'll land this for you.

Comment 24

[:Margaret Leibovic]

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

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

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