fix: Allow for request-based site detection #56
Merged
+61
−37
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors site detection to be request-based by overhauling manager and queryset APIs to accept explicit Site parameters, updates corresponding tests, introduces a compatibility decorator for get_current_site, and revises views and CMS menus to leverage the new request-aware site filtering. Sequence diagram for request-based site detection in views and menussequenceDiagram
participant actor User
participant View
participant "get_current_site"
participant QuerySet
User->>View: Sends HTTP request
View->>"get_current_site": Passes request
"get_current_site"->>View: Returns Site
View->>QuerySet: Calls on_site(site)
QuerySet->>View: Returns filtered queryset
View->>User: Returns response
Class diagram for updated SiteQuerySet and manager methodsclassDiagram
class SiteQuerySet {
+on_site(site: Site) : SiteQuerySet
+filter_by_language(language, site: Site | None = None) : SiteQuerySet
}
class PostContentManager {
+filter_by_language(language, site: Site | None = None)
+on_site(site: Site | None = None)
+get_months(queryset=None, site: Site | None = None)
}
SiteQuerySet <|-- PostContentManager
class TagManager {
+tag_cloud(other_model=None, queryset=None, published: bool = True, site: Site | None = None)
}
Flow diagram for site_compatibility_decorator logicflowchart TD
A["site_compatibility_decorator(func)"] --> B["Try: func(None)"]
B --> C["TypeError? -> Return lambda request: func()"]
B --> D["ImproperlyConfigured? -> Return func"]
B --> E["Else: Return func"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The site_compatibility_decorator currently invokes the wrapped function at import time and doesn't preserve metadata—consider using functools.wraps and inspect.signature to detect the expected parameters instead of a trial call.
- Several manager methods still reference legacy flags in their docstrings; please update the documentation to reflect the new 'site' parameter and its behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The site_compatibility_decorator currently invokes the wrapped function at import time and doesn't preserve metadata—consider using functools.wraps and inspect.signature to detect the expected parameters instead of a trial call.
- Several manager methods still reference legacy flags in their docstrings; please update the documentation to reflect the new 'site' parameter and its behavior.
## Individual Comments
### Comment 1
<location> `tests/test_managers.py:233-238` </location>
<code_context>
"""Test get_months filters by current site"""
- months = Post.objects.get_months(current_site=True)
+ site = Site.objects.get_current()
+ months = Post.objects.get_months(site=site)
assert isinstance(months, list)
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions to verify that months are correctly filtered by site.
Add assertions to confirm that each month in the result matches posts from the specified site, ensuring the filter works as intended.
```suggestion
def test_get_months_with_current_site(self, page_with_menu, many_posts):
"""Test get_months filters by current site"""
site = Site.objects.get_current()
months = Post.objects.get_months(site=site)
assert isinstance(months, list)
# Assert that each month in the result matches posts from the specified site
for year, month in months:
posts_in_month = Post.objects.filter(
site=site,
publish_date__year=year,
publish_date__month=month
)
assert posts_in_month.exists()
for post in posts_in_month:
assert post.site == site
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 86.57% 86.67% +0.10%
==========================================
Files 23 23
Lines 2070 2071 +1
Branches 232 230 -2
==========================================
+ Hits 1792 1795 +3
+ Misses 186 183 -3
- Partials 92 93 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
(use `filter(language=...)` instead)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enable explicit site-based filtering throughout Story models and utilities by accepting a Site instance in manager methods and ensuring compatibility with various django CMS versions.
Enhancements:
Tests: