From 80c9e7fbfe386fc8b4941e2475e9ea4e8e87f251 Mon Sep 17 00:00:00 2001 From: "Stanko K.R." Date: Tue, 11 Aug 2026 13:15:34 +0200 Subject: [PATCH] Adjust to the in-house style - Use jbuilder instead of hashes - Use resource instead of direct HTTP verbs Verbs only make sense if you have one or two routes, if there are multiple that emulate what resource does then it's better to use resource. - Paginate using link headers - Cache responses --- app/controllers/concerns/raw_request_body.rb | 12 ++ .../messages/boosts/by_bots_controller.rb | 36 +++--- .../messages/by_bots_controller.rb | 80 +++++-------- app/views/messages/_message.json.jbuilder | 18 +++ .../messages/boosts/_boost.json.jbuilder | 12 ++ .../boosts/by_bots/show.json.jbuilder | 1 + .../messages/by_bots/index.json.jbuilder | 1 + app/views/users/_user.json.jbuilder | 5 + config/routes.rb | 11 +- .../boosts/by_bots_controller_test.rb | 56 +++++---- .../messages/by_bots_controller_test.rb | 113 +++++++++++------- 11 files changed, 205 insertions(+), 140 deletions(-) create mode 100644 app/controllers/concerns/raw_request_body.rb create mode 100644 app/views/messages/_message.json.jbuilder create mode 100644 app/views/messages/boosts/_boost.json.jbuilder create mode 100644 app/views/messages/boosts/by_bots/show.json.jbuilder create mode 100644 app/views/messages/by_bots/index.json.jbuilder create mode 100644 app/views/users/_user.json.jbuilder diff --git a/app/controllers/concerns/raw_request_body.rb b/app/controllers/concerns/raw_request_body.rb new file mode 100644 index 0000000..0a35b8b --- /dev/null +++ b/app/controllers/concerns/raw_request_body.rb @@ -0,0 +1,12 @@ +module RawRequestBody + extend ActiveSupport::Concern + + private + + def raw_request_body + request.body.rewind + request.body.read.force_encoding("UTF-8") + ensure + request.body.rewind + end +end diff --git a/app/controllers/messages/boosts/by_bots_controller.rb b/app/controllers/messages/boosts/by_bots_controller.rb index 85f93ae..10ee80a 100644 --- a/app/controllers/messages/boosts/by_bots_controller.rb +++ b/app/controllers/messages/boosts/by_bots_controller.rb @@ -1,33 +1,33 @@ -class Messages::Boosts::ByBotsController < ApplicationController +class Messages::Boosts::ByBotsController < Messages::BoostsController + include RawRequestBody + allow_bot_access only: :create + before_action :ensure_content_present + def create - set_message - @boost = @message.boosts.create!(content: read_body) + @boost = @message.boosts.create!(boost_params) broadcast_create - head :created - rescue ActiveRecord::RecordNotFound - head :not_found + render :show, status: :created end private def set_message - @room = Current.user.rooms.find(params[:room_id]) - @message = @room.messages.find(params[:message_id]) + if room = Current.user.rooms.find_by(id: params[:room_id]) + @message = room.messages.find_by(id: params[:message_id]) + end + + head :not_found unless @message end - def read_body - request.body.rewind - request.body.read.force_encoding("UTF-8") - ensure - request.body.rewind + def ensure_content_present + if raw_request_body.blank? + head :unprocessable_content + end end - def broadcast_create - @boost.broadcast_append_to @boost.message.room, :messages, - target: "boosts_message_#{@boost.message.client_message_id}", - partial: "messages/boosts/boost", - attributes: { maintain_scroll: true } + def boost_params + { content: raw_request_body } end end diff --git a/app/controllers/messages/by_bots_controller.rb b/app/controllers/messages/by_bots_controller.rb index ff398ca..f225e51 100644 --- a/app/controllers/messages/by_bots_controller.rb +++ b/app/controllers/messages/by_bots_controller.rb @@ -1,73 +1,57 @@ class Messages::ByBotsController < MessagesController + include RawRequestBody + allow_bot_access only: %i[ index create ] + before_action :set_room + before_action :ensure_body_or_attachment_present, only: :create + def index - set_room @messages = find_paged_messages - render json: messages_as_json(@messages) - rescue ActiveRecord::RecordNotFound - head :not_found + set_pagination_headers end def create - set_room - @message = @room.messages.create_with_attachment!(message_params) - @message.broadcast_create - deliver_webhooks_to_bots + super head :created, location: message_url(@message) - rescue ActiveRecord::RecordNotFound - head :not_found end private - def messages_as_json(messages) - { - room: { - id: @room.id, - name: @room.name - }, - messages: messages.map { |m| message_as_json(m) }, - pagination: pagination_info(messages) - } + def set_room + @room = Current.user.rooms.find_by(id: params[:room_id]) + + head :not_found unless @room end - def message_as_json(message) - { - id: message.id, - body: { - plain: message.plain_text_body, - html: message.body&.body&.to_s - }, - created_at: message.created_at.iso8601, - creator: { - id: message.creator.id, - name: message.creator.name, - is_bot: message.creator.bot? - } - } + def ensure_body_or_attachment_present + if params[:attachment].blank? && raw_request_body.blank? + head :unprocessable_content + end end - def pagination_info(messages) - return {} if messages.empty? - { - oldest_id: messages.last.id, - newest_id: messages.first.id, - has_more: messages.size == Message::PAGE_SIZE - } + def set_pagination_headers + headers["X-Total-Count"] = @room.messages.count.to_s + + if next_page = next_page_params + headers["Link"] = %(<#{room_bot_messages_url(@room, params[:bot_key], **next_page)}>; rel="next") + end + end + + def next_page_params + if @messages.any? + if params[:after].present? + { after: @messages.last.id } if @room.messages.after(@messages.last).exists? + else + { before: @messages.first.id } if @room.messages.before(@messages.first).exists? + end + end end def message_params if params[:attachment] params.permit(:attachment) else - reading(request.body) { |body| { body: body } } + { body: raw_request_body } end end - - def reading(io) - io.rewind - yield io.read.force_encoding("UTF-8") - ensure - io.rewind - end end diff --git a/app/views/messages/_message.json.jbuilder b/app/views/messages/_message.json.jbuilder new file mode 100644 index 0000000..e8abfa6 --- /dev/null +++ b/app/views/messages/_message.json.jbuilder @@ -0,0 +1,18 @@ +json.cache! message do + json.(message, :id) + + json.created_at message.created_at.utc + + json.body do + json.plain_text message.plain_text_body + json.html message.body.to_s + end + + json.creator message.creator, partial: "users/user", as: :user + + json.room do + json.id message.room_id + end + + json.url room_message_url(message.room, message) +end diff --git a/app/views/messages/boosts/_boost.json.jbuilder b/app/views/messages/boosts/_boost.json.jbuilder new file mode 100644 index 0000000..c3392b6 --- /dev/null +++ b/app/views/messages/boosts/_boost.json.jbuilder @@ -0,0 +1,12 @@ +json.cache! boost do + json.(boost, :id, :content) + + json.created_at boost.created_at.utc + + json.booster boost.booster, partial: "users/user", as: :user + + json.message do + json.id boost.message_id + json.url room_message_url(boost.message.room, boost.message) + end +end diff --git a/app/views/messages/boosts/by_bots/show.json.jbuilder b/app/views/messages/boosts/by_bots/show.json.jbuilder new file mode 100644 index 0000000..a3acc9e --- /dev/null +++ b/app/views/messages/boosts/by_bots/show.json.jbuilder @@ -0,0 +1 @@ +json.partial! "messages/boosts/boost", boost: @boost diff --git a/app/views/messages/by_bots/index.json.jbuilder b/app/views/messages/by_bots/index.json.jbuilder new file mode 100644 index 0000000..27db314 --- /dev/null +++ b/app/views/messages/by_bots/index.json.jbuilder @@ -0,0 +1 @@ +json.array! @messages, partial: "messages/message", as: :message diff --git a/app/views/users/_user.json.jbuilder b/app/views/users/_user.json.jbuilder new file mode 100644 index 0000000..4425302 --- /dev/null +++ b/app/views/users/_user.json.jbuilder @@ -0,0 +1,5 @@ +json.cache! user do + json.(user, :id, :name, :role) + + json.avatar_url fresh_user_avatar_url(user) +end diff --git a/config/routes.rb b/config/routes.rb index 26afbcb..03466a1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -62,10 +62,13 @@ Rails.application.routes.draw do resources :rooms do resources :messages - # Bot API endpoints - authenticated via bot_key in URL - get ":bot_key/messages", to: "messages/by_bots#index", as: :bot_messages_index - post ":bot_key/messages", to: "messages/by_bots#create", as: :bot_messages - post ":bot_key/messages/:message_id/boosts", to: "messages/boosts/by_bots#create", as: :bot_message_boosts + nested do + scope path: ":bot_key", as: :bot, defaults: { format: :json } do + resources :messages, controller: "messages/by_bots", only: %i[ index create ] do + resources :boosts, controller: "messages/boosts/by_bots", only: :create + end + end + end scope module: "rooms" do resource :refresh, only: :show diff --git a/test/controllers/messages/boosts/by_bots_controller_test.rb b/test/controllers/messages/boosts/by_bots_controller_test.rb index 09dfd86..a2487e5 100644 --- a/test/controllers/messages/boosts/by_bots_controller_test.rb +++ b/test/controllers/messages/boosts/by_bots_controller_test.rb @@ -3,27 +3,29 @@ require "test_helper" class Messages::Boosts::ByBotsControllerTest < ActionDispatch::IntegrationTest setup do @room = rooms(:watercooler) - @message = messages(:fourth) # Message in watercooler room where bender bot is a member + @message = messages(:fourth) @bot = users(:bender) end - test "create adds a boost to the message" do + test "create adds a boost to the message and returns it" do assert_difference -> { @message.boosts.count }, +1 do post room_bot_message_boosts_url(@room, @bot.bot_key, @message), params: +"👀" assert_response :created end - assert_equal "👀", @message.boosts.last.content + boost = @message.boosts.last + assert_equal "👀", boost.content + assert_equal @bot, boost.booster + + json = JSON.parse(response.body) + assert_equal boost.id, json["id"] + assert_equal "👀", json["content"] + assert_equal @bot.id, json["booster"]["id"] + assert_equal @message.id, json["message"]["id"] + assert_equal room_message_url(@room, @message), json["message"]["url"] end - test "create with emoji reaction" do - assert_difference -> { Boost.count }, +1 do - post room_bot_message_boosts_url(@room, @bot.bot_key, @message), params: +"🎉" - assert_response :created - end - end - - test "create with text reaction" do + test "create with text content" do assert_difference -> { Boost.count }, +1 do post room_bot_message_boosts_url(@room, @bot.bot_key, @message), params: +"Nice!" assert_response :created @@ -38,35 +40,39 @@ class Messages::Boosts::ByBotsControllerTest < ActionDispatch::IntegrationTest end end - test "create requires valid bot key" do + test "create without content" do + assert_no_difference -> { Boost.count } do + post room_bot_message_boosts_url(@room, @bot.bot_key, @message) + assert_response :unprocessable_content + + post room_bot_message_boosts_url(@room, @bot.bot_key, @message), params: +" " + assert_response :unprocessable_content + end + end + + test "create requires a valid bot key" do assert_no_difference -> { Boost.count } do post room_bot_message_boosts_url(@room, "invalid-bot-key", @message), params: +"👀" end - assert_response :redirect # Redirects to login + assert_response :redirect end - test "create returns not_found for room bot is not a member of" do - room_without_bot = rooms(:designers) - message_in_other_room = messages(:first) # Message in designers room - + test "create is not found for a room the bot is not a member of" do assert_no_difference -> { Boost.count } do - post room_bot_message_boosts_url(room_without_bot, @bot.bot_key, message_in_other_room), params: +"👀" + post room_bot_message_boosts_url(rooms(:designers), @bot.bot_key, messages(:first)), params: +"👀" end assert_response :not_found end - test "create returns not_found for message not in the room" do - message_in_other_room = messages(:first) # Message in designers room, not watercooler - + test "create is not found for a message outside the room" do assert_no_difference -> { Boost.count } do - post room_bot_message_boosts_url(@room, @bot.bot_key, message_in_other_room), params: +"👀" + post room_bot_message_boosts_url(@room, @bot.bot_key, messages(:first)), params: +"👀" end assert_response :not_found end - test "create can't be abused to post boosts as regular user" do - user = users(:kevin) - bot_key = "#{user.id}-" + test "create can't be abused to post boosts as a regular user" do + bot_key = "#{users(:kevin).id}-" assert_no_difference -> { Boost.count } do post room_bot_message_boosts_url(@room, bot_key, @message), params: +"👀" diff --git a/test/controllers/messages/by_bots_controller_test.rb b/test/controllers/messages/by_bots_controller_test.rb index f1122da..0c145e7 100644 --- a/test/controllers/messages/by_bots_controller_test.rb +++ b/test/controllers/messages/by_bots_controller_test.rb @@ -1,6 +1,6 @@ require "test_helper" -class Messages::ByBotsControlleTest < ActionDispatch::IntegrationTest +class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest setup do @room = rooms(:watercooler) end @@ -40,6 +40,16 @@ class Messages::ByBotsControlleTest < ActionDispatch::IntegrationTest end end + test "create without a body or attachment" do + assert_no_difference -> { Message.count } do + post room_bot_messages_url(@room, users(:bender).bot_key) + assert_response :unprocessable_content + + post room_bot_messages_url(@room, users(:bender).bot_key), params: +" " + assert_response :unprocessable_content + end + end + test "create can't be abused to post messages as any user" do user = users(:kevin) bot_key = "#{user.id}-" @@ -51,77 +61,90 @@ class Messages::ByBotsControlleTest < ActionDispatch::IntegrationTest assert_response :redirect end - test "index returns messages as JSON" do - get room_bot_messages_index_url(@room, users(:bender).bot_key) + test "index returns the room's messages in the order they were sent" do + get room_bot_messages_url(@room, users(:bender).bot_key) assert_response :success json = JSON.parse(response.body) - assert json["room"]["id"].present? - assert json["room"]["name"].present? - assert json["messages"].is_a?(Array) - assert json["pagination"].present? + assert_equal @room.messages.ordered.map(&:id), json.map { it["id"] } end test "index includes message details" do - # Create a message in the room first - post room_bot_messages_url(@room, users(:bender).bot_key), params: +"Test message for index" + post room_bot_messages_url(@room, users(:bender).bot_key), params: +"Hello from Bender!" - get room_bot_messages_index_url(@room, users(:bender).bot_key) + get room_bot_messages_url(@room, users(:bender).bot_key) + assert_response :success + + message = Message.last + json_message = JSON.parse(response.body).last + assert_equal message.id, json_message["id"] + assert_equal "Hello from Bender!", json_message["body"]["plain_text"] + assert_includes json_message["body"]["html"], "Hello from Bender!" + assert_equal message.created_at.utc.iso8601(3), json_message["created_at"] + assert_equal users(:bender).id, json_message["creator"]["id"] + assert_equal "Bender Bot", json_message["creator"]["name"] + assert_equal "bot", json_message["creator"]["role"] + assert_equal @room.id, json_message["room"]["id"] + assert_equal room_message_url(@room, message), json_message["url"] + end + + test "index pages through older messages with the Link header" do + (Message::PAGE_SIZE - @room.messages.count + 1).times do |i| + @room.messages.create!(body: "Filler #{i}", creator: users(:jason), client_message_id: "filler-#{i}") + end + + get room_bot_messages_url(@room, users(:bender).bot_key) assert_response :success json = JSON.parse(response.body) - message = json["messages"].find { |m| m["body"]["plain"] == "Test message for index" } - assert message.present?, "Expected to find the test message" - assert message["id"].present? - assert message["created_at"].present? - assert message["creator"]["id"].present? - assert message["creator"]["name"].present? - end + assert_equal Message::PAGE_SIZE, json.size + assert_equal "41", response.headers["X-Total-Count"] + assert_not_includes json.map { it["id"] }, messages(:fourth).id - test "index supports pagination with before parameter" do - # Use a message from the watercooler room (where bender is a member) - message_in_room = messages(:thirteenth) # Latest message in watercooler - get room_bot_messages_index_url(@room, users(:bender).bot_key, before: message_in_room.id) + get response.headers["Link"][/<(.*)>/, 1] assert_response :success + + json = JSON.parse(response.body) + assert_equal [ messages(:fourth).id ], json.map { it["id"] } + assert_nil response.headers["Link"] end - test "index supports pagination with after parameter" do - # Use a message from the watercooler room (where bender is a member) - message_in_room = messages(:fourth) # First message in watercooler - get room_bot_messages_index_url(@room, users(:bender).bot_key, after: message_in_room.id) + test "index pages newer messages with after" do + get room_bot_messages_url(@room, users(:bender).bot_key, after: messages(:tenth).id) assert_response :success + + json = JSON.parse(response.body) + assert_equal %i[ eleventh twelfth thirteenth ].map { messages(it).id }, json.map { it["id"] } + assert_nil response.headers["Link"] end - test "index requires valid bot key" do - get room_bot_messages_index_url(@room, "invalid-bot-key") - assert_response :redirect # Redirects to login + test "index in a room with no messages" do + get room_bot_messages_url(rooms(:bender_and_kevin), users(:bender).bot_key) + assert_response :success + + assert_equal [], JSON.parse(response.body) + assert_equal "0", response.headers["X-Total-Count"] + assert_nil response.headers["Link"] end - test "index returns not_found for room bot is not a member of" do - # bender bot is NOT a member of the designers room - room_without_bot = rooms(:designers) - get room_bot_messages_index_url(room_without_bot, users(:bender).bot_key) + test "index requires a valid bot key" do + get room_bot_messages_url(@room, "invalid-bot-key") + assert_response :redirect + end + + test "index is not found for a room the bot is not a member of" do + get room_bot_messages_url(rooms(:designers), users(:bender).bot_key) assert_response :not_found end - test "index works for room bot IS a member of" do - # bender bot IS a member of watercooler - room_with_bot = rooms(:watercooler) - get room_bot_messages_index_url(room_with_bot, users(:bender).bot_key) - assert_response :success - end - - test "create returns not_found for room bot is not a member of" do - # bender bot is NOT a member of the designers room - verify create matches index behavior - room_without_bot = rooms(:designers) + test "create is not found for a room the bot is not a member of" do assert_no_difference -> { Message.count } do - post room_bot_messages_url(room_without_bot, users(:bender).bot_key), params: +"Hello!" + post room_bot_messages_url(rooms(:designers), users(:bender).bot_key), params: +"Hello!" end assert_response :not_found end - test "regular messages index still denied for bots" do - # The standard messages endpoint (not the bot-specific one) should still be forbidden + test "regular messages index remains denied for bots" do get room_messages_url(@room, bot_key: users(:bender).bot_key) assert_response :forbidden end