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

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Comment 1

[Mark Banner (:standard8)]

Moving to MVP as discussions are suggesting a simpler form via a third-party for MLP.

Comment 2

[:shell escalante]

shell: Look at the UI clicker bug and mark up the same.

Comment 3

[Romain Testard [:RT]]

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.

Comment 4

[Romain Testard [:RT]]

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

[:shell escalante]

If telemetry back-end is done in time - we'll feed into that.  If not we'll use intermediary step now to get data.

Comment 6

[Romain Testard [:RT]]

UX: https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#feedback

Comment 7

[Romain Testard [:RT]]

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

Comment 8

[Maire Reavy [:mreavy]]

Waiting on react stuff to land and also verifying UX flow with Darrin & RT

Comment 9

[Romain Testard [:RT]]

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

Comment 10

[Romain Testard [:RT]]

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.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 12

[Darrin Henein [:darrin]]

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.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 14

[Romain Testard [:RT]]

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.

Comment 15

[Romain Testard [:RT]]

Attachment: Call Ended.mp3

Initial sound file which may change later.

Comment 16

[:shell escalante]

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

Comment 17

[Maire Reavy [:mreavy]]

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.

Comment 18

[Romain Testard [:RT]]

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

Comment 19

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

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?

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 21

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

Attachment: 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.

Comment 22

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

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

Comment 23

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

Attachment: (Part 1): Loop desktop client user feedback form.

Rebased patch on top of latest master plus patches for bug 1000131 and 1042799.

Comment 24

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

Attachment: (Part 1): Loop desktop client user feedback form.

Patch rebased on top of latest master now bug 1042799 has landed.

Comment 25

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

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

Comment 26

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

Comment 27

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

Attachment: (Part 1): Loop desktop client user feedback form.

Addressed most comments, will comment for others.

Comment 28

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

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

Comment 29

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

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.

Comment 30

[Romain Testard [:RT]]

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!

Comment 31

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

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.

Comment 32

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

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.

Comment 33

[Romain Testard [:RT]]

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?

Comment 34

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

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.

Comment 35

[Romain Testard [:RT]]

(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

Comment 36

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

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

Comment 37

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

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

Comment 38

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

Attachment: (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

Comment 39

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

Comment 40

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

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?

Comment 41

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

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

Comment 42

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

Attachment: (Part 1): Loop desktop client user feedback form.

Addressed latest comments.

Comment 43

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

Attachment: (Part 1): Loop desktop client user feedback form.

Addressed latest comments, for real this time. Friday.

Comment 44

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

Attachment: (Part 2): Feedback API client.

Revamped patch for Part 2, now happy feedback is submitted as well.

Comment 45

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

Attachment: (Part 2): Feedback API client.

Patch rebased against latest master.

Comment 46

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

Attachment: (Part 2): Feedback API client.

Self-fixing nits (sorry for the bugnoise).

Comment 47

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

Attachment: 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).

Comment 48

[Mark Banner (:standard8)]

Comment on attachment 8466795 [details] [diff] [review]
(Part 2): Feedback API client.

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

Looks good, r=Standard8

Comment 49

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/84caca21e7cc
https://hg.mozilla.org/integration/fx-team/rev/05bd981cb0a1

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84caca21e7cc
https://hg.mozilla.org/mozilla-central/rev/05bd981cb0a1

Comment 51

[Francesco Lodolo [:flod]]

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.

Comment 52

[Maire Reavy [:mreavy]]

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

Comment 53

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

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

Comment 54

[Francesco Lodolo [:flod]]

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

Comment 55

[Mark Banner (:standard8)]

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

Comment 56

[Paul Silaghi, QA [:pauly]]

Question: Are those scrollbars really needed? http://i.imgur.com/RRUhqDF.png?1

Comment 57

[Mark Banner (:standard8)]

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

Comment 58

[u279076]

Verified fixed in Firefox 33 and 34. Is this sufficiently covered by automation or should we have a manual test?

Comment 59

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

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.

Comment 60

[u279076]

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

Comment 61

[Mark Banner (:standard8)]

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

Comment 62

[u279076]

Thanks for elaborating, Mark. Do you want a smoketest created for this or are in-tree tests sufficient?

Comment 63

[Mark Banner (:standard8)]

We don't have functional tests for this currently, so lets do a smoketest for now. We can convert it to a test later.

Comment 64

[u279076]

Can someone please review the following Moztrap testcase:
https://moztrap.mozilla.org/manage/case/14559/

Comment 65

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

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

Comment 66

[u279076]

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