Use resourceful routes for web mailboxes

app/controllers/messages/inboxes_controller.rb

@@ -0,0 +1,8 @@
+module Messages
+  class InboxesController < MailboxesController
+    # Display the list of messages that have been sent to the user.
+    def show
+      @title = t ".title"
+    end
+  end
+end

app/controllers/messages/mailboxes_controller.rb

@@ -0,0 +1,12 @@
+module Messages
+  class MailboxesController < ApplicationController
+    layout "site"
+
+    before_action :authorize_web
+    before_action :set_locale
+
+    authorize_resource :class => Message
+
+    before_action :check_database_readable
+  end
+end

app/controllers/messages/muted_inboxes_controller.rb

@@ -0,0 +1,10 @@
+module Messages
+  class MutedInboxesController < MailboxesController
+    # Display the list of muted messages received by the user.
+    def show
+      @title = t ".title"
+
+      redirect_to messages_inbox_path if current_user.muted_messages.none?
+    end
+  end
+end

app/controllers/messages/outboxes_controller.rb

@@ -0,0 +1,8 @@
+module Messages
+  class OutboxesController < MailboxesController
+    # Display the list of messages that the user has sent to other users.
+    def show
+      @title = t ".title"
+    end
+  end
+end

app/controllers/messages_controller.rb

@@ -49,7 +49,7 @@ class MessagesController < ApplicationController
     elsif @message.save
       flash[:notice] = t ".message_sent"
       UserMailer.message_notification(@message).deliver_later if @message.notify_recipient?
-      redirect_to :action => :inbox
+      redirect_to messages_inbox_path
     else
       @title = t "messages.new.title"
       render :action => "new"
@@ -66,7 +66,7 @@ class MessagesController < ApplicationController
 
       referer = safe_referer(params[:referer]) if params[:referer]
 
-      redirect_to referer || { :action => :inbox }, :status => :see_other
+      redirect_to referer || messages_inbox_path, :status => :see_other
     end
   rescue ActiveRecord::RecordNotFound
     @title = t "messages.no_such_message.title"
@@ -108,23 +108,6 @@ class MessagesController < ApplicationController
     render :action => "no_such_message", :status => :not_found
   end
 
-  # Display the list of messages that have been sent to the user.
-  def inbox
-    @title = t ".title"
-  end
-
-  # Display the list of messages that the user has sent to other users.
-  def outbox
-    @title = t ".title"
-  end
-
-  # Display the list of muted messages received by the user.
-  def muted
-    @title = t ".title"
-
-    redirect_to inbox_messages_path if current_user.muted_messages.none?
-  end
-
   # Set the message as being read or unread.
   def mark
     @message = current_user.messages.unscope(:where => :muted).find(params[:message_id])
@@ -139,9 +122,9 @@ class MessagesController < ApplicationController
     if @message.save
       flash[:notice] = notice
       if @message.muted?
-        redirect_to muted_messages_path, :status => :see_other
+        redirect_to messages_muted_inbox_path, :status => :see_other
       else
-        redirect_to inbox_messages_path, :status => :see_other
+        redirect_to messages_inbox_path, :status => :see_other
       end
     end
   rescue ActiveRecord::RecordNotFound
@@ -160,9 +143,9 @@ class MessagesController < ApplicationController
     end
 
     if current_user.muted_messages.none?
-      redirect_to inbox_messages_path
+      redirect_to messages_inbox_path
     else
-      redirect_to muted_messages_path
+      redirect_to messages_muted_inbox_path
     end
   end
 

app/views/layouts/_header.html.erb

@@ -96,7 +96,7 @@
         </button>
         <div class='dropdown-menu dropdown-menu-end'>
           <%= link_to t("users.show.my_dashboard"), dashboard_path, :class => "dropdown-item" %>
-          <%= link_to inbox_messages_path, :class => "dropdown-item" do %>
+          <%= link_to messages_inbox_path, :class => "dropdown-item" do %>
             <%= t("users.show.my messages") %>
             <span class='badge count-number'><%= number_with_delimiter(current_user.new_messages.size) %></span>
           <% end %>

app/views/messages/inboxes/show.html.erb

