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.

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

[#2560] Treat catalogus as a required field on zaaktype response #1259

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

swrichards
Copy link
Contributor

Previously we allowed for two edge-cases in resolving zaaktypes:

  1. We allowed for catalogus to be absent on a ZaakType to accommodate eSuite, in which the field was absent.
  2. We allowed for importing a ZaakType if it included a URL pointing to a Catalogus that had not previously been created.

This commit no longer allows for these exceptions. With regards to 1, eSuite has since implemented changes to include the catalogus field. As for 2: it was already an edge-case we felt should raise rather than be suppressed, but this is especially true now as we're moving to support multiple ZGW backends. In that scenario, we'll clearly want to only include ZaakTypes from a known Catalogus API, to avoid cross-backend ambiguity.

@swrichards swrichards force-pushed the tasks/2560-catalogus-is-required-on-zaaktype branch from c030bc7 to 2601f8b Compare June 17, 2024 12:54
@swrichards swrichards marked this pull request as ready for review June 17, 2024 12:58
@swrichards swrichards requested a review from pi-sigma June 17, 2024 12:58
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.18%. Comparing base (a7fca15) to head (67f2d2b).
Report is 11 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1259      +/-   ##
===========================================
- Coverage    95.20%   95.18%   -0.02%     
===========================================
  Files          978      978              
  Lines        35595    35567      -28     
===========================================
- Hits         33888    33856      -32     
- Misses        1707     1711       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

try:
catalog = catalog_lookup[zaak_type.catalogus]
except KeyError:
raise ValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for raising ValueError instead of adding the msg to KeyError? According to the docs, ValueError is "Raised when an operation or function receives an argument that has the right type but an inappropriate value". Doesn't seem to be the case here.

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 thinking was that KeyError needlessly exposes the caller to opaque implementation details: the caller wasn't trying to access a mapping, the implementation was, and from their perspective it is not a KeyError. I also wanted to avoid adding a custom exception just for this case. Yet I agree ValueError seems off too. We could go for RuntimeError:

Raised when an error is detected that doesn’t fall in any of the other categories. The associated value is a string indicating what precisely went wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

RuntimeError seems fine as well, though I would then except KeyError as exc and raise RunTimeError from exc so the stack trace is preserved. I wonder though if it's worth the effot to apply this distinction of perspectives to our case, since we are the caller (the calling code is even in the same package). I find this more intuitive in the case of a library.

@swrichards swrichards requested a review from pi-sigma June 17, 2024 14:57
Previously we allowed for two edge-cases in resolving zaaktypes:

1. We allowed for catalogus to be absent on a ZaakType to
   accommodate eSuite, in which the field was absent.
2. We allowed for importing a ZaakType if it included a URL
   pointing to a Catalogus that had not previously been
   created.

This commit no longer allows for these exceptions. With regards
to 1, eSuite has since implemented changes to include the
catalogus field. As for 2: it was already an edge-case we
felt should raise rather than be suppressed, but this is
especially true now as we're moving to support multiple
ZGW backends. In that scenario, we'll clearly want to
only include ZaakTypes from a known Catalogus API, to avoid
cross-backend ambiguity.
@swrichards swrichards force-pushed the tasks/2560-catalogus-is-required-on-zaaktype branch from 2601f8b to 67f2d2b Compare June 17, 2024 15:25
@alextreme alextreme merged commit 3559436 into develop Jun 18, 2024
16 checks passed
@alextreme alextreme deleted the tasks/2560-catalogus-is-required-on-zaaktype branch June 18, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants