OTS Extension for Wagtail configuration of messages #4150

frankduncan · 2024-09-27T04:08:51Z

Like #4149 , this adds an extension to hypha. There's some overlap between the two PRs (some of the base setup). This one differs in that it uses django signals to facilitate communication with an extension.

This extension adds a wagtail configuration for email/slack messaging, overriding the developer required code. It includes simplistic find/replace for various variables depending on the object leading to the message.

Like the #4149, it includes tests and migrations that live apart from the codebase, and therefore should not cause migration conflicts. Also, it includes a number of commits that we need to be included (and tests for them) to declare the signal, even if the meat of the extension is not something you're looking for.

Those are: 9b5012d, c9fcaed

This creates a place to put extensions django applications so that base.py doesn't run into extra changes for people who want extensions. Extension based configuration will also go here.

This is a django signal that can override various parts of the messaging infrastructure (email, slack, etc). See updated documentation for information about the signal, and what it expects/provides.

This sets up the rest of changes coming and needed. Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

This doesn't build out any of the default messages (that comes later), but sets the stage for it. Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

This is not all of them, but a good representation for how to use the variables. We may want to build more out if the extension gets used more. For a large number of the messages, there's non trivial business logic in either the original messages or the generation of them that needs to be moved out or handled in another way. Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

frjo · 2024-09-30T07:04:32Z

We will take a look at this implementation.

Where mush like the idea of allowing easy customisation of messages.

We definitely want to make e-mail the default notification method for all users. The current Slack notifications for staff will remain as an option.

frankduncan · 2024-09-30T11:36:15Z

I think you may be confused by the terminology. "Default" in this extension dictates whether the notifications get sent by default. The idea is letting the administrator decide whether they want emails to be off unless otherwise specified, and whether they want slack messages to be off unless otherwise specified.

We ran into this because the client wanted us to have an allow list of what slack messages, and emails, to send. Rather than just blocking ones the don't. So the use case is if someone wants to only send an email message for determination, and only send a slack message for submission creation, they say "default off" for both slack and email messages, but then add an overriding "turn on for this message only" to both.

This prevents messages going out in the case of an upgrade in hypha adding new ones.

frjo · 2024-09-30T12:55:59Z

I was unclear, the default e-mail part refers to the work we plan to do, hopefully later this year, to have all staff notifications go via e-mail by default.

frankduncan · 2024-09-30T13:10:53Z

Ahh! After this PR gets reviewed/accepted, there will be another which connects to Zulip (dependent on this one), which uses the hypha slack infrastructure. I expect this extension well be helpful to keep that enabled after your changes!

bickelj · 2024-10-08T15:43:19Z

@frjo Like #4149 I think the first commits here are the extension capability and then the last commits are an example of an extension using that infrastructure.

frjo

Making messages configurable will be an excellent improvement of Hypha. This implementation however feels "plastered on".

If we in the future want to change messages we will need to update in several places from what I can see.

Can we not build this in to the activities app directly. Making this the way to handle e-mail notifications?

Are any of your clients using Slack? My guess is that only OTF uses it so we can skip Slack notifications for now.

frjo · 2024-10-09T10:57:06Z

extensions/ots/message_configuration/models.py

+
+
+@register_setting
+class MessagingSetting(BaseSiteSetting, ClusterableModel):


We are moving away from Wagtail sites so BaseGenericSetting is better to use.

See https://github.com/HyphaApp/hypha/blob/a027eb04090f0dd7986f4de10dd6f35efd648773/hypha/core/models/system_settings.py#L14-L18

Cool! I'll tackle this depending on how the overall PR discussion goes :)

frjo · 2024-10-09T10:59:30Z

extensions/ots/message_configuration/static/js/message_configuration_wagtail_admin.js

+        });
+        add_update_events();
+    });
+})(jQuery);


VanillaJS or AlpineJS, we are retiring jQuery.

Not as familiar with those, so probably won't have a chance to do that.

frjo · 2024-10-09T11:22:33Z

extensions/ots/message_configuration/templates/messaging_help.html

+
+<p>{% trans "When the submission is available, the following are available:" %}</p>
+
+<ul>


Add class w-mb-4 to add some margins to the lists.

Good call! I'll make the change depending on how the overarching PR discussion goes :)

frjo · 2024-10-09T11:23:24Z

extensions/ots/message_configuration/templates/messaging_help.html

+  {% trans "Variables that will get replaced follow.  Use the uppercase name (before the colon) in any subject, email message, or slack message." %}
+</p>
+
+<p>{% trans "When the submission is available, the following are available:" %}</p>


Maybe wrap these in <strong>? Makes it easer to see the sections of the text.

I don't think we have a lot of opinions on the styling of the help text, so whatever you guys think looks good.

frjo · 2024-10-09T11:24:15Z

extensions/ots/message_configuration/templates/messaging_help.html

