From 44b08cc35d5b5488919059016a427feae62acb05 Mon Sep 17 00:00:00 2001 From: Mikel Maron Date: Mon, 3 Oct 2016 22:20:04 -0400 Subject: [PATCH] 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) + + #basic_authorization(users(:public_user).email, "test") - assert_difference "changeset.subscribers.count", 1 do - post :subscribe, :id => changeset.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 :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 - - basic_authorization(users(:public_user).email, "test") + diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) - # 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 :not_found + assert_response :forbidden - # 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 + # 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 - 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 - - basic_authorization(users(:public_user).email, "test") + diary_entry = create(:diary_entry, :user_id => users(:normal_user).id) - # 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 :not_found + assert_response :forbidden - # 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 + # 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 - changeset = changesets(:normal_user_closed_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 :not_found end private -- 2.43.2