[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:
https://github.com/openstreetmap/openstreetmap-website/pull/1309#issuecomment-251378359
-------------- 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