This is DTL-1 from the PRD at http://wiki.mozilla.org/Update:RequirementsV33

Still a couple minor todos...submitting for sanity check and adherence to the Mozilla Way (tm)

Yay! The API is going to be awesome. Here are some initial comments on your patch: In controllers/api_controller.php: $error = 'error_addon_notfound'; should be $error = _('error_addon_notfound'); so it will pick up the localized string In views/layouts/rest.thtml: - If this will always be used for xml, it should have: <?php header('Content-type: text/xml'); ?> Otherwise that should be added elsewhere for ApiController::Addon() - I realize it's built in, but we're not using e() anywhere else. Normally we'll just leave that outside of the php block or just use echo. In views/api/addon.thtml: - I think the <rating> code could be cleaned up with a ternary. Something like: <rating><?php echo empty($addon['averagerating']) ? _('addon_not_rated') : $addon['averagerating']; ?></rating> Notice the _() function in this line too. Also, for any strings like "addon_not_rated" that we add, we'll also have to add those to the en-US messages.po and push it to all the other .po files. In both .thtml files: - Both of these should have the standard license blocks in them (just copy from another file) - Both have several links to AMO pages that start with http - they should be https (or even better, key off $_SERVER['HTTPS']) Also, you included the patch for views/api/addon.thtml twice.

Also, it's not in the spec, but doesn't it seem like a good idea to put the file hash in there somewhere? Maybe as an attribute of <install>?

Comment on attachment 285114 [details] [diff] [review] patch for this api call I'm gonna r- citing my previous comments, but don't be discouraged - it's looking good.

