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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Verson Xiong (Leave from Mozilla)]

Attachment: 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

Comment 1

[Verson Xiong (Leave from Mozilla)]

Attachment: logcat_2027.txt

Comment 2

[Eric Chang [:ericcc] [:echang]]

Legacy issue.

Comment 3

[howie [:howie]]

Hi, Arthur can you check on this?

Comment 4

[howie [:howie]]

Triage: blocking

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 !

Comment 6

[[:Cww]]

Will is our dev.

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

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?

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(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

[Will Kahn-Greene [:willkg] ET needinfo? me]

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

[Eric Chang [:ericcc] [:echang]]

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?

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

payload for 400 error :

{"product":"Firefox OS","platform":"Firefox OS","description":"Hi","email":"[e-mail]","version":"2.2.0.0-prerelease","device":"qcom","locale":"en-US"}

payload for 429 error : 
{"product":"Firefox OS","platform":"Firefox OS","description":"Hi","email":"[e-mail]","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.

Comment 12

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: 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 ...

Comment 13

[Will Kahn-Greene [:willkg] ET needinfo? me]

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.

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] EragonJ:bug-1155600 > mozilla-b2g:master

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 !!

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

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!

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] EragonJ:v2.2-bug-1155600 > mozilla-b2g:v2.2

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8599121 [details] [review]
[gaia] EragonJ:bug-1155600 > mozilla-b2g:master

Arthur, can you help me review this patch ? Thanks !

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8599693 [details] [review]
[gaia] EragonJ:v2.2-bug-1155600 > mozilla-b2g:v2.2

This bug too, thank you Arthur :)

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8599693 [details] [review]
[gaia] EragonJ:v2.2-bug-1155600 > mozilla-b2g:v2.2

r=me, thanks!

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

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!

Comment 23

[Francesco Lodolo [:flod]]

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?

Comment 24

[Francesco Lodolo [:flod]]

(forgot the needinfo)

Comment 25

[Matej Novak [:matej]]

(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?

Comment 26

[GitHub Autolander Bot]

Attachment: [gaia] crh0716:1155600 > mozilla-b2g:master

Comment 27

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 28

[Arthur Chen [:arthurcc][inactive after 6/30]]

master: ba860ac479a07bb0265a23d210f62447e872b4fe

Comment 29

[Arthur Chen [:arthurcc][inactive after 6/30]]

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.

Comment 30

[Brogan Zumwalt [Inactive]]

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

Comment 31

[Will Kahn-Greene [:willkg] ET needinfo? me]

(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

[Brogan Zumwalt [Inactive]]

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

[Will Kahn-Greene [:willkg] ET needinfo? me]

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

[Brogan Zumwalt [Inactive]]

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

[Will Kahn-Greene [:willkg] ET needinfo? me]

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

[Brogan Zumwalt [Inactive]]

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

[Will Kahn-Greene [:willkg] ET needinfo? me]

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

[Brogan Zumwalt [Inactive]]

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.

Comment 39

[Will Kahn-Greene [:willkg] ET needinfo? me]

Awesome! Thank you! :)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/7a3ac0c00d834aa62510c9c8e4a9b915f4b47917

Comment 41

[Shally Li（leave from Mozilla）]

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

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.2.mp4