Closed Bug 1048785 Opened 8 years ago Closed 8 years ago

Loop feedback API client should send browser/platform metadata

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal

Tracking

(firefox34 fixed)

RESOLVED FIXED
mozilla34
Tracking Status
firefox34 --- fixed

People

(Reporter: NiKo, Unassigned)

References

Details

User Story

As a Loop Standalone client user
  When I submit my feedback about the call I just had
  Then the User Agent string of the browser I used is attached to the feedback payload object
  So that it's easier to identify particular issues with that browser version

As a Loop Desktop client user
  When I submit my feedback about the call I just had
  Then the Firefox channel, version & platform strings are added to the feedback payload object
  So that it's easier to identify particular issues with that version of Firefox

Attachments

(2 files, 4 obsolete files)

As per spec defined in https://wiki.mozilla.org/Loop/Data_Collection#User_Satisfaction. Note that two different payloads should be sent for desktop client & standalone.
Summary: Loop feedback API client should fill channel, version and platform fields → Loop feedback API client should send browser/platform metadata
Assignee: nobody → nperriault
This is the first part of the work; it adds support for supplementary fields to the FeedbackAPIClient object, and validates them to generate a consistent payload. Conversation router has been updated to add information for platform through the use of navigator.platform, though adding information for channel and version will involve exposing data from gecko through the mozLoop API, which is to be addressed in part 2.
Attachment #8467934 - Flags: review?(standard8)
This should do all that's needed to make the channel, version and OS available from navigator.mozLoop.
Attachment #8468343 - Flags: feedback?(nperriault)
Comment on attachment 8468343 [details] [diff] [review]
Part 2: Extend MozLoopAPI with channel, version and OS details.

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

Looks just fine to me, minor comments.

::: browser/components/loop/MozLoopAPI.jsm
@@ +249,5 @@
> +          version: appInfo.version,
> +          OS: appInfo.OS
> +        };
> +      }
> +    },

Nit: trailing comma (not a big issue as we're in chrome)

::: browser/components/loop/test/mochitest/browser_mozLoop_appVersionInfo.js
@@ +9,5 @@
> +add_task(loadLoopPanel);
> +
> +add_task(function* test_mozLoop_appVersionInfo() {
> +  registerCleanupFunction(function () {
> +    Services.prefs.clearUserPref("loop.test");

Do we need this here?

@@ +12,5 @@
> +  registerCleanupFunction(function () {
> +    Services.prefs.clearUserPref("loop.test");
> +  });
> +
> +  Assert.ok(gMozLoopAPI, "mozLoop should exist");

Isn't this tested in browser_mozLoop_doNotDisturb.js already?
Attachment #8468343 - Flags: feedback?(nperriault) → feedback+
(In reply to Nicolas Perriault (:NiKo`) from comment #4)
> Nit: trailing comma (not a big issue as we're in chrome)

I know Mike prefers this to reduce loc change when we add more items.

> Do we need this here?

Nope, removed.

> > +  Assert.ok(gMozLoopAPI, "mozLoop should exist");
> 
> Isn't this tested in browser_mozLoop_doNotDisturb.js already?

Yes, but I left it in, just as a confirmation, as they can be run separately etc.
Fixed comments, I think we can get this reviewed and do the extra bit in the follow-up.
Attachment #8468343 - Attachment is obsolete: true
Attachment #8469263 - Flags: review?(mdeboer)
Comment on attachment 8469263 [details] [diff] [review]
Part 2: Extend MozLoopAPI with channel, version and OS details.

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +17,5 @@
> +XPCOMUtils.defineLazyGetter(this, "appInfo", function() {
> +  return Cc["@mozilla.org/xre/app-info;1"]
> +           .getService(Ci.nsIXULAppInfo)
> +           .QueryInterface(Ci.nsIXULRuntime);
> +});

this'll bitrot when bug 1000771 lands.

@@ +243,5 @@
> +     */
> +    appVersionInfo: {
> +      enumerable: true,
> +      get: function() {
> +        return {

You can cache this information, as it's not very likely to change during a session ;)

Apart from that, this object will cross chrome-content boundaries, so you'll need to use `Cu.cloneInto(versionInfo, targetWindow);`

::: browser/components/loop/test/mochitest/browser_mozLoop_appVersionInfo.js
@@ +15,5 @@
> +
> +  Assert.ok(appVersionInfo, "should have appVersionInfo");
> +
> +  Assert.equal(appVersionInfo.channel,
> +               Services.prefs.getCharPref("app.update.channel"),

If this is what's going on in UpdateChannel.jsm, why not use fetching this pref instead of loading a module that does the same thing?
Attachment #8469263 - Flags: review?(mdeboer) → feedback+
Rebased previous patch on top of latest fx-team.

Note: Part 2 needs to update conversation.jsx to make the FeedbackAPIClient object to use the new data exposed by the mozLoop API.
Attachment #8467934 - Attachment is obsolete: true
Attachment #8467934 - Flags: review?(standard8)
Attachment #8469363 - Flags: review?(standard8)
Comment on attachment 8469363 [details] [diff] [review]
(Part 1) - Update Loop FeedbackAPIClient to allow attaching more metadata.

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

Looks good, r=Standard8
Attachment #8469363 - Flags: review?(standard8) → review+
Updated for comments, and also extended to set the correct values for the feedback client.
Attachment #8469263 - Attachment is obsolete: true
Attachment #8469979 - Flags: review?(mdeboer)
Comment on attachment 8469979 [details] [diff] [review]
Part 2: Extend Loop feedback details with channel, version and OS details.

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +256,5 @@
> +     */
> +    appVersionInfo: {
> +      enumerable: true,
> +      get: function() {
> +        let defaults = Services.prefs.getDefaultBranch(null);

oops, you forgot to cache this!
Attachment #8469979 - Flags: review?(mdeboer)
Now with the missing caching.
Attachment #8469979 - Attachment is obsolete: true
Attachment #8470097 - Flags: review?(mdeboer)
Comment on attachment 8470097 [details] [diff] [review]
Part 2: Extend MozLoopAPI with channel, version and OS details.

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

Awesome, ship it!
Attachment #8470097 - Flags: review?(mdeboer) → review+
Does this need manual test coverage or is the automation coverage sufficient?
Whiteboard: [qa?]
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #16)
> Does this need manual test coverage or is the automation coverage sufficient?

Mark, can you please answer this?
Flags: qe-verify?
Flags: needinfo?(standard8)
Whiteboard: [qa?]
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #16)
> Does this need manual test coverage or is the automation coverage sufficient?

I believe there's already (or soon to be added) manual testing for feedback, so extending it to ensure that version, channel, and OS are set correctly in the received report would be good.
Flags: needinfo?(standard8)
Flags: qe-verify?
Flags: qe-verify+
Flags: in-moztrap?
Paul, can you please test this to make sure it's working correctly?
Flags: in-moztrap? → needinfo?(paul.silaghi)
QA Contact: paul.silaghi
(In reply to Mark Banner (:standard8) (away until 23rd Oct) from comment #18)
> I believe there's already (or soon to be added) manual testing for feedback,
> so extending it to ensure that version, channel, and OS are set correctly in
> the received report would be good.

After discussing with :pauly we realized that input.mozilla.org wasn't currently displaying any UA information sent to it, preventing this bug to be verified.

I've filed bug 1085344 against the Input product, which is bocking verification here.
Depends on: 1085344
qe- based on comment 20
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(paul.silaghi)
You need to log in before you can comment on or make changes to this bug.