Closed Bug 1076754 Opened 10 years ago Closed 9 years ago

Rewrite the Feedback View in the flux style

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
3

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed
backlog Fx37+

People

(Reporter: standard8, Unassigned)

References

Details

(Whiteboard: [tech-debt])

Attachments

(1 file, 4 obsolete files)

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.
Whiteboard: [tech-debt]
backlog: --- → Fx36+
Blocks: 1064279
backlog: Fx36+ → Fx37+
This patch is far bigger than expected, I'm sorry for this.
Attachment #8525477 - Flags: review?(standard8)
Rebased patch on top of latest fx-team.
Attachment #8525477 - Attachment is obsolete: true
Attachment #8525477 - Flags: review?(standard8)
Attachment #8525494 - Flags: review?(standard8)
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?
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)
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.
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)
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.
Simplified the flow even further, updated tests.
Attachment #8526702 - Attachment is obsolete: true
Attachment #8526702 - Flags: review?(standard8)
Attachment #8527166 - Flags: review?(standard8)
Assignee: nobody → nperriault
Iteration: --- → 36.3
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+
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+
Flags: qe-verify-
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.