[Flame][Settings]Device will prompt user that email address is missing or invalid, or feedback has already been sent when user submits feedback.

VERIFIED FIXED in 2.2 S12 (15may)

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: xiongfuchao, Assigned: eragonj)

Tracking

unspecified
2.2 S12 (15may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.1 affected, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(7 attachments)

Posted video video_2027.mp4
[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

4 years ago
Posted file logcat_2027.txt
Legacy issue.
Flags: needinfo?(hochang)

Comment 3

4 years ago
Hi, Arthur can you check on this?
blocking-b2g: --- → 2.2?
Flags: needinfo?(hochang)

Comment 4

4 years ago
Triage: blocking
Assignee: nobody → ejchen
blocking-b2g: 2.2? → 2.2+
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 6

4 years ago
Will is our dev.
Flags: needinfo?(willkg)
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)
(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).
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?
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)
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.
Posted image inspector.png
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 ...
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 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 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+
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.
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)
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 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 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 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)
(forgot the needinfo)
Flags: needinfo?(matej)
(Reporter)

Updated

4 years ago
Flags: needinfo?(xiongfuchao)
(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 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+
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
master: ba860ac479a07bb0265a23d210f62447e872b4fe
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?
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
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(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?
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?
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.
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?
You have no way to do a manual test by hand and have it hit Input? What's preventing you from doing that?
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?
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.
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)
Awesome! Thank you! :)
Flags: needinfo?(ktucker)
Attachment #8600740 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
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.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.