Use resourceful route for message reply

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index 6638016d72ea55b1cad4fbd908228562d1db00fc..7ed6470b84e5830c37e4184e50743b9d8a96ee8b 100644 (file)
--- a/app/abilities/ability.rb
+++ b/app/abilities/ability.rb
@@ -42,7 +42,7 @@ class Ability
         can :update, DiaryEntry, :user => user
         can [:create], DiaryComment
         can [:make_friend, :remove_friend], Friendship
         can :update, DiaryEntry, :user => user
         can [:create], DiaryComment
         can [:make_friend, :remove_friend], Friendship

-        can [:read, :create, :reply, :inbox, :outbox, :muted, :mark, :unmute, :destroy], Message
+        can [:read, :create, :mark, :unmute, :destroy], Message

         can [:close, :reopen], Note
         can [:read, :update], :preference
         can :update, :profile
         can [:close, :reopen], Note
         can [:read, :update], :preference
         can :update, :profile

diff --git a/app/controllers/messages/replies_controller.rb b/app/controllers/messages/replies_controller.rb
new file mode 100644 (file)
index 0000000..b1e7b8d
--- /dev/null
+++ b/app/controllers/messages/replies_controller.rb
@@ -0,0 +1,50 @@
+module Messages
+  class RepliesController < ApplicationController
+    layout "site"
+
+    before_action :authorize_web
+    before_action :set_locale
+
+    authorize_resource :class => Message
+
+    before_action :check_database_readable
+    before_action :check_database_writable
+
+    allow_thirdparty_images
+
+    # Allow the user to reply to another message.
+    def new
+      message = Message.find(params[:message_id])
+
+      if message.recipient == current_user
+        message.update(:message_read => true)
+
+        @message = Message.new(
+          :recipient => message.sender,
+          :title => "Re: #{message.title.sub(/^Re:\s*/, '')}",
+          :body => "On #{message.sent_on} #{message.sender.display_name} wrote:\n\n#{message.body.gsub(/^/, '> ')}"
+        )
+
+        @title = @message.title
+
+        render "messages/new"
+      elsif message.sender == current_user
+        @message = Message.new(
+          :recipient => message.recipient,
+          :title => "Re: #{message.title.sub(/^Re:\s*/, '')}",
+          :body => "On #{message.sent_on} #{message.sender.display_name} wrote:\n\n#{message.body.gsub(/^/, '> ')}"
+        )
+
+        @title = @message.title
+
+        render "messages/new"
+      else
+        flash[:notice] = t ".wrong_user", :user => current_user.display_name
+        redirect_to login_path(:referer => request.fullpath)
+      end
+    rescue ActiveRecord::RecordNotFound
+      @title = t "messages.no_such_message.title"
+      render "messages/no_such_message", :status => :not_found
+    end
+  end
+end

diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb
index 7162b900a011c4719c18a212f96691d3e68e26b8..26e8a5e09e6602d3a27508b8fb42a3d70b3acd5c 100644 (file)
--- a/app/controllers/messages_controller.rb
+++ b/app/controllers/messages_controller.rb
@@ -10,7 +10,7 @@ class MessagesController < ApplicationController
 
   before_action :lookup_user, :only => [:new, :create]
   before_action :check_database_readable
 
   before_action :lookup_user, :only => [:new, :create]
   before_action :check_database_readable

-  before_action :check_database_writable, :only => [:new, :create, :reply, :mark, :destroy]
+  before_action :check_database_writable, :only => [:new, :create, :mark, :destroy]

 
   allow_thirdparty_images :only => [:new, :create, :show]
 
 
   allow_thirdparty_images :only => [:new, :create, :show]
 


@@ -73,41 +73,6 @@ class MessagesController < ApplicationController
     render :action => "no_such_message", :status => :not_found
   end
 
     render :action => "no_such_message", :status => :not_found
   end
 

