[#2484] case detail pages are multi-zgw-backend-aware #1323
Conversation
7868e76 to
c43a233
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1323 +/- ##
===========================================
- Coverage 95.12% 95.09% -0.04%
===========================================
Files 993 993
Lines 36121 36184 +63
===========================================
+ Hits 34361 34408 +47
- Misses 1760 1776 +16 ☔ View full report in Codecov by Sentry. |
7fc7021 to
8d3d5a6
Compare
There was a problem hiding this comment.
Looking good and seems to work at least when tested locally. One thing I noticed is that when retrieving an individual case from the case list, we get an ape_pie.exceptions.InvalidURLError. This is not caused by your PR (happens on develop as well, but something we need to look into.
| # Note: we cannot be sure which zaken_client to pass here. However, given that it | ||
| # is only used to resolve the zaak, and we do that anyway below, it is safe to pass | ||
| # None. | ||
| ocm = client.retrieve_objectcontactmoment(kcm.contactmoment, "zaak", None) |
There was a problem hiding this comment.
The ContactMomentClient uses the zaken_client to fetch the case and store it on the ocm in case it's not present (see https://github.com/maykinmedia/open-inwoner/blob/develop/src/open_inwoner/openklant/clients.py#L292). How is this going to work if the zaken_client is None? In light of the passing tests, this either works anyway or we're missing a test case.
There was a problem hiding this comment.
Discussed with @swrichards:
- we only use the result of
retrieve_objectcontactmomentto get the url of thezaak, so not passing azaken_clientis fine. - fetching data from the API and processing the data (e.g. resolving resources) should be kept separate. Hence the underlying function(s) that retrieve and resolve resources should be refactored.
| # In principle this should not happen, a zaak should be stored in | ||
| # exactly one backend. But: https://www.xkcd.com/2200/ | ||
| # We should at least receive a ping if this happens. | ||
| logger.error("Found one zaak in multiple backends") |
There was a problem hiding this comment.
Wouldn't it be better to raise an exception if this happens? The control flow would then be:
if (case_count := len(cases_found)) == 0:
logger.error(...)
elif(case_count > 1:
raise Exception
else:
happy flow
| @@ -35,27 +35,27 @@ | |||
| name="kcm_redirect", | |||
| ), | |||
| path( | |||
| "<str:object_id>/document/<str:info_id>/", | |||
| "<str:api_group_id>/<str:object_id>/document/<str:info_id>/", | |||
There was a problem hiding this comment.
| "<str:api_group_id>/<str:object_id>/document/<str:info_id>/", | |
| "<str:object_id>/<str:api_group_id>/document/<str:info_id>/", |
I find the reverse order of api_group_id and object_id more natural, as the latter is the target resource of a request. Similar below.
8d3d5a6 to
aeb116d
Compare
The big change here is that the URL structure for case detail pages, and associated pages such as case document download, now specify the API group to be used in fetching the case details. This is because if we go on object id alone, we have no way of knowing where to fetch the zaak (other than fetching from all backends, which is not desirable, or maintaining an internal mapping).
In other cases, we can do some resolving from contextual hints (such as an URL, if present), but mostly we have to do a lot of passing around of the API group info.