FIX: IMAP post alerter race condition and code improvements (PR #11711)

GitHub

This used to need to be > 1…but then it subtracted the group email username from the array straight after, leaving only 1. This group username removal is handled by post.topic.incoming_email_addresses now.

Maybe I miss something, but is there any reason to add group.email_username to email_addresses if you are going to remove it anyway a few lines down?

    email_addresses = Set.new

Can you combine these conditions with the ones below? I find it a bit weird that you check here for any? and then for none?

Maybe something like this? This way you also avoid combined_addresses, which seems to be an unnecessary computation.

      next if group.present? && (
        to_addresses.any? { |address| address =~ group.email_username_regex } ||
        cc_addresses.any? { |address| address =~ group.email_username_regex }
      )

Nitpick: You can make this a bit shorter.

    email_addresses.delete(group.email_username) if group.present?

This is a good point, I only did this because that is what was there originally, will remove.

What you suggested here is similar to the original code but I found the original code unclear. Also the check suggested here is wrong, we have to go next if none of the emails match the group username. I had to add .any? because I made it so the addresses would always be arrays and could not be nil. If you think the extra computation is unnecessary I will change this section back to the original, I can live with it :slight_smile:

Ah nice yeah the delete is better but won’t need it anyway if we don’t put that email in the set to begin with.

You can leave it as it is. I do not have a strong opinion about it. :slight_smile:

I think IncomingEmail records with from_address == group.email_username can exist, right? We create an IncomingEmail record to avoid double sync of emails sent using SMTP.

Oh right of course, nvm :sweat_smile: