diff --git a/app/jobs/room/destroy_job.rb b/app/jobs/room/destroy_job.rb index dc647e3..af5bef8 100644 --- a/app/jobs/room/destroy_job.rb +++ b/app/jobs/room/destroy_job.rb @@ -1,7 +1,8 @@ class Room::DestroyJob < ApplicationJob discard_on ActiveJob::DeserializationError - def perform(room) + def perform(room, former_member_ids) + room.reset_remote_connections_of(former_member_ids) room.destroy_one_message_at_a_time end end diff --git a/app/models/room.rb b/app/models/room.rb index 9a23847..ed7280d 100644 --- a/app/models/room.rb +++ b/app/models/room.rb @@ -51,12 +51,19 @@ class Room < ApplicationRecord def destroy_later room = open? ? becomes!(Rooms::Closed) : self - transaction do + former_member_ids = transaction do room.save! - room.memberships.delete_all + room.memberships.pluck(:user_id).tap { room.memberships.delete_all } end - Room::DestroyJob.perform_later(room) + Room::DestroyJob.perform_later(room, former_member_ids) + end + + # Deleting the memberships skips the reset that revoking one does, so the former members get it here, all at once, + # before the messages go: their connections stop receiving the room's streams. It's done in the job, not the + # request, because it costs a Redis round trip per member. + def reset_remote_connections_of(user_ids) + User.where(id: user_ids).find_each(&:reset_remote_connections) end # Each message is destroyed in its own transaction, so other writes get through in between. Any message posted diff --git a/app/models/user.rb b/app/models/user.rb index 20d05dd..df2fee1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,8 +50,18 @@ class User < ApplicationRecord end private + # One statement, which SQLite runs under the write lock, so no room is closed between reading the open ones and + # inserting. A room that Room#destroy_later closes is either closed before, and skipped, or after, and takes this + # membership away with the others. A room that granted itself to every user when it was created is skipped too, + # as insert_all did. def grant_membership_to_open_rooms - Membership.insert_all(Rooms::Open.pluck(:id).collect { |room_id| { room_id: room_id, user_id: id } }) + now = Time.current + + Membership.connection.execute Membership.sanitize_sql([ <<~SQL, id, now, now ]) + insert into memberships (room_id, user_id, created_at, updated_at) + select id, ?, ?, ? from rooms where type = 'Rooms::Open' + on conflict do nothing + SQL end def deactived_email_address diff --git a/test/controllers/rooms_controller_test.rb b/test/controllers/rooms_controller_test.rb index 824d884..e8ed51f 100644 --- a/test/controllers/rooms_controller_test.rb +++ b/test/controllers/rooms_controller_test.rb @@ -90,8 +90,9 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest test "destroy takes the room away from its members at once and leaves its messages to a job" do room = rooms(:designers) + member_ids = room.memberships.pluck(:user_id) - assert_enqueued_with(job: Room::DestroyJob, args: [ room ]) do + assert_enqueued_with(job: Room::DestroyJob, args: [ room, member_ids ]) do assert_no_difference -> { Message.count } do delete room_url(room) assert_redirected_to root_url diff --git a/test/models/room_test.rb b/test/models/room_test.rb index 1c425ac..09771f3 100644 --- a/test/models/room_test.rb +++ b/test/models/room_test.rb @@ -1,6 +1,8 @@ require "test_helper" class RoomTest < ActiveSupport::TestCase + include ActionCable::TestHelper + test "grant membership to user" do rooms(:watercooler).memberships.grant_to(users(:kevin)) assert rooms(:watercooler).users.include?(users(:kevin)) @@ -41,6 +43,19 @@ class RoomTest < ActiveSupport::TestCase assert_not Room.exists?(room.id) end + test "a room destroyed later resets the connections of its former members, as revoking their memberships would" do + room = rooms(:designers) + former_members = room.users.to_a + assert former_members.many? + + room.destroy_later + perform_enqueued_jobs only: Room::DestroyJob + + former_members.each do |member| + assert_broadcast_on "action_cable/#{member.to_gid_param}", { type: "disconnect", reconnect: true } + end + 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)) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index efeda3c..61bb1c3 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -12,6 +12,18 @@ class UserTest < ActiveSupport::TestCase end end + test "memberships granted to the open rooms are like any other" do + freeze_time + user = create_new_user + + assert_equal Rooms::Open.ids.sort, user.memberships.pluck(:room_id).sort + user.memberships.each do |membership| + assert membership.involved_in_mentions? + assert_equal Time.current, membership.created_at + assert_equal Time.current, membership.updated_at + end + end + test "deactivating a user deletes push subscriptions, searches, memberships for non-direct rooms, and changes their email address" do assert_difference -> { Membership.count }, -users(:david).memberships.without_direct_rooms.count do assert_difference -> { Push::Subscription.count }, -users(:david).push_subscriptions.count do