Browse Source

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
Mark Coleman 3 weeks ago
parent
commit
1c6adc40b3

+ 35 - 0
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',

+ 3 - 8
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)
 

+ 8 - 44
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