<p><b>@Zverik</b> commented on this pull request.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358">app/controllers/diary_entry_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>Should it be <code>@diary_comment.user</code>, like in the removed part above?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358">app/controllers/diary_entry_controller.rb</a>:</p>
<pre style='color:#555'>>        end
 
+      # Add the commenter to the subscribers if necessary
+      @entry.subscribers << @user unless @entry.subscribers.exists?(@user.id)
</pre>
<p>Why are you adding <code>@user</code>, but checking for existence of <code>@user.id</code>?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358">app/controllers/diary_entry_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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)
</pre>
<p><code>unless</code>?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358">app/models/user.rb</a>:</p>
<pre style='color:#555'>> @@ -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"
</pre>
<p>Should foreign key here be something like <code>diary_entry_id</code>? Or above, in <code>diary_entry.rb</code>, it should be <code>diary_entry_id</code>?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358">app/views/diary_entry/_diary_entry.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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 %>
+
</pre>
<p>The whole change for this file is this empty line. Is it right?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358">app/views/diary_entry/view.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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) %>
</pre>
<p>Should we disallow entry authors to unsubscribe to their entries?</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABWnLZMxeVzsC5GeVefv9A7pnTHj0xzLks5qwh8AgaJpZM4KNPYt">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABWnLdlFbhGY_r4lzjSfigolFhNS2LX0ks5qwh8AgaJpZM4KNPYt.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
  <link itemprop="url" href="https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358"></link>
  <meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/openstreetmap/openstreetmap-website","title":"openstreetmap/openstreetmap-website","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/openstreetmap/openstreetmap-website"}},"updates":{"snippets":[{"icon":"PERSON","message":"@Zverik commented on #1309"}],"action":{"name":"View Pull Request","url":"https://github.com/openstreetmap/openstreetmap-website/pull/1309#pullrequestreview-2676358"}}}</script>