WIP diary comment subscriptions

app/controllers/diary_entry_controller.rb

@@ -3,10 +3,10 @@ class DiaryEntryController < ApplicationController
 
   before_action :authorize_web
   before_action :set_locale
-  before_action :require_user, :only => [:new, :edit, :comment, :hide, :hidecomment]
+  before_action :require_user, :only => [:new, :edit, :comment, :hide, :hidecomment, :subscribe, :unsubscribe]
   before_action :lookup_this_user, :only => [:view, :comments]
   before_action :check_database_readable
-  before_action :check_database_writable, :only => [:new, :edit]
+  before_action :check_database_writable, :only => [:new, :edit, :comment, :hide, :hidecomment, :subscribe, :unsubscribe]
   before_action :require_administrator, :only => [:hide, :hidecomment]
 
   def new
@@ -24,6 +24,10 @@ class DiaryEntryController < ApplicationController
         else
           @user.preferences.create(:k => "diary.default_language", :v => @diary_entry.language_code)
         end
+
+        # Subscribe user to diary comments
+        @diary_entry.subscribers << @user
+
         redirect_to :controller => "diary_entry", :action => "list", :display_name => @user.display_name
       else
         render :action => "edit"
@@ -57,10 +61,17 @@ class DiaryEntryController < ApplicationController
     @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
+          Notifier.diary_comment_notification(@diary_comment, user).deliver_now
+        end
       end
 
+      # Add the commenter to the subscribers if necessary
+      @entry.subscribers << @user unless @entry.subscribers.exists?(@user.id)
+
       redirect_to :controller => "diary_entry", :action => "view", :display_name => @entry.user.display_name, :id => @entry.id
     else
       render :action => "view"
@@ -69,6 +80,24 @@ class DiaryEntryController < ApplicationController
     render :action => "no_such_entry", :status => :not_found
   end
 
+  def subscribe
+    @entry = DiaryEntry.find(params[:id])
+
+    if ! diary_entry.subscribers.exists?(@user.id)
+      diary_entry.subscribers << @user
+
+    redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id
+  end
+
+  def unsubscribe
+    @entry = DiaryEntry.find(params[:id])
+
+    if diary_entry.subscribers.exists?(@user.id)
+      diary_entry.subscribers.delete(@user)
+
+    redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id
+  end
+
   def list
     if params[:display_name]
       @this_user = User.active.find_by_display_name(params[:display_name])

app/models/diary_entry.rb

@@ -4,6 +4,7 @@ class DiaryEntry < ActiveRecord::Base
 
   has_many :comments, -> { order(:id).preload(:user) }, :class_name => "DiaryComment"
   has_many :visible_comments, -> { joins(:user).where(:visible => true, :users => { :status => %w(active confirmed) }).order(:id) }, :class_name => "DiaryComment"
+  has_and_belongs_to_many :subscribers, :class_name => "User", :join_table => "diary_entries_subscribers", :association_foreign_key => "subscriber_id"
 
   scope :visible, -> { where(:visible => true) }
 

app/models/notifier.rb

@@ -83,9 +83,10 @@ class Notifier < ActionMailer::Base
     end
   end
 
-  def diary_comment_notification(comment)
+  # FIXME mail should say your / their depending who's message it is
-    with_recipient_locale comment.diary_entry.user do
+  def diary_comment_notification(comment, recipient)
-      @to_user = comment.diary_entry.user.display_name
+    with_recipient_locale recipient do
+      @to_user = recipient.display_name
       @from_user = comment.user.display_name
       @text = comment.body
       @title = comment.diary_entry.title
@@ -108,7 +109,7 @@ class Notifier < ActionMailer::Base
                           :title => "Re: #{comment.diary_entry.title}")
 
       mail :from => from_address(comment.user.display_name, "c", comment.id, comment.digest),
-           :to => comment.diary_entry.user.email,
+           :to => recipient.email,
            :subject => I18n.t("notifier.diary_comment_notification.subject", :user => comment.user.display_name)
     end
   end

app/models/user.rb

@@ -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"
   has_many :messages, -> { where(:to_user_visible => true).order(:sent_on => :desc).preload(:sender, :recipient) }, :foreign_key => :to_user_id
   has_many :new_messages, -> { where(:to_user_visible => true, :message_read => false).order(:sent_on => :desc) }, :class_name => "Message", :foreign_key => :to_user_id
   has_many :sent_messages, -> { where(:from_user_visible => true).order(:sent_on => :desc).preload(:sender, :recipient) }, :class_name => "Message", :foreign_key => :from_user_id

