Support open archiefbeheer destruction in objects api #719
Conversation
Bencher Report
CharString
force-pushed
the
708-support-open-archiefbeheer-destruction-in-objects-api
branch
from
b090934 to
5c8f34e
Compare
CharString
force-pushed
the
708-support-open-archiefbeheer-destruction-in-objects-api
branch
from
5c8f34e to
bb131da
Compare
CharString
force-pushed
the
708-support-open-archiefbeheer-destruction-in-objects-api
branch
from
f57913a to
1a8758c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #719 +/- ##
==========================================
+ Coverage 84.59% 84.90% +0.31%
==========================================
Files 137 140 +3
Lines 2733 2829 +96
Branches 215 223 +8
==========================================
+ Hits 2312 2402 +90
- Misses 373 375 +2
- Partials 48 52 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stevenbal
left a comment
stevenbal
left a comment
There was a problem hiding this comment.
@CharString could you look into the performance regressions for the list endpoint? There's probably a missing prefetch_related on references #719 (comment)
| type = models.CharField( | ||
| max_length=4, choices=ReferenceType.choices, default=ReferenceType.zaak | ||
| ) |
There was a problem hiding this comment.
Instead of having default zaak, we could probably just make this field required as well?
|
|
||
| def perform_destroy(self, instance): | ||
| instance.object.delete() | ||
| if record := serializer.instance: |
There was a problem hiding this comment.
Is there a scenario where serializer.instance is not set after super().perform_update(serializer)?
| super().perform_create(serializer) | ||
| objects_create_counter.add(1) | ||
|
|
||
| if record := serializer.instance: |
There was a problem hiding this comment.
Is there a scenario where serializer.instance is not set after super().perform_create(serializer)?
| zaak_references = obj.last_record.references.filter(type=ReferenceType.zaak) | ||
|
|
||
| if zaak_urls := list(zaak_references.values_list("url", flat=True)): | ||
| if (zaak_url := request.query_params.get("zaak")) and zaak_urls != [ |
There was a problem hiding this comment.
Could you make sure this query parameter is documented for DELETE in the OAS? Also you could probably switch the ordering of the two if statements to do (zaak_url := request.query_params.get("zaak")) before performing the query to get the zaak references, to avoid unnecessarily doing that query
| ]: | ||
| # OAB is archiving one of many ZAKEN attached to this object; | ||
| # just remove this single zaak reference. | ||
| zaak_references.filter(url="zaak_url").delete() |
There was a problem hiding this comment.
Could you make sure an assertion for this is part of the tests as well? Since the tests currently pass despite this
| zaak_references.filter(url="zaak_url").delete() | |
| zaak_references.filter(url=zaak_url).delete() |
| response = Response( | ||
| {"behouden": [object_url]}, status=status.HTTP_200_OK | ||
| ) |
There was a problem hiding this comment.
Could you make sure this response body + status code is also part of the OAS? Open Klant has this documented as well: https://github.com/maykinmedia/open-klant/blob/b07ad28c057228e90d1c97163a69eb0bc2bfadd9/src/openklant/components/klantinteracties/api/viewsets/klantcontacten.py#L343-L349
| f"{url}?{urlencode({'zaak': zaak_1})}", *GEO_WRITE_KWARGS | ||
| ) | ||
|
|
||
| self.assertEqual(response.status_code, status.HTTP_200_OK, response.data) |
There was a problem hiding this comment.
Let's also add an assertion to verify that the object still exists + that it still has a reference to https://example.com/zaak/2
Fixes #708