From dd297ba59f323aba7acca2896bb2045f9f60b28b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Thu, 5 Mar 2026 18:21:17 -0600 Subject: [PATCH 1/3] Fix CI because Brakeman mismatch --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 98d163e..c040098 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -101,7 +101,7 @@ GEM bootstrap-sass (3.4.1) autoprefixer-rails (>= 5.2.1) sassc (>= 2.0.0) - brakeman (8.0.2) + brakeman (8.0.4) racc builder (3.3.0) capybara (3.40.0) From 51e07ff40feebf52a754877b6dbc9604385d5928 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Thu, 5 Mar 2026 18:19:56 -0600 Subject: [PATCH 2/3] Fix double render in Slack::PuzzlesController when notification fails When send_message rescued a Slack::Web::Api::Errors::SlackError and called head :unprocessable_entity, execution returned to create which then called render json:, raising AbstractController::DoubleRenderError. Added return if performed? to halt after send_message when it has already rendered. --- app/controllers/slack/puzzles_controller.rb | 1 + .../slack/puzzles_controller_test.rb | 59 +++++++++++++++++++ 2 files changed, 60 insertions(+) create mode 100644 test/controllers/slack/puzzles_controller_test.rb diff --git a/app/controllers/slack/puzzles_controller.rb b/app/controllers/slack/puzzles_controller.rb index d71e13e..708adaf 100644 --- a/app/controllers/slack/puzzles_controller.rb +++ b/app/controllers/slack/puzzles_controller.rb @@ -13,6 +13,7 @@ def create view = SlackClient::Views::Success.new.create notification_message = SlackClient::Messages::NewPuzzleNotification.new(puzzle).create send_message(notification_message, channel_id: ENV.fetch("SLACK_NOTIFICATIONS_CHANNEL", nil)) + return if performed? else view = SlackClient::Views::Failure.new.create end diff --git a/test/controllers/slack/puzzles_controller_test.rb b/test/controllers/slack/puzzles_controller_test.rb new file mode 100644 index 0000000..98748b8 --- /dev/null +++ b/test/controllers/slack/puzzles_controller_test.rb @@ -0,0 +1,59 @@ +require "test_helper" + +class Slack::PuzzlesControllerTest < ActionDispatch::IntegrationTest + setup do + ENV["SLACK_SIGNING_SECRET"] = "test_signing_secret" + end + + test "renders ok when puzzle is saved and notification succeeds" do + # Use a payload that fails validation so send_message is never called, + # verifying the action renders 200 without a double render. + params = { payload: puzzle_payload(question: "") } + + post slack_puzzle_path, params: params, + headers: slack_headers(body: params.to_query) + + assert_response :ok + end + + test "does not double render when Slack notification fails after puzzle is saved" do + # Override send_message to simulate a Slack API error that calls head :unprocessable_entity. + # Without the fix, this causes AbstractController::DoubleRenderError because create + # still calls render json: after send_message returns. + original = Slack::ApplicationController.instance_method(:send_message) + Slack::ApplicationController.define_method(:send_message) { |*| head :unprocessable_entity } + + params = { payload: puzzle_payload(question: "What is unique about Ruby's blocks?") } + + post slack_puzzle_path, params: params, + headers: slack_headers(body: params.to_query) + + assert_response :unprocessable_entity + ensure + Slack::ApplicationController.define_method(:send_message, original) + end + + private + + def puzzle_payload(question: "What is Ruby?") + { + user: { id: "U123" }, + view: { + state: { + values: { + question: { question: { value: question } }, + answer: { answer: { selected_option: { value: "ruby" } } }, + explanation: { explanation: { value: "It is a programming language." } }, + link: { link: { value: nil } } + } + } + } + }.to_json + end + + def slack_headers(secret: ENV["SLACK_SIGNING_SECRET"], timestamp: Time.now.to_i, body: "") + ts = timestamp.to_s + sig = "v0=" + OpenSSL::HMAC.hexdigest("SHA256", secret, "v0:#{ts}:#{body}") + { "X-Slack-Request-Timestamp" => ts, "X-Slack-Signature" => sig } + end +end From bc227633d1436f37a7166b16d44e64a9d83fb690 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20V=C3=A1squez?= Date: Thu, 5 Mar 2026 18:38:10 -0600 Subject: [PATCH 3/3] Replace head :unprocessable_entity in send_message with logging and Sentry Calling head :unprocessable_entity inside send_message was a rendering side effect that leaked into callers and wrongly told Slack the request failed even when the puzzle was saved. Now logs the error, captures it in Sentry for full context, and lets create always render 200 on success. --- app/controllers/slack/application_controller.rb | 5 +++-- app/controllers/slack/puzzles_controller.rb | 1 - test/controllers/slack/puzzles_controller_test.rb | 14 +++++--------- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/controllers/slack/application_controller.rb b/app/controllers/slack/application_controller.rb index e587022..43a5783 100644 --- a/app/controllers/slack/application_controller.rb +++ b/app/controllers/slack/application_controller.rb @@ -43,7 +43,8 @@ def open_view(view, trigger_id:) def send_message(message, channel_id:) SlackClient::Client.instance.chat_postMessage(channel: channel_id, blocks: message) - rescue Slack::Web::Api::Errors::SlackError - head :unprocessable_entity + rescue Slack::Web::Api::Errors::SlackError => e + Rails.logger.error "Failed to send Slack message: #{e.message}" + Sentry.capture_exception(e) end end diff --git a/app/controllers/slack/puzzles_controller.rb b/app/controllers/slack/puzzles_controller.rb index 708adaf..d71e13e 100644 --- a/app/controllers/slack/puzzles_controller.rb +++ b/app/controllers/slack/puzzles_controller.rb @@ -13,7 +13,6 @@ def create view = SlackClient::Views::Success.new.create notification_message = SlackClient::Messages::NewPuzzleNotification.new(puzzle).create send_message(notification_message, channel_id: ENV.fetch("SLACK_NOTIFICATIONS_CHANNEL", nil)) - return if performed? else view = SlackClient::Views::Failure.new.create end diff --git a/test/controllers/slack/puzzles_controller_test.rb b/test/controllers/slack/puzzles_controller_test.rb index 98748b8..ee183e8 100644 --- a/test/controllers/slack/puzzles_controller_test.rb +++ b/test/controllers/slack/puzzles_controller_test.rb @@ -5,9 +5,7 @@ class Slack::PuzzlesControllerTest < ActionDispatch::IntegrationTest ENV["SLACK_SIGNING_SECRET"] = "test_signing_secret" end - test "renders ok when puzzle is saved and notification succeeds" do - # Use a payload that fails validation so send_message is never called, - # verifying the action renders 200 without a double render. + test "renders ok when puzzle fails validation" do params = { payload: puzzle_payload(question: "") } post slack_puzzle_path, params: params, @@ -16,19 +14,17 @@ class Slack::PuzzlesControllerTest < ActionDispatch::IntegrationTest assert_response :ok end - test "does not double render when Slack notification fails after puzzle is saved" do - # Override send_message to simulate a Slack API error that calls head :unprocessable_entity. - # Without the fix, this causes AbstractController::DoubleRenderError because create - # still calls render json: after send_message returns. + test "renders ok even when Slack notification fails after puzzle is saved" do + # Notification failure should only log — it must not affect the response to Slack. original = Slack::ApplicationController.instance_method(:send_message) - Slack::ApplicationController.define_method(:send_message) { |*| head :unprocessable_entity } + Slack::ApplicationController.define_method(:send_message) { |*| nil } params = { payload: puzzle_payload(question: "What is unique about Ruby's blocks?") } post slack_puzzle_path, params: params, headers: slack_headers(body: params.to_query) - assert_response :unprocessable_entity + assert_response :ok ensure Slack::ApplicationController.define_method(:send_message, original) end