From: Andy Allan Date: Wed, 29 Aug 2018 06:18:20 +0000 (+0800) Subject: Refactor message creation to use a create action X-Git-Tag: live~2906^2 X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/f38e03f0ed01d7624a6a8ae79234ad1ac3918f2b?hp=06915a77b5af2ab0354e895c3c1ceb0e028f37fb 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 d14605692..6af860671 100644 --- 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 e33bc176d..2c008732c 100644 --- 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 %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index a403d2959..ba9b3be34 100644 --- 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 6a3efe3b0..a8c216bf5 100644 --- 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 bffe3e1df..c5a117bcc 100644 --- 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