From ca3d5caac0cfcf1f643339257fe8016d4cbd1c6d Mon Sep 17 00:00:00 2001
From: Andreas Ellewsen <andreas@ellewsen.no>
Date: Thu, 1 Dec 2022 14:11:51 +0100
Subject: [PATCH] Handle updates better in guest import
Introduces the --update-existing option, set to False by default. Thie
option controls whether or not we want to update already imported
information with the info from the file.
This is usually not what we want since sponsors may have changed the end
dates for already imported roles after the previous import. We choose to
leave it as an option so that we CAN overwrite information if we really
want.
This commit also removes the ignore-id-req option since that only makes
matter worse than not importing the person.
---
greg/management/commands/import_guests.py | 38 ++++-----
greg/tests/conftest.py | 4 +-
greg/tests/management/fixtures/guests.json | 79 +++++++++++++++++++
greg/tests/management/fixtures/guests2.json | 54 +++++++++++++
greg/tests/management/test_import_guests.py | 71 +++++++++++++++++
.../test_import_sponsors_from_cerebrum.py | 10 +--
greg/tests/models/test_organizational_unit.py | 2 +-
7 files changed, 232 insertions(+), 26 deletions(-)
create mode 100644 greg/tests/management/fixtures/guests.json
create mode 100644 greg/tests/management/fixtures/guests2.json
create mode 100644 greg/tests/management/test_import_guests.py
diff --git a/greg/management/commands/import_guests.py b/greg/management/commands/import_guests.py
index 352c45a0..cf745619 100644
--- a/greg/management/commands/import_guests.py
+++ b/greg/management/commands/import_guests.py
@@ -49,7 +49,7 @@ class Command(BaseCommand):
super().__init__(*args, **kwargs)
self._orgunit_id_type = "legacy_stedkode"
self._end_date = datetime.datetime.today() + datetime.timedelta(days=100)
- self._ignore_id_req = False
+ self._update_existing = False
def add_arguments(self, parser: CommandParser) -> None:
parser.add_argument(
@@ -68,10 +68,10 @@ class Command(BaseCommand):
help="End date of roles. Default: today + 100 days",
)
parser.add_argument(
- "--ignore-id-req",
+ "--update-existing",
default=False,
action="store_true",
- help="Imports people without required ids if set",
+ help="Force update of already imported information",
)
def _find_person_from_ids(self, ids: dict) -> Optional[Person]:
@@ -182,10 +182,11 @@ class Command(BaseCommand):
person=person,
type=role_type,
)
- role.comments = role_data["comment"]
- role.end_date = self._end_date
- role.save()
- logger.info("updating_existing_role", role=role, sponsor=role.sponsor)
+ if self._update_existing:
+ role.comments = role_data["comment"]
+ role.end_date = self._end_date
+ role.save()
+ logger.info("updating_existing_role", role=role, sponsor=role.sponsor)
except Role.DoesNotExist:
sponsor = self._find_orgunit_sponsor(orgunit)
@@ -269,20 +270,21 @@ class Command(BaseCommand):
def upsert_person(self, external_id: str, data: dict) -> Optional[Person]:
"""Add or update person"""
- if not self._ignore_id_req and not self._has_required_id(data["ids"]):
+ person = self._find_person_from_ids(data["ids"])
+ if not (person or self._has_required_id(data["ids"])):
logger.error("missing_required_id", external_id=external_id)
return None
- person = self._find_person_from_ids(data["ids"])
if person:
- # Update data
- person.first_name = data["first_name"]
- person.last_name = data["last_name"]
- person.date_of_birth = data["date_of_birth"]
- person.gender = data["gender"]
- person.registration_completed_date = datetime.date.today()
- person.save()
- logger.info("person_updated", person=person)
+ if self._update_existing:
+ # Update data
+ person.first_name = data["first_name"]
+ person.last_name = data["last_name"]
+ person.date_of_birth = data["date_of_birth"]
+ person.gender = data["gender"]
+ person.registration_completed_date = datetime.date.today()
+ person.save()
+ logger.info("person_updated", person=person)
else:
# Create new person
person = Person.objects.create(
@@ -308,7 +310,7 @@ class Command(BaseCommand):
self._orgunit_id_type = options["orgunit_type"]
self._end_date = options["end_date"]
- self._ignore_id_req = options["ignore_id_req"]
+ self._update_existing = options["update_existing"]
with open(options["file"], "r", encoding="UTF-8") as fp:
persons = json.load(fp)
diff --git a/greg/tests/conftest.py b/greg/tests/conftest.py
index d5ab2cfd..1ceabf4b 100644
--- a/greg/tests/conftest.py
+++ b/greg/tests/conftest.py
@@ -159,7 +159,7 @@ def role_type_test_guest() -> RoleType:
def unit_foo() -> OrganizationalUnit:
ou = OrganizationalUnit.objects.create(name_en="foo_unit")
OuIdentifier.objects.create(
- source="orgreg", name="legacy_stedkode", value="12345", orgunit=ou
+ source="orgreg", name="legacy_stedkode", value="123456", orgunit=ou
)
return OrganizationalUnit.objects.get(id=ou.id)
@@ -168,7 +168,7 @@ def unit_foo() -> OrganizationalUnit:
def unit_foo2() -> OrganizationalUnit:
ou = OrganizationalUnit.objects.create(name_en="foo_unit2")
OuIdentifier.objects.create(
- source="orgreg", name="legacy_stedkode", value="23456", orgunit=ou
+ source="orgreg", name="legacy_stedkode", value="234567", orgunit=ou
)
return OrganizationalUnit.objects.get(id=ou.id)
diff --git a/greg/tests/management/fixtures/guests.json b/greg/tests/management/fixtures/guests.json
new file mode 100644
index 00000000..f9731fbb
--- /dev/null
+++ b/greg/tests/management/fixtures/guests.json
@@ -0,0 +1,79 @@
+{
+ "11111": {
+ "first_name": "Foo",
+ "last_name": "New name to be ignored unless specified",
+ "gender": "male",
+ "ids": [
+ {
+ "source_system": "Manual",
+ "external_id": "+4792929292",
+ "id_type": "private_mobile"
+ },
+ {
+ "source_system": "cerebrum",
+ "external_id": "foo@example.com",
+ "id_type": "feide_email"
+ },
+ {
+ "source_system": "cerebrum",
+ "external_id": "foo@uio.no",
+ "id_type": "feide_id"
+ },
+ {
+ "source_system": "cerebrum",
+ "external_id": "11111",
+ "id_type": "migration_id"
+ }
+ ],
+ "date_of_birth": "1975-06-05",
+ "role": [
+ {
+ "comment": "Comment here",
+ "start_date": "2021-12-01",
+ "role_type": "role_foo",
+ "orgunit": "123456"
+ }
+ ]
+ },
+ "3333333": {
+ "first_name": "Test",
+ "last_name": "Surname",
+ "gender": "female",
+ "ids": [
+ {
+ "source_system": "Manual",
+ "external_id": "NO22AB22333",
+ "id_type": "passport_number"
+ },
+ {
+ "source_system": "Manual",
+ "external_id": "+4793939393",
+ "id_type": "private_mobile"
+ },
+ {
+ "source_system": "cerebrum",
+ "external_id": "test.sum@example.com",
+ "id_type": "feide_email"
+ },
+ {
+ "source_system": "cerebrum",
+ "external_id": "testsum@uio.no",
+ "id_type": "feide_id"
+ },
+ {
+ "source_system": "cerebrum",
+ "external_id": "3333333",
+ "id_type": "migration_id"
+ }
+ ],
+ "date_of_birth": "1988-04-05",
+ "role": [
+ {
+ "comment": "First comment",
+ "start_date": "2021-12-01",
+ "role_type": "role_bar",
+ "orgunit": "123456"
+ }
+ ]
+ }
+}
\ No newline at end of file
diff --git a/greg/tests/management/fixtures/guests2.json b/greg/tests/management/fixtures/guests2.json
new file mode 100644
index 00000000..68494cd3
--- /dev/null
+++ b/greg/tests/management/fixtures/guests2.json
@@ -0,0 +1,54 @@
+{
+ "10402": {
+ "first_name": "Foo",
+ "last_name": "Bar",
+ "gender": "male",
+ "ids": [
+ {
+ "source_system": "DFO_SAP",
+ "external_id": "05067500360",
+ "id_type": "norwegian_national_id_number"
+ },
+ {
+ "source_system": "Manual",
+ "external_id": "+4755555555",
+ "id_type": "private_mobile"
+ },
+ {
+ "source_system": "cerebrum",
+ "external_id": "11111",
+ "id_type": "migration_id"
+ }
+ ],
+ "date_of_birth": "1975-03-26",
+ "role": [
+ {
+ "comment": "Comment here",
+ "start_date": "2021-12-01",
+ "role_type": "role_bar",
+ "orgunit": "123456"
+ }
+ ]
+ },
+ "251494": {
+ "first_name": "Test",
+ "last_name": "Surname",
+ "gender": "female",
+ "ids": [
+ {
+ "source_system": "Manual",
+ "external_id": "NO22AB22333",
+ "id_type": "passport_number"
+ }
+ ],
+ "date_of_birth": "1988-04-05",
+ "role": [
+ {
+ "comment": "Updated comment",
+ "start_date": "2021-12-01",
+ "role_type": "role_bar",
+ "orgunit": "123456"
+ }
+ ]
+ }
+}
\ No newline at end of file
diff --git a/greg/tests/management/test_import_guests.py b/greg/tests/management/test_import_guests.py
new file mode 100644
index 00000000..e02c1bcc
--- /dev/null
+++ b/greg/tests/management/test_import_guests.py
@@ -0,0 +1,71 @@
+import pytest
+
+from django.core.management import call_command
+
+from greg.models import Person, Role
+
+
+@pytest.mark.django_db
+def test_import_guests(sponsor_org_unit, role_type_foo, role_type_bar):
+ """Test import of two persons from two files"""
+
+ assert Person.objects.count() == 0
+ assert Role.objects.count() == 0
+
+ # One guest with missing required id, one okay
+ call_command("import_guests", "./greg/tests/management/fixtures/guests.json")
+ assert Person.objects.count() == 1
+ assert Role.objects.count() == 1
+
+ # Two guests with ok ids
+ call_command("import_guests", "./greg/tests/management/fixtures/guests2.json")
+ assert Person.objects.count() == 2
+ assert Role.objects.count() == 2
+
+ # First file again, but a matching id so we import the new role we missed the first
+ # time
+ call_command("import_guests", "./greg/tests/management/fixtures/guests.json")
+ assert Person.objects.count() == 2
+ assert Role.objects.count() == 3
+
+
+@pytest.mark.django_db
+def test_import_guests_update_existing(sponsor_org_unit, role_type_foo, role_type_bar):
+ """Verify that update existing works correctly"""
+
+ assert Person.objects.count() == 0
+ assert Role.objects.count() == 0
+
+ # One guest with missing required id, one okay
+ call_command(
+ "import_guests",
+ "./greg/tests/management/fixtures/guests.json",
+ "--update-existing",
+ )
+ assert Person.objects.count() == 1
+ assert Role.objects.count() == 1
+
+ # Two guests with ok ids
+ call_command(
+ "import_guests",
+ "./greg/tests/management/fixtures/guests2.json",
+ "--update-existing",
+ )
+ assert Person.objects.count() == 2
+ assert Role.objects.count() == 2
+ pe = Person.objects.get(first_name="Foo")
+ assert pe.last_name == "Bar"
+ pe = Person.objects.get(first_name="Test")
+ assert pe.roles.first().comments == "Updated comment"
+ # First file again, but a matching id so we import the new role
+ call_command(
+ "import_guests",
+ "./greg/tests/management/fixtures/guests.json",
+ "--update-existing",
+ )
+ assert Person.objects.count() == 2
+ assert Role.objects.count() == 3
+ pe = Person.objects.get(first_name="Foo")
+ assert pe.last_name == "New name to be ignored unless specified"
+ pe = Person.objects.get(first_name="Test")
+ assert pe.roles.first().comments == "First comment"
diff --git a/greg/tests/management/test_import_sponsors_from_cerebrum.py b/greg/tests/management/test_import_sponsors_from_cerebrum.py
index 2bafed21..f548a7ea 100644
--- a/greg/tests/management/test_import_sponsors_from_cerebrum.py
+++ b/greg/tests/management/test_import_sponsors_from_cerebrum.py
@@ -109,13 +109,13 @@ def test_import_sponsors_from_cerebrum(
"url": "http://example.com/cerebrum/",
"headers": {"X-Gravitee-Api-Key": "fake-key"},
}
- settings.CEREBRUM_MANUAL_SPONSOR_UNITS = ["12345"]
+ settings.CEREBRUM_MANUAL_SPONSOR_UNITS = ["123456"]
requests_mock.get(
- "http://example.com/cerebrum/v1/groups/adm-leder-12345",
+ "http://example.com/cerebrum/v1/groups/adm-leder-123456",
text=group_response.json(),
)
requests_mock.get(
- "http://example.com/cerebrum/v1/groups/adm-leder-12345/members/",
+ "http://example.com/cerebrum/v1/groups/adm-leder-123456/members/",
text=group_members_response.json(),
)
requests_mock.get(
@@ -128,11 +128,11 @@ def test_import_sponsors_from_cerebrum(
)
requests_mock.get(
- "http://example.com/cerebrum/v1/groups/adm-leder-23456",
+ "http://example.com/cerebrum/v1/groups/adm-leder-234567",
text=group_response.json(),
)
requests_mock.get(
- "http://example.com/cerebrum/v1/groups/adm-leder-23456/members/",
+ "http://example.com/cerebrum/v1/groups/adm-leder-234567/members/",
text=group_members_response2.json(),
)
requests_mock.get(
diff --git a/greg/tests/models/test_organizational_unit.py b/greg/tests/models/test_organizational_unit.py
index ac15e902..0d9c2d04 100644
--- a/greg/tests/models/test_organizational_unit.py
+++ b/greg/tests/models/test_organizational_unit.py
@@ -17,7 +17,7 @@ def test_org_repr(unit_foo):
@pytest.mark.django_db
def test_org_str(unit_foo):
- assert str(unit_foo) == " (foo_unit) (None) (12345)"
+ assert str(unit_foo) == " (foo_unit) (None) (123456)"
@pytest.mark.django_db
--
GitLab