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

Add email notifications channel #17914

Merged
merged 31 commits into from
Apr 16, 2024

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Apr 5, 2024

Closes #17882

This PR adds a new email channel to the notifications system.

Backend changes

  • Includes 2 database migrations to add 2 new columns to the notification table.
    • dispatched column: will determine if this notification has been "dispatched" through the additional communication channels. Any existing notification will set this value to "True" so users won't get emails retroactively.
    • galaxy_url column: allows to build URLs from the notification channel plugins that don't have access to the web request context.
  • Move the creation of notifications (database objects) to a celery task if enabled. This can offload resolving the recipient users when we include complex combinations of groups and/or roles as recipients. We can also control if we want to explicitly run this synchronously when we don't use groups or roles.
  • Adds a new periodic celery task (every 10 minutes by default) that will process non dispatched notifications and send them via all available channels. In-app notifications don't depend on this task so they will pop up in Galaxy at their due time. Other external channels, like email, will probably carry some delay depending on the dispatch_notifications_interval configuration option.
  • Adds personalizable email templates for the currently existing types of notifications.

Frontend changes

  • The notification preferences will display a new Email channel if the server "supports" it. The channels supported by the server are passed to the client via a new header named supported-channels when requesting api/notifications/preferences.
  • Navigating to /user/notifications?preferences=true will display the preferences panel open. This makes it easier to link to the preferences from the email templates.

Sample Emails

Shared Item

Screenshot from 2024-04-11 13-46-29

Direct Message

Screenshot from 2024-04-11 13-49-10

See it in action

NotificationEmails

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    • Make sure enable_celery_tasks is set to true in your Galaxy config.
    • Set up the smtp_server in your config. I used https://github.com/rnwood/smtp4dev/
    • Play around with the email templates from lib/galaxy/config/templates/mail/notifications/ by copying them and setting the templates_dir accordingly.
    • Play around disabling the email channel for certain notifications in your preferences.
    • Play around scheduling notifications as an admin.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

lib/galaxy/model/__init__.py Outdated Show resolved Hide resolved
@davelopez davelopez force-pushed the add_email_notifications_channel branch from f9f8a62 to bb776fb Compare April 8, 2024 07:37
@davelopez
Copy link
Contributor Author

davelopez commented Apr 8, 2024

It seems the generated schema name when using generics changes between different versions of Python... https://github.com/galaxyproject/galaxy/actions/runs/8596319163/job/23553029236?pr=17914 🤔

diff --git a/client/src/api/schema/schema.ts b/client/src/api/schema/schema.ts
index 8786ebbb1e..b621015166 100644
--- a/client/src/api/schema/schema.ts
+++ b/client/src/api/schema/schema.ts
@@ -5394,8 +5394,8 @@ export interface components {
             /** Tags */
             tags?: string[] | null;
         };
