Refactor message creation to use a create action

This makes it more conventional, rather than handling posts to the new action. The posting of the form was also reworked to use a hidden field for the displayname, rather than in the url, again for convention.

app/controllers/messages_controller.rb

@@ -4,35 +4,35 @@ class MessagesController < ApplicationController
   before_action :authorize_web
   before_action :set_locale
   before_action :require_user
-  before_action :lookup_user, :only => [:new]
+  before_action :lookup_user, :only => [:new, :create]
   before_action :check_database_readable
-  before_action :check_database_writable, :only => [:new, :reply, :mark, :destroy]
+  before_action :check_database_writable, :only => [:new, :create, :reply, :mark, :destroy]
-  before_action :allow_thirdparty_images, :only => [:new, :show]
+  before_action :allow_thirdparty_images, :only => [:new, :create, :show]
 
   # Allow the user to write a new message to another user. This action also
   # deals with the sending of that message to the other user when the user
   # clicks send.
   # The display_name param is the display name of the user that the message is being sent to.
   def new
-    if request.post?
+    @message = Message.new(message_params.merge(:recipient => @user))
-      if current_user.sent_messages.where("sent_on >= ?", Time.now.getutc - 1.hour).count >= MAX_MESSAGES_PER_HOUR
+    @title = t ".title"
-        flash[:error] = t ".limit_exceeded"
+  end
-      else
+
+  def create
     @message = Message.new(message_params)
     @message.recipient = @user
     @message.sender = current_user
     @message.sent_on = Time.now.getutc
 
-        if @message.save
+    if current_user.sent_messages.where("sent_on >= ?", Time.now.getutc - 1.hour).count >= MAX_MESSAGES_PER_HOUR
+      flash[:error] = t ".limit_exceeded"
+    elsif @message.save
       flash[:notice] = t ".message_sent"
       Notifier.message_notification(@message).deliver_now
       redirect_to :action => :inbox
+      return
     end
-      end
+    render :action => "new"
-    end
-
-    @message ||= Message.new(message_params.merge(:recipient => @user))
-    @title = t ".title"
   end
 
   # Allow the user to reply to another message.

app/views/messages/new.html.erb

@@ -4,7 +4,8 @@
 
 <%= error_messages_for 'message' %>
 
-<%= form_for :message, :html => { :class => 'standard-form' }, :url => new_message_path(@message.recipient) do |f| %>
+<%= form_for @message, :html => { :class => 'standard-form' } do |f| %>
+  <%= hidden_field_tag :display_name, @message.recipient.display_name %>
   <fieldset>
     <div class='form-row'>
       <label class="standard-label" for="message_title"><%= t '.subject' %></label>

config/locales/en.yml

@@ -1194,6 +1194,7 @@ en:
       body: "Body"
       send_button: "Send"
       back_to_inbox: "Back to inbox"
+    create:
       message_sent: "Message sent"
       limit_exceeded: "You have sent a lot of messages recently. Please wait a while before trying to send any more."
     no_such_message:

config/routes.rb

@@ -264,7 +264,7 @@ OpenStreetMap::Application.routes.draw do
   get "/export/embed" => "export#embed"
 
   # messages
-  resources :messages, :only => [:show] do
+  resources :messages, :only => [:create, :show] do
     collection do
       get :inbox
       get :outbox
@@ -272,7 +272,7 @@ OpenStreetMap::Application.routes.draw do
   end
   get "/user/:display_name/inbox", :to => redirect(:path => "/messages/inbox")
   get "/user/:display_name/outbox", :to => redirect(:path => "/messages/outbox")
-  match "/message/new/:display_name" => "messages#new", :via => [:get, :post], :as => "new_message"
+  get "/message/new/:display_name" => "messages#new", :as => "new_message"
   get "/message/read/:message_id", :to => redirect(:path => "/messages/%{message_id}")
   post "/message/mark/:message_id" => "messages#mark", :as => "mark_message"
   match "/message/reply/:message_id" => "messages#reply", :via => [:get, :post], :as => "reply_message"

test/controllers/messages_controller_test.rb

@@ -17,8 +17,8 @@ class MessagesControllerTest < ActionController::TestCase
       { :controller => "messages", :action => "new", :display_name => "username" }
     )
     assert_routing(
-      { :path => "/message/new/username", :method => :post },
+      { :path => "/messages", :method => :post },
-      { :controller => "messages", :action => "new", :display_name => "username" }
+      { :controller => "messages", :action => "create" }
     )
     assert_routing(
       { :path => "/messages/1", :method => :get },
@@ -64,7 +64,8 @@ class MessagesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template "new"
     assert_select "title", "Send message | OpenStreetMap"
-    assert_select "form[action='#{new_message_path(:display_name => recipient_user.display_name)}']", :count => 1 do
+    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", :count => 1
       assert_select "textarea#message_body", :count => 1
       assert_select "input[type='submit'][value='Send']", :count => 1
@@ -90,7 +91,8 @@ class MessagesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template "new"
     assert_select "title", "Send message | OpenStreetMap"
-    assert_select "form[action='#{new_message_path(:display_name => recipient_user.display_name)}']", :count => 1 do
+    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", :count => 1 do
         assert_select "[value='Test Message']"
       end
@@ -118,7 +120,8 @@ class MessagesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template "new"
     assert_select "title", "Send message | OpenStreetMap"
-    assert_select "form[action='#{new_message_path(:display_name => recipient_user.display_name)}']", :count => 1 do
+    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", :count => 1 do
         assert_select "[value='Test Message']"
       end
@@ -146,7 +149,8 @@ class MessagesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template "new"
     assert_select "title", "Send message | OpenStreetMap"
-    assert_select "form[action='#{new_message_path(:display_name => recipient_user.display_name)}']", :count => 1 do
+    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", :count => 1 do
         assert_select "[value='']"
       end
@@ -166,7 +170,7 @@ class MessagesControllerTest < ActionController::TestCase
     # Check that sending a message works
     assert_difference "ActionMailer::Base.deliveries.size", 1 do
       assert_difference "Message.count", 1 do
-        post :new,
+        post :create,
              :params => { :display_name => recipient_user.display_name,
                           :message => { :title => "Test Message", :body => "Test message body" } }
       end
@@ -207,7 +211,7 @@ class MessagesControllerTest < ActionController::TestCase
     assert_no_difference "ActionMailer::Base.deliveries.size" do
       assert_no_difference "Message.count" do
         with_message_limit(0) do
-          post :new,
+          post :create,
                :params => { :display_name => recipient_user.display_name,
                             :message => { :title => "Test Message", :body => "Test message body" } }
           assert_response :success
@@ -246,7 +250,8 @@ class MessagesControllerTest < ActionController::TestCase
     assert_response :success
     assert_template "new"
     assert_select "title", "Re: #{unread_message.title} | OpenStreetMap"
-    assert_select "form[action='#{new_message_path(:display_name => user.display_name)}']", :count => 1 do
+    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: #{unread_message.title}']", :count => 1
       assert_select "textarea#message_body", :count => 1
       assert_select "input[type='submit'][value='Send']", :count => 1