Merge remote-tracking branch 'upstream/pull/5434'

.rubocop_todo.yml

@@ -20,7 +20,8 @@ require:
 FactoryBot/ExcessiveCreateList:
   Exclude:
     - 'test/controllers/api/changeset_comments_controller_test.rb'
-    - 'test/controllers/api/messages_controller_test.rb'
+    - 'test/controllers/api/messages/inboxes_controller_test.rb'
+    - 'test/controllers/api/messages/outboxes_controller_test.rb'
     - 'test/controllers/changesets_controller_test.rb'
     - 'test/controllers/diary_entries_controller_test.rb'
     - 'test/controllers/notes_controller_test.rb'

app/abilities/api_ability.rb

@@ -34,7 +34,7 @@ def initialize(token)
         can :read, UserPreference if scope?(token, :read_prefs)
         can [:update, :update_all, :destroy], UserPreference if scope?(token, :write_prefs)
 
-        can [:inbox, :outbox, :read, :update, :destroy], Message if scope?(token, :consume_messages)
+        can [:read, :update, :destroy], Message if scope?(token, :consume_messages)
         can :create, Message if scope?(token, :send_messages)
 
         if user.terms_agreed?

app/controllers/api/messages/inboxes_controller.rb

@@ -0,0 +1,12 @@
+module Api
+  module Messages
+    class InboxesController < MailboxesController
+      def show
+        @skip_body = true
+        @messages = Message.includes(:sender, :recipient).where(:to_user_id => current_user.id)
+
+        show_messages
+      end
+    end
+  end
+end

app/controllers/api/messages/mailboxes_controller.rb

@@ -0,0 +1,43 @@
+module Api
+  module Messages
+    class MailboxesController < ApiController
+      before_action :authorize
+
+      authorize_resource :message
+
+      before_action :set_request_formats
+
+      private
+
+      def show_messages
+        @messages = @messages.where(:muted => false)
+        if params[:order].nil? || params[:order] == "newest"
+          @messages = @messages.where(:id => ..params[:from_id]) unless params[:from_id].nil?
+          @messages = @messages.order(:id => :desc)
+        elsif params[:order] == "oldest"
+          @messages = @messages.where(:id => params[:from_id]..) unless params[:from_id].nil?
+          @messages = @messages.order(:id => :asc)
+        else
+          raise OSM::APIBadUserInput, "Invalid order specified"
+        end
+
+        limit = params[:limit]
+        if !limit
+          limit = Settings.default_message_query_limit
+        elsif !limit.to_i.positive? || limit.to_i > Settings.max_message_query_limit
+          raise OSM::APIBadUserInput, "Messages limit must be between 1 and #{Settings.max_message_query_limit}"
+        else
+          limit = limit.to_i
+        end
+
+        @messages = @messages.limit(limit)
+
+        # Render the result
+        respond_to do |format|
+          format.xml
+          format.json
+        end
+      end
+    end
+  end
+end

app/controllers/api/messages/outboxes_controller.rb

@@ -0,0 +1,12 @@
+module Api
+  module Messages
+    class OutboxesController < MailboxesController
+      def show
+        @skip_body = true
+        @messages = Message.includes(:sender, :recipient).where(:from_user_id => current_user.id)
+
+        show_messages
+      end
+    end
+  end
+end

app/controllers/api/messages_controller.rb

@@ -11,20 +11,6 @@ class MessagesController < ApiController
 
     before_action :set_request_formats
 
-    def inbox
-      @skip_body = true
-      @messages = Message.includes(:sender, :recipient).where(:to_user_id => current_user.id)
-
-      show_messages
-    end
-
-    def outbox
-      @skip_body = true
-      @messages = Message.includes(:sender, :recipient).where(:from_user_id => current_user.id)
-
-      show_messages
-    end
-
     # Dump the details on a message given in params[:id]
     def show
       @message = Message.includes(:sender, :recipient).find(params[:id])
@@ -111,37 +97,5 @@ def destroy
         format.json { render :action => :show }
       end
     end