(In reply to comment #3) > Also, it's not in the spec, but doesn't it seem like a good idea to put the > file hash in there somewhere? Maybe as an attribute of <install>? I like the idea. Since we do not know how a third-party developer may use the API, we should give them an easy way to validate downloaded files. After all, we don't force anyone to use it, but it sure won't hurt.

Changes made as requested, added hash, added reviews.

I r+'d the current patch so you could commit it, but some more thoughts: - Requesting an add-on that is in the sandbox get's incomplete information: no hash, and no file. We should talk about whether we're going to allow the API to bypass the sandbox or not (CCing Basil to comment on this) - We should add another field for whether an add-on is in the sandbox or not - We're filling in _('addon_not_rated') if there is no rating. Why not just leave it null?

I think we should include sandbox'ed extensions as part of the get-able data. We may change the behavior of the sandbox and queues in the future and this artificial separation may become blurred. I view "sandbox" as an attribute of the add-on, like we have "site specific". This metadata should be returned as part of the get details request.

This is looking good however there is a bunch of information not included in the response that I think is fairly important: ID (install.rdf id, not AMO ID) Version Icon Summary EULA Privacy policy Application compatibility Platform compatibility Also the response currently includes a single thumbnail regardless of whether the add-on actually has a thumbnail (and only ever one if the add-on has multiple images).

Some of the information is not getting it's entities escaped properly, e.g. https://addons.mozilla.org/en-US/firefox/api/addon/2955

Following up from our meeting, I'll make the following changes: - Add guid, version, icon, summary, privacy policy (not eula), application compat, platform compat - More thumbnails - Include sandboxed addons - Indicate whether sandboxed

(In reply to comment #11) > - Add guid, version, icon, summary, privacy policy (not eula), application > compat, platform compat It's actually the eula we need and not the privacy policy

Doh, ok. My notes suck.

Laura, is there any eta on these updates?

Fixed in r8655. Rest to come shortly, I have a bunch of outstanding commits sitting here.

(In reply to comment #15) > Fixed in r8655. Rest to come shortly, I have a bunch of outstanding commits > sitting here. When is that going to go live, or is it available on a staging server I can test against?

Should go in the next push - I can see about staging it somewhere for you before then though.

Icon and summary still not correct, need to fix.

Also - Add application id as well as shortname for compatible apps since this won't currently work with rebranded apps. - Considering trimming status as it has trailing whitespace (translation issue)? - Add ids to status and type - Add one install stanza per available OS. At present some addons are not returning all the valid OSes e.g. 4202.

One other issue that still exists is that the xml still contains a thumbnail field regardless of whether the add-on actually has a thumbnail, for example https://addons.mozilla.org/en-US/firefox/api/addon/3056.

I'm also not seeing any add-ons with ratings. Then again I don't see any ratings on the AMO site itself?

And another issue, the text fields aren't being escaped for xml use, there are plain &'s in there that are breaking the parsing.

Working my way through these issues...

Problem: Different versions in add-ons page and api page Example: Zoomy Page: version 2.0 https://addons.mozilla.org/de/firefox/addon/1225 (in sandbox) API: version 2.1 https://addons.mozilla.org/en-US/firefox/api/addon/1225 http://releases.mozilla.org/pub/mozilla.org/addons/1225/ shows only version 2.0

SOrry, just one other one, I'm not seeing the eula on items like https://addons.mozilla.org/en-US/firefox/api/addon/4988

Added icons, summaries, ids for type and status, eula. Re the other issues: - Install stanza issue: Should be working. (That's all we have in the DB for 4202.) - Thumbnail field issue (when there's no thumbnail) is also an issue with the icon. It's actually a bug in the underlying image component, and the bug is present in the regular a.m.o pages as well. Filing a generic separate bug for that. - The ratings don't seem to be getting used in the db...I assume that's going to change. - XML parsing: examples I can find are entity-fied. Can you give me an example where it's broken?

Excellent this is coming together nicely now. Still seeing a couple of these below though: (In reply to comment #26) > - Install stanza issue: Should be working. (That's all we have in the DB for > 4202.) This is still a problem. I'm afraid I mistyped above, it is addon 5202, the Firefox eBay companion. The AMO pages show versions for all 3 platforms, the API only claims it is compatible with Windows. > - The ratings don't seem to be getting used in the db...I assume that's going > to change. Could we get a bug filed on this please? I would do it myself but I suspect you are better placed to know how to phrase it and where to put it than me. > - XML parsing: examples I can find are entity-fied. Can you give me an example > where it's broken? I am seeing a lot of xml entities happening, possibly this is not an issue with the API but is with specific items in the DB? Anyway one I am seeing it with is https://addons.mozilla.org/en-US/firefox/api/addon/2955

One other oddity. The urls coming back for icon, thumbnail, learnmore etc. are all in the services.addons.mozilla.org host. Not sure if that is meant to be the case or not. For some reason for me these urls work ok, just redirecting to addons.mozilla.org properly, Madhava however sees 403 forbidden errors for them.

r9618 fixes the URL and redirect issues.

r9619 fixes the rating issue.

(In reply to comment #27) > > - XML parsing: examples I can find are entity-fied. Can you give me an example > > where it's broken? > > I am seeing a lot of xml entities happening, possibly this is not an issue with > the API but is with specific items in the DB? Anyway one I am seeing it with is > https://addons.mozilla.org/en-US/firefox/api/addon/2955 > Weirdly, this works perfectly on my local copy, but not on amo. Assuming this is a configuration issue. Digging.

(In reply to comment #19) > Also > - Add application id as well as shortname for compatible apps since this won't > currently work with rebranded apps. Something like this seems to have been added however I think there might have been a misunderstanding over this. To make it easy for the client to check against the id returned would have to be the application's guid ({ec8030f7-c20a-464f-9b0e-13a3a9e97384} for Firefox for example). That said, for now the check I ended up using will actually work in Firefox and Minefield alike, I'm not sure how it reacts to rebranded builds like IceWeasel though.

At the moment, the API shows only the first OS as compatible if there are different files for the OS. Example: https://addons.mozilla.org/de/firefox/api/addon/2313/ (Lightning) shows only Linux

I'd suggest that we could close out this bug, I've been opening up a couple on remaining issues with the results, up to you though Laura.

I need to fix the compatible OS stanza, but I think I'll open a separate ticket for that.

(In reply to comment #35) > I need to fix the compatible OS stanza, but I think I'll open a separate ticket > for that. Which bug for this?

Kohei, it's bug 418659