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

[Ilya Zverev]

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>