Skip to content

iMIP email improvements (take 2) #17456

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

Merged
skjnldsv merged 3 commits into from
Sep 4, 2020
Merged

iMIP email improvements (take 2) #17456

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
781359a
iMIP email improvements (take 2)
brad2014 Oct 8, 2019
b0687b1
Make icon set for CalDav event presentation, use in iMIP email.
brad2014 Oct 10, 2019
442af8c
Minor cleanup: php-cs-fixer, tests, interface consistency
Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
iMIP email improvements (take 2) 
This PR is a replacement for PR #17195. It is intended to be simpler
to review and approve, with fewer changes, some disabled by default.

It addresses issues #12391 and #13555, with the following changes:

- The plainText of iMIP emails has been upgraded as described in
issue #12391. The HTML design style has not been changed.

- Some of the HTML and plainText content has been rearranged
(simplified header language, moving the event title to from text
body to the first item in the bullet list, spelling corrections,
moving the description to the end of the list), per issue #12391.

- The interface for EMailTemplate has been extended: addBodyListItem
now takes an optional `plainIndent` parameter. Existing callers
see no change. Where new calls set the  new parameter >0, the list
item label (metaInfo) is put in column 1, and the value is indented
into column 2 (properly accounting for multiple lines, if any).

- An optional dav config setting has been added,
`invitation_list_attendees`. It defaults to 'no', leaving emails
unchanged. If set by the site admin to 'yes', then iMIP emails
include, for the organizer and each attendee, their name, email,
and a ✔︎ if they have accepted the invitation.

- Minor refactoring.

Notes:

- The labels for organizers and attendees list items are new, and
require translation/localization.

- Dav config settings are documented in the code, but not in the
Administrator's Guide.

Signed-off-by: brad2014 <[email protected]>
  • Loading branch information
