config: allow plain text in owners/peers field (bug 1923018) (#152)

- skip records with no bmo_id when generating yaml anchors/references
- make all Person attributes optional
- use nick for hash when bmo_id is not set
- allow null values in bmo_id
- restrict which nicks can be set without a bmo_id

src/mots/config.py

@@ -42,6 +42,11 @@
     )
 )
 
+# A list of nicks allowed to be used without a bugzilla ID.
+ALLOWED_NICK_ONLY = [
+    "TLMC",
+]
+
 
 class ValidationError(TypeError):
     """Thrown when a particular module is not valid."""
@@ -244,6 +249,13 @@ def clean(file_config: FileConfig, write: bool = True, refresh: bool = True):
             if key not in module or not module[key]:
                 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."
+                        )
+                    continue
                 reference_anchor_for_module(
                     i, person, key, file_config, directory, module
                 )

src/mots/directory.py

@@ -14,7 +14,7 @@
 from mots.module import Module
 from mots.utils import parse_real_name
 
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Optional
 
 if TYPE_CHECKING:
     from mots.config import FileConfig
@@ -192,13 +192,13 @@ def __radd__(self, query_result: "QueryResult") -> "QueryResult":
 class Person:
     """A class representing a person."""
 
-    bmo_id: int
-    name: str
-    nick: str
+    bmo_id: Optional[int] = None
+    name: Optional[str] = ""
+    nick: Optional[str] = ""
 
     def __hash__(self):
         """Return a unique identifier for this person."""
-        return self.bmo_id
+        return self.bmo_id or hash(self.nick)
 
 
 class People:
@@ -215,7 +215,9 @@ def __init__(self, people: list[dict], bmo_data: dict):
         for i, person in enumerate(people):
             logger.debug(f"Adding person {person} to roster...")
 
-            bmo_id = person["bmo_id"] = int(person["bmo_id"])
+            bmo_id = person["bmo_id"] = (
+                int(person["bmo_id"]) if "bmo_id" in person else None
+            )
             if bmo_id in bmo_data and bmo_data[bmo_id]:
                 # Update person's data base on BMO data.
                 bmo_datum = bmo_data[person["bmo_id"]]

src/mots/export.py

@@ -107,8 +107,10 @@ def format_people_for_rst(value: list[dict], indent: int) -> str:
             parsed_person = format_link_for_rst(
                 f"{person['name']} ({person['nick']})", url
             )
-        else:
+        elif "bmo_id" in person:
             parsed_person = format_link_for_rst(person["nick"], url)
+        else:
+            parsed_person = person["nick"]
 
         parsed_people.append(parsed_person)
     return f"\n{' ' * indent}| " + f"\n{' ' * indent}| ".join(parsed_people)
@@ -124,8 +126,10 @@ def format_people_for_md(value: list[dict], indent: int) -> str:
             parsed_person = format_link_for_md(
                 f"{person['name']} ({person['nick']})", url
             )
-        else:
+        elif "bmo_id" in person:
             parsed_person = format_link_for_md(person["nick"], url)
+        else:
+            parsed_person = person["nick"]
 
         parsed_people.append(parsed_person)
     return f"\n{' ' * indent}* " + f"\n{' ' * indent}* ".join(parsed_people)

tests/test_config.py

@@ -286,3 +286,79 @@ def test_clean_added_user_with_refresh(get_bmo_data, repo, config, test_bmo_user
         "name": "tom paris",
         "nick": "paris",
     }, explain_assert_all
