feat: Add the set_html_id template tag to resolve the random html.id attribute. #327
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
Contributor
Reviewer's GuideIntroduces a deterministic HTML id assignment mechanism for frontend UI items via a new template tag, replacing per-instance UUID generation and updating accordion and collapse templates to use the new IDs and parent references consistently. Sequence diagram for HTML id assignment during template renderingsequenceDiagram
participant Browser
participant DjangoTemplateEngine as DjangoTemplateEngine
participant TemplateTag as set_html_id
participant Request
participant Item as FrontendUIItem
Browser->>DjangoTemplateEngine: HTTP GET page
DjangoTemplateEngine->>Item: prepare instance
DjangoTemplateEngine->>TemplateTag: set_html_id(context, Item)
TemplateTag->>Item: check html_id is None
alt html_id is None
TemplateTag->>Request: get attribute frontend_plugins_counter
Request-->>TemplateTag: current_counter or 0
TemplateTag->>TemplateTag: counter = current_counter + 1
TemplateTag->>Item: set html_id = frontend-plugins-counter
TemplateTag->>Request: set attribute frontend_plugins_counter = counter
else html_id already set
TemplateTag-->>DjangoTemplateEngine: return existing html_id
end
TemplateTag-->>DjangoTemplateEngine: return html_id
DjangoTemplateEngine->>DjangoTemplateEngine: render HTML with id attributes from html_id
DjangoTemplateEngine-->>Browser: HTML response with deterministic ids
Class diagram for FrontendUIItem and set_html_id template tagclassDiagram
class FrontendUIItem {
+list _additional_classes
+str html_id
+__init__(*args, **kwargs)
+__getattr__(item)
}
class FrontendTemplateTags {
+set_html_id(context, instance) str
}
FrontendTemplateTags ..> FrontendUIItem : assigns_html_id
Flow diagram for deterministic html_id generationflowchart TD
A["Start rendering template with FrontendUIItem instance"] --> B["Check if instance.html_id is None"]
B -->|Yes| C["Get request from context"]
C --> D["Read request.frontend_plugins_counter or use 0"]
D --> E["Increment counter by 1"]
E --> F["Set instance.html_id to frontend-plugins-counter"]
F --> G["Store updated counter in request.frontend_plugins_counter"]
G --> H["Return instance.html_id for use in HTML id attributes"]
B -->|No| H
File-Level Changes
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 #327 +/- ##
==========================================
+ Coverage 89.00% 89.01% +0.01%
==========================================
Files 124 124
Lines 3383 3396 +13
Branches 287 289 +2
==========================================
+ Hits 3011 3023 +12
Misses 256 256
- Partials 116 117 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Replacing
uuidwithhtml_id = NoneonFrontendUIItemwhile only updating a subset of templates risks breaking any remaining references toinstance.uuid; consider either keepinguuidas a backwards-compatible attribute or auditing all usages to avoid runtimeAttributeErrors. - The
set_html_idtemplate tag assumescontext['request']is always present and mutable; if this tag can be used in contexts without arequest(e.g. offline rendering or custom contexts), you may want to guard access or fall back gracefully. - The per-request counter key
'frontend_plugins_counter'is a magic string stored directly on therequest; consider extracting it as a module-level constant or using a namespaced attribute (e.g. a dict onrequest) to reduce the risk of attribute name collisions and make the behavior clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Replacing `uuid` with `html_id = None` on `FrontendUIItem` while only updating a subset of templates risks breaking any remaining references to `instance.uuid`; consider either keeping `uuid` as a backwards-compatible attribute or auditing all usages to avoid runtime `AttributeError`s.
- The `set_html_id` template tag assumes `context['request']` is always present and mutable; if this tag can be used in contexts without a `request` (e.g. offline rendering or custom contexts), you may want to guard access or fall back gracefully.
- The per-request counter key `'frontend_plugins_counter'` is a magic string stored directly on the `request`; consider extracting it as a module-level constant or using a namespaced attribute (e.g. a dict on `request`) to reduce the risk of attribute name collisions and make the behavior clearer.
## Individual Comments
### Comment 1
<location> `djangocms_frontend/templatetags/frontend.py:77-85` </location>
<code_context>
return mark_safe(" ".join(attrs))
+@register.simple_tag(takes_context=True)
+def set_html_id(context: template.Context, instance: FrontendUIItem) -> str:
+ if instance.html_id is None:
+ request = context["request"]
+ key = "frontend_plugins_counter"
+ counter = getattr(request, key, 0) + 1
+ instance.html_id = f"frontend-plugins-{counter}"
+ setattr(request, key, counter)
+ return instance.html_id
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `context["request"]` directly risks a `KeyError` if the request is missing from the template context.
This tag assumes `"request"` is always present, which is typical for CMS plugin rendering but not guaranteed in all template contexts. To make it more robust, use `context.get("request")` and either return a deterministic fallback ID or raise a clear error when no request is available, instead of allowing a `KeyError` at runtime.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zbohm
changed the title
Member
|
@zbohm Interesting approach! Now, moving one of the plugins to the bottom of the page will make every plugin inbetween show up in the comparison. Maybe a combined approach solves the problem: By default This way changing the order only starts showing differences if an instance is rendered more than once. What do you think? |
Contributor
Author
|
Yes, moving the plugin down will cause the ids to be different. That is the weakness of this solution. You cannot use |
Member
|
That's right... and makes your proposal even more tempting. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Proposal for a solution for a unique and non-random html id attribute
Generating a random uuid is not suitable for version comparison. It is better to define the attribute so that it is the same every time.
The id attribute is set only when rendering HTML tags, according to the order of the tag in the DOM document. This maintains its consistency with respect to its position in the document.
Summary by Sourcery
Introduce a deterministic HTML id mechanism for frontend UI items and update Bootstrap 5 accordion and collapse templates to use it instead of per-instance UUIDs.
New Features:
Enhancements: