From 95caffb1642d52cdda481d3515b36b9417c5d9cf Mon Sep 17 00:00:00 2001 From: Mark Coleman Date: Mon, 19 Jan 2026 16:06:06 +0100 Subject: [PATCH] Refactor: centralize module token substitution logic Per sigprof's review feedback, extract the duplicated token substitution logic into a single resolve_module_token() helper in constants.py. This addresses two review comments: 1. Duplication between ModuleCommonForm.clean() and resolve_name() 2. Duplication between resolve_name() and resolve_label() Benefits: - Single source of truth for substitution logic - MODULE_TOKEN_SEPARATOR constant for future configurability - Cleaner, more maintainable code (-7 net lines) - Easier to modify separator handling in one place --- netbox/dcim/constants.py | 35 +++++++++++++ netbox/dcim/forms/common.py | 11 ++-- .../dcim/models/device_component_templates.py | 52 +++---------------- 3 files changed, 46 insertions(+), 52 deletions(-) diff --git a/netbox/dcim/constants.py b/netbox/dcim/constants.py index 669345d7c..19663f722 100644 --- a/netbox/dcim/constants.py +++ b/netbox/dcim/constants.py @@ -79,6 +79,41 @@ NONCONNECTABLE_IFACE_TYPES = VIRTUAL_IFACE_TYPES + WIRELESS_IFACE_TYPES # MODULE_TOKEN = '{module}' +MODULE_TOKEN_SEPARATOR = '/' + + +def resolve_module_token(text, positions): + """ + Substitute {module} tokens in text with position values. + + Args: + text: String potentially containing {module} tokens + positions: List of position strings from the module tree (root to leaf) + + Returns: + Text with {module} tokens replaced according to these rules: + - Single token: replaced with full path (positions joined by MODULE_TOKEN_SEPARATOR) + - Multiple tokens: replaced level-by-level (first token gets first position, etc.) + + This centralizes the substitution logic used by both ModuleCommonForm.clean() + and ModularComponentTemplateModel.resolve_*() methods. + """ + if not text or MODULE_TOKEN not in text: + return text + + token_count = text.count(MODULE_TOKEN) + + if token_count == 1: + # Single token: substitute with full path (e.g., "1/1" for depth 2) + full_path = MODULE_TOKEN_SEPARATOR.join(positions) + return text.replace(MODULE_TOKEN, full_path, 1) + else: + # Multiple tokens: substitute level-by-level (existing behavior) + result = text + for pos in positions: + result = result.replace(MODULE_TOKEN, pos, 1) + return result + MODULAR_COMPONENT_TEMPLATE_MODELS = Q( app_label='dcim', diff --git a/netbox/dcim/forms/common.py b/netbox/dcim/forms/common.py index fff2a1e00..0c9edaeb4 100644 --- a/netbox/dcim/forms/common.py +++ b/netbox/dcim/forms/common.py @@ -139,14 +139,9 @@ class ModuleCommonForm(forms.Form): ) ) - if token_count == 1: - # Single token: substitute with full path (e.g., "1/1" for depth 2) - full_path = '/'.join([mb.position for mb in module_bays]) - resolved_name = resolved_name.replace(MODULE_TOKEN, full_path, 1) - else: - # Multiple tokens: substitute level-by-level (existing behavior) - for mb in module_bays: - resolved_name = resolved_name.replace(MODULE_TOKEN, mb.position, 1) + # Use centralized helper for token substitution + positions = [mb.position for mb in module_bays] + resolved_name = resolve_module_token(resolved_name, positions) existing_item = installed_components.get(resolved_name) diff --git a/netbox/dcim/models/device_component_templates.py b/netbox/dcim/models/device_component_templates.py index 4ff4315c5..951f5453b 100644 --- a/netbox/dcim/models/device_component_templates.py +++ b/netbox/dcim/models/device_component_templates.py @@ -170,41 +170,17 @@ class ModularComponentTemplateModel(ComponentTemplateModel): return modules def resolve_name(self, module): - if MODULE_TOKEN not in self.name: - return self.name - + """Resolve {module} placeholder(s) in component name.""" if module: - modules = self._get_module_tree(module) - token_count = self.name.count(MODULE_TOKEN) - name = self.name - if token_count == 1: - # Single token: substitute with full path (e.g., "1/1" for depth 2) - full_path = '/'.join([m.module_bay.position for m in modules]) - name = name.replace(MODULE_TOKEN, full_path, 1) - else: - # Multiple tokens: substitute level-by-level (existing behavior) - for m in modules: - name = name.replace(MODULE_TOKEN, m.module_bay.position, 1) - return name + positions = [m.module_bay.position for m in self._get_module_tree(module)] + return resolve_module_token(self.name, positions) return self.name def resolve_label(self, module): - if MODULE_TOKEN not in self.label: - return self.label - + """Resolve {module} placeholder(s) in component label.""" if module: - modules = self._get_module_tree(module) - token_count = self.label.count(MODULE_TOKEN) - label = self.label - if token_count == 1: - # Single token: substitute with full path (e.g., "1/1" for depth 2) - full_path = '/'.join([m.module_bay.position for m in modules]) - label = label.replace(MODULE_TOKEN, full_path, 1) - else: - # Multiple tokens: substitute level-by-level (existing behavior) - for m in modules: - label = label.replace(MODULE_TOKEN, m.module_bay.position, 1) - return label + positions = [m.module_bay.position for m in self._get_module_tree(module)] + return resolve_module_token(self.label, positions) return self.label def resolve_position(self, position, module): @@ -216,21 +192,9 @@ class ModularComponentTemplateModel(ComponentTemplateModel): Fixes Issue #20467. """ - if not position or MODULE_TOKEN not in position: - return position - if module: - modules = self._get_module_tree(module) - token_count = position.count(MODULE_TOKEN) - if token_count == 1: - # Single token: substitute with full path - full_path = '/'.join([m.module_bay.position for m in modules]) - position = position.replace(MODULE_TOKEN, full_path, 1) - else: - # Multiple tokens: substitute level-by-level - for m in modules: - position = position.replace(MODULE_TOKEN, m.module_bay.position, 1) - return position + positions = [m.module_bay.position for m in self._get_module_tree(module)] + return resolve_module_token(position, positions) return position