1048785 - Loop feedback API client should send browser/platform metadata

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Description

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: (Part 1) - Update Loop FeedbackAPIClient to allow attaching more metadata.

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.

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Forgot to mention: current patch sits on top of pending patch for bug 1048834 (https://bug1048834.bugzilla.mozilla.org/attachment.cgi?id=8467772)

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 2: Extend MozLoopAPI with channel, version and OS details.

This should do all that's needed to make the channel, version and OS available from navigator.mozLoop.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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?

Comment 5

[Mark Banner (:standard8)]

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

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 2: Extend MozLoopAPI with channel, version and OS details.

Fixed comments, I think we can get this reviewed and do the extra bit in the follow-up.

Comment 7

[Mike de Boer [:mikedeboer]]

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?

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: (Part 1) - Update Loop FeedbackAPIClient to allow attaching more metadata.

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.

Comment 9

[Mark Banner (: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

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 2: Extend Loop feedback details with channel, version and OS details.

Updated for comments, and also extended to set the correct values for the feedback client.

Comment 11

[Mike de Boer [:mikedeboer]]

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!

Comment 12

[Mark Banner (:standard8)]

Attachment: Part 2: Extend MozLoopAPI with channel, version and OS details.

Now with the missing caching.

Comment 13

[Mike de Boer [:mikedeboer]]

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!

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/eea2155f85dd
https://hg.mozilla.org/integration/fx-team/rev/05d5db0c856e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eea2155f85dd
https://hg.mozilla.org/mozilla-central/rev/05d5db0c856e

Comment 16

[u279076]

Does this need manual test coverage or is the automation coverage sufficient?

Comment 17

[u279076]

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

Comment 18

[Mark Banner (:standard8)]

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

Comment 19

[u279076]

Paul, can you please test this to make sure it's working correctly?

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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

Comment 21

[Paul Silaghi, QA [:pauly]]

qe- based on comment 20