-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change ModerationNotificationJob to ModerationNotificationWorker and move to sidekiq #5683
Change ModerationNotificationJob to ModerationNotificationWorker and move to sidekiq #5683
Conversation
@@ -82,7 +82,7 @@ def send_moderation_notification(notifiable) | |||
# TODO: make this work for articles in the future. only works for comments right now | |||
return if UserBlock.blocking?(notifiable.commentable.user_id, notifiable.user_id) | |||
|
|||
Notifications::ModerationNotificationJob.perform_later(notifiable.id) | |||
Notifications::ModerationNotificationWorker.perform_async(notifiable.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is called from comment in an after_create
callback which means nothing has been committed to the database yet. We need to change that comment callback to be after_create_commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @mstruve. I'll send the changes. Thanks for reviewing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code seems good, you just need to fix spec/services/notifications/moderation/send_spec.rb:16
which is currently broken.
Thank you!
Sure @rhymes. I'll fix it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
#5305
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?
[optional] What gif best describes this PR or how it makes you feel?