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

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

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.

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.

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

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

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

Waiting on react stuff to land and also verifying UX flow with Darrin & 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.

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.

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.

Initial sound file which may change later.

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

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.

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

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.

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.

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

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

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

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

Addressed most comments, will comment for others.

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

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!

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.

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.

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.

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

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

Addressed latest comments.

Addressed latest comments, for real this time. Friday.

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

Patch rebased against latest master.

Self-fixing nits (sorry for the bugnoise).

Updated part 1, rebased on top of master as well to match latest version of part 2 (le sigh).

Comment on attachment 8466795 [details] [diff] [review] (Part 2): Feedback API client. Review of attachment 8466795 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=Standard8

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

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

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.

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

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

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!

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

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.

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

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.