-
-    private
-
-    def show_messages
-      @messages = @messages.where(:muted => false)
-      if params[:order].nil? || params[:order] == "newest"
-        @messages = @messages.where(:id => ..params[:from_id]) unless params[:from_id].nil?
-        @messages = @messages.order(:id => :desc)
-      elsif params[:order] == "oldest"
-        @messages = @messages.where(:id => params[:from_id]..) unless params[:from_id].nil?
-        @messages = @messages.order(:id => :asc)
-      else
-        raise OSM::APIBadUserInput, "Invalid order specified"
-      end
-
-      limit = params[:limit]
-      if !limit
-        limit = Settings.default_message_query_limit
-      elsif !limit.to_i.positive? || limit.to_i > Settings.max_message_query_limit
-        raise OSM::APIBadUserInput, "Messages limit must be between 1 and #{Settings.max_message_query_limit}"
-      else
-        limit = limit.to_i
-      end
-
-      @messages = @messages.limit(limit)
-
-      # Render the result
-      respond_to do |format|
-        format.xml
-        format.json
-      end
-    end
   end
 end

app/views/api/messages/mailboxes/show.json.jbuilder

@@ -0,0 +1,5 @@
+json.partial! "api/root_attributes"
+
+json.messages do
+  json.array! @messages, :partial => "api/messages/message", :as => :message
+end

app/views/api/messages/mailboxes/show.xml.builder

@@ -0,0 +1,5 @@
+xml.instruct!
+
+xml.osm(OSM::API.new.xml_root_attributes) do |osm|
+  osm << (render(:partial => "api/messages/message", :collection => @messages) || "")
+end

config/routes.rb

@@ -80,13 +80,12 @@
       end
     end
 
-    resources :messages, :path => "user/messages", :constraints => { :id => /\d+/ }, :only => [:create, :show, :update, :destroy] do
-      collection do
-        get "inbox"
-        get "outbox"
-      end
+    resources :messages, :path => "user/messages", :constraints => { :id => /\d+/ }, :only => [:create, :show, :update, :destroy]
+    namespace :messages, :path => "user/messages" do
+      resource :inbox, :only => :show
+      resource :outbox, :only => :show
     end
-    post "/user/messages/:id" => "messages#update"
+    post "/user/messages/:id" => "messages#update", :as => nil
 
     resources :traces, :path => "gpx", :only => [:create, :show, :update, :destroy], :id => /\d+/ do
       scope :module => :traces do

test/controllers/api/messages/inboxes_controller_test.rb

