Skip to content

Closes #21929: Eliminate redundant object view templates#21930

Open
jeremystretch wants to merge 5 commits intofeaturefrom
21929-template-cleanup
Open

Closes #21929: Eliminate redundant object view templates#21930
jeremystretch wants to merge 5 commits intofeaturefrom
21929-template-cleanup

Conversation

@jeremystretch
Copy link
Copy Markdown
Member

@jeremystretch jeremystretch commented Apr 15, 2026

Closes: #21929

  • Introduce the get_default_template() utility function to safely resolve the conventional template name for a model
  • Update several feature views to employ get_default_template()
  • Delete all "empty" object view templates
  • Update the applicable view classes to render generic/object.html directly

@jeremystretch

This comment was marked as outdated.

@claude

This comment was marked as outdated.

@jeremystretch
Copy link
Copy Markdown
Member Author

@claude /review Highlight any inconsistencies or gaps

@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Claude finished @jeremystretch's task in 7m 6s —— View job


Code Review: Eliminate Redundant Object View Templates

  • Read changed view files
  • Analyze template deletions
  • Check for inconsistencies or gaps
  • Post review

Overview

This PR deletes "empty" model-specific templates that only contained {% extends 'generic/object.html' %} and updates the corresponding view classes to set template_name = 'generic/object.html' explicitly. A third commit introduces get_default_template() to safely resolve the base template for feature views (changelog, journal, image-attachments, jobs). The overall approach is sound, but there's one real bug and a few gaps worth addressing.


Bug: ObjectContactsView will fail for L2VPN

File: netbox/netbox/views/generic/object_views.py, line 166

'base_template': f'{instance._meta.app_label}/{instance._meta.model_name}.html',

ObjectChildrenView.get() hard-codes base_template using the model's app label and model name. generic/object_children.html then does {% extends base_template %}, so if the template file doesn't exist the page will raise TemplateDoesNotExist.

The get_default_template() helper was correctly applied to ObjectChangeLogView, ObjectImageAttachmentsView, ObjectJournalView, and ObjectJobsView in feature_views.py — but not to ObjectChildrenView, which is the base class for ObjectContactsView.

L2VPN uses ContactsMixin, meaning it gets a Contacts tab auto-registered via register_model_view. Its template vpn/l2vpn.html was deleted in this PR. Navigating to the Contacts tab of any L2VPN will now raise TemplateDoesNotExist: vpn/l2vpn.html.

Fix: Replace line 166 in object_views.py with get_default_template(instance):

'base_template': get_default_template(instance),

Fix this →


Gap: Inconsistent application of get_default_template()

get_default_template() was applied to 4 of the 5 auto-registered feature views but not to the contacts view path (ObjectChildrenView). The fix above closes this gap. It's worth double-checking whether any other ObjectChildrenView subclasses with a hard-coded base_template override could have a similar issue.


Minor observations

  • users/group.html, users/user.html, users/ownergroup.html only override {% block subtitle %}{% endblock %} to suppress the created/last-updated metadata shown by generic/object.html. These were correctly left alone since they have meaningful content.
  • circuits app views have no template_name set and rely on model-specific templates that still exist with real content (breadcrumbs, extra controls). Not a problem; correctly excluded from this PR.
  • get_default_template() performance: The function calls get_template() (which hits Django's template loader cache) on every request for changelog/journal/etc. tabs. With the cached template loader in production this is fine; in DEBUG mode it's a minor filesystem hit per request. Not a blocker, just worth noting.
  • No direct unit tests for get_default_template(). The function is simple and implicitly tested by any view test covering the affected tabs, but an explicit unit test would be a nice safeguard.

Summary: One real bug (L2VPN Contacts tab will raise TemplateDoesNotExist) due to ObjectChildrenView.get() not being updated to use get_default_template(). The rest of the cleanup is consistent and well-structured.

@jeremystretch
Copy link
Copy Markdown
Member Author

Fixed the above bug concerning ObjectChildrenView

@jeremystretch jeremystretch marked this pull request as ready for review April 16, 2026 13:51
@jeremystretch jeremystretch requested review from a team and arthanson and removed request for a team April 16, 2026 13:51
Copy link
Copy Markdown
Collaborator

@arthanson arthanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove all the assignments of template_name = 'generic/object.html' and just put it into netbox.views.generic.object_views.ObjectView it should still work the same and would be consistent with the other Generic object views and less boilerplate with the new get_default_template

Also the doc comment on netbox.views.generic.object_views.ObjectView needs to be changed, currently

    Note: If `template_name` is not specified, it will be determined automatically based on the queryset model.

With the new logic the is reversed, it will always try to check if the {app}/{model} template exists first and then fallback to the template_name.

@jeremystretch
Copy link
Copy Markdown
Member Author

@arthanson That would potentially break plugin usage. We'll probably head that way eventually, but for now we have to be very careful not to alter the base view's behavior.

@jeremystretch jeremystretch requested a review from arthanson April 17, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants