<p></p>
<p><b>@tomhughes</b> requested changes on this pull request.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1694223906">app/controllers/api/messages_controller.rb</a>:</p>
<pre style='color:#555'>> +# 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]
</pre>
<p dir="auto">Why are we skipping the authorization check for these methods?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1694224579">app/controllers/messages_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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]
</pre>
<p dir="auto">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 <code class="notranslate">inbox</code> and <code class="notranslate">show</code>?</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1694224849">app/controllers/api/messages_controller.rb</a>:</p>
<pre style='color:#555'>> +      @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])
</pre>
<p dir="auto">It doesn't matter as much here as it does for <code class="notranslate">inbox</code> and <code class="notranslate">outbox</code> but we might as well add the <code class="notranslate">includes(:sender, :recipient)</code> here as well.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1694225617">test/controllers/api/messages_controller_test.rb</a>:</p>
<pre style='color:#555'>> +      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
</pre>
<p dir="auto">It would probably be better to call this <code class="notranslate">test_update_status</code> 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.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1694225917">test/controllers/api/messages_controller_test.rb</a>:</p>
<pre style='color:#555'>> +      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
</pre>
<p dir="auto">Spelling error...</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#pullrequestreview-2203461824">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLNWUKVRSK776LDXGB3ZOTIZDAVCNFSM6AAAAABFBAKSFGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMBTGQ3DCOBSGQ">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLKAUIRVXFTNWA2XLF3ZOTIZDA5CNFSM6AAAAABFBAKSFGWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUDKYUMA.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/4605/review/2203461824</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/4605#pullrequestreview-2203461824",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4605#pullrequestreview-2203461824",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>