app/views/diary_entry/_diary_entry.html.erb

@@ -34,5 +34,12 @@
     <%= 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 %>
+
+    <% if @user and diary_entry.subscribers.exists?(@user.id) %>
+      <li><%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_url(diary_entry), :method => :post %></li>
+    <% elsif @user %>
+      <li><%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_url(diary_entry), :method => :post %></li>
+    <% end %>
+
   </ul>
 </div>

config/routes.rb

@@ -228,6 +228,8 @@ OpenStreetMap::Application.routes.draw do
   match "/user/:display_name/diary/:id/edit" => "diary_entry#edit", :via => [:get, :post], :id => /\d+/
   match "/user/:display_name/diary/:id/hide" => "diary_entry#hide", :via => :post, :id => /\d+/, :as => :hide_diary_entry
   match "/user/:display_name/diary/:id/hidecomment/:comment" => "diary_entry#hidecomment", :via => :post, :id => /\d+/, :comment => /\d+/, :as => :hide_diary_comment
+  match "/user/:display_name/diary/:id/subscribe" => "diary_entry#subscribe", :via => :post, :as => :diary_entry_subscribe, :id => /\d+/
+  match "/user/:display_name/diary/:id/unsubscribe" => "diary_entry#unsubscribe", :via => :post, :as => :diary_entry_unsubscribe, :id => /\d+/
 
   # user pages
   match "/user/:display_name" => "user#view", :via => :get, :as => "user"

db/migrate/20161002153425_add_join_table_between_users_and_diary_entries.rb

@@ -0,0 +1,23 @@
+class AddJoinTableBetweenUsersAndDiaryEntries < ActiveRecord::Migration
+  def change
+    create_table :diary_entries_subscribers, :id => false do |t|
+      t.column :subscriber_id, :bigint, :null => false
+      t.column :diary_entry_id, :bigint, :null => false
+    end
+
+    add_foreign_key :diary_entries_subscribers, :users, :column => :subscriber_id, :name => "diary_entries_subscribers_subscriber_id_fkey"
+    add_foreign_key :diary_entries_subscribers, :diary_entries, :column => :diary_entry_id, :name => "diary_entries_subscribers_changeset_id_fkey"
+
+    add_index :diary_entries_subscribers, [:subscriber_id, :diary_entry_id], :unique => true, :name => "index_diary_subscribers_on_subscriber_id_and_diary_id"
+    add_index :diary_entries_subscribers, [:diary_entry_id]
+  end
+
+  def up
+    DiaryEntry.find_each do |diary_entry|
+      diary_entry.subscribers << diary_entry.user unless diary_entry.subscribers.exists?(diary_entry.user.id)
+    end
+  end
+
+  def down
+  end
+end

test/controllers/diary_entry_controller_test.rb

@@ -83,6 +83,14 @@ class DiaryEntryControllerTest < ActionController::TestCase
       { :path => "/user/username/diary/1/hidecomment/2", :method => :post },
       { :controller => "diary_entry", :action => "hidecomment", :display_name => "username", :id => "1", :comment => "2" }
     )
+    assert_routing(
+      { :path => "/user/username/diary/1/subscribe", :method => :post },
+      { :controller => "diary_entry", :action => "subscribe", :id => "1" }
+    )
+    assert_routing(
+      { :path => "/user/username/diary/1/unsubscribe", :method => :post },
+      { :controller => "diary_entry", :action => "unsubscribe", :id => "1" }
+    )
   end
 
   def test_new
@@ -148,6 +156,9 @@ class DiaryEntryControllerTest < ActionController::TestCase
     assert_equal new_longitude.to_f, entry.longitude
     assert_equal new_language_code, entry.language_code
 
+    # checks if user was subscribed
+    assert_equal 1, entry.subscribers.length
+
     assert_equal new_language_code, UserPreference.where(:user_id => users(:normal_user).id, :k => "diary.default_language").first.v
 
     new_language_code = "de"
@@ -169,6 +180,9 @@ class DiaryEntryControllerTest < ActionController::TestCase
     assert_equal new_longitude.to_f, entry.longitude
     assert_equal new_language_code, entry.language_code
 
+    # checks if user was subscribed
+    assert_equal 1, entry.subscribers.length
+
     assert_equal new_language_code, UserPreference.where(:user_id => users(:normal_user).id, :k => "diary.default_language").first.v
   end
 