-        /** GenericNotificationCreateRequest[Annotated[int, BeforeValidator, BeforeValidator, PlainSerializer, WithJsonSchema, WithJsonSchema]] */
-        GenericNotificationCreateRequest_Annotated_int__BeforeValidator__BeforeValidator__PlainSerializer__WithJsonSchema__WithJsonSchema__: {
+        /** GenericNotificationCreateRequest[Annotated[int, BeforeValidator(func=<function <lambda> at 0x7ae0525f30d0>), BeforeValidator(func=<function ensure_valid_id at 0x7ae0525e61f0>), PlainSerializer(func=<function <lambda> at 0x7ae0525f3040>, return_type=<class 'str'>, when_used='json'), WithJsonSchema(json_schema={'type': 'string', 'example': '0123456789ABCDEF', 'pattern': '[0-9a-fA-F]+', 'minLength': 16}, mode='serialization'), WithJsonSchema(json_schema={'type': 'string', 'example': '0123456789ABCDEF', 'pattern': '[0-9a-fA-F]+', 'minLength': 16}, mode='validation')]] */
+        "GenericNotificationCreateRequest_Annotated_int__BeforeValidator_func__function__lambda__at_0x7ae0525f30d0____BeforeValidator_func__function_ensure_valid_id_at_0x7ae0525e61f0____PlainSerializer_func__function__lambda__at_0x7ae0525f3040___return_type__class__str____when_used__json____WithJsonSchema_json_schema___type____example____pattern__0-9a-fA-F______minLength___mode__serialization____WithJsonSchema_json_schema___type____example____pattern__0-9a-fA-F______minLength___mode__validation____": {
             /**
              * Notification
              * @description The notification to create. The structure depends on the category.
@@ -5405,10 +5405,10 @@ export interface components {
              * Recipients
              * @description The recipients of the notification. Can be a combination of users, groups and roles.
              */
-            recipients: components["schemas"]["GenericNotificationRecipients_Annotated_int__BeforeValidator__BeforeValidator__PlainSerializer__WithJsonSchema__WithJsonSchema__"];
+            recipients: components["schemas"]["GenericNotificationRecipients_Annotated_int__BeforeValidator_func__function__lambda__at_0x7ae0525f30d0____BeforeValidator_func__function_ensure_valid_id_at_0x7ae0525e61f0____PlainSerializer_func__function__lambda__at_0x7ae0525f3040___return_type__class__str____when_used__json____WithJsonSchema_json_schema___type____example____pattern__0-9a-fA-F______minLength___mode__serialization____WithJsonSchema_json_schema___type____example____pattern__0-9a-fA-F______minLength___mode__validation____"];
         };
-        /** GenericNotificationRecipients[Annotated[int, BeforeValidator, BeforeValidator, PlainSerializer, WithJsonSchema, WithJsonSchema]] */
-        GenericNotificationRecipients_Annotated_int__BeforeValidator__BeforeValidator__PlainSerializer__WithJsonSchema__WithJsonSchema__: {
+        /** GenericNotificationRecipients[Annotated[int, BeforeValidator(func=<function <lambda> at 0x7ae0525f30d0>), BeforeValidator(func=<function ensure_valid_id at 0x7ae0525e61f0>), PlainSerializer(func=<function <lambda> at 0x7ae0525f3040>, return_type=<class 'str'>, when_used='json'), WithJsonSchema(json_schema={'type': 'string', 'example': '0123456789ABCDEF', 'pattern': '[0-9a-fA-F]+', 'minLength': 16}, mode='serialization'), WithJsonSchema(json_schema={'type': 'string', 'example': '0123456789ABCDEF', 'pattern': '[0-9a-fA-F]+', 'minLength': 16}, mode='validation')]] */
+        "GenericNotificationRecipients_Annotated_int__BeforeValidator_func__function__lambda__at_0x7ae0525f30d0____BeforeValidator_func__function_ensure_valid_id_at_0x7ae0525e61f0____PlainSerializer_func__function__lambda__at_0x7ae0525f3040___return_type__class__str____when_used__json____WithJsonSchema_json_schema___type____example____pattern__0-9a-fA-F______minLength___mode__serialization____WithJsonSchema_json_schema___type____example____pattern__0-9a-fA-F______minLength___mode__validation____": {
             /**
              * Group IDs
              * @description The list of encoded group IDs of the groups that should receive the notification.
@@ -9673,7 +9673,7 @@ export interface components {
             variant: components["schemas"]["NotificationVariant"];
         };
         /** NotificationCreateRequestBody */
-        NotificationCreateRequestBody: components["schemas"]["GenericNotificationCreateRequest_Annotated_int__BeforeValidator__BeforeValidator__PlainSerializer__WithJsonSchema__WithJsonSchema__"];
+        NotificationCreateRequestBody: components["schemas"]["GenericNotificationCreateRequest_Annotated_int__BeforeValidator_func__function__lambda__at_0x7ae0525
f30d0____BeforeValidator_func__function_ensure_valid_id_at_0x7ae0525e61f0____PlainSerializer_func__function__lambda__at_0x7ae0525f3040___return_type__class__str___
_when_used__json____WithJsonSchema_json_schema___type____example____pattern__0-9a-fA-F______minLength___mode__serialization____WithJsonSchema_json_schema___type___
_example____pattern__0-9a-fA-F______minLength___mode__validation____"];
         /** NotificationCreatedResponse */
         NotificationCreatedResponse: {
             /**

Update

I fixed it with a workaround to not use Generics directly in the exposed API models because they will get this extremely long auto-generated name that even changes between different versions of Python... it is ugly. Still, the other result was uglier I think 😅

@davelopez davelopez force-pushed the add_email_notifications_channel branch from 5af1dde to 4575d1c Compare April 8, 2024 16:41
@davelopez
Copy link
Contributor Author

I'm completely puzzled by this error in unit and package tests...

=========================== short test summary info ============================
ERROR lib/galaxy/schema/notifications.py - re.error: unbalanced parenthesis at position 434
!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!
=========== 25 deselected, 77 warnings, 1 error in 104.37s (0:01:44) ===========

I cannot see any unbalanced parenthesis in the whole file, let alone at position 434. In addition, this only fails consistently in Python 3.8 but not in Python 3.12 😵

am I missing something obvious?

@mvdbeek
Copy link
Member

mvdbeek commented Apr 9, 2024

Arg, that happened to me to at one point and was a big mystery. Unfortunately I didn't remember what the problem was :(

@mvdbeek
Copy link
Member

mvdbeek commented Apr 9, 2024

It might be an issue with how the generics are compiled ...

@mvdbeek
Copy link
Member

mvdbeek commented Apr 9, 2024

This is what I see locally:

^\s*class\s*NotificationVariant\b
^\s*class\s*MandatoryNotificationCategory\b
^\s*class\s*PersonalNotificationCategory\b
^\s*class\s*MessageNotificationContentBase\b
^\s*class\s*ActionLink\b
^\s*class\s*BroadcastNotificationContent\b
^\s*class\s*MessageNotificationContent\b
^\s*class\s*NewSharedItemNotificationContent\b
^\s*class\s*NotificationResponse\b
^\s*class\s*UserNotificationResponse\b
^\s*class\s*BroadcastNotificationResponse\b
^\s*class\s*UserNotificationListResponse\b
^\s*class\s*BroadcastNotificationListResponse\b
^\s*class\s*NotificationStatusSummary\b
^\s*class\s*NotificationCreateData\b
^\s*class\s*GenericNotificationRecipients\b
^\s*class\s*GenericNotificationCreateRequest\b
^\s*class\s*GenericNotificationCreateRequest[int]\b
^\s*class\s*GenericNotificationRecipients[int]\b
^\s*class\s*GenericNotificationRecipients[Annotated[int, BeforeValidator(func=<function <lambda> at 0x1056794c0>), BeforeValidator(func=<function ensure_valid_id at 0x105679d30>), PlainSerializer(func=<function <lambda> at 0x105679280>, return_type=<class 'str'>, when_used='json'), WithJsonSchema(json_schema={'type': 'string', 'example': '0123456789ABCDEF', 'pattern': '[0-9a-fA-F]+', 'minLength': 16}, mode='serialization'), WithJsonSchema(json_schema={'type': 'string', 'example': '0123456789ABCDEF', 'pattern': '[0-9a-fA-F]+', 'minLength': 16}, mode='validation')]]\b

@davelopez
Copy link
Contributor Author

It might be an issue with how the generics are compiled ...

Makes sense... It seems there are differences between Python versions around them as per the first issue I encountered with the client schema generation... 😞 I'll try to dig a bit more into it.

@mvdbeek
Copy link
Member

mvdbeek commented Apr 9, 2024

I think https://github.com/davelopez/galaxy/blob/4575d1c47845d5b0a23e664b8d2390794f4dda0b/lib/galaxy/schema/invocation.py#L98-L102 was the solution when I ran into it.

@davelopez
Copy link
Contributor Author

Thank you so much @mvdbeek! That was it!

I moved the logic to a GenericModel under a new generics module so it can be reused. I hope that is fine.

@davelopez davelopez force-pushed the add_email_notifications_channel branch 2 times, most recently from 2674499 to b72abfa Compare April 11, 2024 11:31
@davelopez davelopez marked this pull request as ready for review April 11, 2024 12:56
@github-actions github-actions bot added this to the 24.1 milestone Apr 11, 2024
@davelopez
Copy link
Contributor Author

I think this is finally ready for review so I updated the PR description.

I will follow up with some additional changes that don't quite fit here.

@davelopez davelopez force-pushed the add_email_notifications_channel branch from b72abfa to 45f4a7f Compare April 11, 2024 13:04
@davelopez davelopez added area/admin highlight/admin Included in admin/dev release notes labels Apr 11, 2024
davelopez and others added 24 commits April 16, 2024 17:06
Unfortunately I thought the default for columns was "non-nullable" so these columns are nullable in the database, but they will never be in the app.

This change matches the type expected in the app with the real nullable state in the database.
This will still create the notifications and associations in the database on request, but will defer sending emails (or any other channel) to a different function called `dispatch_pending_notifications_via_channels`.
Is not yet deprecated in Python 3.7
External notification channels (like email) are processed in Celery tasks. When Celery is not enabled in the server, we cannot show those channels to the user and we have to indicate in the API that those will be ignored.
Workaround issues with generics auto-generated names when building the client schema.
This is useful when we know in advance the list of recipient users and that no group or roles need to be resolved. Like when we share an item with a list of users.
This commit updates the notification creation and sharing logic to include the `galaxy_url` field in the `NotificationCreateRequest` and `SharedItemNotificationFactory` classes. This field is used to generate links in the notification content and is passed as an argument when sending notifications to recipients.
@davelopez davelopez force-pushed the add_email_notifications_channel branch from 2864b6c to 0a287f2 Compare April 16, 2024 15:35
@davelopez davelopez marked this pull request as ready for review April 16, 2024 15:48
Copy link
Member

@jdavcs jdavcs left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you!

@jdavcs jdavcs merged commit b60bbbd into galaxyproject:dev Apr 16, 2024
55 of 56 checks passed
@davelopez davelopez deleted the add_email_notifications_channel branch April 17, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support email notification channel
3 participants