:Margaret Leibovic Reporter
	Description • 12 years ago

Filing this bug to keep track of it, since we'll need it when bug 799553 lands. It shouldn't be too hard to change the API calls we make, we'll just want to coordiante with the people working on input.mozilla.org.

	:Margaret Leibovic Reporter
	Comment 1 • 11 years ago

See bug 895675 comment 25 for details about the new API. We'll just need to update the API we currently use in sendFeedback():
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutFeedback.xhtml#134

Nicolas, I know you worked on about:feedback recently. Is this a bug you might be interested in taking? :)

	Nicolas Carlo [:nickecarlo]
	Comment 2 • 11 years ago

Margaret, I'd be happy to take this but my college year is about to start and I'll be extremely busy until the end of September. Not sure if you need to get this done right away or not?

I can take the bug and if someone else can finish it before I do I'd be happy to give it to them.

Let me know if you're happy with that and I'll take it.

	:Margaret Leibovic Reporter
	Comment 3 • 11 years ago

There's no big rush for this, but we can leave it unassigned for now in case someone else can get to it first. Feel free to assign yourself if you start working on a patch though! :)

	Sushant Hiray [:sushant]
	Comment 4 • 11 years ago

@Nicolas if you are still busy, I would like to try my hands on this. 
Though I haven't worked with feedback API.

	:Margaret Leibovic Reporter
	Comment 5 • 11 years ago

Sushant, I'm assigning you to this bug, since it doesn't look like Nicloas has started working on it at all.

I mentioned in comment 1 which changes need to be made, but let me know if you need more guidance. You can navigate directly to about:feedback to test your changes, but you should be sure to change out the input URL to use the staging server for testing, so that we don't pollute the actual feedback data. More information on that is available here: http://fjord.readthedocs.org/en/latest/api.html

	Sushant Hiray [:sushant]
	Comment 6 • 11 years ago

Margaret,

When I checked the status report of the request I was sending, it was sending an "OPTIONS" request as opposed to POST request I was actually sending.
So I was getting a status message of 301 (Moved Permanently)

When I do the same thing using curl, I'm able to successfully send the request and as well view it on https://input.allizom.org/

I created a temp html file to get the response. It is stored here: http://wncc-iitb.org/itsp/temp2.html

	Will Kahn-Greene [:willkg] ET needinfo? me
	Comment 7 • 11 years ago

