Top-Button redirects to List-View #91
Conversation
WalkthroughRefactors attachment tab/button registration to pass and return URL pattern names between functions, and changes the add-attachment template's post-action redirect to the object's attachment list view resolved via the computed URL pattern. Changes
Sequence DiagramsequenceDiagram
participant GTE as get_template_extensions
participant RTV as register_attachment_tab_view
participant CAB as create_add_attachment_button
participant TPL as Template
GTE->>RTV: register_attachment_tab_view(model)
RTV->>RTV: compute & register view_name
RTV-->>GTE: return view_name
Note over GTE: build url_pattern_name = "{app_label}:{model_name}_{view_name}"
GTE->>CAB: create_add_attachment_button(model_name, url_pattern_name)
CAB->>CAB: inject `object_type_attachment_list` = url_pattern_name into context
CAB-->>TPL: extension renders button with `object_type_attachment_list`
TPL->>TPL: add-attachment link resolves attachment-list URL with object's pk as return_url
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
netbox_attachments/templates/netbox_attachments/add_attachment_button.html (1)
2-2: Fix context key typo, URL-encode return_url, and guard on object presence.
- Rename context var to "object_type_attachment_list" (correct spelling) and match Python.
- Use
{% url ... as %}+|urlencodeto avoid malformed querystrings.- Ensure
objectexists before rendering to avoid template errors whenobjectis absent.Apply:
-{% if perms.netbox_attachments.add_netboxattachment %} - <a href="{% url 'plugins:netbox_attachments:netboxattachment_add' %}?object_type={{ object|content_type_id }}&object_id={{ object.pk }}&return_url={% url object_type_attachement_list pk=object.pk %}" - class="btn btn-secondary"> +{% if perms.netbox_attachments.add_netboxattachment and object %} + {% url object_type_attachment_list pk=object.pk as attachment_list_url %} + <a href="{% url 'plugins:netbox_attachments:netboxattachment_add' %}?object_type={{ object|content_type_id }}&object_id={{ object.pk }}&return_url={{ attachment_list_url|urlencode }}" + class="btn btn-secondary"> <i class="mdi mdi-paperclip" aria-hidden="true"></i> Add Attachment </a> {% endif %}Sanity-check: load any eligible object detail page, click “Add Attachment,” save, and confirm redirect lands on the attachment list tab/view for that object.
netbox_attachments/template_content.py (1)
60-69: Docstring out of sync with new parameter.Document the new
url_pattern_namearg for clarity and future maintenance.-def create_add_attachment_button(model_name: str, url_pattern_name: str) -> Type[PluginTemplateExtension]: +def create_add_attachment_button(model_name: str, url_pattern_name: str) -> Type[PluginTemplateExtension]: @@ - Args: - model_name: String in format "<app_label>.<model>" (e.g., "dcim.device") + Args: + model_name: String in format "<app_label>.<model>" (e.g., "dcim.device") + url_pattern_name: Fully qualified URL pattern name for the attachment list view + (e.g., "dcim:device_device-attachment_list")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netbox_attachments/template_content.py(4 hunks)netbox_attachments/templates/netbox_attachments/add_attachment_button.html(1 hunks)
🔇 Additional comments (2)
netbox_attachments/template_content.py (2)
81-120: Solid view registration; returning view_name is helpful.The registration flow and returned
view_namelook correct and cohesive with URL construction. LGTM.
160-167: Confirm URL name construction remains stable across NetBox versions.
url_pattern_name = f"{app_label}:{model_name}_{view_name}"assumes NetBox’sregister_model_viewnaming convention remains unchanged. If NetBox alters this, reversals will break at runtime.Please verify against your target NetBox version:
- Confirm
reverse("{app_label}:{model_name}_{model_name}-attachment_list", kwargs={"pk": <obj_pk>})resolves.- If there’s any doubt, consider a fallback: on NoReverseMatch, default
return_urltoobject.get_absolute_url().Optionally, add a small unit test that registers a dummy model view and asserts the reverse name format.
m-hau
force-pushed
the
feature/redirect
branch
from
30d4210 to
7448bc7
Compare
Change the redirect target of the top "Add Attachment" button from the url you are currently at to the attachment-list-view of the object where the attachment will/was created. This makes it consistent with for example the "Add Component" buttons for devices.
m-hau
force-pushed
the
feature/redirect
branch
from
7448bc7 to
7f2a032
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
netbox_attachments/templates/netbox_attachments/add_attachment_button.html (1)
2-6: Harden return_url: add reverse fallback and URL-encode.Avoid template 500s if reverse fails and ensure proper encoding.
Apply:
{% if perms.netbox_attachments.add_netboxattachment %} - <a href="{% url 'plugins:netbox_attachments:netboxattachment_add' %}?object_type={{ object|content_type_id }}&object_id={{ object.pk }}&return_url={% url object_type_attachment_list pk=object.pk %}" + {% url object_type_attachment_list pk=object.pk as list_url %} + <a href="{% url 'plugins:netbox_attachments:netboxattachment_add' %}?object_type={{ object|content_type_id }}&object_id={{ object.pk }}&return_url={{ list_url|default:object.get_absolute_url|urlencode }}" class="btn btn-secondary"> <i class="mdi mdi-paperclip" aria-hidden="true"></i> Add Attachment </a> {% endif %}
- Uses “as list_url” to avoid NoReverseMatch at render time and falls back to object.get_absolute_url.
- Adds urlencode to return_url.
Please confirm the reverse succeeds across a few models (e.g., dcim.Device, ipam.VRF) after this change.
netbox_attachments/template_content.py (1)
85-124: Return the fully qualified URL pattern name from registration to remove duplication.Compute the FQN once in register_attachment_tab_view and return it; reduces drift and string-crafting errors downstream.
Apply:
-def register_attachment_tab_view(model: Type[Model]) -> str: +def register_attachment_tab_view(model: Type[Model]) -> str: @@ - model_name = model._meta.model_name + app_label = model._meta.app_label + model_name = model._meta.model_name view_name = f"{model_name}-attachment_list" view_path = view_name # Path matches the name @@ register_model_view(model, name=view_name, path=view_path)(AttachmentTabView) - return view_name + # Return fully-qualified URL pattern name used by {% url %} + return f"{app_label}:{model_name}_{view_name}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netbox_attachments/template_content.py(4 hunks)netbox_attachments/templates/netbox_attachments/add_attachment_button.html(1 hunks)
🔇 Additional comments (2)
netbox_attachments/template_content.py (2)
60-81: Context key now matches template; good.The key object_type_attachment_list aligns with the template usage. Looks consistent.
163-171: The review comment is incorrect and should be rejected.The comment assumes
register_attachment_tab_view()returns a fully-qualified URL pattern name (FQN), but the function actually returns onlyview_name— a partial name like "site-attachment_list" (line 129). The current code's formatting at line 170 (f"{app_label}:{model_name}_{view_name}") is necessary to build the complete pattern name required by Django's reverse().The proposed refactor would pass an incomplete name to
create_add_attachment_button(), breaking the functionality.Note: The current formatting appears to duplicate the model name ("dcim:site_site-attachment_list"), which may warrant a separate fix, but that requires changes to how
view_nameis constructed inregister_attachment_tab_view()itself—not adoption of the proposed refactor.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
netbox_attachments/template_content.py (1)
77-80: Ensure template key alignment; add temporary fallback to avoid breakagePast commits had the misspelling “attachement”. If the template wasn’t updated in the same commit, rendering will break. Pass both keys for one release to be safe, then remove the fallback.
- return self.render( - "netbox_attachments/add_attachment_button.html", - extra_context={'object_type_attachment_list': url_pattern_name} - ) + return self.render( + "netbox_attachments/add_attachment_button.html", + extra_context={ + "object_type_attachment_list": url_pattern_name, + # Temporary compatibility until all templates are updated + "object_type_attachement_list": url_pattern_name, + }, + )Run to verify no remaining uses of the misspelled key:
#!/bin/bash rg -n "object_type_attachement_list" -C2
🧹 Nitpick comments (2)
netbox_attachments/template_content.py (2)
96-99: Optional: adopt constant, shorter path "attachments"Per PR notes, consider simplifying the path to “attachments” for cleaner URLs (e.g., /dcim/devices/1234/attachments/). This is cosmetic and can be a follow-up.
- view_name = f"{model_name}-attachment_list" - view_path = view_name # Path matches the name + view_name = f"{model_name}-attachment_list" + view_path = "attachments"
85-99: Refactor is sound; verify naming redundancy is intentionalThe suggestion to centralize URL name generation is a valid refactor. The code currently constructs the fully qualified URL name twice (once implicitly via
register_model_viewand once explicitly for the button), and your recommendation consolidates this into a single return statement.Verification confirms:
- NetBox's
register_model_viewgenerates URL names in the format<app_label>:<model_name>_<view_name>- Current code at lines 126–127 constructs
url_pattern_name = f"{app_label}:{model_name}_{view_name}"which is consistent with NetBox's behavior- The refactoring maintains this convention while eliminating code duplication
One note: the current
view_namealready includes the model name (e.g.,"site-attachment_list"), resulting in redundant naming like"dcim:site_site-attachment_list". If this double-prefixing is unintentional, consider simplifyingview_nameto just"attachment_list"to produce cleaner URL names like"dcim:site_attachment_list". Otherwise, the refactoring as proposed is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
netbox_attachments/template_content.py(4 hunks)netbox_attachments/templates/netbox_attachments/add_attachment_button.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- netbox_attachments/templates/netbox_attachments/add_attachment_button.html
🔇 Additional comments (1)
netbox_attachments/template_content.py (1)
60-68: Docstring/example: verify correctness of URL name sampleThe example "dcim:device_device-attachment_list" assumes NetBox composes names as "{model}_{name}". Please verify and update the example if NetBox differs (e.g., underscores vs hyphens).
1 participant
Change the redirect target of the top "Add Attachment" button from the url you are currently at to the attachment list view of the object where the attachment will/was created. This makes it consistent with for example the "Add Component" buttons for devices.
The implementation is probably not perfect, I was mostly playing around to see if/how it can be done. It uses the fact that registering a view also always creates a matching url-pattern. The name of that url-pattern is predictably derived from the view name, and can then be passed to Django's
urltemplate function (together with the primary key of the object).It would have actually been easier if the view name wouldn't also include the model name again and just be a constant string (like
attachments). This would also make the URLs look a bit nicer, instead of/dcim/devices/1234/device-attachment_list/you get/dcim/devices/1234/attachments/.Summary by CodeRabbit
Improvements
Refactor