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

Added automatic birthday e-mails #371

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SvenMokveldje
Copy link
Contributor

I have implemented the birthday mails. Now it should send a very nice birthday card. Future feature is adding the borrelkaart to it.

SvenMokveldje and others added 9 commits December 20, 2023 20:03
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
@tomudding tomudding changed the title Feature/birthday mails Added automatic birthday e-mails Feb 14, 2024
@tomudding tomudding self-requested a review February 14, 2024 14:31
@tomudding
Copy link
Member

Psalm error is expected and will be resolved once on main.

Copy link
Member

@tomudding tomudding left a 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:

K9 (Android):
Screenshot_2024-02-14-17-02-31-018_com fsck k9
Screenshot_2024-02-14-17-02-23-138_com fsck k9

GMail (Android):
Screenshot_2024-02-14-17-04-23-206_com google android gm

GMail (Web):
image

SOGo:
image

Thunderbird:
image

.idea/codeception.xml Show resolved Hide resolved
.idea/phpspec.xml Show resolved Hide resolved
public function getCurrentBirthdays(): array
{
$qb = $this->getRepository()->createQueryBuilder('m');
$qb->select('m')
Copy link
Member

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).

@tomudding tomudding requested a review from rinkp February 14, 2024 16:09
@rinkp
Copy link
Member

rinkp commented Feb 16, 2024

Reply so I get updated about changes

Copy link
Member

@rinkp rinkp left a 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

@tomudding tomudding marked this pull request as draft August 18, 2024 15:16
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.

3 participants