-
Notifications
You must be signed in to change notification settings - Fork 7
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
Added automatic birthday e-mails #371
base: main
Are you sure you want to change the base?
Conversation
I have implemented the Birthday mails, the only thing that has to be done to fully integrate it is to make the template correct.
Implemented a folding card and also changed the sendEmailTemplate such that you can say you do not want to send the GEWIS template.
Jort changed it such that it looks good right now
…ldje/gewisdb into feature/birthday-mails
d5e8a64
to
140912b
Compare
Psalm error is expected and will be resolved once on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The e-mail looks nice, however, there are several issues with the delivery (see screenshots below). Also make sure that it is properly documented somewhere that a new board should be reasonably quick with updating the template if they do not want these e-mails to contain the old template after July 1.
I do not see a cronjob, so at this point the e-mails will have to be send manually. Only the ABC can do this. There should be a cronjob that does this once per day, I would recommend at like 10:00 each day. Or perhaps earlier so people see the card when they wake up (so for example at 07:00).
I have actually sent the e-mail to myself on various platforms. I would argue that the e-mail in its current format is not suitable. See the results:
public function getCurrentBirthdays(): array | ||
{ | ||
$qb = $this->getRepository()->createQueryBuilder('m'); | ||
$qb->select('m') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we already noticed, this does mean that people who are born on the 29th of February only get a birthday card once very 4 years.
We discussed several ways to fix this, I think the best way to fix this is to group these people in non-leap years with the 28th of February (otherwise the "card" is a day late which is not nice).
I suggest to use the following:
(MONTH(m.birth) = MONTH(CURRENT_DATE()) AND DAYOFMONTH(m.birth) = DAYOFMONTH(CURRENT_DATE())) OR
(MONTH(m.birth) = 2 AND DAYOFMONTH(m.birth) = 29 AND MONTH(CURRENT_DATE()) = 2 AND DAYOFMONTH(CURRENT_DATE()) = 28 AND YEAR(CURRENT_DATE()) % 4 != 0)
Note that this requires the introduction of YEAR
similar to MONTH
. Furthermore, this takes a shortcut by only accounting for divisibility by 4 and not 100 or 400. However, the next year where this would be problematic (i.e. two mails will be send) is 2100, I hope that this code is no longer used at that point (if GEWIS still exists).
Reply so I get updated about changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can pick 3 of the following 4 things:
- deliverability (spam filters)
- email client compatibility
- aesthetically pleasing
- effective time investment
Getting the first three right, will almost certainly cost a lot of time.
My proposal is that you make the content in the email less interactive and include a hyperlink to a webpage (static HTML with some JavaScript).
If needed, you can use URL parameters (using /#/
since it doesn't need to be sent to the server) to make it personal.
For testing on mobile phones I propose you use these: K9, Outlook for Android, Gmail for Android, iOS mail
For testing on desktops I propose: Outlook, Thunderbird, Outlook Web Access
I have implemented the birthday mails. Now it should send a very nice birthday card. Future feature is adding the borrelkaart to it.