@@ -2,12 +2,12 @@
   <%= javascript_include_tag "messages" %>
 <% end %>
 
-<%= render :partial => "heading", :locals => { :active_link_path => inbox_messages_path } %>
+<%= render :partial => "heading", :locals => { :active_link_path => messages_inbox_path } %>
 
-<h4><%= t "messages.inbox.messages", :new_messages => t(".new_messages", :count => current_user.new_messages.size), :old_messages => t(".old_messages", :count => current_user.messages.size - current_user.new_messages.size) %></h4>
+<h4><%= t ".messages", :new_messages => t(".new_messages", :count => current_user.new_messages.size), :old_messages => t(".old_messages", :count => current_user.messages.size - current_user.new_messages.size) %></h4>
 
 <% if current_user.messages.size > 0 %>
-  <%= render :partial => "messages_table", :locals => { :columns => %w[from subject date], :messages => current_user.messages, :inner_partial => "message_summary" } %>
+  <%= render :partial => "messages_table", :locals => { :columns => %w[from subject date], :messages => current_user.messages } %>
 <% else %>
   <div><%= t(".no_messages_yet_html", :people_mapping_nearby_link => link_to(t(".people_mapping_nearby"), dashboard_path)) %></div>
 <% end %>

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

@@ -3,8 +3,8 @@
 <% content_for :heading do %>
   <h1><%= t("users.show.my messages") %></h1>
   <ul class="nav nav-tabs">
-    <% { t(".my_inbox") => inbox_messages_path, t(".my_outbox") => outbox_messages_path, t(".muted_messages") => muted_messages_path }.each do |label, path| %>
-    <% next if path == muted_messages_path && current_user.muted_messages.none? %>
+    <% { t(".my_inbox") => messages_inbox_path, t(".my_outbox") => messages_outbox_path, t(".muted_messages") => messages_muted_inbox_path }.each do |label, path| %>
+    <% next if path == messages_muted_inbox_path && current_user.muted_messages.none? %>
 
     <li class="nav-item">
       <% if path == active_link_path %>

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

@@ -8,6 +8,6 @@
     </tr>
   </thead>
   <tbody>
-    <%= render :partial => inner_partial, :collection => messages, :as => "message" %>
+    <%= render :partial => "message", :collection => messages %>
   </tbody>
 </table>

app/views/messages/muted_inboxes/show.html.erb

@@ -2,8 +2,8 @@
   <%= javascript_include_tag "messages" %>
 <% end %>
 
-<%= render :partial => "heading", :locals => { :active_link_path => muted_messages_path } %>
+<%= render :partial => "heading", :locals => { :active_link_path => messages_muted_inbox_path } %>
 
 <h4><%= t ".messages", :count => current_user.muted_messages.size %></h4>
 
-<%= render :partial => "messages_table", :locals => { :columns => %w[from subject date], :messages => current_user.muted_messages, :inner_partial => "message_summary" } %>
+<%= render :partial => "messages_table", :locals => { :columns => %w[from subject date], :messages => current_user.muted_messages } %>

app/views/messages/new.html.erb

@@ -8,5 +8,5 @@
   <%= f.richtext_field :body, :cols => 80, :rows => 20 %>
 
   <%= f.primary %>
-  <%= link_to t(".back_to_inbox"), inbox_messages_path, :class => "btn btn-link" %>
+  <%= link_to t(".back_to_inbox"), messages_inbox_path, :class => "btn btn-link" %>
 <% end %>

app/views/messages/outboxes/show.html.erb

@@ -2,12 +2,12 @@
   <%= javascript_include_tag "messages" %>
 <% end %>
 
-<%= render :partial => "heading", :locals => { :active_link_path => outbox_messages_path } %>
+<%= render :partial => "heading", :locals => { :active_link_path => messages_outbox_path } %>
 
 <h4><%= t ".messages", :count => current_user.sent_messages.size %></h4>
 
 <% if current_user.sent_messages.size > 0 %>
-  <%= render :partial => "messages_table", :locals => { :columns => %w[to subject date], :messages => current_user.sent_messages, :inner_partial => "sent_message_summary" } %>
+  <%= render :partial => "messages_table", :locals => { :columns => %w[to subject date], :messages => current_user.sent_messages } %>
 <% else %>
   <div class="messages"><%= t(".no_sent_messages_html", :people_mapping_nearby_link => link_to(t(".people_mapping_nearby"), dashboard_path)) %></div>
 <% end %>

