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

[#3524] Ignore family members without BSN #3561

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

SilviaAmAm
Copy link
Contributor

Fixes #3524

@SilviaAmAm SilviaAmAm marked this pull request as draft October 27, 2023 10:07
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (df68b79) 95.97% compared to head (4db05df) 95.97%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3561   +/-   ##
=======================================
  Coverage   95.97%   95.97%           
=======================================
  Files         679      679           
  Lines       21840    21844    +4     
  Branches     2519     2521    +2     
=======================================
+ Hits        20960    20964    +4     
  Misses        610      610           
  Partials      270      270           
Files Coverage Δ
src/openforms/contrib/haal_centraal/clients/brp.py 100.00% <100.00%> (ø)

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

@SilviaAmAm SilviaAmAm marked this pull request as ready for review October 27, 2023 10:32
Copy link
Contributor

@CharString CharString left a comment

Choose a reason for hiding this comment

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

It's fine, so I approve, just a remark and a question:

  • The v2 mocks return a different response for essentially the same call to the same endpoint. If I understand correctly, the real endpoint response would always contain both children and partners. I'd try to avoid these kinds of mocks that assume a certain implementation. In this case it never tries to reference the partners key if they aren't requested. For instance: a refactor of the implementation that always processes everything from the api response and only filters just what's being asked in the answer, would work against the real api, but trigger these tests. (contrived example, but in essence, I like be able to refactor without changing the test suite and have the tests as a canary in the coal mine to detect when I broke something)

  • Is just mentioning the omission of people without BSN is enough information for a form designer to be able to cater for these cases? Do we need to give examples or suggestions, like a repeating group for "missing familiy members"? I guess they already have to deal with submitters that have no BSN themselves and when the BRP services are unreachable for some reason.

docs/manual/forms/form_fields.rst Outdated Show resolved Hide resolved
@sergei-maertens
Copy link
Member

sergei-maertens commented Oct 27, 2023

  • Is just mentioning the omission of people without BSN is enough information for a form designer to be able to cater for these cases? Do we need to give examples or suggestions, like a repeating group for "missing familiy members"? I guess they already have to deal with submitters that have no BSN themselves and when the BRP services are unreachable for some reason.

They definitely can't cater for these cases, but to be able to do that they're going to have to tell us how they expect to handle that. I think we have too little domain knowledge and attempts to get that knowledge from actual teams building forms have been unproductive.

@sergei-maertens
Copy link
Member

My sausage fingers accidentally closed the PR, sorry!

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Please rebase on master to sort out the CI failures

@sergei-maertens sergei-maertens force-pushed the fix/3524-partner-no-bsn branch from 410ed1b to 4db05df Compare October 30, 2023 18:52
@sergei-maertens sergei-maertens merged commit f221ef6 into master Oct 31, 2023
25 checks passed
@sergei-maertens sergei-maertens deleted the fix/3524-partner-no-bsn branch October 31, 2023 10:04
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.

Deal with the case where Haal Centraal returns a partner without BSN
3 participants