-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: Fix ApphookReloadMiddleware not handling new language variants #8362
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
base: main
Are you sure you want to change the base?
fix: Fix ApphookReloadMiddleware not handling new language variants #8362
Conversation
- Add signal handler for PageContent creation to trigger URL reloading - Only triggers for pages with apphooks to minimize performance impact - Prevents NoReverseMatch exceptions when creating language variants - Add comprehensive test suite with 5 test cases - Maintains backward compatibility Fixes django-cms#8357
Reviewer's GuideThis PR extends the ApphookReloadMiddleware mechanism by adding a post_save signal handler for PageContent creations on apphook pages, ensuring URL patterns are reloaded immediately when new language variants are added and preventing NoReverseMatch errors, while maintaining backward compatibility and minimal performance impact. Sequence diagram for PageContent creation triggering URL reloadsequenceDiagram
participant D as "Django ORM"
participant S as "Signal Handler (handle_pagecontent_post_save)"
participant P as "PageContent Instance"
participant M as "ApphookReloadMiddleware"
D->>S: post_save(PageContent, created=True)
S->>P: Check if page has apphook
alt Page has apphook
S->>M: set_restart_trigger()
end
Class diagram for new PageContent signal handler integrationclassDiagram
class PageContent {
+page: Page
+language: str
}
class handle_pagecontent_post_save {
+post_save(sender, instance, created, **kwargs)
}
PageContent <.. handle_pagecontent_post_save : post_save signal
class ApphookReloadMiddleware {
+set_restart_trigger()
}
handle_pagecontent_post_save --> ApphookReloadMiddleware : calls set_restart_trigger()
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `cms/tests/test_apphook_reload_language_variants.py:24-25` </location>
<code_context>
+ if m != 'cms.middleware.utils.ApphookReloadMiddleware'
+ ]
+)
+class ApphookReloadLanguageVariantTests(CMSTestCase):
+ """Test that URL reloading is triggered when creating language variants for apphook pages."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Suggestion to add a test for simultaneous creation of multiple language variants.
Add a test that creates multiple language variants in rapid succession for the same apphook page to verify URL reloading handles concurrent updates without race conditions.
```suggestion
class ApphookReloadLanguageVariantTests(CMSTestCase):
"""Test that URL reloading is triggered when creating language variants for apphook pages."""
def test_simultaneous_creation_of_multiple_language_variants(self):
"""
Test that creating multiple language variants in rapid succession for the same apphook page
triggers URL reloading correctly and does not cause race conditions.
"""
from django.utils import translation
# Create the initial apphook page in the default language
page = create_page(
title="Apphook Page",
template="INHERIT",
language="en",
apphook=SampleApp.__name__,
published=True,
)
# List of languages to add as variants
languages = ["fr", "de", "it", "es"]
# Patch UrlconfRevision to monitor reloads
with patch("cms.middleware.utils.ApphookReloadMiddleware.reload_urlconf") as mock_reload:
# Create language variants in rapid succession
for lang in languages:
with translation.override(lang):
create_page_content(
language=lang,
page=page,
title=f"Apphook Page {lang}",
)
# Assert reload_urlconf was called for each new variant
self.assertEqual(mock_reload.call_count, len(languages))
# Optionally, check that no exceptions were raised and all variants exist
for lang in languages:
self.assertTrue(page.title_set.filter(language=lang).exists())
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@Aaditya1273 Could you please run |
| from cms.models import UrlconfRevision | ||
| from cms.test_utils.testcases import CMSTestCase | ||
| from cms.test_utils.project.sampleapp.cms_apps import SampleApp | ||
| from cms.utils.setup import apphooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This import is causing all the tests to fail.
fix: ApphookReloadMiddleware not handling new languages causing reverse exception
Fixes #8357
Description
This PR fixes issue #8357 where creating new language variants for pages with apphooks causes
django.urls.exceptions.NoReverseMatchexceptions until the server is restarted.Problem
When a page with an apphook exists in one language and you create a new language variant, the URL patterns are not reloaded. This causes
NoReverseMatchexceptions when trying to reverse URLs for the apphook in the new language, even after creating the language variant for the apphook page itself.Root Cause
The
ApphookReloadMiddlewareonly triggers URL reloading when:application_urlsfield)But it does NOT trigger when new language variants are created for pages with apphooks.
Solution
Added a signal handler that triggers URL reloading when new
PageContentobjects (language variants) are created for pages that have apphooks configured.Changes Made
cms/signals/pagecontent.py(NEW) - Addedpost_savesignal handler forPageContentmodel that triggersset_restart_trigger()when new language variants are created for apphook pagescms/signals/__init__.py(MODIFIED) - Added import to ensure the new signal handler is connectedcms/tests/test_apphook_reload_language_variants.py(NEW) - Comprehensive test suite with 5 test cases covering all scenariosKey Features
set_restart_trigger()mechanismExample
Before this fix: