Skip to content

Fix cells caching by using cache_key_with_version instead of cache version#7532

Merged
oriolgual merged 6 commits intodecidim:developfrom
armandfardeau:fix/cache-version
Mar 5, 2021
Merged

Fix cells caching by using cache_key_with_version instead of cache version#7532
oriolgual merged 6 commits intodecidim:developfrom
armandfardeau:fix/cache-version

Conversation

@armandfardeau
Copy link
Copy Markdown
Contributor

@armandfardeau armandfardeau commented Mar 3, 2021 •
edited
Loading

🎩 What? Why?

Fix nil value in cache hash when ActiveRecord::Base.cache_versioning is set to false.

📌 Related Issues

Testing

Not sure how this should be tested right now.

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • ❓ CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • ❌AVOID breaking the continuous integration build.
  • ❌AVOID making significant changes to the overall architecture.

@armandfardeau armandfardeau marked this pull request as ready for review March 3, 2021 23:40
@ahukkanen
Copy link
Copy Markdown
Contributor

ahukkanen commented Mar 4, 2021

Not sure how this should be tested right now.

I think a rather easy ways to add some testing for it would be to create e.g. 20 proposal records and add a test for the Decidim::Proposals::ProposalMCell#cache_hash for each of these records to ensure there are no duplicates between the 20 recrods.

If you want to test the case in question (running under Rails 5.1 defaults), you could run the same test by temporarily disabling cache versioning from the proposals by stubbing the method allow(Decidim::Proposals::Proposal).to receive(:cache_versioning).and_return(false).

We recently bumped into another similar issue with different Rails 5.1 defaults which was fixed and tested using a similar method (#7488).

As per how to test this in an actual use case, just merge and backport this fix, deploy it to Metadecidim and see if the problem disappears that there currently is. 😄

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 4, 2021

Not sure how this should be tested right now.

I think a rather easy ways to add some testing for it would be to create e.g. 20 proposal records and add a test for the Decidim::Proposals::ProposalMCell#cache_hash for each of these records to ensure there are no duplicates between the 20 recrods.

If you want to test the case in question (running under Rails 5.1 defaults), you could run the same test by temporarily disabling cache versioning from the proposals by stubbing the method allow(Decidim::Proposals::Proposal).to receive(:cache_versioning).and_return(false).

We recently bumped into another similar issue with different Rails 5.1 defaults which was fixed and tested using a similar method (#7488).

As per how to test this in an actual use case, just merge and backport this fix, deploy it to Metadecidim and see if the problem disappears that there currently is. 😄

Thanks, I will add the test asap

@armandfardeau armandfardeau marked this pull request as draft March 4, 2021 10:41
@armandfardeau armandfardeau marked this pull request as ready for review March 4, 2021 14:34
@armandfardeau armandfardeau marked this pull request as draft March 4, 2021 14:34
@armandfardeau armandfardeau marked this pull request as ready for review March 4, 2021 14:37
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 4, 2021

@armandfardeau tests don't seem to be getting enqueued, could you try rebasing the PR to latest develop? If this doesn't work we might need to revert #7534 😞

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 4, 2021

@armandfardeau tests don't seem to be getting enqueued, could you try rebasing the PR to latest develop? If this doesn't work we might need to revert #7534 😞

I fear that's some tweaking is needed.

@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 5, 2021

@armandfardeau could you update this branch with develop? I've reverted #7534 so that the CI is back on, sorry for the issues 🙏

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 5, 2021

@armandfardeau could you update this branch with develop? I've reverted #7534 so that the CI is back on, sorry for the issues 🙏

Done 😄

@oriolgual oriolgual merged commit db502f5 into decidim:develop Mar 5, 2021
entantoencuanto added a commit that referenced this pull request Mar 5, 2021
@entantoencuanto
Merge branch 'develop' into chore/rails-6-upgrade
3530ec4 
* develop:
  Fix infinite loop when impersonated session time runs out (#7221)
  New Crowdin updates (#7543)
  Migrate Admin menus to Menu Registry Part 2 (#7382)
  Replace xls with xlsx (#7421)
  Use cache_key_with_version instead of cache version (#7532)
  Add support for ElectionGuard voting scheme (#7454)
  Fix record encryptor trying to decrypt empty strings (#7542)
  Revert "Don't schedule CI jobs for locales PRs (#7534)" (#7546)
  New Crowdin updates (#7540)
  New Crowdin updates (#7539)
@mrcasals mrcasals added the type: fix PRs that implement a fix for a bug label Mar 8, 2021
@mrcasals mrcasals changed the title Use cache_key_with_version instead of cache version Fix proposal card caching by using cache_key_with_version instead of cache version Mar 8, 2021
@mrcasals mrcasals changed the title Fix proposal card caching by using cache_key_with_version instead of cache version Fix cells caching by using cache_key_with_version instead of cache version Mar 8, 2021
@armandfardeau armandfardeau deleted the fix/cache-version branch March 8, 2021 09:17
mrcasals pushed a commit that referenced this pull request Mar 8, 2021
@armandfardeau @mrcasals
Use cache_key_with_version instead of cache version (#7532)
8f456ca 
* Use cache_key_with_version instead of cache version

* Use cache_key_with_version on last activity cell

* Test when multiple proposals are cached

* Fix typo in test name
This was referenced Mar 8, 2021
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@oriolgual oriolgual oriolgual approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: core module: proposals type: fix PRs that implement a fix for a bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Proposals listing caching has broken the listing and searching under Rails 5.1 default configurations

4 participants

@armandfardeau @ahukkanen @mrcasals @oriolgual