Desktop client needs to provide simple feedback at the end of a call.

VERIFIED FIXED in Firefox 34

Status

Hello (Loop)
Client
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: RT, Assigned: NiKo)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox34 verified)

Details

(Whiteboard: p=1)

User Story

As a FF browser user, I get prompted to provide feedback at the end of a call so that I can let the product owner know how I feel about it.

Attachments

(3 attachments, 10 obsolete attachments)

155.43 KB, audio/mpeg
Details
66.35 KB, patch
standard8
: review+
Details | Diff | Splinter Review
145.14 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Moving to MVP as discussions are suggesting a simpler form via a third-party for MLP.
Blocks: 971986
No longer blocks: 972866
Component: General → Client
(Reporter)

Updated

4 years ago
Blocks: 972030
(Reporter)

Updated

4 years ago
No longer blocks: 972030

Updated

4 years ago
No longer blocks: 972014

Updated

4 years ago
Blocks: 972014
No longer blocks: 971986
(Reporter)

Updated

4 years ago
User Story: (updated)

Updated

4 years ago
Priority: -- → P1
Target Milestone: --- → mozilla32

Updated

4 years ago
Whiteboard: [s=ui32]
(Reporter)

Updated

4 years ago
Blocks: 1012743
(Reporter)

Updated

4 years ago
No longer blocks: 972014

Updated

4 years ago
Priority: P1 → P2
Whiteboard: [s=ui32] → p=?
Target Milestone: mozilla32 → mozilla33

Updated

4 years ago
Summary: Desktop client needs UI to provide simple feedback at the end of a call. → Desktop client needs to provide simple feedback at the end of a call.

Comment 2

3 years ago
shell: Look at the UI clicker bug and mark up the same.
Flags: needinfo?(sescalante)
(Reporter)

Comment 3

3 years ago
Copying comment from Bug 1003180

We want an experience integrated into the client UX so that at the end of a call the conversation window prompts the user to provide feedback. This is optional for the user.
I discussed with Darrin who will provide UX.

The UX I proposed:
Screen with Happy or Sad displayed at the end of the call in the conversation window
    If happy clicked, thank the user for the feedback and close conversation window
    If Sad clicked display another screen: "What makes you sad? []Bad audio [] Bad Video []Got disconnected []Horrible UI []Other with text field

This although requires server back-end to store the data and I'd like to understand if this is technically complex or not. We will require this back-end also for the mobile application which can't nicely display a Google form on mobile form factor.

If this is likely to take time to implement this, an intermediary step could be to display a link to a Google form at the end of the call in the convresation window, when the user clicks it a new tab with the form opens. An example of the form is: 
https://docs.google.com/forms/d/1KSr1Zy5SupSxXk-6JE2x3pzRulrd4wo6jbcgCvC3rC0/viewform

I needinfo Mark for comments on whether we have a simple way to store that data which should help understand if we need the intermediate Google form step.
(Reporter)

Updated

3 years ago
Priority: P2 → P1
(Reporter)

Updated

3 years ago
Depends on: 1024568
(Reporter)

Comment 4

3 years ago
Feedback from Darrin through e-mail:

I think one thing I would add is that we should consider the wording of the feedback options. Madhava and I were looking at them and thought something simpler like “What makes you sad?” and the options:

    Audio Quality
    Video Quality
    Was Disconnected
    Confusing
    Other (w/ text field)

I don’t think users are going to know what UI means (let alone Horrible UI!), or how/whether it was the source of their poor experience.

Comment 5

3 years ago
If telemetry back-end is done in time - we'll feed into that.  If not we'll use intermediary step now to get data.
Flags: needinfo?(sescalante)
Whiteboard: p=? → p=1
Blocks: 1027062

Updated

3 years ago
Blocks: 1003180
Assignee: nobody → rgauthier
(Reporter)

Comment 6

3 years ago
UX: https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback
(Reporter)

Comment 7

3 years ago
Please note that this bug does not include the "Report user" functionality shown in the UX (separate bug).
(Reporter)

Updated

3 years ago
Blocks: 1036879
(Reporter)

Updated

3 years ago
No longer blocks: 1003180
Waiting on react stuff to land and also verifying UX flow with Darrin & RT
(Reporter)

Comment 9

3 years ago
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #8)
> Waiting on react stuff to land and also verifying UX flow with Darrin & RT

The UX flow is available on https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback ("report user" part should be discarded for now)
You can click the happy and sad faces to see how the UI reacts to user actions. The UX for this part is complete now - let me know if further clarifications are needed.
(Reporter)

Comment 10

3 years ago
After discussions with Darrin, we'll skip the call terminated screen (https://bugzilla.mozilla.org/show_bug.cgi?id=1000197) and implement only this user feedback screen with a call back button along side the report user button. 

This is pending UX amendment from Darrin.
Note that we don't currently have the architecture set up to support callbacks from the desktop client to the standalone client, so this button doesn't really make sense as part of this bug.  

I think we could spin up a new bug to do that, but it's not clear to me that it makes sense to do before the standalone client starts using the websocket protocol.
One UI note: in this case we have decided not to use the UI from the visual design spec (the fox happy/sad faces) but instead use the normal circular happy/sad faces as shown in https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/

Please let me know if clarification is needed.
Note that what I was trying to (unclearly) say is that both sides should be providing feedback, but the desktop side (this bug) shouldn't have the "call back" button, because that won't (at the moment) work.
(Reporter)

Updated

3 years ago
Blocks: 1039959
(Reporter)

Comment 14

3 years ago
Dan you're right - my mistake.
The "call-back" button only applies to the link clicker feedback form for iteration 1 - https://bugzilla.mozilla.org/show_bug.cgi?id=974873

So agreed that for this bug we should just implement as originally planned https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback and ignore the 2 buttons at the bottom.

"Call-back" button will be implemented once we support direct calling in the client or when we handle call-back URLs in the client - I have create a bug to track this https://bugzilla.mozilla.org/show_bug.cgi?id=1039959

"Report" will be implemented when we have decided on underlying mechanisms for this.

Updated

3 years ago
Target Milestone: mozilla33 → 33 Sprint 3- 7/21
(Reporter)

Comment 15

3 years ago
Created attachment 8458611 [details]
Call Ended.mp3

Initial sound file which may change later.
(Reporter)

Updated

3 years ago
Depends on: 1041439

Comment 16

3 years ago
darrin added feedback form - which is also call terminated view https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback

Updated

3 years ago
Assignee: rgauthier → nperriault
Target Milestone: 33 Sprint 3- 7/21 → 34 Sprint 1- 8/4
RT -- Niko is implementing this now and needs to know where he can find the right form fields for user feedback.  We see a high-level list in https://bugzilla.mozilla.org/show_bug.cgi?id=1003180#c17 .  Is there a more detailed list for the implementation?   Is it somewhere in Darrin's UX mock ups?  Thanks.
Flags: needinfo?(rtestard)
(Reporter)

Comment 18

3 years ago
If you go to https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback
You can click the sad face and the list appears.

It is the same list as on the bug above mentioned:
[] Audio quality
[] Video quality 
[] Was disconnected 
[] Confusing
[] Other with text field

Please note we want to know also if the user clicked "happy" or "sad". More details are only required if they clicked "sad".

Romain
Flags: needinfo?(rtestard)
Thanks RT.

So ongoing work is viewable here https://github.com/n1k0/gecko-dev/commit/e58c8c56de994209837edcd6a685db659fb0553e and the patch is getting BIG (and tests still to be written);

Would it make sense to submit a first part for this patch for review? Once reviewed I'd be implementing the form submission, with request sent to input.moz. Thoughts?
If there's an easy way to chunk up the patch so that the first chunk 

a) would not be displayed to users just yet
b) would have tests completed for it

That would be an awesome way forward.  I'm totally open to discussing other ways to land it piecemeal, and I bet others would be too.
Created attachment 8462896 [details] [diff] [review]
0001-Bug-972992-Part-1-Loop-desktop-client-user-feedback-.patch

This is the first part of the patch adding a new FeedbackView at the end of a call for Desktop users. It needs bug XXX and associated patch 1042799 to land first.

Please note that feedback form data are not actually sent over the wire for now — that will be addressed in part 2.

Also, the "call ended" sound will be added in part 3.
Attachment #8462896 - Flags: review?(dmose)
For next parts (posting feedback data to input.moz), it seems like the Input API [1] doesn't currently allow posting custom fields, while it's required here; hence marking at least 1041664 as a dependency here.


[1]: http://fjord.readthedocs.org/en/latest/api.html#posting-product-feedback-api-v1-feedback
Depends on: 1041664
Created attachment 8463905 [details] [diff] [review]
(Part 1): Loop desktop client user feedback form.

Rebased patch on top of latest master plus patches for bug 1000131 and 1042799.
Attachment #8462896 - Attachment is obsolete: true
Attachment #8462896 - Flags: review?(dmose)
Attachment #8463905 - Flags: review?(standard8)
Created attachment 8464582 [details] [diff] [review]
(Part 1): Loop desktop client user feedback form.

Patch rebased on top of latest master now bug 1042799 has landed.
Attachment #8463905 - Attachment is obsolete: true
Attachment #8463905 - Flags: review?(standard8)
Attachment #8464582 - Flags: review?(standard8)
Created attachment 8465025 [details] [diff] [review]
(Part 2): Feedback API client.

This is the second part of the feature implementation, introducing the FeedbackApiClient, so the whole thing works.

Note: I've introduced a loop.feedback.baseUrl pref to store the input server endpoint to use, but discovered that a
app.feedback.baseURL global preference was already available; though it's not accessible using navigator.mozLoop.getLoopCharPref, 
as this one isn't prefixed with "loop.".

I think we could live with a dupe temporarily until we find a proper way to access global prefs in Loop.
Attachment #8465025 - Flags: review?(standard8)
Comment on attachment 8464582 [details] [diff] [review]
(Part 1): Loop desktop client user feedback form.

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

In general this looks good, but I think there's a few things that need addressing.

