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.

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

Refactor Podcasts Bust Cache job #5421

Merged
merged 5 commits into from
Jan 28, 2020

Conversation

maykonmenezes
Copy link
Contributor

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

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Migrated Podcasts::BustCacheJob to a side worker

Related Tickets & Documents

#5305

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed

[optional] What gif best describes this PR or how it makes you feel?

alt_text

@maykonmenezes maykonmenezes requested a review from a team January 9, 2020 18:32
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 9, 2020
mstruve
mstruve previously approved these changes Jan 9, 2020
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.

@maykonmenezes thanks for another solid PR!!!

@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 9, 2020
it "triggers cache busting on save" do
expect { build(:podcast).save }.to have_enqueued_job.on_queue("podcasts_bust_cache").once
podcast.save
expect(Podcasts::BustCacheWorker).to have_received(:perform_async).with(podcast.path)
Copy link
Contributor

Choose a reason for hiding this comment

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

just a note: this should probably check the job is in the queue with the sidekiq test helpers, instead of using mocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I will change it ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets do this spec like you did the one above and I think this will be ready to go!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 10, 2020
@atsmith813
Copy link
Contributor

@mstruve is this one safe to remove the old job and spec in the same PR? Looks like this is called in an after_save when on the Podcast model. I'm not sure how often we're actually saving podcasts 🤔

Otherwise, LGTM 💯

@mstruve
Copy link
Contributor

mstruve commented Jan 27, 2020

I believe this one is good bc

  • when we have a new record there will be no cache to bust because it is new.
  • If the path changes there is no cache to bust because the path is new and a cache is not set yet.
  • For every other change, the path will remain the same and thus its not a problem if the rest of the record is not updated in the database bc we only care about the path.

Is that logic sound? Did I miss any use cases?

@mstruve mstruve requested a review from rhymes January 27, 2020 22:07
Copy link
Contributor

@atsmith813 atsmith813 left a comment

Choose a reason for hiding this comment

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

It makes sense to me! Thanks, @mstruve and @maykonmenezes! 💯

@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 27, 2020
before do
allow(Pages::BustCacheWorker).to receive(:perform_async)
end
let(:page) { create(:page) }
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be a build(:page) ? the reason why this test passes is because create also runs save

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it matters bc that block below asserts that a new job is enqueued on save. Whether it's a new record or already existing one is inconsequential

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.

Good job @maykonmenezes!

@mstruve mstruve merged commit 3b3d2be into forem:master Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: reviewed-approved bot applied label for PR's where reviewer approves changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants