Create message read_mark resource

app/abilities/ability.rb

@@ -43,7 +43,7 @@ class Ability
         can :update, DiaryEntry, :user => user
         can [:create], DiaryComment
         can [:show, :create, :destroy], Follow
-        can [:read, :create, :mark, :unmute, :destroy], Message
+        can [:read, :create, :unmute, :destroy], Message
         can [:close, :reopen], Note
         can [:read, :update], :preference
         can :update, :profile

app/controllers/messages/read_marks_controller.rb

@@ -0,0 +1,39 @@
+module Messages
+  class ReadMarksController < ApplicationController
+    layout "site"
+
+    before_action :authorize_web
+    before_action :set_locale
+
+    authorize_resource :message
+
+    before_action :check_database_readable
+    before_action :check_database_writable
+
+    def create
+      mark true
+    end
+
+    def destroy
+      mark false
+    end
+
+    private
+
+    def mark(message_read)
+      @message = current_user.messages.unscope(:where => :muted).find(params[:message_id])
+      @message.message_read = message_read
+      if @message.save
+        flash[:notice] = t ".notice"
+        if @message.muted?
+          redirect_to messages_muted_inbox_path, :status => :see_other
+        else
+          redirect_to messages_inbox_path, :status => :see_other
+        end
+      end
+    rescue ActiveRecord::RecordNotFound
+      @title = t "messages.no_such_message.title"
+      render :template => "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, :mark, :destroy]
+  before_action :check_database_writable, :only => [:new, :create, :destroy]
 
   allow_thirdparty_images :only => [:new, :create, :show]
 
@@ -73,30 +73,6 @@ class MessagesController < ApplicationController
     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])
-    if params[:mark] == "unread"
-      message_read = false
-      notice = t ".as_unread"
-    else
-      message_read = true
-      notice = t ".as_read"
-    end
-    @message.message_read = message_read
-    if @message.save
-      flash[:notice] = notice
-      if @message.muted?
-        redirect_to messages_muted_inbox_path, :status => :see_other
-      else
-        redirect_to messages_inbox_path, :status => :see_other
-      end
-    end
-  rescue ActiveRecord::RecordNotFound
-    @title = t "messages.no_such_message.title"
-    render :action => "no_such_message", :status => :not_found
-  end
-
   # Moves message into Inbox by unsetting the muted-flag
   def unmute
     message = current_user.muted_messages.find(params[:message_id])

app/views/messages/mailboxes/_message.html.erb

@@ -4,8 +4,8 @@
   <td class="text-nowrap"><%= l message.sent_on, :format => :friendly %></td>
   <td class="text-nowrap">
     <div class="d-flex justify-content-end gap-1">
-      <%= button_to t(".unread_button"), message_mark_path(message, :mark => "unread"), :class => "btn btn-sm btn-primary", :form => { :data => { :turbo => true }, :class => "inbox-mark-unread", :hidden => !message.message_read? } %>
-      <%= button_to t(".read_button"), message_mark_path(message, :mark => "read"), :class => "btn btn-sm btn-primary", :form => { :data => { :turbo => true }, :class => "inbox-mark-read", :hidden => message.message_read? } %>
+      <%= button_to t(".unread_button"), message_read_mark_path(message), :method => :delete, :class => "btn btn-sm btn-primary", :form => { :data => { :turbo => true }, :class => "inbox-mark-unread", :hidden => !message.message_read? } %>
+      <%= button_to t(".read_button"), message_read_mark_path(message), :method => :post, :class => "btn btn-sm btn-primary", :form => { :data => { :turbo => true }, :class => "inbox-mark-read", :hidden => message.message_read? } %>
       <%= button_to t(".destroy_button"), message_path(message, :referer => request.fullpath), :method => :delete, :class => "btn btn-sm btn-danger", :form => { :data => { :turbo => true }, :class => "destroy-message" } %>
       <% if message.muted? %>
         <%= button_to t(".unmute_button"), message_unmute_path(message), :method => :patch, :class => "btn btn-sm btn-secondary", :form => { :data => { :turbo => true } } %>

app/views/messages/show.html.erb

