Merge remote-tracking branch 'openstreetmap/pull/1309'

app/assets/stylesheets/common.scss

@@ -1679,6 +1679,13 @@ tr.turn:hover {
   float: left;
 }
 
+
+.diary-subscribe-buttons {
+  position:relative;
+  top: -30px;
+  left: 130px;
+}
+
 /* Rules for the log in page */
 
 #login_auth_buttons {

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.subscriptions.create(:user => @user)
+
         redirect_to :action => "list", :display_name => @user.display_name
       else
         render :action => "edit"
@@ -57,9 +61,16 @@ 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.subscriptions.create(:user => @user) unless @entry.subscribers.exists?(@user.id)
 
       redirect_to :action => "view", :display_name => @entry.user.display_name, :id => @entry.id
     else
@@ -69,6 +80,26 @@ class DiaryEntryController < ApplicationController
     render :action => "no_such_entry", :status => :not_found
   end
 
+  def subscribe
+    diary_entry = DiaryEntry.find(params[:id])
+
+    diary_entry.subscriptions.create(:user => @user) unless diary_entry.subscribers.exists?(@user.id)
+
+    redirect_to :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id
+  rescue ActiveRecord::RecordNotFound
+    render :action => "no_such_entry", :status => :not_found
+  end
+
+  def unsubscribe
+    diary_entry = DiaryEntry.find(params[:id])
+
+    diary_entry.subscriptions.where(:user => @user).delete_all if diary_entry.subscribers.exists?(@user.id)
+
+    redirect_to :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id
+  rescue ActiveRecord::RecordNotFound
+    render :action => "no_such_entry", :status => :not_found
+  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,8 @@ 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_many :subscriptions, :class_name => "DiaryEntrySubscription"
+  has_many :subscribers, :through => :subscriptions, :source => :user
 
   scope :visible, -> { where(:visible => true) }
 

app/models/diary_entry_subscription.rb

@@ -0,0 +1,4 @@
+class DiaryEntrySubscription < ActiveRecord::Base
+  belongs_to :user
+  belongs_to :diary_entry
+end

app/models/notifier.rb

@@ -83,9 +83,9 @@ class Notifier < ActionMailer::Base
     end
   end
 
-  def diary_comment_notification(comment)
-    with_recipient_locale comment.diary_entry.user do
-      @to_user = comment.diary_entry.user.display_name
+  def diary_comment_notification(comment, recipient)
+    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 +108,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,8 @@ 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_many :diary_entry_subscriptions, :class_name => "DiaryEntrySubscription"
+  has_many :diary_subscriptions, :through => :diary_entry_subscriptions, :source => :diary_entry
   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/view.html.erb

@@ -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) %>
+    <div class="diary-subscribe-buttons"><%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %></div>
+  <% elsif @user %>
+    <div class="diary-subscribe-buttons"><%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %></div>
+  <% end %>
 <% end %>
 
 <%= if_not_logged_in(:div) do %>

config/locales/en.yml

@@ -1235,9 +1235,9 @@ en:
     partners_title: Partners
   notifier:
     diary_comment_notification:
-      subject: "[OpenStreetMap] %{user} commented on your diary entry"
+      subject: "[OpenStreetMap] %{user} commented on a diary entry"
       hi: "Hi %{to_user},"
-      header: "%{from_user} has commented on your recent OpenStreetMap diary entry with the subject %{subject}:"
+      header: "%{from_user} has commented on the OpenStreetMap diary entry with the subject %{subject}:"
       footer: "You can also read the comment at %{readurl} and you can comment at %{commenturl} or reply at %{replyurl}"
     message_notification:
       subject_header: "[OpenStreetMap] %{subject}"

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,19 @@
+require "migrate"
+
+class AddJoinTableBetweenUsersAndDiaryEntries < ActiveRecord::Migration
+  def self.up
+    create_table :diary_entry_subscriptions, :id => false do |t|
+      t.column :user_id, :bigint, :null => false
+      t.column :diary_entry_id, :bigint, :null => false
+    end
+
+    add_primary_key :diary_entry_subscriptions, [:user_id, :diary_entry_id]
+    add_index :diary_entry_subscriptions, [:diary_entry_id]
+    add_foreign_key :diary_entry_subscriptions, :diary_entries, :name => "diary_entry_subscriptions_diary_entry_id_fkey"
+    add_foreign_key :diary_entry_subscriptions, :users, :name => "diary_entry_subscriptions_user_id_fkey"
+  end
+
+  def self.down
+    drop_table :diary_entry_subscriptions
+  end
+end

