Standalone link clicker UI needs a rooms feedback form

RESOLVED FIXED in Firefox 35

Status

Hello (Loop)
Client
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: RT, Assigned: NiKo)

Tracking

unspecified
mozilla36
Points:
2
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [rooms])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8501018 [details]
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.
(Reporter)

Updated

3 years ago
No longer blocks: 1077257

Comment 1

3 years ago
find the bug like this to dupe...t he one with strings and details in 35+
backlog: --- → -
Flags: needinfo?(sescalante)
Target Milestone: mozilla35 → ---
Keeping this one open since I think we want a separate bug for Desktop feedback and Standalone feedback for Rooms.
backlog: - → Fx35+
Priority: -- → P1
Summary: Standalone link clicker UI needs a better feedback form → Standalone link clicker UI needs a rooms feedback form
Blocks: 1074659

Updated

3 years ago
Flags: needinfo?(sescalante)
Assignee: nobody → nperriault
Iteration: --- → 36.3
Points: --- → 2
Created attachment 8528468 [details] [diff] [review]
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.
Attachment #8528468 - Flags: feedback?(standard8)
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.
Attachment #8528468 - Flags: feedback?(standard8)
Created attachment 8529134 [details] [diff] [review]
Feedback form displayed for Loop standalone rooms.

Here's a simpler approach, which is enough to ship imho.
Attachment #8528468 - Attachment is obsolete: true
Attachment #8529134 - Flags: review?(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.
Attachment #8529134 - Flags: review?(standard8) → review+
 https://hg.mozilla.org/integration/fx-team/rev/c6dc9a2f152c
Iteration: 36.3 → 37.1
Target Milestone: --- → mozilla36
Created 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.

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.
Attachment #8529240 - Flags: review?(adam)

Comment 9

3 years ago
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.
Attachment #8529240 - Flags: review?(adam) → review+
Created attachment 8529255 [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.

Updated patch to fix review comments.
Attachment #8529240 - Attachment is obsolete: true
Attachment #8529255 - Flags: review+
Follow-up patch pushed:

https://hg.mozilla.org/integration/fx-team/rev/ce3eeca4f793
https://hg.mozilla.org/mozilla-central/rev/c6dc9a2f152c
https://hg.mozilla.org/mozilla-central/rev/ce3eeca4f793
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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
Attachment #8529134 - Flags: approval-mozilla-aurora?

Updated

3 years ago
Attachment #8529255 - Flags: approval-mozilla-aurora?
status-firefox35: --- → affected
status-firefox36: --- → fixed
Flags: qe-verify-
Flags: in-moztrap-
Comment on attachment 8529134 [details] [diff] [review]
Feedback form displayed for Loop standalone rooms.

Approval Request Comment
Transfer request to beta
Attachment #8529134 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?

Updated

3 years ago
Attachment #8529255 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8529134 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8529255 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/b0f3e2aeb66d
https://hg.mozilla.org/releases/mozilla-beta/rev/8689e7804a1f

Updated

3 years ago
status-firefox35: affected → fixed
Blocks: 1077257
You need to log in before you can comment on or make changes to this bug.