(In reply to Sushant Hiray [:sushant] from comment #6)
> Margaret,
> 
> When I checked the status report of the request I was sending, it was
> sending an "OPTIONS" request as opposed to POST request I was actually
> sending.
> So I was getting a status message of 301 (Moved Permanently)
> 
> When I do the same thing using curl, I'm able to successfully send the
> request and as well view it on https://input.allizom.org/
> 
> I created a temp html file to get the response. It is stored here:
> http://wncc-iitb.org/itsp/temp2.html

There are two things going on here:

1. The POST is to http://input.allizom.org/ and not https://input.allizom.org/ (http vs. https) so that's why you're getting back an HTTP 301.

2. Your test is POSTing to the wrong url. See the API for the correct url to POST to.

3. Your test is in the context of a web-page and you're doing an xhr POST. The web browser will pre-flight that with an OPTIONS request to find out if CORS is allowed for that resource. So once you fix the http vs. https issue and the url issue, you'll bump into the fact that I haven't added CORS-related headers to the Input API, yet (see https://bugzilla.mozilla.org/show_bug.cgi?id=895675#c9). With FirefoxOS, they didn't have problems. I don't know if Firefox for Android bumps into CORS things in about:feedback. Ergo, your test isn't in the same context as what you'll be implementing.

Anyhow, if the CORS thing turns out to be an issue in the about:feedback context, too, let me know and I'll fix the headers on Input.

	Will Kahn-Greene [:willkg] ET needinfo? me
	Comment 8 • 11 years ago

I finished up the code changes to the Input API that I needed to do before pushing it to production. One thing I changed is that the "product" field restricts you to specific values.

For Firefox for Android, the "product" value needs to be "Firefox for Android". That value is case-sensitive. If you send something else, you'll get an HTTP 400 back with an explanation that the product is not valid.

Hope that helps!

	Will Kahn-Greene [:willkg] ET needinfo? me
	Comment 9 • 11 years ago

The Input API is pushed to production now.

Please continue to use input.allizom.org for testing.

	:Margaret Leibovic Reporter
	Comment 10 • 10 years ago

Unassinging, since this hadn't been touched in a long time.

Vivek, you may be interested in this, since you've been poking around this area of the code for bug 1007436.

	Vivek Balakrishnan[:vivek] Assignee
	Comment 11 • 10 years ago

Hi Margaret,

Thanks, I'll look into this bug in parallel.

	Will Kahn-Greene [:willkg] ET needinfo? me
	Comment 12 • 10 years ago

For the record, the Input API is documented here:

http://fjord.readthedocs.org/en/latest/api.html#posting-product-feedback-api-v1-feedback

The product string for Firefox for Android is "Firefox for Android".

	Vivek Balakrishnan[:vivek] Assignee
	Comment 14 • 10 years ago

Comment on attachment 8477725 [details] [diff] [review]
799562.patch

Initial patch to post feedback data using the api. 

Margaret,
Only posting negative feedback are being posted. should I include positive feedback as well?

As discussed over irc, please suggest what other optional data we would need.

	Vivek Balakrishnan[:vivek] Assignee
	Comment 15 • 10 years ago

Will,

Is the description field limited to 10000 characters in database.[1]
can you also please provide the char limit for url field.

[1] https://input.mozilla.org/en-GB/feedback#1

	Will Kahn-Greene [:willkg] ET needinfo? me
	Comment 16 • 10 years ago

The description is not limited in the database or the API currently. It's limited in that form plus there's a character counter. There's a few reasons for that, but I don't remember what they are at the moment.

Looks like the url field is limited to 200 characters in the db, but I don't think we set max lengths anywhere. I'll have to double-check that. I might be able to get to that this weekend, but if not, I can do it Thursday.

I'll keep the needinfo open until I figure that out.

	:Margaret Leibovic Reporter
	Comment 17 • 10 years ago

Comment on attachment 8477725 [details] [diff] [review]
799562.patch

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

Nice, this is looking really good. I just have a few comments. Let's wait to hear if willkg has any other feedback, but if not, I think we could land this soon.

::: mobile/android/chrome/content/aboutFeedback.js
@@ +96,5 @@
>    // of showing an error message for us.
>    if (!descriptionElement.validity.valid)
>  	return;
>  
> +  let data = {}

Make sure you include semi-colons at the end of lines.

@@ +99,5 @@
>  
> +  let data = {}
> +  data["happy"] = false;
> +  data["description"] = descriptionElement.value;
> +  data["product"] = "Firefox for Android"

You should add a comment explaining that this exact string is required by input.mozilla.org. It might be worth pulling it into a constant at the top of the file to make this more obvious.

@@ +109,5 @@
>  	return;
>  
>    // Only send a URL string if the user provided one.
>    if (urlElement.value) {
> +	data["url"] = urlElement.value;

Nit: while you're here, you should fix this indentation to be 2 spaces.

	Vivek Balakrishnan[:vivek] Assignee
	Comment 18 • 10 years ago

A new patch that address the Feedback comments.

	:Margaret Leibovic Reporter
	Comment 19 • 10 years ago

Comment on attachment 8479311 [details] [diff] [review]
799562.patch

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

One small piece of feedback, but with that addressed this looks great!

::: mobile/android/chrome/content/aboutFeedback.js
@@ +4,5 @@
>  
>  "use strict";
>  
> +// input.mozilla.org expects "Firefox for Android" as the product.
> +const feedbackProductString = "Firefox for Android";

We generally like to follow the java-style convention of putting constants in all-caps, so this should be FEEDBACK_PRODUCT_STRING.

	Vivek Balakrishnan[:vivek] Assignee
	Comment 20 • 10 years ago

minor coding convention change

	:Margaret Leibovic Reporter
	Comment 21 • 10 years ago

Comment on attachment 8479353 [details] [diff] [review]
799562.patch

Great, thanks!

There's not a big rush to land this, so let's wait until willkg gets back to hear if there are any changes we should make before landing.

	Will Kahn-Greene [:willkg] ET needinfo? me
	Comment 22 • 10 years ago

I updated the API documentation adding max length to relevant fields. I also added some notes about PII-relevant informational text.

To go back through the questions:

1. The description field does not have a max length, but we like to cap it at 10,000 because feedback longer than that tends to either be spam or cover many topics which makes analysis more difficult.

2. The URL field does have a max length of 200 characters. I wrote up bug 1059826 to fix a problem where that max length isn't currently being validated, so it kicks up errors when saving to the db.


I think that covers everything. If not, please let me know.

	Carsten Book [:Tomcat]
	Comment 23 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/32c0ac7ebfb2

	Will Kahn-Greene [:willkg] ET needinfo? me
	Comment 25 • 10 years ago

I screwed up and didn't look at the code changes closely enough. This also needs to send the version.

+  let data = {};
+  data["happy"] = false;
+  data["description"] = descriptionElement.value;
+  data["product"] = FEEDBACK_PRODUCT_STRING;

Something like:

    data["version"] = some-way-to-get-the-version;

Otherwise there's no way for us to know what version the feedback applies to.

Here's an example I posted just now:

https://input.mozilla.org/en-US/dashboard/response/4594582

It lists the version as "Unknown".

Should I write up a new bug for adding the version or can we reopen this one?

	:Margaret Leibovic Reporter
	Comment 26 • 10 years ago

(In reply to Will Kahn-Greene [:willkg] from comment #25)
> I screwed up and didn't look at the code changes closely enough. This also
> needs to send the version.
> 
> +  let data = {};
> +  data["happy"] = false;
> +  data["description"] = descriptionElement.value;
> +  data["product"] = FEEDBACK_PRODUCT_STRING;
> 
> Something like:
> 
>     data["version"] = some-way-to-get-the-version;
> 
> Otherwise there's no way for us to know what version the feedback applies to.
> 
> Here's an example I posted just now:
> 
> https://input.mozilla.org/en-US/dashboard/response/4594582
> 
> It lists the version as "Unknown".
> 
> Should I write up a new bug for adding the version or can we reopen this one?

New bug please, and cc me and vivek.

vivek, would you be able to spin up a quick fix for that issue?

	Vivek Balakrishnan[:vivek] Assignee
	Comment 27 • 10 years ago

Yes, I can fix it in.