config/locales/en.yml

@@ -1741,28 +1741,6 @@ en:
       confirmation_sent: We've sent a new confirmation note to %{email} and as soon as you confirm your account you'll be able to get mapping.
       whitelist: If you use an antispam system which sends confirmation requests then please make sure you whitelist %{sender} as we are unable to reply to any confirmation requests.
   messages:
-    inbox:
-      title: "Inbox"
-      messages: "You have %{new_messages} and %{old_messages}"
-      new_messages:
-        one: "%{count} new message"
-        other: "%{count} new messages"
-      old_messages:
-        one: "%{count} old message"
-        other: "%{count} old messages"
-      no_messages_yet_html: "You have no messages yet. Why not get in touch with some of the %{people_mapping_nearby_link}?"
-      people_mapping_nearby: "people mapping nearby"
-    messages_table:
-      from: "From"
-      to: "To"
-      subject: "Subject"
-      date: "Date"
-      actions: "Actions"
-    message_summary:
-      unread_button: "Mark as unread"
-      read_button: "Mark as read"
-      destroy_button: "Delete"
-      unmute_button: "Move to Inbox"
     new:
       title: "Send message"
       send_message_to_html: "Send a new message to %{name}"
@@ -1774,18 +1752,6 @@ en:
       title: "No such message"
       heading: "No such message"
       body: "Sorry there is no message with that id."
-    outbox:
-      title: "Outbox"
-      messages:
-        one: "You have %{count} sent message"
-        other: "You have %{count} sent messages"
-      no_sent_messages_html: "You have no sent messages yet. Why not get in touch with some of the %{people_mapping_nearby_link}?"
-      people_mapping_nearby: "people mapping nearby"
-    muted:
-      title: "Muted Messages"
-      messages:
-        one: "%{count} muted message"
-        other: "You have %{count} muted messages"
     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:
@@ -1795,12 +1761,6 @@ 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."
-    sent_message_summary:
-      destroy_button: "Delete"
-    heading:
-      my_inbox: "My Inbox"
-      my_outbox: "My Outbox"
-      muted_messages: "Muted messages"
     mark:
       as_read: "Message marked as read"
       as_unread: "Message marked as unread"
@@ -1809,6 +1769,50 @@ en:
       error: "The message could not be moved to the Inbox."
     destroy:
       destroyed: "Message deleted"
+    mailboxes:
+      heading:
+        my_inbox: "My Inbox"
+        my_outbox: "My Outbox"
+        muted_messages: "Muted messages"
+      messages_table:
+        from: "From"
+        to: "To"
+        subject: "Subject"
+        date: "Date"
+        actions: "Actions"
+      message:
+        unread_button: "Mark as unread"
+        read_button: "Mark as read"
+        destroy_button: "Delete"
+        unmute_button: "Move to Inbox"
+    inboxes:
+      show:
+        title: "Inbox"
+        messages: "You have %{new_messages} and %{old_messages}"
+        new_messages:
+          one: "%{count} new message"
+          other: "%{count} new messages"
+        old_messages:
+          one: "%{count} old message"
+          other: "%{count} old messages"
+        no_messages_yet_html: "You have no messages yet. Why not get in touch with some of the %{people_mapping_nearby_link}?"
+        people_mapping_nearby: "people mapping nearby"
+    muted_inboxes:
+      show:
+        title: "Muted Messages"
+        messages:
+          one: "%{count} muted message"
+          other: "You have %{count} muted messages"
+    outboxes:
+      show:
+        title: "Outbox"
+        messages:
+          one: "You have %{count} sent message"
+          other: "You have %{count} sent messages"
+        no_sent_messages_html: "You have no sent messages yet. Why not get in touch with some of the %{people_mapping_nearby_link}?"
+        people_mapping_nearby: "people mapping nearby"
+      message:
+        destroy_button: "Delete"
   passwords:
     new:
       title: "Lost password"

config/routes.rb

