[#2863] When retrieving eHerkenning-cases, filter on either vestigingsnummer or rsin/kvk, but not both

[alextreme]

No description provided.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.29%. Comparing base (5e660ba) to head (74d168e).
Report is 7 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/open_inwoner/cms/cases/views/mixins.py	92.30%	1 Missing ⚠️
src/open_inwoner/openzaak/clients.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1488   +/-   ##
========================================
  Coverage    94.29%   94.29%           
========================================
  Files         1066     1066           
  Lines        40141    40185   +44     
========================================
+ Hits         37849    37894   +45     
+ Misses        2292     2291    -1     

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

[alextreme]

Waiting until I can conform this works as expected

[alextreme]

Confirmed to be working for Venray, to be double-checked by both @pi-sigma and @swrichards

[pi-sigma]

Looking good. The vestigingsnummer is validated in the CompanyBranchChoiceForm to correspond to one of the branches retrieved from the KVK API. The logic for retrieval/validation of vestigingsnummer will have to be adapted when when we support eHerkenning login scoped to a vestigingsnummer.

[alextreme]

Converting to a draft PR, as when clicking through the user gets a 403

[alextreme]

@pi-sigma FYI my first approach was too simplistic, in that retrieval using vestigingnummer is also done within the openzaak app. A few tests still fail but I'm not sure if this is due to having to update the mocks or that my approach is incorrect.

[pi-sigma]

@alextreme The test for retrieving cases with vestigingsnummer failed because the value of fetch_cases_for_company is cached and the cache is not cleared between subsequent subtest runs, which resulted in cache hits because we're no longer supplying kvk or rsin. I decided to break up the test into two separate tests. This means code duplication, but I'd rather deal with this than investigating the caching mechanism + subtest behavior every time something goes wrong.

[alextreme]

@swrichards FYI, and I'm starting to agree with Paul's opinion that subtests are making debugging failing tests quite a bit more complicated (in this case it was anything but obvious why a test is failing from the Github actions output), we may need to re-evaluate using them. But because of this please do a double-check

[swrichards]

@swrichards FYI, and I'm starting to agree with Paul's opinion that subtests are making debugging failing tests quite a bit more complicated (in this case it was anything but obvious why a test is failing from the Github actions output), we may need to re-evaluate using them. But because of this please do a double-check

I think parametrized tests are a very valuable pattern that improves test readability and coverage, but at the same time, I Agree subTest is often more trouble than it's worth as it does not provide full test isolation (see elsewhere mocks, Django messages, etc. which are not properly cleared). For this reason I would prefer pytest which has first-class support for test parametrization, among other nice things, but that's another discussion for another time. TLDR: let's avoid subTest for anything that involves Django components or complex mocks (I still think it's fine for simple unit tests involving pure'ish functions).

[alextreme]

Did a bit of debugging this evening to figure this out, and noticed that the roles were being checked versus the kvk/rsin via the detail mixin. Also increased the loglevel to info and ensured that the logmessages were distinct from each other to assist in figuring this out quicker next time

[alextreme]

To be verified on Venray install just in case

[alextreme]

@pi-sigma solution verified to be working on venray testenvironment