From 18e81dd4733b8b43cad7effd3419d49bfbcbb0ad Mon Sep 17 00:00:00 2001 From: bedrock-adam Date: Sun, 12 Oct 2025 14:01:37 +1100 Subject: [PATCH 1/5] commit schema and return values (WIP) --- db/migrate/create_outboxer_messages.rb | 2 ++ lib/outboxer/message.rb | 12 ++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/db/migrate/create_outboxer_messages.rb b/db/migrate/create_outboxer_messages.rb index 3ba45ab1..be4ad8ba 100644 --- a/db/migrate/create_outboxer_messages.rb +++ b/db/migrate/create_outboxer_messages.rb @@ -1,6 +1,8 @@ class CreateOutboxerMessages < ActiveRecord::Migration[6.1] def up create_table :outboxer_messages do |t| + t.integer :lock_version, null: false, default: 0 + t.string :status, limit: 255, null: false t.string :messageable_id, limit: 255, null: false diff --git a/lib/outboxer/message.rb b/lib/outboxer/message.rb index 9b8606fd..1837887e 100644 --- a/lib/outboxer/message.rb +++ b/lib/outboxer/message.rb @@ -119,6 +119,7 @@ def queue(messageable: nil, messageable_type: nil, messageable_id: nil, { id: message.id, + lock_version: message.lock_version, status: message.status, messageable_type: message.messageable_type, messageable_id: message.messageable_id, @@ -284,9 +285,10 @@ def publishing(time: ::Time) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) { - id: message[:id], - messageable_type: message[:messageable_type], - messageable_id: message[:messageable_id] + id: message.id, + lock_version: message.lock_version, + messageable_type: message.messageable_type, + messageable_id: message.messageable_id } end end @@ -392,7 +394,9 @@ def publishing_failed(id:, error: nil, time: ::Time) .where(status: Status::FAILED, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) - nil + { + lock_version: message.lock_version + } end end end From abf5273612292bf4901327b637bc12e403ccde0f Mon Sep 17 00:00:00 2001 From: bedrock-adam Date: Sun, 12 Oct 2025 17:40:51 +1100 Subject: [PATCH 2/5] pass build --- lib/outboxer/message.rb | 41 +++++++------ lib/outboxer/web.rb | 59 +++++++++++-------- lib/outboxer/web/views/messages.erb | 29 +++++---- spec/lib/outboxer/message/delete_spec.rb | 4 +- spec/lib/outboxer/message/publish_spec.rb | 7 +-- spec/lib/outboxer/message/requeue_spec.rb | 6 +- spec/lib/outboxer/web/message/delete_spec.rb | 2 +- spec/lib/outboxer/web/message/requeue_spec.rb | 2 +- spec/lib/outboxer/web/messages/update_spec.rb | 32 +++++----- 9 files changed, 101 insertions(+), 81 deletions(-) diff --git a/lib/outboxer/message.rb b/lib/outboxer/message.rb index 0f5555ec..618d8628 100644 --- a/lib/outboxer/message.rb +++ b/lib/outboxer/message.rb @@ -119,11 +119,7 @@ def queue(messageable: nil, messageable_type: nil, messageable_id: nil, { id: message.id, - lock_version: message.lock_version, - status: message.status, - messageable_type: message.messageable_type, - messageable_id: message.messageable_id, - updated_at: message.updated_at + lock_version: message.lock_version } end rescue MissingPartition @@ -204,7 +200,9 @@ def publish(logger: nil, time: ::Time) begin yield message rescue StandardError => error - publishing_failed(id: message[:id], error: error, time: time) + publishing_failed( + id: message[:id], lock_version: message[:lock_version], + error: error, time: time) logger&.error( "Outboxer message publishing failed id=#{message[:id]} " \ @@ -212,7 +210,8 @@ def publish(logger: nil, time: ::Time) "#{error.class}: #{error.message}\n" \ "#{error.backtrace.join("\n")}") rescue Exception => error - publishing_failed(id: message[:id], error: error, time: time) + publishing_failed( + id: message[:id], lock_version: message[:lock_version], error: error, time: time) logger&.fatal( "Outboxer message publishing failed id=#{message[:id]} " \ @@ -222,7 +221,7 @@ def publish(logger: nil, time: ::Time) raise else - published(id: message[:id], time: time) + published(id: message[:id], lock_version: message[:lock_version], time: time) logger&.info( "Outboxer message published id=#{message[:id]} " \ @@ -255,7 +254,7 @@ def publishing(time: ::Time) ActiveRecord::Base.connection_pool.with_connection do ActiveRecord::Base.transaction do message = Models::Message - .select(:id, :messageable_type, :messageable_id) + .select(:id, :lock_version, :messageable_type, :messageable_id) .where(status: Status::QUEUED) .order(:id) .limit(1) @@ -266,6 +265,7 @@ def publishing(time: ::Time) nil else message.update!( + lock_version: message.lock_version, status: Status::PUBLISHING, updated_at: current_utc_time, publishing_at: current_utc_time) @@ -308,7 +308,7 @@ def publishing(time: ::Time) # @note Executes within a transaction using a `SELECT ... FOR UPDATE` lock. # @example # Outboxer::Message.published(id: message[:id], time: Time) - def published(id:, time: ::Time) + def published(id:, lock_version:, time: ::Time) current_utc_time = time.now.utc ActiveRecord::Base.connection_pool.with_connection do @@ -319,6 +319,8 @@ def published(id:, time: ::Time) raise Error, "Status must be publishing" end + message.update!(lock_version: lock_version, updated_at: Time.now.utc) + message.exceptions.each { |exception| exception.frames.each(&:delete) } message.exceptions.delete_all message.delete @@ -337,7 +339,7 @@ def published(id:, time: ::Time) .where(status: Status::PUBLISHED, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) - nil + {} end end end @@ -356,7 +358,7 @@ def published(id:, time: ::Time) # @note Runs inside a transaction and uses `SELECT ... FOR UPDATE` locking. # @example # Outboxer::Message.publishing_failed(id: message[:id], time: Time) - def publishing_failed(id:, error: nil, time: ::Time) + def publishing_failed(id:, lock_version:, error: nil, time: ::Time) current_utc_time = time.now.utc ActiveRecord::Base.connection_pool.with_connection do @@ -368,6 +370,7 @@ def publishing_failed(id:, error: nil, time: ::Time) end message.update!( + lock_version: lock_version, status: Status::FAILED, updated_at: current_utc_time, failed_at: current_utc_time) @@ -483,12 +486,13 @@ def find_by_id(id:) # Deletes a message by ID. # @param id [Integer] the ID of the message to delete. # @return [Hash] details of the deleted message. - def delete(id:, time: ::Time) + def delete(id:, lock_version:, time: ::Time) current_utc_time = time.now.utc ActiveRecord::Base.connection_pool.with_connection do ActiveRecord::Base.transaction do message = Models::Message.includes(exceptions: :frames).lock.find_by!(id: id) + message.update!(lock_version: lock_version, updated_at: current_utc_time) message.exceptions.each { |exception| exception.frames.each(&:delete) } message.exceptions.delete_all @@ -520,7 +524,7 @@ def can_requeue?(status:) # @param publisher_name [String, nil] the name of the publisher. # @param time [Time] current time context used to update timestamps. # @return [Hash] updated message details. - def requeue(id:, publisher_id: nil, publisher_name: nil, + def requeue(id:, lock_version:, publisher_id: nil, publisher_name: nil, time: ::Time) current_utc_time = time.now.utc @@ -530,11 +534,8 @@ def requeue(id:, publisher_id: nil, publisher_name: nil, partition = calculate_partition(id: message.id) - Models::MessageCount - .where(status: message.status, partition: partition) - .update_all(["value = value - ?, updated_at = ?", 1, current_utc_time]) - message.update!( + lock_version: lock_version, status: Message::Status::QUEUED, updated_at: current_utc_time, queued_at: current_utc_time, @@ -544,6 +545,10 @@ def requeue(id:, publisher_id: nil, publisher_name: nil, publisher_id: publisher_id, publisher_name: publisher_name) + Models::MessageCount + .where(status: message.status, partition: partition) + .update_all(["value = value - ?, updated_at = ?", 1, current_utc_time]) + Models::MessageCount .where(status: Status::QUEUED, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) diff --git a/lib/outboxer/web.rb b/lib/outboxer/web.rb index c0394447..1764879e 100755 --- a/lib/outboxer/web.rb +++ b/lib/outboxer/web.rb @@ -494,56 +494,65 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end post "/messages/update" do - ids = params[:selected_ids].map(&:to_i) flash = {} + selected_messages = params.fetch("selected_messages", {}).values + case params[:action] when "requeue_by_ids" - requeued_count = 0 - failed_count = 0 + requeued_message_count = 0 + failed_message_count = 0 - ids.each do |id| - Outboxer::Message.requeue(id: id) - requeued_count += 1 + selected_messages.each do |message_attributes| + Outboxer::Message.requeue( + id: message_attributes["id"].to_i, + lock_version: message_attributes["lock_version"].to_i + ) + requeued_message_count += 1 rescue StandardError => error settings.logger.error( - "[Outboxer::Web] Failed to requeue message id=#{id}\n" \ + "[Outboxer::Web] Failed to requeue message id=#{message_attributes["id"]}\n" \ "error_class=#{error.class}\n" \ "error_message=#{error.message.inspect}" ) - failed_count += 1 + failed_message_count += 1 end - if requeued_count.positive? - flash[:success] = "Requeued #{pluralise(requeued_count, "message")}" + if requeued_message_count.positive? + flash[:success] = "Requeued #{pluralise(requeued_message_count, 'message')}" end - if failed_count.positive? - flash[:danger] = "Requeue failed for #{pluralise(failed_count, "message")}" + if failed_message_count.positive? + flash[:danger] = "Requeue failed for #{pluralise(failed_message_count, 'message')}" end + when "delete_by_ids" - deleted_count = 0 - failed_count = 0 + deleted_message_count = 0 + failed_message_count = 0 - ids.each do |id| - Outboxer::Message.delete(id: id) - deleted_count += 1 + selected_messages.each do |message_attributes| + Outboxer::Message.delete( + id: message_attributes["id"].to_i, + lock_version: message_attributes["lock_version"].to_i + ) + deleted_message_count += 1 rescue StandardError => error settings.logger.error( - "[Outboxer::Web] Failed to delete message id=#{id}\n" \ + "[Outboxer::Web] Failed to delete message id=#{message_attributes["id"]}\n" \ "error_class=#{error.class}\n" \ "error_message=#{error.message.inspect}" ) - failed_count += 1 + failed_message_count += 1 end - if deleted_count.positive? - flash[:success] = "Deleted #{pluralise(deleted_count, "message")}" + if deleted_message_count.positive? + flash[:success] = "Deleted #{pluralise(deleted_message_count, 'message')}" end - if failed_count.positive? - flash[:danger] = "Delete failed for #{pluralise(failed_count, "message")}" + if failed_message_count.positive? + flash[:danger] = "Delete failed for #{pluralise(failed_message_count, 'message')}" end + else raise "Unknown action: #{params[:action]}" end @@ -656,7 +665,7 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end post "/message/:id/requeue" do - Message.requeue(id: params[:id]) + Message.requeue(id: params[:id], lock_version: params[:lock_version]) denormalised_query_params = denormalise_query_params( status: params[:status], @@ -679,7 +688,7 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end post "/message/:id/delete" do - Message.delete(id: params[:id]) + Message.delete(id: params[:id], lock_version: params[:lock_version]) denormalised_query_params = denormalise_query_params( status: params[:status], diff --git a/lib/outboxer/web/views/messages.erb b/lib/outboxer/web/views/messages.erb index 88cb3060..78da1e1a 100644 --- a/lib/outboxer/web/views/messages.erb +++ b/lib/outboxer/web/views/messages.erb @@ -17,8 +17,9 @@ <% end %> - <% messages.each do |message| %> - + <% messages.each_with_index do |message, i| %> + + <% end %> +
<% if Outboxer::Message.can_requeue?(status: denormalised_query_params[:status]) %> @@ -48,8 +51,9 @@ <% end %> - <% messages.each do |message| %> - + <% messages.each_with_index do |message, i| %> + + <% end %>
diff --git a/spec/lib/outboxer/message/delete_spec.rb b/spec/lib/outboxer/message/delete_spec.rb index 7b3938eb..bf3613a1 100644 --- a/spec/lib/outboxer/message/delete_spec.rb +++ b/spec/lib/outboxer/message/delete_spec.rb @@ -13,7 +13,7 @@ module Outboxer let!(:exception) { create(:outboxer_exception, message: message) } let!(:frame) { create(:outboxer_frame, exception: exception) } - let!(:result) { Message.delete(id: message.id) } + let!(:result) { Message.delete(id: message.id, lock_version: message.lock_version) } it "deletes the message" do expect(Models::Message).not_to exist(message.id) @@ -42,7 +42,7 @@ module Outboxer let!(:exception) { create(:outboxer_exception, message: message) } let!(:frame) { create(:outboxer_frame, exception: exception) } - let!(:result) { Message.delete(id: message.id) } + let!(:result) { Message.delete(id: message.id, lock_version: message.lock_version) } it "deletes the message" do expect(Models::Message).not_to exist(message.id) diff --git a/spec/lib/outboxer/message/publish_spec.rb b/spec/lib/outboxer/message/publish_spec.rb index 66074ef3..0c45b6d6 100644 --- a/spec/lib/outboxer/message/publish_spec.rb +++ b/spec/lib/outboxer/message/publish_spec.rb @@ -23,11 +23,10 @@ module Outboxer end expect(yielded_id).to eq(queued_message.id) - expect(result).to eq({ + expect(result).to include( id: queued_message.id, messageable_type: queued_message.messageable_type, - messageable_id: queued_message.messageable_id - }) + messageable_id: queued_message.messageable_id) expect(Models::Message.published.count).to eql(0) end @@ -37,7 +36,7 @@ module Outboxer raise StandardError, "temporary failure" end - expect(result).to eq({ + expect(result).to include({ id: queued_message.id, messageable_type: queued_message.messageable_type, messageable_id: queued_message.messageable_id diff --git a/spec/lib/outboxer/message/requeue_spec.rb b/spec/lib/outboxer/message/requeue_spec.rb index 29bb4d8c..9cea628c 100644 --- a/spec/lib/outboxer/message/requeue_spec.rb +++ b/spec/lib/outboxer/message/requeue_spec.rb @@ -6,7 +6,9 @@ module Outboxer context "when failed message" do let!(:failed_message) { create(:outboxer_message, :failed) } - let!(:queued_message) { Message.requeue(id: failed_message.id) } + let!(:queued_message) do + Message.requeue(id: failed_message.id, lock_version: failed_message.lock_version) + end it "returns queued message" do expect(queued_message[:id]).to eq(failed_message.id) @@ -24,7 +26,7 @@ module Outboxer let!(:updated_at) { queued_message.updated_at } it "modifies updated_at" do - Message.requeue(id: queued_message.id) + Message.requeue(id: queued_message.id, lock_version: queued_message.lock_version) queued_message.reload diff --git a/spec/lib/outboxer/web/message/delete_spec.rb b/spec/lib/outboxer/web/message/delete_spec.rb index cd4951cc..0ebf7a23 100644 --- a/spec/lib/outboxer/web/message/delete_spec.rb +++ b/spec/lib/outboxer/web/message/delete_spec.rb @@ -15,7 +15,7 @@ def app before do header "Host", "localhost" - post "/message/#{message[:id]}/delete" + post "/message/#{message[:id]}/delete", { lock_version: message[:lock_version] } follow_redirect! end diff --git a/spec/lib/outboxer/web/message/requeue_spec.rb b/spec/lib/outboxer/web/message/requeue_spec.rb index dd51c574..3d932d1c 100644 --- a/spec/lib/outboxer/web/message/requeue_spec.rb +++ b/spec/lib/outboxer/web/message/requeue_spec.rb @@ -15,7 +15,7 @@ def app before do header "Host", "localhost" - post "/message/#{message[:id]}/requeue" + post "/message/#{message[:id]}/requeue", { lock_version: message[:lock_version] } follow_redirect! end diff --git a/spec/lib/outboxer/web/messages/update_spec.rb b/spec/lib/outboxer/web/messages/update_spec.rb index 715bc669..ee24efa7 100644 --- a/spec/lib/outboxer/web/messages/update_spec.rb +++ b/spec/lib/outboxer/web/messages/update_spec.rb @@ -1,5 +1,4 @@ require "rails_helper" - require_relative "../../../../../lib/outboxer/web" RSpec.describe "POST /messages/update", type: :request do @@ -24,16 +23,16 @@ def app before do message_2.update!(status: Outboxer::Message::Status::PUBLISHING) message_3.update!(status: Outboxer::Message::Status::FAILED) - header "Host", "localhost" end context "when action is requeue_by_ids" do - let(:ids) { [message_2.id, message_3.id] } - before do post "/messages/update", { - selected_ids: ids, + selected_messages: { + "0" => { id: message_2.id.to_s, lock_version: message_2.lock_version.to_s }, + "1" => { id: message_3.id.to_s, lock_version: message_3.lock_version.to_s } + }, action: "requeue_by_ids", status: :failed, page: 1, @@ -42,7 +41,6 @@ def app order: :desc, time_zone: "Australia/Sydney" } - follow_redirect! end @@ -55,19 +53,20 @@ def app it "redirects with flash message" do expect(last_response).to be_ok expect(last_request.url).to include( - "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney") - + "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney" + ) expected_flash = URI.encode_www_form_component("success:Requeued 2 messages") expect(last_request.url).to include("flash=#{expected_flash}") end end context "when action is delete_by_ids" do - let(:ids) { [message_2.id, message_3.id] } - before do post "/messages/update", { - selected_ids: ids, + selected_messages: { + "0" => { id: message_2.id.to_s, lock_version: message_2.lock_version.to_s }, + "1" => { id: message_3.id.to_s, lock_version: message_3.lock_version.to_s } + }, action: "delete_by_ids", status: :failed, page: 1, @@ -76,7 +75,6 @@ def app order: :desc, time_zone: "Australia/Sydney" } - follow_redirect! end @@ -87,18 +85,20 @@ def app it "redirects with flash message" do expect(last_response).to be_ok expect(last_request.url).to include( - "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney") + "messages?status=failed&sort=queued_at&order=desc&per_page=10&time_zone=Australia%2FSydney" + ) expected_flash = URI.encode_www_form_component("success:Deleted 2 messages") expect(last_request.url).to include("flash=#{expected_flash}") end end context "with invalid action" do - let(:ids) { [message_2.id, message_3.id] } - before do post "/messages/update", { - selected_ids: ids, + selected_messages: { + "0" => { id: message_2.id.to_s, lock_version: message_2.lock_version.to_s }, + "1" => { id: message_3.id.to_s, lock_version: message_3.lock_version.to_s } + }, action: "invalid", status: :failed, page: 1, From a30cb37fbc3d362e8c9e82f468edd49563c879a4 Mon Sep 17 00:00:00 2001 From: bedrock-adam Date: Sun, 12 Oct 2025 17:41:20 +1100 Subject: [PATCH 3/5] fix rubocop --- lib/outboxer/web.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/outboxer/web.rb b/lib/outboxer/web.rb index 1764879e..7e69a5f1 100755 --- a/lib/outboxer/web.rb +++ b/lib/outboxer/web.rb @@ -519,11 +519,11 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end if requeued_message_count.positive? - flash[:success] = "Requeued #{pluralise(requeued_message_count, 'message')}" + flash[:success] = "Requeued #{pluralise(requeued_message_count, "message")}" end if failed_message_count.positive? - flash[:danger] = "Requeue failed for #{pluralise(failed_message_count, 'message')}" + flash[:danger] = "Requeue failed for #{pluralise(failed_message_count, "message")}" end when "delete_by_ids" @@ -546,11 +546,11 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, end if deleted_message_count.positive? - flash[:success] = "Deleted #{pluralise(deleted_message_count, 'message')}" + flash[:success] = "Deleted #{pluralise(deleted_message_count, "message")}" end if failed_message_count.positive? - flash[:danger] = "Delete failed for #{pluralise(failed_message_count, 'message')}" + flash[:danger] = "Delete failed for #{pluralise(failed_message_count, "message")}" end else From 34f55b5ead227355ae14598c00213bc01efa3195 Mon Sep 17 00:00:00 2001 From: bedrock-adam Date: Sun, 12 Oct 2025 18:48:25 +1100 Subject: [PATCH 4/5] fix errors --- lib/outboxer/message.rb | 14 ++- lib/outboxer/web/views/message.erb | 38 +++++--- lib/outboxer/web/views/messages.erb | 141 +++++++++++++++++----------- 3 files changed, 119 insertions(+), 74 deletions(-) diff --git a/lib/outboxer/message.rb b/lib/outboxer/message.rb index 618d8628..601a8f83 100644 --- a/lib/outboxer/message.rb +++ b/lib/outboxer/message.rb @@ -407,6 +407,7 @@ def publishing_failed(id:, lock_version:, error: nil, time: ::Time) # Serializes message attributes into a hash. # # @param id [Integer] message ID. + # @param lock_version [Integer] message lock_version. # @param status [String] message status. # @param messageable_type [String] type of the messageable entity. # @param messageable_id [String] ID of the messageable entity. @@ -418,11 +419,12 @@ def publishing_failed(id:, lock_version:, error: nil, time: ::Time) # @param publisher_id [Integer, nil] optional publisher ID. # @param publisher_name [String, nil] optional publisher name. # @return [Hash] serialized message details. - def serialize(id:, status:, messageable_type:, messageable_id:, updated_at:, + def serialize(id:, lock_version:, status:, messageable_type:, messageable_id:, updated_at:, queued_at: nil, publishing_at: nil, published_at: nil, failed_at: nil, publisher_id: nil, publisher_name: nil) { id: id, + lock_version: lock_version, status: status, messageable_type: messageable_type, messageable_id: messageable_id, @@ -452,6 +454,7 @@ def find_by_id(id:) { id: message.id, + lock_version: message.lock_version, status: message.status, messageable_type: message.messageable_type, messageable_id: message.messageable_id, @@ -534,6 +537,8 @@ def requeue(id:, lock_version:, publisher_id: nil, publisher_name: nil, partition = calculate_partition(id: message.id) + original_status = message.status + message.update!( lock_version: lock_version, status: Message::Status::QUEUED, @@ -546,15 +551,15 @@ def requeue(id:, lock_version:, publisher_id: nil, publisher_name: nil, publisher_name: publisher_name) Models::MessageCount - .where(status: message.status, partition: partition) + .where(status: original_status, partition: partition) .update_all(["value = value - ?, updated_at = ?", 1, current_utc_time]) Models::MessageCount - .where(status: Status::QUEUED, partition: partition) + .where(status: message.status, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) Models::MessageTotal - .where(status: Status::QUEUED, partition: partition) + .where(status: message.status, partition: partition) .update_all(["value = value + ?, updated_at = ?", 1, current_utc_time]) { id: id } @@ -640,6 +645,7 @@ def list(status: LIST_STATUS_DEFAULT, messages: paginated_messages.map do |message| { id: message.id, + lock_version: message.lock_version, status: message.status.to_sym, messageable_type: message.messageable_type, messageable_id: message.messageable_id, diff --git a/lib/outboxer/web/views/message.erb b/lib/outboxer/web/views/message.erb index 055d3c1d..119bf69a 100644 --- a/lib/outboxer/web/views/message.erb +++ b/lib/outboxer/web/views/message.erb @@ -4,25 +4,33 @@

Message <%= message[:id] %>

-
" method="post" style="display: inline;"> - <% normalised_query_params.each do |key, param| %> - - <% end %> - + " method="post" + style="display: inline;"> + <% normalised_query_params.each do |key, param| %> + + <% end %> + + +
+ -
" method="post" style="display: inline;" + " method="post" + style="display: inline;" onsubmit="return confirm('Are you sure you want to delete this message?');"> + <% normalised_query_params.each do |key, param| %> + + <% end %> + - <% normalised_query_params.each do |key, param| %> - - <% end %> - +
diff --git a/lib/outboxer/web/views/messages.erb b/lib/outboxer/web/views/messages.erb index 78da1e1a..0b8e0d48 100644 --- a/lib/outboxer/web/views/messages.erb +++ b/lib/outboxer/web/views/messages.erb @@ -85,19 +85,27 @@
<% normalised_query_params.each do |key, param| %> - + + <% end %> + + <% messages.each_with_index do |message, i| %> + + <% end %> -
@@ -165,23 +173,28 @@
" method="post"> <% normalised_query_params.each do |key, param| %> - + <% end %> -
" method="post"> <% normalised_query_params.each do |key, param| %> - + <% end %> + - @@ -247,58 +260,76 @@ From d54cb9815767fd6d8d5bcb6f37155fdd3a6312ca Mon Sep 17 00:00:00 2001 From: bedrock-adam Date: Mon, 13 Oct 2025 21:37:54 +1100 Subject: [PATCH 5/5] fix bugs --- lib/outboxer/web.rb | 21 ++-- lib/outboxer/web/views/messages.erb | 114 +++++++----------- spec/lib/outboxer/message/list/filter_spec.rb | 4 + .../message/list/no_arguments_spec.rb | 5 + spec/lib/outboxer/message/serialize_spec.rb | 2 + spec/lib/outboxer/web/messages/update_spec.rb | 26 ++-- 6 files changed, 78 insertions(+), 94 deletions(-) diff --git a/lib/outboxer/web.rb b/lib/outboxer/web.rb index 7e69a5f1..005edd67 100755 --- a/lib/outboxer/web.rb +++ b/lib/outboxer/web.rb @@ -496,22 +496,25 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, post "/messages/update" do flash = {} - selected_messages = params.fetch("selected_messages", {}).values + selected_messages = params.fetch("selected_messages", []).map do |pair| + id, lock_version = pair.split(":").map(&:to_i) + { id: id, lock_version: lock_version } + end case params[:action] when "requeue_by_ids" requeued_message_count = 0 failed_message_count = 0 - selected_messages.each do |message_attributes| + selected_messages.each do |selected_message| Outboxer::Message.requeue( - id: message_attributes["id"].to_i, - lock_version: message_attributes["lock_version"].to_i + id: selected_message[:id], + lock_version: selected_message[:lock_version] ) requeued_message_count += 1 rescue StandardError => error settings.logger.error( - "[Outboxer::Web] Failed to requeue message id=#{message_attributes["id"]}\n" \ + "[Outboxer::Web] Failed to requeue message id=#{selected_message[:id]}\n" \ "error_class=#{error.class}\n" \ "error_message=#{error.message.inspect}" ) @@ -530,15 +533,15 @@ def normalise_query_string(status: Message::LIST_STATUS_DEFAULT, deleted_message_count = 0 failed_message_count = 0 - selected_messages.each do |message_attributes| + selected_messages.each do |selected_message| Outboxer::Message.delete( - id: message_attributes["id"].to_i, - lock_version: message_attributes["lock_version"].to_i + id: selected_message[:id], + lock_version: selected_message[:lock_version] ) deleted_message_count += 1 rescue StandardError => error settings.logger.error( - "[Outboxer::Web] Failed to delete message id=#{message_attributes["id"]}\n" \ + "[Outboxer::Web] Failed to delete message id=#{selected_message[:id]}\n" \ "error_class=#{error.class}\n" \ "error_message=#{error.message.inspect}" ) diff --git a/lib/outboxer/web/views/messages.erb b/lib/outboxer/web/views/messages.erb index 0b8e0d48..32581d73 100644 --- a/lib/outboxer/web/views/messages.erb +++ b/lib/outboxer/web/views/messages.erb @@ -14,12 +14,12 @@ " method="post" class="me-2"> <% normalised_query_params.each do |key, param| %> - + <% end %> <% messages.each_with_index do |message, i| %> - - + "> <% end %> + - - - - + + +
<% if messages.empty? %> @@ -139,7 +131,13 @@ <% messages.each do |message| %> - + + "> + " class="custom-link"> <%= message[:id] %> @@ -264,48 +262,21 @@ document.addEventListener('DOMContentLoaded', function () { const checkboxes = document.querySelectorAll('.individual-check'); const checkAll = document.getElementById('checkAll'); const bulkActionsToolbar = document.getElementById('bulkActionsToolbar'); - const bulkActionForm = document.getElementById('bulkActionForm'); + + // Hide toolbar initially (enhancement only) + bulkActionsToolbar.classList.add('d-none'); function toggleBulkActionsToolbar() { const anyChecked = Array.from(checkboxes).some(checkbox => checkbox.checked); bulkActionsToolbar.classList.toggle('d-none', !anyChecked); - updateSelectedMessages(); - } - - function updateSelectedMessages() { - // Remove previously generated hidden fields - bulkActionForm.querySelectorAll('input[name^="selected_messages"]').forEach(el => el.remove()); - - // For each checked message, add both id and lock_version fields - Array.from(checkboxes) - .filter(checkbox => checkbox.checked) - .forEach((checkbox, index) => { - const messageId = checkbox.dataset.messageId; - const lockVersion = checkbox.dataset.lockVersion; - - if (messageId && lockVersion) { - const idField = document.createElement('input'); - idField.type = 'hidden'; - idField.name = `selected_messages[${index}][id]`; - idField.value = messageId; - - const lockField = document.createElement('input'); - lockField.type = 'hidden'; - lockField.name = `selected_messages[${index}][lock_version]`; - lockField.value = lockVersion; - - bulkActionForm.appendChild(idField); - bulkActionForm.appendChild(lockField); - } - }); } - // Handle individual checkboxes + // Individual checkbox toggles toolbar visibility checkboxes.forEach(checkbox => { checkbox.addEventListener('change', toggleBulkActionsToolbar); }); - // Handle "Check All" toggle + // "Select All" toggle if (checkAll) { checkAll.addEventListener('change', function (e) { checkboxes.forEach(box => (box.checked = e.target.checked)); @@ -313,7 +284,7 @@ document.addEventListener('DOMContentLoaded', function () { }); } - // Confirm Delete All + // Confirmation prompts for destructive actions const deleteAllBtn = document.getElementById('deleteAllBtn'); if (deleteAllBtn) { deleteAllBtn.addEventListener('click', function (e) { @@ -321,7 +292,6 @@ document.addEventListener('DOMContentLoaded', function () { }); } - // Confirm Retry All const retryAllBtn = document.getElementById('retryAllBtn'); if (retryAllBtn) { retryAllBtn.addEventListener('click', function (e) { diff --git a/spec/lib/outboxer/message/list/filter_spec.rb b/spec/lib/outboxer/message/list/filter_spec.rb index 2debbdca..79008b6a 100644 --- a/spec/lib/outboxer/message/list/filter_spec.rb +++ b/spec/lib/outboxer/message/list/filter_spec.rb @@ -51,6 +51,7 @@ module Outboxer messages: [ { id: message_1.id, + lock_version: message_1.lock_version, status: message_1.status.to_sym, messageable_type: message_1.messageable_type, messageable_id: message_1.messageable_id, @@ -65,6 +66,7 @@ module Outboxer }, { id: message_4.id, + lock_version: message_4.lock_version, status: message_4.status.to_sym, messageable_type: message_4.messageable_type, messageable_id: message_4.messageable_id, @@ -89,6 +91,7 @@ module Outboxer messages: [ { id: message_5.id, + lock_version: message_5.lock_version, status: message_5.status.to_sym, messageable_type: message_5.messageable_type, messageable_id: message_5.messageable_id, @@ -113,6 +116,7 @@ module Outboxer messages: [ { id: message_2.id, + lock_version: message_2.lock_version, status: message_2.status.to_sym, messageable_type: message_2.messageable_type, messageable_id: message_2.messageable_id, diff --git a/spec/lib/outboxer/message/list/no_arguments_spec.rb b/spec/lib/outboxer/message/list/no_arguments_spec.rb index 7dd90b61..4ba176b3 100644 --- a/spec/lib/outboxer/message/list/no_arguments_spec.rb +++ b/spec/lib/outboxer/message/list/no_arguments_spec.rb @@ -61,6 +61,7 @@ module Outboxer messages: [ { id: message_1.id, + lock_version: message_1.lock_version, status: message_1.status.to_sym, messageable_type: message_1.messageable_type, messageable_id: message_1.messageable_id, @@ -75,6 +76,7 @@ module Outboxer }, { id: message_2.id, + lock_version: message_2.lock_version, status: message_2.status.to_sym, messageable_type: message_2.messageable_type, messageable_id: message_2.messageable_id, @@ -89,6 +91,7 @@ module Outboxer }, { id: message_3.id, + lock_version: message_3.lock_version, status: message_3.status.to_sym, messageable_type: message_3.messageable_type, messageable_id: message_3.messageable_id, @@ -103,6 +106,7 @@ module Outboxer }, { id: message_4.id, + lock_version: message_4.lock_version, status: message_4.status.to_sym, messageable_type: message_4.messageable_type, messageable_id: message_4.messageable_id, @@ -117,6 +121,7 @@ module Outboxer }, { id: message_5.id, + lock_version: message_5.lock_version, status: message_5.status.to_sym, messageable_type: message_5.messageable_type, messageable_id: message_5.messageable_id, diff --git a/spec/lib/outboxer/message/serialize_spec.rb b/spec/lib/outboxer/message/serialize_spec.rb index 3777c5d5..14da995d 100644 --- a/spec/lib/outboxer/message/serialize_spec.rb +++ b/spec/lib/outboxer/message/serialize_spec.rb @@ -7,6 +7,7 @@ module Outboxer expect( Message.serialize( id: 1, + lock_version: 2, status: "publishing", messageable_type: "Invoice", messageable_id: "INV-001", @@ -20,6 +21,7 @@ module Outboxer ).to eq( { id: 1, + lock_version: 2, status: "publishing", messageable_type: "Invoice", messageable_id: "INV-001", diff --git a/spec/lib/outboxer/web/messages/update_spec.rb b/spec/lib/outboxer/web/messages/update_spec.rb index ee24efa7..af5c305c 100644 --- a/spec/lib/outboxer/web/messages/update_spec.rb +++ b/spec/lib/outboxer/web/messages/update_spec.rb @@ -29,10 +29,10 @@ def app context "when action is requeue_by_ids" do before do post "/messages/update", { - selected_messages: { - "0" => { id: message_2.id.to_s, lock_version: message_2.lock_version.to_s }, - "1" => { id: message_3.id.to_s, lock_version: message_3.lock_version.to_s } - }, + selected_messages: [ + "#{message_2.id}:#{message_2.lock_version}", + "#{message_3.id}:#{message_3.lock_version}" + ], action: "requeue_by_ids", status: :failed, page: 1, @@ -45,7 +45,7 @@ def app end it "queues selected messages" do - expect(Outboxer::Models::Message.queued.pluck(:id)).to eq([ + expect(Outboxer::Models::Message.queued.pluck(:id)).to match_array([ message_1.id, message_2.id, message_3.id ]) end @@ -63,10 +63,10 @@ def app context "when action is delete_by_ids" do before do post "/messages/update", { - selected_messages: { - "0" => { id: message_2.id.to_s, lock_version: message_2.lock_version.to_s }, - "1" => { id: message_3.id.to_s, lock_version: message_3.lock_version.to_s } - }, + selected_messages: [ + "#{message_2.id}:#{message_2.lock_version}", + "#{message_3.id}:#{message_3.lock_version}" + ], action: "delete_by_ids", status: :failed, page: 1, @@ -95,10 +95,10 @@ def app context "with invalid action" do before do post "/messages/update", { - selected_messages: { - "0" => { id: message_2.id.to_s, lock_version: message_2.lock_version.to_s }, - "1" => { id: message_3.id.to_s, lock_version: message_3.lock_version.to_s } - }, + selected_messages: [ + "#{message_2.id}:#{message_2.lock_version}", + "#{message_3.id}:#{message_3.lock_version}" + ], action: "invalid", status: :failed, page: 1,