FIX: Handle edge cases for group SMTP email job (PR #13631)

Skip group SMTP email (and add log) if:

  • topic is deleted
  • post is deleted
  • smtp has been disabled for the group

Skip without log if:

  • enable_smtp site setting is false
  • disable_emails site setting is yes

GitHub

It looks like we only need to fetch this record if certain conditions have been met. I think we should extract this to a method so that the DB query doesn’t have to be executed upfront. Also we can memoize this in case it is called more than once.

A bit of code duplication happening here which is a sign that we can extract stuff into a method.

This will create the Topic AR object which is a little wasteful considering that we don’t use that object anywhere else in this method.

      if Topic.exists?(id: post.topic_id)

How about something like this?

Something doesn’t seem right here, the integers should be different between the keys.

The assertion here is pretty generic since any email being sent in the process will result in this test passing. Is there a simple assertion that we test the email we expect is sent? Maybe a test for the subject or template?

Damn it, silly copy paste mistake, sorry.

Yes I can easily add this

      group.update!(smtp_enabled: false)

Curious, what if the group is deleted? :slight_smile:

I think if the group is deleted we can just return? No point logging it haha

Yeah will just return in that case I think that’s fine. Rare for a group to be deleted…and if it is how is the job even enqueued haha?

Super race condition. Group is deleted before the sidekiq job has executed.

I didn’t extract this because it’s used in the skip logs, but I think I made it better by making a different scope on User to find by the primary email so we aren’t loading the UserEmail record just to get the User here