@@ -20,7 +20,7 @@
 <div>
   <%= 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(".unread_button"), message_read_mark_path(@message), :method => "delete", :class => "btn btn-primary" %>
     <%= link_to t(".destroy_button"), message_path(@message), :method => "delete", :class => "btn btn-danger" %>
   <% else %>
     <%= link_to t(".destroy_button"), message_path(@message), :method => "delete", :class => "btn btn-danger" %>

config/locales/en.yml

@@ -1782,14 +1782,16 @@ en:
       destroy_button: "Delete"
       back: "Back"
       wrong_user: "You are logged in as '%{user}' but the message you have asked to read was not sent by or to that user. Please log in as the correct user in order to read it."
-    mark:
-      as_read: "Message marked as read"
-      as_unread: "Message marked as unread"
     unmute:
       notice: "Message has been moved to Inbox"
       error: "The message could not be moved to the Inbox."
     destroy:
       destroyed: "Message deleted"
+    read_marks:
+      create:
+        notice: "Message marked as read"
+      destroy:
+        notice: "Message marked as unread"
     mailboxes:
       heading:
         my_inbox: "My Inbox"

config/routes.rb

@@ -322,10 +322,12 @@ OpenStreetMap::Application.routes.draw do
 
   # messages
   resources :messages, :path_names => { :new => "new/:display_name" }, :id => /\d+/, :only => [:new, :create, :show, :destroy] do
-    post :mark
     patch :unmute
 
-    resource :reply, :module => :messages, :path_names => { :new => "new" }, :only => :new
+    scope :module => :messages do
+      resource :reply, :path_names => { :new => "new" }, :only => :new
+      resource :read_mark, :only => [:create, :destroy]
+    end
   end
   namespace :messages, :path => "/messages" do
     resource :inbox, :only => :show

test/controllers/messages/read_marks_controller_test.rb

@@ -0,0 +1,126 @@
+require "test_helper"
+
+module Messages
+  class ReadMarksControllerTest < ActionDispatch::IntegrationTest
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/messages/1/read_mark", :method => :post },
+        { :controller => "messages/read_marks", :action => "create", :message_id => "1" }
+      )
+      assert_routing(
+        { :path => "/messages/1/read_mark", :method => :delete },
+        { :controller => "messages/read_marks", :action => "destroy", :message_id => "1" }
+      )
+    end
+
+    def test_create_when_not_logged_in
+      message = create(:message, :unread)
+
+      post message_read_mark_path(message)
+      assert_response :forbidden
+    end
+
+    def test_create_as_other_user
+      message = create(:message, :unread)
+      session_for(create(:user))
+
+      post message_read_mark_path(message)
+      assert_response :not_found
+      assert_template "no_such_message"
+    end
+
+    def test_create_as_sender
+      message = create(:message, :unread)
+      session_for(message.sender)
+
+      post message_read_mark_path(message)
+      assert_response :not_found
+      assert_template "no_such_message"
+    end
+
+    def test_create_as_recipient
+      message = create(:message, :unread)
+      session_for(message.recipient)
+
+      post message_read_mark_path(message)
+      assert_redirected_to messages_inbox_path
+      assert message.reload.message_read
+    end
+
+    def test_create_on_missing_message
+      session_for(create(:user))
+
+      post message_read_mark_path(99999)
+      assert_response :not_found
+      assert_template "no_such_message"
+    end
+
+    def test_create_on_message_from_muted_user
+      sender_user = create(:user)
+      recipient_user = create(:user)
+      create(:user_mute, :owner => recipient_user, :subject => sender_user)
+      message = create(:message, :unread, :sender => sender_user, :recipient => recipient_user)
+      session_for(recipient_user)
+
+      post message_read_mark_path(message)
+      assert_redirected_to messages_muted_inbox_path
+      assert message.reload.message_read
+    end
+
+    def test_destroy_when_not_logged_in
+      message = create(:message, :read)
+
+      delete message_read_mark_path(message)
+      assert_response :forbidden
+    end
+
+    def test_destroy_as_other_user
+      message = create(:message, :read)
+      session_for(create(:user))
+
+      delete message_read_mark_path(message)
+      assert_response :not_found
+      assert_template "no_such_message"
+    end
+
+    def test_destroy_as_sender
+      message = create(:message, :read)
+      session_for(message.sender)
+
+      delete message_read_mark_path(message)
+      assert_response :not_found
+      assert_template "no_such_message"
+    end
+
+    def test_destroy_as_recipient
+      message = create(:message, :read)
+      session_for(message.recipient)
+
+      delete message_read_mark_path(message)
+      assert_redirected_to messages_inbox_path
+      assert_not message.reload.message_read
+    end
+
+    def test_destroy_on_missing_message
+      session_for(create(:user))
+
+      delete message_read_mark_path(99999)
+      assert_response :not_found
+      assert_template "no_such_message"
+    end
+
+    def test_destroy_on_message_from_muted_user
+      sender_user = create(:user)
+      recipient_user = create(:user)
+      create(:user_mute, :owner => recipient_user, :subject => sender_user)
+      message = create(:message, :read, :sender => sender_user, :recipient => recipient_user)
+      session_for(recipient_user)
+
+      delete message_read_mark_path(message, :mark => "unread")
+      assert_redirected_to messages_muted_inbox_path
+      assert_not message.reload.message_read
+    end
+  end
+end

test/controllers/messages_controller_test.rb

@@ -16,10 +16,6 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
       { :path => "/messages/1", :method => :get },
       { :controller => "messages", :action => "show", :id => "1" }
     )
-    assert_routing(
-      { :path => "/messages/1/mark", :method => :post },
-      { :controller => "messages", :action => "mark", :message_id => "1" }
-    )
     assert_routing(
       { :path => "/messages/1", :method => :delete },
       { :controller => "messages", :action => "destroy", :id => "1" }
@@ -259,84 +255,6 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     assert_template "no_such_message"
   end
 
-  ##
-  # test the mark action
-  def test_mark
-    sender_user = create(:user)
-    recipient_user = create(:user)
-    other_user = create(:user)
-    message = create(:message, :unread, :sender => sender_user, :recipient => recipient_user)
-
-    # Check that the marking a message requires us to login
-    post message_mark_path(message)
-    assert_response :forbidden
-
-    # Login as a user with no messages
-    session_for(other_user)
-
-    # Check that marking a message we didn't send or receive fails
-    post message_mark_path(message)
-    assert_response :not_found
-    assert_template "no_such_message"
-
-    # Login as the message sender_user
-    session_for(sender_user)
-
-    # Check that marking a message we sent fails
-    post message_mark_path(message)
-    assert_response :not_found
-    assert_template "no_such_message"
-
-    # Login as the message recipient_user
-    session_for(recipient_user)
-
-    # Check that the marking a message read works
-    post message_mark_path(message, :mark => "read")
-    assert_redirected_to messages_inbox_path
-    assert Message.find(message.id).message_read
-
-    # Check that the marking a message unread works
-    post message_mark_path(message, :mark => "unread")
-    assert_redirected_to messages_inbox_path
-    assert_not Message.find(message.id).message_read
-
-    # Check that the marking a message read works and redirects to inbox from the message page
-    post message_mark_path(message, :mark => "read"), :headers => { :referer => message_path(message) }
-    assert_redirected_to messages_inbox_path
-    assert Message.find(message.id).message_read
-
-    # Check that the marking a message unread works and redirects to inbox from the message page
-    post message_mark_path(message, :mark => "unread"), :headers => { :referer => message_path(message) }
-    assert_redirected_to messages_inbox_path
-    assert_not Message.find(message.id).message_read
-
-    # Asking to mark a message with a bogus ID should fail
-    post message_mark_path(99999)
-    assert_response :not_found
-    assert_template "no_such_message"
-  end
-
-  ##
-  # test the mark action for messages from muted users
-  def test_mark_muted
-    sender_user = create(:user)
-    recipient_user = create(:user)
-    create(:user_mute, :owner => recipient_user, :subject => sender_user)
-    message = create(:message, :unread, :sender => sender_user, :recipient => recipient_user)
-
-    session_for(recipient_user)
-
-    # Check that the marking a message read works
-    post message_mark_path(message, :mark => "read")
-    assert_redirected_to messages_muted_inbox_path
-    assert Message.find(message.id).message_read
-
-    # Check that the marking a message unread works
-    post message_mark_path(message, :mark => "unread")
-    assert_redirected_to messages_muted_inbox_path
-    assert_not Message.find(message.id).message_read
-  end
-
   ##
   # test the destroy action
   def test_destroy