-
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
✨ [#1868] Implement Mijn vragen for companies #904
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #904 +/- ##
===========================================
+ Coverage 92.91% 92.92% +0.01%
===========================================
Files 817 818 +1
Lines 28027 28212 +185
===========================================
+ Hits 26041 26217 +176
- Misses 1986 1995 +9 ☔ View full report in Codecov by Sentry. |
459ca9b
to
69d0276
Compare
69d0276
to
1d41a6e
Compare
I moved the feature flag to toggle between RSIN/KvK to the OpenKlant config, so this PR is ready for review @alextreme @pi-sigma |
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.
Looks good, nice work. @pi-sigma after your review I'll do a final check
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.
I wonder if some of the duplication in openklant.wrap
could/should be removed, along the lines of the comment I left. Perhaps I've missed a reason for writing different functions that specifically work with bsn
, kvk
, and rsin
. Otherwise looks good to me.
kcm = fetch_klantcontactmoment_for_kvk_or_rsin( | ||
kwargs["kcm_uuid"], kvk_or_rsin | ||
) | ||
|
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.
- We're performing the same check in the list and detail view.
- The code for
fetch_klantcontactmoment_for_bsn
andfetch_clantcontactmoment_for_kvk_or_rsin
inwrap.py
is (almost?) identical, ditto for the list versions.
Perhaps we could make the function more generic, i.e. fetch_klantcontactmoment
, which works with bsn
, kvk
, and rsin
. We could perform this check and grab the relevant data in the KlantContactMomentBaseView
and store it in the context as USER_IDENTIFIER
, which could be bsn
, kvk
, or rsin
. Then we could do kcm = fetch_klant_contactmoment(USER_IDENTIFIER)
.
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're right, it was a bit too much copy pasting. I modified it 904b902, I chose not to add the fetch parameters to the context data, because this check is also performed in another view (+ we don't want to expose things in context
that we're not gonna use in our templates).
0ae99d0
to
1263111
Compare
1263111
to
904b902
Compare
task: https://taiga.maykinmedia.nl/project/open-inwoner/task/1868