From a887174a3bf3c1310261dd8a6580bc462ee08432 Mon Sep 17 00:00:00 2001 From: Zeid Zabaneh Date: Mon, 24 Oct 2022 10:36:19 -0400 Subject: [PATCH 1/2] config: ensure persons are initialized with required keys (bug 1797083) - make `Person.nick` and `Person.name` optional, default to empty string - serialize newly added persons before adding to people entry - add test to test Person class --- src/mots/config.py | 3 ++- src/mots/directory.py | 9 +++++++-- tests/conftest.py | 28 ++++++++++++++++++++++++++++ tests/test_config.py | 10 +++++++++- tests/test_directory.py | 18 ++++++++++++++++++ 5 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/mots/config.py b/src/mots/config.py index be0a9ca..5b576e8 100644 --- a/src/mots/config.py +++ b/src/mots/config.py @@ -16,7 +16,7 @@ from ruamel.yaml import YAML from mots.bmo import get_bmo_data -from mots.directory import Directory, People +from mots.directory import Directory, People, Person from mots.export import export_to_format from mots.module import Module from mots.utils import generate_machine_readable_name @@ -160,6 +160,7 @@ def clean(file_config: FileConfig, write: bool = True): if key not in module or not module[key]: continue for i, person in enumerate(module[key]): + person = Person(**person).serialize() if person["bmo_id"] not in directory.people.by_bmo_id: file_config.config["people"].append(person) module[key][i] = person diff --git a/src/mots/directory.py b/src/mots/directory.py index fac0280..d8b4b48 100644 --- a/src/mots/directory.py +++ b/src/mots/directory.py @@ -174,13 +174,17 @@ class Person: """A class representing a person.""" bmo_id: int - name: str - nick: str + name: str | None = "" + nick: str | None = "" def __hash__(self): """Return a unique identifier for this person.""" return self.bmo_id + def serialize(self): + """Return dictionary representation of Person.""" + return asdict(self) + class People: """A people directory searchable by name, email, or BMO ID.""" @@ -210,6 +214,7 @@ def __init__(self, people, bmo_data: dict): self.people.append(Person(**person)) self.by_bmo_id[person["bmo_id"]] = i logger.debug(f"Person {person} added to position {i}.") + self.serialized = [asdict(person) for person in self.people] def refresh_by_bmo_id(self): diff --git a/tests/conftest.py b/tests/conftest.py index 6a74398..efba297 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -121,3 +121,31 @@ def config(): }, ], } + + +@pytest.fixture +def config_with_bmo_ids_only(): + return { + "repo": "test_repo", + "created_at": "2021-09-10 12:53:22.383393", + "updated_at": "2021-09-10 12:53:22.383393", + "people": [], + "export": {"format": "rst", "path": "mots.rst"}, + "modules": [ + { + "machine_name": "domesticated_animals", + "exclude_submodule_paths": True, + "exclude_module_paths": True, + "includes": [ + "canines/**/*", + "felines/**/*", + "bovines/**/*", + "birds/**/*", + "pigs/**/*", + ], + "excludes": ["canines/red_fox"], + "owners": [{"bmo_id": 1}], + "peers": [{"bmo_id": 1}], + } + ], + } diff --git a/tests/test_config.py b/tests/test_config.py index 95ff488..9707e34 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -4,7 +4,7 @@ """Test config module.""" -from mots.config import calculate_hashes, FileConfig +from mots.config import calculate_hashes, FileConfig, clean def test_calculate_hashes(config): @@ -32,3 +32,11 @@ def test_FileConfig__check_hashes(repo): "da39a3ee5e6b4b0d3255bfef95601890afd80709 does not match ghjk", "export file is out of date.", ] + + +def test_clean_with_no_nick(repo, config_with_bmo_ids_only): + """Ensures clean function runs without any errors if nick is missing.""" + file_config = FileConfig(repo / "mots.yml") + file_config.config = config_with_bmo_ids_only + file_config.write() + clean(file_config) diff --git a/tests/test_directory.py b/tests/test_directory.py index e08b791..8901c52 100644 --- a/tests/test_directory.py +++ b/tests/test_directory.py @@ -4,8 +4,11 @@ """Tests for directory module.""" +import pytest + from mots.directory import Directory, Person, QueryResult from mots.module import Module + from mots.config import FileConfig @@ -184,3 +187,18 @@ def test_directory__QueryResult_empty_addition(): empty_result = QueryResult() other_empty_result = QueryResult() assert not (empty_result + other_empty_result) + + +def test_Person(): + with pytest.raises(TypeError): + Person() + + person = Person(0) + assert person.bmo_id == 0 + assert person.nick == "" + assert person.name == "" + + person = Person(0, name="Tester", nick="tester") + assert person.bmo_id == 0 + assert person.name == "Tester" + assert person.nick == "tester" From 9dc4a01ec3579eb5a175f02d8e8ed9e02a2e065a Mon Sep 17 00:00:00 2001 From: Zeid Zabaneh Date: Mon, 24 Oct 2022 10:50:00 -0400 Subject: [PATCH 2/2] config: update with bmo data again after parsing modules (bug 1797102) - update using bmo data after parsing modules - ensure only unique people are added to people list - add test that mocks get_bmo_data to avoid requiring API call --- src/mots/config.py | 5 +++++ src/mots/directory.py | 10 ++++++---- tests/test_config.py | 13 ++++++++++++- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/mots/config.py b/src/mots/config.py index 5b576e8..474c8ec 100644 --- a/src/mots/config.py +++ b/src/mots/config.py @@ -190,6 +190,11 @@ def clean(file_config: FileConfig, write: bool = True): submodule["machine_name"] = generate_machine_readable_name( submodule["name"] ) + # Update people again from BMO in case new people were added. + people = file_config.config["people"] + bmo_data = get_bmo_data(people) + updated_people = People(people, bmo_data) + file_config.config["people"] = updated_people.serialized file_config.config["modules"].sort(key=lambda x: x["machine_name"]) if write: diff --git a/src/mots/directory.py b/src/mots/directory.py index d8b4b48..7d02d2d 100644 --- a/src/mots/directory.py +++ b/src/mots/directory.py @@ -5,6 +5,8 @@ """Directory classes for mots.""" from __future__ import annotations +from __future__ import annotations + from collections import defaultdict from dataclasses import asdict from dataclasses import dataclass @@ -192,11 +194,10 @@ class People: def __init__(self, people, bmo_data: dict): logger.debug(f"Initializing people directory with {len(people)} people...") - self.people = [] + self.people = set() self.by_bmo_id = {} - people = list(people) - for i, person in enumerate(people): + for i, person in enumerate(list(people)): logger.debug(f"Adding person {person} to roster...") bmo_id = person["bmo_id"] = int(person["bmo_id"]) @@ -211,10 +212,11 @@ def __init__(self, people, bmo_data: dict): person["name"] = person.get("name", "") person["nick"] = person.get("nick", "") - self.people.append(Person(**person)) + self.people.add(Person(**person)) self.by_bmo_id[person["bmo_id"]] = i logger.debug(f"Person {person} added to position {i}.") + self.people = list(self.people) self.serialized = [asdict(person) for person in self.people] def refresh_by_bmo_id(self): diff --git a/tests/test_config.py b/tests/test_config.py index 9707e34..b26c2cb 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -4,6 +4,8 @@ """Test config module.""" +from unittest import mock + from mots.config import calculate_hashes, FileConfig, clean @@ -34,9 +36,18 @@ def test_FileConfig__check_hashes(repo): ] -def test_clean_with_no_nick(repo, config_with_bmo_ids_only): +@mock.patch("mots.config.get_bmo_data") +def test_clean_with_no_nick(get_bmo_data, repo, config_with_bmo_ids_only): """Ensures clean function runs without any errors if nick is missing.""" + get_bmo_data.return_value = { + 1: {"bmo_id": 1, "nick": "somenick", "real_name": "Some Name"} + } file_config = FileConfig(repo / "mots.yml") file_config.config = config_with_bmo_ids_only file_config.write() + + assert file_config.config["people"] == [] clean(file_config) + assert file_config.config["people"] == [ + {"bmo_id": 1, "nick": "somenick", "name": "Some Name"} + ]