Merge branch 'GREG-88-ou-identifiers' into 'master'

Introduce OuIdentifiers

See merge request !128

greg/admin.py

@@ -2,6 +2,7 @@ from django.contrib import admin
 from reversion.admin import VersionAdmin
 
 from greg.models import (
+    OuIdentifier,
     Invitation,
     InvitationLink,
     Person,
@@ -91,10 +92,21 @@ class ConsentTypeAdmin(VersionAdmin):
     readonly_fields = ("id", "created", "updated")
 
 
+class OuIdentifierInline(admin.TabularInline):
+    model = OuIdentifier
+    extra = 1
+
+
+class IdentifierAdmin(VersionAdmin):
+    list_display = ("id", "name", "source", "value")
+    search_fields = ("id", "value")
+
+
 class OrganizationalUnitAdmin(VersionAdmin):
-    list_display = ("id", "orgreg_id", "name_en", "parent")
+    list_display = ("id", "name_en", "parent")
     readonly_fields = ("id", "created", "updated")
-    search_fields = ("name_en", "id", "orgreg_id")
+    search_fields = ("name_en", "id")
+    inlines = (OuIdentifierInline,)
 
 
 class OrganizationalUnitInline(admin.TabularInline):
@@ -132,3 +144,4 @@ admin.site.register(Sponsor, SponsorAdmin)
 admin.site.register(SponsorOrganizationalUnit, SponsorOrganizationalUnitAdmin)
 admin.site.register(Invitation, InvitationAdmin)
 admin.site.register(InvitationLink, InvitationLinkAdmin)
+admin.site.register(OuIdentifier, IdentifierAdmin)

greg/api/serializers/organizational_unit.py

 from rest_framework.serializers import ModelSerializer
+from greg.api.serializers.ouidentifier import OuIdentifierSerializer
 
 from greg.models import OrganizationalUnit
 
 
 class OrganizationalUnitSerializer(ModelSerializer):
+    identifiers = OuIdentifierSerializer(many=True)
+
     class Meta:
         model = OrganizationalUnit
-        fields = "__all__"
+        fields = [
+            "id",
+            "created",
+            "updated",
+            "name_nb",
+            "name_en",
+            "active",
+            "deleted",
+            "parent",
+            "identifiers",
+        ]

greg/api/serializers/ouidentifier.py

+from rest_framework.serializers import ModelSerializer
+
+from greg.models import OuIdentifier
+
+
+class OuIdentifierSerializer(ModelSerializer):
+    class Meta:
+        model = OuIdentifier
+        fields = ["id", "source", "name", "value"]

greg/management/commands/import_from_orgreg.py

 """
 Fetch all OUs from OrgReg and add the complete tree to Greg.
 
+Ignores OrganizationalUnits without identifiers with source and name matching global
+variables ORGREG_SOURCE and ORGREG_NAME
+
 Assumes that the header used for authentication is of the type
 'X-Gravitee-Api-Key': 'token'.
 
@@ -16,7 +19,7 @@ from django.conf import settings
 from django.core.management.base import BaseCommand
 from orgreg_client import OrgUnit
 
-from greg.models import OrganizationalUnit
+from greg.models import OrganizationalUnit, OuIdentifier
 
 logger = logging.getLogger(__name__)
 
@@ -26,13 +29,25 @@ class Command(BaseCommand):
     processed: Dict[int, OrganizationalUnit] = {}
 
     def _get_or_create_and_set_values(
-        self, ou, values: Mapping[str, Union[str, int, bool]]
+        self, ou: OrgUnit, values: Mapping[str, Union[str, int, bool]]
     ):
         """Upsert ou with latest values and store in processed dict."""
-
-        self.processed[ou.ou_id], created = OrganizationalUnit.objects.get_or_create(
-            orgreg_id=str(ou.ou_id)
-        )
+        try:
+            identifier = OuIdentifier.objects.get(
+                name=settings.ORGREG_NAME, source=settings.ORGREG_SOURCE, value=ou.ou_id
+            )
+        except OuIdentifier.DoesNotExist:
+            self.processed[ou.ou_id] = OrganizationalUnit.objects.create()
+            identifier = OuIdentifier.objects.create(
+                name=settings.ORGREG_NAME,
+                source=settings.ORGREG_SOURCE,
+                value=ou.ou_id,
+                orgunit=self.processed[ou.ou_id],
+            )
+            created = True
+        else:
+            self.processed[ou.ou_id] = identifier.orgunit
+            created = False
         for k, v in values.items():
             setattr(self.processed[ou.ou_id], k, v)
         self.processed[ou.ou_id].save()
@@ -100,13 +115,17 @@ class Command(BaseCommand):
         client = orgreg_client.get_client(**settings.ORGREG_CLIENT)
 
         # Fetch already present OUs and those in OrgReg
-        current_ous = {int(i.orgreg_id): i for i in OrganizationalUnit.objects.all()}
+        current_ous = {
+            int(i.value): i.orgunit
+            for i in OuIdentifier.objects.filter(
+                source=settings.ORGREG_SOURCE, name=settings.ORGREG_NAME
+            )
+        }
 
         logger.info("Fetch 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 and inactive if
-        # valid_to is set to a date before today
+        # Set deleted if an OU from Greg no longer exists in OrgReg
         logger.info("Set deleted tag on removed OUs...")
         for ou_id, ou in current_ous.items():
             if ou_id not in orgreg_ous:

greg/migrations/0012_ou_identifiers.py

+# Generated by Django 3.2.8 on 2021-10-28 11:44
+
+import dirtyfields.dirtyfields
+from django.conf import settings
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+def move_orgreg_id_to_identifiers(apps, schema_editor):
+    OuIdentifier = apps.get_model("greg", "OuIdentifier")
+    OrganizationalUnit = apps.get_model("greg", "OrganizationalUnit")
+    for ou in OrganizationalUnit.objects.all():
+        value = getattr(ou, "orgreg_id", None)
+        if value:
+            OuIdentifier.objects.create(
+                orgunit=ou,
+                name=settings.ORGREG_NAME,
+                source=settings.ORGREG_SOURCE,
+                value=value,
+            )
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("greg", "0011_role_remove_id_suffix"),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name="OuIdentifier",
+            fields=[
+                (
+                    "id",
+                    models.BigAutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name="ID",
+                    ),
+                ),
+                ("created", models.DateTimeField(auto_now_add=True)),
+                ("updated", models.DateTimeField(auto_now=True)),
+                ("name", models.CharField(max_length=256)),
+                ("source", models.CharField(max_length=256)),
+                ("value", models.CharField(max_length=256)),
+            ],
+            bases=(dirtyfields.dirtyfields.DirtyFieldsMixin, models.Model),
+        ),
+        migrations.AddField(
+            model_name="ouidentifier",
+            name="orgunit",
+            field=models.ForeignKey(
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name="identifiers",
+                to="greg.organizationalunit",
+            ),
+        ),
+        migrations.RunPython(move_orgreg_id_to_identifiers),
+        migrations.RemoveConstraint(
+            model_name="organizationalunit",
+            name="unique_orgreg_id",
+        ),
+        migrations.RemoveField(
+            model_name="organizationalunit",
+            name="orgreg_id",
+        ),
+        migrations.AddConstraint(
+            model_name="ouidentifier",
+            constraint=models.UniqueConstraint(
+                fields=("name", "value"), name="unique_identifier"
+            ),
+        ),
+    ]

greg/models.py

@@ -364,12 +364,32 @@ class Consent(BaseModel):
         )
 
 
+class OuIdentifier(BaseModel):
+    """Generic identifier"""
+
+    name = models.CharField(max_length=256)
+    source = models.CharField(max_length=256)
+    value = models.CharField(max_length=256)
+    orgunit = models.ForeignKey(
+        "OrganizationalUnit", on_delete=models.CASCADE, related_name="identifiers"
+    )
+
+    class Meta:
+        constraints = [
+            models.UniqueConstraint(name="unique_identifier", fields=["name", "value"])
+        ]
+
+    def __repr__(self):
+        return "{}(id={!r}, name={!r}, value={!r})".format(
+            self.__class__.__name__, self.pk, self.name, self.value
+        )
+
+
 class OrganizationalUnit(BaseModel):
     """
     An organizational unit. Units can be organized in a hierarchical manner.
     """
 
-    orgreg_id = models.CharField(max_length=256)
     name_nb = models.CharField(max_length=256)
     name_en = models.CharField(max_length=256)
     parent = models.ForeignKey("self", on_delete=models.PROTECT, null=True, blank=True)
@@ -377,17 +397,12 @@ class OrganizationalUnit(BaseModel):
     deleted = models.BooleanField(default=False)
 
     def __repr__(self):
-        return "{}(id={!r}, orgreg_id={!r}, name_en={!r}, parent={!r})".format(
-            self.__class__.__name__, self.pk, self.orgreg_id, self.name_en, self.parent
+        return "{}(id={!r}, name_en={!r}, parent={!r})".format(
+            self.__class__.__name__, self.pk, self.name_en, self.parent
         )
 
     def __str__(self):
-        return "{} ({})".format(str(self.name_en or self.name_nb), self.orgreg_id)
-
-    class Meta:
-        constraints = [
-            models.UniqueConstraint(name="unique_orgreg_id", fields=["orgreg_id"])
-        ]
+        return "{}".format(str(self.name_en or self.name_nb))
 
 
 class Sponsor(BaseModel):

greg/tests/api/test_person.py

@@ -38,7 +38,7 @@ def role_type_visiting_professor() -> RoleType:
 @pytest.fixture
 def unit_human_resources() -> OrganizationalUnit:
     return OrganizationalUnit.objects.create(
-        orgreg_id="org_unit_1", name_nb="Personal", name_en="Human Resources"
+        name_nb="Personal", name_en="Human Resources"
     )
 
 

greg/tests/conftest.py

@@ -10,6 +10,7 @@ import pytest
 from greg.models import (
     Consent,
     Notification,
+    OuIdentifier,
     Person,
     Sponsor,
     SponsorOrganizationalUnit,
@@ -136,10 +137,18 @@ def role_type_test_guest() -> RoleType:
 
 @pytest.fixture
 def unit_foo() -> OrganizationalUnit:
-    ou = OrganizationalUnit.objects.create(orgreg_id="12345", name_en="foo_unit")
+    ou = OrganizationalUnit.objects.create(name_en="foo_unit")
     return OrganizationalUnit.objects.get(id=ou.id)
 
 
+@pytest.fixture
+def ouidentifier_foo(unit_foo) -> OuIdentifier:
+    ouid = OuIdentifier.objcets.create(
+        source="orgreg", name="orgreg", value="12345", orgunit=unit_foo
+    )
+    return OuIdentifier.objects.get(id=ouid.id)
+
+
 @pytest.fixture
 def role_person_foo(
     person_foo: Person,

greg/tests/management/test_import_from_orgreg.py

 import datetime
 
 import pytest
+from django.conf import settings
 from django.core.management import call_command
 from orgreg_client import OrgUnit, OrgUnitList
 
-from greg.models import OrganizationalUnit
+from greg.models import OrganizationalUnit, OuIdentifier
 
 
 @pytest.fixture
 def old_unit():
-    OrganizationalUnit.objects.create(orgreg_id="4", name_nb="a", name_en="b")
-    return OrganizationalUnit.objects.get(orgreg_id="4")
+    ou = OrganizationalUnit.objects.create(name_nb="a", name_en="b")
+    return OrganizationalUnit.objects.get(id=ou.id)
+
+
+@pytest.fixture
+def old_identifier(unit_foo):
+    ouid = OuIdentifier.objects.create(
+        orgunit=unit_foo,
+        name=settings.ORGREG_NAME,
+        source=settings.ORGREG_SOURCE,
+        value="4",
+    )
+    return OuIdentifier.objects.get(id=ouid.id)
 
 
 @pytest.fixture
@@ -44,7 +56,7 @@ def orgreg_response():
 
 
 @pytest.mark.django_db
-def test_command_ou_init(requests_mock, old_unit, orgreg_response):
+def test_command_ou_init(requests_mock, old_identifier, orgreg_response):
     requests_mock.get("https://example.com/fake/ou/", text=orgreg_response.json())
     assert OrganizationalUnit.objects.all().count() == 1
     call_command("import_from_orgreg")
@@ -53,11 +65,20 @@ def test_command_ou_init(requests_mock, old_unit, orgreg_response):
     assert OrganizationalUnit.objects.all().count() == 4
 
     # Ensure tree is built correctly
-    assert OrganizationalUnit.objects.get(orgreg_id="3").parent.orgreg_id == "2"
-    assert OrganizationalUnit.objects.get(orgreg_id="3").parent.parent.orgreg_id == "1"
+    bottom = OrganizationalUnit.objects.get(
+        identifiers__source="orgreg", identifiers__value="3"
+    )
+    assert bottom.name_nb == "bar"
+    assert bottom.parent.identifiers.get().value == "2"
+    assert bottom.parent.name_en == "baz"
+    assert bottom.parent.parent.identifiers.get().value == "1"
+    assert bottom.parent.parent.name_nb == "foo"
 
     # Ensure unknown org is marked deleted
-    assert OrganizationalUnit.objects.get(orgreg_id="4").deleted is True
+    old_unit = OrganizationalUnit.objects.get(
+        identifiers__source="orgreg", identifiers__value="4"
+    )
+    assert old_unit.deleted
 
 
 @pytest.mark.django_db

greg/tests/models/test_organizational_unit.py

@@ -5,19 +5,16 @@ from greg.models import OrganizationalUnit
 
 @pytest.mark.django_db
 def test_set_parent():
-    parent = OrganizationalUnit.objects.create(orgreg_id="parent")
-    child = OrganizationalUnit.objects.create(orgreg_id="child", parent=parent)
+    parent = OrganizationalUnit.objects.create()
+    child = OrganizationalUnit.objects.create(parent=parent)
     assert list(OrganizationalUnit.objects.filter(parent__id=parent.id)) == [child]
 
 
 @pytest.mark.django_db
 def test_org_repr(unit_foo):
-    assert (
-        repr(unit_foo)
-        == "OrganizationalUnit(id=1, orgreg_id='12345', name_en='foo_unit', parent=None)"
-    )
+    assert repr(unit_foo) == "OrganizationalUnit(id=1, name_en='foo_unit', parent=None)"
 
 
 @pytest.mark.django_db
 def test_org_str(unit_foo):
-    assert str(unit_foo) == "foo_unit (12345)"
+    assert str(unit_foo) == "foo_unit"

greg/tests/models/test_person.py

@@ -94,7 +94,7 @@ def feide_verified(person: Person, feide_id: Identity) -> Person:
 def test_add_multiple_roles_to_person(
     person: Person, role_type_foo: RoleType, role_type_bar: RoleType
 ):
-    ou = OrganizationalUnit.objects.create(orgreg_id="12345", name_en="Test unit")
+    ou = OrganizationalUnit.objects.create(name_en="Test unit")
     role_with(
         person=person,
         type=role_type_foo,

greg/tests/models/test_sponsor.py

@@ -27,12 +27,12 @@ def sponsor_bar() -> Sponsor:
 
 @pytest.fixture
 def unit1() -> OrganizationalUnit:
-    return OrganizationalUnit.objects.create(orgreg_id="1", name_en="First unit")
+    return OrganizationalUnit.objects.create(name_en="First unit")
 
 
 @pytest.fixture
 def unit2() -> OrganizationalUnit:
-    return OrganizationalUnit.objects.create(orgreg_id="2", name_en="Second unit")
+    return OrganizationalUnit.objects.create(name_en="Second unit")
 
 
 @pytest.mark.django_db

greg/tests/models/test_sponsor_orgunit.py

@@ -5,5 +5,5 @@ import pytest
 def test_sponsor_org_repr(sponsor_org_unit):
     assert (
         repr(sponsor_org_unit)
-        == "SponsorOrganizationalUnit(id=1, sponsor=Sponsor(id=1, feide_id='guy@example.org', first_name='Sponsor', last_name='Guy'), organizational_unit=OrganizationalUnit(id=1, orgreg_id='12345', name_en='foo_unit', parent=None), hierarchical_access=False)"
+        == "SponsorOrganizationalUnit(id=1, sponsor=Sponsor(id=1, feide_id='guy@example.org', first_name='Sponsor', last_name='Guy'), organizational_unit=OrganizationalUnit(id=1, name_en='foo_unit', parent=None), hierarchical_access=False)"
     )

greg/tests/populate_database.py

@@ -50,7 +50,7 @@ class DatabasePopulation:
             self.random = random.Random()
 
     def populate_database(self):
-        for i in range(10):
+        for _ in range(10):
             first_name = self.faker.first_name()
             last_name = self.faker.last_name()
 
@@ -82,21 +82,19 @@ class DatabasePopulation:
                 RoleType.objects.create(identifier=role_type, name_en=role_type)
             )
 
-        for i in range(10):
+        for _ in range(10):
             self.units.append(
-                OrganizationalUnit.objects.create(
-                    orgreg_id=f"12345{i}", name_en=self.faker.company()
-                )
+                OrganizationalUnit.objects.create(name_en=self.faker.company())
             )
 
-        for i in range(5):
+        for _ in range(5):
             self.sponsors.append(
                 Sponsor.objects.create(
                     feide_id=self.faker.bothify(text="???####@uio.no")
                 )
             )
 
-        for i in range(10):
+        for _ in range(10):
             self.consent_types.append(
                 ConsentType.objects.create(
                     identifier=self.faker.slug(),
@@ -162,14 +160,14 @@ class DatabasePopulation:
                 Role.objects.create(
                     person=random.choice(self.persons),
                     type=random.choice(self.role_types),
-                    orgunit_id=random.choice(self.units),
+                    orgunit=random.choice(self.units),
                     start_date=self.faker.date_this_decade(),
                     end_date=self.faker.date_this_decade(
                         before_today=False, after_today=True
                     ),
                     contact_person_unit=self.faker.name(),
                     available_in_search=self.random.random() > 0.5,
-                    sponsor_id=random.choice(self.sponsors),
+                    sponsor=random.choice(self.sponsors),
                 )
                 person_role_count += 1
             except IntegrityError:

greg/tests/test_notifications.py

@@ -15,7 +15,7 @@ from greg.models import (
 
 @pytest.fixture
 def org_unit_bar() -> OrganizationalUnit:
-    return OrganizationalUnit.objects.create(orgreg_id="bar_unit")
+    return OrganizationalUnit.objects.create()
 
 
 @pytest.fixture

gregsite/settings/base.py

@@ -267,3 +267,7 @@ ENVIRONMENT = "unknown"
 INTERNAL_RK_PREFIX = "no.{instance}.greg".format(instance=INSTANCE_NAME)
 
 FEIDE_SOURCE = "feide"
+
+# Used by the OU import from orgreg to distinguish the OuIdentifiers from others
+ORGREG_SOURCE = "orgreg"
+ORGREG_NAME = "orgreg_id"

gregui/api/serializers/ouidentifier.py

+from rest_framework.serializers import ModelSerializer
+
+from greg.models import OuIdentifier
+
+
+class IdentifierSerializer(ModelSerializer):
+    class Meta:
+        model = OuIdentifier
+        fields = ("id", "name", "source", "value")

gregui/api/serializers/unit.py

 from rest_framework.serializers import ModelSerializer
 
 from greg.models import OrganizationalUnit
+from gregui.api.serializers.ouidentifier import IdentifierSerializer
 
 
 class UnitSerializerId(ModelSerializer):
+    identifiers = IdentifierSerializer(many=True, read_only=True)
+
     class Meta:
         model = OrganizationalUnit
-        fields = ("orgreg_id", "name_nb", "name_en")
+        fields = ("name_nb", "name_en", "identifiers")

gregui/api/views/invitation.py

@@ -66,7 +66,7 @@ class InvitationView(CreateAPIView, DestroyAPIView):
 
         invitationlink = InvitationLink.objects.filter(
             invitation__role__person_id=person.id,
-            invitation__role__sponsor_id=sponsor_user.sponsor_id,
+            invitation__role__sponsor_id=sponsor_user.sponsor.id,
         )
         # TODO: send email to invited guest
         print(invitationlink)

gregui/tests/conftest.py

@@ -129,9 +129,7 @@ def client() -> APIClient:
 
 @pytest.fixture
 def unit_foo() -> OrganizationalUnit:
-    ou = OrganizationalUnit.objects.create(
-        orgreg_id="12345", name_en="Foo EN", name_nb="Foo NB"
-    )
+    ou = OrganizationalUnit.objects.create(name_en="Foo EN", name_nb="Foo NB")
     return OrganizationalUnit.objects.get(id=ou.id)