[#890] Downloadable zaakinformatieobject documenten #327

Bartvaderkin · 2022-11-15T11:35:25Z

No description provided.

src/open_inwoner/accounts/views/cases.py

Bartvaderkin · 2022-11-15T14:23:33Z

src/open_inwoner/openzaak/info_objects.py

+@dataclass
+class ZaakInformatieObject(ZGWModel):
+    url: str
+    informatieobject: str
+    zaak: str
+    # aard_relatie_weergave: str
+    titel: str
+    # beschrijving: str
+    registratiedatum: datetime
+
+
+def fetch_case_information_objects(case_url: str) -> List[ZaakInformatieObject]:
+    client = build_client("zaak")
+
+    if client is None:
+        return []
+
+    try:
+        response = client.list(
+            "zaakinformatieobject",
+            request_kwargs={
+                "params": {"zaak": case_url},
+            },
+        )
+    except RequestException as e:
+        logger.exception("exception while making request", exc_info=e)
+        return []
+    except ClientError as e:
+        logger.exception("exception while making request", exc_info=e)
+        return []
+
+    case_info_objects = factory(ZaakInformatieObject, response)
+
+    return case_info_objects


This was in another file that got crowded.

TBH I'm not sure that fetching ZaakInformatieObject belongs to this file, because it requests Zaken API

For now we have 3 Openzaak APIs (Zaken, Catalogi and now Documenten) which we requests. Let's agree hot we organize functions to fetch these APIs
It can be done based on:

API (3 files: each file to request resources from one particular API)

openinwoner views (separate functions needed for different views per view basis, but some functions can be used in several views)

@alextreme @Bartvaderkin @vaszig let's agree to one option and stick to it?
I like the first option, I'd suggest to have 3 files - cases, catalog and documents (or zaken, catalogi and documenten) and put all functions there

I agree with the first approach. It seems the most logical to me as well.

I agree with option 1. We'll need to coordinate how/when to refactor that.

Let's do it in the PR and we will merge it and rebase our branches and be done with this :)

I've added the refactoring to the PR. The commit-diff is interesting as it is trying to make sense of code movements.

…formatieobject documenten

Bartvaderkin · 2022-11-15T14:31:45Z

Note to the reviewer: I have another ticket about stronger access control, where we check if the case/document actually belong to the current user's BSN (taiga 912)

codecov-commenter · 2022-11-15T15:10:04Z

Codecov Report

Merging #327 (521c09f) into develop (10f5494) will increase coverage by 0.02%.
The diff coverage is 95.31%.

@@             Coverage Diff             @@
##           develop     #327      +/-   ##
===========================================
+ Coverage    96.45%   96.48%   +0.02%     
===========================================
  Files          419      426       +7     
  Lines        12724    12994     +270     
===========================================
+ Hits         12273    12537     +264     
- Misses         451      457       +6

Impacted Files	Coverage Δ
src/open_inwoner/accounts/urls.py	`100.00% <ø> (ø)`
src/open_inwoner/openzaak/clients.py	`80.95% <60.00%> (-6.55%)`	⬇️
src/open_inwoner/openzaak/info_objects.py	`88.23% <88.23%> (ø)`
src/open_inwoner/openzaak/utils.py	`92.85% <92.85%> (ø)`
src/open_inwoner/accounts/views/cases.py	`99.20% <97.95%> (-0.80%)`	⬇️
src/open_inwoner/accounts/views/__init__.py	`100.00% <100.00%> (ø)`
.../open_inwoner/components/templatetags/file_tags.py	`86.48% <100.00%> (+2.70%)`	⬆️
src/open_inwoner/openzaak/cases.py	`84.21% <100.00%> (-0.64%)`	⬇️
...migrations/0003_openzaakconfig_document_service.py	`100.00% <100.00%> (ø)`
...004_openzaakconfig_document_max_confidentiality.py	`100.00% <100.00%> (ø)`
... and 11 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

annashamray

looks nice, just several remarks

src/open_inwoner/openzaak/info_objects.py

annashamray · 2022-11-16T15:30:57Z

src/open_inwoner/openzaak/info_objects.py

+
+
+@dataclass
+class InformatieObject(ZGWModel):


Why not to reuse zgw_consumers.api_models.documenten.Document class?

@annashamray This was a change necessary for all due to the zgw_consumers models not being compatible with the e-Suite ZGW API, as documented here: https://taiga.maykinmedia.nl/project/open-inwoner/wiki/afwijkingen-e-suite-tov-zgw-standaard

In short, e-Suite ZGW API doesn't return all the keys in responses which zgw_consumers assumes does always return. This was at least the case this summer.

We can discuss this further IRL if necessary

Oh! I completely forgot (and maybe didn't know) about e-Suite
But taiga link includes only ZaakInformatieObject differences, it doesn't include any info about InformatieObject resource

So I'll keep this comment unresolved, but resolve the same comment about ZaakInformatieObject class

You're correct, we didn't request IO's earlier, only ZIO's.

I expect we will also need to tweak the IO model further to get this to work with e-Suite, so having an OIP-specific IO model makes this more workable. For this current iteration we can keep the scope to only integrating with Open Zaak, once this is on staging we can do further e-Suite testing to double-check if this also works there as expected.

The alternative is that we temporarily deploy this branch on acceptance.

Does e-Suite just doesn't add certain fields or do they have different fields altogether?

I think we should look into fixing the factory() function so it at least doesn't crash when fields are missing (it did that even for the IO model from ZGW and the test OpenZaak environment). Or we can keep two sets of dataclasses, one of OpenZaak and one for e-Suite.

I discussed with @annashamray and we leave it like this for now to land this PR, and then create an issue to look at refactoring the factory function so we can run e-Suite and OpenZaak from same dataclasses.

e-Suite adheres to the same JSON responses as OpenZaak, however with less fields. But the main difference is that empty fields aren't returned in the JSON response.

If you want we can investigate this on staging together if necessary based on the acceptance environments.

If it is just less fields then we should be able to improve the factory function to deal with that and set some default/empty values.

@sergei-maertens noted we can easily create E-Suite compatible dataclasses by extending the official dataclasses and overriding the problematic missing fields with fields with Optional datatype and a default value.

This would keep us from compromising our accuracy for their non-spec implementation and make the differences visible in the E-suite compatible dataclasses.

We should pick that up in a different task once this PR lands.

I created a task for this: https://taiga.maykinmedia.nl/project/open-inwoner/task/920

src/open_inwoner/openzaak/info_objects.py

annashamray · 2022-11-16T15:42:11Z

src/open_inwoner/openzaak/info_objects.py

+@dataclass
+class ZaakInformatieObject(ZGWModel):
+    url: str
+    informatieobject: str
+    zaak: str
+    # aard_relatie_weergave: str
+    titel: str
+    # beschrijving: str
+    registratiedatum: datetime
+
+
+def fetch_case_information_objects(case_url: str) -> List[ZaakInformatieObject]:
+    client = build_client("zaak")
+
+    if client is None:
+        return []
+
+    try:
+        response = client.list(
+            "zaakinformatieobject",
+            request_kwargs={
+                "params": {"zaak": case_url},
+            },
+        )
+    except RequestException as e:
+        logger.exception("exception while making request", exc_info=e)
+        return []
+    except ClientError as e:
+        logger.exception("exception while making request", exc_info=e)
+        return []
+
+    case_info_objects = factory(ZaakInformatieObject, response)
+
+    return case_info_objects


TBH I'm not sure that fetching ZaakInformatieObject belongs to this file, because it requests Zaken API

For now we have 3 Openzaak APIs (Zaken, Catalogi and now Documenten) which we requests. Let's agree hot we organize functions to fetch these APIs
It can be done based on:

API (3 files: each file to request resources from one particular API)

openinwoner views (separate functions needed for different views per view basis, but some functions can be used in several views)

@alextreme @Bartvaderkin @vaszig let's agree to one option and stick to it?
I like the first option, I'd suggest to have 3 files - cases, catalog and documents (or zaken, catalogi and documenten) and put all functions there

src/open_inwoner/openzaak/info_objects.py

src/open_inwoner/openzaak/tests/test_cases.py

src/open_inwoner/accounts/views/cases.py

annashamray · 2022-11-16T16:06:46Z

The test coverage is very nice!

Co-authored-by: Anna Shamray <[email protected]>

…ility()

… variant

src/open_inwoner/accounts/views/cases.py

…I appropriate files

…n code pattern

Bart van der Schoor added 2 commits November 15, 2022 09:44

[#890] Downloadable zaakinformatieobject documenten

d5c23c5

[#890] Added tests for downloadable zaakinformatieobject documenten

76bdf20

Bartvaderkin commented Nov 15, 2022

View reviewed changes

src/open_inwoner/accounts/views/cases.py Show resolved Hide resolved

Bartvaderkin commented Nov 15, 2022

View reviewed changes

[#890] Added status/confidentiality filtering for downloadable zaakin…

5e40ce3

…formatieobject documenten

Bartvaderkin force-pushed the feature/890-downlaodable-case-documents branch from 2998ac4 to 5e40ce3 Compare November 15, 2022 14:27

[#890] Cleanup lingering refactoring noise and unused imports

0b2e772

Bartvaderkin marked this pull request as ready for review November 15, 2022 16:01

Bartvaderkin mentioned this pull request Nov 15, 2022

[#912] Harden access control on case-details/documents #329

Merged

annashamray requested review from alextreme and vaszig November 16, 2022 15:26

annashamray requested changes Nov 16, 2022

View reviewed changes

Bartvaderkin and others added 4 commits November 17, 2022 09:28

Apply suggestions from code review

902cae0

Co-authored-by: Anna Shamray <[email protected]>

[#890] Applied some more PR feedback

adf4f05

[#890] Added docstring and code hardening to filter_info_object_visib…

2fe968d

…ility()

[#890] Refactored to DRY-up fetch_single_information_object() and its…

521c09f

… variant

annashamray reviewed Nov 17, 2022

View reviewed changes

src/open_inwoner/accounts/views/cases.py Show resolved Hide resolved

Bart van der Schoor added 2 commits November 17, 2022 10:52

[#890] Refactored Openzaak fetch_functions and dataclasses to live AP…

a730a66

…I appropriate files

[#890] Renamed the views in cases.py to be more consistent with commo…

759eb53

…n code pattern

Bartvaderkin requested a review from annashamray November 17, 2022 10:15

annashamray approved these changes Nov 17, 2022

View reviewed changes

Bartvaderkin assigned alextreme Nov 17, 2022

alextreme merged commit 9f84143 into develop Nov 17, 2022

alextreme deleted the feature/890-downlaodable-case-documents branch November 17, 2022 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#890] Downloadable zaakinformatieobject documenten #327

[#890] Downloadable zaakinformatieobject documenten #327

Bartvaderkin commented Nov 15, 2022

Bartvaderkin Nov 15, 2022

annashamray Nov 16, 2022

vaszig Nov 17, 2022

Bartvaderkin Nov 17, 2022

annashamray Nov 17, 2022

Bartvaderkin Nov 17, 2022

Bartvaderkin commented Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading

annashamray left a comment

annashamray Nov 16, 2022

alextreme Nov 16, 2022 •

edited

Loading

annashamray Nov 16, 2022

alextreme Nov 16, 2022

Bartvaderkin Nov 17, 2022

Bartvaderkin Nov 17, 2022 •

edited

Loading

alextreme Nov 17, 2022

Bartvaderkin Nov 17, 2022

Bartvaderkin Nov 17, 2022

Bartvaderkin Nov 17, 2022

annashamray Nov 16, 2022

annashamray commented Nov 16, 2022

[#890] Downloadable zaakinformatieobject documenten #327

[#890] Downloadable zaakinformatieobject documenten #327

Conversation

Bartvaderkin commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bartvaderkin commented Nov 15, 2022 • edited Loading

codecov-commenter commented Nov 15, 2022 • edited Loading

Codecov Report

annashamray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextreme Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bartvaderkin Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annashamray commented Nov 16, 2022

Bartvaderkin commented Nov 15, 2022 •

edited

Loading

codecov-commenter commented Nov 15, 2022 •

edited

Loading

alextreme Nov 16, 2022 •

edited

Loading

Bartvaderkin Nov 17, 2022 •

edited

Loading