Closed Bug 799562 Opened 9 years ago Closed 7 years ago

Update about:feedback to use new feedback API when input.mozilla.org changes

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: Margaret, Assigned: vivek, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

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.
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? :)
Depends on: 895675
No longer depends on: 799553
Flags: needinfo?(nickecarlo)
Whiteboard: [mentor=margaret][lang=js]
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.
Flags: needinfo?(nickecarlo)
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! :)
@Nicolas if you are still busy, I would like to try my hands on this. 
Though I haven't worked with feedback API.
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
Assignee: nobody → hiraysushant
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
(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.
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!
The Input API is pushed to production now.

Please continue to use input.allizom.org for testing.
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=js] → [lang=js]
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.
Assignee: hiraysushant → nobody
Hi Margaret,

Thanks, I'll look into this bug in parallel.
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".
Attached patch 799562.patch (obsolete) — Splinter Review
Attachment #8477725 - Flags: feedback?
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.
Attachment #8477725 - Flags: feedback? → feedback?(margaret.leibovic)
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
Flags: needinfo?(willkg)
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.
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.
Attachment #8477725 - Flags: feedback?(margaret.leibovic) → feedback+
Attached patch 799562.patch (obsolete) — Splinter Review
A new patch that address the Feedback comments.
Attachment #8477725 - Attachment is obsolete: true
Attachment #8479311 - Flags: review?(margaret.leibovic)
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.
Attachment #8479311 - Flags: review?(margaret.leibovic) → review+
Attached patch 799562.patchSplinter Review
minor coding convention change
Attachment #8479311 - Attachment is obsolete: true
Attachment #8479353 - Flags: review?(margaret.leibovic)
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.
Attachment #8479353 - Flags: review?(margaret.leibovic) → review+
Assignee: nobody → vivekb.balakrishnan
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.
Flags: needinfo?(willkg)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/32c0ac7ebfb2
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/32c0ac7ebfb2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 35
Blocks: 1064038
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?
(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?
Flags: needinfo?(vivekb.balakrishnan)
Yes, I can fix it in.
Flags: needinfo?(vivekb.balakrishnan)
Blocks: 1071117
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.