@@ -307,16 +307,16 @@ OpenStreetMap::Application.routes.draw do
   get "/export/embed" => "export#embed"
 
   # messages
-  resources :messages, :only => [:create, :show, :destroy] do
+  resources :messages, :id => /\d+/, :only => [:create, :show, :destroy] do
     post :mark
     patch :unmute
 
     match :reply, :via => [:get, :post]
-    collection do
-      get :inbox
-      get :muted
-      get :outbox
   end
+  namespace :messages, :path => "/messages" do
+    resource :inbox, :only => :show
+    resource :muted_inbox, :path => "muted", :only => :show
+    resource :outbox, :only => :show
   end
   get "/user/:display_name/inbox", :to => redirect(:path => "/messages/inbox")
   get "/user/:display_name/outbox", :to => redirect(:path => "/messages/outbox")

test/controllers/messages/inboxes_controller_test.rb

@@ -0,0 +1,36 @@
+require "test_helper"
+
+module Messages
+  class InboxesControllerTest < ActionDispatch::IntegrationTest
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/messages/inbox", :method => :get },
+        { :controller => "messages/inboxes", :action => "show" }
+      )
+    end
+
+    def test_show
+      user = create(:user)
+      read_message = create(:message, :read, :recipient => user)
+
+      session_for(user)
+
+      get messages_inbox_path
+      assert_response :success
+      assert_select ".content-inner > table.messages-table > tbody", :count => 1 do
+        assert_select "tr", :count => 1
+        assert_select "tr#inbox-#{read_message.id}", :count => 1 do
+          assert_select "a[href='#{user_path read_message.sender}']", :text => read_message.sender.display_name
+          assert_select "a[href='#{message_path read_message}']", :text => read_message.title
+        end
+      end
+    end
+
+    def test_show_requires_login
+      get messages_inbox_path
+      assert_redirected_to login_path(:referer => messages_inbox_path)
+    end
+  end
+end

test/controllers/messages/muted_inboxes_controller_test.rb

@@ -0,0 +1,14 @@
+require "test_helper"
+
+module Messages
+  class MutedInboxesControllerTest < ActionDispatch::IntegrationTest
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/messages/muted", :method => :get },
+        { :controller => "messages/muted_inboxes", :action => "show" }
+      )
+    end
+  end
+end

test/controllers/messages/outboxes_controller_test.rb

@@ -0,0 +1,36 @@
+require "test_helper"
+
+module Messages
+  class OutboxesControllerTest < ActionDispatch::IntegrationTest
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/messages/outbox", :method => :get },
+        { :controller => "messages/outboxes", :action => "show" }
+      )
+    end
+
+    def test_show
+      user = create(:user)
+      message = create(:message, :sender => user)
+
+      session_for(user)
+
+      get messages_outbox_path
+      assert_response :success
+      assert_select ".content-inner > table.messages-table > tbody", :count => 1 do
+        assert_select "tr", :count => 1
+        assert_select "tr#outbox-#{message.id}", :count => 1 do
+          assert_select "a[href='#{user_path message.recipient}']", :text => message.recipient.display_name
+          assert_select "a[href='#{message_path message}']", :text => message.title
+        end
+      end
+    end
+
+    def test_show_requires_login
+      get messages_outbox_path
+      assert_redirected_to login_path(:referer => messages_outbox_path)
+    end
+  end
+end

test/controllers/messages_controller_test.rb

@@ -4,14 +4,6 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
   ##
   # test all routes which lead to this controller
   def test_routes
