From f536725ac3021c78cc2fa54053ceb6721d40da10 Mon Sep 17 00:00:00 2001
From: Jonas Braathen <jonas.braathen@usit.uio.no>
Date: Tue, 3 Jan 2023 15:44:56 +0100
Subject: [PATCH] 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 | 69 +++++++++++--------
.../management/commands/import_from_orgreg.py | 45 +++++++++++-
.../management/test_import_from_orgreg.py | 18 +++--
3 files changed, 91 insertions(+), 41 deletions(-)
diff --git a/greg/importers/orgreg.py b/greg/importers/orgreg.py
index 1dd5cc58..ee58b24a 100644
--- a/greg/importers/orgreg.py
+++ b/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)
diff --git a/greg/management/commands/import_from_orgreg.py b/greg/management/commands/import_from_orgreg.py
index 975cd1b9..13dff58b 100644
--- a/greg/management/commands/import_from_orgreg.py
+++ b/greg/management/commands/import_from_orgreg.py
@@ -1,12 +1,15 @@
"""
-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",
diff --git a/greg/tests/management/test_import_from_orgreg.py b/greg/tests/management/test_import_from_orgreg.py
index 1dbc1eb3..845a9dfa 100644
--- a/greg/tests/management/test_import_from_orgreg.py
+++ b/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"
--
GitLab