Rewrite the Feedback View in the flux style

RESOLVED FIXED in Firefox 35

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Unassigned)

Tracking

unspecified
mozilla36
Points:
3
Dependency tree / graph
Bug Flags:
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [tech-debt])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
With the direct calling using the flux pattern, we should move the feedback view to also use a flux pattern.

The basic outline of this would be:

- Create a Feedback Store which contains the state and manages the flow
- Rework the view to only display the state, and not contain any logic.
(Reporter)

Updated

4 years ago
Whiteboard: [tech-debt]

Updated

4 years ago
backlog: --- → Fx36+
(Reporter)

Updated

4 years ago
Blocks: 1064279

Updated

4 years ago
backlog: Fx36+ → Fx37+
Depends on: 1094137
Created attachment 8525477 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v1.

This patch is far bigger than expected, I'm sorry for this.
Attachment #8525477 - Flags: review?(standard8)
Created attachment 8525494 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v1.1.

Rebased patch on top of latest fx-team.
Attachment #8525477 - Attachment is obsolete: true
Attachment #8525477 - Flags: review?(standard8)
Attachment #8525494 - Flags: review?(standard8)
(Reporter)

Comment 3

4 years ago
Comment on attachment 8525494 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v1.1.

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

I'm about 3/4 of the way through this. Just saving the comment below for now, hopefully will get to the rest later today.

::: browser/components/loop/content/shared/js/actions.js
@@ +344,5 @@
> +
> +    /**
> +     * Send feedback data.
> +     */
> +    SendFeedback: Action.define("sendFeedback", {

I'm not quite sure I see the need for separate set feedback details/send feedback actions.

Can you explain a bit more about why they are different?
Created attachment 8526665 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v2.

Adressed the early comment, suppressed the FEEDBACK_STATES.FILLED state and associated action.
Attachment #8525494 - Attachment is obsolete: true
Attachment #8525494 - Flags: review?(standard8)
Attachment #8526665 - Flags: review?(standard8)
(Reporter)

Comment 5

4 years ago
Comment on attachment 8526665 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v2.

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

I'm now down to just finishing off the test files.

::: browser/components/loop/content/shared/js/feedbackStore.js
@@ +53,5 @@
> +    getInitialStoreState: function() {
> +      return {feedbackState: FEEDBACK_STATES.INIT};
> +    },
> +
> +    setFeedbackMood: function(actionData) {

nit: we should jsdoc these functions.

::: browser/components/loop/content/shared/js/feedbackViews.jsx
@@ +241,5 @@
> +      this.listenTo(this.props.feedbackStore, "change", this._onStoreStateChanged);
> +    },
> +
> +    componentDidMount: function() {
> +      this.play("terminated");

For rooms, we'll need the room-leave sound which is in the RoomsAudioMixin. Not sure what to do here. I'm happy for it not to be addressed here and to address it in a desktop patch.
Created attachment 8526702 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v2.1.

Addressed comments.

(In reply to Mark Banner (:standard8) from comment #5)
> Not sure what to do here. I'm happy for it not to be addressed here and to
> address it in a desktop patch.

Yeah, let's address this in bug 1086512.
Attachment #8526665 - Attachment is obsolete: true
Attachment #8526665 - Flags: review?(standard8)
Attachment #8526702 - Flags: review?(standard8)
Comment on attachment 8526702 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v2.1.

Bah, erroneous patch attached. Needs rest.
Attachment #8526702 - Flags: review?(standard8)
Comment on attachment 8526702 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v2.1.

Nope, it was the right one, git-bz has been confusing here (but the remark about rest still applies).
Attachment #8526702 - Flags: review?(standard8)
(Reporter)

Comment 9

4 years ago
Comment on attachment 8526702 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v2.1.

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

Getting there!

::: browser/components/loop/test/shared/feedbackViews_test.js
@@ +9,5 @@
> +var l10n = navigator.mozL10n || document.mozL10n;
> +var TestUtils = React.addons.TestUtils;
> +var sharedViews = loop.shared.views;
> +
> +describe("loop.shared.views.FeedbackView", function() {

The tests here seem to be testing the whole flow (including the store), not just the feedback view.

For example, in one place you're checking that the fakeFeedbackApiClient has send called on it, whereas I'd have expected just testing for the action being generated as per the rest of the flux views that we do.
Created attachment 8527166 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v3.

Simplified the flow even further, updated tests.
Attachment #8526702 - Attachment is obsolete: true
Attachment #8526702 - Flags: review?(standard8)
Attachment #8527166 - Flags: review?(standard8)
(Reporter)

Updated

4 years ago
Assignee: nobody → nperriault
Iteration: --- → 36.3
(Reporter)

Comment 11

4 years ago
Comment on attachment 8527166 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v3.

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

Looks great. r=Standard8, just one minor nit to fix.

::: browser/components/loop/content/shared/js/feedbackStore.js
@@ +80,5 @@
> +
> +      this.setStoreState({feedbackState: FEEDBACK_STATES.PENDING});
> +    },
> +
> +    sendFeedbackError: function(actionData) {

nit: missing jsdoc
Attachment #8527166 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/40eeaa1a8c0d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8527166 [details] [diff] [review]
Moved Loop feedback flow to Flux, patch v3.

Approval Request Comment
[Feature/regressing bug #]: n/a

[User impact if declined]: Needed to allow us to have post-call feedback forms for Rooms (two bugs likely to land on central tomorrow or sooner depend on this refactor).  Rewriting the rooms feedback using the old structure would be riskier than uplifting this rewrite, and the forms are needed

[Describe test coverage new/current, TBPL]: Includes tests

[Risks and why]: See above.

[String/UUID change made/needed]: none
Attachment #8527166 - Flags: approval-mozilla-aurora?
Attachment #8527166 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
status-firefox35: --- → fixed
status-firefox36: --- → fixed
Flags: qe-verify-
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.