Closed
Bug 1171415
Opened 9 years ago
Closed 9 years ago
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
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(Not tracked)
People
(Reporter: RT, Assigned: andreio)
References
Details
(Whiteboard: [metrics])
User Story
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) See attached file for UX
Attachments
(3 files, 22 obsolete files)
|
23.58 KB,
image/png
|
Details | |
|
1.26 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
|
181.85 KB,
patch
|
Details | Diff | Splinter Review |
NPS.png
No description provided.
|
Reporter | |
Comment 1•9 years ago
|
||
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)
|
|
Reporter | |
Comment 2•9 years ago
|
||
|
|
Reporter | |
Updated•9 years ago
|
User Story: (updated)
|
||
Updated•9 years ago
|
Rank: 19
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [metrics]
|
|
||
Comment 3•9 years ago
|
||
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.
|
||
Updated•9 years ago
|
Flags: needinfo?(standard8)
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → andrei.br92
|
|
Assignee | |
Comment 4•9 years ago
|
||
|
|
Assignee | |
Comment 5•9 years ago
|
||
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?
Attachment #8626682 -
Flags: review?(standard8)
|
|
||
Comment 6•9 years ago
|
||
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.
Flags: needinfo?(standard8)
Attachment #8626682 -
Flags: review?(standard8) → review+
|
|
||
Comment 8•9 years ago
|
||
(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.
|
|
||
Updated•9 years ago
|
Keywords: leave-open
|
||
Comment 9•9 years ago
|
||
|
|
Assignee | |
Comment 10•9 years ago
|
||
|
|
Assignee | |
Comment 11•9 years ago
|
||
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.
Attachment #8627550 -
Flags: review?(standard8)
|
|
||
Comment 12•9 years ago
|
||
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".
Attachment #8627550 -
Flags: review?(standard8) → review?(mdeboer)
|
||
Comment 13•9 years ago
|
||
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?
Attachment #8627550 -
Flags: review?(mdeboer) → feedback-
|
|
Assignee | |
Comment 15•9 years ago
|
||
(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.
Iteration: 42.1 - Jul 13 → ---
|
|
Assignee | |
Comment 16•9 years ago
|
||
Attachment #8627550 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628556 -
Flags: review?(mdeboer)
|
|
||
Comment 17•9 years ago
|
||
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!
Attachment #8628556 -
Flags: review?(mdeboer)
|
|
Assignee | |
Comment 18•9 years ago
|
||
Attachment #8628556 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 19•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628556 -
Attachment is obsolete: false
|
|
Assignee | |
Comment 20•9 years ago
|
||
|
|
Assignee | |
Comment 21•9 years ago
|
||
|
|
Assignee | |
Comment 22•9 years ago
|
||
|
|
Assignee | |
Comment 23•9 years ago
|
||
|
|
Assignee | |
Comment 24•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628976 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628989 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628992 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628994 -
Flags: review?(mdeboer)
|
|
Assignee | |
Comment 25•9 years ago
|
||
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.
Attachment #8628995 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628942 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8628938 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 26•9 years ago
|
||
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.
Flags: needinfo?(mdeboer)
|
|
||
Comment 27•9 years ago
|
||
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.
Attachment #8628976 -
Flags: review?(mdeboer) → review-
|
|
||
Comment 28•9 years ago
|
||
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??
Attachment #8628989 -
Flags: review?(mdeboer) → review-
|
|
||
Comment 29•9 years ago
|
||
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!
Attachment #8628992 -
Flags: review?(mdeboer) → review+
|
|
||
Comment 30•9 years ago
|
||
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.
Attachment #8628994 -
Flags: review?(mdeboer) → review-
|
|
||
Comment 31•9 years ago
|
||
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.
Attachment #8628995 -
Flags: review?(mdeboer) → review-
|
|
||
Updated•9 years ago
|
Flags: needinfo?(mdeboer)
|
|
Assignee | |
Comment 32•9 years ago
|
||
(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!
|
|
||
Updated•9 years ago
|
Iteration: --- → 42.1 - Jul 13
|
|
Assignee | |
Comment 33•9 years ago
|
||
Attachment #8628976 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 34•9 years ago
|
||
Attachment #8630270 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 35•9 years ago
|
||
Attachment #8628989 -
Attachment is obsolete: true
Attachment #8628992 -
Attachment is obsolete: true
Attachment #8628994 -
Attachment is obsolete: true
Attachment #8628995 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 36•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8630274 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8630277 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8630278 -
Flags: review?(mdeboer)
|
|
Assignee | |
Comment 37•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8630274 -
Flags: review?(mdeboer) → review-
|
|
||
Comment 39•9 years ago
|
||
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!
Attachment #8630277 -
Flags: review?(mdeboer) → review-
|
|
||
Comment 40•9 years ago
|
||
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!
Attachment #8630278 -
Flags: review?(mdeboer) → feedback+
|
|
Assignee | |
Comment 41•9 years ago
|
||
Attachment #8630274 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 42•9 years ago
|
||
Attachment #8630277 -
Attachment is obsolete: true
Attachment #8630278 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8630771 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8630772 -
Flags: review?(mdeboer)
|
|
||
Updated•9 years ago
|
Points: --- → 5
|
|
||
Comment 43•9 years ago
|
||
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 :(
Attachment #8630771 -
Flags: review?(mdeboer) → review-
|
|
||
Comment 44•9 years ago
|
||
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.
Attachment #8630772 -
Flags: review?(mdeboer) → review+
|
|
Reporter | |
Comment 45•9 years ago
|
||
Gareth, can you please provide the surveygizmo link for the survey we'll use as discussed?
Flags: needinfo?(garethcull.bugs)
|
|
Reporter | |
Comment 46•9 years ago
|
||
(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.
Flags: needinfo?(standard8)
Flags: needinfo?(andrei.br92)
|
|
||
Comment 47•9 years ago
|
||
(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.
|
|
Reporter | |
Comment 48•9 years ago
|
||
(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•9 years ago
|
||
(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.
Flags: needinfo?(standard8)
|
|
Assignee | |
Comment 50•9 years ago
|
||
In that case I think we should move forward with landing this bug as soon as we have the feedback form URL.
Flags: needinfo?(andrei.br92)
|
|
||
Comment 51•9 years ago
|
||
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•9 years ago
|
||
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
Flags: needinfo?(garethcull.bugs) → needinfo?(frios)
|
|
Assignee | |
Comment 53•9 years ago
|
||
Attachment #8630772 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 54•9 years ago
|
||
Attachment #8633310 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 55•9 years ago
|
||
Attachment #8630771 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8633314 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8633315 -
Flags: review?(mdeboer)
|
|
||
Comment 56•9 years ago
|
||
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.
Attachment #8633315 -
Flags: review?(mdeboer) → review+
|
|
Assignee | |
Comment 57•9 years ago
|
||
Attachment #8633315 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 58•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8633314 -
Attachment is obsolete: true
Attachment #8633314 -
Flags: review?(mdeboer)
|
|
Assignee | |
Comment 59•9 years ago
|
||
Attachment #8628556 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8633570 -
Flags: review?(mdeboer)
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8633572 -
Flags: review?(mdeboer)
|
|
||
Comment 60•9 years ago
|
||
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?
Attachment #8633572 -
Flags: review?(mdeboer) → review-
|
|
||
Comment 61•9 years ago
|
||
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?
Attachment #8633570 -
Flags: review?(mdeboer) → review+
|
|
Assignee | |
Comment 62•9 years ago
|
||
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.
Flags: needinfo?(mdeboer)
|
|
||
Comment 63•9 years ago
|
||
(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.
Flags: needinfo?(mdeboer)
|
|
Assignee | |
Comment 64•9 years ago
|
||
Attachment #8633570 -
Attachment is obsolete: true
Attachment #8633572 -
Attachment is obsolete: true
Attachment #8633574 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•9 years ago
|
Attachment #8634208 -
Flags: review?(mdeboer)
|
|
||
Comment 65•9 years ago
|
||
Comment on attachment 8634208 [details] [diff] [review]
Implement NPS feedback form for Loop
Review of attachment 8634208 [details] [diff] [review]:
-----------------------------------------------------------------
rs=me
Attachment #8634208 -
Flags: review?(mdeboer) → review+
|
|
||
Updated•9 years ago
|
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
|
||
Updated•9 years ago
|
Summary: [meta] 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 → 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
|
|
Assignee | |
Comment 67•9 years ago
|
||
Attachment #8634208 -
Attachment is obsolete: true
|
|
||
Comment 68•9 years ago
|
||
|
|
Assignee | |
Updated•9 years ago
|
Keywords: checkin-needed
|
||
Comment 69•9 years ago
|
||
Keywords: checkin-needed
|
|
||
Comment 71•9 years ago
|
||
We left the leave-open on by mistake, hence removing it.
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
|
||
Comment 72•9 years ago
|
||
Just noting that we are working with l10n on translation for this: http://www.surveygizmo.com/s3/2227372/Firefox-Hello-Product-Survey
Flags: needinfo?(frios)
You need to log in
before you can comment on or make changes to this bug.
Description
•