-  # Allow the user to reply to another message.
-  def reply
-    message = Message.find(params[:message_id])
-
-    if message.recipient == current_user
-      message.update(:message_read => true)
-
-      @message = Message.new(
-        :recipient => message.sender,
-        :title => "Re: #{message.title.sub(/^Re:\s*/, '')}",
-        :body => "On #{message.sent_on} #{message.sender.display_name} wrote:\n\n#{message.body.gsub(/^/, '> ')}"
-      )
-
-      @title = @message.title
-
-      render :action => "new"
-    elsif message.sender == current_user
-      @message = Message.new(
-        :recipient => message.recipient,
-        :title => "Re: #{message.title.sub(/^Re:\s*/, '')}",
-        :body => "On #{message.sent_on} #{message.sender.display_name} wrote:\n\n#{message.body.gsub(/^/, '> ')}"
-      )
-
-      @title = @message.title
-
-      render :action => "new"
-    else
-      flash[:notice] = t ".wrong_user", :user => current_user.display_name
-      redirect_to login_path(:referer => request.fullpath)
-    end
-  rescue ActiveRecord::RecordNotFound
-    @title = t "messages.no_such_message.title"
-    render :action => "no_such_message", :status => :not_found
-  end
-

   # Set the message as being read or unread.
   def mark
     @message = current_user.messages.unscope(:where => :muted).find(params[:message_id])
   # Set the message as being read or unread.
   def mark
     @message = current_user.messages.unscope(:where => :muted).find(params[:message_id])

diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb
index dee3dafbed35562ab865f11c64fcad4d61e8a067..1dd13fb2d220787af6562f11281abb6bd0da07cc 100644 (file)
--- a/app/mailers/user_mailer.rb
+++ b/app/mailers/user_mailer.rb
@@ -78,7 +78,7 @@ class UserMailer < ApplicationMailer
       @text = message.body
       @title = message.title
       @readurl = message_url(message)
       @text = message.body
       @title = message.title
       @readurl = message_url(message)

-      @replyurl = message_reply_url(message)
+      @replyurl = new_message_reply_url(message)

       @author = @from_user
 
       attach_user_avatar(message.sender)
       @author = @from_user
 
       attach_user_avatar(message.sender)

diff --git a/app/views/messages/show.html.erb b/app/views/messages/show.html.erb
index 99d6d0435512d0f8e33ef8ccc3112dec762ad20f..d3147c9699c3c24c95b1a12aa2c7d74acf24be0c 100644 (file)
--- a/app/views/messages/show.html.erb
+++ b/app/views/messages/show.html.erb
@@ -18,7 +18,7 @@
 <div class="richtext text-break"><%= @message.body.to_html %></div>
 
 <div>
 <div class="richtext text-break"><%= @message.body.to_html %></div>
 
 <div>

-  <%= link_to t(".reply_button"), message_reply_path(@message), :class => "btn btn-primary" %>
+  <%= link_to t(".reply_button"), new_message_reply_path(@message), :class => "btn btn-primary" %>

   <% if current_user == @message.recipient %>
     <%= link_to t(".unread_button"), message_mark_path(@message, :mark => "unread"), :method => "post", :class => "btn btn-primary" %>
     <%= link_to t(".destroy_button"), message_path(@message), :method => "delete", :class => "btn btn-danger" %>
   <% if current_user == @message.recipient %>
     <%= link_to t(".unread_button"), message_mark_path(@message, :mark => "unread"), :method => "post", :class => "btn btn-primary" %>
     <%= link_to t(".destroy_button"), message_path(@message), :method => "delete", :class => "btn btn-danger" %>

diff --git a/config/locales/en.yml b/config/locales/en.yml
index 4f2174206548e2f5267daf6ad4c39a896c9e710d..2a34c0a599372a8ecbd482b3cb9684dbd61985df 100644 (file)
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1752,8 +1752,6 @@ en:
       title: "No such message"
       heading: "No such message"
       body: "Sorry there is no message with that id."
       title: "No such message"
       heading: "No such message"
       body: "Sorry there is no message with that id."

-    reply:
-      wrong_user: "You are logged in as '%{user}' but the message you have asked to reply to was not sent to that user. Please log in as the correct user in order to reply."

     show:
       title: "Read message"
       reply_button: "Reply"
     show:
       title: "Read message"
       reply_button: "Reply"


@@ -1813,6 +1811,9 @@ en:
         people_mapping_nearby: "people mapping nearby"
       message:
         destroy_button: "Delete"
         people_mapping_nearby: "people mapping nearby"
       message:
         destroy_button: "Delete"

