-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
✨ [#182] changed internetaken actor field to actoren #218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked through the code, but I don't have enough domain knowledge about this API to understand if the it's what the client wants. For this goal it's better to wait for Joeri or somebody else know knows Open Klant API
...ntinteracties/migrations/0016_remove_internetaak_actor_interneactorenthoughmodel_and_more.py
Outdated
Show resolved
Hide resolved
class InterneTaakSerializer(serializers.HyperlinkedModelSerializer): | ||
toegewezen_aan_actor = ActorForeignKeySerializer( | ||
required=True, | ||
required=False, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/openklant/components/klantinteracties/models/internetaken.py
Outdated
Show resolved
Hide resolved
response["toegewezen_aan_actor"] = ActorForeignKeySerializer( | ||
instance.actoren.order_by("interneactorenthoughmodel__order").first(), | ||
context={**self.context}, | ||
).data | ||
|
||
response["toegewezen_aan_actoren"] = ActorForeignKeySerializer( | ||
instance.actoren.all().order_by("interneactorenthoughmodel__order"), | ||
context={**self.context}, | ||
many=True, | ||
).data | ||
return response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code necessary? It looks like it duplicates the code with field declaration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It unfortunately is necessary. The issue is very well described in the Django Ordered Model library:
https://github.com/django-ordered-model/django-ordered-model?tab=readme-ov-file#ordering-of-manytomany-relationship-query-results
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: |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 isinstance(actoren, list): | ||
actor_uuids = [str(actor.get("uuid")) for actor in actoren] | ||
else: | ||
actor_uuids = [glom(actoren, "first.uuid", skip_exc=PathAccessError)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
validated_data["actor"] = Actor.objects.get(uuid=str(actor.get("uuid"))) | ||
if "actoren" in validated_data: | ||
actoren = validated_data.pop("actoren") | ||
instance.actoren.clear() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
69cf949
to
6a954b0
Compare
6a954b0
to
bdf63f9
Compare
Besproken met Bart, en de feedback van Anna en de twijfels bij Bart laten zien dat er hier (in hun en mijn ogen onnodige) complexiteit wordt geintroduceerd. Bovendien is in de refinement te weinig aandacht geweest voor het ordering binnen de reeks van actoren die hierdoor ontstaat. Deze PR gaat onhold voor nu totdat Joeri terug is, de specificatie is te weinig uitgewerkt (en spreekt zichzelf tevens tegen) om hier een klap op te geven |
Ordering can be on pk of the through model, and not OrderedModel. These are the scenario's. Whether Write is an POST, PUT or PATCH, these rules apply: 1
2
3
4
Note: This case clearly has 2 applications that work differently. The first application set A, B via the new attribute, but the second application uses the deprecated attribute. If this application really sets the value via the old attribute, we have to respect that it now has 1 value. 5
6
|
a282a9f
to
5f0d918
Compare
src/openklant/components/klantinteracties/admin/internetaken.py
Outdated
Show resolved
Hide resolved
5f0d918
to
fe4001d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but it's compare old and new actoren before removing them in update action
Fixes #182
Changes