Various changes to OrgReg import

- Support running the OrgReg import directly via management command
- Only log OUs as updated if there are changes
- Log changed attributes with new and old value
- Fix updating changed OU identifiers

greg/importers/orgreg.py

@@ -128,42 +128,45 @@ class OrgregImporter:
         return matching_ids[0]
 
     def _upsert_identifier(
-        self, source: str, identity_type: str, identity_in_orgreg_value: str, ou_id: int
+        self, source: str, identifier_type: str, new_value: str, ou_id: int
     ):
+        # strip whitespace, in case of \r or \n
+        new_value = new_value.strip()
+
         # Check if the id exists
-        identify_in_db = (
+        saved_identifier = (
             self.processed[ou_id]
             .identifiers.filter(
-                value=identity_in_orgreg_value,
-                name=identity_type,
+                name=identifier_type,
             )
             .first()
         )
 
-        if identify_in_db:
-            if identify_in_db.value != identity_in_orgreg_value:
+        if saved_identifier:
+            if saved_identifier.value != new_value:
                 logger.info(
-                    "Updating id: source: %s, type: %s, old_id: %s, new_id %s",
+                    "Updating identifier for ou_id=%r type=%r source=%r old_value=%r new_value=%r",
+                    ou_id,
+                    identifier_type,
                     source,
-                    identity_type,
-                    identify_in_db.value,
-                    identity_in_orgreg_value,
+                    saved_identifier.value,
+                    new_value,
                 )
-                identify_in_db.value = identity_in_orgreg_value
-                identify_in_db.save()
+                saved_identifier.value = new_value
+                saved_identifier.save()
         else:
             OuIdentifier.objects.create(
-                name=identity_type,
+                name=identifier_type,
                 source=source,
-                value=identity_in_orgreg_value,
+                value=new_value,
                 orgunit=self.processed[ou_id],
             )
             logger.info(
-                "Added new id to ou: %s, type: %s, source: %s value: %s",
+                "Added new identifier to ou_id=%r type=%r source=%r value=%r",
                 ou_id,
-                identity_type,
+                identifier_type,
                 source,
-                identity_in_orgreg_value,
+                new_value,
             )
 
     def _get_or_create_and_set_values(
@@ -172,7 +175,6 @@ class OrgregImporter:
         values: Mapping[str, Union[str, int, bool, OrganizationalUnit]],
     ):
         """Upsert ou with latest values and store in processed dict."""
-
         try:
             identifier = OuIdentifier.objects.get(
                 name=settings.ORGREG_NAME, source=settings.ORGREG_SOURCE, value=ou.ou_id
@@ -192,13 +194,21 @@ class OrgregImporter:
         for k, v in values.items():
             setattr(self.processed[ou.ou_id], k, v)
         self.upsert_extra_identities(ou)
+        changes = None
+        if self.processed[ou.ou_id].is_dirty(check_relationship=True):
+            changes = self.processed[ou.ou_id].get_dirty_fields(
+                check_relationship=True, verbose=True
+            )
         self.processed[ou.ou_id].save()
-        logger.info(
-            "%s %s with %s",
-            "Created" if created else "Updated",
-            self.processed[ou.ou_id],
-            values,
-        )
+        if changes:
+            logger.info(
+                "%s %s with %s",
+                "Created" if created else "Updated",
+                self.processed[ou.ou_id],
+                changes,
+            )
+        else:
+            logger.info("No changes for %s", self.processed[ou.ou_id])
 
     def _upsert_ou(self, ou: OrgUnit, ous: Mapping[int, OrgUnit]):
         """
@@ -219,11 +229,12 @@ class OrgregImporter:
             return
         values: dict[str, Any] = {"deleted": False}
         # add names if present
+        # we strip whitespace as \r and \n sometimes trickle through
         if ou.name:
             if ou.name.nob:
-                values["name_nb"] = ou.name.nob
+                values["name_nb"] = ou.name.nob.strip()
             if ou.name.eng:
-                values["name_en"] = ou.name.eng
+                values["name_en"] = ou.name.eng.strip()
 
         # Set inactive if ou is no longer valid
         is_inactive = ou.valid_to and ou.valid_to < datetime.date.today()
@@ -263,11 +274,11 @@ class OrgregImporter:
             )
         }
 
-        logger.info("Fetch OUs from Orgreg...")
+        logger.info("Fetching OUs from Orgreg...")
         orgreg_ous = {i.ou_id: i for i in client.get_ou()}
 
         # Set deleted if an OU from Greg no longer exists in OrgReg
-        logger.info("Set deleted tag on removed OUs...")
+        logger.info("Setting deleted tag on removed OUs...")
         for ou_id, ou in current_ous.items():
             if ou_id not in orgreg_ous:
                 logger.info("%s marked as deleted", ou)
@@ -276,6 +287,6 @@ class OrgregImporter:
                 continue
 
         # Create new OUs recursively to ensure parents exist before children
-        logger.info("Update OU values...")
+        logger.info("Updating OU values...")
         for ou_id, ou in orgreg_ous.items():
             self._upsert_ou(ou, orgreg_ous)

greg/management/commands/import_from_orgreg.py

 """
-Command for scheduling the django-q task for importing OUs from Orgreg
+Command for running or scheduling the django-q task for
+importing OUs from OrgReg.
 """
 import logging
 
+from django.db import transaction
 from django.conf import settings
-from django.core.management.base import BaseCommand
+from django.core.management.base import BaseCommand, CommandParser
 from django_q.tasks import schedule
 
+from greg.importers.orgreg import OrgregImporter
 
 logger = logging.getLogger(__name__)
 
@@ -14,7 +17,45 @@ logger = logging.getLogger(__name__)
 class Command(BaseCommand):
     help = __doc__
 
+    def add_arguments(self, parser: CommandParser) -> None:
+        parser.add_argument(
+            "--schedule",
+            default=False,
+            action="store_true",
+            help="Add a scheduled task for running the import",
+        )
+        parser.add_argument(
+            "--run-once",
+            default=False,
+            action="store_true",
+            help="Run the import once",
+        )
+        parser.add_argument(
+            "--commit",
+            default=False,
+            action="store_true",
+            help="Commit the changes done by running the import once",
+        )
+
     def handle(self, *args, **options):
+        if options["schedule"]:
+            self.schedule()
+            return
+
+        commit = options['commit']
+
+        if options["run_once"]:
+            with transaction.atomic():
+                importer = OrgregImporter()
+                importer.handle()
+                if not commit:
+                    logger.info('Rolling back changes...')
+                    transaction.set_rollback(True)
+                else:
+                    logger.info('Changes committed')
+
+
+    def schedule(self):
         logger.info("Scheduling orgreg import task...")
         schedule(
             func="greg.tasks.import_from_orgreg",

greg/tests/management/test_import_from_orgreg.py

@@ -36,9 +36,9 @@ def org_unit():
 
 
 @pytest.mark.django_db
-def test_command_ou_init():
+def test_command_with_schedule_flag_schedules_task():
     assert Schedule.objects.all().count() == 0
-    call_command("import_from_orgreg")
+    call_command("import_from_orgreg", "--schedule")
     assert Schedule.objects.all().count() == 1
     sc = Schedule.objects.first()
     assert sc.func == "greg.tasks.import_from_orgreg"
@@ -73,18 +73,16 @@ def test_upsert_extra_identities_update(org_unit):
     """
     Verify that already imported values are updated
     """
-    organizational_unit = OrganizationalUnit.objects.create(
-        name_nb="foo_nb", name_en="foo_en"
-    )
+    ou = OrganizationalUnit.objects.create(name_nb="foo_nb", name_en="foo_en")
+    OuIdentifier.objects.create(name="short_name", value="OLD", orgunit=ou)
     OuIdentifier.objects.create(
-        name="short_name", value="FOO", orgunit=organizational_unit
-    )
-    OuIdentifier.objects.create(
-        name="legacy_stedkode", value="1", orgunit=organizational_unit, source="sapuio"
+        name="legacy_stedkode", value="OLD", orgunit=ou, source="sapuio"
     )
     assert OuIdentifier.objects.all().count() == 2
 
     org_importer = OrgregImporter()
-    org_importer.processed[org_unit.ou_id] = organizational_unit
+    org_importer.processed[org_unit.ou_id] = ou
     org_importer.upsert_extra_identities(org_unit)
     assert OuIdentifier.objects.all().count() == 2
+    assert ou.identifiers.filter(name="short_name").first().value == "FOO"
+    assert ou.identifiers.filter(name="legacy_stedkode").first().value == "1"