not api endpoints, moved to button, fixed notifier message, fixed tests

diff --git a/app/controllers/diary_entry_controller.rb b/app/controllers/diary_entry_controller.rb
index 454912b0ee65a75fa4d5619902ef611c8447b0a7..07980adc8101789927b22c0ded2122ef3368bbce 100644 (file)
--- a/app/controllers/diary_entry_controller.rb
+++ b/app/controllers/diary_entry_controller.rb
@@ -81,21 +81,27 @@ class DiaryEntryController < ApplicationController
   end
 
   def subscribe
   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
 
     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
 
     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
   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)
 
     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
 
     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
   end
 
   def list

diff --git a/app/models/notifier.rb b/app/models/notifier.rb
index 3b7d063f0770624e054376c6f0341f35c6dd4f2a..a498e4edf6cd88cb45c8e0be9bc8bb4f5d3bff2d 100644 (file)
--- a/app/models/notifier.rb
+++ b/app/models/notifier.rb
@@ -83,7 +83,6 @@ class Notifier < ActionMailer::Base
     end
   end
 
     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
   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 e7084da80727c4e1054e873c4d1144b4740994e4..db15e39cdb335bb910835288313b5f3d957f2c1e 100644 (file)
--- 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 %>
 
       <%= 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>
   </ul>
 </div>

diff --git a/app/views/diary_entry/view.html.erb b/app/views/diary_entry/view.html.erb
index d12942a7b9b9f98f8286afde37f359fc6dd5f774..657156474ac2125024831429de7d08fa466f7642 100644 (file)
--- 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 %>
     <%= 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 %>
 <% end %>
 
 <%= if_not_logged_in(:div) do %>

diff --git a/config/locales/en.yml b/config/locales/en.yml
index aa92b73101821f43e05693e52ce90d3456be6e67..bf4bd7b1959bcabe17cf117d5ecb5ec1eefc19be 100644 (file)
--- 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
       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?
         <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
         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.
       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
       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: |
   help_page:
     title: Getting Help
     introduction: |


@@ -1224,20 +1224,20 @@ en:
       License page</a> for details.
     legal_title: Legal
     legal_html: |
       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>
       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:
       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},"
       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}"
       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 51563ce4a676f3436e0516c5277f3a2e17611448..cbe72a6dfb8c3fcd6bf5616c5084dbd7d169bae5 100644 (file)
--- 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 },
     )
     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 },
     )
     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
 
     )
   end
 


@@ -330,21 +330,23 @@ class DiaryEntryControllerTest < ActionController::TestCase
       assert_select "h2", :text => "No entry with the id: 9999", :count => 1
     end
 
       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
     # 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
 
       end
     end
     assert_response :success
     assert_template :view
 

-    # FIXME assert number of subscribers

     # Now try again with the right id
     # Now try again with the right id

-    assert_difference "ActionMailer::Base.deliveries.size", 1 do
+    assert_difference "ActionMailer::Base.deliveries.size", entry.subscribers.count do

       assert_difference "DiaryComment.count", 1 do
       assert_difference "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
       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
 
     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
     # 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(" ")
 
     # 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
     # 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
   ##
   # 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
     end

-    assert_response :success
+    assert_response :redirect

   end
 
   ##
   # test subscribe fail
   def test_subscribe_fail
   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
     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
 
     # 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
     end

-    assert_response :conflict

   end
 
   ##
   # test unsubscribe success
   def test_unsubscribe_success
   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
     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
   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
     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
     end

-    assert_response :not_found

   end
 
   private
   end
 
   private