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

Cache not invalidated when using a PlaceholderField outside the CMS #6912 #6956

Merged

Conversation

pierreben
Copy link

@pierreben pierreben commented Dec 3, 2020

Description

Since v3.6.0, when using a PlaceholderField outside the CMS, the cache is not invalidated when updating the placeholder content.
On this commit, the value of the clear_cache attribute changed from True to False, and the clear_cache method is never called in this case.

I've created a new pull request as the previous was invalid and added tests.

Related resources

Steps to reproduce

  1. Create a custom model containing a PlaceholderField
class MyModel(models.Model):
    body_placeholder = PlaceholderField("Body")
  1. Activate CACHE on the CMS
CMS_CACHE_DURATIONS = {
    "menus": 3600,
    "content": 3600,
    "permissions": 3600
}
  1. Create a MyModel instance, a view and template to display it

  2. Edit the my_model_object placeholder field content within a template using the render_placeholder template tag:

{% render_placeholder my_model_object.body_placeholder %}

  1. Open the MyModel instance view in a private session to compare the content.

  2. Repeat steps 4 and 5 twice if the content doesn't differs.

Expected behaviour

The contents are always the same between the two browser sessions.

Actual behaviour

The contents differs as the cache is not invalidated when updating the body_placeholder content.

Suggested correction

In the mark_as_dirty method, force clear_cache when the attached model is not a Page or a StaticPlaceholder.

Checklist

  • I have opened this pull request against develop
  • I have updated the CHANGELOG.rst
  • I have added or modified the tests when changing logic

Benjamin PIERRE added 3 commits December 2, 2020 18:35
Force placeholder cache clearing when updating a PlaceholderField content
Add a test to check that the mark_as_dirty method of placeholder will clear placeholder cache for custom model having a Placeholderfield and edited from the cms.admin.placeholderadmin methods
@pierreben
Copy link
Author

pierreben commented Jan 7, 2021

Any news on this issue ?

It affects all ours projects using djangocms-blog as described in this related issue :(

Aiky30
Aiky30 previously requested changes Jan 22, 2021
Copy link
Contributor

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

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

@pierreben My apologies that it has taken this long to review your change. This looks good and I'm grateful that you have also provided a test.

Can you please add a changelog entry into the changelog, this ensures that we mention your fix in any official releases.

Once this has a changelog entry int he unreleased section of the changelog I will approve this change, once approved we can start to look at getting it merged and released.

Again thank you for your effort regarding this fix.

@pierreben
Copy link
Author

@Aiky30 , thank you for the fix analysis.

I've updated the changelog ;)

@palmitoto
Copy link

Why this MR is not yet merged ? Django-cms is not working without that patch.

A solution is provided since December 2020, it's too long to patch a critical issue like that.

@NicolaiRidani
Copy link
Sponsor

Why this MR is not yet merged ? Django-cms is not working without that patch.

A solution is provided since December 2020, it's too long to patch a critical issue like that.

Hi @palmitoto I can understand your annoyance about the delay. Unfortunately, we are not in a good position at the moment in terms of resources, plus there is a problem with Travis that we are working on solving. But rest assured that we will try to resolve this quickly.

With the founding of the django CMS Association, we have started an initiative that aims to foster contribution. Among many other community groups, we also have a work group for reviewing pull requests and a work group that takes care of release management. Both teams are happy to receive any support. We invite everyone to join us!

If you want to learn more, feel free to contact me, otherwise more information can be found here:

You are also welcome to join our Slack channel: www.django-cms.org/slack to get in touch with the team.

Best
Nicolai

@greengoaxe greengoaxe added this to Ready to be merged in Issue / PR work group Apr 21, 2021
@greengoaxe
Copy link

Hi @Aiky30,
may you review and approved it?
Thanks,
greengoaxe

@greengoaxe greengoaxe moved this from Ready to be merged to Ready to be reviewed in Issue / PR work group Apr 28, 2021
@goutnet
Copy link
Contributor

goutnet commented May 18, 2021

@Aiky30 since we are trying to make the Changelog happen more automatically, and based on your comments, I am updating the branch and schedule this one for merge.

@goutnet goutnet dismissed Aiky30’s stale review May 18, 2021 16:41

will add the changelog entry directly from squash commit

Issue / PR work group automation moved this from Ready to be reviewed to Ready to be merged May 18, 2021
@goutnet goutnet merged commit 3ce63d7 into django-cms:develop May 19, 2021
Issue / PR work group automation moved this from Ready to be merged to Done May 19, 2021
@goutnet
Copy link
Contributor

goutnet commented May 19, 2021

merged! and right in time for 3.9.0!

@goutnet goutnet added this to the 3.9.0 milestone May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

8 participants