feat: Remove PlaceholderRelationSerializer and add Placeholder content instead #73
Conversation
…aceholderSerializer
Reviewer's GuideThis PR removes the separate PlaceholderRelationSerializer, refactors PlaceholderSerializer to handle content, HTML, and details directly during representation, and updates PageContentSerializer to use the enhanced PlaceholderSerializer with simplified URL generation. Class diagram for updated PlaceholderSerializer and removal of PlaceholderRelationSerializerclassDiagram
class PlaceholderSerializer {
+label: CharField
+language: CharField
+content: ListSerializer(JSONField)
+details: URLField
+html: CharField
+__init__(request, language, render_plugins)
+to_representation(instance)
+get_details(instance)
}
class PlaceholderRelationSerializer {
-content_type_id: IntegerField
-object_id: IntegerField
-slot: CharField
-details: URLField
-__init__(language)
-to_representation(instance)
-get_details(instance)
}
PlaceholderRelationSerializer <|-- PlaceholderSerializer : "removed, replaced by"
Class diagram for updated PageContentSerializer placeholders fieldclassDiagram
class PageContentSerializer {
+placeholders: PlaceholderSerializer[many=True]
+__init__()
+to_representation(page_content)
}
PageContentSerializer --> PlaceholderSerializer : uses
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 and found some issues that need to be addressed.
- Avoid mutating the placeholder model instance inside to_representation—build the serialized dict or use SerializerMethodFields so you don’t introduce side-effects on the original object.
- get_details currently appends all request.GET params to the placeholder URL; consider whitelisting only relevant flags (e.g. html or preview) to avoid leaking unrelated query parameters.
- The to_representation method is getting quite long—extracting plugin rendering and HTML assembly into small helper methods or a dedicated service will improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Avoid mutating the placeholder model instance inside to_representation—build the serialized dict or use SerializerMethodFields so you don’t introduce side-effects on the original object.
- get_details currently appends all request.GET params to the placeholder URL; consider whitelisting only relevant flags (e.g. html or preview) to avoid leaking unrelated query parameters.
- The to_representation method is getting quite long—extracting plugin rendering and HTML assembly into small helper methods or a dedicated service will improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `djangocms_rest/serializers/placeholders.py:27-29` </location>
<code_context>
- if placeholder and request and language:
- if render_plugins:
+ def to_representation(self, instance):
+ instance.label = instance.get_label()
+ instance.language = self.language
+ instance.details = self.get_details(instance)
+ if instance and self.request and self.language:
+ if self.render_plugins:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Directly mutating the instance in 'to_representation' can have unintended side effects.
Using a local dictionary to build the representation avoids unintended changes to the input object and prevents side effects if the instance is reused elsewhere.
</issue_to_address>
### Comment 2
<location> `djangocms_rest/serializers/placeholders.py:62-63` </location>
<code_context>
- else:
- api_endpoint += "?preview=1"
- return api_endpoint
+ if self.request.GET:
+ url += f"?{self.request.GET.urlencode()}"
+ return url
</code_context>
<issue_to_address>
**🚨 issue (security):** Appending all query parameters to the details URL may expose sensitive or irrelevant data.
Filter query parameters to include only those necessary for the details endpoint to avoid leaking sensitive or irrelevant information.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| instance.label = instance.get_label() | ||
| instance.language = self.language | ||
| instance.details = self.get_details(instance) |
There was a problem hiding this comment.
suggestion (bug_risk): Directly mutating the instance in 'to_representation' can have unintended side effects.
Using a local dictionary to build the representation avoids unintended changes to the input object and prevents side effects if the instance is reused elsewhere.
There was a problem hiding this comment.
@metaforx What do you think about this? Maybe we change the pattern.
…un/djangocms-rest into feat/immediate-placeholder-content
…un/djangocms-rest into feat/immediate-placeholder-content
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
+ Coverage 90.83% 91.23% +0.39%
==========================================
Files 18 18
Lines 851 844 -7
Branches 98 97 -1
==========================================
- Hits 773 770 -3
+ Misses 48 46 -2
+ Partials 30 28 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@metaforx This should also reduce the number of API calls: Content is already fetched with a page,. |
|
@fsbraun: Very nice 👍. I tested it locally and could fetch content as expected from root and subpage. Should we still add the option to only fetch metadata? Or is there still very little JSON data, even when the page is large? I was just thinking about whether there is a case where we would want to fetch page metadata without also fetching the content. One other issue we should fix, which was also present in earlier endpoints. "placeholders": [
{
"slot": "string",
"label": "string",
"language": "string",
"details": "string",
"html": ""
}
]
"placeholders": [
{
"slot": "content",
"label": "Content",
"language": "en",
"content": [ <- missing in schema
{
}
]It looks set in code, but I do not get it when fetching the schema. |
|
@metaforx I fixed the schema issue and added test for the schema. I do not think that generating the content is too traffic heavy: In the end it's a page worth of content. Once we have added the path feature to the menus, would the menus provide enough meta info on the site structure and also on the pages? We also have I'll leave it for you to decide if we add a set of |
This PR adds the Placeholder content directly to placeholder relation fields, e.g., for the page detail endpoint:
Tests need to be adjusted.
Summary by Sourcery
Provide placeholder content, HTML, and details URL directly in page responses by consolidating placeholder serialization into a single serializer
New Features:
Enhancements:
Tests: