Opened 13 years ago

Closed 12 years ago

#277 closed enhancement (fixed)

Enable "favoriting" of media

Reported by: Aaron Williamson Owned by: Aaron Williamson
Priority: minor Milestone: 0.3.2
Component: programming Keywords: post-sql
Cc: Parent Tickets:

Description (last modified by Christopher Allan Webber)

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,

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

Change History (16)

comment:1 by Aaron Williamson, 13 years ago

Owner: set to Aaron Williamson

comment:1 by Aaron Williamson, 13 years ago

Status: NewFeedback
Merge request:
`\_requests/28 <>`_

comment:2 by Elrond, 13 years ago

Status: FeedbackIn Progress

That looks already great!

I have a few comments/suggestions though:

Could you try the following on media\_favorite?


    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?

comment:3 by Christopher Allan Webber, 12 years ago

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

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

comment:4 by Christopher Allan Webber, 12 years ago

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?

comment:5 by Elrond, 12 years ago

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.

comment:6 by Will Kahn-Greene, 12 years ago

The original url for this bug was .

comment:7 by Elrond, 12 years ago

Keywords: post-sql added
Milestone: 0.2.1
Type: defectenhancement

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.

comment:8 by Aaron Williamson, 12 years ago

Created a merge request to replace "favoriting" and "galleries" with "collecting":

comment:9 by Aaron Williamson, 12 years ago

Owner: changed from Aaron Williamson to Aaron Williamson

comment:10 by Aaron Williamson, 12 years ago

Milestone: 0.3.1

comment:11 by Christopher Allan Webber, 12 years ago

Description: modified (diff)

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

    col = Column('collections', Integer)

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

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:
-    User, MediaEntry, Tag, MediaTag, MediaComment, MediaFile, FileKeynames,
+    User, MediaEntry, Tag, MediaTag, MediaComment, Collection, CollectionItem, MediaFile, FileKeynames,
  • 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(

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

comment:12 by Will Kahn-Greene, 12 years ago

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?

comment:13 by Aaron Williamson, 12 years ago

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.

comment:14 by Aaron Williamson, 12 years ago


comment:15 by joar, 12 years ago

Resolution: fixed
Status: acceptedclosed

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

Note: See TracTickets for help on using tickets.