From 08cb9ecca4a36216097506e9d594544db7496b9b Mon Sep 17 00:00:00 2001
From: Roman Rizzi <rizziromanalejandro@gmail.com>
Date: Tue, 13 Sep 2022 12:43:24 -0300
Subject: [PATCH] FIX: Don't delete previous messages when we're inside the
`sent_recently` window. (#18239)
`delete_previous!` deletes existing topics even when we cannot send a new one due to the `limit_once_per` option. The dashboard problems PM gets deleted the next time the job runs (30 minutes), so the inbox could be empty when
admins click on the summary notification.
---
app/services/group_message.rb | 30 +++++++++++++++---------------
spec/jobs/dashboard_stats_spec.rb | 8 ++++++++
2 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/app/services/group_message.rb b/app/services/group_message.rb
index 4ffac25e128..1f24289434a 100644
--- a/app/services/group_message.rb
+++ b/app/services/group_message.rb
@@ -27,23 +27,23 @@ class GroupMessage
end
def create
- unless sent_recently?
- post = PostCreator.create(
- Discourse.system_user,
- target_group_names: [@group_name],
- archetype: Archetype.private_message,
- subtype: TopicSubtype.system_message,
- title: I18n.t("system_messages.#{@message_type}.subject_template", message_params),
- raw: I18n.t("system_messages.#{@message_type}.text_body_template", message_params)
- )
- remember_message_sent
- post
- else
- false
- end
+ return false if sent_recently?
+
+ post = PostCreator.create(
+ Discourse.system_user,
+ target_group_names: [@group_name],
+ archetype: Archetype.private_message,
+ subtype: TopicSubtype.system_message,
+ title: I18n.t("system_messages.#{@message_type}.subject_template", message_params),
+ raw: I18n.t("system_messages.#{@message_type}.text_body_template", message_params)
+ )
+ remember_message_sent
+ post
end
- def delete_previous!(match_raw: true)
+ def delete_previous!(respect_sent_recently: true, match_raw: true)
+ return false if respect_sent_recently && sent_recently?
+
posts = Post
.joins(topic: { topic_allowed_groups: :group })
.where(topic: {
diff --git a/spec/jobs/dashboard_stats_spec.rb b/spec/jobs/dashboard_stats_spec.rb
index 02682958d1f..3f2304db9cb 100644
--- a/spec/jobs/dashboard_stats_spec.rb
+++ b/spec/jobs/dashboard_stats_spec.rb
@@ -4,6 +4,7 @@ RSpec.describe ::Jobs::DashboardStats do
let(:group_message) { GroupMessage.new(Group[:admins].name, :dashboard_problems, limit_once_per: 7.days.to_i) }
def clear_recently_sent!
+ # We won't immediately create new PMs due to the limit_once_per option, reset the value for testing purposes.
Discourse.redis.del(group_message.sent_recently_key)
end
@@ -31,6 +32,13 @@ RSpec.describe ::Jobs::DashboardStats do
expect(new_topic.title).to eq(old_topic.title)
end
+ it 'respects the sent_recently? check when deleting previous message' do
+ Discourse.redis.setex(AdminDashboardData.problems_started_key, 14.days.to_i, 3.days.ago)
+ expect { described_class.new.execute({}) }.to change { Topic.count }.by(1)
+
+ expect { described_class.new.execute({}) }.not_to change { Topic.count }
+ end
+
it 'duplicates message if previous one has replies' do
Discourse.redis.setex(AdminDashboardData.problems_started_key, 14.days.to_i, 3.days.ago)
expect { described_class.new.execute({}) }.to change { Topic.count }.by(1)