FEATURE: remove duplicated messages about new advices (PR #14319)

Discourse is sending regularly message to admins when potential problems are persisted. Most of the time they have exactly the same content. In that case, when there are no replies, the old one should be trashed before a new one is created.

GitHub

This is reasonably safe but I wonder if we could harden it more:

  • Ensure the topic was created by system user
  • Include a maximum date range. It’s probably OK if they are warned again after a few months.
    posts = Post
      .joins(topic: { topic_allowed_groups: :group })
      .where(topic: {
        posts_count: 1,
        user_id: Discourse.system_user,
        archetype: Archetype.private_message,
        subtype: TopicSubtype.system_message,
        title: I18n.t("system_messages.#{@message_type}.subject_template", message_params),
        topic_allowed_groups: { 
          groups: { name: @group_name } 
        }
      })
      .where("posts.created_at > ?", 3.months.ago)
      .where(raw: I18n.t("system_messages.#{@message_type}.text_body_template", message_params).rstrip)

I kind of feel like this is much clearer as opposed to splitting up the where clause.

I think these need to be in the same transaction.

The fact that we’re clearing in a before block means we’re leaking state in the tests here. It’ll be better if we clean up after each test is done.

    expect(topic.reload.deleted_at.present?).to eq(true)

Should this be 3 months ago per the description of the test?

Let us move this into a constant so that it is clearer why 3 months.

I believe this should be deletes and not duplicated ?

I don’t think this assertion is sufficient, the count has changed but what we’re interested here is that the count decreased by one. Actually wouldn’t this be no change since we deleted a old topic and created a new one?

No, it was changed after Robin’s comment https://github.com/discourse/discourse/pull/14319#discussion_r707480652 “Include a maximum date range. It’s probably OK if they are warned again after a few months.”

So my understanding was that if last message was long time ago, we can duplicate

O in this case we’re missing a test for deleting the duplicated message.

Hm, no, this test is ensuring that we have 2 messages (3 months old and one recent). In that case, duplication is fine - we can have 2 messages

Another test is ensuring that if you have a message 2 weeks old, and new one, exactly same will be created - in that case, 2 weeks old should be deleted: https://github.com/discourse/discourse/pull/14319/files#diff-135d1a94ab5e9ffdc235d210cbcba2125fe5cff802cd9f0220daf81297cce350R24