-    assert_routing(
-      { :path => "/messages/inbox", :method => :get },
-      { :controller => "messages", :action => "inbox" }
-    )
-    assert_routing(
-      { :path => "/messages/outbox", :method => :get },
-      { :controller => "messages", :action => "outbox" }
-    )
     assert_routing(
       { :path => "/message/new/username", :method => :get },
       { :controller => "messages", :action => "new", :display_name => "username" }
@@ -179,7 +171,7 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
         end
       end
     end
-    assert_redirected_to inbox_messages_path
+    assert_redirected_to messages_inbox_path
     assert_equal "Message sent", flash[:notice]
     e = ActionMailer::Base.deliveries.first
     assert_equal [recipient_user.email], e.to
@@ -332,57 +324,6 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
     assert_template "no_such_message"
   end
 
-  ##
-  # test the inbox action
-  def test_inbox
-    user = create(:user)
-    read_message = create(:message, :read, :recipient => user)
-    # Check that the inbox page requires us to login
-    get inbox_messages_path
-    assert_redirected_to login_path(:referer => inbox_messages_path)
-
-    # Login
-    session_for(user)
-
-    # Check that we can view our inbox when logged in
-    get inbox_messages_path
-    assert_response :success
-    assert_template "inbox"
-    assert_select ".content-inner > table.messages-table > tbody", :count => 1 do
-      assert_select "tr", :count => 1
-      assert_select "tr#inbox-#{read_message.id}", :count => 1 do
-        assert_select "a[href='#{user_path read_message.sender}']", :text => read_message.sender.display_name
-        assert_select "a[href='#{message_path read_message}']", :text => read_message.title
-      end
-    end
-  end
-
-  ##
-  # test the outbox action
-  def test_outbox
-    user = create(:user)
-    message = create(:message, :sender => user)
-
-    # Check that the outbox page requires us to login
-    get outbox_messages_path
-    assert_redirected_to login_path(:referer => outbox_messages_path)
-
-    # Login
-    session_for(user)
-
-    # Check that we can view our outbox when logged in
-    get outbox_messages_path
-    assert_response :success
-    assert_template "outbox"
-    assert_select ".content-inner > table.messages-table > tbody", :count => 1 do
-      assert_select "tr", :count => 1
-      assert_select "tr#outbox-#{message.id}", :count => 1 do
-        assert_select "a[href='#{user_path message.recipient}']", :text => message.recipient.display_name
-        assert_select "a[href='#{message_path message}']", :text => message.title
-      end
-    end
-  end
-
   ##
   # test the mark action
   def test_mark
@@ -416,22 +357,22 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
 
     # Check that the marking a message read works
     post message_mark_path(message, :mark => "read")
-    assert_redirected_to inbox_messages_path
+    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 inbox_messages_path
+    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 inbox_messages_path
+    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 inbox_messages_path
+    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
@@ -452,12 +393,12 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
 
     # Check that the marking a message read works
     post message_mark_path(message, :mark => "read")
-    assert_redirected_to muted_messages_path
+    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 muted_messages_path
+    assert_redirected_to messages_muted_inbox_path
     assert_not Message.find(message.id).message_read
   end
 
@@ -487,15 +428,15 @@ class MessagesControllerTest < ActionDispatch::IntegrationTest
 
     # Check that the destroy a received message works
     delete message_path(read_message)
-    assert_redirected_to inbox_messages_path
+    assert_redirected_to messages_inbox_path
     assert_equal "Message deleted", flash[:notice]
     m = Message.find(read_message.id)
     assert m.from_user_visible
     assert_not m.to_user_visible
 
     # Check that the destroying a sent message works
-    delete message_path(sent_message, :referer => outbox_messages_path)
-    assert_redirected_to outbox_messages_path
+    delete message_path(sent_message, :referer => messages_outbox_path)
+    assert_redirected_to messages_outbox_path
     assert_equal "Message deleted", flash[:notice]
     m = Message.find(sent_message.id)
     assert_not m.from_user_visible

test/system/messages_test.rb

@@ -6,7 +6,7 @@ class MessagesTest < ApplicationSystemTestCase
     create(:message, :recipient => user)
     sign_in_as(user)
 
-    visit inbox_messages_path
+    visit messages_inbox_path
     assert_text "You have 1 new message and 0 old messages"
 
     click_on "Delete"
@@ -18,7 +18,7 @@ class MessagesTest < ApplicationSystemTestCase
     create(:message, :sender => user)
     sign_in_as(user)
 
-    visit outbox_messages_path
+    visit messages_outbox_path
     assert_text "You have 1 sent message"
 
     click_on "Delete"
@@ -32,7 +32,7 @@ class MessagesTest < ApplicationSystemTestCase
     create(:message, :sender => muted_user, :recipient => user)
     sign_in_as(user)
 
-    visit muted_messages_path
+    visit messages_muted_inbox_path
     assert_text "1 muted message"
 
     click_on "Delete"