@@ -316,6 +330,7 @@ class DiaryEntryControllerTest < ActionController::TestCase
       assert_select "h2", :text => "No entry with the id: 9999", :count => 1
     end
 
+    # FIXME assert number of subscribers
     # Now try an invalid comment with an empty body
     assert_no_difference "ActionMailer::Base.deliveries.size" do
       assert_no_difference "DiaryComment.count" do
@@ -325,6 +340,7 @@ class DiaryEntryControllerTest < ActionController::TestCase
     assert_response :success
     assert_template :view
 
+    # FIXME assert number of subscribers
     # Now try again with the right id
     assert_difference "ActionMailer::Base.deliveries.size", 1 do
       assert_difference "DiaryComment.count", 1 do
@@ -344,6 +360,8 @@ class DiaryEntryControllerTest < ActionController::TestCase
     assert_equal users(:public_user).id, comment.user_id
     assert_equal "New comment", comment.body
 
+    # FIXME check number of subscribers
+
     # Now view the diary entry, and check the new comment is present
     get :view, :display_name => entry.user.display_name, :id => entry.id
     assert_response :success
@@ -362,6 +380,7 @@ class DiaryEntryControllerTest < ActionController::TestCase
     # Generate some spammy content
     spammy_text = 1.upto(50).map { |n| "http://example.com/spam#{n}" }.join(" ")
 
+    # FIXME assert number of subscribers
     # Try creating a spammy comment
     assert_difference "ActionMailer::Base.deliveries.size", 1 do
       assert_difference "DiaryComment.count", 1 do
@@ -641,6 +660,96 @@ class DiaryEntryControllerTest < ActionController::TestCase
     assert_response :not_found
   end
 
+  ##
+  # test subscribe success
+  def test_subscribe_success
+    basic_authorization(users(:public_user).email, "test")
+    changeset = changesets(:normal_user_closed_change)
+
+    assert_difference "changeset.subscribers.count", 1 do
+      post :subscribe, :id => changeset.id
+    end
+    assert_response :success
+  end
+
+  ##
+  # test subscribe fail
+  def test_subscribe_fail
+    # unauthorized
+    changeset = changesets(:normal_user_closed_change)
+    assert_no_difference "changeset.subscribers.count" do
+      post :subscribe, :id => changeset.id
+    end
+    assert_response :unauthorized
+
+    basic_authorization(users(:public_user).email, "test")
+
+    # bad changeset id
+    assert_no_difference "changeset.subscribers.count" do
+      post :subscribe, :id => 999111
+    end
+    assert_response :not_found
+
+    # not closed changeset
+    changeset = changesets(:normal_user_first_change)
+    assert_no_difference "changeset.subscribers.count" do
+      post :subscribe, :id => changeset.id
+    end
+    assert_response :conflict
+
+    # trying to subscribe when already subscribed
+    changeset = changesets(:normal_user_subscribed_change)
+    assert_no_difference "changeset.subscribers.count" do
+      post :subscribe, :id => changeset.id
+    end
+    assert_response :conflict
+  end
+
+  ##
+  # test unsubscribe success
+  def test_unsubscribe_success
+    basic_authorization(users(:public_user).email, "test")
+    changeset = changesets(:normal_user_subscribed_change)
+
+    assert_difference "changeset.subscribers.count", -1 do
+      post :unsubscribe, :id => changeset.id
+    end
+    assert_response :success
+  end
+
+  ##
+  # test unsubscribe fail
+  def test_unsubscribe_fail
+    # unauthorized
+    changeset = changesets(:normal_user_closed_change)
+    assert_no_difference "changeset.subscribers.count" do
+      post :unsubscribe, :id => changeset.id
+    end
+    assert_response :unauthorized
+
+    basic_authorization(users(:public_user).email, "test")
+
+    # bad changeset id
+    assert_no_difference "changeset.subscribers.count" do
+      post :unsubscribe, :id => 999111
+    end
+    assert_response :not_found
+
+    # not closed changeset
+    changeset = changesets(:normal_user_first_change)
+    assert_no_difference "changeset.subscribers.count" do
+      post :unsubscribe, :id => changeset.id
+    end
+    assert_response :conflict
+
+    # trying to unsubscribe when not subscribed
+    changeset = changesets(:normal_user_closed_change)
+    assert_no_difference "changeset.subscribers.count" do
+      post :unsubscribe, :id => changeset.id
+    end
+    assert_response :not_found
+  end
+
   private
 
   def check_diary_list(*entries)