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

[Tom Hughes]

@tomhughes requested changes on this pull request.



> +# 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]
+
+    authorize_resource
+
+    around_action :api_call_handle_error, :api_call_timeout
+
+    before_action :set_request_formats
+
+    skip_authorization_check :only => [:inbox, :outbox]

Why are we skipping the authorization check for these methods?

> @@ -9,8 +9,7 @@ class MessagesController < ApplicationController
   authorize_resource
 
   before_action :lookup_user, :only => [:new, :create]
-  before_action :check_database_readable
-  before_action :check_database_writable, :only => [:new, :create, :reply, :mark, :destroy]
+  before_action :check_api_writable, :only => [:new, :create, :reply, :mark, :destroy]

Why are we changing the non-API access to check the API status instead of the database status? and why are we getting rid of the readable check for methods like `inbox` and `show`?

> +      @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.find(params[:id])

It doesn't matter as much here as it does for `inbox` and `outbox` but we might as well add the `includes(:sender, :recipient)` here as well.

> +      jsm = js["message"]
+      assert_not_nil jsm
+      assert_equal msg.id, jsm["id"]
+      assert_equal sender.id, jsm["from_user_id"]
+      assert_equal sender.display_name, jsm["from_display_name"]
+      assert_equal recipient.id, jsm["to_user_id"]
+      assert_equal recipient.display_name, jsm["to_display_name"]
+      assert_equal msg.title, jsm["title"]
+      assert_not_nil jsm["sent_on"]
+      assert_equal !msg.from_user_visible, jsm["deleted"]
+      assert_not jsm.key?("message_read")
+      assert_equal "markdown", jsm["body_format"]
+      assert_equal msg.body, jsm["body"]
+    end
+
+    def test_read_status

It would probably be better to call this `test_update_status` as the current name can be read two ways and I initially read it as testing that the status can be read rather than that the read status can be changed.

> +      user3_token = create(:oauth_access_token,
+                           :resource_owner_id => user3.id,
+                           :scopes => %w[send_messages consume_messages])
+      user3_auth = bearer_authorization_header(user3_token.token)
+
+      # 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 auithorized users

Spelling error...

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

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