@brad2014
brad2014 committed Aug 20, 2020
commit 781359a5827f8a6fa477cbc23b0c8ef5e94bac23
178 changes: 127 additions & 51 deletions apps/dav/lib/CalDAV/Schedule/IMipPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ class IMipPlugin extends SabreIMipPlugin {
public const METHOD_REQUEST = 'request';
public const METHOD_REPLY = 'reply';
public const METHOD_CANCEL = 'cancel';
public const IMIP_INDENT = 15; // Enough for the length of all body bullet items, in all languages

/**
* @param IConfig $config
Expand Down Expand Up @@ -204,26 +205,6 @@ public function schedule(Message $iTipMessage) {
$meetingTitle = $vevent->SUMMARY;
$meetingDescription = $vevent->DESCRIPTION;

$start = $vevent->DTSTART;
if (isset($vevent->DTEND)) {
$end = $vevent->DTEND;
} elseif (isset($vevent->DURATION)) {
$isFloating = $vevent->DTSTART->isFloating();
$end = clone $vevent->DTSTART;
$endDateTime = $end->getDateTime();
$endDateTime = $endDateTime->add(DateTimeParser::parse($vevent->DURATION->getValue()));
$end->setDateTime($endDateTime, $isFloating);
} elseif (!$vevent->DTSTART->hasTime()) {
$isFloating = $vevent->DTSTART->isFloating();
$end = clone $vevent->DTSTART;
$endDateTime = $end->getDateTime();
$endDateTime = $endDateTime->modify('+1 day');
$end->setDateTime($endDateTime, $isFloating);
} else {
$end = clone $vevent->DTSTART;
}

$meetingWhen = $this->generateWhenString($l10n, $start, $end);
Comment on lines -207 to -226
Copy link
Contributor Author

@brad2014 brad2014 Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into the generateWhenString function, where it belongs.


$meetingUrl = $vevent->URL;
$meetingLocation = $vevent->LOCATION;
Expand Down Expand Up @@ -263,8 +244,7 @@ public function schedule(Message $iTipMessage) {

$this->addSubjectAndHeading($template, $l10n, $method, $summary,
$meetingAttendeeName, $meetingInviteeName);
$this->addBulletList($template, $l10n, $meetingWhen, $meetingLocation,
$meetingDescription, $meetingUrl);
$this->addBulletList($template, $l10n, $vevent);


// Only add response buttons to invitation requests: Fix Issue #11230
Expand Down Expand Up @@ -370,7 +350,6 @@ private function getLastOccurrence(VCalendar $vObject) {
return $lastOccurrence;
}


/**
* @param Message $iTipMessage
* @return null|Property
Expand Down Expand Up @@ -420,10 +399,29 @@ private function getAttendeeRSVP(Property $attendee = null) {

/**
* @param IL10N $l10n
* @param Property $dtstart
* @param Property $dtend
* @param VEvent $vevent
*/
private function generateWhenString(IL10N $l10n, Property $dtstart, Property $dtend) {
private function generateWhenString(IL10N $l10n, VEvent $vevent) {

$dtstart = $vevent->DTSTART;
if (isset($vevent->DTEND)) {
$dtend = $vevent->DTEND;
} elseif (isset($vevent->DURATION)) {
$isFloating = $vevent->DTSTART->isFloating();
$dtend = clone $vevent->DTSTART;
$endDateTime = $end->getDateTime();
$endDateTime = $endDateTime->add(DateTimeParser::parse($vevent->DURATION->getValue()));
$dtend->setDateTime($endDateTime, $isFloating);
} elseif (!$vevent->DTSTART->hasTime()) {
$isFloating = $vevent->DTSTART->isFloating();
$dtend = clone $vevent->DTSTART;
$endDateTime = $end->getDateTime();
$endDateTime = $endDateTime->modify('+1 day');
$dtend->setDateTime($endDateTime, $isFloating);
} else {
$dtend = clone $vevent->DTSTART;
}

$isAllDay = $dtstart instanceof Property\ICalendar\Date;

/** @var Property\ICalendar\Date | Property\ICalendar\DateTime $dtstart */
Expand Down Expand Up @@ -507,49 +505,127 @@ private function isDayEqual(\DateTime $dtStart, \DateTime $dtEnd) {
* @param IL10N $l10n
* @param string $method
* @param string $summary
* @param string $attendeeName
* @param string $inviteeName
*/
private function addSubjectAndHeading(IEMailTemplate $template, IL10N $l10n,
$method, $summary, $attendeeName, $inviteeName) {
$method, $summary) {
if ($method === self::METHOD_CANCEL) {
$template->setSubject('Cancelled: ' . $summary);
$template->addHeading($l10n->t('Invitation canceled'), $l10n->t('Hello %s,', [$attendeeName]));
$template->addBodyText($l10n->t('The meeting »%1$s« with %2$s was canceled.', [$summary, $inviteeName]));
$template->setSubject('Canceled: ' . $summary);
$template->addHeading($l10n->t('Invitation canceled'));
Comment on lines -516 to +510
Copy link
Contributor Author

@brad2014 brad2014 Oct 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made spellings consistent. "Canceled" is en_us. "Cancelled" is en_gb. Got rid of the non-en_us »« by moving the title to the front of the list (instead of a proceeding bodyText).

Other notes:

Also "Hello %s" is error-prone and disconcerting on an auto-generated email (the attendeeName is not necessarily appropriate for a salutation, and it may be missing altogether). I hope you agree the result is better without it.

Also, in the cancelation case, it isn't right to say "The meeting ${subject} with ${inviteeName} was canceled," because:

  • The mislabeled inviteeName is actually organizerName (I fixed that)
  • The organizer/invitor of a meeting (perhaps an admin) is not necessarily who the meeting is with.
  • A cancellation message does not indicate that the meeting was canceled, it means that the invitation was withdrawn (the meeting may still be happening, you're just no longer invited!).

Copy link
Member

@georgehrke georgehrke Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

} elseif ($method === self::METHOD_REPLY) {
$template->setSubject('Re: ' . $summary);
$template->addHeading($l10n->t('Invitation updated'), $l10n->t('Hello %s,', [$attendeeName]));
$template->addBodyText($l10n->t('The meeting »%1$s« with %2$s was updated.', [$summary, $inviteeName]));
$template->addHeading($l10n->t('Invitation updated'));
} else {
$template->setSubject('Invitation: ' . $summary);
$template->addHeading($l10n->t('%1$s invited you to »%2$s«', [$inviteeName, $summary]), $l10n->t('Hello %s,', [$attendeeName]));
$template->addHeading($l10n->t('Invitation'));
}
}

/**
* @param IEMailTemplate $template
* @param IL10N $l10n
* @param string $time
* @param string $location
* @param string $description
* @param string $url
* @param VEVENT $vevent
*/
private function addBulletList(IEMailTemplate $template, IL10N $l10n, $time, $location, $description, $url) {
$template->addBodyListItem($time, $l10n->t('When:'),
$this->getAbsoluteImagePath('filetypes/text-calendar.svg'));
private function addBulletList(IEMailTemplate $template, IL10N $l10n, $vevent) {

if ($location) {
$template->addBodyListItem($location, $l10n->t('Where:'),
$this->getAbsoluteImagePath('filetypes/location.svg'));
if ($vevent->SUMMARY) {
$template->addBodyListItem($vevent->SUMMARY, $l10n->t('Title:'),
$this->getAbsoluteImagePath('filetypes/text.svg'),'','',self::IMIP_INDENT);
}
if ($description) {
$template->addBodyListItem((string)$description, $l10n->t('Description:'),
$this->getAbsoluteImagePath('filetypes/text.svg'));
$meetingWhen = $this->generateWhenString($l10n, $vevent);
if ($meetingWhen) {
$template->addBodyListItem($meetingWhen, $l10n->t('Time:'),
$this->getAbsoluteImagePath('filetypes/text-calendar.svg'),'','',self::IMIP_INDENT);
}
if ($url) {
$template->addBodyListItem((string)$url, $l10n->t('Link:'),
$this->getAbsoluteImagePath('filetypes/link.svg'));
if ($vevent->LOCATION) {
$template->addBodyListItem($vevent->LOCATION, $l10n->t('Location:'),
$this->getAbsoluteImagePath('filetypes/location.svg'),'','',self::IMIP_INDENT);
}
if ($vevent->URL) {
$template->addBodyListItem(sprintf('<a href="%s">%s</a>',
htmlspecialchars($vevent->URL),
Copy link
Member

@georgehrke georgehrke Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickvergessen Does this look sane?

Copy link
Member

@nickvergessen nickvergessen Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

htmlspecialchars($vevent->URL)),
$l10n->t('Link:'),
$this->getAbsoluteImagePath('filetypes/link.svg'),
$vevent->URL,'',self::IMIP_INDENT);
}

$this->addAttendees($template, $l10n, $vevent);

/* Put description last, like an email body, since it can be arbitrarily long */
if ($vevent->DESCRIPTION) {
$template->addBodyListItem($vevent->DESCRIPTION, $l10n->t('Description:'),
$this->getAbsoluteImagePath('filetypes/text.svg'),'','',self::IMIP_INDENT);
}
}

/**
* addAttendees: add organizer and attendee names/emails to iMip mail.
*
* Enable with DAV setting: invitation_list_attendees (default: no)
*
* The default is 'no', which matches old behavior, and is privacy preserving.
*
* To enable including attendees in invitation emails:
* % php occ config:app:set dav invitation_list_attendees --value yes
*
* @param IEMailTemplate $template
* @param IL10N $l10n
* @param Message $iTipMessage
* @param int $lastOccurrence
* @author brad2014 on github.com
*/

private function addAttendees(IEMailTemplate $template, IL10N $l10n, VEvent $vevent) {
if ($this->config->getAppValue('dav', 'invitation_list_attendees', 'no') === 'no') {
Copy link
Contributor Author

@brad2014 brad2014 Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new function is a no-op until the admin enables it (it is disabled by default, to match current e-mails, and because there may be policy reasons why iMip mails should not disclose so prominently the names and email addresses of all invited attendees (though there is nothing in the text that is not also in the ics file).

It adds to the list (before the description) the name/email/accept-status of the organizer and each attendee.

TODO: Really, the checkmark for "accepted" is lovely, but too limited. There should be a mark for (a) not replied yet, (b) accepted, (c) declined.

return;
}

if (isset($vevent->ORGANIZER)) {
$organizer = $vevent->ORGANIZER;
$organizerURI = $organizer->getNormalizedValue();
list($scheme,$organizerEmail) = explode(':',$organizerURI,2); # strip off scheme mailto:
$organizerName = isset($organizer['CN']) ? $organizer['CN'] : null;
$organizerHTML = sprintf('<a href="%s">%s</a>',
htmlspecialchars($organizerURI),
htmlspecialchars($organizerName ?: $organizerEmail));
$organizerText = sprintf('%s <%s>', $organizerName, $organizerEmail);
if (isset($organizer['PARTSTAT'])
&& strcasecmp($organizer['PARTSTAT'], 'ACCEPTED') === 0) {
$organizerHTML .= ' ✔︎';
$organizerText .= ' ✔︎';
}
$template->addBodyListItem($organizerHTML, $l10n->t('Organizer:'),
$this->getAbsoluteImagePath('filetypes/text-vcard.svg'),
$organizerText,'',self::IMIP_INDENT);
}

$attendees = $vevent->select('ATTENDEE');
if (count($attendees) === 0) {
return;
}

$attendeesHTML = [];
$attendeesText = [];
foreach ($attendees as $attendee) {
$attendeeURI = $attendee->getNormalizedValue();
list($scheme,$attendeeEmail) = explode(':',$attendeeURI,2); # strip off scheme mailto:
$attendeeName = isset($attendee['CN']) ? $attendee['CN'] : null;
$attendeeHTML = sprintf('<a href="%s">%s</a>',
htmlspecialchars($attendeeURI),
htmlspecialchars($attendeeName ?: $attendeeEmail));
$attendeeText = sprintf('%s <%s>', $attendeeName, $attendeeEmail);
if (isset($attendee['PARTSTAT'])
&& strcasecmp($attendee['PARTSTAT'], 'ACCEPTED') === 0) {
$attendeeHTML .= ' ✔︎';
$attendeeText .= ' ✔︎';
}
array_push($attendeesHTML, $attendeeHTML);
array_push($attendeesText, $attendeeText);
}

$template->addBodyListItem(implode('<br/>',$attendeesHTML), $l10n->t('Attendees:'),
$this->getAbsoluteImagePath('filetypes/text-vcard.svg'),
implode("\n",$attendeesText),'',self::IMIP_INDENT);
}

/**
Expand Down
30 changes: 24 additions & 6 deletions lib/private/Mail/EMailTemplate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,14 +450,16 @@ public function addBodyText(string $text, $plainText = '') {
* if empty the $text is used, if false none will be used
* @param string|bool $plainMetaInfo Meta info that is used in the plain text email
* if empty the $metaInfo is used, if false none will be used
* @param integer plainIndent If > 0, Indent plainText by this amount.
* @since 12.0.0
*/
public function addBodyListItem(string $text, string $metaInfo = '', string $icon = '', $plainText = '', $plainMetaInfo = '') {
public function addBodyListItem(string $text, string $metaInfo = '', string $icon = '', $plainText = '', $plainMetaInfo = '', $plainIndent = 0) {
$this->ensureBodyListOpened();
Copy link
Contributor Author

@brad2014 brad2014 Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a (backward-compatible) interface change to the EMailTemplate interface.

If the plainIndent is not specified by the caller, this function is unchanged. As of this commit, iMipPlugin.php is the only caller that uses it. Other similar emails (event notifications, for example), might use it in the future.


if ($plainText === '') {
$plainText = $text;
$text = htmlspecialchars($text);
$text = str_replace("\n", "<br/>", $text); // convert newlines to HTML breaks
Copy link
Contributor Author

@brad2014 brad2014 Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes an oops. Newlines in plaintext are line-breaks in HTML.

}
if ($plainMetaInfo === '') {
$plainMetaInfo = $metaInfo;
Expand All @@ -475,11 +477,27 @@ public function addBodyListItem(string $text, string $metaInfo = '', string $ico
}
$this->htmlBody .= vsprintf($this->listItem, [$icon, $htmlText]);
if ($plainText !== false) {
$this->plainBody .= ' * ' . $plainText;
if ($plainMetaInfo !== false) {
$this->plainBody .= ' (' . $plainMetaInfo . ')';
if ($plainIndent === 0) {
/*
* If plainIndent is not set by caller, this is the old NC17 layout code.
*/
$this->plainBody .= ' * ' . $plainText;
if ($plainMetaInfo !== false) {
$this->plainBody .= ' (' . $plainMetaInfo . ')';
}
$this->plainBody .= PHP_EOL;
} else {
/*
* Caller can set plainIndent > 0 to format plainText in tabular fashion.
* with plainMetaInfo in column 1, and plainText in column 2.
* The plainMetaInfo label is right justified in a field of width
* "plainIndent". Multilines after the first are indented plainIndent+1
* (to account for space after label). Fixes: #12391
*/
$this->plainBody .= sprintf("%${plainIndent}s %s\n",
$plainMetaInfo,
str_replace("\n", "\n" . str_repeat(' ', $plainIndent+1), $plainText));
}
$this->plainBody .= PHP_EOL;
}
}

Expand Down Expand Up @@ -538,7 +556,7 @@ public function addBodyButtonGroup(string $textLeft,
$textColor = $this->themingDefaults->getTextColorPrimary();

$this->htmlBody .= vsprintf($this->buttonGroup, [$color, $color, $urlLeft, $color, $textColor, $textColor, $textLeft, $urlRight, $textRight]);
$this->plainBody .= $plainTextLeft . ': ' . $urlLeft . PHP_EOL;
$this->plainBody .= PHP_EOL . $plainTextLeft . ': ' . $urlLeft . PHP_EOL;
Copy link
Contributor Author

@brad2014 brad2014 Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change mimics, for the plainText version of the e-mail, the fact that the buttons have space above them, separating them from the List. It looks better.

$this->plainBody .= $plainTextRight . ': ' . $urlRight . PHP_EOL . PHP_EOL;
}

Expand Down