As a desktop client user I want to fill-in an NPS survey every 6 months so that I can express what I feel about Hello Acceptance criteria: - Standalone link clicker UI - Remove prompt to provide feedback at the end of a call. The user gets directed to the "Join conversation page" when he leaves a conversation (this is where the user is currently taken to after submitting feedback) - Desktop client UI - Remove prompt to provide feedback at the end of a call. The conversation window closes when he leaves a conversation (this is where the user is currently taken to after submitting feedback) - At the end of a succesful conversation, if the desktop client user has not entered the NPS survey (clicked the blue button) in the last 6 months, he gets prompted to leave feedback through a "leave feedback" button. The user can then select to not provide feedback (close conversation window) or provide feedback (select blue "leave feedback" button). Selecting the blue "leave feedback" button will open a new tab to the URL containing the survey (hosted on surveygizmo, URL will be provided soon)

likely better to redirect to URL we own to keep consistent URL (even if change survey). Mark is going to ask if we can keep on same server on loop-client.

Comment on attachment 8626682 [details] [diff] [review] Add feedback string for NPS survey Should the removal of the current feedback form view & strings be in a separate patch from the introduction of the new feedback view?

Comment on attachment 8626682 [details] [diff] [review] Add feedback string for NPS survey Lets go with this. Hopefully we'll get the UI in real soon.

