From b57cabddedcaa2e07743af25d9352eb04c0d3bad Mon Sep 17 00:00:00 2001 From: Zeid Zabaneh <2043828+zzzeid@users.noreply.github.com> Date: Fri, 22 Nov 2024 08:57:34 -0500 Subject: [PATCH] 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 | 12 +++++++ src/mots/directory.py | 14 ++++---- src/mots/export.py | 8 +++-- tests/test_config.py | 76 +++++++++++++++++++++++++++++++++++++++++++ tests/test_export.py | 4 +-- tests/test_module.py | 29 +++++++++++++++++ 6 files changed, 133 insertions(+), 10 deletions(-) diff --git a/src/mots/config.py b/src/mots/config.py index e446445..10c9a25 100644 --- a/src/mots/config.py +++ b/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 ) diff --git a/src/mots/directory.py b/src/mots/directory.py index 12f3a05..8f25fe6 100644 --- a/src/mots/directory.py +++ b/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"]] diff --git a/src/mots/export.py b/src/mots/export.py index e8ac438..e321fd5 100644 --- a/src/mots/export.py +++ b/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) diff --git a/tests/test_config.py b/tests/test_config.py index 84cdf76..8e92cc2 100644 --- a/tests/test_config.py +++ b/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) diff --git a/tests/test_export.py b/tests/test_export.py index cf40199..66754f8 100644 --- a/tests/test_export.py +++ b/tests/test_export.py @@ -140,7 +140,7 @@ def test_export_format_people_for_rst(config): "\n | `jill (jill) `__" "\n | `otis (otis) `__" "\n | `angel (angel) `__" - "\n | `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" ) diff --git a/tests/test_module.py b/tests/test_module.py index e5b3b8e..09a6d1c 100644 --- a/tests/test_module.py +++ b/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(