Enable "favoriting" of media

[Aaron Williamson]

Users should be able to mark media entries as favorites by clicking
a button/link in the entry detail view (and probably from gallery
views as well). This change will likely require the creation of a
new "UserFavorite" model with a structure like:

{'user': user\_id,
'media\_entry': media\_entry\_id,
'created': datetime.datetime.now()}

...and also a new field on MediaEntry containing a count of
UserFavorites for that entry (to avoid expensive queries to
generate this number dynamically for every request).

[Aaron Williamson]

Merge request:
`https://gitorious.org/mediagoblin/mediagoblin/merge\_requests/28 <https://gitorious.org/mediagoblin/mediagoblin/merge_requests/28>`_

[Elrond]

Hi!

That looks already great!

I have a few comments/suggestions though:

Could you try the following on media\_favorite?

::

    @get_user_media_entry
    @require_active_login
    def media_favorite(request, media_entry):

... and use ``media_entry_id = media['_id']`` then? It should
handle bad media\_ids better.

You added this piece of code:

::

        # See if the user has favorited this media
        user = request.db.User.find_one({
                'username': request.matchdict['user']})

I can't see, where you use ``user`` afterwards, can you help me?

And finally, there is a bonus to get:
When deleting a media, could you make sure that all the favorites
for that media are also deleted?

[Christopher Allan Webber]

I'm reviewing now also.


-  What Elrond said re: the user assignment. Looks like you're
   using request.user (which is appropriate!), so that bit isn't
   needed.
-  Looking at the media\_favorite... that looks great mostly!
   Except one thing... there's a few confusing things about getting
   the media\_entry\_id and etc.
   
   -  First of all, there's an unused media\_entry assignment here.
   -  Secondly, it looks like the way the media entry id is being
      gotten is via the URL component. What this means is that
      /u/cwebber/m/media-objectid-here/favorite/ will work, but
      /u/cwebber/m/slug-id/favorite/ will not. Here's a simple solution!
      Use the @get\_user\_media\_entry decorator, that wasy it'll allow
      either.

-  I was going to say maybe you could trick users into favoriting,
   but I guess since you're using POST that's prevented! Nice work :)
-  media\_favorite has a lot of stuff that doesn't really comply to
   PEP-8 / is over 80 characters.
-  Tests look great!

Lastly, do we want to add a "favorited media" view, or maybe a
"recently favorited" stuff to the user's homepage at least? If we
do, we might have to update the pagination, which currently expects
a mongodb cursor, whereas we'd be not having a cursor of the media
entries themselves but of the favorites... so we'd probably need to
turn the current page into a list. Probably not too hard, might
require a small amount of refactoring.

[Christopher Allan Webber]

I've also gotten a bit fuzzy on remembering what the state of this
is. Presumably per our last IRC conversations, it's got the right
information, it's just not really being used, yeah?

It would be great to merge this into this month's release. Is there
something I can do to help?

[Elrond]

Okay, didn't happen for 0.2.0.

Hopefully we'll get it in for the next release!

I remember, that favoriting worked, just there was no way to see,
what one favorited, and who favorited a given media.
Assuming this is correct and everything else works, I'd vote for
merging it anyway "as is" and then go on from there.

[Will Kahn-Greene]

The original url for this bug was ​http://bugs.foocorp.net/issues/618 .

[Elrond]

I think, there was code to do this on mongodb.
But as sql is sooo close, *I* think we should try to keep changes to mongo structure small.
So tagging this for post-sql.
And removed any version info until we know about versions again.

[Aaron Williamson]

Created a merge request to replace "favoriting" and "galleries" with "collecting": ​https://gitorious.org/mediagoblin/mediagoblin/merge_requests/41

[Christopher Allan Webber]

(this review below mostly done from a car, sorry for annoyingness... the questions still apply:)

This is a very brief review without being able to test it. I hope to do an actually-test-it review shortly. Here's from a code skim though:

• What's this "association_proxy"?

    collections_helper = relationship("CollectionItem",
        cascade="all, delete-orphan"
        )
    collections = association_proxy("collections_helper", "in_collection")

Relatedly, I'm confused by:

@RegisterMigration(4, MIGRATIONS)
def add_mediaentry_collections(db_conn):
    metadata = MetaData(bind=db_conn.bind)

    media_entry = Table('core__media_entries', metadata, autoload=True,
           autoload_with=db_conn.bind)

    col = Column('collections', Integer)
    col.create(media_entry)
    db_conn.commit()

... why an integer?

I guess I wonder why each collection doesn't have a foreignkey to the MediaEntry?

Admittedly I don't know anything about association_proxy and am writing this when on my laptop without intarwebs access. :)

... okay, I've read more, and ​http://docs.sqlalchemy.org/en/rel_0_7/orm/extensions/associationproxy.html

does seem like a really cool interface. Is the coolness of that interface the primary reason for the proxy table though? I somewhat wonder if you couldn't get something similar with general sqlalchemy relationships assuming it linked directly to the table.

See below for me wondering if this has anticipated extra extensibility beyond MediaEntry stuff...

• I notice there's a Collection and CollectionItem table, which is pretty awesome. But I also notice there's a collections mixin. Not that it's wrong, just trying to understand... why the mixin? Do we anticipate more things than these two tables to use this functionality?

Premature abstraction might be right or wrong, not sure. Would be useful to know your thinking.

• Style nitpick... should wrap at 80 characters:

 MODELS = [
-    User, MediaEntry, Tag, MediaTag, MediaComment, MediaFile, FileKeynames,
+    User, MediaEntry, Tag, MediaTag, MediaComment, Collection, CollectionItem, MediaFile, FileKeynames,
     MediaAttachmentFile]

• Again here:

  form.collection.query = request.db.Collection.query.filter(filt).order_by(request.db.Collection.title)

could be done as:

  form.collection.query = request.db.Collection.query.filter(
      filt).order_by(request.db.Collection.title)

Overall this looks really promising, and I'm definitely excited about it! :)

[Will Kahn-Greene]

Did this land in 0.3.1? I don't think it did.

Is this something we can finish up in the next couple of weeks so it can land in 0.3.2?

[Aaron Williamson]

Yes, joar has reviewed it and I think we've sorted out all of the bugs. I believe all that's left is for him to merge it.

[joar]

Aaron fixed some additinal bugs I found, and now it's merged!