Skip to content

fix: Ensure correct placeholder retrieval for PageContent instances #8088

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
Merged
merged 5 commits into from
Dec 9, 2024
Merged

fix: Ensure correct placeholder retrieval for PageContent instances #8088

merged 5 commits into from
Dec 9, 2024

Conversation

fsbraun
Copy link
Member

@fsbraun fsbraun commented Nov 29, 2024 •
edited by sourcery-ai bot
Loading

Description

Fix placeholder inheritance issue by ensuring correct retrieval of placeholders when the toolbar object is a PageContent instance.

After confirming that the fix works, a regression test needs to be added.

Fixes #8087

Related resources

  • #...
  • #...

Checklist

  • I have opened this pull request against develop-4
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined the channel #pr-reviews on our Discord Server to find a “pr review buddy” who is going to review my pull request.

Summary by Sourcery

Fix placeholder retrieval for PageContent instances and add a regression test to ensure the fix works. Enhance caching by replacing lru_cache with cache in several functions and update linting rules in pyproject.toml.

Bug Fixes:

  • Fix placeholder inheritance issue by ensuring correct retrieval of placeholders when the toolbar object is a PageContent instance.

Enhancements:

  • Replace lru_cache with cache for improved caching in various functions.

Tests:

  • Add a regression test to verify the correct retrieval of placeholders using the toolbar.

Chores:

  • Update pyproject.toml to ignore specific linting rules related to typing annotations.

@sourcery-ai Sourcery AI
Copy link
Contributor

sourcery-ai bot commented Nov 29, 2024 •
edited
Loading

Reviewer's Guide by Sourcery

This PR fixes a placeholder inheritance issue by improving how placeholders are retrieved when dealing with PageContent instances. The implementation modifies the content object retrieval logic and includes a regression test. Additionally, some caching decorators are updated from lru_cache to cache, and ruff linting configurations are adjusted.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Improved placeholder retrieval logic for PageContent instances
  • Added type checking for PageContent instances when comparing toolbar object
  • Simplified placeholder retrieval by directly using toolbar object
  • Added null check for page_content before rescanning placeholders
  • Added regression test to verify correct placeholder retrieval
 cms/plugin_rendering.py
cms/tests/test_rendering.py
Updated caching decorator usage across multiple files
  • Replaced @lru_cache(maxsize=None) with @cache decorator
  • Updated caching for cms extension apps retrieval
  • Updated caching for cms config apps retrieval
  • Updated caching for menu renderer retrieval
  • Updated caching for plugin class retrieval
 cms/app_registration.py
cms/context_processors.py
cms/utils/plugins.py
Updated ruff linting configuration
  • Added ignore rules for typing List related warnings
  • Added UP006 and UP035 to extend-ignore list
 pyproject.toml

Assessment against linked issues

Issue Objective Addressed Explanation
#8087 Fix the AttributeError when previewing pages from other sites in Django-Admin where 'EmptyPageContent' object has no attribute '_state' ✅

Possibly linked issues

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@fsbraun fsbraun mentioned this pull request Nov 29, 2024
Closed
2 tasks
@django-cms django-cms deleted a comment from github-actions bot Nov 29, 2024
@django-cms django-cms deleted a comment from github-actions bot Nov 29, 2024
@django-cms django-cms deleted a comment from github-actions bot Nov 29, 2024
@fsbraun
Copy link
Member Author

fsbraun commented Nov 29, 2024

@MacLake You did mention an issue with placeholder inheritance. Would this PR help?

@fsbraun fsbraun changed the title fix: Failed placeholder inheritance @sourcery-ai Nov 29, 2024
@sourcery-ai sourcery-ai bot changed the title @sourcery-ai fix: Ensure correct placeholder retrieval for PageContent instances Nov 29, 2024
@fsbraun fsbraun mentioned this pull request Nov 29, 2024
Closed
2 tasks
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 9, 2024

👋 Hi there!

Please remember to MERGE COMMIT pull requests from develop or develop-4!

Do not SQUASH commits to preserve history for the changelog.

@fsbraun fsbraun requested a review from a team December 9, 2024 12:16
@fsbraun fsbraun marked this pull request as ready for review December 9, 2024 12:16
sourcery-ai[bot]
sourcery-ai bot reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @fsbraun - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please split the cache decorator changes (@lru_cache -> @cache) into a separate PR with proper documentation of the rationale for this change. The placeholder fix should stand on its own.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 9, 2024

👋 Hi there!

Please remember to MERGE COMMIT pull requests from develop or develop-4!

Do not SQUASH commits to preserve history for the changelog.

@fsbraun fsbraun mentioned this pull request Dec 9, 2024
Merged
4 tasks
vinitkumar
vinitkumar requested changes Dec 9, 2024
View reviewed changes
@fsbraun
Copy link
Member Author

fsbraun commented Dec 9, 2024

@vinitkumar The changes in cache and lru_cache are to satisfy the ruff linter.

@vinitkumar vinitkumar merged commit f0f3171 into release/4.1.x Dec 9, 2024
92 checks passed
@fsbraun fsbraun deleted the fix/backport-8087 branch December 9, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sourcery-ai sourcery-ai[bot] sourcery-ai[bot] left review comments

@vinitkumar vinitkumar vinitkumar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fsbraun @vinitkumar