[openstreetmap/openstreetmap-website] Diary entry comment subscriptions (#1309)
Ilya Zverev
notifications at github.com
Tue Oct 4 09:40:16 UTC 2016
Zverik commented on this pull request.
> @@ -57,10 +61,17 @@ def comment
@diary_comment = @entry.comments.build(comment_params)
@diary_comment.user = @user
if @diary_comment.save
- if @diary_comment.user != @entry.user
- Notifier.diary_comment_notification(@diary_comment).deliver_now
+
+ # Notify current subscribers of the new comment
+ @entry.subscribers.visible.each do |user|
+ if @user != user
Should it be `@diary_comment.user`, like in the removed part above?
> end
+ # Add the commenter to the subscribers if necessary
+ @entry.subscribers << @user unless @entry.subscribers.exists?(@user.id)
Why are you adding `@user`, but checking for existence of `@user.id`?
> @@ -69,6 +80,30 @@ def comment
render :action => "no_such_entry", :status => :not_found
end
+ def subscribe
+ diary_entry = DiaryEntry.find(params[:id])
+
+ if ! diary_entry.subscribers.exists?(@user.id)
`unless`?
> @@ -4,6 +4,7 @@ class User < ActiveRecord::Base
has_many :traces, -> { where(:visible => true) }
has_many :diary_entries, -> { order(:created_at => :desc) }
has_many :diary_comments, -> { order(:created_at => :desc) }
+ has_and_belongs_to_many :diary_entries_subscriptions, :class_name => "DiaryEntry", :join_table => "diary_entries_subscribers", :foreign_key => "subscriber_id"
Should foreign key here be something like `diary_entry_id`? Or above, in `diary_entry.rb`, it should be `diary_entry_id`?
> @@ -34,5 +34,6 @@
<%= if_administrator(:li) do %>
<%= link_to t('diary_entry.diary_entry.hide_link'), hide_diary_entry_path(:display_name => diary_entry.user.display_name, :id => diary_entry.id), :method => :post, :data => { :confirm => t('diary_entry.diary_entry.confirm') } %>
<% end %>
+
The whole change for this file is this empty line. Is it right?
> @@ -21,6 +21,11 @@
<%= richtext_area :diary_comment, :body, :cols => 80, :rows => 15 %>
<%= submit_tag t('diary_entry.view.save_button') %>
<% end %>
+ <% if @user and @entry.subscribers.exists?(@user.id) %>
Should we disallow entry authors to unsubscribe to their entries?
--
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#pullrequestreview-2676358
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20161004/94db3266/attachment.html>
More information about the rails-dev
mailing list