::: browser/components/loop/content/js/conversation.jsx
@@ +254,5 @@
>       */
> +    feedback: function() {
> +      // XXX pass in the feedback sender object
> +      this.loadReactComponent(sharedViews.FeedbackView({
> +        // XXX for now we pass in a fake feeback API client (see bug 972992)

These two XXX comments seem like duplicates. Can we just have one with the bug ref?

::: browser/components/loop/content/shared/js/views.jsx
@@ +356,5 @@
> +   * Feedback outer layout.
> +   */
> +  var FeedbackLayout = React.createClass({
> +    propTypes: {
> +      sendFeedback: React.PropTypes.func,

sendFeedback isn't used in this class, but title is. I'm a little surprised this isn't failing.

Also, children is a used props as well.

@@ +357,5 @@
> +   */
> +  var FeedbackLayout = React.createClass({
> +    propTypes: {
> +      sendFeedback: React.PropTypes.func,
> +      reset:        React.PropTypes.func

I think it would be worth documenting that if reset isn't specified, then the back button isn't shown. That wasn't obvious to me until I was looking at some test things.

@@ +464,5 @@
> +   * Feedback received view.
> +   */
> +  var FeedbackReceived = React.createClass({
> +    getInitialState: function() {
> +      return {countdown: 5};

In the UX specs, it implies a shorter, 2-second countdown for close window in the success case, that's probably slightly better, as 5 seems a bit long.

I think the number should also be a constant near the top of the file.

@@ +497,5 @@
> +      feedbackApiClient: React.PropTypes.object.isRequired
> +    },
> +
> +    getInitialState: function() {
> +      return {step: this.props.step};

Step really needs documenting in the class description or somewhere. Without going through the whole class, it is difficult to see what it is used for, and the possible values.

@@ +518,5 @@
> +    },
> +
> +    sendFeedback: function(fields) {
> +      this.props.feedbackApiClient.send(fields, this._onFeedbackSent);
> +      // XXX we could possibly add a new step state value for pending submission

I think given the 5 second count down, we're fine not to have a step for pending submission, as long as we're guaranteed to have sent it by then.

If you think there's an issue we need fixed, please file a bug and reference it here, or remove the XXX.

@@ +524,5 @@
> +
> +    _onFeedbackSent: function(err) {
> +      if (err) {
> +        // XXX better end user error reporting, needs spec
> +        alert("Unable to send user feedback");

Lets just log a console error for now, modal dialogs are painful.

Also, please file a bug on considering what to do with handling this case and reference the bug in the comment.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +256,1 @@
>          // XXX When the call is ended gracefully, we should check that we

As you're in the area, please could you file a bug on adding the test for this XXX and add the bug number here?

::: browser/components/loop/test/shared/views_test.js
@@ +416,5 @@
> +        var happyFace = comp.getDOMNode().querySelector(".face-happy");
> +
> +        TestUtils.Simulate.click(happyFace);
> +
> +        expect(comp.getDOMNode().querySelectorAll(".info").length).eql(1);

It isn't entirely obvious that something with a class of ".info" is what we need to prove this test is complete, e.g. we could have got here by incorrectly displaying the sad feedback page with the back button.

There's probably a combination needed of improving the class name and checking the back button isn't displayed.

@@ +460,5 @@
> +          target: {checked: true}
> +        });
> +      TestUtils.Simulate.submit(comp.getDOMNode().querySelector("form"));
> +
> +      expect(comp.state.step).eql("finished");

This seems in conflict with the first test, where you're checking for visibility of something.

Also, we can get to finished from happy, so although unlikely, I'm not sure this is the right test result to check for.
Attachment #8464582 - Flags: review?(standard8) → review-
Depends on: 1046738
Created attachment 8465417 [details] [diff] [review]
(Part 1): Loop desktop client user feedback form.

Addressed most comments, will comment for others.
Attachment #8464582 - Attachment is obsolete: true
Attachment #8465417 - Flags: review?(standard8)
(In reply to Mark Banner (:standard8) from comment #26)
> These two XXX comments seem like duplicates. Can we just have one with the
> bug ref?

While this is fixed in part 2, I removed it anyway.

> sendFeedback isn't used in this class, but title is. I'm a little surprised
> this isn't failing.

Fixed.

> Also, children is a used props as well.

Well, children is a default prop available in every React component (though it can be null); so I've now explicitely required a single child to be present.

> I think it would be worth documenting that if reset isn't specified, then
> the back button isn't shown. That wasn't obvious to me until I was looking
> at some test things.

Well, the test for this prop to check for display the button is four lines above. Added a comment anyway.

> In the UX specs, it implies a shorter, 2-second countdown for close window
> in the success case, that's probably slightly better, as 5 seems a bit long.

I don't see the same thing here https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback

Click on the sad face, then the form submit button, it says "This window will close in 5 seconds"…

Screenshot http://cl.ly/image/1s3z1F3e1k10/Screen%20Shot%202014-07-31%20at%2015.05.20.png

Also, 2 seconds might not be enough for some people.

> I think the number should also be a constant near the top of the file.

Fixed, added WINDOW_AUTOCLOSE_TIMEOUT_IN_SECONDS (as a var though because this is shared code).

> > +    getInitialState: function() {
> > +      return {step: this.props.step};
> 
> Step really needs documenting in the class description or somewhere

Fixed, also added a PropTypes check.

> > +    sendFeedback: function(fields) {
> > +      this.props.feedbackApiClient.send(fields, this._onFeedbackSent);
> > +      // XXX we could possibly add a new step state value for pending submission
> 
> I think given the 5 second count down, we're fine not to have a step for
> pending submission, as long as we're guaranteed to have sent it by then.

I've added a pending boolean flag to state to prevent multiple form submissions by disabling the submit button after first submission (tests added as well).

> Lets just log a console error for now, modal dialogs are painful.

Fixed.

> Also, please file a bug on considering what to do with handling this case
> and reference the bug in the comment.

Filed bug 1046738 to track this.

> As you're in the area, please could you file a bug on adding the test for
> this XXX and add the bug number here?

Sure, see bug 1046744.

> > +        expect(comp.getDOMNode().querySelectorAll(".info").length).eql(1);
> 
> It isn't entirely obvious that something with a class of ".info" is what we
> need to prove this test is complete, e.g. we could have got here by
> incorrectly displaying the sad feedback page with the back button.
> 
> There's probably a combination needed of improving the class name and
> checking the back button isn't displayed.

Fixed, added a new .thank-you class and added a test to check for presence of the back button.

> Also, we can get to finished from happy, so although unlikely, I'm not sure
> this is the right test result to check for.

Fixed, now testing for the .thank-you class.
So since I've attached patch for Part 2, support for a new category field has been deployed https://bugzilla.mozilla.org/show_bug.cgi?id=1003180#c30

I'll update the patch tomorrow so we'll use this. 

Also, latest tests against input.allizom.org show that neither "Loop" nor "Hello" are considered valid product names atm.

Poking :RT here.
Flags: needinfo?(rtestard)
(Reporter)

Comment 30

3 years ago
Not sure here.
I NI Will  - can you confirm if a "loop" product has been added and how we can see the result from our data submission on  input.allizom.org?
Thanks!
Flags: needinfo?(rtestard) → needinfo?(willkg)
No one asked me to create a product, yet, so I haven't created one. I'll do that now.

Per our phone conversations, there's no way for you to see any results for data submission unless you want to make the results of Hello/Loop responses public. If you want to make them public, then they'll be visible on the front page dashboard. If you don't want to make them public, then we'll have to figure out something else, but it'll probably involve asking me or one of the User Advocacy people to query data for you.
Flags: needinfo?(willkg)
I added the "Hello" product to the dashboard. Use "Hello" in the product field.

I tested this against Input stage (input.allizom.org) and noticed it's not connecting to rabbitmq, so it's erroring out now.

My vote is use production (input.mozilla.org). After we're done testing things, we can wipe out existing test feedback for the Hello product.
(Reporter)

Comment 33

3 years ago
Thanks.
If we make it public initially will we be able to make it private later?
If yes then let's start with public so we're agile whilst in dev mode. Then we can work-out a process to get the data -- probably once we get to Beta. 
Does that sound good?
People are definitely watching Input. So if we make this public, people will see it. If you don't mind that, I can make it public for now and then switch it to private when you tell me to.

We talked about getting the data on the call. You and Cheng were going to work that out, but I thought he promised monthly rollups or CSV downloads something like that. You should talk to him and Matt about that side of things.
(Reporter)

Updated

3 years ago
No longer depends on: 1046738
(Reporter)

Comment 35

3 years ago
(In reply to Will Kahn-Greene [:willkg] from comment #34)
> People are definitely watching Input. So if we make this public, people will
> see it. If you don't mind that, I can make it public for now and then switch
> it to private when you tell me to.
> 
OK let's do that. Although is-it possible to change the name to "Loop" as I am hearing from marketing that the "Hello" brand should not be visible until we are on Beta?
> We talked about getting the data on the call. You and Cheng were going to
> work that out, but I thought he promised monthly rollups or CSV downloads
> something like that. You should talk to him and Matt about that side of
> things.
OK, I'll talk to them - thanks
For the records, it's been decided we'll just silent errors when sending feedback data fails: https://bugzilla.mozilla.org/show_bug.cgi?id=1046738#c6
(In reply to Romain Testard [:RT] from comment #35)
> (In reply to Will Kahn-Greene [:willkg] from comment #34)
> > People are definitely watching Input. So if we make this public, people will
> > see it. If you don't mind that, I can make it public for now and then switch
> > it to private when you tell me to.
>
> OK let's do that. Although is-it possible to change the name to "Loop" as I
> am hearing from marketing that the "Hello" brand should not be visible until
> we are on Beta?

I changed it back to "Loop" and made the product public. All "Loop" data will be public until we switch it to private. Use "Loop" in the product field.
Blocks: 1047406
Blocks: 1047410
Created attachment 8466216 [details] [diff] [review]
(Part 2): Feedback API client.

Refreshed patch for Part 2:

- we now use the new "category" field added to the input.mozilla.org API 
- the conversation window is now updated after a call has ended
Attachment #8465025 - Attachment is obsolete: true
Attachment #8465025 - Flags: review?(standard8)
Attachment #8466216 - Flags: review?(standard8)
Comment on attachment 8465417 [details] [diff] [review]
(Part 1): Loop desktop client user feedback form.

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

Looks good, just a few nits remaining.

::: browser/components/loop/content/shared/js/views.jsx
@@ +356,5 @@
>    /**
> +   * Feedback outer layout.
> +   * 
> +   * Props:
> +   * - 

nit: please check this file generally for trailing whitespace / white space on black lines, there's several locations.

@@ +546,5 @@
> +    },
> +
> +    _onFeedbackSent: function(err) {
> +      if (err) {
> +        // XXX better end user error reporting, needs spec

Need to xref the bug here.

::: browser/components/loop/test/shared/views_test.js
@@ +429,5 @@
> +        TestUtils.Simulate.click(sadFace);
> +
> +        expect(comp.getDOMNode().querySelectorAll("form").length).eql(1);
> +      });
> +    

nit: whitespace

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +50,5 @@
> +feedback_submit_button=Submit
> +feedback_back_button=Back
> +## LOCALIZATION NOTE (feedback_window_will_close_in): In this item, don't
> +## translate the part between {{..}}
> +feedback_window_will_close_in=This window will close in {{countdown}} seconds

I'm going to note it here (I should have noticed this before), this should really use plural forms.

However, our content infra doesn't expose that possibility currently. I don't think we should block landing this, so lets just raise a bug and deal with it in a follow-up.
Attachment #8465417 - Flags: review?(standard8) → review+
Comment on attachment 8466216 [details] [diff] [review]
(Part 2): Feedback API client.

+loop.FeedbackAPIClient = (function($) {
+  "use strict";
+
+  /**
+   * Feedback API client. Sends feedback data to an input.mozilla.com compatible

Nit: It's "input.mozilla.ORG".


+      // description is filled with the reason by default
+      var description = fields.reason;
+
+      // append custom text provided by the user to description, if any
+      if (fields.reason === "other" && fields.custom) {
+        description += ": " + fields.custom;
+      }
+
+      return {
+        happy:       false,     // We only submit feedback from unhappy users
+        product:     this._product,
+        category:    fields.reason,

This seems like you're putting the reason in the description and also in the category fields. That's fine, but seems funny. You might end up with skewed data when doing text analysis on the description field.

What does fields.reason look like? Is it an english string like "UI is confusing" or is it a short value like "confusing"? If the latter, maybe put the english string in the description field and use the value in the category field?
(In reply to Will Kahn-Greene [:willkg] from comment #40)
> This seems like you're putting the reason in the description and also in the
> category fields.

Yeah, this is to always have something in the description field, while you can't leave the field empty.

> You might end up with skewed data when doing text analysis on the description field.

Do you have an example in mind?

> What does fields.reason look like? 

It's an identifier, possible values are audio_quality, video_quality, disconnected, confusing, other.

> If the latter, maybe put
> the english string in the description field and use the value in the
> category field?

Hmm, I could use the form field labels, good idea. Thanks for the feedback!
Created attachment 8466274 [details] [diff] [review]
(Part 1): Loop desktop client user feedback form.

Addressed latest comments.
Attachment #8465417 - Attachment is obsolete: true
Attachment #8466274 - Flags: review?(standard8)
Created attachment 8466275 [details] [diff] [review]
(Part 1): Loop desktop client user feedback form.

Addressed latest comments, for real this time. Friday.
Attachment #8466274 - Attachment is obsolete: true
Attachment #8466274 - Flags: review?(standard8)
Attachment #8466275 - Flags: review?(standard8)
Created attachment 8466436 [details] [diff] [review]
(Part 2): Feedback API client.

Revamped patch for Part 2, now happy feedback is submitted as well.
Attachment #8466216 - Attachment is obsolete: true
Attachment #8466216 - Flags: review?(standard8)
Attachment #8466436 - Flags: review?(standard8)
Created attachment 8466780 [details] [diff] [review]
(Part 2): Feedback API client.

Patch rebased against latest master.
Attachment #8466436 - Attachment is obsolete: true
Attachment #8466436 - Flags: review?(standard8)
Attachment #8466780 - Flags: review?(standard8)
Created attachment 8466795 [details] [diff] [review]
(Part 2): Feedback API client.

Self-fixing nits (sorry for the bugnoise).
Attachment #8466780 - Attachment is obsolete: true
Attachment #8466780 - Flags: review?(standard8)
Attachment #8466795 - Flags: review?(standard8)
Created attachment 8466972 [details] [diff] [review]
0001-Bug-972992-Part-1-Loop-desktop-client-user-feedback-.patch

Updated part 1, rebased on top of master as well to match latest version of part 2 (le sigh).
Attachment #8466275 - Attachment is obsolete: true
Attachment #8466275 - Flags: review?(standard8)
Attachment #8466972 - Flags: review?(standard8)
Attachment #8466972 - Flags: review?(standard8) → review+
Comment on attachment 8466795 [details] [diff] [review]
(Part 2): Feedback API client.

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

Looks good, r=Standard8
Attachment #8466795 - Flags: review?(standard8) → review+
https://hg.mozilla.org/integration/fx-team/rev/84caca21e7cc
https://hg.mozilla.org/integration/fx-team/rev/05bd981cb0a1
Target Milestone: 34 Sprint 1- 8/4 → mozilla33
Target Milestone: mozilla33 → mozilla34
https://hg.mozilla.org/mozilla-central/rev/84caca21e7cc
https://hg.mozilla.org/mozilla-central/rev/05bd981cb0a1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Blocks: 974873
Do we support plural forms in Loop .properties files? Because this one is going to be a pain to localize

feedback_window_will_close_in=This window will close in {{countdown}} seconds

It's also broken for "1 seconds" in English.
(In reply to Francesco Lodolo [:flod] from comment #51)
> Do we support plural forms in Loop .properties files? Because this one is
> going to be a pain to localize
> 
> feedback_window_will_close_in=This window will close in {{countdown}} seconds
> 
> It's also broken for "1 seconds" in English.

Francesco -- Can you file a new bug for these issues so they don't get missed?  Thanks.
Flags: needinfo?(francesco.lodolo)
(In reply to Francesco Lodolo [:flod] from comment #51)
> Do we support plural forms in Loop .properties files? Because this one is
> going to be a pain to localize
> 
> feedback_window_will_close_in=This window will close in {{countdown}} seconds
> 
> It's also broken for "1 seconds" in English.

It really depends on bug 986983, we can't do much except ugly hacks in the meanwhile…
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #52)
> Francesco -- Can you file a new bug for these issues so they don't get
> missed?  Thanks.

Sure, I'll set the dependency to this bug as well.

(In reply to Nicolas Perriault (:NiKo`) from comment #53)
> It really depends on bug 986983, we can't do much except ugly hacks in the
> meanwhile…

Right bug? Because that's a Pontoon (tool) bug, that has nothing to do with webl10n. Gaia has had plural forms for a lot of times, it all depends on what version of l10n.js you're using.
Flags: needinfo?(francesco.lodolo)
Lets get the new bug filed, and continue the discussion there. I did mention plural forms in comment 39, obviously forgot to file the bug :-(
Depends on: 1048882
Blocks: 1003180
Depends on: 1049565
Question: Are those scrollbars really needed? http://i.imgur.com/RRUhqDF.png?1
(In reply to Paul Silaghi, QA [:pauly] from comment #56)
> Question: Are those scrollbars really needed?
> http://i.imgur.com/RRUhqDF.png?1

Looks like a problem specific to windows (Mac is fine). Please can you file a follow-up bug? Thanks!
Depends on: 1050276
Verified fixed in Firefox 33 and 34. Is this sufficiently covered by automation or should we have a manual test?
Status: RESOLVED → VERIFIED
QA Contact: anthony.s.hughes
Whiteboard: p=1 → p=1 [qa+]
I'd also like to have instructions on how to do a manual test so that I can run through them if I ever have to make changes to the API handling code in Input.
(In reply to Will Kahn-Greene [:willkg] from comment #59)
> I'd also like to have instructions on how to do a manual test so that I can
> run through them if I ever have to make changes to the API handling code in
> Input.

Hi Will, I think the steps are:

1. Click "invite someone to talk" button
2. Click "copy"
3. Have someone load your URL and start a call
4. Accept the incoming call and chat for a few minutes
5. End the call
> You should see a happy and sad face inside the chat panel at the bottom of the window
> Clicking happy or sad should say "Thank you for your feedback" and the panel should close after 5 seconds

There's probably an additional verification level to confirm the feedback is being sent properly but I'm not sure how to check that.
Flags: qe-verify+
Whiteboard: p=1 [qa+] → p=1
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #60)
> (In reply to Will Kahn-Greene [:willkg] from comment #59)
> > I'd also like to have instructions on how to do a manual test so that I can
> > run through them if I ever have to make changes to the API handling code in
> > Input.
> 
> Hi Will, I think the steps are:
> 
> 1. Click "invite someone to talk" button
> 2. Click "copy"
> 3. Have someone load your URL and start a call
> 4. Accept the incoming call and chat for a few minutes
> 5. End the call

You can also initiate the call yourself i.e. both ends on the same computer (although, iirc Linux has issues with sharing audio/video devices across more than one process).

> > You should see a happy and sad face inside the chat panel at the bottom of the window
> > Clicking happy or sad should say "Thank you for your feedback" and the panel should close after 5 seconds
> 
> There's probably an additional verification level to confirm the feedback is
> being sent properly but I'm not sure how to check that.

The best way to do this is to set the pref "loop.feedback.baseUrl" to "https://input.allizom.org/api/v1/feedback"

You can then watch the output on https://input.allizom.org/?product=Loop

This is preferred for testing, so it doesn't get in the way of real data.
Thanks for elaborating, Mark. Do you want a smoketest created for this or are in-tree tests sufficient?
We don't have functional tests for this currently, so lets do a smoketest for now. We can convert it to a test later.
Depends on: 1060459
Can someone please review the following Moztrap testcase:
https://moztrap.mozilla.org/manage/case/14559/
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #64)
> Can someone please review the following Moztrap testcase:
> https://moztrap.mozilla.org/manage/case/14559/

Looks good, but I think it misses the case where you enter a custom feedback message.
(In reply to Nicolas Perriault (:NiKo`) from comment #65)
> (In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #64)
> > Can someone please review the following Moztrap testcase:
> > https://moztrap.mozilla.org/manage/case/14559/
> 
> Looks good, but I think it misses the case where you enter a custom feedback
> message.

Sorry I missed that. I'll make sure it gets added.
Depends on: 1088884
status-firefox34: --- → verified
You need to log in before you can comment on or make changes to this bug.