[change] Made menu item registration idempotent #641

Previously, calling `register_menu_group` or `register_menu_subitem`
twice with the same configuration at the same position would always
raise an `ImproperlyConfigured` exception. This behavior prevented
re-registration, even when the same application was re-initializing its
menu items during testing.

This change modifies these functions to check if the configuration being
registered is identical to the one already present at that position. If
the configurations match, the function now logs an informational message
and skips the re-registration instead of raising an error. This makes
the menu item registration process idempotent, improving testability and
handling of repeated app initializations.

Closes #641

Author: Saksham Jain

docs/developer/navigation-menu.rst

@@ -97,8 +97,12 @@ Code example:
         position=3, config={"label": _("My Link"), "url": "https://link.com"}
     )
 
-An ``ImproperlyConfigured`` exception is raised if a menu element is
-already registered at the same position.
+If the same ``config`` is registered at the same position more than once
+(e.g. because ``AppConfig.ready()`` is called again during testing), the
+call is a no-op and a log message is emitted at ``INFO`` level.
+
+An ``ImproperlyConfigured`` exception is raised if a *different* menu
+element is already registered at the same position.
 
 An ``ImproperlyConfigured`` exception is raised if the supplied
 configuration does not match with the different types of possible
@@ -278,8 +282,10 @@ Code example:
 An ``ImproperlyConfigured`` exception is raised if the group is not
 already registered at ``group_position``.
 
-An ``ImproperlyConfigured`` exception is raised if the group already has
-an item registered at ``item_position``.
+If the same ``config`` is registered at the same ``item_position`` more
+than once, the call is a no-op and a log message is emitted at ``INFO``
+level. An ``ImproperlyConfigured`` exception is raised if a *different*
+item is already registered at ``item_position``.
 
 It is only possible to register links to specific models or custom URL. An
 ``ImproperlyConfigured`` exception is raised if the configuration of group

openwisp_utils/admin_theme/menu.py

@@ -1,10 +1,14 @@
+import logging
+
 from django.apps import registry
 from django.core.exceptions import ImproperlyConfigured
 from django.urls import reverse
 from django.urls.exceptions import NoReverseMatch
 
 from ..utils import SortedOrderedDict
 
+logger = logging.getLogger(__name__)
+
 MENU = SortedOrderedDict()
 
 
@@ -19,6 +23,7 @@ def __init__(self, config):
             raise ImproperlyConfigured(
                 f'"config" should be a type of "dict". Error for config- {config}'
             )
+        self.config = config
 
     def get_context(self, request=None):
         return self.create_context(request)
@@ -60,7 +65,6 @@ def __init__(self, config):
         self.model = model
         self.set_label(config)
         self.icon = config.get("icon")
-        self.config = config
 
     def set_label(self, config=None):
         if config.get("label"):
@@ -185,6 +189,12 @@ def register_menu_group(position, config):
     if not isinstance(config, dict):
         raise ImproperlyConfigured('config should be a type of "dict"')
     if position in MENU:
+        if MENU[position].config == config:
+            logger.info(
+                f"A group/link with config {config} is already registered at"
+                f' position "{position}", skipping re-registration.'
+            )
+            return
         item_description = "link"
         if isinstance(MENU[position], MenuGroup):
             item_description = "group"
@@ -244,6 +254,13 @@ def register_menu_subitem(group_position, item_position, config):
             f'Invalid config "{config}" provided for sub group item'
         )
     if item_position in group.items:
+        if group.items[item_position].config == config:
+            logger.info(
+                f"A group item with config {config} is already registered at"
+                f' position "{item_position}" in the group at position'
+                f' "{group_position}", skipping re-registration.'
+            )
+            return
         name = group.items[item_position]
         raise ImproperlyConfigured(
             f'A group item with config {config} is being registered at position\

tests/test_project/tests/test_menu.py

@@ -88,6 +88,32 @@ def test_register_menu_groups(self):
             with self.assertRaises(ImproperlyConfigured):
                 register_menu_group(position=1, config=model_link_config)
 
+        with self.subTest(
+            "Registering identical config at an occupied position is idempotent"
+        ):
+            with self.assertLogs(
+                "openwisp_utils.admin_theme.menu", level="INFO"
+            ) as captured:
+                register_menu_group(position=1, config=menu_link_config)
+            self.assertIn("already registered", captured.output[0])
+            self.assertIsInstance(MENU[1], MenuLink)
+
+        with self.subTest("Re-registering identical ModelLink config is idempotent"):
+            with self.assertLogs(
+                "openwisp_utils.admin_theme.menu", level="INFO"
+            ) as captured:
+                register_menu_group(position=2, config=model_link_config)
+            self.assertIn("already registered", captured.output[0])
+            self.assertIsInstance(MENU[2], ModelLink)
+
+        with self.subTest("Re-registering identical MenuGroup config is idempotent"):
+            with self.assertLogs(
+                "openwisp_utils.admin_theme.menu", level="INFO"
+            ) as captured:
+                register_menu_group(position=3, config=menu_group_config)
+            self.assertIn("already registered", captured.output[0])
+            self.assertIsInstance(MENU[3], MenuGroup)
+
         with self.subTest("Registering with invalid position"):
             with self.assertRaises(ImproperlyConfigured):
                 register_menu_group(
@@ -143,7 +169,9 @@ def test_register_menu_subitem(self):
                 register_menu_subitem(group_position=100, item_position=1, config=[])
             with self.assertRaises(ImproperlyConfigured):
                 register_menu_subitem(
-                    group_position=100, item_position=1, config=config
+                    group_position=100,
+                    item_position=1,
+                    config=self._get_menu_link_config(label="a different label"),
                 )
             with self.assertRaises(ImproperlyConfigured):
                 register_menu_subitem(
@@ -156,6 +184,18 @@ def test_register_menu_subitem(self):
                     config=self._get_menu_group_config(),
                 )
 
+        with self.subTest(
+            "Registering identical config at an occupied item position is idempotent"
+        ):
+            with self.assertLogs(
+                "openwisp_utils.admin_theme.menu", level="INFO"
+            ) as captured:
+                register_menu_subitem(
+                    group_position=100, item_position=1, config=config
+                )
+            self.assertIn("already registered", captured.output[0])
+            self.assertIsInstance(MENU[100].items[1], MenuLink)
+
         with self.subTest("Test menu subitem with valid data"):
             model_link_config = self._get_model_link_config()
             register_menu_subitem(group_position=100, item_position=3, config=config)