db/migrate/20161011010929_subscribe_authors_to_diary_entries.rb

@@ -0,0 +1,10 @@
+class SubscribeAuthorsToDiaryEntries < ActiveRecord::Migration
+  def up
+    DiaryEntry.find_each do |diary_entry|
+      diary_entry.subscriptions.create(:user => diary_entry.user) unless diary_entry.subscribers.exists?(diary_entry.user.id)
+    end
+  end
+
+  def down
+  end
+end

db/structure.sql

@@ -549,6 +549,14 @@ CREATE SEQUENCE diary_entries_id_seq
 
 ALTER SEQUENCE diary_entries_id_seq OWNED BY diary_entries.id;
 
+-- Name: diary_entry_subscriptions; Type: TABLE; Schema: public; Owner: -
+--
+
+CREATE TABLE diary_entry_subscriptions (
+    user_id bigint NOT NULL,
+    diary_entry_id bigint NOT NULL
+);
+
 
 --
 -- Name: friends; Type: TABLE; Schema: public; Owner: -
@@ -1448,6 +1456,14 @@ ALTER TABLE ONLY diary_entries
     ADD CONSTRAINT diary_entries_pkey PRIMARY KEY (id);
 
 
+--
+-- Name: diary_entry_subscriptions_pkey; Type: CONSTRAINT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY diary_entry_subscriptions
+    ADD CONSTRAINT diary_entry_subscriptions_pkey PRIMARY KEY (user_id, diary_entry_id);
+
+
 --
 -- Name: friends_pkey; Type: CONSTRAINT; Schema: public; Owner: -
 --
@@ -1828,6 +1844,11 @@ CREATE UNIQUE INDEX index_changesets_subscribers_on_subscriber_id_and_changeset_
 CREATE UNIQUE INDEX index_client_applications_on_key ON client_applications USING btree (key);
 
 
+-- Name: index_diary_entry_subscriptions_on_diary_entry_id; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE INDEX index_diary_entry_subscriptions_on_diary_entry_id ON diary_entry_subscriptions USING btree (diary_entry_id);
+
 --
 -- Name: index_note_comments_on_body; Type: INDEX; Schema: public; Owner: -
 --
@@ -2212,6 +2233,22 @@ ALTER TABLE ONLY diary_entries
     ADD CONSTRAINT diary_entries_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id);
 
 
+-- Name: diary_entry_subscriptions_diary_entry_id_fkey; Type: FK CONSTRAINT; Schema: public; Owner: -
+--
+--
+
+ALTER TABLE ONLY diary_entry_subscriptions
+    ADD CONSTRAINT diary_entry_subscriptions_diary_entry_id_fkey FOREIGN KEY (diary_entry_id) REFERENCES diary_entries(id);
+
+
+--
+-- Name: diary_entry_subscriptions_user_id_fkey; Type: FK CONSTRAINT; Schema: public; Owner: -
+--
+
+ALTER TABLE ONLY diary_entry_subscriptions
+    ADD CONSTRAINT diary_entry_subscriptions_user_id_fkey FOREIGN KEY (user_id) REFERENCES users(id);
+
+
 --
 -- Name: friends_friend_user_id_fkey; Type: FK CONSTRAINT; Schema: public; Owner: -
 --
@@ -2643,4 +2680,3 @@ INSERT INTO schema_migrations (version) VALUES ('7');
 INSERT INTO schema_migrations (version) VALUES ('8');
 
 INSERT INTO schema_migrations (version) VALUES ('9');
-

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", :display_name => "username", :id => "1" }
+    )
+    assert_routing(
+      { :path => "/user/username/diary/1/unsubscribe", :method => :post },
+      { :controller => "diary_entry", :action => "unsubscribe", :display_name => "username", :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,26 +330,32 @@ class DiaryEntryControllerTest < ActionController::TestCase
       assert_select "h2", :text => "No entry with the id: 9999", :count => 1
     end
 
+    post :subscribe, { :id => entry.id, :display_name => entry.user.display_name }, { :user => users(:normal_user).id }
+
     # Now try an invalid comment with an empty body
     assert_no_difference "ActionMailer::Base.deliveries.size" do
       assert_no_difference "DiaryComment.count" do
+        assert_no_difference "entry.subscribers.count" do
           post :comment, { :display_name => entry.user.display_name, :id => entry.id, :diary_comment => { :body => "" } }, { :user => users(:public_user).id }
         end
       end
+    end
     assert_response :success
     assert_template :view
 
     # Now try again with the right id
-    assert_difference "ActionMailer::Base.deliveries.size", 1 do
+    assert_difference "ActionMailer::Base.deliveries.size", entry.subscribers.count do
       assert_difference "DiaryComment.count", 1 do
+        assert_difference "entry.subscribers.count", 1 do
           post :comment, { :display_name => entry.user.display_name, :id => entry.id, :diary_comment => { :body => "New comment" } }, { :user => users(:public_user).id }
         end
       end
+    end
     assert_response :redirect
     assert_redirected_to :action => :view, :display_name => entry.user.display_name, :id => entry.id
     email = ActionMailer::Base.deliveries.first
     assert_equal [users(:normal_user).email], email.to
-    assert_equal "[OpenStreetMap] #{users(:public_user).display_name} commented on your diary entry", email.subject
+    assert_equal "[OpenStreetMap] #{users(:public_user).display_name} commented on a diary entry", email.subject
     assert_match /New comment/, email.text_part.decoded
     assert_match /New comment/, email.html_part.decoded
     ActionMailer::Base.deliveries.clear
@@ -358,6 +378,7 @@ class DiaryEntryControllerTest < ActionController::TestCase
   def test_comment_spammy
     # Find the entry to comment on
     entry = create(:diary_entry, :user_id => users(:normal_user).id)
+    post :subscribe, { :id => entry.id, :display_name => entry.user.display_name }, { :user => users(:normal_user).id }
 
     # Generate some spammy content
     spammy_text = 1.upto(50).map { |n| "http://example.com/spam#{n}" }.join(" ")
@@ -372,7 +393,7 @@ class DiaryEntryControllerTest < ActionController::TestCase
     assert_redirected_to :action => :view, :display_name => entry.user.display_name, :id => entry.id
     email = ActionMailer::Base.deliveries.first
     assert_equal [users(:normal_user).email], email.to
-    assert_equal "[OpenStreetMap] #{users(:public_user).display_name} commented on your diary entry", email.subject
+    assert_equal "[OpenStreetMap] #{users(:public_user).display_name} commented on a diary entry", email.subject
     assert_match %r{http://example.com/spam}, email.text_part.decoded
     assert_match %r{http://example.com/spam}, email.html_part.decoded
     ActionMailer::Base.deliveries.clear
@@ -642,6 +663,64 @@ class DiaryEntryControllerTest < ActionController::TestCase
     assert_response :not_found
   end
 
+  def test_subscribe_success
+    diary_entry = create(:diary_entry, :user_id => users(:normal_user).id)
+
+    assert_difference "diary_entry.subscribers.count", 1 do
+      post :subscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id }
+    end
+    assert_response :redirect
+  end
+
+  def test_subscribe_fail
+    diary_entry = create(:diary_entry, :user_id => users(:normal_user).id)
+
+    # not signed in
+    assert_no_difference "diary_entry.subscribers.count" do
+      post :subscribe, :id => diary_entry.id, :display_name => diary_entry.user.display_name
+    end
+    assert_response :forbidden
+
+    # bad diary id
+    post :subscribe, { :id => 999111, :display_name => "username" }, { :user => users(:public_user).id }
+    assert_response :not_found
+
+    # trying to subscribe when already subscribed
+    post :subscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id }
+    assert_no_difference "diary_entry.subscribers.count" do
+      post :subscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id }
+    end
+  end
+
+  def test_unsubscribe_success
+    diary_entry = create(:diary_entry, :user_id => users(:normal_user).id)
+
+    post :subscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id }
+    assert_difference "diary_entry.subscribers.count", -1 do
+      post :unsubscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id }
+    end
+    assert_response :redirect
+  end
+
+  def test_unsubscribe_fail
+    diary_entry = create(:diary_entry, :user_id => users(:normal_user).id)
+
+    # not signed in
+    assert_no_difference "diary_entry.subscribers.count" do
+      post :unsubscribe, :id => diary_entry.id, :display_name => diary_entry.user.display_name
+    end
+    assert_response :forbidden
+
+    # bad diary id
+    post :unsubscribe, { :id => 999111, :display_name => "username" }, { :user => users(:public_user).id }
+    assert_response :not_found
+
+    # trying to unsubscribe when not subscribed
+    assert_no_difference "diary_entry.subscribers.count" do
+      post :unsubscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id }
+    end
+  end
+
   private
 
   def check_diary_list(*entries)