[openstreetmap/openstreetmap-website] Add Messages API (PR #4605)

[Tom Hughes]

@tomhughes requested changes on this pull request.

I've had a first pass over the code and an initial peek at the tests and added my comments. I'll definitely want @gravitystorm to have a look as well before we merge.

> @@ -0,0 +1,154 @@
+# The MessagesController is the RESTful interface to Message objects
+
+module Api
+  class MessagesController < ApiController
+    before_action :authorize
+
+    before_action :check_api_writable, :only => [:create, :update, :destroy]
+    before_action :check_api_readable, :except => [:create, :update, :destroy]

This should be `inbox`, `outbox` and `show` surely?

> +    around_action :api_call_handle_error, :api_call_timeout
+
+    before_action :set_request_formats
+
+    skip_authorization_check :only => [:inbox, :outbox]
+
+    def inbox
+      @skip_body = true
+      @messages = Message.where(:to_user_id => current_user.id)
+
+      show_messages
+    end
+
+    def outbox
+      @skip_body = true
+      @messages = Message.where :from_user_id => current_user.id

Can we please add parentheses for consistency with the `inbox` action.

> +      raise OSM::APIBadUserInput, "No body was given" if params[:body].blank?
+      raise OSM::APIBadUserInput, "No body format was given" if params[:body_format].blank?
+
+      # Extract the arguments
+      if params[:recipient_id]
+        recipient_id = params[:recipient_id].to_i
+        recipient = User.find(recipient_id)
+      elsif params[:recipient]
+        recipient_display_name = params[:recipient]
+        recipient = User.find_by(:display_name => recipient_display_name)
+      else
+        raise OSM::APIBadUserInput, "No recipient was given"
+      end
+      title = params[:title]
+      body = params[:body]
+      body_format = params[:body_format]

Is there any particular point to copying these into local variables which are only going to be used once?

> +
+      raise OSM::APIAccessDenied if current_user.id != @message.from_user_id && current_user.id != @message.to_user_id
+
+      # Render the result
+      respond_to do |format|
+        format.xml
+        format.json
+      end
+    end
+
+    # Create a new message from current user
+    def create
+      # Check the arguments are sane
+      raise OSM::APIBadUserInput, "No title was given" if params[:title].blank?
+      raise OSM::APIBadUserInput, "No body was given" if params[:body].blank?
+      raise OSM::APIBadUserInput, "No body format was given" if params[:body_format].blank?

Do we want to allow a choice of body format? There's no choice with the existing web interface and the field mostly exists to support legacy messages from before we switched to markdown and all new messages are markdown.

> +      else
+        raise OSM::APIAccessDenied
+      end
+
+      @message.save!
+
+      # Return a copy of the message
+      respond_to do |format|
+        format.xml { render :action => :show }
+        format.json { render :action => :show }
+      end
+    end
+
+    private
+
+    def show_messages

This should probably exclude deleted messages and maybe muted ones as well?

> @@ -0,0 +1,10 @@
+json.id message.id
+json.from_user_id message.from_user_id
+json.to_user_id message.to_user_id
+json.title message.title
+json.sent_on message.sent_on.xmlschema
+json.message_read message.message_read
+json.from_user_visible message.from_user_visible
+json.to_user_visible message.to_user_visible

This means user can see whether the other person has deleted the message which isn't something that can see at the moment - do we want that?

> @@ -0,0 +1,10 @@
+json.id message.id
+json.from_user_id message.from_user_id
+json.to_user_id message.to_user_id
+json.title message.title
+json.sent_on message.sent_on.xmlschema
+json.message_read message.message_read

This allows a sender to see if the recipient has read a message which is not possible at the moment - do we want that?

> @@ -0,0 +1,10 @@
+json.id message.id
+json.from_user_id message.from_user_id
+json.to_user_id message.to_user_id

Do we want to include the user names as well?

> @@ -0,0 +1,11 @@
+xml.tag! "message", :id => message.id,
+                    :from_user_id => message.from_user_id,
+                    :to_user_id => message.to_user_id,
+                    :sent_on => message.sent_on.xmlschema,
+                    :message_read => message.message_read,
+                    :from_user_visible => message.from_user_visible,
+                    :to_user_visible => message.to_user_visible,
+                    :body_format => message.body_format do

All comments about the JSON version apply here as well.

> @@ -0,0 +1,7 @@
+json.partial! "api/root_attributes"
+
+json.messages(@messages) do |message|
+  xml.tag! "messages" do

This should be in the XML version surely?

> @@ -78,6 +78,15 @@
       end
     end
 
+    resources :messages, :path => "user/messages", :constraints => { :id => /\d+/ }, :only => [:create, :show, :destroy], :controller => "messages", :as => :api_messages do
+      collection do
+        get "inbox"
+        get "outbox"
+      end
+    end
+
+    post "/user/messages/:id" => "messages#update", :as => :api_message_update

Why can't this be done by allowing `:update` on the resource block above?

> @@ -78,6 +78,15 @@
       end
     end
 
+    resources :messages, :path => "user/messages", :constraints => { :id => /\d+/ }, :only => [:create, :show, :destroy], :controller => "messages", :as => :api_messages do

Do we want to nest messages under user? The IDs are global so there's no need for it unless we think there's some reason it makes more sense that way.

> +require "test_helper"
+
+module Api
+  class MessagesControllerTest < ActionDispatch::IntegrationTest
+    def setup
+      super
+      # Stub nominatim response for note locations
+      stub_request(:get, %r{^https://nominatim\.openstreetmap\.org/reverse\?})
+        .to_return(:status => 404)
+    end
+
+    ##
+    # test all routes which lead to this controller
+    def test_routes
+      assert_routing(
+        { :path => "/api/0.6/user/messages/inbox", :method => :get },

This (and outbox) should probably have XML and JSON versions as for show?

> @@ -0,0 +1,570 @@
+require "test_helper"
+
+module Api
+  class MessagesControllerTest < ActionDispatch::IntegrationTest
+    def setup

Has this been copied from notes and not removed?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4605#pullrequestreview-2080911144
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4605/review/2080911144 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240527/b7cbb6fd/attachment-0001.htm>