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.

Sign up for GitHub

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

Jump to bottom

Add email sender feature #3414

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add email sender feature #3414

wants to merge 2 commits into from

Conversation

JobDoesburg
Copy link
Contributor

Summary

Adds a simple emailer feature, useful for certain committees to send styled emails (like the alumnicie @ricotewechel )

@JobDoesburg JobDoesburg requested a review from DeD1rk October 2, 2023 19:09
@DeD1rk
Copy link
Member

DeD1rk commented Oct 3, 2023

I'm a bit hesitant about merging something like this. It's simple enough like this of course but there are many things we would probably have to add to really be valuable:

  • In my opinion, sending anything from info@thalia.nu should be moderated, even if not to a moderated mailing list.
  • The txt version with striptags filter is probably rarely accurate (e.g. <br> won't become newlines, and any newlines in the HTML version don't take effect in an HTML email). Actually, even the HTML editor won't be close to what recipients wil see.
  • People will ask to be able to add attachments.
  • Some integration with mailinglists might be desirable.

I see most email clients make it basically impossible to customize the <html> and <body> tags, so just publishing the templates we use might not be a real solution either...

@JobDoesburg
Copy link
Contributor Author

Good points:

  1. We could send emails from noreply@ instead of info@. Or what would you think of sending on behalf of the mailing list of a group that the user belongs to?
  2. We're not doing attachments.
  3. My intention is that people don't write HTML per se. But that they could when they would really want to

@JobDoesburg
Copy link
Contributor Author

Oh and I deliberately didn't implement any integration with mailinglists because it creates another dependency, and I wouldn't really know what functionality it would offer.

@JobDoesburg
Copy link
Contributor Author

In the future we might be able to extend this with some mail-merger like functionality

@DeD1rk
Copy link
Member

DeD1rk commented Oct 3, 2023

sending on behalf of the mailing list of a group that the user belongs

That would be very nice I think. Then maybe this should go in the mailinglists app or a new app that depends on it. It's definitely not a 'core' part of thaliawebsite (nothing should depend on this) so then it shouldn't be in thaliawebsite/utils.

We're not doing attachments.

Sure, we can make that pretty explicit and be done with it.

My intention is that people don't write HTML per se. But that they could when they would really want to

Alright, that works. We might want to do some replacing of newlines to
s then

@JobDoesburg
Copy link
Contributor Author

@DeD1rk so a new app? Or mailinglists?

@DeD1rk
Copy link
Member

DeD1rk commented Oct 3, 2023

I guess then in mailinglists is fine. The permission can then just be on the MailingList model.

@JobDoesburg
Copy link
Contributor Author

Still in favor of the auxiliary model with functional permissions...
I could also make a new model that logs the sent emails, instead of BCC'ing all of them

@JobDoesburg
Copy link
Contributor Author

Btw, the thing is that the mailinglists app is very specific to Google Workspace, while this feature deliberately doesn't have any integrations like that, because I want it to be very simple...

@ricotewechel
Copy link

Yeey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DeD1rk DeD1rk Awaiting requested review from DeD1rk

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JobDoesburg @DeD1rk @ricotewechel