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

Status: RESOLVED FIXED

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

Comment 1

[Romain Testard [:RT]]

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)

Comment 2

[Romain Testard [:RT]]

Attachment: NPS.png

Comment 3

[:shell escalante]

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 4

[Andrei Oprea [:andreio]]

Attachment: Add feedback string for NPS survey

Comment 5

[Andrei Oprea [:andreio]]

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 6

[Mark Banner (:standard8)]

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.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a07df7553b74

Comment 8

[Mark Banner (:standard8)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a07df7553b74

Comment 10

[Andrei Oprea [:andreio]]

Attachment: Implement new feedback view for desktop loop client

Comment 11

[Andrei Oprea [:andreio]]

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 12

[Mark Banner (:standard8)]

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 13

[Mike de Boer [:mikedeboer]]

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?

Comment 14

[:shell escalante]

assess for uplift to fx41 once done

Comment 15

[Andrei Oprea [:andreio]]

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

[Andrei Oprea [:andreio]]

Attachment: Implement new feedback view for desktop loop client

Comment 17

[Mike de Boer [:mikedeboer]]

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 18

[Andrei Oprea [:andreio]]

Attachment: Add new feedback view for desktop side

Comment 19

[Andrei Oprea [:andreio]]

Attachment: Remove previous feedback view and tests

Comment 20

[Andrei Oprea [:andreio]]

Attachment: Part 1: Add new feedback view for desktop client

Comment 21

[Andrei Oprea [:andreio]]

Attachment: Part 2: Remove previous feedback form from views and tests

Comment 22

[Andrei Oprea [:andreio]]

Attachment: Part 3: Fix test warnigs

Comment 23

[Andrei Oprea [:andreio]]

Attachment: Part 4: Remove old feeback views and tests

Comment 24

[Andrei Oprea [:andreio]]

Attachment: Part 5: Add compiled .js files

Comment 25

[Andrei Oprea [:andreio]]

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.

Comment 26

[Andrei Oprea [:andreio]]

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 27

[Mike de Boer [:mikedeboer]]

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 28

[Mike de Boer [:mikedeboer]]

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 29

[Mike de Boer [:mikedeboer]]

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 30

[Mike de Boer [:mikedeboer]]

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 31

[Mike de Boer [:mikedeboer]]

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.

Comment 32

[Andrei Oprea [:andreio]]

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

Comment 33

[Andrei Oprea [:andreio]]

Attachment: Part 1: Add new feedback view for desktop client

Comment 34

[Andrei Oprea [:andreio]]

Attachment: Part 1: Add new feedback view for desktop client

Comment 35

[Andrei Oprea [:andreio]]

Attachment: Part 2: Remove previous feedback form views and tests

Comment 36

[Andrei Oprea [:andreio]]

Attachment: Part 3: Add tests for new feedback view

Comment 37

[Andrei Oprea [:andreio]]

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 38

[Mike de Boer [:mikedeboer]]

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 39

[Mike de Boer [:mikedeboer]]

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 40

[Mike de Boer [:mikedeboer]]

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 41

[Andrei Oprea [:andreio]]

Attachment: Part 1: Add new feedback view for desktop client

Comment 42

[Andrei Oprea [:andreio]]

Attachment: Part 2: Remove previous feedback form views and tests

Comment 43

[Mike de Boer [:mikedeboer]]

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 44

[Mike de Boer [:mikedeboer]]

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.

Comment 45

[Romain Testard [:RT]]

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

Comment 46

[Romain Testard [:RT]]

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

Comment 47

[Mike de Boer [:mikedeboer]]

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

Comment 48

[Romain Testard [:RT]]

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

Comment 49

[Mark Banner (:standard8)]

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

Comment 50

[Andrei Oprea [:andreio]]

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

Comment 51

[Mark Banner (:standard8)]

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.

Comment 52

[Gareth Cull [:garethc]]

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 53

[Andrei Oprea [:andreio]]

Attachment: Part 2: Remove previous feedback form views and tests

Comment 54

[Andrei Oprea [:andreio]]

Attachment: Part 2: Remove previous feedback form views and tests

Comment 55

[Andrei Oprea [:andreio]]

Attachment: Part 1: Add new feedback view for desktop client

Comment 56

[Mike de Boer [:mikedeboer]]

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 57

[Andrei Oprea [:andreio]]

Attachment: Part 2: Remove previous feedback form views and tests

Comment 58

[Andrei Oprea [:andreio]]

Attachment: Part 1: Add new feedback view for desktop client

Comment 59

[Andrei Oprea [:andreio]]

Attachment: Implement NPS feedback form for Loop

Comment 60

[Mike de Boer [:mikedeboer]]

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 61

[Mike de Boer [:mikedeboer]]

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?

Comment 62

[Andrei Oprea [:andreio]]

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.

Comment 63

[Mike de Boer [:mikedeboer]]

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

[Andrei Oprea [:andreio]]

Attachment: Implement NPS feedback form for Loop

Comment 65

[Mike de Boer [:mikedeboer]]

Comment on attachment 8634208 [details] [diff] [review]
Implement NPS feedback form for Loop

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

rs=me

Comment 66

[Ryan VanderMeulen [:RyanVM]]

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

Comment 67

[Andrei Oprea [:andreio]]

Attachment: Implement NPS feedback form for Loop

Comment 68

[Mark Banner (:standard8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e7fe7b0867a

Comment 69

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c29c62b67e07

Comment 70

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c29c62b67e07

Comment 71

[Mark Banner (:standard8)]

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

Comment 72

[Fabio Rios [:frios]]

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