Closed
Bug 799562
Opened 12 years ago
Closed 10 years ago
Update about:feedback to use new feedback API when input.mozilla.org changes
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: Margaret, Assigned: vivek, Mentored)
References
Details
(Whiteboard: [lang=js])
Attachments
(1 file, 2 obsolete files)
4.15 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
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.
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? :)
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.
Flags: needinfo?(nickecarlo)
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! :)
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.
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
Assignee: nobody → hiraysushant
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
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.
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!
Comment 9•11 years ago
|
||
The Input API is pushed to production now.
Please continue to use input.allizom.org for testing.
Updated•10 years ago
|
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=js] → [lang=js]
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.
Assignee: hiraysushant → nobody
Assignee | ||
Comment 11•10 years ago
|
||
Hi Margaret,
Thanks, I'll look into this bug in parallel.
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".
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8477725 -
Flags: feedback?
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.
Attachment #8477725 -
Flags: feedback? → feedback?(margaret.leibovic)
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
Flags: needinfo?(willkg)
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.
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.
Attachment #8477725 -
Flags: feedback?(margaret.leibovic) → feedback+
Assignee | ||
Comment 18•10 years ago
|
||
A new patch that address the Feedback comments.
Attachment #8477725 -
Attachment is obsolete: true
Attachment #8479311 -
Flags: review?(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.
Attachment #8479311 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 20•10 years ago
|
||
minor coding convention change
Attachment #8479311 -
Attachment is obsolete: true
Attachment #8479353 -
Flags: review?(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.
Attachment #8479353 -
Flags: review?(margaret.leibovic) → review+
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → vivekb.balakrishnan
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.
Updated•10 years ago
|
Flags: needinfo?(willkg)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 23•10 years ago
|
||
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
Comment 24•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 35
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?
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?
Flags: needinfo?(vivekb.balakrishnan)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•