(In reply to Pulsebot from comment #7) > https://hg.mozilla.org/integration/fx-team/rev/a07df7553b74 Note to L10n: we landed this ahead of time as we expect the UI will likely land before the merges on Monday, but given the travel weekend, we wanted to be extra sure wrt l10n.

Comment on attachment 8627550 [details] [diff] [review] Implement new feedback view for desktop loop client This implements the new feedback view for desktop client & adds the tests for it. Still needed: - Removing the old view and tests. - Mock shows a title string which was not added.

Comment on attachment 8627550 [details] [diff] [review] Implement new feedback view for desktop loop client Review of attachment 8627550 [details] [diff] [review]: ----------------------------------------------------------------- Passing review to Mike to spread out load a bit. ::: browser/components/loop/content/js/roomViews.jsx @@ +737,5 @@ > + * > + * @returns {boolean} > + * @private > + */ > + _shouldRenderFeedbackView: function() { I think this might still want to check if the room has been "used" or not, i.e. if two people actually joined the room. I'm not sure its worth prompting for the survey if they've only created a room - maybe it is - please check with RT/sevaan as to what they want. The "used" state is in activeRoomStore. ::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx @@ +75,3 @@ > case ROOM_STATES.INIT: > case ROOM_STATES.READY: { > // XXX: In ENDED state, we should rather display the feedback form. Should definitely drop the XXX now! I thought a bit about the flows and I can't think of an issue with doing the ended state here (rather than hooking something up to go back to "init".

Comment on attachment 8627550 [details] [diff] [review] Implement new feedback view for desktop loop client Review of attachment 8627550 [details] [diff] [review]: ----------------------------------------------------------------- I'm sorry, Andrei, but there's still a _lot_ to do here... If you don't understand why this is an f-, please contact me directly. Thanks! ::: browser/app/profile/firefox.js @@ +1755,5 @@ > pref("loop.debug.dispatcher", false); > pref("loop.debug.websocket", false); > pref("loop.debug.sdk", false); > pref("loop.debug.twoWayMediaTelemetry", false); > +pref("loop.feedback.timestamp", "0"); Please use a more canonical name for this pref, for example: `loop.feedback.dateLastSeenMs` ::: browser/components/loop/content/conversation.html @@ -38,5 @@ > <script type="text/javascript" src="loop/shared/js/fxOSActiveRoomStore.js"></script> > <script type="text/javascript" src="loop/shared/js/activeRoomStore.js"></script> > <script type="text/javascript" src="loop/shared/js/feedbackStore.js"></script> > <script type="text/javascript" src="loop/shared/js/views.js"></script> > - <script type="text/javascript" src="loop/shared/js/feedbackViews.js"></script> I think you wanted to do an `hg rename` (or `git mv`), because I'm not seeing the removed files. ::: browser/components/loop/content/js/feedbackViews.jsx @@ +12,5 @@ > + openURL: React.PropTypes.func.isRequired > + }, > + > + onFeedbackButtonClick: function() { > + this.props.openURL("http://mozilla.com"); Please keep passing in the `mozLoop` object as dependency, not just the functions you need. @@ +20,5 @@ > + render: function() { > + return ( > + <div className="feedback-view-container"> > + <div className="feedback-hello-logo" /> > + <div className="feedback-button-container"> I *think* we have a shared button and buttongroup views that make sure that the button is styled properly and stretched to full width. You can also use flexbox without using the buttongroup, which is probably cleaner. @@ +22,5 @@ > + <div className="feedback-view-container"> > + <div className="feedback-hello-logo" /> > + <div className="feedback-button-container"> > + <button onClick={this.onFeedbackButtonClick} > + ref="feedbackFormBtn"> You're not using this ref. Please remove it. ::: browser/components/loop/content/js/roomViews.jsx @@ +591,5 @@ > sharedMixins.WindowCloseMixin > ], > > + statics: { > + feedbackPeriod: 15770000 * 1000 // 6 months Please move this to a pref. @@ +728,5 @@ > + * @private > + */ > + _setFeedbackTimestamp: function() { > + this.props.mozLoop.setLoopPref("feedback.timestamp", > + (new Date()).toISOString()); why an ISO string? I'd prefer a UNIX timestamp here. @@ +809,5 @@ > case ROOM_STATES.ENDED: { > + if (shouldRenderFeedbackView) { > + return ( > + <feedbackViews.FeedbackView > + onAfterFeedbackReceived={this.closeWindow} neat, but I'd prefer to have a `handleFeedbackReceived` function on this component and call `this.closeWindow()` in there. ::: browser/components/loop/content/shared/css/conversation.css @@ +427,5 @@ > + background-image: url("../img/happy.png"); > + background-position: center; > + background-size: contain; > + background-repeat: no-repeat; > + order: 0; `order: 0` is implicit. Please remove it. @@ +429,5 @@ > + background-size: contain; > + background-repeat: no-repeat; > + order: 0; > + flex: 2 1 auto; > + align-self: auto; implicit? @@ +435,5 @@ > + margin: 30px 0; > +} > + > +.feedback-button-container { > + order: 0; implicit. @@ +441,5 @@ > + margin: 30px; > + align-self: center; > +} > + > +.feedback-button-container button { This is too custom, and you're kind of re-inventing the wheel here. The shade of blue throughout Hello, for example, is already defined in other places and should be re-used. See my comment in feedbackView.jsx. ::: browser/components/loop/jar.mn @@ +11,5 @@ > content/browser/loop/libs/l10n.js (content/libs/l10n.js) > > # Desktop script > content/browser/loop/js/client.js (content/js/client.js) > + content/browser/loop/js/feedbackViews.js (content/js/feedbackViews.js) You should also remove the old file from the jar. The build system didn't complain, because you didn't do a proper rename operation. When we'd land this patch as-is, however, there'd be build failures. ;) ::: browser/components/loop/test/desktop-local/index.html @@ +57,5 @@ > <script src="../../content/shared/js/views.js"></script> > <script src="../../content/shared/js/textChatStore.js"></script> > <script src="../../content/shared/js/textChatView.js"></script> > <script src="../../content/shared/js/feedbackViews.js"></script> > + <script src="../../content/js/feedbackViews.js"></script> ...and the old ones are still lingering around :/ ::: browser/components/loop/test/desktop-local/roomViews_test.js @@ +8,5 @@ > var TestUtils = React.addons.TestUtils; > var sharedActions = loop.shared.actions; > var sharedUtils = loop.shared.utils; > var sharedViews = loop.shared.views; > + var sharedMixins = loop.shared.mixins; Why do you need this now? @@ +338,5 @@ > + props = _.extend({ > + dispatcher: dispatcher, > + roomStore: roomStore, > + mozLoop: fakeMozLoop > + }, props); I like this pattern, please keep it. However, you don't really need it here (See below). @@ +517,5 @@ > loop.roomViews.DesktopRoomConversationView); > }); > > + it("should render the FeedbackView if roomState is `ENDED` and feedback" + > + "timestamp is older than 6 months", function() { Please don't break off test name strings. @@ +528,5 @@ > > + view = mountTestComponent({ > + mozLoop: { > + getLoopPref: stub, > + setLoopPref: sinon.stub() 1. Please add `setLoopPref: sinon.stub()` in the very first `beforeEach` in this test file. 2. Instead of passing in a different mozLoop object here, do the following: ```js fakeMozLoop.getLoopPref = sinon.stub().returns(Date.now() - 15770000 * 1000); // ... view = mountTestComponent(); ``` @@ +540,5 @@ > + it("should set new feedback.timestamp when FeedbackView is rendered", > + function() { > + var date = new Date(new Date() - 15770000 * 1000); > + var getStub = sinon.stub().returns(date.toISOString()); > + var setStub = sinon.stub(); Same here. @@ +559,4 @@ > }); > > + it("should not render the feedback view if feedback timestamp is less " + > + "than 6 months", function() { Stop the breaking, please. @@ +589,5 @@ > + setLoopPref: sinon.stub() > + } > + }); > + var closeWindowStub = sandbox.stub(view, "closeWindow"); > + view.setState({foo: "bar"}); Please put spaces around object literal braces: `{ foo: "bar" }` ::: browser/components/loop/ui/index.html @@ +60,2 @@ > <script src="../content/shared/js/views.js"></script> > <script src="../content/shared/js/feedbackViews.js"></script> Shouldn't this one be moved/ removed too? ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +295,5 @@ > feedback_rejoin_button=Rejoin > ## LOCALIZATION NOTE (feedback_report_user_button): Used to report a user in the case of > ## an abusive user. > feedback_report_user_button=Report User > +feedback_request_button=Leave Feedback This already landed, no?

assess for uplift to fx41 once done

(In reply to Mark Banner (:standard8) from comment #12) > Comment on attachment 8627550 [details] [diff] [review] > Implement new feedback view for desktop loop client > > Review of attachment 8627550 [details] [diff] [review]: > ----------------------------------------------------------------- > > Passing review to Mike to spread out load a bit. > > ::: browser/components/loop/content/js/roomViews.jsx > @@ +737,5 @@ > > + * > > + * @returns {boolean} > > + * @private > > + */ > > + _shouldRenderFeedbackView: function() { > > I think this might still want to check if the room has been "used" or not, > i.e. if two people actually joined the room. I'm not sure its worth > prompting for the survey if they've only created a room - maybe it is - > please check with RT/sevaan as to what they want. > > The "used" state is in activeRoomStore. > That will never happen because leaving a room before a conversation has started is done via the `leaveRoom` method which (when the room is not used) will just call closeWindow and not show any feedback form.

Comment on attachment 8628556 [details] [diff] [review] Implement new feedback view for desktop loop client This hasn't happened to me before, but this patch is way too large for me to review in one go. Please split this up into multiple parts, grouped by theme. Sorry about the delay this might cause, but it's also a nice thing to learn to think as a reviewer, no? Looking forward to the multiple patches!

Comment on attachment 8628995 [details] [diff] [review] Part 5: Add compiled .js files This is just the compiled files and do not really need a review.

I tried splitting the patch into logical parts. I cannot make any guarantees that each individual patch will work on its own. I would refer back to the original large patch for a working version.

Comment on attachment 8628976 [details] [diff] [review] Part 1: Add new feedback view for desktop client Review of attachment 8628976 [details] [diff] [review]: ----------------------------------------------------------------- General comment: You forgot to add the generated .js files here. ::: browser/app/profile/firefox.js @@ +1757,5 @@ > pref("loop.debug.websocket", false); > pref("loop.debug.sdk", false); > pref("loop.debug.twoWayMediaTelemetry", false); > +pref("loop.feedback.dateLastSeenSec", 0); > +pref("loop.feedback.period", 15770000); // 6 months nit: missing dot. @@ +1758,5 @@ > pref("loop.debug.sdk", false); > pref("loop.debug.twoWayMediaTelemetry", false); > +pref("loop.feedback.dateLastSeenSec", 0); > +pref("loop.feedback.period", 15770000); // 6 months > +pref("loop.feedback.formURL", "http://mozilla.org"); nit: I know this is a placeholder, but let' commit this with `https://` at least ;) ::: browser/components/loop/content/js/conversationViews.jsx @@ +776,5 @@ > + _shouldRenderFeedbackView: function() { > + var delta; > + var timestamp = this.props.mozLoop.getLoopPref("feedback.dateLastSeenSec") * 1000; > + var feedbackPeriod = this.props.mozLoop > + .getLoopPref("feedback.period") * 1000; I don't know, but it seems to me that for each time `render()` gets called, it fetches two prefs via mozLoop. Please move these two pref fetches to a `getInitialState` function. Or `componentWillMount`, whatever seems more appropriate to you. @@ +783,5 @@ > + if (timestamp === 0) { > + return true; > + } > + > + delta = (new Date()) - (new Date(timestamp)); Why not do `var delta = ...` here? + I don't think the additional braces are needed around `new Date()`. ::: browser/components/loop/content/js/feedbackViews.jsx @@ +27,5 @@ > + this.handleFeedbackReceived(); > + }, > + > + handleFeedbackReceived: function() { > + this.props.onAfterFeedbackReceived(); You might as well collapse this into the function above. ::: browser/components/loop/content/js/roomViews.jsx @@ +736,5 @@ > + */ > + _shouldRenderFeedbackView: function() { > + var delta; > + var timestamp = this.props.mozLoop.getLoopPref("feedback.dateLastSeenSec") * 1000; > + var feedbackPeriod = this.props.mozLoop.getLoopPref("feedback.period") * 1000; Same comment as conversationViews.jsx, really. @@ +744,5 @@ > + if (timestamp === 0) { > + return true; > + } > + > + delta = (new Date()) - (new Date(timestamp)); ditto.

Comment on attachment 8628989 [details] [diff] [review] Part 2: Remove previous feedback form from views and tests Review of attachment 8628989 [details] [diff] [review]: ----------------------------------------------------------------- This is close, but you need to add the generated files here too and once you address my comments below too, it'll be r+. ::: browser/components/loop/test/desktop-local/feedbackViews_test.js @@ +22,5 @@ > + }); > + > + describe("FeedbackView", function() { > + var openURLStub, getLoopPrefStub, feedbackReceivedStub, > + fakeURL = "fake.form", mozLoop; Each var declaration on its own line, please ;) @@ +45,5 @@ > + }; > + }); > + > + it("should render a feedback view", function() { > + var component = mountTestComponent(); What we usually do is declare `view = mountTestComponent();` and 1) declare `var view;` above with the rest of the variables. 2) add an afterEach() where you do `view = null` and restore the sandbox. @@ +48,5 @@ > + it("should render a feedback view", function() { > + var component = mountTestComponent(); > + > + //expect(component.getDOMNode().querySelector(".feedback-hello-logo")) > + //.to.not.eql(null); Forgot to remove this? ::: browser/components/loop/test/mochitest/browser.ini @@ -19,5 @@ > [browser_mozLoop_context.js] > [browser_mozLoop_prefs.js] > [browser_mozLoop_doNotDisturb.js] > skip-if = buildapp == 'mulet' > -[browser_mozLoop_pluralStrings.js] Why remove this??

Comment on attachment 8628992 [details] [diff] [review] Part 3: Fix test warnigs Review of attachment 8628992 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, this might race with my patch in bug 1171940, but we'll see!

Comment on attachment 8628994 [details] [diff] [review] Part 4: Remove old feeback views and tests Review of attachment 8628994 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/test/mochitest/browser_mozLoop_pluralStrings.js @@ -15,5 @@ > -add_task(function* test_mozLoop_pluralStrings() { > - Assert.ok(gMozLoopAPI, "mozLoop should exist"); > - > - var strings = JSON.parse(gMozLoopAPI.getStrings("feedback_window_will_close_in2")); > - Assert.equal(gMozLoopAPI.getPluralForm(0, strings.textContent), I understand that this functionality is tested with feedback specific strings, but that doesn't mean that the feature is not used anymore now that you removed the views. Please find other strings to replace these with and keep the test.

Comment on attachment 8628995 [details] [diff] [review] Part 5: Add compiled .js files Review of attachment 8628995 [details] [diff] [review]: ----------------------------------------------------------------- Ah, here they are! Please add them to the patches they belong to, so I can review them side-by-side.

(In reply to Mike de Boer [:mikedeboer] from comment #27) > I don't know, but it seems to me that for each time `render()` gets called, > it fetches two prefs via mozLoop. > Please move these two pref fetches to a `getInitialState` function. Or > `componentWillMount`, whatever seems more appropriate to you. Thanks for cathing this!

Just noticed one of the patch was submitted with TODOs for the method description. I'll fix this when I will re-upload the patch as just 1 file.

Comment on attachment 8630274 [details] [diff] [review] Part 1: Add new feedback view for desktop client Review of attachment 8630274 [details] [diff] [review]: ----------------------------------------------------------------- This is getting very close! I'm glad we talked about this yesterday. :-) ::: browser/components/loop/content/js/conversation.jsx @@ +42,5 @@ > + * > + * @private > + */ > + _setFeedbackTimestamp: function() { > + var timestamp = (new Date()).getTime() / 1000; This'll yield a fractional, so you'll probably want to `Math.floor()` this. Again, no need to put additional braces around the `new Date()` struct. @@ +62,5 @@ > + > + /** > + * We only show the feedback for once every 6 months. > + * > + * @returns {boolean} No it doesn't. @@ +65,5 @@ > + * > + * @returns {boolean} > + * @private > + */ > + onCallTerminated: function() { Please rename this function to `handleCallTerminated`. The property name should stay `onCallTerminated`. It's grown into a convention of ours to uniquely identify handler member functions with `handle`. @@ +66,5 @@ > + * @returns {boolean} > + * @private > + */ > + onCallTerminated: function() { > + var delta = new Date() - (new Date(this.state.feedbackTimestamp)); Please move this down to where you use it. @@ +68,5 @@ > + */ > + onCallTerminated: function() { > + var delta = new Date() - (new Date(this.state.feedbackTimestamp)); > + > + this.play("terminated"); AFAICT this sound is only played for direct calls. Can you leave it there, please? @@ +75,5 @@ > + // 0 is default value for pref. Always show feedback form on first use. > + if (this.state.feedbackTimestamp === 0 || > + delta >= this.state.feedbackPeriod) { > + this._setFeedbackTimestamp(); > + return this._switchToFeedbackForm(); Please replace the above two lines with: ```js this.props.dispatcher.dispatch(new sharedActions.ShowFeedbackForm()); return; ``` ...and remove the two functions from this component. The implementation of `_setFeedbackTimestamp` will move to the `showFeedbackForm` action handler in the conversationAppStore. @@ +85,2 @@ > render: function() { > + if (this.state.showFeedbackForm === true) { `if (this.state.showFeedbackForm) {` will do. ::: browser/components/loop/content/js/conversationAppStore.js @@ +37,5 @@ > + getInitialStoreState: function() { > + return { > + // How often to display the form. Convert seconds to ms. > + feedbackPeriod: this._mozLoop.getLoopPref("feedback.period") * 1000, > + // Date last seen the timestamp. Convert seconds to ms. nit: 'Date when the feedback form was last presented.' @@ +64,5 @@ > this.trigger("change"); > }, > > /** > + * TODO: add desc You might as well, before you submit a patch for review ;) @@ +67,5 @@ > /** > + * TODO: add desc > + */ > + showFeedbackForm: function() { > + this.setStoreState({ To state the obvious: You'll be setting the pref here as well! Rule of thumb: the more logic you can move to a store, the better. ::: browser/components/loop/content/js/conversationViews.jsx @@ +634,5 @@ > > + /** > + * XXX Will be implemented in 1176162. Added in to remove warnings in the > + * console. > + */ nit: please try to focus you work on this single bug, otherwise all kinds of stuff will gradually sneak in. @@ +759,5 @@ > + componentDidUpdate: function(prevProps, prevState) { > + // Handle timestamp and window closing only when the call has terminated. > + if (prevState.callState === CALL_STATES.ONGOING && > + this.state.callState === CALL_STATES.FINISHED) { > + this.props.onCallTerminated(); Please add a prop for this. See roomViews.jsx. ::: browser/components/loop/content/js/roomViews.jsx @@ +720,5 @@ > + componentDidUpdate: function(prevProps, prevState) { > + // Handle timestamp and window closing only when the call has terminated. > + if (prevState.roomState === ROOM_STATES.ENDED && > + this.state.roomState === ROOM_STATES.ENDED) { > + this.props.onCallTerminated(); Well, erm, if you introduce this prop, you should add it to the propTypes section, no? Please make it required and update the tests accordingly.

Comment on attachment 8630277 [details] [diff] [review] Part 2: Remove previous feedback form views and tests Review of attachment 8630277 [details] [diff] [review]: ----------------------------------------------------------------- All the other removals look right to me. This is going to be a nice cleanup! ::: browser/components/loop/test/mochitest/browser.ini @@ +19,5 @@ > [browser_mozLoop_context.js] > [browser_mozLoop_prefs.js] > [browser_mozLoop_doNotDisturb.js] > skip-if = buildapp == 'mulet' > +[browser_mozLoop_feedbackSettings.js] I don't understand this change... ? ::: browser/components/loop/test/mochitest/browser_mozLoop_feedbackSettings.js @@ +11,5 @@ > Components.utils.import("resource://gre/modules/Promise.jsm", this); > > add_task(loadLoopPanel); > > +add_task(function* test_mozLoop_feedbackSettings() { I'm sorry, but this is not what you're supposed to do here. Please change the plural forms to use _other_ strings than `feedback_window_will_close_in2`. The tests is all about `getPluralForm` and has _nothing_ to do with the feedback form in particular. We use `getPluralForm` in many other areas, so please keep the tests in-tact!

Comment on attachment 8630278 [details] [diff] [review] Part 3: Add tests for new feedback view Review of attachment 8630278 [details] [diff] [review]: ----------------------------------------------------------------- Let's follow your own idea: create a new test file for this new component - especially since the component itself is inside another file than views.jsx!

Comment on attachment 8630771 [details] [diff] [review] Part 1: Add new feedback view for desktop client Review of attachment 8630771 [details] [diff] [review]: ----------------------------------------------------------------- r- just for the icon issue, which I'm sure we'll be able to resolve today. Can you fix the nits in the meantime too? Thanks! I'm very happy with the clean end result for conversation.jsx! ::: browser/components/loop/content/js/conversation.jsx @@ +46,5 @@ > + > + /** > + * We only show the feedback for once every 6 months, otherwise close > + * the window. > + * nit: superfluous newline. ::: browser/components/loop/jar.mn @@ +31,5 @@ > content/browser/loop/shared/css/common.css (content/shared/css/common.css) > content/browser/loop/shared/css/conversation.css (content/shared/css/conversation.css) > > # Shared images > + content/browser/loop/shared/img/helloicon.png (content/shared/img/helloicon.png) Oh, I missed this! Was this here before? Can you move all this to the next patch? If a new image needs to be added, why not an SVG? ::: browser/components/loop/test/desktop-local/conversationAppStore_test.js @@ +120,5 @@ > + > + sinon.assert.calledOnce(setLoopPrefStub); > + sinon.assert.calledWithExactly(setLoopPrefStub, > + "feedback.dateLastSeenSec", > + 1); Please put this on one line: ```js sinon.assert.calledWithExactly(setLoopPrefStub, "feedback.dateLastSeenSec", 1); ``` ::: browser/components/loop/test/desktop-local/conversationViews_test.js @@ +777,5 @@ > + > + view = mountTestComponent({ > + callState: CALL_STATES.FINISHED > + }); > + // Force a state change so that it triggers componentDidUpdate nit: missing dot. ::: browser/components/loop/test/desktop-local/conversation_test.js @@ +231,5 @@ > + > + TestUtils.findRenderedComponentWithType(ccView, FeedbackView); > + }); > + > + it("should dispatch a ShowFeedbackForm action if timestamp is 0", Please fix the indentation here and move the whole block below back. ::: browser/components/loop/test/desktop-local/feedbackViews_test.js @@ +79,5 @@ > + }); > + > + it("should fetch the feedback form URL from the prefs", function() { > + mozLoop.getLoopPref = sinon.stub().withArgs("feedback.formURL") > + .returns(fakeURL); nit: indent this with one level. ::: browser/components/loop/test/desktop-local/index.html @@ -46,5 @@ > </script> > > <!-- App scripts --> > <script src="../../content/shared/js/utils.js"></script> > - <script src="../../content/shared/js/feedbackApiClient.js"></script> You're mixing up patches a bit :(

Comment on attachment 8630772 [details] [diff] [review] Part 2: Remove previous feedback form views and tests Review of attachment 8630772 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Thanks.

Gareth, can you please provide the surveygizmo link for the survey we'll use as discussed?

(In reply to :shell escalante from comment #3) > likely better to redirect to URL we own to keep consistent URL (even if > change survey). Mark is going to ask if we can keep on same server on > loop-client. Did you guys discuss this? Ideally we'd like the flexibility to not hard-code the surveygizmo URL. Is this possible given what's been done? Also would be cool to understand how the redirect can be managed.

(In reply to Romain Testard [:RT] from comment #46) > Did you guys discuss this? Ideally we'd like the flexibility to not > hard-code the surveygizmo URL. > Is this possible given what's been done? Also would be cool to understand > how the redirect can be managed. We're not hard-coding anything, the URL is set as a pref value. This can be changed to anything we want during the 12 weeks this is riding the trains.

(In reply to Mike de Boer [:mikedeboer] from comment #47) > (In reply to Romain Testard [:RT] from comment #46) > > Did you guys discuss this? Ideally we'd like the flexibility to not > > hard-code the surveygizmo URL. > > Is this possible given what's been done? Also would be cool to understand > > how the redirect can be managed. > > We're not hard-coding anything, the URL is set as a pref value. This can be > changed to anything we want during the 12 weeks this is riding the trains. OK thanks - any idea who could help us regarding getting a URL that we'd use to redirect to a given survey at a given time so we have maximum flexibility there? Do I need to raise a separate bug?

(In reply to Romain Testard [:RT] from comment #48) > OK thanks - any idea who could help us regarding getting a URL that we'd use > to redirect to a given survey at a given time so we have maximum flexibility > there? Do I need to raise a separate bug? I can have that discussion when I'm back - its incorporated in some other questions.

In that case I think we should move forward with landing this bug as soon as we have the feedback form URL.

Andrei: maybe we could land it with a small tweak - i.e. don't show the form/store the date if the pref is empty. That way we still get the clean up landed and it doesn't bitrot too much.

Here is the link to the NPS survey: http://www.surveygizmo.com/s3/2227372/Firefox-Hello-Product-Survey Fabio, are you able to get this translated and I'll work in survey gizmo to update the translations once that is ready? I'll email you a pdf you can send off. Thanks. Gareth

Comment on attachment 8633315 [details] [diff] [review] Part 1: Add new feedback view for desktop client Review of attachment 8633315 [details] [diff] [review]: ----------------------------------------------------------------- Code-wise, this looks really good! :) r=me with comments addressed. ::: browser/components/loop/content/js/conversation.jsx @@ +12,4 @@ > var CallControllerView = loop.conversationViews.CallControllerView; > var DesktopRoomConversationView = loop.roomViews.DesktopRoomConversationView; > var GenericFailureView = loop.conversationViews.GenericFailureView; > + var FeedbackView = loop.feedbackViews.FeedbackView; nit: can you keep these declarations in alphabetical order? ::: browser/components/loop/content/js/feedbackViews.jsx @@ +43,5 @@ > + > + return { > + FeedbackView: FeedbackView > + }; > + nit: superfluous newline. ::: browser/components/loop/content/shared/img/helloicon.svg @@ +1,2 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<!-- Generator: Adobe Illustrator 19.0.0, SVG Export Plug-In . SVG Version: 6.00 Build 0) --> Please remove this comment. @@ +1,4 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<!-- Generator: Adobe Illustrator 19.0.0, SVG Export Plug-In . SVG Version: 6.00 Build 0) --> > +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" > + viewBox="0 0 64 64" enable-background="new 0 0 64 64" xml:space="preserve"> nit: awkward indentation. @@ +2,5 @@ > +<!-- Generator: Adobe Illustrator 19.0.0, SVG Export Plug-In . SVG Version: 6.00 Build 0) --> > +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" > + viewBox="0 0 64 64" enable-background="new 0 0 64 64" xml:space="preserve"> > +<path fill-rule="evenodd" clip-rule="evenodd" fill="#4E92DF" d="M32,0C14.3,0,0,12.6,0,28.1c0,7.7,3.6,14.7,9.3,19.8 > + c-1,3.5-3,8.3-6.9,12.9c0.7,1.2,11.7-3,19.4-6.1c3.2,0.9,6.6,1.5,10.2,1.5c17.7,0,32-12.6,32-28.1S49.7,0,32,0z M41.6,16.9 nit: weird indentation. ::: browser/components/loop/test/desktop-local/conversationAppStore_test.js @@ +112,5 @@ > + sinon.assert.calledOnce(showFeedbackFormStub); > + }); > + > + it("should set feedback timestamp on ShowFeedbackForm action", > + function() { nit: I think this `function() {` still fits on the one line? ::: browser/components/loop/test/desktop-local/conversation_test.js @@ +12,2 @@ > var sharedModels = loop.shared.models, > + fakeWindow, sandbox, getLoopPrefStub, setLoopPrefStub, mozL10nGet; nit: please make this it's own `var` declaration for the uninitialized variables here.

Comment on attachment 8633572 [details] [diff] [review] Part 1: Add new feedback view for desktop client Review of attachment 8633572 [details] [diff] [review]: ----------------------------------------------------------------- The two issues as listed below block the landing of this patch. I'm eager to hear how this came to be and to review an updated patch this evening. ::: browser/components/loop/content/shared/img/helloicon.svg @@ +1,1 @@ > +<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 64 64"><path fill-rule="evenodd" clip-rule="evenodd" fill="#4E92DF" d="M32 0C14.3 0 0 12.6 0 28.1c0 7.7 3.6 14.7 9.3 19.8-1 3.5-3 8.3-6.9 12.9.7 1.2 11.7-3 19.4-6.1 3.2.9 6.6 1.5 10.2 1.5 17.7 0 32-12.6 32-28.1S49.7 0 32 0zm9.6 16.9c2.3 0 4.2 1.9 4.2 4.2 0 2.3-1.9 4.2-4.2 4.2-2.3 0-4.2-1.9-4.2-4.2-.1-2.3 1.8-4.2 4.2-4.2zm-19.3 0c2.3 0 4.2 1.9 4.2 4.2 0 2.3-1.9 4.2-4.2 4.2-2.3 0-4.2-1.9-4.2-4.2-.1-2.3 1.8-4.2 4.2-4.2zM32 47.7h-.1-.1c-8.6 0-18.1-5.5-20.3-14.9 5.8 2.7 13.8 3.8 20.4 3.8 6.6 0 14.7-1.2 20.4-3.8-2.2 9.3-11.7 14.9-20.3 14.9z"/></svg> \ No newline at end of file Hmm, this sure looks compressed, but would be wrong for use in the SVG maps like icons-10x10.svg and family. Just something to keep in mind, that's all. ::: browser/components/loop/jar.mn @@ +31,5 @@ > content/browser/loop/shared/css/common.css (content/shared/css/common.css) > content/browser/loop/shared/css/conversation.css (content/shared/css/conversation.css) > > # Shared images > + content/browser/loop/shared/img/helloicon.png (content/shared/img/helloicon.png) You might want to s/helloicon.png/helloicon.svg/. Otherwise it 1) won't compile, because the png does not exist - did you test this?!?! 2) look wrong, because there's only whitespace in the new feedback form. ::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx @@ +111,5 @@ > <a className="btn btn-info" href={loop.config.downloadFirefoxUrl}> > {mozL10n.get("rooms_room_full_call_to_action_nonFx_label", { > brandShortname: mozL10n.get("brandShortname") > + case ROOM_STATES.ENDED: > + case ROOM_STATES.INIT: How on earth did this end up here?

Comment on attachment 8633570 [details] [diff] [review] Part 2: Remove previous feedback form views and tests Review of attachment 8633570 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the change below carried over to the previous patch. ::: browser/components/loop/standalone/content/js/standaloneRoomViews.jsx @@ +111,5 @@ > > render: function() { > switch(this.props.roomState) { > + case ROOM_STATES.ENDED: > + case ROOM_STATES.INIT: Ah, here they are... strange... I expected them in the right place in patch 1 - can you see why?

With 18 obsolete patches to this bug I feel I wasted more time splitting the diff into two patches then actually doing the work. The patches don't work individually because it's replacing one view with the other so my proposal is to go forward with just one patch.

(In reply to Andrei Oprea [:andreio] from comment #62) > With 18 obsolete patches to this bug I feel I wasted more time splitting the > diff into two patches then actually doing the work. The patches don't work > individually because it's replacing one view with the other so my proposal > is to go forward with just one patch. I hope this didn't demotivate you too much. I hope you do understand that everything in one single patch is a bit overwhelming, no? So, r=me on the big patch with the jar.mn issue fixed.

Comment on attachment 8634208 [details] [diff] [review] Implement NPS feedback form for Loop Review of attachment 8634208 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Do you have a Try link handy for this? :)

We left the leave-open on by mistake, hence removing it.

Just noting that we are working with l10n on translation for this: http://www.surveygizmo.com/s3/2227372/Firefox-Hello-Product-Survey