not api endpoints, moved to button, fixed notifier message, fixed tests
authorMikel Maron <mikel_maron@yahoo.com>
Tue, 4 Oct 2016 02:20:04 +0000 (22:20 -0400)
committerMikel Maron <mikel_maron@yahoo.com>
Tue, 4 Oct 2016 02:20:04 +0000 (22:20 -0400)
app/controllers/diary_entry_controller.rb
app/models/notifier.rb
app/views/diary_entry/_diary_entry.html.erb
app/views/diary_entry/view.html.erb
config/locales/en.yml
test/controllers/diary_entry_controller_test.rb

index 454912b..07980ad 100644 (file)
@@ -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
index 3b7d063..a498e4e 100644 (file)
@@ -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
index e7084da..db15e39 100644 (file)
       <%= link_to t('diary_entry.diary_entry.hide_link'), hide_diary_entry_path(:display_name => diary_entry.user.display_name, :id => diary_entry.id), :method => :post, :data => { :confirm => t('diary_entry.diary_entry.confirm') } %>
     <% end %>
 
-    <% if @user and diary_entry.subscribers.exists?(@user.id) %>
-      <li><%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_url(diary_entry), :method => :post %></li>
-    <% elsif @user %>
-      <li><%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_url(diary_entry), :method => :post %></li>
-    <% end %>
-
   </ul>
 </div>
index d12942a..6571564 100644 (file)
     <%= richtext_area :diary_comment, :body, :cols => 80, :rows => 15 %>
     <%= submit_tag t('diary_entry.view.save_button') %>
   <% end %>
+  <% if @user and @entry.subscribers.exists?(@user.id) %>
+    <div style='position:relative; top: -30px; left: 130px'><%= link_to t('javascripts.changesets.show.unsubscribe'), diary_entry_unsubscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %></div>
+  <% elsif @user %>
+    <div style='position:relative; top: -30px; left: 130px'><%= link_to t('javascripts.changesets.show.subscribe'), diary_entry_subscribe_path(:display_name => @entry.user.display_name, :id => @entry.id), :method => :post, :class => :button %></div>
+  <% end %>
 <% end %>
 
 <%= if_not_logged_in(:div) do %>
index aa92b73..bf4bd7b 100644 (file)
@@ -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 
-        <a href='http://wiki.openstreetmap.org/wiki/Import/Guidelines'>Imports</a> and 
+        any activities other than editing by hand, please read and follow the guidelines on
+        <a href='http://wiki.openstreetmap.org/wiki/Import/Guidelines'>Imports</a> and
         <a href='http://wiki.openstreetmap.org/wiki/Automated_Edits_code_of_conduct'>Automated Edits</a>.
     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 <a class='icon note'></a> 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
-        <a href='/copyright'>copyright page</a> for more legal information, or contact the appropriate 
-        <a href='http://wiki.osmfoundation.org/wiki/Working_Groups'>OSMF working group</a>.  
+        <a href='/copyright'>copyright page</a> for more legal information, or contact the appropriate
+        <a href='http://wiki.osmfoundation.org/wiki/Working_Groups'>OSMF working group</a>.
   help_page:
     title: Getting Help
     introduction: |
@@ -1224,20 +1224,20 @@ en:
       License page</a> for details.
     legal_title: Legal
     legal_html: |
-      This site and many other related services are formally operated by the  
-      <a href='http://osmfoundation.org/'>OpenStreetMap Foundation</a> (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
+      <a href='http://osmfoundation.org/'>OpenStreetMap Foundation</a> (OSMF)
+      on behalf of the community. Use of all OSMF operated services is subject
       to our <a href="http://wiki.openstreetmap.org/wiki/Acceptable_Use_Policy">
       Acceptable Use Policies</a> and our <a href="http://wiki.osmfoundation.org/wiki/Privacy_Policy">Privacy Policy</a>
-      <br> 
-      Please <a href='http://osmfoundation.org/Contact'>contact the OSMF</a> 
+      <br>
+      Please <a href='http://osmfoundation.org/Contact'>contact the OSMF</a>
       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}"
index 51563ce..cbe72a6 100644 (file)
@@ -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