Per the spec, need to output user-created collections in the API format.
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.
Attachment #363334 - Flags: review?(rdoherty) → review-
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
(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 on attachment 363986 [details] [diff] [review] Sharing API patch, now with User model changes Tested and working!
Attachment #363986 - Flags: review?(rdoherty) → review+
Checked into AMO bandwagon as r22611
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
Attachment #363986 - Flags: review?(laura) → review?
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.