+    replies:
+      new:
+        wrong_user: "You are logged in as '%{user}' but the message you have asked to reply to was not sent to that user. Please log in as the correct user in order to reply."

   passwords:
     new:
       title: "Lost password"
   passwords:
     new:
       title: "Lost password"

diff --git a/config/routes.rb b/config/routes.rb
index 4ab8f307b008e3ff561f9671a15b1dd13378f77b..d213030c6729466bb2f0b48144ff60e284c25063 100644 (file)
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -315,7 +315,7 @@ OpenStreetMap::Application.routes.draw do
     post :mark
     patch :unmute
 
     post :mark
     patch :unmute
 

-    match :reply, :via => [:get, :post]
+    resource :reply, :module => :messages, :path_names => { :new => "new" }, :only => :new

   end
   namespace :messages, :path => "/messages" do
     resource :inbox, :only => :show
   end
   namespace :messages, :path => "/messages" do
     resource :inbox, :only => :show


@@ -326,6 +326,7 @@ OpenStreetMap::Application.routes.draw do
   get "/user/:display_name/outbox", :to => redirect(:path => "/messages/outbox")
   get "/message/new/:display_name", :to => redirect(:path => "/messages/new/%{display_name}")
   get "/message/read/:message_id", :to => redirect(:path => "/messages/%{message_id}")
   get "/user/:display_name/outbox", :to => redirect(:path => "/messages/outbox")
   get "/message/new/:display_name", :to => redirect(:path => "/messages/new/%{display_name}")
   get "/message/read/:message_id", :to => redirect(:path => "/messages/%{message_id}")

+  get "/messages/:message_id/reply", :to => redirect(:path => "/messages/%{message_id}/reply/new")

 
   # muting users
   scope "/user/:display_name" do
 
   # muting users
   scope "/user/:display_name" do

diff --git a/test/controllers/messages/replies_controller_test.rb b/test/controllers/messages/replies_controller_test.rb
new file mode 100644 (file)
index 0000000..a839a44
--- /dev/null
+++ b/test/controllers/messages/replies_controller_test.rb
@@ -0,0 +1,69 @@
+require "test_helper"
+
+module Messages
+  class RepliesControllerTest < ActionDispatch::IntegrationTest
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/messages/1/reply/new", :method => :get },
+        { :controller => "messages/replies", :action => "new", :message_id => "1" }
+      )
+    end
+
+    def test_new
+      user = create(:user)
+      recipient_user = create(:user)
+      other_user = create(:user)
+      message = create(:message, :unread, :sender => user, :recipient => recipient_user)
+
+      # Check that the message reply page requires us to login
+      get new_message_reply_path(message)
+      assert_redirected_to login_path(:referer => new_message_reply_path(message))
+
+      # Login as the wrong user
+      session_for(other_user)
+
+      # Check that we can't reply to somebody else's message
+      get new_message_reply_path(message)
+      assert_redirected_to login_path(:referer => new_message_reply_path(message))
+      assert_equal "You are logged in as '#{other_user.display_name}' but the message you have asked to reply to was not sent to that user. Please log in as the correct user in order to reply.", flash[:notice]
+
+      # Login as the right user
+      session_for(recipient_user)
+
+      # Check that the message reply page loads
+      get new_message_reply_path(message)
+      assert_response :success
+      assert_template "new"
+      assert_select "title", "Re: #{message.title} | OpenStreetMap"
+      assert_select "form[action='/messages']", :count => 1 do
+        assert_select "input[type='hidden'][name='display_name'][value='#{user.display_name}']"
+        assert_select "input#message_title[value='Re: #{message.title}']", :count => 1
+        assert_select "textarea#message_body", :count => 1
+        assert_select "input[type='submit'][value='Send']", :count => 1
+      end
+      assert Message.find(message.id).message_read
+
+      # Login as the sending user
+      session_for(user)
+
+      # Check that the message reply page loads
+      get new_message_reply_path(message)
+      assert_response :success
+      assert_template "new"
+      assert_select "title", "Re: #{message.title} | OpenStreetMap"
+      assert_select "form[action='/messages']", :count => 1 do
+        assert_select "input[type='hidden'][name='display_name'][value='#{recipient_user.display_name}']"
+        assert_select "input#message_title[value='Re: #{message.title}']", :count => 1
+        assert_select "textarea#message_body", :count => 1
+        assert_select "input[type='submit'][value='Send']", :count => 1
+      end
+
+      # Asking to reply to a message with a bogus ID should fail
+      get new_message_reply_path(99999)
+      assert_response :not_found
+      assert_template "no_such_message"
+    end
+  end
+end

