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.

diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb
index d1460569243045db32ab88b05d7169771aa0ae15..6af860671d1190bdec8c06e6996b4c84bc21b6a9 100644 (file)
--- a/app/controllers/messages_controller.rb
+++ b/app/controllers/messages_controller.rb
@@ -4,37 +4,37 @@ 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 :allow_thirdparty_images, :only => [:new, :show]
+  before_action :check_database_writable, :only => [:new, :create, :reply, :mark, :destroy]
+  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?
-      if current_user.sent_messages.where("sent_on >= ?", Time.now.getutc - 1.hour).count >= MAX_MESSAGES_PER_HOUR
-        flash[:error] = t ".limit_exceeded"
-      else
-        @message = Message.new(message_params)
-        @message.recipient = @user
-        @message.sender = current_user
-        @message.sent_on = Time.now.getutc
-
-        if @message.save
-          flash[:notice] = t ".message_sent"
-          Notifier.message_notification(@message).deliver_now
-          redirect_to :action => :inbox
-        end
-      end
-    end
-
-    @message ||= Message.new(message_params.merge(:recipient => @user))
+    @message = Message.new(message_params.merge(:recipient => @user))
     @title = t ".title"
   end
 
+  def create
+    @message = Message.new(message_params)
+    @message.recipient = @user
+    @message.sender = current_user
+    @message.sent_on = Time.now.getutc
+
+    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
+    render :action => "new"
+  end
+
   # Allow the user to reply to another message.
   def reply
     message = Message.find(params[:message_id])

diff --git a/app/views/messages/new.html.erb b/app/views/messages/new.html.erb
index e33bc176d107427b31678f9fbe2602111348a119..2c008732c1eb389b411c6df0c3e8e7f3b00631d1 100644 (file)
--- a/app/views/messages/new.html.erb
+++ b/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>

diff --git a/config/locales/en.yml b/config/locales/en.yml
index a403d29592d189ecc9f8823c09807c50839ce730..ba9b3be342b28a53e9eb159da50355be15682b50 100644 (file)
--- a/config/locales/en.yml
+++ b/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:

diff --git a/config/routes.rb b/config/routes.rb
index 6a3efe3b05674370850163e370c9ccade1452c1b..a8c216bf5e961e23199dc20f4244bb80d8807523 100644 (file)
--- a/config/routes.rb
+++ b/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"

diff --git a/test/controllers/messages_controller_test.rb b/test/controllers/messages_controller_test.rb
index bffe3e1df57710708c844b916504eff06e15de99..c5a117bcc4cecaa9ee219f036dfa87941850d54d 100644 (file)
--- a/test/controllers/messages_controller_test.rb
+++ b/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 },
-      { :controller => "messages", :action => "new", :display_name => "username" }
+      { :path => "/messages", :method => :post },
+      { :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