+
+
+@mock.patch("mots.config.get_bmo_data")
+def test_clean_added_user_no_bmo_id_with_refresh(
+    get_bmo_data, repo, config, test_bmo_user_data
+):
+    get_bmo_data.return_value = test_bmo_user_data
+
+    file_config = FileConfig(repo / "mots.yml")
+    file_config.load()
+
+    assert file_config.config["people"] == config["people"]
+    file_config.config["modules"][0]["owners"].append({"nick": "TLMC"})
+    file_config.write()
+
+    assert len(file_config.config["people"]) == 4
+
+    # Compare the old and new people lists without the new entry, they should match.
+    new = sorted(
+        [dict(person) for person in file_config.config["people"]],
+        key=itemgetter("bmo_id"),
+    )
+    original = sorted(config["people"], key=itemgetter("bmo_id"))
+    assert new == original
+
+    # Run clean with refresh.
+    clean(file_config, refresh=True)
+
+    # It is expected that all people will have been updated via BMO, except the ones
+    # without a bmo_id.
+    explain_assert_all = "Record should have been updated after clean."
+    people = file_config.config["people"]
+    assert len(people) == 4
+    assert people[0] == {
+        "bmo_id": 1,
+        "name": "tuvok",
+        "nick": "2vk",
+    }, explain_assert_all
+    assert people[1] == {
+        "bmo_id": 0,
+        "name": "janeway",
+        "nick": "captain",
+    }, explain_assert_all
+    assert people[2] == {
+        "bmo_id": 2,
+        "name": "neelix",
+        "nick": "cooks4u",
+    }, explain_assert_all
+    assert people[3] == {
+        "bmo_id": 4,
+        "name": "tom paris",
+        "nick": "paris",
+    }, explain_assert_all
+
+    modules = file_config.config["modules"]
+    assert modules[0]["owners"][0] == people[1]
+    assert modules[0]["owners"][1] == {"nick": "TLMC"}
+    assert modules[1]["owners"][0] == people[2]
+
+
+@mock.patch("mots.config.get_bmo_data")
+def test_clean_added_user_no_bmo_id_with_refresh_invalid_nick(
+    get_bmo_data, repo, config, test_bmo_user_data
+):
+    get_bmo_data.return_value = test_bmo_user_data
+
+    file_config = FileConfig(repo / "mots.yml")
+    file_config.load()
+
+    assert file_config.config["people"] == config["people"]
+    file_config.config["modules"][0]["owners"].append({"nick": "ABCD"})
+    file_config.write()
+
+    # Run clean with refresh.
+    with pytest.raises(ValueError):
+        clean(file_config, refresh=True)

tests/test_export.py

@@ -140,7 +140,7 @@ def test_export_format_people_for_rst(config):
         "\n        | `jill (jill) <https://people.mozilla.org/s?query=jill>`__"
         "\n        | `otis (otis) <https://people.mozilla.org/s?query=otis>`__"
         "\n        | `angel (angel) <https://people.mozilla.org/s?query=angel>`__"
-        "\n        | `unnamed <https://people.mozilla.org/s?query=unnamed>`__"
+        "\n        | unnamed"
     )
 
 
@@ -153,7 +153,7 @@ def test_export_format_people_for_md(config):
         "\n    * [jill (jill)](https://people.mozilla.org/s?query=jill)"
         "\n    * [otis (otis)](https://people.mozilla.org/s?query=otis)"
         "\n    * [angel (angel)](https://people.mozilla.org/s?query=angel)"
-        "\n    * [unnamed](https://people.mozilla.org/s?query=unnamed)"
+        "\n    * unnamed"
     )
 
 

tests/test_module.py

@@ -97,6 +97,35 @@ def test_module__Module__submodule_with_no_peers_or_owners(repo):
     assert len(m.submodules[0].owners) == 0
 
 
+def test_module__Module__with_peers_and_owners_without_bmo_id(repo):
+    """Ensure a module's string peers/owners are ignored.
+
+    Only references to people listed in the directory should be stored
+    in the module itself. However, the export should include the plain
+    text references.
+    """
+    m = dict(
+        name="Some Module",
+        machine_name="some_module",
+        owners=[{"nick": "otis", "bmo_id": 2}, {"nick": "owner string"}],
+        peers=[{"nick": "jill", "bmo_id": 1}, {"nick": "peer string"}],
+        includes="*",
+        submodules=[
+            dict(
+                name="Submodule",
+                machine_name="submodule",
+                peers=[{"nick": "submodule peer string"}],
+                excludes="some_path",
+            ),
+        ],
+    )
+    m = Module(**m, repo_path=str(repo))
+    assert len(m.peers) == 2
+    assert len(m.owners) == 2
+    assert len(m.submodules[0].peers) == 1
+    assert len(m.submodules[0].owners) == 0
+
+
 def test_module__Module__validate__invalid_machine_name(repo):
     """Ensure an error is thrown when a submodule has an invalid machine name."""
     m = dict(