feat: Add menu endpoints #49
Open
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 adds REST API support for CMS menus by introducing a parameterized MenuView and related serializers, refactors base API views for unified preview handling, extends utility functions for API-based node selection, patches CMS navigation nodes to include API endpoints, updates URL routing for menu and preview patterns, and brings tests and documentation in line with these changes. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
+ Coverage 90.97% 91.44% +0.47%
==========================================
Files 17 18 +1
Lines 720 807 +87
Branches 81 87 +6
==========================================
+ Hits 655 738 +83
- Misses 42 44 +2
- Partials 23 25 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fsbraun
commented
Sep 2, 2025
![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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `djangocms_rest/serializers/menus.py:31` </location>
<code_context>
+ )
+ return serializer.data
+
+ def to_representation(self, obj: NavigationNode) -> dict:
+ """Customize the base representation of the NavigationNode."""
+ return {
+ "id": obj.id,
+ "title": obj.title,
+ "url": get_absolute_frontend_url(self.request, obj.url),
+ "api_endpoint": get_absolute_frontend_url(self.request, obj.api_endpoint),
+ "visible": obj.visible,
+ "selected": obj.selected
+ or obj.attr.get("is_home", False)
+ and getattr(self.request, "is_home", False),
+ "attr": obj.attr,
+ "level": obj.level,
+ "children": self.get_children(obj),
+ }
+
</code_context>
<issue_to_address>
The selected field logic may be confusing due to operator precedence.
Add parentheses to the 'selected' field expression to ensure the logic is clear and operator precedence does not cause unintended behavior.
</issue_to_address>
### Comment 2
<location> `djangocms_rest/utils.py:60` </location>
<code_context>
+@contextmanager
</code_context>
<issue_to_address>
Patching class attributes in a context manager may cause thread-safety issues.
Consider instance-level patching or alternative methods to ensure thread safety in concurrent scenarios.
</issue_to_address>
### Comment 3
<location> `tests/endpoints/test_menu.py:9` </location>
<code_context>
+from cms.models import Page
+
+
+class PageListAPITestCase(BaseCMSRestTestCase):
+ def test_get_menu_default(self):
+ """
</code_context>
<issue_to_address>
Consider adding tests for invalid input and error conditions for the menu endpoint.
Please add tests for invalid parameters, such as negative levels, non-existent paths, and malformed requests, to verify error handling and status code responses.
</issue_to_address>
### Comment 4
<location> `tests/endpoints/test_menu.py:60` </location>
<code_context>
+ self.assertFalse(results[1]["selected"])
+ self.assertFalse(results[2]["selected"])
+
+ def test_get_menu_with_children(self):
+ """
+ Test the menu endpoint (/api/{language}/menu/{path}/) with child pages.
</code_context>
<issue_to_address>
Test coverage for deeply nested children in the menu structure is missing.
Please add a test case with a multi-level page hierarchy to verify recursive serialization of nested children.
Suggested implementation:
```python
def test_get_menu_with_deeply_nested_children(self):
"""
Test the menu endpoint (/api/{language}/menu/{path}/) with deeply nested child pages.
Verifies:
- Recursive serialization of nested children
- All levels have correct titles and structure
"""
# Create a multi-level page hierarchy: root -> child -> grandchild
root_page = Page.objects.create(title="Root", slug="root", parent=None, language="en")
child_page = Page.objects.create(title="Child", slug="child", parent=root_page, language="en")
grandchild_page = Page.objects.create(title="Grandchild", slug="grandchild", parent=child_page, language="en")
url = reverse(
"menu",
kwargs={
```
```python
url = reverse(
"menu",
kwargs={
"language": "en",
"path": "root",
}
)
response = self.client.get(url)
assert response.status_code == 200
results = response.json()
# Root should have one child
assert results[0]["title"] == "Root"
assert len(results[0]["children"]) == 1
assert results[0]["children"][0]["title"] == "Child"
# Child should have one child (grandchild)
child_result = results[0]["children"][0]
assert len(child_result["children"]) == 1
assert child_result["children"][0]["title"] == "Grandchild"
# Grandchild should have no children
grandchild_result = child_result["children"][0]
assert grandchild_result["children"] == []
```
</issue_to_address>
### Comment 5
<location> `tests/endpoints/test_menu.py:56` </location>
<code_context>
+ self.assertEqual(results[2]["children"], [])
+
+ # Selected: Root page
+ self.assertTrue(results[0]["selected"])
+ self.assertFalse(results[1]["selected"])
+ self.assertFalse(results[2]["selected"])
+
+ def test_get_menu_with_children(self):
</code_context>
<issue_to_address>
Missing test for menu endpoint with a non-existent language code.
Please add a test that verifies a 404 response when the menu endpoint is requested with an invalid language code.
</issue_to_address>
### Comment 6
<location> `djangocms_rest/cms_config.py:47` </location>
<code_context>
return file.url if file.is_public else None
+def patch_get_menu_node_for_page_content(method: callable) -> callable:
+ def inner(self, page_content: PageContent, *args, **kwargs):
+ node = method(self, page_content, *args, **kwargs)
</code_context>
<issue_to_address>
Consider replacing patch functions with a subclass and method override to improve code clarity and maintainability.
Consider replacing the free‐standing patch functions with a simple subclass + override. This keeps all the same behavior but is much easier to trace:
```python
# new subclass, no more patch_* helpers
from cms.cms_menus import CMSMenu
class APICMSMenu(CMSMenu):
def get_menu_node_for_page_content(self, page_content, *args, **kwargs):
node = super().get_menu_node_for_page_content(page_content, *args, **kwargs)
node.api_endpoint = get_page_api_endpoint(
page_content.page,
page_content.language,
)
return node
```
Then wire it up in your AppConfig:
```python
class RESTCMSConfig(CMSAppConfig):
cms_enabled = True
cms_toolbar_mixin = RESTToolbarMixin
cms_menus = [APICMSMenu] # ← use our subclass here
def ready(self):
super().ready()
Page.add_to_class("get_api_endpoint", get_page_api_endpoint)
if File:
File.add_to_class("get_api_endpoint", get_file_api_endpoint)
```
This drops `patch_get_menu_node_for_page_content`, `patch_page_menu` and `add_api_endpoint` while preserving identical behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| from cms.models import Page | ||
|
|
||
|
|
||
| class PageListAPITestCase(BaseCMSRestTestCase): |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for invalid input and error conditions for the menu endpoint.
Please add tests for invalid parameters, such as negative levels, non-existent paths, and malformed requests, to verify error handling and status code responses.
| self.assertFalse(results[1]["selected"]) | ||
| self.assertFalse(results[2]["selected"]) | ||
|
|
||
| def test_get_menu_with_children(self): |
There was a problem hiding this comment.
suggestion (testing): Test coverage for deeply nested children in the menu structure is missing.
Please add a test case with a multi-level page hierarchy to verify recursive serialization of nested children.
Suggested implementation:
def test_get_menu_with_deeply_nested_children(self):
"""
Test the menu endpoint (/api/{language}/menu/{path}/) with deeply nested child pages.
Verifies:
- Recursive serialization of nested children
- All levels have correct titles and structure
"""
# Create a multi-level page hierarchy: root -> child -> grandchild
root_page = Page.objects.create(title="Root", slug="root", parent=None, language="en")
child_page = Page.objects.create(title="Child", slug="child", parent=root_page, language="en")
grandchild_page = Page.objects.create(title="Grandchild", slug="grandchild", parent=child_page, language="en")
url = reverse(
"menu",
kwargs={ url = reverse(
"menu",
kwargs={
"language": "en",
"path": "root",
}
)
response = self.client.get(url)
assert response.status_code == 200
results = response.json()
# Root should have one child
assert results[0]["title"] == "Root"
assert len(results[0]["children"]) == 1
assert results[0]["children"][0]["title"] == "Child"
# Child should have one child (grandchild)
child_result = results[0]["children"][0]
assert len(child_result["children"]) == 1
assert child_result["children"][0]["title"] == "Grandchild"
# Grandchild should have no children
grandchild_result = child_result["children"][0]
assert grandchild_result["children"] == []
Comment on lines
+56
to
+58
| self.assertTrue(results[0]["selected"]) | ||
| self.assertFalse(results[1]["selected"]) | ||
| self.assertFalse(results[2]["selected"]) |
There was a problem hiding this comment.
suggestion (testing): Missing test for menu endpoint with a non-existent language code.
Please add a test that verifies a 404 response when the menu endpoint is requested with an invalid language code.
| from django.contrib.sites.models import Site | ||
| from rest_framework.reverse import reverse | ||
|
|
||
| from tests.base import BaseCMSRestTestCase |
There was a problem hiding this comment.
issue (code-quality): Don't import test modules. (dont-import-test-modules)
Explanation
Don't import test modules.Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
This comment was marked as resolved.
This comment was marked as resolved.
|
Hide id |
|
We likely shoud add type checking for breadcrumb endpoints, similar to pages. #types.py
# API Response Type Definitions
PAGE_META_FIELD_TYPES = {
"title": str,
"page_title": str,
"menu_title": str,
"meta_description": (str, type(None)),
"redirect": (str, type(None)),
"in_navigation": bool,
"soft_root": bool,
"template": str,
"xframe_options": str,
"limit_visibility_in_menu": (bool, type(None)),
"language": str,
"path": str,
"absolute_url": str,
"is_home": bool,
"login_required": bool,
"languages": list,
"is_preview": bool,
"application_namespace": (str, type(None)),
"creation_date": (str, "datetime"),
"changed_date": (str, "datetime"),
}
# test_plugin_renderer.py
...
# Data & Type Validation
for page in results:
for field, expected_type in type_checks.items():
assert_field_types(
self,
page,
field,
expected_type,
)
``` |
|
Suggest stricter type checks:
I would commit this to PR, but I think policy prevents it, which is fine with me. This has to be added to all test cases, but only once to the menu, as we reuse it in breadcrumbs. #types.py
MENU_FIELD_TYPES = {
"title": str,
# "menu_title": str, #missing?
"namespace": (str, type(None)), # application_namespace?
"url": str, # absolute_url?
"api_endpoint": str,
"visible": bool,
"selected": bool,
"attr": dict,
"level": (int, type(None)),
"children": (list, type(None)),
}
#types_menu.py
type_checks = MENU_FIELD_TYPES
...
# Data & Type Validation
for menu_item in results:
for field, expected_type in type_checks.items():
assert_field_types(
self,
menu_item,
field,
expected_type,
)
#menus.py
return {
"id": obj.id,
"title": obj.title,
"namespace": obj.namespace if hasattr(obj, "namespace") else None,
"url": get_absolute_frontend_url(self.request, obj.url),
"api_endpoint": get_absolute_frontend_url(self.request, obj.api_endpoint),
"visible": obj.visible,
"selected": obj.selected
or obj.attr.get("is_home", False)
and getattr(self.request, "is_home", False),
"attr": obj.attr,
"level": obj.level,
"children": self.get_children(obj),
} |
|
@fsbraun The only issue with ?preview is with Placeholders: This shoud be retrievable via Retrieving unpublished objects via the placeholders endpoint using the ?preview flag is working. for you? |
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.
Fixes #4
Summary by Sourcery
Add menu endpoints for navigation and unify preview handling via query parameters. Introduce serializers and utilities for menu structures, enhance CMS menu nodes with API endpoints, refactor base views for preview logic, rename routes accordingly, and update tests and documentation.
New Features:
Enhancements:
Documentation:
Tests:
?previewquery parameter and updated route names