Closed
Bug 1155600
Opened 10 years ago
Closed 10 years ago
[Flame][Settings]Device will prompt user that email address is missing or invalid, or feedback has already been sent when user submits feedback.
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(blocking-b2g:2.2+, b2g-v2.1 affected, b2g-v2.2 verified, b2g-master verified)
People
(Reporter: xiongfuchao, Assigned: eragonj)
Details
Attachments
(7 files)
3.36 MB,
video/mp4
|
Details | |
105.42 KB,
text/plain
|
Details | |
381.08 KB,
image/png
|
Details | |
46 bytes,
text/x-github-pull-request
|
arthurcc
:
review+
arthurcc
:
feedback+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
arthurcc
:
review+
|
Details | Review |
46 bytes,
text/x-github-pull-request
|
arthurcc
:
review+
bajaj
:
approval-gaia-v2.2+
|
Details | Review |
5.93 MB,
video/mp4
|
Details |
[1.Description]:
[Flame][v2.2 & v3.0][Settings]If user try to send a feedback with correct address, device fails to send, and will prompt that whether Email address is missing or invalid. If user check email and try again,device will prompt that user has already sent feedback.
Found time:20:27
See attachment:logcat_2028.txt & video_2027.mp4
[2.Testing Steps]:
1. Open Settings.
2. Tap Improve Firefox OS.
3. Tap Send Mozilla Feedback.
4. Tap happy or sad.
5. Input any words in detail field.
6. Select "Check here te let us ...".
7. Input a correct address.
8. Tap send button.
9. Tap OK and check address.
10. Send again.
[3.Expected Result]:
8. User can send feedback successfully.
10.User can sent feedback successfully.
[4.Actual Result]:
8.Fail to send feedback and device will prompt that Email address is missing or invalid.
10.Fail to send feedback and device will prompt that user has already sent feedback.
[5.Reproduction build]:
Flame 2.2 version(Affected):
Build ID 20150416162504
Gaia Revision d50b8a3919a7b4d8d289f150d3b9bed704ebafa9
Gaia Date 2015-04-16 21:46:57
Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5ebf32030512
Gecko Version 37.0
Device Name flame
Firmware(Release) 4.4.2
Firmware(Incremental) eng.cltbld.20150416.195720
Firmware Date Thu Apr 16 19:57:29 EDT 2015
Bootloader L1TC000118D0
Flame 3.0version(Affected):
Build ID 20150416160206
Gaia Revision 3cd0a9facce26c2acc7be3755a17131a6358e33f
Gaia Date 2015-04-16 16:33:22
Gecko Revision https://hg.mozilla.org/mozilla-central/rev/51e3cb11a258
Gecko Version 40.0a1
Device Name flame
Firmware(Release) 4.4.2
Firmware(Incremental) eng.cltbld.20150416.191700
Firmware Date Thu Apr 16 19:17:10 EDT 2015
Bootloader L1TC000118D0
[6.Reproduction Frequency]:
Always Recurrence,5/5
[7.TCID]:
Free Test
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
Comment 3•10 years ago
|
||
Hi, Arthur can you check on this?
blocking-b2g: --- → 2.2?
Flags: needinfo?(hochang)
Assignee | ||
Comment 5•10 years ago
|
||
Ahh, after investigating the root cause for a while, I noticed that what Settings do is just trying to show related UI based on any response coming from server. For server part, it seems our endpoint is : https://input.allizom.org/api/v1/feedback/
So, Cheng, can you help us shed some lights here why at first if users input a right email with feedback, Settings app will get 400 error from server, and then for the second press, because the server may think the user already provided feedback already, it directly returned 429 error (Too many requests).
There are nothing much that Settings app can do here, any idea ?
Thanks !
Flags: needinfo?(cww)
Comment 7•10 years ago
|
||
Stepping back a bit, I think there are two problems here. First off, for the 429 scenario, I wouldn't mention that the feedback was already submitted at all. That particular rate-limiting is there to prevent double-submits which happens all the time. If the user sends exactly the same feedback, then it'll trigger, but that should only happen in a double-submit scenario and when the user is abusing Input. Thus it's not something you should be notifying the user of because there's nothing the user can do about it plus it suggests the user did something wrong. Instead, I'd treat it as if the feedback was submitted successfully.
In this particular situation, the POST that gets back the 400 error has something wrong with it. That's a bug somewhere. Once we fix that, then the later 429 error will be moot.
Second problem is the 400 error. There isn't enough information here for me to have any idea what it might be. What is the JSON payload being sent to the server in this case?
Flags: needinfo?(xiongfuchao)
Flags: needinfo?(willkg)
Flags: needinfo?(cww)
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Will Kahn-Greene [:willkg] from comment #7)
> In this particular situation, the POST that gets back the 400 error has
> something wrong with it. That's a bug somewhere. Once we fix that, then the
> later 429 error will be moot.
>
Sounds good !
> Second problem is the 400 error. There isn't enough information here for me
> to have any idea what it might be. What is the JSON payload being sent to
> the server in this case?
Let's wait for Verson's comment here :), but based on my experiment, I used latest 2.2 flame and type the same feedback + email can reproduce it (please check attached mp4 for more information).
Comment 9•10 years ago
|
||
I saw the mp4 and the log, but neither have the information I'm looking for. I need to know what the settings app is actually sending Input. If you don't know that or can't figure it out, let me know.
My current theory is that the settings app is sending bad data which is a very common reason for getting an HTTP 400.
http://fjord.readthedocs.org/en/latest/api.html#responses
Without being able to see the actual data in the payload, it's hard to guess more than that.
Other interesting questions:
Can you reproduce it with other email addresses and descriptions or just that set?
Does this happen with all feedback from the settings app?
Comment 10•10 years ago
|
||
Hi Will, Is there a tool to capture that? Or we just capture the tcp dump?
(In reply to Will Kahn-Greene [:willkg] from comment #7)
> Second problem is the 400 error. There isn't enough information here for me
> to have any idea what it might be. What is the JSON payload being sent to
> the server in this case?
Flags: needinfo?(willkg)
Assignee | ||
Comment 11•10 years ago
|
||
payload for 400 error :
{"product":"Firefox OS","platform":"Firefox OS","description":"Hi","email":"versonx@gmail.com","version":"2.2.0.0-prerelease","device":"qcom","locale":"en-US"}
payload for 429 error :
{"product":"Firefox OS","platform":"Firefox OS","description":"Hi","email":"versonx@gmail.com","version":"2.2.0.0-prerelease","device":"qcom","locale":"en-US"}
Any idea ? BTW, it seems that there is no "happy" field compared with your documentation.
Assignee | ||
Comment 12•10 years ago
|
||
In addition to missing "happy" field problem, I noticed that yesterday, I can still get the right this._xhr.status from server, but right now it would keep reporting `0`.
And also, after doing more tests, I got blocked (? not sure) from server and there is no any response passing back (please check my attachment)
What an interesting syndrome ...
Comment 13•10 years ago
|
||
Looking at the JSON you have in comment #11, both are missing the "happy" field. That'll definitely cause an HTTP 400 error. I'm puzzled that the settings app then turns around and tells the user the email address isn't correct. That's pretty unhelpful in this scenario. However, once the "happy" bug is fixed, maybe it's more likely to be correct.
A year or so ago, I reviewed the original code and it was correct then. I think that landed in Firefox OS 1.3. Seems like someone has changed the code to the settings app since then.
I know enough about Firefox OS development to know that there's a bunch of code in a bunch of branches. However, I have no idea where the 2.2 code is. If you can point me at the code for the version you're having problems with, I can take a look at it and suggest fixes.
Flags: needinfo?(willkg)
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8599121 [details] [review]
[gaia] EragonJ:bug-1155600 > mozilla-b2g:master
Arthur, for this bug, it seems that from long time ago, Settings app haven't sent any `happy` field back to server so this will cause 400 error. What I did here is adding the missing field back and make sure we did pass all required fields.
And for this patch, as what Will said, there is one problem that Settings app should fix here, please check below code snippet first :
https://github.com/mozilla-b2g/gaia/pull/29788/files#diff-2e82ef990d24b325353b5f6b93cfc0dbR158
In default error case, we will hint users "Sorry, the system can’t send your suggestion due to a data connection error. Please try again when the issue is resolved." and I think this may mislead users because there might be no problems about data connection.
So for me, I think the better strings here would be more general like "Unknown error happened, please try again."
And there is another weird thing I found, if we use `onstatechange` here, the xhr.status would not be the right number what we expected, it would keep reporting 0 when the handler is called. So, what I did here is use `onload` to make sure the request is finished and we can get the right status from xhr.
Any comment / idea would be appreciated, thanks !!
Attachment #8599121 -
Flags: feedback?(arthur.chen)
Comment 16•10 years ago
|
||
Comment on attachment 8599121 [details] [review]
[gaia] EragonJ:bug-1155600 > mozilla-b2g:master
For master we can have a new string reporting the generic error. As for v2.2 I think it should be ok to keep it due to the string freeze.
In general using onload and checking the ready state are the same thing, so I have no concern on the replacement. f=me, thanks!
Attachment #8599121 -
Flags: feedback?(arthur.chen) → feedback+
Comment 17•10 years ago
|
||
Assignee | ||
Comment 18•10 years ago
|
||
To make things clear, let me tell you guys about more details that in these two patches.
For master patch, because we are able to edit locale strings, I introduced 'unknown-error' string for 400 & default case. Based on what Will said before, 400 includes an error case that users' email is in a wrong format, but because we did handle this in Gaia, there should not be no such situation for this. So, 400 & default case are the same, we just throw out unknown-error to users. And for 429, we will treat it as success here.
For 2.2 patch, because there is no way to edit strings, I will keep the same error message for 400 & default case, but as what I said above, because we did handle invalid email case in Gaia, based on our assumption, there should be no 400 status coming back and we won't show `wrong-email` alert here. And for 429, it is treated as success case as well.
For two patches, I also fix the missing `email` field problem.
Hope this information is clear enough for reviewers and others who are interested in this bug.
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8599121 [details] [review]
[gaia] EragonJ:bug-1155600 > mozilla-b2g:master
Arthur, can you help me review this patch ? Thanks !
Attachment #8599121 -
Flags: review?(arthur.chen)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8599693 [details] [review]
[gaia] EragonJ:v2.2-bug-1155600 > mozilla-b2g:v2.2
This bug too, thank you Arthur :)
Attachment #8599693 -
Flags: review?(arthur.chen)
Comment 21•10 years ago
|
||
Comment on attachment 8599693 [details] [review]
[gaia] EragonJ:v2.2-bug-1155600 > mozilla-b2g:v2.2
r=me, thanks!
Attachment #8599693 -
Flags: review?(arthur.chen) → review+
Comment 22•10 years ago
|
||
Comment on attachment 8599121 [details] [review]
[gaia] EragonJ:bug-1155600 > mozilla-b2g:master
r=me from the aspect of code.
Flod, in this patch a string for generic error was added. Would you mind help review the string? Thanks!
Attachment #8599121 -
Flags: review?(arthur.chen)
Attachment #8599121 -
Flags: review+
Attachment #8599121 -
Flags: feedback?(francesco.lodolo)
Comment 23•10 years ago
|
||
Comment on attachment 8599121 [details] [review]
[gaia] EragonJ:bug-1155600 > mozilla-b2g:master
What you need here is copy review, not l10n, and I'm not the right person for that.
@Matej
feedback-errormessage-unknown-error=Sorry, it seems something is wrong here, please try again.
Any feedback?
Attachment #8599121 -
Flags: feedback?(francesco.lodolo)
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(xiongfuchao)
Comment 25•10 years ago
|
||
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #23)
> Comment on attachment 8599121 [details] [review]
> [gaia] EragonJ:bug-1155600 > mozilla-b2g:master
>
> What you need here is copy review, not l10n, and I'm not the right person
> for that.
>
> @Matej
>
> feedback-errormessage-unknown-error=Sorry, it seems something is wrong here,
> please try again.
>
> Any feedback?
I would consider shortening it:
Something went wrong. Please try again.
Does that work?
Flags: needinfo?(matej)
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
Comment on attachment 8600740 [details] [review]
[gaia] crh0716:1155600 > mozilla-b2g:master
Thanks Matej and flod!
EJ is on PTO. Updated the string in the new PR.
Attachment #8600740 -
Flags: review+
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 28•10 years ago
|
||
master: ba860ac479a07bb0265a23d210f62447e872b4fe
Comment 29•10 years ago
|
||
Comment on attachment 8600740 [details] [review]
[gaia] crh0716:1155600 > mozilla-b2g:master
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 973446
[User impact] if declined: User feedback is not able to be sent.
[Testing completed]: Unit test updated.
[Risk to taking this patch] (and alternatives if risky): Low. The patch adds the missing field.
[String changes made]: We only added the new string in the PR against master. The one against v2.2 does not contain the string change.
Attachment #8600740 -
Flags: approval-gaia-v2.2?
Comment 30•10 years ago
|
||
Issue verified fixed in Flame 3.0
On first feedback submit with email address user receives successful feedback message (no 400 error message), on second submit with same email address the user receives same successful feedback message (no old 429 error message.)
Leaving verifyme awaiting uplift for 2.2 and 2.1
Device: Flame 3.0 Master
Build ID: 20150505010204
Gaia: 70077825aab2c7a79611befb40a5fe7e610d5443
Gecko: 102d0e9aa9e1
Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
Version: 40.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 31•10 years ago
|
||
(In reply to Brogan Zumwalt [:BroganZ] from comment #30)
> Issue verified fixed in Flame 3.0
>
> On first feedback submit with email address user receives successful
> feedback message (no 400 error message), on second submit with same email
> address the user receives same successful feedback message (no old 429 error
> message.)
>
> Leaving verifyme awaiting uplift for 2.2 and 2.1
>
> Device: Flame 3.0 Master
> Build ID: 20150505010204
> Gaia: 70077825aab2c7a79611befb40a5fe7e610d5443
> Gecko: 102d0e9aa9e1
> Gonk: a9f3f8fb8b0844724de32426b7bcc4e6dc4fa2ed
> Version: 40.0a1 (3.0)
> Firmware Version: v18D-1
> User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0
Can you also verify that submitting happy feedback to Input yields happy feedback and that sad feedback yields sad feedback?
Comment 32•10 years ago
|
||
Will how would I grab this info (happy versus sad reports) from the device? I am not seeing it in logcat, and I doubt information sent via POST would also be stored on the device like a crash report. Could you point me in the right direction?
Comment 33•10 years ago
|
||
I would manually do the tests and check Input which is we did when this was first implemented in 2013.
For a more programmatic way, I don't know how to do that. It'd have to be something baked into the settings app.
Comment 34•10 years ago
|
||
We can't run feedback_send_test.js on our end as we don't have a server set up to handle the feedback responses. We wouldn't be able to capture whether the appropriate feedback was sent. Is there any other way of confirming this Will?
Comment 35•10 years ago
|
||
You have no way to do a manual test by hand and have it hit Input? What's preventing you from doing that?
Comment 36•10 years ago
|
||
I may have misunderstood. I had thought you were asking me to run an individual unit test manually. Reading through this thread again I think you are asking me to check https://input.allizom.org/api/v1/feedback/ after submitting feedback.
Is that correct?
Comment 37•10 years ago
|
||
Yes. I would like someone to verify that the settings app can submit both happy *and* sad feedback to Input. I think that needs to be done manually and by hand and not as a unit test at least once. I've seen no evidence that's been done, yet.
I don't particularly care if you test against Input -stage (https://input.allizom.org/) or Input -prod (https://input.mozilla.org/).
It'd be great if after verifying it works correctly that someone turned it into a unit test with a mock Input, too, so then we're not in exactly this position in 3 months like the last time around when the code got refactored and broken.
Sorry if I wasn't clearer earlier. I'm not really sure who anyone in this bug is, how Firefox OS development works, or how QA works on Firefox OS.
Comment 38•10 years ago
|
||
Alright, 3.0 with same environmental variables as above, both happy and sad show correctly on Input production site after manual testing on Flame device.
Happy: https://input.mozilla.org/en-US/dashboard/response/5347560
Sad: https://input.mozilla.org/en-US/dashboard/response/5347570
Apologies for the confusion on my part.
Flags: needinfo?(ktucker)
Comment 39•10 years ago
|
||
Awesome! Thank you! :)
Updated•10 years ago
|
Flags: needinfo?(ktucker)
Updated•10 years ago
|
Attachment #8600740 -
Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
Comment 40•10 years ago
|
||
Target Milestone: --- → 2.2 S12 (15may)
Comment 41•10 years ago
|
||
This bug has been verified as pass on latest build of Flame v2.2 and Nexus 5 v2.2 by the STR in Comment 0.
Actual results: User can send feedback successfully,and prompt "Thanks for the feedback!We use it to help create better experiences in future releases of Firefox OS" pops up.
See attachment: verified_v2.2.mp4
Reproduce rate: 0/5
Device: Flame v2.2 build(Pass)
Build ID 20150510002505
Gaia Revision 528ef60e7cda09ad43478065f5d33bda398fbeb7
Gaia Date 2015-05-08 23:40:58
Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8d04cc085cf5
Gecko Version 37.0
Device Name flame
Firmware(Release) 4.4.2
Firmware(Incremental) eng.cltbld.20150510.042512
Firmware Date Sun May 10 04:25:23 EDT 2015
Bootloader L1TC000118D0
Device: Nexus 5 v2.2 build (Pass)
Build ID 20150510002505
Gaia Revision 528ef60e7cda09ad43478065f5d33bda398fbeb7
Gaia Date 2015-05-08 23:40:58
Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8d04cc085cf5
Gecko Version 37.0
Device Name hammerhead
Firmware(Release) 5.1
Firmware(Incremental) eng.cltbld.20150510.043050
Firmware Date Sun May 10 04:31:07 EDT 2015
Bootloader HHZ12f
-------------------------------------------------------------
Leaving "verifyme" for v2.1 uplift & verification.
Comment 42•10 years ago
|
||
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
You need to log in
before you can comment on or make changes to this bug.
Description
•