<p></p>
<p><b>@tomhughes</b> requested changes on this pull request.</p>
<p dir="auto">I've had a first pass over the code and an initial peek at the tests and added my comments. I'll definitely want <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/gravitystorm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://github.com/gravitystorm">@gravitystorm</a> to have a look as well before we merge.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616044072">app/controllers/api/messages_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -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]
</pre>
<p dir="auto">This should be <code class="notranslate">inbox</code>, <code class="notranslate">outbox</code> and <code class="notranslate">show</code> surely?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616046477">app/controllers/api/messages_controller.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p dir="auto">Can we please add parentheses for consistency with the <code class="notranslate">inbox</code> action.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616049391">app/controllers/api/messages_controller.rb</a>:</p>
<pre style='color:#555'>> + 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]
</pre>
<p dir="auto">Is there any particular point to copying these into local variables which are only going to be used once?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616052895">app/controllers/api/messages_controller.rb</a>:</p>
<pre style='color:#555'>> +
+ 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?
</pre>
<p dir="auto">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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616061310">app/controllers/api/messages_controller.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p dir="auto">This should probably exclude deleted messages and maybe muted ones as well?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616062910">app/views/api/messages/_message.json.jbuilder</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">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?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616063751">app/views/api/messages/_message.json.jbuilder</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">This allows a sender to see if the recipient has read a message which is not possible at the moment - do we want that?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616064032">app/views/api/messages/_message.json.jbuilder</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,10 @@
+json.id message.id
+json.from_user_id message.from_user_id
+json.to_user_id message.to_user_id
</pre>
<p dir="auto">Do we want to include the user names as well?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616064589">app/views/api/messages/_message.xml.builder</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">All comments about the JSON version apply here as well.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616065404">app/views/api/messages/outbox.json.jbuilder</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,7 @@
+json.partial! "api/root_attributes"
+
+json.messages(@messages) do |message|
+ xml.tag! "messages" do
</pre>
<p dir="auto">This should be in the XML version surely?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616066881">config/routes.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">Why can't this be done by allowing <code class="notranslate">:update</code> on the resource block above?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616067863">config/routes.rb</a>:</p>
<pre style='color:#555'>> @@ -78,6 +78,15 @@
end
end
+ resources :messages, :path => "user/messages", :constraints => { :id => /\d+/ }, :only => [:create, :show, :destroy], :controller => "messages", :as => :api_messages do
</pre>
<p dir="auto">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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616070323">test/controllers/api/messages_controller_test.rb</a>:</p>
<pre style='color:#555'>> +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 },
</pre>
<p dir="auto">This (and outbox) should probably have XML and JSON versions as for show?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4605#discussion_r1616071117">test/controllers/api/messages_controller_test.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,570 @@
+require "test_helper"
+
+module Api
+ class MessagesControllerTest < ActionDispatch::IntegrationTest
+ def setup
</pre>
<p dir="auto">Has this been copied from notes and not removed?</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-2080911144">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLK4EDRYN2SDNDSO4MDZEMZ6BAVCNFSM6AAAAABFBAKSFGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBQHEYTCMJUGQ">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLNSB6OKLVKVQFQCABLZEMZ6BA5CNFSM6AAAAABFBAKSFGWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTT4BAXSQ.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/2080911144</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-2080911144",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4605#pullrequestreview-2080911144",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>