Unchecking deceased children by default

[robinmolen]

Closes #5857

Changes

Changing the default value of the Family members prefill plugin option include_deceased to False. With that, the Family member prefill plugin tests have also been updated, to reflect the new default situation.

Checklist

Check off the items that are completed or not relevant.

• Impact on features

  • Checked copying a form
  • Checked import/export of a form
  • Config checks in the configuration overview admin page
  • Checked new model fields are usable in the admin
  • Problem detection in the admin email digest is handled

• Dockerfile/scripts

  • Updated the Dockerfile with the necessary scripts from the ./bin folder

• Commit hygiene

  • Commit messages refer to the relevant Github issue
  • Commit messages explain the "why" of change, not the how

• Documentation

  • Added documentation which describes the changes

[vaszig]

• Looking at the ticket I am a bit confused. I thought that according to this #5857 (comment) we would expect an answer from NL DS (maybe you are aware of something else though).

• Maybe we should change the default values for these too?
  https://github.com/open-formulieren/open-forms/blob/main/src/openforms/contrib/haal_centraal/clients/brp.py#L211
  https://github.com/open-formulieren/open-forms/blob/main/src/stuf/stuf_bg/client.py#L105

[robinmolen]

Looking at the ticket I am a bit confused. I thought that according to this #5857 (comment) we would expect an answer from NL DS (maybe you are aware of something else though).

During the previous refinement (9th of March) we decided that we can safely pick this up, and if municipalities expected it differently they would let us know.

Maybe we should change the default values for these too?
https://github.com/open-formulieren/open-forms/blob/main/src/openforms/contrib/haal_centraal/clients/brp.py#L211
https://github.com/open-formulieren/open-forms/blob/main/src/stuf/stuf_bg/client.py#L105

Good point! I've updated the default include_deceased values of these functions 👍

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.07%. Comparing base (25c6806) to head (936078e).
⚠️ Report is 32 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6065      +/-   ##
==========================================
- Coverage   97.09%   97.07%   -0.02%     
==========================================
  Files         883      883              
  Lines       33192    33189       -3     
  Branches     2973     2972       -1     
==========================================
- Hits        32228    32219       -9     
- Misses        648      652       +4     
- Partials      316      318       +2     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[vaszig]

I have some questions/remarks again!

Apart from the ones below I have one more concerning the mocked files in the openforms/contrib/haal_centraal/tests/files, I would expect that the ones for v2 are not necessary any more, no?

[vaszig]

This is confusing to me..This is a test concerning v1.3 according to the class attribute, but here we use a BSN which is not part of the mocked data in ingeschrevenpersonen.v1-full.json (this comment is relevant with the tests below too).

I mean we use requests_mock here but we get children from the container, not the mocked data.

[robinmolen]

The reason why i updated the BSN values, and the responses, was to keep the v2 and v1.3 tests aligned. The tests use the same setup which decides the get_family_members() function parameters and expected results.

I thought updating the v1.3 tests conform the container data was the correct thing to do, but if its not let me know. I do see i've missed some BSN value updates in the results, i'll correct those

[vaszig]

In my opinion this is a bit confusing. I would expect the tests for v2 to use the container and the the v1 the mocked data (since the container is part of the testing suite for v2 - https://brp-api.github.io/Haal-Centraal-BRP-bevragen/v2/getting-started#probeer-en-test-de-api-lokaal). So, if sharing a class is not the case any more I would split them. @sergei-maertens do you agree?

[sergei-maertens]

The point of sharing a base class was to enforce the same input would lead to the same output for both versions of the API.

If we must align the test BSNs, then I'd also update the mock data so that the test itself remains consistent with the mock data, while also being consistent with the v2 test.

Separating the test classes entirely introduces the risk that we have different behaviours.

On the other hand, afaik nobody actually uses BRP v1.3 as it never really went into production, and it is scheduled for removal in Open Forms 4.0 (see #6087) so let's not spend too much effort on this.

[robinmolen]

In these tests the v1.3 mock files are (and were) used twice as input. For these tests the v2 tests can use the same BSN values, as this results into the same test result. For the tests that needed to be updated, we could create mock files, but that would just result into moving the json data around.

The v1.3 mock files are also used in other tests, updating these will create more work. And considering BRP v1.3 will most likely be removed, its probably not worth it.

So, i think we can safely keep it as is

[robinmolen]

Edit: the v2 test files are used by other tests, like CoSignPrefillV2Tests. So we cannot delete them just yet. These tests should probably also be updated to use VCR, but i think thats out of scope

[sergei-maertens]

Does the github release issue template need updates with the new cassette paths?

[robinmolen]

There is indeed a small update for the prepare release template.

It now names openforms.contrib.haal_centraal.tests.test_integration specifically, i'll make it more global as multiple files in the openforms.contrib.haal_centraal namespace now use VCR cassettes.