From 443080d7b05b5e2cc3310699f5e9d5edde010aca Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Mon, 3 Oct 2016 15:04:22 -0400 Subject: [PATCH] WIP diary comment subscriptions --- app/controllers/diary_entry_controller.rb | 37 +++++- app/models/diary_entry.rb | 1 + app/models/notifier.rb | 9 +- app/models/user.rb | 1 + app/views/diary_entry/_diary_entry.html.erb | 7 ++ config/routes.rb | 2 + ...n_table_between_users_and_diary_entries.rb | 23 ++++ .../diary_entry_controller_test.rb | 109 ++++++++++++++++++ 8 files changed, 181 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20161002153425_add_join_table_between_users_and_diary_entries.rb diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index c0b6ece38..454912b0e 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/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]) diff --git a/app/models/diary_entry.rb b/app/models/diary_entry.rb index 368ee3aca..07bb60a10 100644 --- a/app/models/diary_entry.rb +++ b/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) } diff --git a/app/models/notifier.rb b/app/models/notifier.rb index 23f7b9907..3b7d063f0 100644 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@ -83,9 +83,10 @@ 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 + # FIXME mail should say your / their depending who's message it is + 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 +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 diff --git a/app/models/user.rb b/app/models/user.rb index a550b9f05..22cdfabdc 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/views/diary_entry/_diary_entry.html.erb b/app/views/diary_entry/_diary_entry.html.erb index 410e13047..e7084da80 100644 --- a/app/views/diary_entry/_diary_entry.html.erb +++ b/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) %> +
  • <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_url(diary_entry), :method => :post %>
    • + <% elsif @user %> +
  • <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_url(diary_entry), :method => :post %>
    • + <% end %> + diff --git a/config/routes.rb b/config/routes.rb index 085d67417..59c0dac86 100644 --- a/config/routes.rb +++ b/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" diff --git a/db/migrate/20161002153425_add_join_table_between_users_and_diary_entries.rb b/db/migrate/20161002153425_add_join_table_between_users_and_diary_entries.rb new file mode 100644 index 000000000..cd8414ee7 --- /dev/null +++ b/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 diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index 6e07a4091..51563ce4a 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/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) -- 2.43.2