-
Notifications
You must be signed in to change notification settings - Fork 6
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
[#2288] Handle None questions in case detail view #1132
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1132 +/- ##
========================================
Coverage 95.06% 95.06%
========================================
Files 940 940
Lines 33155 33169 +14
========================================
+ Hits 31518 31532 +14
Misses 1637 1637 ☔ View full report in Codecov by Sentry. |
ContactmomentenClient, | ||
"retrieve_objectcontactmomenten_for_zaak", | ||
autospec=True, | ||
return_value=[None], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mock is not accurate and doesn't seem to test the modification.
It makes retrieve_objectcontactmomenten_for_zaak()
return a list with a single None value.
While the code change doesn't handle that. It loops over the objectcontactmomenten
as if they can't be null and then does a null check on an attribute of an object in that list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modification ensures sure that object.contactmoment
is not None
, so that contactmoment.registratiedatum
further down doesn't raise AttributeError
. getattr(None, "contactmoment", None)
returns None
, in exactly the same way that getattr(objectcontactmoment, "contactmoment", None)
would return None
for an objectcontactmoment
that happens to lack a contactmoment
. The test will therefore pass with the modification and fail without it. I didn't see a reason to create a fake objectcontactmoment
; the mechnism is exactly the same (getattr -> AttributeError -> None
).
Also, the code does not assume AFAIK that objectcontactmomenten
can't be null; it creates an empty list, calls retrieve_objectcontactmomenten_for_zaak
to populate it, then loops over the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You made a modification that handles if an external sub resource is None, so the test should test with that same subresource being None. And not rely on some random other None value somewhere that happens to make the test pass for some unrelated reason.
With this test we still don't know if you modification works or makes sense, because the situation it handles is not present in the test.
3964ee1
to
5b6d746
Compare
Taiga: #2288