Skip to content

#20923 - Convert extras to new declarative UI layout#21765

Open
bctiemann wants to merge 3 commits intomainfrom
20923-extras-declarative-layouts
Open

#20923 - Convert extras to new declarative UI layout#21765
bctiemann wants to merge 3 commits intomainfrom
20923-extras-declarative-layouts

Conversation

@bctiemann
Copy link
Copy Markdown
Contributor

@bctiemann bctiemann commented Mar 26, 2026

#20923 (partial — extras app)

Summary

Test plan

  • All extras view tests pass (extras.tests.test_views)

@bctiemann bctiemann changed the title Convert extras to new declarative UI layout Closes #20923 - Convert extras to new declarative UI layout Mar 26, 2026
@bctiemann bctiemann changed the title Closes #20923 - Convert extras to new declarative UI layout #20923 - Convert extras to new declarative UI layout Mar 26, 2026
@bctiemann
Copy link
Copy Markdown
Contributor Author

@claude /review Does this PR cover all object detail views within the extras app? Are there any discrepancies or inconsistencies with other apps that have been migrated (circuits, dcim, etc.)? Note that extras has some particularly bespoke panel layouts compared to other apps.

@claude
Copy link
Copy Markdown

claude bot commented Mar 26, 2026

Claude finished @bctiemann's task in 7m 9s —— View job


PR Review: #20923 - Convert extras to new declarative UI layout


Coverage Assessment

All 15 standalone model detail views in extras have been migrated. The remaining ObjectView subclasses are correctly excluded:

View Status Reason
ObjectConfigContextView Excluded ✅ Abstract tab view used by other apps
ObjectRenderConfigView Excluded ✅ Abstract tab view used by other apps
BaseScriptView / ScriptView / ScriptSourceView / ScriptJobsView Excluded ✅ Highly specialized script UX, not panel-based
ScriptResultView Excluded ✅ Custom table + HTMX polling logic

Panel Content Fidelity

Comparing new panel definitions against the old templates, all field data is preserved correctly:

  • ChoiceAttr correctly calls get_FOO_color() via choice.html template, rendering colored badges — the JournalEntry.kind badge display is preserved.
  • registry (used by EventRuleEventTypesPanel) is provided globally via a Django context processor, so no get_extra_context override is needed on EventRuleView.
  • Sync warnings are preserved: SyncCodePanel includes {% include 'inc/sync_warning.html' %}, and configcontext_data.html does too.
  • Paginated choices in CustomFieldChoiceSetView are correctly passed through get_extra_contextCustomFieldChoiceSetChoicesPanel.get_context.

Issues & Observations

1. CustomFieldObjectTypesPanel is redundant (minor)

CustomFieldObjectTypesPanel (panels.py:214) is identical to ObjectTypesPanel — same template (extras/panels/object_types.html), same default title. It could be replaced with a direct ObjectTypesPanel() call in CustomFieldView, consistent with how CustomLinkView, EventRuleView, and ExportTemplateView already use the generic ObjectTypesPanel. The class and its __all__ entry could be removed.

Fix this →

2. Tag: "Tagged Items" count removed from attributes panel (minor)

The old tag.html showed a "Tagged Items" row ({{ taggeditem_table.rows|length }}) inside the Tag attributes card. The new TagPanel omits this. The table is visible in the bottom panel, but the quick-glance count in the attributes card is gone. Whether to restore it (e.g., as a NumericAttr on a computed property) is a judgement call.

3. Asymmetric column splits normalized (intentional change)

Several old templates used non-standard column splits to give more width to content panels:

  • configcontext.html: col-md-5 / col-md-7 (wider right for JSON data)
  • customlink.html: col-md-5 / col-md-7
  • journalentry.html: col-md-5 / col-md-7

The new SimpleLayout uses the standard 50/50 split, consistent with other migrated apps. This is the right trade-off for layout consistency.

4. ConfigTemplate vs ExportTemplate data source approach (preserved intentionally)

ConfigTemplatePanel bakes data_source, data_file, data_synced, auto_sync_enabled directly into the panel, while ExportTemplateView uses a separate TemplatePanel('core/inc/datafile_panel.html'). This matches the old templates exactly — not an inconsistency introduced by this PR.

5. CustomField.description loses markdown rendering (pre-existing behavior difference)

The old customfield.html used {{ object.description|markdown|placeholder }}, but the new TextAttr renders plain text. However, this is consistent with how description is handled in all other migrated apps (circuits, tenancy, core) — none use a markdown-rendering attr. This appears to be a deliberate decision in the new system's design.


Consistency with Other Migrated Apps

The extras migration is consistent with circuits, tenancy, and core:

  • Uses ui_panels module with ObjectAttributesPanel subclasses per model
  • Reuses generic panels (CommentsPanel, TagsPanel, CustomFieldsPanel, JSONPanel, TemplatePanel, ContextTablePanel) from netbox.ui.panels
  • bottom_panels used appropriately for full-width table panels (TagView tagged objects, ImageAttachmentView image display)
  • TemplatePanel used correctly for complex panels that don't fit the attrs model

The only structural pattern difference is that extras uses more bespoke template-based panels (e.g., configcontext_assignment.html, eventrule_event_types.html, imageattachment_file.html) compared to circuits which is almost entirely ObjectAttributesPanel — this is expected given the complexity of extras models.


Summary

✅ All extras model detail views are covered
✅ Panel content accurately mirrors old templates
✅ Consistent with other migrated apps
⚠️ CustomFieldObjectTypesPanel is redundant — can be removed
⚠️ Tag attributes panel loses the quick "Tagged Items" count

@bctiemann
Copy link
Copy Markdown
Contributor Author

bctiemann commented Mar 26, 2026

Addressed 2 (the TagPanel count); left 1 as deliberate (it will make it easier to customize CustomField panels later).

For 5, I'm not convinced that losing the Markdown behavior in other already-merged migrations is intentional. Before merging this I would like to bring up looking for such cases and restoring the Markdown parsing if appropriate.

nb: I caught a case where get_FOO_display had been replaced with a raw value, so that might be another pattern to look out for.

@bctiemann bctiemann requested review from a team and pheus and removed request for a team March 26, 2026 22:41
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.

1 participant