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

[#2723] Support multiple backends for Notifications API #1378

Merged
merged 4 commits into from
Sep 16, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Sep 3, 2024

Taiga: https://taiga.maykinmedia.nl/project/open-inwoner/task/2723

Related: https://taiga.maykinmedia.nl/project/open-inwoner/task/2700

Note: the notificaties.test.openzaak backend does not set an auth header when sending test notifications for newly created subscriptions, which can lead to false positive error logs when testing webhooks (cf. https://sentry.maykinmedia.nl/organizations/maykin-media/issues/363204/?project=284)

@pi-sigma pi-sigma force-pushed the task/2723-notification-backends branch from a55e8de to 4f7894c Compare September 4, 2024 12:36
@pi-sigma pi-sigma changed the title [#2723] Support multiple notification APIs [#2723] Support multiple backends for Notifications API Sep 4, 2024
@pi-sigma pi-sigma force-pushed the task/2723-notification-backends branch from 4f7894c to 3b0dc38 Compare September 4, 2024 13:02
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 90.33613% with 23 lines in your changes missing coverage. Please review.

Project coverage is 94.94%. Comparing base (4bc9462) to head (912095b).
Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
...0002_migrate_data_from_notifications_api_common.py 66.66% 15 Missing ⚠️
src/notifications/models.py 91.30% 4 Missing ⚠️
src/notifications/validators.py 88.88% 2 Missing ⚠️
src/notifications/admin.py 95.00% 1 Missing ⚠️
src/notifications/tests/utils.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1378      +/-   ##
===========================================
- Coverage    94.96%   94.94%   -0.03%     
===========================================
  Files         1032     1043      +11     
  Lines        37949    38290     +341     
===========================================
+ Hits         36039    36353     +314     
- Misses        1910     1937      +27     

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

@pi-sigma pi-sigma force-pushed the task/2723-notification-backends branch from 3b0dc38 to 99027dc Compare September 5, 2024 14:48
@pi-sigma pi-sigma marked this pull request as ready for review September 6, 2024 14:19
@pi-sigma pi-sigma force-pushed the task/2723-notification-backends branch 2 times, most recently from bd94d65 to 351e2aa Compare September 7, 2024 09:24
src/notifications/tests/test_migrations.py Outdated Show resolved Hide resolved
src/notifications/tests/test_migrations.py Outdated Show resolved Hide resolved
# tasks from showing up in the admin when OIP is run with
# Docker; this needs to be fixed this in the library eventually;
# for now we load it after all our apps.
"notifications_api_common",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Further to my other comments: I think this needs to stay in for the moment. We really only want this to be removed once we're positive the migration has been completed.

We should check in with @sergei-maertens -- he once mentioned they had a similar issue in open form, and the solution involved using the checks framework to ensure users upgrade via a certain path (e.g., you cannot go from 1.x to 1.z without first going through 1.y). With something like that, we could enforce that the migration is run before the users upgrade to a version that drops the package.

Copy link
Member

Choose a reason for hiding this comment

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

This is the implementation: https://github.com/open-formulieren/open-forms/blob/a4cfd74aa47a547c7a36055daf8611af128b7bba/src/openforms/upgrades/upgrade_paths.py#L78

Next week is Open Forms off-week - I could use that time to extract this code into a re-usable django-app/package that you could then use in OIP for these kind of migrations. Would that be acceptable?

src/notifications/models.py Outdated Show resolved Hide resolved
from rest_framework import status
from rest_framework.response import Response
from rest_framework.views import APIView
from zgw_consumers.api_models.base import factory

from notifications.api.serializers import NotificatieSerializer
from open_inwoner.configurations.models import SiteConfiguration
from open_inwoner.openzaak.api_models import Notification
from open_inwoner.openzaak.auth import get_valid_subscription_from_request
Copy link
Contributor

Choose a reason for hiding this comment

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

This still has references to notifications_api_common, and is I think where the logic for going from a singleton to many configs would also have to be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked: the only lingering references to notifications_api_common are in INSTALLED_APPS, the data migration and the associated test.

I am not sure we need special logic in the NotificationsWebhookBaseView to handle different backends (in the way we need special logic to handle differen ZGW backends). We need to be able to register webhooks with different backends, but this is now possible. When a request comes in at the NotificationsWebhookBaseView, we determine the subscription from the request, create a notification from the data and pass it on to the notification handler.

@pi-sigma pi-sigma force-pushed the task/2723-notification-backends branch 4 times, most recently from 91a1034 to 1278f77 Compare September 13, 2024 09:02
Copy link
Contributor

@swrichards swrichards left a comment

Choose a reason for hiding this comment

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

One nitpick, otherwise looks good to me.

src/notifications/admin.py Show resolved Hide resolved
@alextreme alextreme merged commit 7f55115 into develop Sep 16, 2024
20 checks passed
@alextreme alextreme deleted the task/2723-notification-backends branch September 16, 2024 12:18
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