Closed
Bug 1048834
Opened 10 years ago
Closed 10 years ago
Loop desktop client should send expected description on sad feedback
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(firefox34 verified)
VERIFIED
FIXED
mozilla34
Tracking | Status | |
---|---|---|
firefox34 | --- | verified |
People
(Reporter: NiKo, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
standard8
:
review+
dhenein
:
ui-review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → nperriault
Assignee | ||
Comment 1•10 years ago
|
||
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.
Attachment #8467772 -
Flags: review?(standard8)
Comment 2•10 years ago
|
||
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/mbanner@mozilla.com-b0e6127d1dc0/
It should be available in an hour or two (check for the green status on https://tbpl.mozilla.org/?tree=Try&rev=b0e6127d1dc0).
Attachment #8467772 -
Flags: ui-review?(dhenein)
Comment 3•10 years ago
|
||
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?
Attachment #8467772 -
Flags: ui-review?(dhenein) → ui-review-
Assignee | ||
Comment 4•10 years ago
|
||
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?"
Attachment #8467772 -
Attachment is obsolete: true
Attachment #8467772 -
Flags: review?(standard8)
Attachment #8468567 -
Flags: review?(standard8)
Comment 5•10 years ago
|
||
Comment on attachment 8468567 [details] [diff] [review]
Fixed empty sad feedback description field for Loop for predefined categories.
Thanks Niko!
Attachment #8468567 -
Flags: ui-review+
Comment 6•10 years ago
|
||
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!
Attachment #8468567 -
Flags: review?(standard8) → review+
Comment 7•10 years ago
|
||
Target Milestone: --- → mozilla34
Comment 8•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 10•10 years ago
|
||
Does this need manual test coverage or is the automation sufficient?
Whiteboard: [qa?]
Comment 11•10 years ago
|
||
(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?
Flags: qe-verify?
Flags: needinfo?(nperriault)
Whiteboard: [qa?]
Assignee | ||
Comment 12•10 years ago
|
||
(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/
Flags: needinfo?(nperriault)
Comment 13•10 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify? → qe-verify+
status-firefox34:
--- → verified
QA Contact: anthony.s.hughes
You need to log in
before you can comment on or make changes to this bug.
Description
•