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

✨ [open-zaak/open-notificaties#156] Several changes to Kanalen #14

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

stevenbal
Copy link
Collaborator

@stevenbal stevenbal commented Aug 30, 2024

Related issue: open-zaak/open-notificaties#156

Requires zgw-consumers to be upgraded in Open Zaak

Changes:

  • Support nested fields on kanaal kenmerken

@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.21%. Comparing base (7abd32e) to head (68c8ba8).

Files with missing lines Patch % Lines
notifications_api_common/kanalen.py 92.85% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
- Coverage   89.23%   89.21%   -0.03%     
==========================================
  Files          29       29              
  Lines         604      612       +8     
==========================================
+ Hits          539      546       +7     
- Misses         65       66       +1     
Flag Coverage Δ
89.21% <93.33%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@stevenbal stevenbal force-pushed the feature/kanaal-kenmerken-nested-fields branch 2 times, most recently from 3d1f4b7 to dfce6ea Compare September 26, 2024 14:22
@stevenbal stevenbal marked this pull request as draft September 26, 2024 14:23
@stevenbal stevenbal force-pushed the feature/kanaal-kenmerken-nested-fields branch 4 times, most recently from 410e968 to d0ea73e Compare September 27, 2024 12:18
@stevenbal stevenbal force-pushed the feature/kanaal-kenmerken-nested-fields branch from 64885ff to a1caa4c Compare October 24, 2024 09:21
@alextreme
Copy link
Member

@stevenbal see if you can revisit this PR next week now that open-zaak/open-zaak#1800 has been completed

@stevenbal stevenbal force-pushed the feature/kanaal-kenmerken-nested-fields branch 2 times, most recently from 741e8d9 to 99e77b4 Compare December 5, 2024 08:59
@stevenbal stevenbal marked this pull request as ready for review December 5, 2024 09:00
Copy link

@Coperh Coperh left a comment

Choose a reason for hiding this comment

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

Support nested fields on kanaal kenmerken

I guess this is suppose the extra_kwargs[kenmerk_1][field_name] and is for supporting stuff added in open zaak rather than here?

Have some comments and I do not think the nested fields are actually tested or at least the help text

notifications_api_common/kanalen.py Outdated Show resolved Hide resolved
self,
obj: Model,
data: Union[Dict, None] = None,
request: Union[Request, None] = None, # noqa
Copy link

Choose a reason for hiding this comment

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

What does this request do?

Copy link

Choose a reason for hiding this comment

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

Could this just not be in data?

edit: actually probably not

Copy link

Choose a reason for hiding this comment

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

Actually again this is something fore open zaak?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, this is necessary for Open Zaak to create the URLs: https://github.com/open-zaak/open-zaak/pull/1757/files#diff-b74ad95db3a00961f14403f57ab65597401cc8b8b1ef88e162d3214c4baf70d6R54.

data is the actual request data (JSON) that is posted, so it can't really be a part of that I think

tests/test_register_webhook.py Outdated Show resolved Hide resolved
tests/test_register_webhook.py Outdated Show resolved Hide resolved
notifications_api_common/kanalen.py Show resolved Hide resolved
@stevenbal
Copy link
Collaborator Author

I guess this is suppose the extra_kwargs[kenmerk_1][field_name] and is for supporting stuff added in open zaak rather than here?

that's right, I've added the help_text stuff to be able to mark the help text for zaaktype.catalogus as experimental in Open Zaak

@stevenbal stevenbal changed the title ✨ [open-zaak/open-notificaties#156] Support nested fields on kanaal kenmerken ✨ [open-zaak/open-notificaties#156] Several changes to Kanaal Dec 6, 2024
@stevenbal stevenbal changed the title ✨ [open-zaak/open-notificaties#156] Several changes to Kanaal ✨ [open-zaak/open-notificaties#156] Several changes to Kanalen Dec 6, 2024
@stevenbal stevenbal requested a review from Coperh December 6, 2024 11:12
Copy link

@Coperh Coperh left a comment

Choose a reason for hiding this comment

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

Yep, looks good.

Copy link
Contributor

@annashamray annashamray left a comment

Choose a reason for hiding this comment

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

Looks good

notifications_api_common/kanalen.py Outdated Show resolved Hide resolved
notifications_api_common/kanalen.py Outdated Show resolved Hide resolved
@stevenbal stevenbal force-pushed the feature/kanaal-kenmerken-nested-fields branch from 90b3d3d to 1a0cd10 Compare December 12, 2024 12:59
@stevenbal stevenbal marked this pull request as draft December 12, 2024 13:31
@stevenbal stevenbal force-pushed the feature/kanaal-kenmerken-nested-fields branch from 1a0cd10 to 48b9571 Compare December 13, 2024 14:18
@stevenbal stevenbal marked this pull request as ready for review December 17, 2024 09:27
@stevenbal stevenbal merged commit 3300aed into main Dec 17, 2024
7 checks passed
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