From 4bd95493b169dadfcebe45364ecbf156a76f9a9e Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Thu, 2 Jan 2025 11:41:43 +0300 Subject: [PATCH] Use resourceful route for message reply --- app/abilities/ability.rb | 2 +- .../messages/replies_controller.rb | 50 ++++++++++++++ app/controllers/messages_controller.rb | 37 +--------- app/mailers/user_mailer.rb | 2 +- app/views/messages/show.html.erb | 2 +- config/locales/en.yml | 5 +- config/routes.rb | 3 +- .../messages/replies_controller_test.rb | 69 +++++++++++++++++++ test/controllers/messages_controller_test.rb | 65 ----------------- 9 files changed, 128 insertions(+), 107 deletions(-) create mode 100644 app/controllers/messages/replies_controller.rb create mode 100644 test/controllers/messages/replies_controller_test.rb diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index 6638016d72..a2c19cfc91 100644 --- a/app/abilities/ability.rb +++ b/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 diff --git a/app/controllers/messages/replies_controller.rb b/app/controllers/messages/replies_controller.rb new file mode 100644 index 0000000000..b1e7b8d192 --- /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 7162b900a0..26e8a5e09e 100644 --- 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 :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]) diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index dee3dafbed..1dd13fb2d2 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -78,7 +78,7 @@ def message_notification(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) diff --git a/app/views/messages/show.html.erb b/app/views/messages/show.html.erb index 99d6d04355..d3147c9699 100644 --- a/app/views/messages/show.html.erb +++ b/app/views/messages/show.html.erb @@ -18,7 +18,7 @@
<%= @message.body.to_html %>
- <%= 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" %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 4f21742065..2a34c0a599 100644 --- 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." - 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" diff --git a/config/routes.rb b/config/routes.rb index 3ede0d33d5..b85a59500d 100644 --- a/config/routes.rb +++ b/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 diff --git a/test/controllers/messages/replies_controller_test.rb b/test/controllers/messages/replies_controller_test.rb new file mode 100644 index 0000000000..a839a44582 --- /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 ac7ca9a061..229777c65b 100644 --- a/test/controllers/messages_controller_test.rb +++ b/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