From 845ca7efcf79af798ac3e61c18eb21dbaf6906ab Mon Sep 17 00:00:00 2001
From: Zeid Zabaneh <2043828+zzzeid@users.noreply.github.com>
Date: Wed, 27 Nov 2024 10:26:33 -0500
Subject: [PATCH] config: add alias check to submodules (bug 1933580) (#156)

---
 src/mots/config.py   | 18 +++++++++++++-----
 tests/test_config.py | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/src/mots/config.py b/src/mots/config.py
index 10c9a25..c69013d 100644
--- a/src/mots/config.py
+++ b/src/mots/config.py
@@ -186,6 +186,15 @@ def calculate_hashes(config: dict, export: bytes) -> tuple[dict, dict]:
     return original_hashes, hashes
 
 
+def raise_if_nick_is_invalid(person):
+    """Check if provided person dictionary is valid."""
+    if "bmo_id" not in person and person.get("nick") not in ALLOWED_NICK_ONLY:
+        raise ValueError(
+            f"Nick must be one of {', '.join(ALLOWED_NICK_ONLY)} when "
+            "provided without bmo_id."
+        )
+
+
 def clean(file_config: FileConfig, write: bool = True, refresh: bool = True):
     """Clean and re-sort configuration file.
 
@@ -250,11 +259,7 @@ def clean(file_config: FileConfig, write: bool = True, refresh: bool = True):
                 continue
             for i, person in enumerate(module[key]):
                 if "bmo_id" not in person:
-                    if person.get("nick") not in ALLOWED_NICK_ONLY:
-                        raise ValueError(
-                            f"Nick must be one of {', '.join(ALLOWED_NICK_ONLY)} when "
-                            "provided without bmo_id."
-                        )
+                    raise_if_nick_is_invalid(person)
                     continue
                 reference_anchor_for_module(
                     i, person, key, file_config, directory, module
@@ -289,6 +294,9 @@ def clean(file_config: FileConfig, write: bool = True, refresh: bool = True):
                     if key not in submodule or not submodule[key]:
                         continue
                     for i, person in enumerate(submodule[key]):
+                        if "bmo_id" not in person:
+                            raise_if_nick_is_invalid(person)
+                            continue
                         reference_anchor_for_module(
                             i, person, key, file_config, directory, submodule
                         )
diff --git a/tests/test_config.py b/tests/test_config.py
index 8e92cc2..ad8fb81 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -299,6 +299,9 @@ def test_clean_added_user_no_bmo_id_with_refresh(
 
     assert file_config.config["people"] == config["people"]
     file_config.config["modules"][0]["owners"].append({"nick": "TLMC"})
+    file_config.config["modules"][0]["submodules"][0]["owners"].append(
+        {"nick": "TLMC"}
+    )
     file_config.write()
 
     assert len(file_config.config["people"]) == 4
@@ -345,6 +348,8 @@ def test_clean_added_user_no_bmo_id_with_refresh(
     assert modules[0]["owners"][1] == {"nick": "TLMC"}
     assert modules[1]["owners"][0] == people[2]
 
+    assert modules[0]["submodules"][0]["owners"][1] == {"nick": "TLMC"}
+
 
 @mock.patch("mots.config.get_bmo_data")
 def test_clean_added_user_no_bmo_id_with_refresh_invalid_nick(
@@ -362,3 +367,17 @@ def test_clean_added_user_no_bmo_id_with_refresh_invalid_nick(
     # Run clean with refresh.
     with pytest.raises(ValueError):
         clean(file_config, refresh=True)
+
+    file_config.config["modules"][0]["owners"].pop()
+    file_config.write()
+
+    clean(file_config, refresh=True)
+
+    file_config.config["modules"][0]["submodules"][0]["owners"].append(
+        {"nick": "ABCD"}
+    )
+    file_config.write()
+
+    # Run clean with refresh, this time after changing a submodule.
+    with pytest.raises(ValueError):
+        clean(file_config, refresh=True)