Use resourceful route for message reply

app/abilities/ability.rb

@@ -42,7 +42,7 @@ def initialize(user)
         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, :inbox, :outbox, :muted, :mark, :unmute, :destroy], Message
         can [:close, :reopen], Note
         can [:read, :update], :preference
         can :update, :profile

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

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 :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]
 
@@ -73,41 +73,6 @@ def destroy
     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])

app/views/messages/show.html.erb

@@ -18,7 +18,7 @@
 <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" %>

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."
-    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"
@@ -1813,6 +1811,9 @@ en:
         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"

config/routes.rb

@@ -315,7 +315,7 @@
     post :mark
     patch :unmute
 
-    match :reply, :via => [:get, :post]
+    resource :reply, :module => :messages, :only => :new
   end
   namespace :messages, :path => "/messages" do
     resource :inbox, :only => :show
@@ -326,6 +326,7 @@
   get "/user/:display_name/outbox", :to => redirect(:path => "/messages/outbox")
   get "/message/new/:display_name" => "messages#new", :as => "new_message"
   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

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

test/controllers/messages_controller_test.rb

@@ -20,14 +20,6 @@ def test_routes
       { :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" }
@@ -219,63 +211,6 @@ def test_new_limit
     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