Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move BadgeAchievements::SendEmailNotificationWorker to Sidekiq #5779

Merged
merged 5 commits into from
Jan 31, 2020
Merged

Move BadgeAchievements::SendEmailNotificationWorker to Sidekiq #5779

merged 5 commits into from
Jan 31, 2020

Conversation

atsmith813
Copy link
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Optimization

Description

This creates a new BadgeAchievements::SendEmailNotificationWorker (spec included) and starts sending jobs to it. Once this is merged and deployed, I will open a second PR to remove the old job code and spec.

Related Tickets & Documents

Added to documentation?

  • no documentation needed

moving_gif

@atsmith813 atsmith813 requested a review from a team January 28, 2020 00:02
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 28, 2020
citizen428
citizen428 previously approved these changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@citizen428 citizen428 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good @atsmith813, just left a general comment.

app/workers/badge_achievements/send_email_notification_worker.rb Show resolved Hide resolved
@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 28, 2020
rhymes
rhymes previously approved these changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @atsmith813!

Your call if you want to incorporate this suggestion: https://github.com/thepracticaldev/dev.to/pull/5779/files#r371597928

mstruve
mstruve suggested changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@mstruve mstruve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to update the callback otherwise this looks good!

app/models/badge_achievement.rb
@@ -32,7 +32,7 @@ def notify_recipient
def send_email_notification
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is called in an after_create callback, we need to update that to be after_create_commit to ensure the record is in the db before we attempt to send a notification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @mstruve here. Although, it seems to me that this was a preexisting issue since we were enqueuing this with perform_later before the call to save from the after_create had actually executed 😬

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaidehijoshi it was not an issue with DelayedJob bc the jobs were so delayed that by the time they ran the records were always committed to the db which is why we have to now switch a bunch of these now bc Sidekiq is way faster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol, what a happy (and slow) coincidence! Thanks for clarifying 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vaidehijoshi funnily enough we never encountered this issue because Delayed Job was too slow at picking up jobs. A concurrency error that wasn't because the client was too slow to trigger it :D

Copy link
Contributor Author

@atsmith813 atsmith813 Jan 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's entirely my bad, asleep at the wheel there 🙈Thanks for catching that!

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 28, 2020
@atsmith813 atsmith813 dismissed stale reviews from rhymes and citizen428 via eb428ad January 29, 2020 15:59
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Jan 29, 2020
rhymes
rhymes approved these changes Jan 29, 2020
View reviewed changes
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @atsmith813, looks great and thanks @mstruve for being on top of after_create callbacks :D

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 29, 2020
@rhymes rhymes requested a review from mstruve January 29, 2020 16:17
@mstruve mstruve merged commit 783da4b into forem:master Jan 31, 2020
@pr-triage pr-triage bot removed the PR: reviewed-approved bot applied label for PR's where reviewer approves changes label Jan 31, 2020
@pr-triage pr-triage bot added the PR: merged bot applied label for PR's that are merged label Jan 31, 2020
@weekly-digest weekly-digest bot mentioned this pull request Feb 3, 2020
Closed
@atsmith813 atsmith813 mentioned this pull request Feb 3, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhymes rhymes rhymes approved these changes

@mstruve mstruve mstruve approved these changes

@citizen428 citizen428 citizen428 left review comments

@jacobherrington jacobherrington Awaiting requested review from jacobherrington

@Zhao-Andy Zhao-Andy Awaiting requested review from Zhao-Andy

@vaidehijoshi vaidehijoshi Awaiting requested review from vaidehijoshi

Assignees
No one assigned
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@atsmith813 @citizen428 @rhymes @mstruve @vaidehijoshi