Create message read_mark resource

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index d4172bfd16470ea9091123c72591161bd0dfcfca..262e92c26df4793fac47ce2a209d94c1b63d4f64 100644 (file)
--- a/app/abilities/ability.rb
+++ b/app/abilities/ability.rb
@@ -43,7 +43,7 @@ class Ability
         can :update, DiaryEntry, :user => user
         can [:create], DiaryComment
         can [:show, :create, :destroy], Follow
         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
         can [:close, :reopen], Note
         can [:read, :update], :preference
         can :update, :profile

diff --git a/app/controllers/messages/read_marks_controller.rb b/app/controllers/messages/read_marks_controller.rb
new file mode 100644 (file)
index 0000000..2db0b89
--- /dev/null
+++ b/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

diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb
index cc5f6c56d19dd4a6481534bc082eace36fa403ef..5a63dfd6e018f8179bf3028a28f800c516ceb236 100644 (file)
--- 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 :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]
 
 
   allow_thirdparty_images :only => [:new, :create, :show]
 


@@ -73,30 +73,6 @@ class MessagesController < ApplicationController
     render :action => "no_such_message", :status => :not_found
   end
 
     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])
   # Moves message into Inbox by unsetting the muted-flag
   def unmute
     message = current_user.muted_messages.find(params[:message_id])

diff --git a/app/views/messages/mailboxes/_message.html.erb b/app/views/messages/mailboxes/_message.html.erb
index 7213cf80dcedb66c8a20c2ecf06d4671ba372fc5..22ef4d7c28e304c5e868c925d420ee8540b8c330 100644 (file)
--- a/app/views/messages/mailboxes/_message.html.erb
+++ b/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">
   <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 } } %>
       <%= 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 } } %>

diff --git a/app/views/messages/show.html.erb b/app/views/messages/show.html.erb
index d3147c9699c3c24c95b1a12aa2c7d74acf24be0c..ff403b099b28d3206bc44c69d5a77b7e1777606c 100644 (file)
--- a/app/views/messages/show.html.erb
+++ b/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 %>
 <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" %>
     <%= 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" %>

diff --git a/config/locales/en.yml b/config/locales/en.yml
index a7125a3a7d5c8602597b14d29604bdf2fba3ec01..d1df36f8388289bf5ba2bbc68b868a6aab8a53cb 100644 (file)
--- a/config/locales/en.yml
+++ b/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."
       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"
     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"
     mailboxes:
       heading:
         my_inbox: "My Inbox"

diff --git a/config/routes.rb b/config/routes.rb
index d8e83b97b3838cdb725249dd0824b9f312af5a8f..453c625adea0b8c28f9b83cdd2a832f469cdd4ac 100644 (file)
--- a/config/routes.rb
+++ b/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
 
   # messages
   resources :messages, :path_names => { :new => "new/:display_name" }, :id => /\d+/, :only => [:new, :create, :show, :destroy] do

-    post :mark

     patch :unmute
 
     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
   end
   namespace :messages, :path => "/messages" do
     resource :inbox, :only => :show

diff --git a/test/controllers/messages/read_marks_controller_test.rb b/test/controllers/messages/read_marks_controller_test.rb
new file mode 100644 (file)
index 0000000..e02e46b
--- /dev/null
+++ b/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

diff --git a/test/controllers/messages_controller_test.rb b/test/controllers/messages_controller_test.rb
index b2bb71b1c71e8b1484983d9956f89487b55d5d21..bdf88600cb9f75bae864dc1ea9fe4b2bd5938b50 100644 (file)
--- a/test/controllers/messages_controller_test.rb
+++ b/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" }
     )
       { :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" }
     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
 
     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
   ##
   # test the destroy action
   def test_destroy