[openstreetmap/openstreetmap-website] Diary entry comment subscriptions (#1309)

Andy Allan notifications at github.com
Tue Oct 4 12:49:50 UTC 2016

My main concern would be to rewrite the subscribers table to be more idiomatic, namely to have a "diary_entry_subscriptions" table with user_id and diary_entry_id. Note the change from _subscribers to _subscriptions is subtle but reasonably important. You can create and delete a subscription, for example, and that makes more sense than to "create a subscriber".

This also allows the relationships to be easier to understand, e.g. a diary_entry `has_many :subscriptions, :through => :diary_entry_subscriptions` and `has_many :subscribers, :through => :subscriptions` or similar. A User `has_many :subscribed_diaries, :through => :diary_entry_subscriptions` etc. Then diary_entry.subscribers would still be a list of users, for example. Having to specify the :join_table and :association_foreign_key is a code smell, so best avoided, especially given we're creating a fresh table rather than dealing with a legacy structure.

The @entry.subscribers << @user is also non-idiomatic. Something more like @entry.subscriptions.create(user: @user) would be preferred.

Anything involving `user.id` in the controllers should be avoided - again, it's a minor code smell, and there's very likely to be an alternative approach that just uses the User objects.

If you want to go full-on clean-code, then the subscribe and unsubscribe POST targets should be handled by a DiaryEntrySubscriptionsController, which would have just `create` and `delete` methods (i.e. you are creating and deleting subscription objects) rather than adding even more non-standard methods to the DiaryEntryController. Let me know if you want to discuss this further.

I realise that many non-idiomatic approaches are used throughout the codebase, but I'd like to not let the situation get any worse!

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20161004/fd6842d3/attachment.html>

More information about the rails-dev mailing list