478125 - [S-1.1.3] User-created Feeds

Status: RESOLVED FIXED

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

Description

[Justin Scott [:fligtar]]

Per the spec, need to output user-created collections in the API format.

Comment 1

[Les Orchard [:lorchard]]

Attachment: Sharing API megapatch

Apologies in advance for this huge patch.  Unlike Ryan, who sensibly posted piecemeal patches as he did the scaffolding work, I went on a tear late last week - and then reworked it all this week once Ryan implemented roles on collections.

This patch should apply on the bandwagon branch of addons.  

It includes a test suite at /en-US/firefox/tests?case=controllers%2Fsharing_api_controller.test.php

To use the tests, you'll need to switch to a test database using a clean import of the remora schema and remora test data.  All of the tests should pass, except for a handful or so at the end that are placeholders for work I still have to finish or verify.

To exercise the API by hand, I added some walkthrough docs to the spec:

https://wiki.mozilla.org/User:LesOrchard/BandwagonAPI#Examples_.2F_Walkthrough

Sending an r? to both Ryan and Laura, hoping one or both of you guys have time to subject yourselves to this patch.

Comment 2

[Ryan Doherty (:rdoherty)]

Comment on attachment 363334 [details] [diff] [review]
Sharing API megapatch

Looks pretty good overall, r-'ing it because I'm getting an undefined index error here:

site/app/controllers/sharing_api_controller.php on line 166

which references $this->User->hasAndBelongsToMany_full['Collections'],

I didn't see any changes to the User model in the patch that would put 'Collections' into hasAndBelongsToMany_full.

And here are my minor quibbles/questions:

-It doesn't seem like there is any permissions checking when deleting a collection
-I saw  ( empty($_SERVER['HTTPS']) ? 'http' : 'https' ) repeated a few times when creating urls, should it be part of resolveUrl?
-I think $_SERVER['HTTP_HOST'] could be the SITE_URL constant
-Why does publishCollections return collections and publish them? It just seems a little weird to me, but I could be wrong.
-getAddonsForView is 200 lines long, it seems like it could benefit from being split up into a few functions.
-deleteByAddonIdAndCollectionId should return a boolean

Comment 3

[Les Orchard [:lorchard]]

Attachment: Sharing API patch, now with User model changes

(In reply to comment #2)
> (From update of attachment 363334 [details] [diff] [review])

> I didn't see any changes to the User model in the patch that would put
> 'Collections' into hasAndBelongsToMany_full.

Doh.  Looks like I left that file out of my patch tracking.  Just added and
will upload a new patch.

> -It doesn't seem like there is any permissions checking when deleting a
> collection

The permission checking is done up in the collection_detail() function before
dispatching to either PUT or DELETE, by using isWriteHttpMethod to detect a 
write and checking the Collection model's isWritableByUser method.

> -I saw  ( empty($_SERVER['HTTPS']) ? 'http' : 'https' ) repeated a few times
> when creating urls, should it be part of resolveUrl?

resolveUrl is used to resolve a relative URL with respect to any given absolute
URL.  Really, resolveUrl should be moved out of the controller into a utility
library at some point if we end up doing more API work like this.

But, $_SERVER['HTTPS'] is part of determining the absolute base URL for
the current specific request.  Sticking that into resolveUrl would tie that
utility method to the current request, rather than leaving it a general purpose
utility.

> -I think $_SERVER['HTTP_HOST'] could be the SITE_URL constant

The SITE_URL constant doesn't reflect whether or not HTTPS is in effect, which
I think happens when the API is used in production.  HTTPS + HTTP_HOST seemed
more accurate to what was really happening for a given request.

> -Why does publishCollections return collections and publish them? It just seems
> a little weird to me, but I could be wrong.

It publishes them just because at one point I was always immediately publishing
the return value anyway, so I just joined the two steps in that one method.
I guess it It still returns them as an artifact of that change, and I figured
it might be useful at some point.

> -getAddonsForView is 200 lines long, it seems like it could benefit from being
> split up into a few functions.

That method is one that I lifted mostly from the original API controller, with
some tweaks and reformatting.  It probably should be split up, but it produces
the correct format, and I don't entirely understand that format well enough
to mess with it.  That's one thing I don't have a spec for.

> -deleteByAddonIdAndCollectionId should return a boolean

Changed.

Comment 4

[Ryan Doherty (:rdoherty)]

Comment on attachment 363986 [details] [diff] [review]
Sharing API patch, now with User model changes

Tested and working!

Comment 5

[Les Orchard [:lorchard]]

Checked into AMO bandwagon as r22611

Comment 6

[Les Orchard [:lorchard]]

Comment on attachment 363986 [details] [diff] [review]
Sharing API patch, now with User model changes

Cancelling review for laura - comments would still be welcomed, but checking this thing in so we can get it staged sooner.