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

@@ -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)

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)
 

greg/tests/management/fixtures/guests.json

+{
+    "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

greg/tests/management/fixtures/guests2.json

+{
+    "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

greg/tests/management/test_import_guests.py

+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"

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(

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