1079225 - Standalone link clicker UI needs a rooms feedback form

Status: RESOLVED FIXED

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

Description

[Romain Testard [:RT]]

Attachment: feedback form.png

Per 1077257 we need a feedback form allowing users to provide details around any issues they may experience.
We should just leave the text box available all the time and submit its contents when submitting feedback even if the user doesn't select any category.

Comment 1

[:shell escalante]

find the bug like this to dupe...t he one with strings and details in 35+

Comment 2

[Maire Reavy [:mreavy]]

Keeping this one open since I think we want a separate bug for Desktop feedback and Standalone feedback for Rooms.

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Feedback form displayed for Loop standalone rooms, patch v1.

This patch implements the feedback form for standalone users, though also introduces a change in the flow, including for Desktop; basically everytime a user had an actual video conversation with at least one participant (which is our current maximum), the room state transitions to ENDED and renders the feedback form, otherwise switches back to READY. 

Notes: 
- Styling still needs to be done. 
- Tests aren't written just yet, as I wanted to discuss the changes first.

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8528468 [details] [diff] [review]
Feedback form displayed for Loop standalone rooms, patch v1.

This patch isn't good and has too many side effects. I'll take an easier route.

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Feedback form displayed for Loop standalone rooms.

Here's a simpler approach, which is enough to ship imho.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8529134 [details] [diff] [review]
Feedback form displayed for Loop standalone rooms.

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

Looks good, one minor comment. As you're not about, I'll fix it and get it landed.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +375,5 @@
>       */
>      remotePeerDisconnected: function() {
>        // As we only support two users at the moment, we just set this
>        // back to joined.
> +      this.setStoreState({roomState: ROOM_STATES.JOINED});

I guess you were intending to make this ENDED. However, I had a chat to Maire, and we agreed that at the moment we should just do feedback when the user leaves the room, not a remote peer.

So I'll put this back to session connected - even though the remote peer is disconnected, the sdk is still connected.

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Mark Banner (:standard8)]

Attachment: Follow-up to bug 1079225 - Fix formatting of the waiting for media message in Loop rooms, and ensure feedback can be given for multiple conversations in a row.

This fixes a couple of things I just realised were wrong in the previous patch:

- The waiting for media message was formatted in the wrong location in the display (needed wrapping with a div)
- If you completed one call (or room in a conversation), then gave feedback, and then had another call, the second time, you wouldn't be able to give feedback (the store wasn't resetting).

This fixes both of those and updates tests. I've not included a test for checking that we dispatch the action for the Feedback view for calls, as we don't seem to have any coverage for that, and due to the way the code is, testing it would be quite difficult. Also, given that the code could either be rewritten or go away soon, I'm not sure its worth that much effort.

Comment 9

[Adam Roach [:abr]]

Comment on attachment 8529240 [details] [diff] [review]
Follow-up to bug 1079225 - Fix formatting of the waiting for media message in Loop rooms, and ensure feedback can be given for multiple conversations in a row.

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

r+ -- I'd like to see a test for the feedbackStore#feedbackComplete method, and I have a naming nit, but this is otherwise good to go.

::: browser/components/loop/content/shared/js/feedbackStore.js
@@ +97,5 @@
> +    /**
> +     * Resets the store to its initial state as feedback has been completed,
> +     * i.e. ready for the next round of feedback.
> +     */
> +    feedbackComplete: function() {

I don't see a test for this method.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +690,5 @@
>        return nextState.callStatus !== this.state.callStatus;
>      },
>  
>      callStatusSwitcher: function(status) {
> +      this.props.feedbackStore.dispatchAction(new sharedActions.FeedbackComplete());

This seems like an awkward place to put this, given the name of the function. Rename the function to make it clearer that it has this side effect, or refactor to do this dispatch elsewhere.

Comment 10

[Mark Banner (:standard8)]

Attachment: Follow-up to bug 1079225 - Fix formatting of the waiting for media message in Loop rooms, and ensure feedback can be given for multiple conversations in a row.

Updated patch to fix review comments.

Comment 11

[Mark Banner (:standard8)]

Follow-up patch pushed:

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6dc9a2f152c
https://hg.mozilla.org/mozilla-central/rev/ce3eeca4f793

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8529134 [details] [diff] [review]
Feedback form displayed for Loop standalone rooms.

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: This is almost all standalone code (small changes to a few shared files; several of which are just reformatting).  As such, this runs on the server, so mostly this just makes merges easier (especially for the few shared files).

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

[Risks and why]: See above.  Very low risk.

[String/UUID change made/needed]: none.

Note: applies to the followup as well

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8529134 [details] [diff] [review]
Feedback form displayed for Loop standalone rooms.

Approval Request Comment
Transfer request to beta

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/b0f3e2aeb66d
https://hg.mozilla.org/releases/mozilla-beta/rev/8689e7804a1f