From 443080d7b05b5e2cc3310699f5e9d5edde010aca Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Mon, 3 Oct 2016 15:04:22 -0400 Subject: [PATCH 01/14] 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) From 44b08cc35d5b5488919059016a427feae62acb05 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Mon, 3 Oct 2016 22:20:04 -0400 Subject: [PATCH 02/14] not api endpoints, moved to button, fixed notifier message, fixed tests --- app/controllers/diary_entry_controller.rb | 10 +- app/models/notifier.rb | 1 - app/views/diary_entry/_diary_entry.html.erb | 6 - app/views/diary_entry/view.html.erb | 5 + config/locales/en.yml | 24 ++-- .../diary_entry_controller_test.rb | 112 ++++++++---------- 6 files changed, 72 insertions(+), 86 deletions(-) diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index 454912b0e..07980adc8 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/app/controllers/diary_entry_controller.rb @@ -81,21 +81,27 @@ class DiaryEntryController < ApplicationController end def subscribe - @entry = DiaryEntry.find(params[:id]) + diary_entry = DiaryEntry.find(params[:id]) if ! diary_entry.subscribers.exists?(@user.id) diary_entry.subscribers << @user + end redirect_to :controller => "diary_entry", :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 - @entry = DiaryEntry.find(params[:id]) + diary_entry = DiaryEntry.find(params[:id]) if diary_entry.subscribers.exists?(@user.id) diary_entry.subscribers.delete(@user) + end redirect_to :controller => "diary_entry", :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 diff --git a/app/models/notifier.rb b/app/models/notifier.rb index 3b7d063f0..a498e4edf 100644 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@ -83,7 +83,6 @@ class Notifier < ActionMailer::Base end end - # 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 diff --git a/app/views/diary_entry/_diary_entry.html.erb b/app/views/diary_entry/_diary_entry.html.erb index e7084da80..db15e39cd 100644 --- a/app/views/diary_entry/_diary_entry.html.erb +++ b/app/views/diary_entry/_diary_entry.html.erb @@ -35,11 +35,5 @@ <%= 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/app/views/diary_entry/view.html.erb b/app/views/diary_entry/view.html.erb index d12942a7b..657156474 100644 --- a/app/views/diary_entry/view.html.erb +++ b/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) %> +
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    + <% elsif @user %> +
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    + <% end %> <% end %> <%= if_not_logged_in(:div) do %> diff --git a/config/locales/en.yml b/config/locales/en.yml index aa92b7310..bf4bd7b19 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1118,8 +1118,8 @@ en: paragraph_1_html: | OpenStreetMap has few formal rules but we expect all participants to collaborate with, and communicate with, the community. If you are considering - any activities other than editing by hand, please read and follow the guidelines on - Imports and + any activities other than editing by hand, please read and follow the guidelines on + Imports and Automated Edits. questions: title: Any questions? @@ -1145,7 +1145,7 @@ en: title: Join the community explanation_html: | If you have noticed a problem with our map data, for example a road is missing or your address, the best way to - proceed is to join the OpenStreetMap community and add or repair the data yourself. + proceed is to join the OpenStreetMap community and add or repair the data yourself. add_a_note: instructions_html: | Just click or the same icon on the map display. @@ -1155,8 +1155,8 @@ en: title: Other concerns explanation_html: | If you have concerns about how our data is being used or about the contents please consult our - copyright page for more legal information, or contact the appropriate - OSMF working group. + copyright page for more legal information, or contact the appropriate + OSMF working group. help_page: title: Getting Help introduction: | @@ -1224,20 +1224,20 @@ en: License page for details. legal_title: Legal legal_html: | - This site and many other related services are formally operated by the - OpenStreetMap Foundation (OSMF) - on behalf of the community. Use of all OSMF operated services is subject + This site and many other related services are formally operated by the + OpenStreetMap Foundation (OSMF) + on behalf of the community. Use of all OSMF operated services is subject to our Acceptable Use Policies and our Privacy Policy -
    - Please contact the OSMF +
    + Please contact the OSMF if you have licensing, copyright or other legal questions and issues. 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}" diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index 51563ce4a..cbe72a6df 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -85,11 +85,11 @@ class DiaryEntryControllerTest < ActionController::TestCase ) assert_routing( { :path => "/user/username/diary/1/subscribe", :method => :post }, - { :controller => "diary_entry", :action => "subscribe", :id => "1" } + { :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", :id => "1" } + { :controller => "diary_entry", :action => "unsubscribe", :display_name => "username", :id => "1" } ) end @@ -330,21 +330,23 @@ 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 - post :comment, { :display_name => entry.user.display_name, :id => entry.id, :diary_comment => { :body => "" } }, { :user => users(:public_user).id } + 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 - # FIXME assert number of subscribers # 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 - post :comment, { :display_name => entry.user.display_name, :id => entry.id, :diary_comment => { :body => "New comment" } }, { :user => users(:public_user).id } + 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 @@ -360,8 +362,6 @@ 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 @@ -380,7 +380,6 @@ 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 @@ -663,91 +662,74 @@ class DiaryEntryControllerTest < ActionController::TestCase ## # test subscribe success def test_subscribe_success - basic_authorization(users(:public_user).email, "test") - changeset = changesets(:normal_user_closed_change) + diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) - assert_difference "changeset.subscribers.count", 1 do - post :subscribe, :id => changeset.id + #basic_authorization(users(:public_user).email, "test") + + 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 :success + assert_response :redirect 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 + diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) - basic_authorization(users(:public_user).email, "test") - - # bad changeset id - assert_no_difference "changeset.subscribers.count" do - post :subscribe, :id => 999111 + # not signed in + assert_no_difference "diary_entry.subscribers.count", 1 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 - # 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 + 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 - assert_response :conflict end ## # test unsubscribe success def test_unsubscribe_success - basic_authorization(users(:public_user).email, "test") - changeset = changesets(:normal_user_subscribed_change) + diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) - assert_difference "changeset.subscribers.count", -1 do - post :unsubscribe, :id => changeset.id + assert_difference "diary_entry.subscribers.count", -1 do + post :unsubscribe, {:id => diary_entry.id, :display_name => diary_entry.user.display_name}, { :user => users(:normal_user).id} end - assert_response :success + assert_response :redirect + + 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 ## # 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 + diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) - basic_authorization(users(:public_user).email, "test") - - # bad changeset id - assert_no_difference "changeset.subscribers.count" do - post :unsubscribe, :id => 999111 + # 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 - # not closed changeset - changeset = changesets(:normal_user_first_change) - assert_no_difference "changeset.subscribers.count" do - post :unsubscribe, :id => changeset.id + # trying to subscribe when already 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 - 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 From 3c01d2e80dc0e5b6942eb7b965c991f5eba5fef2 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Thu, 6 Oct 2016 20:31:10 -0400 Subject: [PATCH 03/14] more idiomatic models for diary entry subscriptions --- app/controllers/diary_entry_controller.rb | 12 ++++-------- app/models/diary_entry.rb | 3 ++- app/models/diary_entry_subscription.rb | 4 ++++ app/models/user.rb | 3 ++- app/views/diary_entry/_diary_entry.html.erb | 1 - app/views/diary_entry/view.html.erb | 2 +- ...dd_join_table_between_users_and_diary_entries.rb | 13 +++++-------- 7 files changed, 18 insertions(+), 20 deletions(-) create mode 100644 app/models/diary_entry_subscription.rb diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index 07980adc8..90a08615c 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/app/controllers/diary_entry_controller.rb @@ -26,7 +26,7 @@ class DiaryEntryController < ApplicationController end # Subscribe user to diary comments - @diary_entry.subscribers << @user + @diary_entry.subscriptions.create(user: @user) redirect_to :controller => "diary_entry", :action => "list", :display_name => @user.display_name else @@ -70,7 +70,7 @@ class DiaryEntryController < ApplicationController end # Add the commenter to the subscribers if necessary - @entry.subscribers << @user unless @entry.subscribers.exists?(@user.id) + @entry.subscriptions.create(user: @user) unless @entry.subscribers.exists?(@user) redirect_to :controller => "diary_entry", :action => "view", :display_name => @entry.user.display_name, :id => @entry.id else @@ -83,9 +83,7 @@ class DiaryEntryController < ApplicationController def subscribe diary_entry = DiaryEntry.find(params[:id]) - if ! diary_entry.subscribers.exists?(@user.id) - diary_entry.subscribers << @user - end + diary_entry.subscriptions.create(user: @user) unless diary_entry.subscribers.exists?(@user) redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id rescue ActiveRecord::RecordNotFound @@ -95,9 +93,7 @@ class DiaryEntryController < ApplicationController def unsubscribe diary_entry = DiaryEntry.find(params[:id]) - if diary_entry.subscribers.exists?(@user.id) - diary_entry.subscribers.delete(@user) - end + diary_entry.subscriptions.where(user: @user).delete_all if diary_entry.subscribers.exists?(@user) redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id rescue ActiveRecord::RecordNotFound diff --git a/app/models/diary_entry.rb b/app/models/diary_entry.rb index 07bb60a10..48f3e69bd 100644 --- a/app/models/diary_entry.rb +++ b/app/models/diary_entry.rb @@ -4,7 +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_and_belongs_to_many :subscribers, :class_name => "User", :join_table => "diary_entries_subscribers", :association_foreign_key => "subscriber_id" + has_many :subscriptions, :class_name => 'DiaryEntrySubscription' + has_many :subscribers, :through => :subscriptions, :source => :user scope :visible, -> { where(:visible => true) } diff --git a/app/models/diary_entry_subscription.rb b/app/models/diary_entry_subscription.rb new file mode 100644 index 000000000..b0a563ea8 --- /dev/null +++ b/app/models/diary_entry_subscription.rb @@ -0,0 +1,4 @@ +class DiaryEntrySubscription < ActiveRecord::Base + belongs_to :user + belongs_to :diary_entry +end diff --git a/app/models/user.rb b/app/models/user.rb index 22cdfabdc..c3e02e0ab 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,7 +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_and_belongs_to_many :diary_entries_subscriptions, :class_name => "DiaryEntry", :join_table => "diary_entries_subscribers", :foreign_key => "subscriber_id" + 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 diff --git a/app/views/diary_entry/_diary_entry.html.erb b/app/views/diary_entry/_diary_entry.html.erb index db15e39cd..410e13047 100644 --- a/app/views/diary_entry/_diary_entry.html.erb +++ b/app/views/diary_entry/_diary_entry.html.erb @@ -34,6 +34,5 @@ <%= 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 %> - diff --git a/app/views/diary_entry/view.html.erb b/app/views/diary_entry/view.html.erb index 657156474..67ca96e72 100644 --- a/app/views/diary_entry/view.html.erb +++ b/app/views/diary_entry/view.html.erb @@ -21,7 +21,7 @@ <%= 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) %> + <% if @user and @entry.subscribers and @entry.subscribers.exists?(@user) %>
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    <% elsif @user %>
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    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 index cd8414ee7..b99dbbdb8 100644 --- 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 @@ -1,20 +1,17 @@ class AddJoinTableBetweenUsersAndDiaryEntries < ActiveRecord::Migration def change - create_table :diary_entries_subscribers, :id => false do |t| - t.column :subscriber_id, :bigint, :null => false + 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_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] + add_index :diary_entry_subscriptions, [:user_id, :diary_entry_id], :unique => true, :name => "index_diary_subscriptions_on_user_id_and_diary_entry_id" + add_index :diary_entry_subscriptions, [: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) + diary_entry.subscriptions.create(user: diary_entry.user) unless diary_entry.subscribers.exists?(@user) end end From e055eaf690a32f5d42c53bd1655090a0e8da2ac7 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Thu, 6 Oct 2016 21:33:47 -0400 Subject: [PATCH 04/14] tests passing --- app/controllers/diary_entry_controller.rb | 6 +++--- app/views/diary_entry/view.html.erb | 2 +- test/controllers/diary_entry_controller_test.rb | 12 +++++------- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index 90a08615c..bd2e45143 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/app/controllers/diary_entry_controller.rb @@ -70,7 +70,7 @@ class DiaryEntryController < ApplicationController end # Add the commenter to the subscribers if necessary - @entry.subscriptions.create(user: @user) unless @entry.subscribers.exists?(@user) + @entry.subscriptions.create(user: @user) unless @entry.subscribers.exists?(@user.id) redirect_to :controller => "diary_entry", :action => "view", :display_name => @entry.user.display_name, :id => @entry.id else @@ -83,7 +83,7 @@ class DiaryEntryController < ApplicationController def subscribe diary_entry = DiaryEntry.find(params[:id]) - diary_entry.subscriptions.create(user: @user) unless diary_entry.subscribers.exists?(@user) + diary_entry.subscriptions.create(user: @user) unless diary_entry.subscribers.exists?(@user.id) redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id rescue ActiveRecord::RecordNotFound @@ -93,7 +93,7 @@ class DiaryEntryController < ApplicationController def unsubscribe diary_entry = DiaryEntry.find(params[:id]) - diary_entry.subscriptions.where(user: @user).delete_all if diary_entry.subscribers.exists?(@user) + diary_entry.subscriptions.where(user: @user).delete_all if diary_entry.subscribers.exists?(@user.id) redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id rescue ActiveRecord::RecordNotFound diff --git a/app/views/diary_entry/view.html.erb b/app/views/diary_entry/view.html.erb index 67ca96e72..3e9686922 100644 --- a/app/views/diary_entry/view.html.erb +++ b/app/views/diary_entry/view.html.erb @@ -21,7 +21,7 @@ <%= richtext_area :diary_comment, :body, :cols => 80, :rows => 15 %> <%= submit_tag t('diary_entry.view.save_button') %> <% end %> - <% if @user and @entry.subscribers and @entry.subscribers.exists?(@user) %> + <% if @user and @entry.subscribers and @entry.subscribers.exists?(@user.id) %>
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    <% elsif @user %>
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index cbe72a6df..91131ee8d 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -330,6 +330,8 @@ 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 @@ -353,7 +355,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 /New comment/, email.text_part.decoded assert_match /New comment/, email.html_part.decoded ActionMailer::Base.deliveries.clear @@ -376,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(" ") @@ -390,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 @@ -699,11 +702,6 @@ class DiaryEntryControllerTest < ActionController::TestCase def test_unsubscribe_success diary_entry = create(:diary_entry, :user_id => users(:normal_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(:normal_user).id} - end - assert_response :redirect - 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} From bb22e23dfb923fd543973a73f161e6e295eaa465 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Thu, 6 Oct 2016 21:47:29 -0400 Subject: [PATCH 05/14] css in right place; fix structure.sql --- app/assets/stylesheets/common.scss | 7 +++++++ app/views/diary_entry/view.html.erb | 4 ++-- db/structure.sql | 22 +++++++++++++++++++++- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/common.scss b/app/assets/stylesheets/common.scss index e8c480f56..7e7940d35 100644 --- a/app/assets/stylesheets/common.scss +++ b/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 { diff --git a/app/views/diary_entry/view.html.erb b/app/views/diary_entry/view.html.erb index 3e9686922..b1159195d 100644 --- a/app/views/diary_entry/view.html.erb +++ b/app/views/diary_entry/view.html.erb @@ -22,9 +22,9 @@ <%= submit_tag t('diary_entry.view.save_button') %> <% end %> <% if @user and @entry.subscribers and @entry.subscribers.exists?(@user.id) %> -
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    +
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    <% elsif @user %> -
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    +
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    <% end %> <% end %> diff --git a/db/structure.sql b/db/structure.sql index cbed6c8ce..a3e80ba45 100644 --- a/db/structure.sql +++ b/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: - @@ -1828,6 +1836,19 @@ 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_diary_subscriptions_on_user_id_and_diary_entry_id; Type: INDEX; Schema: public; Owner: - +-- + +CREATE UNIQUE INDEX index_diary_subscriptions_on_user_id_and_diary_entry_id ON diary_entry_subscriptions USING btree (user_id, diary_entry_id); + + -- -- Name: index_note_comments_on_body; Type: INDEX; Schema: public; Owner: - -- @@ -2643,4 +2664,3 @@ INSERT INTO schema_migrations (version) VALUES ('7'); INSERT INTO schema_migrations (version) VALUES ('8'); INSERT INTO schema_migrations (version) VALUES ('9'); - From a32076abd6369b74ac81db5eeffbc40bdf64d597 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Fri, 7 Oct 2016 06:32:05 -0400 Subject: [PATCH 06/14] fix rubocop warnings --- app/controllers/diary_entry_controller.rb | 8 +++---- app/models/diary_entry.rb | 2 +- app/models/user.rb | 2 +- ...n_table_between_users_and_diary_entries.rb | 2 +- .../diary_entry_controller_test.rb | 22 +++++++++---------- 5 files changed, 17 insertions(+), 19 deletions(-) diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb index bd2e45143..fdc0e9d24 100644 --- a/app/controllers/diary_entry_controller.rb +++ b/app/controllers/diary_entry_controller.rb @@ -26,7 +26,7 @@ class DiaryEntryController < ApplicationController end # Subscribe user to diary comments - @diary_entry.subscriptions.create(user: @user) + @diary_entry.subscriptions.create(:user => @user) redirect_to :controller => "diary_entry", :action => "list", :display_name => @user.display_name else @@ -70,7 +70,7 @@ class DiaryEntryController < ApplicationController end # Add the commenter to the subscribers if necessary - @entry.subscriptions.create(user: @user) unless @entry.subscribers.exists?(@user.id) + @entry.subscriptions.create(:user => @user) unless @entry.subscribers.exists?(@user.id) redirect_to :controller => "diary_entry", :action => "view", :display_name => @entry.user.display_name, :id => @entry.id else @@ -83,7 +83,7 @@ class DiaryEntryController < ApplicationController def subscribe diary_entry = DiaryEntry.find(params[:id]) - diary_entry.subscriptions.create(user: @user) unless diary_entry.subscribers.exists?(@user.id) + diary_entry.subscriptions.create(:user => @user) unless diary_entry.subscribers.exists?(@user.id) redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id rescue ActiveRecord::RecordNotFound @@ -93,7 +93,7 @@ class DiaryEntryController < ApplicationController def unsubscribe diary_entry = DiaryEntry.find(params[:id]) - diary_entry.subscriptions.where(user: @user).delete_all if diary_entry.subscribers.exists?(@user.id) + diary_entry.subscriptions.where(:user => @user).delete_all if diary_entry.subscribers.exists?(@user.id) redirect_to :controller => "diary_entry", :action => "view", :display_name => diary_entry.user.display_name, :id => diary_entry.id rescue ActiveRecord::RecordNotFound diff --git a/app/models/diary_entry.rb b/app/models/diary_entry.rb index 48f3e69bd..e756432fd 100644 --- a/app/models/diary_entry.rb +++ b/app/models/diary_entry.rb @@ -4,7 +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_many :subscriptions, :class_name => 'DiaryEntrySubscription' + has_many :subscriptions, :class_name => "DiaryEntrySubscription" has_many :subscribers, :through => :subscriptions, :source => :user scope :visible, -> { where(:visible => true) } diff --git a/app/models/user.rb b/app/models/user.rb index c3e02e0ab..3ff9277f0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,7 +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_many :diary_entry_subscriptions, :class_name => 'DiaryEntrySubscription' + 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 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 index b99dbbdb8..ad642f96c 100644 --- 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 @@ -11,7 +11,7 @@ class AddJoinTableBetweenUsersAndDiaryEntries < ActiveRecord::Migration def up DiaryEntry.find_each do |diary_entry| - diary_entry.subscriptions.create(user: diary_entry.user) unless diary_entry.subscribers.exists?(@user) + diary_entry.subscriptions.create(:user => diary_entry.user) unless diary_entry.subscribers.exists?(@user.id) end end diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index 91131ee8d..77d340ffe 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -330,7 +330,7 @@ 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} + 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 @@ -378,7 +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} + 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(" ") @@ -667,10 +667,8 @@ class DiaryEntryControllerTest < ActionController::TestCase def test_subscribe_success diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) - #basic_authorization(users(:public_user).email, "test") - 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} + post :subscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id } end assert_response :redirect end @@ -687,13 +685,13 @@ class DiaryEntryControllerTest < ActionController::TestCase assert_response :forbidden # bad diary id - post :subscribe, {:id => 999111, :display_name => "username"}, { :user => users(:public_user).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} + 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} + post :subscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name}, { :user => users(:public_user).id } end end @@ -702,9 +700,9 @@ class DiaryEntryControllerTest < ActionController::TestCase 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} + 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} + post :unsubscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id } end assert_response :redirect end @@ -721,12 +719,12 @@ class DiaryEntryControllerTest < ActionController::TestCase assert_response :forbidden # bad diary id - post :unsubscribe, {:id => 999111, :display_name => "username"}, { :user => users(:public_user).id} + post :unsubscribe, { :id => 999111, :display_name => "username" }, { :user => users(:public_user).id } assert_response :not_found # trying to subscribe when already 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} + post :unsubscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id } end end From 8272a53ab9af5018c4f4b7b99d2a189d867fdecc Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Fri, 7 Oct 2016 06:36:14 -0400 Subject: [PATCH 07/14] fix rubocop warnings --- test/controllers/diary_entry_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index 77d340ffe..ed92de66f 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -691,7 +691,7 @@ class DiaryEntryControllerTest < ActionController::TestCase # 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 } + post :subscribe, { :id => diary_entry.id, :display_name => diary_entry.user.display_name }, { :user => users(:public_user).id } end end From 6b7ebf6dc40da609267ad38df981cd31d99e1538 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Fri, 7 Oct 2016 09:04:44 -0400 Subject: [PATCH 08/14] clean up comments --- test/controllers/diary_entry_controller_test.rb | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index ed92de66f..f36de9ad5 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -662,8 +662,6 @@ class DiaryEntryControllerTest < ActionController::TestCase assert_response :not_found end - ## - # test subscribe success def test_subscribe_success diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) @@ -673,8 +671,6 @@ class DiaryEntryControllerTest < ActionController::TestCase assert_response :redirect end - ## - # test subscribe fail def test_subscribe_fail diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) @@ -695,8 +691,6 @@ class DiaryEntryControllerTest < ActionController::TestCase end end - ## - # test unsubscribe success def test_unsubscribe_success diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) @@ -707,8 +701,6 @@ class DiaryEntryControllerTest < ActionController::TestCase assert_response :redirect end - ## - # test unsubscribe fail def test_unsubscribe_fail diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) @@ -722,7 +714,7 @@ class DiaryEntryControllerTest < ActionController::TestCase post :unsubscribe, { :id => 999111, :display_name => "username" }, { :user => users(:public_user).id } assert_response :not_found - # trying to subscribe when already subscribed + # 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 From 43ef60b6d5cd4db4e9c7aee2e567ea5166cfb2ac Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Mon, 10 Oct 2016 21:18:03 -0400 Subject: [PATCH 09/14] break out author subscriptions to its own migration --- ...5_add_join_table_between_users_and_diary_entries.rb | 8 -------- ...0161011010929_subscribe_authors_to_diary_entries.rb | 10 ++++++++++ 2 files changed, 10 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20161011010929_subscribe_authors_to_diary_entries.rb 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 index ad642f96c..4c41f725f 100644 --- 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 @@ -9,12 +9,4 @@ class AddJoinTableBetweenUsersAndDiaryEntries < ActiveRecord::Migration add_index :diary_entry_subscriptions, [:diary_entry_id] end - def up - DiaryEntry.find_each do |diary_entry| - diary_entry.subscriptions.create(:user => diary_entry.user) unless diary_entry.subscribers.exists?(@user.id) - end - end - - def down - end end diff --git a/db/migrate/20161011010929_subscribe_authors_to_diary_entries.rb b/db/migrate/20161011010929_subscribe_authors_to_diary_entries.rb new file mode 100644 index 000000000..2dfbbf2c8 --- /dev/null +++ b/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 From 5cd00ddfd182a7be435f3c3af110b1d52ddf1446 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Tue, 11 Oct 2016 15:44:37 -0400 Subject: [PATCH 10/14] rubocop clean up --- ...61002153425_add_join_table_between_users_and_diary_entries.rb | 1 - 1 file changed, 1 deletion(-) 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 index 4c41f725f..276abeef4 100644 --- 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 @@ -8,5 +8,4 @@ class AddJoinTableBetweenUsersAndDiaryEntries < ActiveRecord::Migration add_index :diary_entry_subscriptions, [:user_id, :diary_entry_id], :unique => true, :name => "index_diary_subscriptions_on_user_id_and_diary_entry_id" add_index :diary_entry_subscriptions, [:diary_entry_id] end - end From 3726d561ea0f5d363599ef4daf70eafef3abac38 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Tue, 11 Oct 2016 20:42:10 -0400 Subject: [PATCH 11/14] last minor fixes --- app/views/diary_entry/view.html.erb | 2 +- ...add_join_table_between_users_and_diary_entries.rb | 12 ++++++++++-- test/controllers/diary_entry_controller_test.rb | 2 +- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/views/diary_entry/view.html.erb b/app/views/diary_entry/view.html.erb index b1159195d..6a2a21abc 100644 --- a/app/views/diary_entry/view.html.erb +++ b/app/views/diary_entry/view.html.erb @@ -21,7 +21,7 @@ <%= richtext_area :diary_comment, :body, :cols => 80, :rows => 15 %> <%= submit_tag t('diary_entry.view.save_button') %> <% end %> - <% if @user and @entry.subscribers and @entry.subscribers.exists?(@user.id) %> + <% if @user and @entry.subscribers.exists?(@user.id) %>
    <%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    <% elsif @user %>
    <%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %>
    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 index 276abeef4..cb8e455dd 100644 --- 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 @@ -1,11 +1,19 @@ +require "migrate" + class AddJoinTableBetweenUsersAndDiaryEntries < ActiveRecord::Migration - def change + 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_index :diary_entry_subscriptions, [:user_id, :diary_entry_id], :unique => true, :name => "index_diary_subscriptions_on_user_id_and_diary_entry_id" + 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 diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index f36de9ad5..6ebf4ec09 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -675,7 +675,7 @@ class DiaryEntryControllerTest < ActionController::TestCase diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) # not signed in - assert_no_difference "diary_entry.subscribers.count", 1 do + 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 From 6a57901071f3c97e422261da03d972b9b3036de0 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Tue, 11 Oct 2016 20:44:58 -0400 Subject: [PATCH 12/14] adding back trailing newlines --- config/locales/en.yml | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index bf4bd7b19..61c82211e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1118,8 +1118,8 @@ en: paragraph_1_html: | OpenStreetMap has few formal rules but we expect all participants to collaborate with, and communicate with, the community. If you are considering - any activities other than editing by hand, please read and follow the guidelines on - Imports and + any activities other than editing by hand, please read and follow the guidelines on + Imports and Automated Edits. questions: title: Any questions? @@ -1145,7 +1145,7 @@ en: title: Join the community explanation_html: | If you have noticed a problem with our map data, for example a road is missing or your address, the best way to - proceed is to join the OpenStreetMap community and add or repair the data yourself. + proceed is to join the OpenStreetMap community and add or repair the data yourself. add_a_note: instructions_html: | Just click or the same icon on the map display. @@ -1155,8 +1155,8 @@ en: title: Other concerns explanation_html: | If you have concerns about how our data is being used or about the contents please consult our - copyright page for more legal information, or contact the appropriate - OSMF working group. + copyright page for more legal information, or contact the appropriate + OSMF working group. help_page: title: Getting Help introduction: | @@ -1224,13 +1224,13 @@ en: License page for details. legal_title: Legal legal_html: | - This site and many other related services are formally operated by the - OpenStreetMap Foundation (OSMF) - on behalf of the community. Use of all OSMF operated services is subject + This site and many other related services are formally operated by the + OpenStreetMap Foundation (OSMF) + on behalf of the community. Use of all OSMF operated services is subject to our Acceptable Use Policies and our Privacy Policy
    - Please contact the OSMF + Please contact the OSMF if you have licensing, copyright or other legal questions and issues. partners_title: Partners notifier: From 99afab6d615f0213ae8d943705ad654c85146a6a Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Tue, 11 Oct 2016 20:45:44 -0400 Subject: [PATCH 13/14] adding back trailing newlines --- config/locales/en.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 61c82211e..23307bdc5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1229,7 +1229,7 @@ en: on behalf of the community. Use of all OSMF operated services is subject to our Acceptable Use Policies and our Privacy Policy -
    +
    Please contact the OSMF if you have licensing, copyright or other legal questions and issues. partners_title: Partners From 3a14abde1beae0438d043f536b8417ea92dc2eb7 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Tue, 11 Oct 2016 21:22:00 -0400 Subject: [PATCH 14/14] Update structure.sql --- db/structure.sql | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/db/structure.sql b/db/structure.sql index a3e80ba45..5e9b74195 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -1456,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: - -- @@ -1841,14 +1849,6 @@ CREATE UNIQUE INDEX index_client_applications_on_key ON client_applications USIN CREATE INDEX index_diary_entry_subscriptions_on_diary_entry_id ON diary_entry_subscriptions USING btree (diary_entry_id); - --- --- Name: index_diary_subscriptions_on_user_id_and_diary_entry_id; Type: INDEX; Schema: public; Owner: - --- - -CREATE UNIQUE INDEX index_diary_subscriptions_on_user_id_and_diary_entry_id ON diary_entry_subscriptions USING btree (user_id, diary_entry_id); - - -- -- Name: index_note_comments_on_body; Type: INDEX; Schema: public; Owner: - -- @@ -2233,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: - --