No email notifications for posts that require approval

Continuing the discussion from Get notified of new topics, but not posts:

We’re having issues with only some new topics generating emails to people watching a particular category.

Thanks @eviltrout for having a look at this initially. After some additional debugging it seems the issue is not related to to “Watching first post” users only, but also users that are “Watching” the category. However, it seems to be working for mailing list mode users, as well as for people watching that particular thread.

I think the issue is related to posts ending up in the moderation queue. We currently moderate the first 5 posts a new users submits (approve post count = 5), and previously we also had “approve unless trust level = 2”, but turned that off now for debugging:

I would expect that once a post in the moderation queue is approved, that it would trigger the same email notifications as if it was posted without being subject to moderation. But that does not seem to be the case.

Currently:

  • If a post is not subject to moderation: Triggers all expected email notifications (mailing list, watching category, watching first post, watching topic)
  • If a post is subject to moderation: After being approved, only trigger some email notifications: (mailing list, watching topic)

I assume this is a bug?

Are you 100% certain of this?

I just spent a while trying to reproduce this but couldn’t.

Approving queued posts goes through our PostCreator object and in tests I was able to confirm approving a post entered the code path for watching the first post.

2 Likes

Are you sure those users are not active on the website? Discourse will not email someone who is currently browsing the website, because we would be double notifying them.

Yes, I’m 100% sure they are not all active (we’re talking hundreds or thousands of users per category). I’ve also checked the box for a couple users that they’ll receive emails even when active on the site—still nothing (except when the poster is TL2+).

We’re running v1.6.0.beta12 +64, if that matters. I’ll ask one of our devs to chime in here.

I looked into the code and after some debugging, found out that PostAlert job skipped running the
PostAlerter.post_created(post) if post && post.topic line as the post was not yet commited to database due to QueuedPost wrapping the whole thing into a transaction.

I’ll be delaying the jobs by a few seconds as a workaround, but I think that is not a real fix to this or is it?

3 Likes

Does the above help you repro @eviltrout?

This is almost certainly the cause, thanks! We’ve seen this before where transactions aren’t complete before sidekiq processes the jobs (because it’s too fast).

Delaying by a few seconds is a bit of a dangerous fix as if the database is slow or if there is another delay it will continue to break.

It seems PostCreator is not transaction safe. Perhaps we should adjust it so that it only performs the enqueing after the post/topic is committed?

2 Likes

I briefly looked into what was involved with PostCreator to detect whether it was in a transaction and to enqueue the jobs afterwards but it seemed like a nightmare due to Rails’ internals. I think long term there is a better way to structure the PostCreator object to prevent this, but the following fix should solve this particular case:

https://github.com/discourse/discourse/commit/2cb4cb7b7286f8bcfc9da094b35c0aedf7d7ce0a

4 Likes

Can you add that to the large comment at the top of post_creator.rb?

skip_jobs: This is required to be true if you’re running PostCreator in a transaction. Call #enqueue_jobs after the transaction finishes.

2 Likes

Good call:

https://github.com/discourse/discourse/commit/79245a25a38ab556eeb0c36ea25faa48947cc8d8

Obligatory “Code Comments” meme:

4 Likes