Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ [#182] changed internetaken actor field to actoren #218

Merged
merged 4 commits into from
Sep 4, 2024
Merged

✨ [#182] changed internetaken actor field to actoren #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4537ab1
:sparkles: [#182] changed internetaken actor field to actoren
bart-maykin Aug 8, 2024
b9e4b58
:card_file_box: [#182] made sure that the reverse datamigration of kl…
bart-maykin Aug 15, 2024
bdf63f9
:card_file_box: [#182] changed migration name and order because of tw…
bart-maykin Aug 15, 2024
fe4001d
:art: [#182] changed ordered though model to normal though model
bart-maykin Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/openklant/components/klantinteracties/admin/actoren.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
Medewerker,
OrganisatorischeEenheid,
)
from .internetaken import InterneTaakInlineAdmin


class GeautomatiseerdeActorInlineAdmin(admin.StackedInline):
Expand Down Expand Up @@ -52,7 +51,6 @@ class ActorAdmin(admin.ModelAdmin):
GeautomatiseerdeActorInlineAdmin,
MedewerkerInlineAdmin,
OrganisatorischeEenheidInlineAdmin,
InterneTaakInlineAdmin,
)
fieldsets = (
(
Expand Down
7 changes: 5 additions & 2 deletions src/openklant/components/klantinteracties/admin/internetaken.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ class InterneTaakAdmin(admin.ModelAdmin):
"afgehandeld_op",
)
list_filter = (
"actor",
"actoren",
"status",
)
readonly_fields = ("toegewezen_op",)
readonly_fields = (
"uuid",
"toegewezen_op",
)
6 changes: 3 additions & 3 deletions src/openklant/components/klantinteracties/api/filterset/internetaken.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
class InternetaakFilterSet(FilterSet):
toegewezen_aan_actor__uuid = filters.UUIDFilter(
help_text=_("Zoek internetaak object op basis van het toegewezen actor uuid."),
field_name="actor__uuid",
field_name="actoren__uuid",
)
toegewezen_aan_actor__url = filters.CharFilter(
help_text=_("Zoek internetaak object op basis van het toegewezen actor url."),
Expand All @@ -35,7 +35,7 @@ class Meta:
"nummer",
"status",
"toegewezen_op",
"actor__naam",
"actoren__naam",
"klantcontact__uuid",
"klantcontact__nummer",
"toegewezen_aan_actor__uuid",
Expand All @@ -47,7 +47,7 @@ class Meta:
def filter_toegewezen_aan_actor_url(self, queryset, name, value):
try:
url_uuid = uuid.UUID(value.rstrip("/").split("/")[-1])
return queryset.filter(actor__uuid=url_uuid)
return queryset.filter(actoren__uuid=url_uuid)
except ValueError:
return queryset.none()

Expand Down
81 changes: 72 additions & 9 deletions src/openklant/components/klantinteracties/api/serializers/internetaken.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from django.db import transaction
from django.utils.translation import gettext_lazy as _

from drf_spectacular.utils import extend_schema_serializer
from glom import PathAccessError, glom
from rest_framework import serializers

from openklant.components.klantinteracties.api.serializers.actoren import (
Expand Down Expand Up @@ -33,12 +35,21 @@ class Meta:
}


@extend_schema_serializer(deprecate_fields=["toegewezen_aan_actor"])
class InterneTaakSerializer(serializers.HyperlinkedModelSerializer):
toegewezen_aan_actor = ActorForeignKeySerializer(
required=True,
required=False,
Copy link
Contributor

@annashamray annashamray Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's possible that a task doesn't have any actor, isn't it?
Is this change intended? Can it affect a process in some way?

Nvm, I saw a validation that it can't be possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed not intended for a IntereTaak to not have an Actor.
But because we now have 2 (1 temporary) Actor fields we can't have them set as required.
So I moved the "required" clause to the validation function within the serializer.

allow_null=False,
help_text=_("Actor die een interne taak toegewezen kreeg."),
source="actor",
help_text=_("Eerste actor die een interne taak toegewezen kreeg."),
source="actoren.first",
many=False,
)
toegewezen_aan_actoren = ActorForeignKeySerializer(
required=False,
allow_null=False,
help_text=_("Actoren die een interne taak toegewezen kreeg."),
source="actoren",
many=True,
)
aanleidinggevend_klantcontact = KlantcontactForeignKeySerializer(
required=True,
Expand All @@ -56,6 +67,7 @@ class Meta:
"gevraagde_handeling",
"aanleidinggevend_klantcontact",
"toegewezen_aan_actor",
"toegewezen_aan_actoren",
"toelichting",
"status",
"toegewezen_op",
Expand All @@ -70,7 +82,52 @@ class Meta:
},
}

def to_representation(self, instance):
response = super().to_representation(instance)
response["toegewezen_aan_actor"] = ActorForeignKeySerializer(
instance.actoren.order_by("internetakenactorenthoughmodel__pk").first(),
context={**self.context},
).data

response["toegewezen_aan_actoren"] = ActorForeignKeySerializer(
instance.actoren.all().order_by("internetakenactorenthoughmodel__pk"),
context={**self.context},
many=True,
).data
return response

# TODO: remove when depricated actoren field gets removed
def _validate_actoren(self):
toegewezen_aan_actor = "toegewezen_aan_actor" in self.initial_data
toegewezen_aan_actoren = "toegewezen_aan_actoren" in self.initial_data

if toegewezen_aan_actor == toegewezen_aan_actoren:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible? They should have different types since one is iterable and another one is not
What happens if they are not equal? Could you please add a test for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am basically checking here if the fields got send by the user.
If toegewezen_aan_actor is True then the user send it, if not then it is False and the same goes for toegewezen_aan_actoren.

This way I can validate if only one of the two fields got used. If both are False then neither were send, and if both are True then both got send.

if toegewezen_aan_actor:
message = _(
"`toegewezen_aan_actor` en `toegewezen_aan_actoren` mag niet beiden gebruikt worden."
)
else:
message = _(
"`toegewezen_aan_actor` of `toegewezen_aan_actoren` is required (mag niet beiden gebruiken)."
)

# If patch don't raise required error
if self.context.get("request").method == "PATCH":
return

raise serializers.ValidationError(message)

# TODO: remove when depricated actoren field gets removed
def _get_actoren(self, actoren):
if isinstance(actoren, list):
actor_uuids = [str(actor.get("uuid")) for actor in actoren]
else:
actor_uuids = [glom(actoren, "first.uuid", skip_exc=PathAccessError)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand this line. Could you please provide an example when it happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure of both actor fields are different.

The single object returns a validated data of {"first": {"uuid": "..."}}
And the multiple objects return a validated data of [{"uuid": "..."}, {"uuid": "..."}]

This is just a temporary function to fetch the data I need to update and create the objects.


return [Actor.objects.get(uuid=uuid) for uuid in actor_uuids]

def validate(self, attrs):
self._validate_actoren()
status = attrs.get("status", None)
if status is None and self.instance is not None:
status = self.instance.status
Expand All @@ -89,21 +146,27 @@ def validate(self, attrs):

@transaction.atomic
def create(self, validated_data):
actor_uuid = str(validated_data.pop("actor").get("uuid"))
actoren = validated_data.pop("actoren", None)
klantcontact_uuid = str(validated_data.pop("klantcontact").get("uuid"))

validated_data["actor"] = Actor.objects.get(uuid=actor_uuid)
validated_data["klantcontact"] = Klantcontact.objects.get(
uuid=klantcontact_uuid
)

return super().create(validated_data)
internetaak = super().create(validated_data)
if actoren:
for actor in self._get_actoren(actoren):
internetaak.actoren.add(actor)

return internetaak

@transaction.atomic
def update(self, instance, validated_data):
if "actor" in validated_data:
if actor := validated_data.pop("actor", None):
validated_data["actor"] = Actor.objects.get(uuid=str(actor.get("uuid")))
if "actoren" in validated_data:
actoren = validated_data.pop("actoren")
instance.actoren.clear()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always delete and create relations seems unnecessary. I think it's better to compare first that old and new actors are not the same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be done in the future but because we currently are bound to a temporary ordering model. I think it is not worth the time and resources to create the functionality of comparing the data between the existing data and the user submitted data and compare the order of both.

Deleting the relations and creating them afterwards will guarantee that the order will always be correct. So I think that for now this is a fine solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's keep it until we get rid of the deprecated field and OrderedModel

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to modify it now?

for actor in self._get_actoren(actoren):
instance.actoren.add(actor)

if "klantcontact" in validated_data:
if klantcontact := validated_data.pop("klantcontact", None):
Expand Down
10 changes: 5 additions & 5 deletions src/openklant/components/klantinteracties/api/tests/test_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,19 +1071,19 @@ def setUp(self):
self.klantcontact5,
) = KlantcontactFactory.create_batch(5)
self.internetaak = InterneTaakFactory.create(
actor=self.actor, klantcontact=self.klantcontact
actoren=[self.actor], klantcontact=self.klantcontact
)
self.internetaak2 = InterneTaakFactory.create(
actor=self.actor2, klantcontact=self.klantcontact2
actoren=[self.actor2], klantcontact=self.klantcontact2
)
self.internetaak3 = InterneTaakFactory.create(
actor=self.actor3, klantcontact=self.klantcontact3
actoren=[self.actor3], klantcontact=self.klantcontact3
)
self.internetaak4 = InterneTaakFactory.create(
actor=self.actor4, klantcontact=self.klantcontact4
actoren=[self.actor4], klantcontact=self.klantcontact4
)
self.internetaak5 = InterneTaakFactory.create(
actor=self.actor5, klantcontact=self.klantcontact5
actoren=[self.actor5], klantcontact=self.klantcontact5
)

def test_filter_toegewezen_aan_actor_url(self):
Expand Down
Loading
Loading