+</ul>
+
+{% for id, default in self.email_messages.items %}
+  <div class="is-hidden" attr-email-id="{{ id }}">


Can we just use "hidden" attribute instead of adding a class and a custom css file?

frjo · 2024-10-09T11:30:02Z

extensions/ots/message_configuration/models.py

+    )
+    email_subject = models.TextField(null=True, blank=True)
+    email_message = models.TextField(null=True, blank=True)
+    slack_message = models.TextField(null=True, blank=True)


The two messages fields should be textareas I believe.

I can see that!

frankduncan · 2024-10-25T15:57:36Z

Making messages configurable will be an excellent improvement of Hypha. This implementation however feels "plastered on".

Yes! The architecture of the messaging was a bit hard to puncture. Our first attempt was to add it directly into the activities app. However, there was a lot of business logic, both in the templates themselves, as well as in the actual calls (for instance, https://github.com/HyphaApp/hypha/blob/main/hypha/apply/activity/adapters/base.py#L56-L63 adds a lot of complexity)

We were able to get as far as our client needed with the first pass of configuration in the activities app, as a lot of the logic in the base hypha activities app was able to be bypassed (they really just wanted far more simplistic messaging around a small subset of messages). However, building out something that could reasonably reproduce what hypha is currently doing with configuration, turned out to be a much bigger project than we had budget for. If you guys can do that, that would be awesome! In the meantime, we settled for saying "hey, add a hook into the base messaging app, and let us do a reasonable job of using that hook for our clients"

If we in the future want to change messages we will need to update in several places from what I can see.

Yes and no. As long as you make the base tests pass from the activities app, then you can safely ignore the extension. We just need the contract maintained of turning email on and off in the default case, as well as being able to replace the message generated by the activities app with one of our own.

Can we not build this in to the activities app directly. Making this the way to handle e-mail notifications?

See above! It looked like a pretty massive project to take what's currently in hypha to that method. If you guys can come up with a good way, that would be even better than our extension. One thing we did decide we needed for our client was no logic in the configuration. Simple string replacement only.

Are any of your clients using Slack? My guess is that only OTF uses it so we can skip Slack notifications for now.

Yes! We actually subclassed the slack adapter to add zulip (coming to the hypha codebase after we work through this PR). They use it for general channel things (not targeted messages) for general events, like a submission happening. This lets people in their organization stay aware of general stuff in hypha without having to subscribe to events, or even have accounts on the hypha instance.

frankduncan · 2024-10-25T16:05:25Z

Overall, a potential successful merge here is to say "we don't want the extension, but we will accept the changes to the base hypha app that enable your extension." That way, we can move forward on our end and let you guys put together a project that is more in line with how you'd want this feature to work in mainline hypha.

If that's what you end up doing, we can create another PR with just the changes we need to enable our extension to work in isolation :)

frankduncan added 9 commits September 26, 2024 21:35

Add top level infrastructure for new extensions work

9b5012d

This creates a place to put extensions django applications so that base.py doesn't run into extra changes for people who want extensions. Extension based configuration will also go here.

Add and use a message_hook

c9fcaed

This is a django signal that can override various parts of the messaging infrastructure (email, slack, etc). See updated documentation for information about the signal, and what it expects/provides.

Add Messaging Settings objects and database

862f933

This sets up the rest of changes coming and needed. Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

Add information panel, as well as ability to add default email messages

d366b88

This doesn't build out any of the default messages (that comes later), but sets the stage for it. Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

Pull settings and use with email/slack signals

9a4abe3

Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

Add replacement strings to settings based messages

5291678

Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

Add documentation in the settings for the message replacement strings

4cd9764

Issue ots/clients/ardc/hypha-tracker#218: Permit outgoing messages/notifications to be configured in Wagtail

Add tests for message_configuration extension

8abb879

frankduncan changed the title ~~Extension ots wagtail email configuration~~ OTS Extension for Wagtail configuration of messages Sep 27, 2024

frjo requested changes Oct 9, 2024

View reviewed changes

frjo closed this Nov 8, 2024

frankduncan mentioned this pull request Nov 8, 2024

Extension ots newsletter signup #4149

Closed



		@register_setting
		class MessagingSetting(BaseSiteSetting, ClusterableModel):


		<p>{% trans "When the submission is available, the following are available:" %}</p>

		<ul>

OTS Extension for Wagtail configuration of messages #4150

OTS Extension for Wagtail configuration of messages #4150

Uh oh!

Conversation

frankduncan commented Sep 27, 2024

Uh oh!

frjo commented Sep 30, 2024

Uh oh!

frankduncan commented Sep 30, 2024

Uh oh!

frjo commented Sep 30, 2024

Uh oh!

frankduncan commented Sep 30, 2024

Uh oh!

bickelj commented Oct 8, 2024

Uh oh!

frjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankduncan commented Oct 25, 2024

Uh oh!

frankduncan commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants