-
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
[#2871] Refactor ZGW imports to work on different environments #1497
Conversation
1550dbd
to
7bca41d
Compare
7bca41d
to
a5c818e
Compare
3c6db4f
to
5d89630
Compare
…aints - use (domein, rsin) as natural key for CatalogusConfig - use combination of `omschrivjing` and (catalogus_domain, catalogus_rsin) for natural keys of nested ZGW configs (ZaakTypeConfig, ZaakTypeInformatieObjectTypeConfig etc.) and use (domein, rsin) support import/export of ZGW objects across different environments (where the url is different)
5d89630
to
6b34be3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1497 +/- ##
=========================================
Coverage 94.29% 94.30%
=========================================
Files 1066 1067 +1
Lines 40180 40295 +115
=========================================
+ Hits 37889 38001 +112
- Misses 2291 2294 +3 ☔ View full report in Codecov by Sentry. |
e451213
to
2198320
Compare
def update_zaaktype_config(source_config: ZaakTypeConfig, jsonl: str): | ||
try: | ||
target = ZaakTypeConfig.objects.get_by_natural_key( | ||
identificatie=source_config.identificatie, | ||
catalogus_domein=source_config.catalogus.domein, | ||
catalogus_rsin=source_config.catalogus.rsin, | ||
) | ||
except ZaakTypeConfig.MultipleObjectsReturned as exc: | ||
raise ZGWImportError.from_exception_and_jsonl(exc, jsonl) | ||
except (CatalogusConfig.DoesNotExist, ZaakTypeConfig.DoesNotExist) as exc: | ||
raise ZGWImportError.from_exception_and_jsonl(exc, jsonl) | ||
else: | ||
exclude_fields = [ | ||
"id", | ||
"catalogus", | ||
] | ||
for field in source_config._meta.fields: | ||
field_name = field.name | ||
|
||
if field_name in exclude_fields: | ||
pass |
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 generally feel there is a fair bit of duplication in these functions. I wonder whether we couldn't simply inline all of these functions in the match
statement of the main loop, whereby each branch would define the target and the exclude fields, and then the loop would only do the updating logic once.
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 guess the related object handling is slightly different, no worries if it's too much hassle, but it might make sense to mark the functions as private with the _
prefix).
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 moved the exclude_fields
into the match statement, which makes the helper functions slimmer. I'm not sure more inlining would improve readability further, due to the difference between CatalogusConfig
, ZaaktypeConfig
, and the nested configs (the match statement would get pretty crowded).
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.
Nice work, few suggestions here and there.
15cecb9
to
d1c148e
Compare
- overwrite only editable fields when importing ZGW entities, skip read-only fields (url, domein, rsin)
d1c148e
to
24fffb7
Compare
CatalogusConfig
and a combination ofomschrijving
+zaaktype_identificatie
for nested ZGW objects likeZaakTypeStatusTypeConfig
to support import/export of ZGW objects across different environments (where the url is different)Taiga: https://taiga.maykinmedia.nl/project/open-inwoner/task/2871