diff --git a/CHANGELOG.md b/CHANGELOG.md index b2cef0b..13e6c5e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,22 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [11.3.0] - 2026-07-17 + +### Fixed + +- Attachments never appeared on Custom Object detail pages when `netbox_attachments` was listed before `netbox_custom_objects` in `PLUGINS` (issue #110). Template extensions were built while this plugin's `ready()` ran, but `netbox_custom_objects` withholds its dynamically generated models until its own `ready()` has finished, so model discovery found nothing and no panel was ever registered — regardless of `scope_filter`. Custom objects are now served by a single, globally registered template extension that resolves the object at render time, so plugin order in `PLUGINS` no longer matters. +- Custom Object Types created after startup now get an attachment panel without restarting NetBox. Previously the model list was enumerated once during startup, so any type created later was invisible until the next restart. + +### Added + +- Startup warning (Django system check `netbox_attachments.W001`–`W003`) when `PLUGINS_CONFIG["netbox_attachments"]` still contains `apps`, `allowed_models`, or `mode`. These were replaced by `scope_filter` and `applied_scope` in v7.1.0, and NetBox keeps unknown keys without reading them, so a stale config silently fell back to the default `scope_filter` — which covers several core apps, making the configuration look partly honored (issue #110). The check names the replacement setting; run `manage.py check` to see it. + +### Changed + +- Custom object models are no longer enumerated at startup, removing this plugin's own database access during app loading and its dependency on `PLUGINS` ordering. (`apps.get_models()` is still called, so `netbox_custom_objects` may itself query when it loads first — but the plugin no longer drives that.) +- `display_setting` now accepts the same identifier for custom objects that `scope_filter` does — `netbox_custom_objects.`. It previously keyed off the internal, primary-key-derived model name (`netbox_custom_objects.table12model`), so the documented identifier silently never matched and per-type display overrides had no effect. + ## [11.2.3] - 2026-06-02 ### Added diff --git a/docs/configuration.md b/docs/configuration.md index d8ae047..da59a4a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -2,6 +2,24 @@ Configure plugin settings under `PLUGINS_CONFIG["netbox_attachments"]`. +## Removed settings + +These were replaced in v7.1.0 and are **ignored** if still present: + +| Removed | Use instead | +| --- | --- | +| `apps` | `scope_filter` | +| `allowed_models` | `scope_filter` | +| `mode` | `applied_scope` (values are `app`/`model`, not `permissive`/`restrictive`) | + +NetBox keeps unrecognized keys in `PLUGINS_CONFIG` without reading them, so a stale config does not raise an error — the replacement setting silently falls back to its default instead. Because that default already covers several core apps, part of the configuration keeps working while the rest does not, which is easy to mistake for a bug in the plugin. + +Since v11.3.0 a Django system check warns about each removed setting at startup. To see them: + +```bash +python3 manage.py check +``` + ## Settings ### `applied_scope` @@ -62,14 +80,21 @@ Controls top-level **Attachments** dropdown creation when using `additional_tab` - Type: `dict[str, str]` - Default: `{}` -Per-model display override map. +Per-model display override map. Keys use the same identifiers as `scope_filter`: `app_label.model` for standard models, and `netbox_custom_objects.` for custom objects. Example: ```python -{"dcim.device": "left_page", "ipam.vlan": "additional_tab"} +{ + "dcim.device": "left_page", + "ipam.vlan": "additional_tab", + "netbox_custom_objects.display_calibration": "right_page", +} ``` +!!! note + Before v11.3.0, custom objects were keyed here by an internal, primary-key-derived model name (`netbox_custom_objects.table12model`), so the identifier above silently had no effect. + ## Example Configuration ```python @@ -92,6 +117,25 @@ PLUGINS_CONFIG = { } ``` -## Custom Objects Limitation +## Custom Objects + +Attachments work on [netbox-custom-objects](https://github.com/netboxlabs/netbox-custom-objects) models. Enable them like any other app: + +- `applied_scope: "app"` — add `netbox_custom_objects` to `scope_filter` +- `applied_scope: "model"` — add `netbox_custom_objects.` + +Custom object types created after NetBox starts are picked up automatically; no restart is needed. + +### Display limitations + +A custom object detail page is rendered by the Custom Objects plugin's own template, which hardcodes its tab bar and its button row. It renders only the `left_page`, `right_page`, and `full_width_page` panel hooks, so on custom objects: + +- **`additional_tab` is unavailable** — there is no way for a plugin to add a tab to that page. The effective display falls back to `full_width_page`, unless you pick a side panel explicitly: + + ```python + "display_setting": {"netbox_custom_objects.display_calibration": "right_page"}, + ``` + +- **`create_add_button` has no effect** — the top-level "Attachments" dropdown cannot be rendered there. Use the panel's own "Add Attachment" and "Link Existing" buttons instead. -`additional_tab` mode is not available for custom object models using non-standard URL routing. For such models, the effective fallback is `full_width_page` unless an explicit left/right panel mode is configured. +Both limitations are properties of the Custom Objects detail template, not of this plugin, and would need an upstream change to lift. diff --git a/netbox_attachments/__init__.py b/netbox_attachments/__init__.py index 7b01b64..2b71cf4 100644 --- a/netbox_attachments/__init__.py +++ b/netbox_attachments/__init__.py @@ -17,7 +17,7 @@ class NetBoxAttachmentsConfig(PluginConfig): author = "Jan Krupa" base_url = "netbox-attachments" default_settings = { - "applied_scope": "app", # Changed from 'mode' - options: 'app' or 'model' + "applied_scope": "app", "scope_filter": [ "dcim", "ipam", @@ -25,7 +25,7 @@ class NetBoxAttachmentsConfig(PluginConfig): "tenancy", "virtualization", "wireless", - ], # Merged from 'apps' and 'allowed_models' + ], "display_default": "additional_tab", "create_add_button": True, "display_setting": {}, @@ -34,5 +34,11 @@ class NetBoxAttachmentsConfig(PluginConfig): min_version = "4.5.0" max_version = "4.6.99" + def ready(self): + super().ready() + + # Import for the @register side effect; see checks.py for what it warns about. + from netbox_attachments import checks # noqa: F401 + config = NetBoxAttachmentsConfig diff --git a/netbox_attachments/checks.py b/netbox_attachments/checks.py new file mode 100644 index 0000000..9c39060 --- /dev/null +++ b/netbox_attachments/checks.py @@ -0,0 +1,62 @@ +"""Django system checks. + +Warns about settings that were removed in v7.1.0 when ``apps`` and +``allowed_models`` were merged into ``scope_filter`` and ``mode`` became +``applied_scope``. NetBox's ``PluginConfig.validate()`` only fills in *missing* +keys, so a removed setting is not an error: it survives untouched in +``PLUGINS_CONFIG`` while nothing reads it, and the replacement silently falls +back to its default. That default covers several core apps, so part of the +configuration keeps working and the rest fails without a clue (issue #110). +""" + +from django.core.checks import Warning, register + +from netbox_attachments.utils import _get_plugin_settings +from netbox_attachments.version import __version__ + +_MERGED_HINT = "'apps' and 'allowed_models' were merged, so combine both into the single 'scope_filter' list." + +# Removed setting -> (replacement, stable check ID, extra hint). +# The IDs are a published contract (CHANGELOG, SILENCED_SYSTEM_CHECKS): never +# renumber or reuse one, even if an entry is dropped from this table. +# `mode` is not a pure rename — its values changed along with its name. +REMOVED_SETTINGS = { + "apps": ("scope_filter", "netbox_attachments.W001", _MERGED_HINT), + "allowed_models": ("scope_filter", "netbox_attachments.W002", _MERGED_HINT), + "mode": ( + "applied_scope", + "netbox_attachments.W003", + "Its values changed too: 'applied_scope' accepts 'app' or 'model', not 'permissive'/'restrictive'.", + ), +} + +# Pinned to this release's tag so the linked page describes the settings this +# install actually has; a moving main-branch URL would drift or 404. +_DOCS_URL = f"https://github.com/Kani999/netbox-attachments/blob/v{__version__}/docs/configuration.md" + + +@register() +def check_removed_settings(app_configs, **kwargs): + """Warn for each removed plugin setting still present in PLUGINS_CONFIG.""" + plugin_settings = _get_plugin_settings() + + warnings = [] + for removed, (replacement, check_id, extra_hint) in REMOVED_SETTINGS.items(): + if removed not in plugin_settings: + continue + + if replacement in plugin_settings: + effect = f"Your configured '{replacement}' is in effect; delete the stale '{removed}' key." + else: + effect = f"'{replacement}' falls back to its default value." + + hint = f"Use '{replacement}' in PLUGINS_CONFIG['netbox_attachments'] instead. {extra_hint} See {_DOCS_URL}" + warnings.append( + Warning( + f"netbox-attachments: the '{removed}' setting was removed in v7.1.0 and is ignored. {effect}", + hint=hint, + id=check_id, + ) + ) + + return warnings diff --git a/netbox_attachments/migrations/0012_netboxattachment_owner.py b/netbox_attachments/migrations/0012_netboxattachment_owner.py index ed38af9..02c8290 100644 --- a/netbox_attachments/migrations/0012_netboxattachment_owner.py +++ b/netbox_attachments/migrations/0012_netboxattachment_owner.py @@ -3,21 +3,20 @@ class Migration(migrations.Migration): - dependencies = [ - ('netbox_attachments', '0011_netboxattachmentassignment_index'), - ('users', '0015_owner'), + ("netbox_attachments", "0011_netboxattachmentassignment_index"), + ("users", "0015_owner"), ] operations = [ migrations.AddField( - model_name='netboxattachment', - name='owner', + model_name="netboxattachment", + name="owner", field=models.ForeignKey( blank=True, null=True, on_delete=django.db.models.deletion.PROTECT, - to='users.owner', + to="users.owner", ), ), ] diff --git a/netbox_attachments/migrations/0013_alter_netboxattachmentassignment_custom_field_data.py b/netbox_attachments/migrations/0013_alter_netboxattachmentassignment_custom_field_data.py index 6670eee..aec8936 100644 --- a/netbox_attachments/migrations/0013_alter_netboxattachmentassignment_custom_field_data.py +++ b/netbox_attachments/migrations/0013_alter_netboxattachmentassignment_custom_field_data.py @@ -3,15 +3,14 @@ class Migration(migrations.Migration): - dependencies = [ - ('netbox_attachments', '0012_netboxattachment_owner'), + ("netbox_attachments", "0012_netboxattachment_owner"), ] operations = [ migrations.AlterField( - model_name='netboxattachmentassignment', - name='custom_field_data', + model_name="netboxattachmentassignment", + name="custom_field_data", field=models.JSONField( blank=True, default=dict, diff --git a/netbox_attachments/tables.py b/netbox_attachments/tables.py index 1550ff4..e809b57 100644 --- a/netbox_attachments/tables.py +++ b/netbox_attachments/tables.py @@ -78,9 +78,7 @@ def get_missing_parent_row_class(record): if count is not None: return "table-danger" if count == 0 else "" # Fallback for views without annotation - logger.warning( - "assignment_count annotation missing for %r; falling back to exists() query", record - ) + logger.warning("assignment_count annotation missing for %r; falling back to exists() query", record) return "table-danger" if not record.attachment_assignments.exists() else "" diff --git a/netbox_attachments/template_content.py b/netbox_attachments/template_content.py index 8eafbce..59c82d2 100644 --- a/netbox_attachments/template_content.py +++ b/netbox_attachments/template_content.py @@ -4,10 +4,17 @@ from django.db.models import Count from django.db.utils import OperationalError -from netbox_attachments.utils import _get_plugin_settings, is_custom_object_model, validate_object_type +from netbox_attachments.utils import ( + _get_plugin_settings, + custom_object_identifier, + is_custom_object_model, + validate_object_type, +) logger = logging.getLogger(__name__) +ATTACHMENT_PANEL_TEMPLATE = "netbox_attachments/netbox_attachment_panel.html" + def _resolve_display_preference(app_model_name: str, plugin_settings: dict) -> str: default_display = plugin_settings.get("display_default", "additional_tab") @@ -31,6 +38,42 @@ def resolve_effective_display_preference( return display_preference +def resolve_display_preference_for_model(model, plugin_settings: dict | None = None) -> str: + """ + The one model -> effective display position answer, shared by the startup loop and + the render-time panel so the two cannot drift (issue #112). + + Custom objects are keyed in display_setting by their type name — the same + identifier scope_filter uses — but resolving that name costs a query, so it is + looked up only when an override exists that could match. The additional_tab -> + full_width_page fallback for custom objects lives in + resolve_effective_display_preference and therefore applies to every caller. + """ + settings_data = _get_plugin_settings() if plugin_settings is None else plugin_settings + is_custom_object = is_custom_object_model(model) + + display_key = model._meta.label_lower + if is_custom_object: + display_settings = settings_data.get("display_setting") + if isinstance(display_settings, dict) and display_settings: + display_key = custom_object_identifier(model) or display_key + + return resolve_effective_display_preference( + display_key, + is_custom_object=is_custom_object, + plugin_settings=settings_data, + ) + + +def _render_or_empty(extension, template_name: str, label: str, extra_context: dict | None = None) -> str: + """Render a template on a PluginTemplateExtension, degrading to '' on any failure.""" + try: + return extension.render(template_name, extra_context=extra_context) + except Exception as exc: + logger.error(f"Failed to render {template_name} for {label}: {exc}") + return "" + + def render_attachment_panel(self) -> str: model_name = self.models[0] if (hasattr(self, "models") and self.models) else getattr(self, "model", None) if model_name is None: @@ -39,15 +82,7 @@ def render_attachment_panel(self) -> str: if "." not in str(model_name): logger.error(f"Invalid model name format: {model_name!r}") return "" - try: - return self.render("netbox_attachments/netbox_attachment_panel.html") - except Exception as exc: - logger.error(f"Failed to render attachment panel for {model_name}: {exc}") - return "" - - -def get_display_preference(app_model_name: str) -> str: - return _resolve_display_preference(app_model_name, _get_plugin_settings()) + return _render_or_empty(self, ATTACHMENT_PANEL_TEMPLATE, str(model_name)) def create_add_attachment_button(model_name: str, url_pattern_name: str): @@ -57,14 +92,12 @@ class AddAttachmentButton(PluginTemplateExtension): models = [model_name] def buttons(self): - try: - return self.render( - "netbox_attachments/add_attachment_button.html", - extra_context={"object_type_attachment_list": url_pattern_name}, - ) - except Exception as e: - logger.error(f"Failed to render add attachment button for {model_name}: {e}") - return "" + return _render_or_empty( + self, + "netbox_attachments/add_attachment_button.html", + model_name, + extra_context={"object_type_attachment_list": url_pattern_name}, + ) return AddAttachmentButton @@ -122,33 +155,64 @@ def get_children(self, request, parent): return view_name -def discover_custom_object_models(): - try: - from django.apps import apps +def render_custom_object_panel(extension, position: str) -> str: + """ + Render the attachment panel for `position` if the object in context is an in-scope + custom object configured to display there; otherwise return an empty string. + + Ordered cheapest-check-first: a global extension is offered every object in NetBox, + so non-custom-object pages must exit on a string compare before any settings or + database work. The CustomObjectType name lookups further down are cached per + generated class (see custom_object_identifier). + """ + obj = extension.context.get("object") + model = type(obj) + # NetBox hands global extensions whatever is in context — None, a non-model, or + # even a model class (whose type() is the metaclass). Everything downstream reads + # the CLASS's _meta, so that is what must exist; checking obj._meta instead would + # let a model class through and crash on ModelBase._meta. + if not hasattr(model, "_meta"): + return "" - custom_objects_app = apps.get_app_config("netbox_custom_objects") - all_models = list(custom_objects_app.get_models()) + if not is_custom_object_model(model): + return "" - logger.debug(f"Found {len(all_models)} total models in netbox_custom_objects app") + if resolve_display_preference_for_model(model) != position: + return "" - custom_object_models = [m for m in all_models if is_custom_object_model(m)] + if not validate_object_type(model): + return "" - logger.info(f"Discovered {len(custom_object_models)} custom object models for attachments") + return _render_or_empty(extension, ATTACHMENT_PANEL_TEMPLATE, model._meta.label_lower) - return custom_object_models - except LookupError: - logger.info("NetBox Custom Objects plugin not found - custom objects support disabled") - return [] - except ImportError as e: - logger.warning(f"Could not import netbox_custom_objects: {e}") - return [] - except Exception as e: - logger.error( - f"Unexpected error discovering custom object models: {e}", - exc_info=True, - ) - return [] +def create_custom_object_attachment_panel(): + """ + Build the globally registered extension that renders attachment panels on custom + object detail pages. + + Custom object models cannot be enumerated at import time: netbox_custom_objects + withholds its dynamic models until its own ready() has finished, and plugin ready() + order follows PLUGINS, so they are absent whenever this plugin loads first (issue + #110). Registering with models = None defers the decision to render time, when the + models reliably exist — which also lets object types created after startup work + without a NetBox restart. + """ + from netbox.plugins import PluginTemplateExtension + + class CustomObjectAttachmentPanel(PluginTemplateExtension): + models = None + + def left_page(self): + return render_custom_object_panel(self, "left_page") + + def right_page(self): + return render_custom_object_panel(self, "right_page") + + def full_width_page(self): + return render_custom_object_panel(self, "full_width_page") + + return CustomObjectAttachmentPanel def get_template_extensions() -> List[Type]: @@ -160,7 +224,10 @@ def get_template_extensions() -> List[Type]: except Exception: return [] - extensions = [] + # Registered up front so custom object support survives any failure below: it + # enumerates no models and touches no database. Self-gating, so it is also safe + # to register without netbox_custom_objects installed. + extensions = [create_custom_object_attachment_panel()] try: plugin_settings = _get_plugin_settings() @@ -170,18 +237,15 @@ def get_template_extensions() -> List[Type]: logger.warning("Invalid create_add_button value, defaulting to True") should_add_button = True + # Custom objects are deliberately not collected here; CustomObjectAttachmentPanel + # handles them at render time. all_models = list(apps.get_models()) logger.debug(f"Found {len(all_models)} standard Django models") - custom_object_models = discover_custom_object_models() - if custom_object_models: - all_models.extend(custom_object_models) - logger.info(f"Added {len(custom_object_models)} custom object models to processing queue") - seen_models = set() unique_models = [] for model in all_models: - model_id = f"{model._meta.app_label}.{model._meta.model_name}" + model_id = model._meta.label_lower if model_id not in seen_models: seen_models.add(model_id) unique_models.append(model) @@ -193,18 +257,22 @@ def get_template_extensions() -> List[Type]: ) for model in unique_models: + # When netbox_custom_objects loads first its dynamic models are already in + # apps.get_models(); a per-model extension here would render a second panel + # alongside the global one, and the additional_tab branch below must never + # see them (their detail pages cannot host tabs or top buttons). + # Consolidating the two mechanisms is tracked in issue #112. + if is_custom_object_model(model): + continue + if not validate_object_type(model): continue app_label = model._meta.app_label model_name = model._meta.model_name - app_model_name = f"{app_label}.{model_name}" + app_model_name = model._meta.label_lower - display_preference = resolve_effective_display_preference( - app_model_name, - is_custom_object=is_custom_object_model(model), - plugin_settings=plugin_settings, - ) + display_preference = resolve_display_preference_for_model(model, plugin_settings=plugin_settings) if display_preference == "additional_tab": view_name = register_attachment_tab_view(model) diff --git a/netbox_attachments/tests/conftest.py b/netbox_attachments/tests/conftest.py new file mode 100644 index 0000000..ba1c759 --- /dev/null +++ b/netbox_attachments/tests/conftest.py @@ -0,0 +1,26 @@ +"""Shared fakes for the standalone pytest suite.""" + +from types import SimpleNamespace + + +def fake_model(app_label: str, model_name: str): + """ + A stand-in model class whose _meta mirrors every attribute production code reads. + + One definition on purpose: when the code under test starts reading a new _meta + attribute (as label_lower was added in #111), it is added here once instead of + being chased through per-file copies. Call it for a class, call the result for + an instance: fake_model("dcim", "device")(). + """ + return type( + "FakeModel", + (), + { + "_meta": SimpleNamespace( + app_label=app_label, + model_name=model_name, + label_lower=f"{app_label}.{model_name}", + abstract=False, + ) + }, + ) diff --git a/netbox_attachments/tests/test_checks.py b/netbox_attachments/tests/test_checks.py new file mode 100644 index 0000000..d119393 --- /dev/null +++ b/netbox_attachments/tests/test_checks.py @@ -0,0 +1,95 @@ +"""Unit tests for the removed-settings system check.""" + +from netbox_attachments import checks + + +def test_no_warnings_for_modern_config(monkeypatch): + monkeypatch.setattr( + checks, + "_get_plugin_settings", + lambda: { + "applied_scope": "app", + "scope_filter": ["dcim", "netbox_custom_objects"], + "display_default": "additional_tab", + }, + ) + + assert checks.check_removed_settings(None) == [] + + +def test_no_warnings_when_plugin_unconfigured(monkeypatch): + monkeypatch.setattr(checks, "_get_plugin_settings", lambda: {}) + + assert checks.check_removed_settings(None) == [] + + +def test_apps_setting_warns_and_names_replacement(monkeypatch): + """The config from issue #110: 'apps' is silently ignored without this check.""" + monkeypatch.setattr( + checks, + "_get_plugin_settings", + lambda: {"apps": ["dcim", "netbox_custom_objects"], "display_default": "additional_tab"}, + ) + + warnings = checks.check_removed_settings(None) + + assert len(warnings) == 1 + assert "'apps' setting was removed" in warnings[0].msg + assert "scope_filter" in warnings[0].hint + + +def test_allowed_models_setting_warns(monkeypatch): + monkeypatch.setattr(checks, "_get_plugin_settings", lambda: {"allowed_models": ["dcim.device"]}) + + warnings = checks.check_removed_settings(None) + + assert len(warnings) == 1 + assert "scope_filter" in warnings[0].hint + + +def test_mode_setting_warns_that_values_also_changed(monkeypatch): + monkeypatch.setattr(checks, "_get_plugin_settings", lambda: {"mode": "permissive"}) + + warnings = checks.check_removed_settings(None) + + assert len(warnings) == 1 + assert "applied_scope" in warnings[0].hint + assert "permissive" in warnings[0].hint + + +def test_check_ids_are_stable_not_positional(monkeypatch): + """W-ids are a published contract (SILENCED_SYSTEM_CHECKS); they must not renumber + when earlier table entries are absent from the user's config.""" + monkeypatch.setattr(checks, "_get_plugin_settings", lambda: {"mode": "permissive"}) + + warnings = checks.check_removed_settings(None) + + assert [w.id for w in warnings] == ["netbox_attachments.W003"] + + +def test_message_acknowledges_configured_replacement(monkeypatch): + """A half-migrated config (old and new key both set) must not claim the default is in use.""" + monkeypatch.setattr( + checks, + "_get_plugin_settings", + lambda: {"apps": ["dcim"], "scope_filter": ["dcim", "netbox_custom_objects"]}, + ) + + warnings = checks.check_removed_settings(None) + + assert len(warnings) == 1 + assert "Your configured 'scope_filter' is in effect" in warnings[0].msg + assert "default" not in warnings[0].msg + + +def test_each_removed_setting_warns_once_with_a_unique_id(monkeypatch): + monkeypatch.setattr( + checks, + "_get_plugin_settings", + lambda: {"apps": ["dcim"], "allowed_models": ["dcim.device"], "mode": "permissive"}, + ) + + warnings = checks.check_removed_settings(None) + + assert len(warnings) == 3 + assert len({w.id for w in warnings}) == 3 diff --git a/netbox_attachments/tests/test_configuration.py b/netbox_attachments/tests/test_configuration.py index b306527..b64289f 100644 --- a/netbox_attachments/tests/test_configuration.py +++ b/netbox_attachments/tests/test_configuration.py @@ -1,13 +1,7 @@ """Unit tests for standalone pytest execution.""" -from types import SimpleNamespace - from netbox_attachments import utils - - -class FakeModel: - def __init__(self, app_label, model_name): - self._meta = SimpleNamespace(app_label=app_label, model_name=model_name, abstract=False) +from netbox_attachments.tests.conftest import fake_model class FakeInstance: @@ -52,9 +46,9 @@ def test_validate_object_type_app_scope(monkeypatch): lambda: {"applied_scope": "app", "scope_filter": ["dcim", "ipam"]}, ) - assert utils.validate_object_type(FakeModel("dcim", "device")) is True - assert utils.validate_object_type(FakeModel("ipam", "ipaddress")) is True - assert utils.validate_object_type(FakeModel("tenancy", "tenant")) is False + assert utils.validate_object_type(fake_model("dcim", "device")) is True + assert utils.validate_object_type(fake_model("ipam", "ipaddress")) is True + assert utils.validate_object_type(fake_model("tenancy", "tenant")) is False def test_validate_object_type_model_scope_specific_and_mixed(monkeypatch): @@ -67,10 +61,10 @@ def test_validate_object_type_model_scope_specific_and_mixed(monkeypatch): }, ) - assert utils.validate_object_type(FakeModel("dcim", "device")) is True - assert utils.validate_object_type(FakeModel("ipam", "ipaddress")) is True - assert utils.validate_object_type(FakeModel("virtualization", "cluster")) is True - assert utils.validate_object_type(FakeModel("ipam", "prefix")) is False + assert utils.validate_object_type(fake_model("dcim", "device")) is True + assert utils.validate_object_type(fake_model("ipam", "ipaddress")) is True + assert utils.validate_object_type(fake_model("virtualization", "cluster")) is True + assert utils.validate_object_type(fake_model("ipam", "prefix")) is False def test_validate_object_type_invalid_config_graceful(monkeypatch): @@ -80,7 +74,7 @@ def test_validate_object_type_invalid_config_graceful(monkeypatch): lambda: {"applied_scope": "invalid", "scope_filter": "dcim"}, ) - assert utils.validate_object_type(FakeModel("dcim", "device")) is False + assert utils.validate_object_type(fake_model("dcim", "device")) is False def test_validate_object_type_empty_scope_disables_all(monkeypatch): @@ -90,5 +84,5 @@ def test_validate_object_type_empty_scope_disables_all(monkeypatch): lambda: {"applied_scope": "model", "scope_filter": []}, ) - assert utils.validate_object_type(FakeModel("dcim", "device")) is False - assert utils.validate_object_type(FakeModel("ipam", "ipaddress")) is False + assert utils.validate_object_type(fake_model("dcim", "device")) is False + assert utils.validate_object_type(fake_model("ipam", "ipaddress")) is False diff --git a/netbox_attachments/tests/test_new_features.py b/netbox_attachments/tests/test_new_features.py index e4211b8..eb7c1b6 100644 --- a/netbox_attachments/tests/test_new_features.py +++ b/netbox_attachments/tests/test_new_features.py @@ -323,11 +323,35 @@ def test_object_attachment_for_object_table_tags_in_default_columns(): pytest.fail("NetBoxAttachmentForObjectTable.Meta.default_columns not found") +def _queryset_calls(source, class_name): + """ + Map method name -> its string arguments for `class_name`'s queryset chain. + + Parsed rather than matched as text: where the formatter chooses to break the chain + says nothing about what it does, and a reformat should not fail this test. + """ + for node in ast.walk(ast.parse(source)): + if not (isinstance(node, ast.ClassDef) and node.name == class_name): + continue + for stmt in node.body: + targets = getattr(stmt, "targets", []) + if not (isinstance(stmt, ast.Assign) and any(getattr(t, "id", None) == "queryset" for t in targets)): + continue + calls = {} + expr = stmt.value + while isinstance(expr, ast.Call) and isinstance(expr.func, ast.Attribute): + calls[expr.func.attr] = tuple(a.value for a in expr.args if isinstance(a, ast.Constant)) + expr = expr.func.value + return calls + return {} + + def test_views_py_assignment_list_prefetches_tags(): """NetBoxAttachmentAssignmentListView queryset must select_related FKs and prefetch 'tags'.""" - source = _VIEWS_PY.read_text() - assert 'select_related("attachment", "object_type")' in source - assert '.prefetch_related("tags")' in source + calls = _queryset_calls(_VIEWS_PY.read_text(), "NetBoxAttachmentAssignmentListView") + + assert calls.get("select_related") == ("attachment", "object_type") + assert "tags" in calls.get("prefetch_related", ()) def test_views_py_panel_list_prefetches_tags(): diff --git a/netbox_attachments/tests/test_template_extensions.py b/netbox_attachments/tests/test_template_extensions.py index 5fa76a0..6a3705c 100644 --- a/netbox_attachments/tests/test_template_extensions.py +++ b/netbox_attachments/tests/test_template_extensions.py @@ -1,15 +1,50 @@ """Unit tests for template display decision helpers.""" from netbox_attachments import template_content +from netbox_attachments.tests.conftest import fake_model -def test_get_display_preference_uses_default_when_unset(monkeypatch): +class FakeExtension: + """Stands in for PluginTemplateExtension: the two attributes the panel body touches.""" + + def __init__(self, obj, raises=False): + self.context = {"object": obj} + self.raises = raises + self.rendered = [] + + def render(self, template_name, extra_context=None): + if self.raises: + raise RuntimeError("template blew up") + self.rendered.append(template_name) + return f"" + + +def make_object(app_label="netbox_custom_objects", model_name="table3model"): + """An instance whose type() carries _meta, since the panel inspects the class.""" + return fake_model(app_label, model_name)() + + +def stub_custom_object_support(monkeypatch, *, is_custom_object=True, in_scope=True, settings=None): + """ + Pin the collaborators the panel gates on. The real ones need netbox_custom_objects + installed, which the standalone pytest run does not have. + """ + monkeypatch.setattr(template_content, "is_custom_object_model", lambda model: is_custom_object) + monkeypatch.setattr(template_content, "validate_object_type", lambda model: in_scope) + monkeypatch.setattr( + template_content, + "_get_plugin_settings", + lambda: settings if settings is not None else {"display_default": "full_width_page"}, + ) + + +def test_display_preference_uses_default_when_unset(monkeypatch): monkeypatch.setattr(template_content, "_get_plugin_settings", lambda: {}) - assert template_content.get_display_preference("dcim.device") == "additional_tab" + assert template_content.resolve_effective_display_preference("dcim.device") == "additional_tab" -def test_get_display_preference_uses_model_override(monkeypatch): +def test_display_preference_uses_model_override(monkeypatch): monkeypatch.setattr( template_content, "_get_plugin_settings", @@ -19,8 +54,17 @@ def test_get_display_preference_uses_model_override(monkeypatch): }, ) - assert template_content.get_display_preference("dcim.device") == "left_page" - assert template_content.get_display_preference("dcim.site") == "right_page" + assert template_content.resolve_effective_display_preference("dcim.device") == "left_page" + assert template_content.resolve_effective_display_preference("dcim.site") == "right_page" + + +def test_resolver_forces_full_width_for_custom_objects_at_the_resolver_layer(monkeypatch): + """The no-tab-for-custom-objects rule must hold for ANY caller of the shared + resolver — not just because the startup loop happens to skip custom objects.""" + stub_custom_object_support(monkeypatch, settings={"display_default": "additional_tab"}) + model = fake_model("netbox_custom_objects", "table3model") + + assert template_content.resolve_display_preference_for_model(model) == "full_width_page" def test_resolve_effective_display_preference_for_custom_object_auto_converts(): @@ -54,3 +98,144 @@ def test_get_template_extensions_returns_empty_outside_netbox_runtime(): assert isinstance(extensions, list) assert extensions == [] + + +def test_custom_object_panel_renders_at_configured_position(monkeypatch): + stub_custom_object_support(monkeypatch, settings={"display_default": "full_width_page"}) + extension = FakeExtension(make_object()) + + assert template_content.render_custom_object_panel(extension, "full_width_page") == ( + "" + ) + assert template_content.render_custom_object_panel(extension, "left_page") == "" + assert template_content.render_custom_object_panel(extension, "right_page") == "" + + +def test_custom_object_panel_honours_left_page_setting(monkeypatch): + stub_custom_object_support(monkeypatch, settings={"display_default": "left_page"}) + extension = FakeExtension(make_object()) + + assert template_content.render_custom_object_panel(extension, "left_page") != "" + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + + +def test_custom_object_panel_falls_back_from_additional_tab_to_full_width(monkeypatch): + """Custom object pages cannot host a tab, so additional_tab must land on full_width_page.""" + stub_custom_object_support(monkeypatch, settings={"display_default": "additional_tab"}) + extension = FakeExtension(make_object()) + + assert template_content.render_custom_object_panel(extension, "full_width_page") != "" + assert template_content.render_custom_object_panel(extension, "left_page") == "" + + +def test_custom_object_panel_skips_non_custom_objects(monkeypatch): + stub_custom_object_support(monkeypatch, is_custom_object=False) + extension = FakeExtension(make_object("dcim", "device")) + + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + assert extension.rendered == [] + + +def test_custom_object_panel_skips_out_of_scope_custom_objects(monkeypatch): + stub_custom_object_support(monkeypatch, in_scope=False) + extension = FakeExtension(make_object()) + + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + + +def test_custom_object_panel_skips_when_no_object_in_context(monkeypatch): + """PluginTemplateExtension.render reads context['object'], which can be absent or None.""" + stub_custom_object_support(monkeypatch) + extension = FakeExtension(None) + + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + + +def test_custom_object_panel_skips_objects_without_meta(monkeypatch): + """NetBox offers global extensions whatever is in context; it need not be a model.""" + stub_custom_object_support(monkeypatch) + extension = FakeExtension("not a model") + + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + + +def test_custom_object_panel_declines_model_classes(monkeypatch): + """A model CLASS in context must be declined, not crash: type(cls) is the metaclass, + which has no _meta — the guard must test the class the code actually reads.""" + stub_custom_object_support(monkeypatch) + model_class = type(make_object()) + extension = FakeExtension(model_class) + + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + assert extension.rendered == [] + + +def test_custom_object_panel_swallows_render_errors(monkeypatch): + stub_custom_object_support(monkeypatch) + extension = FakeExtension(make_object(), raises=True) + + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + + +def test_custom_object_panel_display_setting_uses_the_type_name_key(monkeypatch): + """ + display_setting must accept the identifier scope_filter uses (the type name), not the + internal table*model name, or the documented key silently never matches. + """ + stub_custom_object_support( + monkeypatch, + settings={ + "display_default": "full_width_page", + "display_setting": {"netbox_custom_objects.cotab2_asset": "left_page"}, + }, + ) + monkeypatch.setattr( + template_content, + "custom_object_identifier", + lambda model: "netbox_custom_objects.cotab2_asset", + ) + extension = FakeExtension(make_object(model_name="table134model")) + + assert template_content.render_custom_object_panel(extension, "left_page") != "" + assert template_content.render_custom_object_panel(extension, "full_width_page") == "" + + +def test_custom_object_panel_skips_identifier_lookup_without_display_setting(monkeypatch): + """Resolving the type name costs a query, so it must not run when no override exists.""" + calls = [] + stub_custom_object_support(monkeypatch, settings={"display_default": "full_width_page"}) + monkeypatch.setattr(template_content, "custom_object_identifier", lambda model: calls.append(model) or None) + extension = FakeExtension(make_object()) + + assert template_content.render_custom_object_panel(extension, "full_width_page") != "" + assert calls == [] + + +def test_custom_object_panel_falls_back_when_identifier_unresolvable(monkeypatch): + stub_custom_object_support( + monkeypatch, + settings={ + "display_default": "full_width_page", + "display_setting": {"netbox_custom_objects.something_else": "left_page"}, + }, + ) + monkeypatch.setattr(template_content, "custom_object_identifier", lambda model: None) + extension = FakeExtension(make_object()) + + assert template_content.render_custom_object_panel(extension, "full_width_page") != "" + + +def test_custom_object_panel_defers_scope_check_until_position_matches(monkeypatch): + """ + validate_object_type can query the database, so it must run only for the one hook + that will render — not once per hook, and never on non-custom-object pages. + """ + calls = [] + stub_custom_object_support(monkeypatch, settings={"display_default": "full_width_page"}) + monkeypatch.setattr(template_content, "validate_object_type", lambda model: calls.append(model) or True) + extension = FakeExtension(make_object()) + + for position in ("left_page", "right_page", "full_width_page"): + template_content.render_custom_object_panel(extension, position) + + assert len(calls) == 1 diff --git a/netbox_attachments/utils.py b/netbox_attachments/utils.py index 1b6f0e0..0b2fac5 100644 --- a/netbox_attachments/utils.py +++ b/netbox_attachments/utils.py @@ -1,3 +1,4 @@ +from functools import lru_cache from pathlib import Path from django.conf import settings @@ -84,6 +85,45 @@ def is_custom_object_model(model): return False +def custom_object_identifier(model): + """ + Return the config identifier for a custom object model, or None if unresolvable. + + Custom object models are named after their backing table's primary key + ("table134model"), which is opaque and differs between installs, so settings key off + the Custom Object Type's name instead: "netbox_custom_objects.cotab2_asset". Both + scope_filter and display_setting must use this, or one of them silently never matches. + """ + if not is_custom_object_model(model): + return None + + return _resolve_custom_object_identifier(model) + + +@lru_cache(maxsize=256) +def _resolve_custom_object_identifier(model): + """ + CustomObjectType lookup, cached per generated model class. + + The scope check and the display-key resolution can each need this during one page + render; uncached that is repeated identical queries. Caching on the class is sound + because netbox_custom_objects regenerates the dynamic class whenever its + CustomObjectType is saved (a rename means a new class object, i.e. a new cache + key). A queryset .update() bypasses that signal and would serve a stale name until + restart — the same tradeoff the CO plugin's own model cache makes. Only custom + object classes reach this (the public wrapper filters), so standard models never + churn the LRU. + """ + try: + from netbox_custom_objects.models import CustomObjectType + + cot = CustomObjectType.objects.get(id=model.custom_object_type_id) + except (ImportError, AttributeError, ObjectDoesNotExist): + return None + + return f"{model._meta.app_label}.{cot.name}" + + def validate_object_type(model): """ Determines if a Django model is permitted to have attachments. @@ -122,17 +162,13 @@ def validate_object_type(model): if app_label in scope_filter: return True # Need model_identifier for the specific-model check. - # For custom objects, resolve the identifier via DB (only when necessary). + # For custom objects this resolves via DB, so it runs only when necessary. if is_custom_object_model(model): - try: - from netbox_custom_objects.models import CustomObjectType - - cot = CustomObjectType.objects.get(id=model.custom_object_type_id) - model_identifier = f"{model._meta.app_label}.{cot.name}" - except (ImportError, AttributeError, ObjectDoesNotExist): + model_identifier = custom_object_identifier(model) + if model_identifier is None: return False else: - model_identifier = f"{model._meta.app_label}.{model._meta.model_name}" + model_identifier = model._meta.label_lower return model_identifier in scope_filter return False @@ -156,7 +192,7 @@ def get_enabled_object_type_queryset(): # Standard models for model in apps.get_models(): - key = f"{model._meta.app_label}.{model._meta.model_name}" + key = model._meta.label_lower if key in seen: continue seen.add(key) @@ -167,7 +203,7 @@ def get_enabled_object_type_queryset(): try: custom_app = apps.get_app_config("netbox_custom_objects") for model in custom_app.get_models(): - key = f"{model._meta.app_label}.{model._meta.model_name}" + key = model._meta.label_lower if key in seen: continue seen.add(key) diff --git a/netbox_attachments/version.py b/netbox_attachments/version.py index b46cd04..80314c5 100644 --- a/netbox_attachments/version.py +++ b/netbox_attachments/version.py @@ -1 +1 @@ -__version__ = "11.2.3" +__version__ = "11.3.0" diff --git a/netbox_attachments/views.py b/netbox_attachments/views.py index 52cbe7a..cab7bd2 100644 --- a/netbox_attachments/views.py +++ b/netbox_attachments/views.py @@ -177,9 +177,8 @@ class NetBoxAttachmentAssignmentView(generic.ObjectView): @register_model_view(models.NetBoxAttachmentAssignment, name="list", path="", detail=False) class NetBoxAttachmentAssignmentListView(generic.ObjectListView): - queryset = ( - models.NetBoxAttachmentAssignment.objects.select_related("attachment", "object_type") - .prefetch_related("tags") + queryset = models.NetBoxAttachmentAssignment.objects.select_related("attachment", "object_type").prefetch_related( + "tags" ) table = tables.NetBoxAttachmentAssignmentTable filterset = filtersets.NetBoxAttachmentAssignmentFilterSet