Skip to content
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

[#2526] Allow for additional ZGW service backends to be configured #1240

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

swrichards
Copy link
Contributor

This is a first step towards allowing multiple ZGW backends throughout the platform. To maintain backwards compatibility with the current datamodel, we proxy the current service config fields to the new ZGWApiGroupConfig model as we work our way towards making all ZGW client invocations multi-backend aware.

@swrichards swrichards force-pushed the tasks/2526-additional-zgw-service-config branch from caca0df to 0fdb750 Compare June 5, 2024 09:56

dependencies = [
("zgw_consumers", "0019_alter_service_uuid"),
("openzaak", "0047_delete_statustranslation"),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note we depend on 0047 rather than 0048 on account of #1228

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 89.31298% with 14 lines in your changes missing coverage. Please review.

Project coverage is 95.20%. Comparing base (6157084) to head (dc065b0).
Report is 6 commits behind head on develop.

Files Patch % Lines
...s/0050_migrate_zgw_root_fields_to_multi_backend.py 58.33% 10 Missing ⚠️
src/open_inwoner/openzaak/models.py 94.02% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1240      +/-   ##
===========================================
- Coverage    95.22%   95.20%   -0.03%     
===========================================
  Files          974      978       +4     
  Lines        35471    35595     +124     
===========================================
+ Hits         33778    33888     +110     
- Misses        1693     1707      +14     

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

@swrichards swrichards force-pushed the tasks/2526-additional-zgw-service-config branch 3 times, most recently from 4adab74 to 1b32755 Compare June 5, 2024 16:17
@swrichards swrichards marked this pull request as ready for review June 5, 2024 16:18
@swrichards swrichards force-pushed the tasks/2526-additional-zgw-service-config branch from 1b32755 to 715bc06 Compare June 5, 2024 20:49
@swrichards swrichards force-pushed the tasks/2526-additional-zgw-service-config branch from 715bc06 to bd2e67b Compare June 5, 2024 20:52
@alextreme
Copy link
Member

Seems okay, also for @pi-sigma to review first after his holiday

@alextreme
Copy link
Member

@stevenbal review vanuit jouw kant zou wenselijk zijn hiervoor, dit hadden we volgens mij in april samen met Bart al voorbesproken maar lag even stil

Copy link
Contributor

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor remarks, looks good overall 👍

src/open_inwoner/openzaak/admin.py Outdated Show resolved Hide resolved
Comment on lines +53 to +55
open_zaak_config = models.ForeignKey(
"openzaak.OpenZaakConfig", on_delete=models.PROTECT, related_name="api_groups"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenZaakConfig is a SingletonModel and there will only be one instance of it, so I don't think we need a ForeignKey to it on this model and can just use .get_solo if we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FK approach feels simpler and cheaper to me: it's more declarative and avoids custom logic on both sides of the relation (.get_solo and ZGWApiGroupConfig.objects.all()) and in the admin (which would be needed because we would lack a declared through-field). But if you feel strongly the other way I am happy to make the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah that's true, I don't feel too strongly about either way, so it's fine

src/open_inwoner/openzaak/tests/test_migrations.py Outdated Show resolved Hide resolved
@swrichards
Copy link
Contributor Author

Some minor remarks, looks good overall 👍

Useful comments @stevenbal, thanks. Just one open question left in #1240 (comment).

@swrichards swrichards force-pushed the tasks/2526-additional-zgw-service-config branch from bd2e67b to e41c456 Compare June 14, 2024 08:12
This is a first step towards allowing multiple ZGW backends
throughout the platform. To maintain backwards compatibility
with the current datamodel, we proxy the current service
config fields to the new ZGWApiGroupConfig model as we
work our way towards making all ZGW client invocations
multi-backend aware.
@swrichards swrichards force-pushed the tasks/2526-additional-zgw-service-config branch from e41c456 to dc065b0 Compare June 14, 2024 08:15
@alextreme alextreme merged commit a7fca15 into develop Jun 17, 2024
16 checks passed
@alextreme alextreme deleted the tasks/2526-additional-zgw-service-config branch June 17, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants