Closed Bug 1066062 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
fennec 35+ ---

People

(Reporter: willkg, Assigned: vivek, Mentored)

References

Details

(Keywords: regression, Whiteboard: [lang=js])

Attachments

(2 files, 1 obsolete file)

+++ 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.
Assignee: nobody → vivekb.balakrishnan
[Tracking Requested - why for this release]:
tracking-fennec: --- → ?
tracking-fennec: ? → 35+
Regression from bug 799562. This needs to be a forward fix as the input site has changed.
Keywords: regression
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.
Summary: Add "version" (and "channel"?) to about:feedback submission → Add "version" (and "channel"?), nix "source" in about:feedback submission
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.
Flags: needinfo?(vivekb.balakrishnan)
Margaret, 
I'll take this a priority and look into this now.
Flags: needinfo?(vivekb.balakrishnan)
Attached patch 1066062.patch (obsolete) — Splinter Review
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
Attachment #8492149 - Flags: review?(margaret.leibovic)
Vivek: The feedback on Input looks good to me. Thank you for working on this!
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.
Attachment #8492149 - Flags: review?(margaret.leibovic)
Attachment #8492149 - Flags: review+
Attachment #8492149 - Flags: feedback?(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.
Attachment #8492149 - Flags: feedback?(rnewman) → feedback+
(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.
Attached patch 1066062.patchSplinter Review
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
Attachment #8492149 - Attachment is obsolete: true
Attachment #8492395 - Flags: review?(margaret.leibovic)
Flags: needinfo?(willkg)
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
Comment on attachment 8492395 [details] [diff] [review]
1066062.patch

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

Looks good!
Attachment #8492395 - Flags: review?(margaret.leibovic) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/101da1df6c95
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(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.
Flags: needinfo?(willkg)
(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?
Flags: needinfo?(vivekb.balakrishnan)
(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
(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.
That sounds good to me!
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.
Attachment #8493228 - Flags: review?(margaret.leibovic)
Flags: needinfo?(vivekb.balakrishnan)
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.
Attachment #8493228 - Flags: review?(margaret.leibovic) → review+
You need to log in before you can comment on or make changes to this bug.