diff --git a/app/controllers/rooms_controller.rb b/app/controllers/rooms_controller.rb index 276265a..237d35a 100644 --- a/app/controllers/rooms_controller.rb +++ b/app/controllers/rooms_controller.rb @@ -12,7 +12,7 @@ class RoomsController < ApplicationController end def destroy - @room.destroy + @room.destroy_later broadcast_remove_room redirect_to root_url diff --git a/app/jobs/room/destroy_job.rb b/app/jobs/room/destroy_job.rb new file mode 100644 index 0000000..dc647e3 --- /dev/null +++ b/app/jobs/room/destroy_job.rb @@ -0,0 +1,7 @@ +class Room::DestroyJob < ApplicationJob + discard_on ActiveJob::DeserializationError + + def perform(room) + room.destroy_one_message_at_a_time + end +end diff --git a/app/models/room.rb b/app/models/room.rb index 20865fc..9a23847 100644 --- a/app/models/room.rb +++ b/app/models/room.rb @@ -45,6 +45,27 @@ class Room < ApplicationRecord end end + # Takes the room away from its members at once, and leaves its messages to Room::DestroyJob. Destroying them in + # the room's own transaction held the database's write lock, and stopped every other write, until the last one. + # An open room is closed first, so that no one who joins the account before the job runs is let in. + def destroy_later + room = open? ? becomes!(Rooms::Closed) : self + + transaction do + room.save! + room.memberships.delete_all + end + + Room::DestroyJob.perform_later(room) + end + + # Each message is destroyed in its own transaction, so other writes get through in between. Any message posted + # meanwhile goes with the room. + def destroy_one_message_at_a_time + messages.find_each(&:destroy) + destroy + end + def receive(message) unread_memberships(message) push_later(message) diff --git a/test/controllers/rooms/directs_controller_test.rb b/test/controllers/rooms/directs_controller_test.rb index a1d7b06..383788e 100644 --- a/test/controllers/rooms/directs_controller_test.rb +++ b/test/controllers/rooms/directs_controller_test.rb @@ -25,8 +25,10 @@ class Rooms::DirectsControllerTest < ActionDispatch::IntegrationTest sign_in :kevin assert_difference -> { Room.count }, -1 do - delete rooms_direct_url(rooms(:david_and_kevin)) - assert_redirected_to root_url + perform_enqueued_jobs do + delete rooms_direct_url(rooms(:david_and_kevin)) + assert_redirected_to root_url + end end end diff --git a/test/controllers/rooms_controller_test.rb b/test/controllers/rooms_controller_test.rb index dfba3bb..824d884 100644 --- a/test/controllers/rooms_controller_test.rb +++ b/test/controllers/rooms_controller_test.rb @@ -83,11 +83,23 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest test "destroy" do assert_turbo_stream_broadcasts :rooms, count: 1 do assert_difference -> { Room.count }, -1 do - delete room_url(rooms(:designers)) + perform_enqueued_jobs { delete room_url(rooms(:designers)) } end end end + test "destroy takes the room away from its members at once and leaves its messages to a job" do + room = rooms(:designers) + + assert_enqueued_with(job: Room::DestroyJob, args: [ room ]) do + assert_no_difference -> { Message.count } do + delete room_url(room) + assert_redirected_to root_url + end + end + assert_empty room.memberships.reload + end + test "destroy only allowed for creators or those who can administer" do sign_in :jz @@ -99,7 +111,7 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest rooms(:designers).update! creator: users(:jz) assert_difference -> { Room.count }, -1 do - delete room_url(rooms(:designers)) + perform_enqueued_jobs { delete room_url(rooms(:designers)) } end end diff --git a/test/models/room_test.rb b/test/models/room_test.rb index b113726..1c425ac 100644 --- a/test/models/room_test.rb +++ b/test/models/room_test.rb @@ -30,8 +30,54 @@ class RoomTest < ActiveSupport::TestCase assert Rooms::Closed.new.closed? end + test "an open room destroyed later lets no one in who joins the account before the job runs" do + room = rooms(:pets) + + room.destroy_later + newcomer = User.create!(name: "Newcomer", email_address: "newcomer@example.com", password: "secret123456") + + assert_not newcomer.memberships.exists?(room_id: room.id) + perform_enqueued_jobs only: Room::DestroyJob + assert_not Room.exists?(room.id) + end + + test "destroying one message at a time leaves nothing of the room behind" do + room = rooms(:designers) + searchable = room.messages.create!(body: "Kept in the search index", creator: users(:david)) + searchable.attachment.attach io: StringIO.new("hello"), filename: "hello.txt", content_type: "text/plain" + message_ids = room.messages.ids + assert Boost.where(message_id: message_ids).exists? + assert_equal 1, search_index_rows(searchable) + + room.destroy_one_message_at_a_time + + assert_not Room.exists?(room.id) + assert_empty Message.where(id: message_ids) + assert_empty Boost.where(message_id: message_ids) + assert_empty ActionText::RichText.where(record_type: "Message", record_id: message_ids) + assert_empty ActiveStorage::Attachment.where(record_type: "Message", record_id: message_ids) + assert_enqueued_jobs 1, only: ActiveStorage::PurgeJob + assert_equal 0, search_index_rows(searchable) + end + + test "each message is destroyed in its own transaction, so other writes get through in between" do + room = rooms(:designers) + messages = room.messages.count + transactions = 0 + count_transactions = ->(*, payload) { transactions += 1 if payload[:sql].start_with?("RELEASE SAVEPOINT") } + + ActiveSupport::Notifications.subscribed(count_transactions, "sql.active_record") { room.destroy_one_message_at_a_time } + + assert_equal messages + 1, transactions + end + test "default involvement for new users" do room = Rooms::Closed.create_for({ name: "Hello!", creator: users(:david) }, users: [ users(:kevin), users(:david) ]) assert room.memberships.all? { |m| m.involved_in_mentions? } end + + private + def search_index_rows(message) + Message.connection.select_value("select count(*) from message_search_index where rowid = #{message.id}") + end end