@@ -0,0 +1,170 @@
+require "test_helper"
+
+module Api
+  module Messages
+    class InboxesControllerTest < ActionDispatch::IntegrationTest
+      ##
+      # test all routes which lead to this controller
+      def test_routes
+        assert_routing(
+          { :path => "/api/0.6/user/messages/inbox", :method => :get },
+          { :controller => "api/messages/inboxes", :action => "show" }
+        )
+        assert_routing(
+          { :path => "/api/0.6/user/messages/inbox.xml", :method => :get },
+          { :controller => "api/messages/inboxes", :action => "show", :format => "xml" }
+        )
+        assert_routing(
+          { :path => "/api/0.6/user/messages/inbox.json", :method => :get },
+          { :controller => "api/messages/inboxes", :action => "show", :format => "json" }
+        )
+      end
+
+      def test_show
+        user1 = create(:user)
+        user1_auth = bearer_authorization_header(user1, :scopes => %w[send_messages consume_messages])
+
+        user2 = create(:user)
+        user2_auth = bearer_authorization_header(user2, :scopes => %w[send_messages consume_messages])
+
+        user3 = create(:user)
+        user3_auth = bearer_authorization_header(user3, :scopes => %w[send_messages consume_messages])
+
+        # create some messages between users
+        # user | inbox | outbox
+        #   1  |   0   |   3
+        #   2  |   2   |   1
+        #   3  |   2   |   0
+        create(:message, :unread, :sender => user1, :recipient => user2)
+        create(:message, :unread, :sender => user1, :recipient => user2)
+        create(:message, :unread, :sender => user1, :recipient => user3)
+        create(:message, :unread, :sender => user2, :recipient => user3)
+
+        # only authorized users
+        get api_messages_inbox_path
+        assert_response :unauthorized
+
+        # no messages in user1.inbox
+        get api_messages_inbox_path, :headers => user1_auth
+        assert_response :success
+        assert_equal "application/xml", response.media_type
+        assert_select "message", :count => 0
+
+        # 2 messages in user2.inbox
+        get api_messages_inbox_path, :headers => user2_auth
+        assert_response :success
+        assert_equal "application/xml", response.media_type
+        assert_select "message", :count => 2 do
+          assert_select "[from_user_id]"
+          assert_select "[from_display_name]"
+          assert_select "[to_user_id='#{user2.id}']"
+          assert_select "[to_display_name='#{user2.display_name}']"
+          assert_select "[sent_on]"
+          assert_select "[message_read='false']"
+          assert_select "[deleted='false']"
+          assert_select "[body_format]"
+          assert_select "body", false
+          assert_select "title"
+        end
+
+        # 2 messages in user3.inbox
+        get api_messages_inbox_path, :headers => user3_auth
+        assert_response :success
+        assert_equal "application/xml", response.media_type
+        assert_select "message", :count => 2 do
+          assert_select "[from_user_id]"
+          assert_select "[from_display_name]"
+          assert_select "[to_user_id='#{user3.id}']"
+          assert_select "[to_display_name='#{user3.display_name}']"
+          assert_select "[sent_on]"
+          assert_select "[message_read='false']"
+          assert_select "[deleted='false']"
+          assert_select "[body_format]"
+          assert_select "body", false
+          assert_select "title"
+        end
+      end
+
+      def test_show_paged_asc
+        recipient = create(:user)
+        recipient_auth = bearer_authorization_header(recipient, :scopes => %w[consume_messages])
+
+        sender = create(:user)
+
+        create_list(:message, 100, :unread, :sender => sender, :recipient => recipient)
+
+        msgs_read = {}
+        params = { :order => "oldest", :limit => 20 }
+        10.times do
+          get api_messages_inbox_path(:format => "json"),
+              :params => params,
+              :headers => recipient_auth
+          assert_response :success
+          assert_equal "application/json", response.media_type
+          js = ActiveSupport::JSON.decode(@response.body)
+          jsm = js["messages"]
+          assert_operator jsm.count, :<=, 20
+
+          break if jsm.nil? || jsm.count.zero?
+
+          assert_operator(jsm[0]["id"], :>=, params[:from_id]) unless params[:from_id].nil?
+          # ensure ascending order
+          (0..jsm.count - 1).each do |i|
+            assert_operator(jsm[i]["id"], :<, jsm[i + 1]["id"]) unless i == jsm.count - 1
+            msgs_read[jsm[i]["id"]] = jsm[i]
+          end
+          params[:from_id] = jsm[jsm.count - 1]["id"]
+        end
+        assert_equal 100, msgs_read.count
+      end
+
+      def test_show_paged_desc
+        recipient = create(:user)
+        recipient_auth = bearer_authorization_header(recipient, :scopes => %w[consume_messages])
+
+        sender = create(:user)
+
+        create_list(:message, 100, :unread, :sender => sender, :recipient => recipient)
+
+        real_max_id = -1
+        msgs_read = {}
+        params = { :order => "newest", :limit => 20 }
+        10.times do
+          get api_messages_inbox_path(:format => "json"),
+              :params => params,
+              :headers => recipient_auth
+          assert_response :success
+          assert_equal "application/json", response.media_type
+          js = ActiveSupport::JSON.decode(@response.body)
+          jsm = js["messages"]
+          assert_operator jsm.count, :<=, 20
+
+          break if jsm.nil? || jsm.count.zero?
+
+          if params[:from_id].nil?
+            real_max_id = jsm[0]["id"]
+          else
+            assert_operator jsm[0]["id"], :<=, params[:from_id]
+          end
+          # ensure descending order
+          (0..jsm.count - 1).each do |i|
+            assert_operator(jsm[i]["id"], :>, jsm[i + 1]["id"]) unless i == jsm.count - 1
+            msgs_read[jsm[i]["id"]] = jsm[i]
+          end
+          params[:from_id] = jsm[jsm.count - 1]["id"]
+        end
+        assert_equal 100, msgs_read.count
+        assert_not_equal(-1, real_max_id)
+
+        # invoke without min_id/max_id parameters, verify that we get the last batch
+        get api_messages_inbox_path(:format => "json"), :params => { :limit => 20 }, :headers => recipient_auth
+        assert_response :success
+        assert_equal "application/json", response.media_type
+        js = ActiveSupport::JSON.decode(@response.body)
+        jsm = js["messages"]
+        assert_not_nil jsm
+        assert_equal real_max_id, jsm[0]["id"]
+      end
+    end
+  end
+end