399913 - Add API call for retrieving addon details

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: API, defect)

Description

[Laura Thomson :laura]

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

Comment 1

[Laura Thomson :laura]

Attachment: patch for this api call

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

Comment 2

[Wil Clouser [:clouserw]]

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.

Comment 3

[Wil Clouser [:clouserw]]

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 4

[Wil Clouser [:clouserw]]

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.

Comment 5

[Fred Wenzel [:wenzel]]

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

Comment 6

[Laura Thomson :laura]

Attachment: revised patch

Changes made as requested, added hash, added reviews.

Comment 7

[Wil Clouser [:clouserw]]

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?

Comment 8

[Basil Hashem [:baz]]

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.

Comment 9

[Dave Townsend [:mossop]]

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).

Comment 10

[Dave Townsend [:mossop]]

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

Comment 11

[Laura Thomson :laura]

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

Comment 12

[Dave Townsend [:mossop]]

(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

Comment 13

[Laura Thomson :laura]

Doh, ok.  My notes suck.

Comment 14

[Dave Townsend [:mossop]]

Laura, is there any eta on these updates?

Comment 15

[Laura Thomson :laura]

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

Comment 16

[Dave Townsend [:mossop]]

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

Comment 17

[Laura Thomson :laura]

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

Comment 18

[Laura Thomson :laura]

Icon and summary still not correct, need to fix.

Comment 19

[Laura Thomson :laura]

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.

Comment 20

[Dave Townsend [:mossop]]

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.

Comment 21

[Dave Townsend [:mossop]]

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

Comment 22

[Dave Townsend [:mossop]]

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

Comment 23

[Laura Thomson :laura]

Working my way through these issues...

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 25

[Dave Townsend [:mossop]]

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

Comment 26

[Laura Thomson :laura]

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?

Comment 27

[Dave Townsend [:mossop]]

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

Comment 28

[Dave Townsend [:mossop]]

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.

Comment 29

[Laura Thomson :laura]

r9618 fixes the URL and redirect issues.

Comment 30

[Laura Thomson :laura]

r9619 fixes the rating issue.

Comment 31

[Laura Thomson :laura]

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

Comment 32

[Dave Townsend [:mossop]]

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

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 34

[Dave Townsend [:mossop]]

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.

Comment 35

[Laura Thomson :laura]

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

Comment 36

[Kohei Yoshino [:kohei]]

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

Comment 37

[Basil Hashem [:baz]]

Kohei, it's bug 418659