[S-1.1.3] User-created Feeds

RESOLVED FIXED in BW-M2

Status

addons.mozilla.org Graveyard
Collections
RESOLVED FIXED
10 years ago
2 years ago

People

(Reporter: fligtar, Assigned: lorchard)

Tracking

unspecified
BW-M2

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Per the spec, need to output user-created collections in the API format.
(Assignee)

Comment 1

10 years ago
Created attachment 363334 [details] [diff] [review]
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.
Attachment #363334 - Flags: review?(rdoherty)
Attachment #363334 - Flags: review?(laura)
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
(Assignee)

Comment 3

10 years ago
Created attachment 363986 [details] [diff] [review]
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.
Attachment #363334 - Attachment is obsolete: true
Attachment #363986 - Flags: review?(rdoherty)
Attachment #363986 - Flags: review?(laura)
Attachment #363334 - Flags: review?(laura)
Comment on attachment 363986 [details] [diff] [review]
Sharing API patch, now with User model changes

Tested and working!
Attachment #363986 - Flags: review?(rdoherty) → review+
(Assignee)

Comment 5

10 years ago
Checked into AMO bandwagon as r22611
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

10 years ago
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?
(Assignee)

Updated

10 years ago
Attachment #363986 - Flags: 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.