diff --git a/test/controllers/messages_controller_test.rb b/test/controllers/messages_controller_test.rb
index f72e695936813142a723b4aaef28cf6bb8c4f0ec..9249908929426d32086ee4297f41d1cacf6198e1 100644 (file)
--- a/test/controllers/messages_controller_test.rb
+++ b/test/controllers/messages_controller_test.rb
@@ -20,14 +20,6 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
       { :path => "/messages/1/mark", :method => :post },
       { :controller => "messages", :action => "mark", :message_id => "1" }
     )
       { :path => "/messages/1/mark", :method => :post },
       { :controller => "messages", :action => "mark", :message_id => "1" }
     )

-    assert_routing(
-      { :path => "/messages/1/reply", :method => :get },
-      { :controller => "messages", :action => "reply", :message_id => "1" }
-    )
-    assert_routing(
-      { :path => "/messages/1/reply", :method => :post },
-      { :controller => "messages", :action => "reply", :message_id => "1" }
-    )

     assert_routing(
       { :path => "/messages/1", :method => :delete },
       { :controller => "messages", :action => "destroy", :id => "1" }
     assert_routing(
       { :path => "/messages/1", :method => :delete },
       { :controller => "messages", :action => "destroy", :id => "1" }


@@ -219,63 +211,6 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
     end
   end
 

-  ##
-  # test the reply action
-  def test_reply
-    user = create(:user)
-    recipient_user = create(:user)
-    other_user = create(:user)
-    message = create(:message, :unread, :sender => user, :recipient => recipient_user)
-
-    # Check that the message reply page requires us to login
-    get message_reply_path(message)
-    assert_redirected_to login_path(:referer => message_reply_path(message))
-
-    # Login as the wrong user
-    session_for(other_user)
-
-    # Check that we can't reply to somebody else's message
-    get message_reply_path(message)
-    assert_redirected_to login_path(:referer => message_reply_path(message))
-    assert_equal "You are logged in as '#{other_user.display_name}' but the message you have asked to reply to was not sent to that user. Please log in as the correct user in order to reply.", flash[:notice]
-
-    # Login as the right user
-    session_for(recipient_user)
-
-    # Check that the message reply page loads
-    get message_reply_path(message)
-    assert_response :success
-    assert_template "new"
-    assert_select "title", "Re: #{message.title} | OpenStreetMap"
-    assert_select "form[action='/messages']", :count => 1 do
-      assert_select "input[type='hidden'][name='display_name'][value='#{user.display_name}']"
-      assert_select "input#message_title[value='Re: #{message.title}']", :count => 1
-      assert_select "textarea#message_body", :count => 1
-      assert_select "input[type='submit'][value='Send']", :count => 1
-    end
-    assert Message.find(message.id).message_read
-
-    # Login as the sending user
-    session_for(user)
-
-    # Check that the message reply page loads
-    get message_reply_path(message)
-    assert_response :success
-    assert_template "new"
-    assert_select "title", "Re: #{message.title} | OpenStreetMap"
-    assert_select "form[action='/messages']", :count => 1 do
-      assert_select "input[type='hidden'][name='display_name'][value='#{recipient_user.display_name}']"
-      assert_select "input#message_title[value='Re: #{message.title}']", :count => 1
-      assert_select "textarea#message_body", :count => 1
-      assert_select "input[type='submit'][value='Send']", :count => 1
-    end
-
-    # Asking to reply to a message with a bogus ID should fail
-    get message_reply_path(99999)
-    assert_response :not_found
-    assert_template "no_such_message"
-  end
-

   ##
   # test the show action
   def test_show
   ##
   # test the show action
   def test_show