Mail Feature: Scheduled Sending of Mails #10243
Conversation
| string $a_m_subject, | ||
| string $a_m_message, | ||
| int $a_draft_id = 0, | ||
| ?ilDateTime $a_send_time = null, |
There was a problem hiding this comment.
Please use a DateTimeImmutable here.
Since the "Schedule Date/Time" is a future date/time, we MUST NOT store this a UTC.
We MUST use the user timezone and the corresponding date/time string. Storing this as UTC can lead to issues like described here: https://andreas.heigl.org/2016/12/22/why-not-to-convert-a-datetime-to-timestamp/ / https://heiglandreas.github.io/slidedeck/time_is_an_illusion/20191113_aachen/index.html#/
So I recommend introducing two new explicit database fields in a database update objective.
| ?string $a_tpl_context_id = null, | ||
| array $a_tpl_context_params = [] | ||
| ): int { | ||
| $a_send_time?->switchTimeZone('UTC'); |
There was a problem hiding this comment.
See my comment above: Storing a Sent Time (past) is okay, but a "Schedule Date/Time" (future) should be stored as a date/time string with the user's timezone in a separate field.
Addendum: We should NOT even "normalize" the timezone for the "Send Date". I suggested this years a go as a pull request, an while discussing this on the JourFixe, we stumbled upon the issue of how to tread the date/time values of all existing records in the database table? Now one knows the respective user timezones of these records.
| if ($use_placeholders) { | ||
| $message = $this->replacePlaceholders($message, $sender_usr_id); | ||
| } | ||
| $a_send_time?->switchTimeZone('UTC'); |
There was a problem hiding this comment.
See comment above.
There was a problem hiding this comment.
Hi @fhelfer,
thx for the PR. I left some inline comments.
Furthermore:
- We need a setup migration, which creates an "Outbox" folder in
mail_obj_dataandmail_treefor every existing user. - Design decision: A scheduled "Mail" MUST NOT be scheduled for a past point in time
- Please keep the "Form"-related changes minimal, since PR #9423 will introduce a KS/UI form.
- A scheduled "Mail" (not sent, yet) record can still be edited. If the user initiates this process, the mail is automatically moved to the "Drafts" folder (immediately).
Best regards,
Michael
| 'Error sending terminated mail with id ' . ($mail['mail_id'] ?? 'unknown') . ': ' . $e->getMessage() | ||
| ); | ||
| $job_result->setMessage($e->getMessage()); | ||
| return $job_result; |
There was a problem hiding this comment.
| return $job_result; | |
| return $job_result; |
| return $job_result; | ||
| } | ||
|
|
||
| public function getOutboxMails(): Generator |
There was a problem hiding this comment.
It would be good if we could describe the shape of the array and use this type in the Genertor.
...
/**
* @phpstan-type MailWithFolderDatabaseRecordType array{
* obj_id: int,
* }
*/
class ilMail
{
...AI could really help to define such record types.
You could then use \Generator<MailWithFolderDatabaseRecordType> and import this type into consuming classes:
...
/**
* @phpstan-import-type MailWithFolderDatabaseRecordType from \ilMail
*/
class MailConsumer
{
...| { | ||
| $res = $this->db->queryF( | ||
| 'SELECT obj_id FROM ' . $this->table_mail_obj_data . ' WHERE user_id = %s AND m_type = %s', | ||
| ['integer', 'text'], |
There was a problem hiding this comment.
Please use ilDBConstants types here.
| AND send_time IS NOT NULL | ||
| AND send_time <= %s | ||
| SQL, | ||
| ['text', 'timestamp'], |
There was a problem hiding this comment.
Please use ilDBConstants types here.
| private GlobalHttpState $http; | ||
| private ilLanguage $lng; | ||
| private ilSetting $settings; | ||
| private ilDBInterface $db; | ||
| private ilObjUser $user; | ||
| private bool $init_done = false; | ||
| private JobManager $cron_manager; | ||
| private ilMail $mail; | ||
| private ilFormatMail $umail; |
There was a problem hiding this comment.
Could we make them readonly, or the complete class??
| 'd_drafts' => 'drafts', | ||
| 'e_sent' => 'sent', | ||
| 'z_local' => 'local', | ||
| 'o_outbox' => 'outbox', |
There was a problem hiding this comment.
Please use e_outbox and create a database update objective (NOT a migration) for the "Mail" component for ILIAS 11, which modifies all e_sent entries and renames them to f_sent.
2 participants
https://docu.ilias.de/go/wiki/wpage_8629_1357