From 3186f726af6dea33b1a793451f767c2c817c9db0 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Wed, 24 Aug 2022 09:04:28 +0200 Subject: [PATCH 1/2] FIX: deletes trashed messages when hitting retention limit Prior to this fix, we would keep trashed messages in the DB forever. --- .../scheduled/delete_old_chat_messages.rb | 2 + spec/jobs/delete_old_chat_messages_spec.rb | 38 +++++++++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/app/jobs/scheduled/delete_old_chat_messages.rb b/app/jobs/scheduled/delete_old_chat_messages.rb index 025c9d2f1..5b265fe74 100644 --- a/app/jobs/scheduled/delete_old_chat_messages.rb +++ b/app/jobs/scheduled/delete_old_chat_messages.rb @@ -14,6 +14,7 @@ def delete_public_channel_messages ChatMessage .in_public_channel + .with_deleted .created_before(SiteSetting.chat_channel_retention_days.days.ago) .in_batches(of: 200) .each do |relation| @@ -27,6 +28,7 @@ def delete_dm_channel_messages ChatMessage .in_dm_channel + .with_deleted .created_before(SiteSetting.chat_dm_retention_days.days.ago) .in_batches(of: 200) .each do |relation| diff --git a/spec/jobs/delete_old_chat_messages_spec.rb b/spec/jobs/delete_old_chat_messages_spec.rb index 4c1ac2f6e..5698e5b14 100644 --- a/spec/jobs/delete_old_chat_messages_spec.rb +++ b/spec/jobs/delete_old_chat_messages_spec.rb @@ -33,6 +33,14 @@ created_at: base_date - 30.days - 1.second, ) end + fab!(:public_trashed_days_old_30) do + Fabricate( + :chat_message, + chat_channel: public_channel, + message: "hi", + created_at: base_date - 30.days - 1.second, + ) + end fab!(:dm_channel) do Fabricate( @@ -67,6 +75,14 @@ created_at: base_date - 30.days - 1.second, ) end + fab!(:dm_trashed_days_old_30) do + Fabricate( + :chat_message, + chat_channel: dm_channel, + message: "hi", + created_at: base_date - 30.days - 1.second, + ) + end before { freeze_time(base_date) } @@ -83,8 +99,15 @@ described_class.new.execute expect(public_days_old_0.deleted_at).to be_nil expect(public_days_old_10.deleted_at).to be_nil - expect { public_days_old_20 }.to raise_exception(ActiveRecord::RecordNotFound) - expect { public_days_old_30 }.to raise_exception(ActiveRecord::RecordNotFound) + expect { public_days_old_20.reload }.to raise_exception(ActiveRecord::RecordNotFound) + expect { public_days_old_30.reload }.to raise_exception(ActiveRecord::RecordNotFound) + end + + it "deletes trashed messages correctly" do + SiteSetting.chat_channel_retention_days = 20 + public_trashed_days_old_30.trash! + described_class.new.execute + expect { public_trashed_days_old_30.reload }.to raise_exception(ActiveRecord::RecordNotFound) end it "does nothing when no messages fall in the time range" do @@ -115,8 +138,15 @@ described_class.new.execute expect(dm_days_old_0.deleted_at).to be_nil expect(dm_days_old_10.deleted_at).to be_nil - expect { dm_days_old_20 }.to raise_exception(ActiveRecord::RecordNotFound) - expect { dm_days_old_30 }.to raise_exception(ActiveRecord::RecordNotFound) + expect { dm_days_old_20.reload }.to raise_exception(ActiveRecord::RecordNotFound) + expect { dm_days_old_30.reload }.to raise_exception(ActiveRecord::RecordNotFound) + end + + it "deletes trashed messages correctly" do + SiteSetting.chat_dm_retention_days = 20 + dm_trashed_days_old_30.trash! + described_class.new.execute + expect { dm_trashed_days_old_30.reload }.to raise_exception(ActiveRecord::RecordNotFound) end it "does nothing when no messages fall in the time range" do From b33637c81cdab38cdebf6fafcf4ec4e016e42770 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Wed, 24 Aug 2022 09:13:05 +0200 Subject: [PATCH 2/2] not necessary --- spec/jobs/delete_old_chat_messages_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/jobs/delete_old_chat_messages_spec.rb b/spec/jobs/delete_old_chat_messages_spec.rb index 5698e5b14..6622d11ce 100644 --- a/spec/jobs/delete_old_chat_messages_spec.rb +++ b/spec/jobs/delete_old_chat_messages_spec.rb @@ -99,8 +99,8 @@ described_class.new.execute expect(public_days_old_0.deleted_at).to be_nil expect(public_days_old_10.deleted_at).to be_nil - expect { public_days_old_20.reload }.to raise_exception(ActiveRecord::RecordNotFound) - expect { public_days_old_30.reload }.to raise_exception(ActiveRecord::RecordNotFound) + expect { public_days_old_20 }.to raise_exception(ActiveRecord::RecordNotFound) + expect { public_days_old_30 }.to raise_exception(ActiveRecord::RecordNotFound) end it "deletes trashed messages correctly" do @@ -138,8 +138,8 @@ described_class.new.execute expect(dm_days_old_0.deleted_at).to be_nil expect(dm_days_old_10.deleted_at).to be_nil - expect { dm_days_old_20.reload }.to raise_exception(ActiveRecord::RecordNotFound) - expect { dm_days_old_30.reload }.to raise_exception(ActiveRecord::RecordNotFound) + expect { dm_days_old_20 }.to raise_exception(ActiveRecord::RecordNotFound) + expect { dm_days_old_30 }.to raise_exception(ActiveRecord::RecordNotFound) end it "deletes trashed messages correctly" do