1048834 - Loop desktop client should send expected description on sad feedback

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

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

Right now when an end user clicks on a predefined category on the feedback form after a call, only "Sad User" is sent to the feedback API, while it's supposed to attach the field label in such a case.

Comment 1

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

Attachment: Fixed empty sad feedback description field for Loop for predefined categories.

Also, I removed the __ alias for l10n.get in views.jsx as it was preventing proper stubbing in related tests. I've filed bug 1048900 to further address this issue globally.

Last, the UI showcase page now shows l10n string ids in lieu of the previous "fake text" default string.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8467772 [details] [diff] [review]
Fixed empty sad feedback description field for Loop for predefined categories.

In general, the patch looks reasonable. However, I have concerns about this, especially in light of bug 1049028.

Therefore, requesting feedback from Darrin. My concerns are:

1) The patch disables the "Other" text-entry field, when "Other" isn't the selected option.

2) The text in the text-entry field is changed to match the selected feedback category, e.g. select "Video quality", and the text-entry field changes to "Video quality" as well.

Darrin, there's a try build in progress here:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b0e6127d1dc0/

It should be available in an hour or two (check for the green status on https://tbpl.mozilla.org/?tree=Try&rev=b0e6127d1dc0).

Comment 3

[Darrin Henein [:darrin]]

Comment on attachment 8467772 [details] [diff] [review]
Fixed empty sad feedback description field for Loop for predefined categories.

Looks great! I do understand your concern though, that wasn't exactly what I had intended. 

The Other text field should be disabled unless Other is selected. The text-entry field should remain empty unless Other is selected. We do not need to populate the text field with the feedback selection (i.e. it should be empty rather than contain "Video Quality" if Video Quality is selected). 

Is that clear?

Comment 4

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

Attachment: Fixed empty sad feedback description field for Loop for predefined categories.

New version of the patch after discussing with :darrin and as per bug 1049028 (which should be marked as fixed if this ever lands):

- clicking on the custom description text input will automaticaly select the "other" category choice
- custom description text placeholder text has been updated to "What went wrong?"

Comment 5

[Darrin Henein [:darrin]]

Comment on attachment 8468567 [details] [diff] [review]
Fixed empty sad feedback description field for Loop for predefined categories.

Thanks Niko!

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8468567 [details] [diff] [review]
Fixed empty sad feedback description field for Loop for predefined categories.

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

Great, much nicer!

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a29e8343c897

Comment 10

[u279076]

Does this need manual test coverage or is the automation sufficient?

Comment 11

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> Does this need manual test coverage or is the automation sufficient?

Niko, can you please answer this?

Comment 12

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

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #10)
> Does this need manual test coverage or is the automation sufficient?

The unit tests in place should get us covered I think, though as we don't have functional tests just yet, it's possibly good to have the whole feedback workflow verified by hand. 

Existing manual tests[0] for bug 972992 should be enough for this bug, though ensure to add a case for user provided custom feedback message (see my comment on the bug).

[0]: https://moztrap.mozilla.org/manage/case/14559/

Comment 13

[u279076]

Thanks Niko. I've amended the smoketest to cover the submitting additional feedback case. I've also tested that custom feedback messages are sent to input.allizom.org.