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

iMIP email improvements (take 2) #17456

Merged
merged 3 commits into from
Sep 4, 2020
Merged

iMIP email improvements (take 2) #17456

Show file tree
Hide file tree
Changes from all commits
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
186 changes: 132 additions & 54 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 @@ -3,7 +3,6 @@
* @copyright Copyright (c) 2016, ownCloud, Inc.
* @copyright Copyright (c) 2017, Georg Ehrke
*
* @author brad2014 <brad2014@users.noreply.github.com>
* @author Brad Rubenstein <brad@wbr.tech>
* @author Christoph Wurst <christoph@winzerhof-wurst.at>
* @author Georg Ehrke <oc.list@georgehrke.com>
Expand Down Expand Up @@ -108,6 +107,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 +204,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

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 @@ -261,10 +241,8 @@ public function schedule(Message $iTipMessage) {

$summary = ((string) $summary !== '') ? (string) $summary : $l10n->t('Untitled event');

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


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


/**
* @param Message $iTipMessage
* @return null|Property
Expand Down Expand Up @@ -420,10 +397,28 @@ 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 = $dtend->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 = $dtend->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 +502,132 @@ 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

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 VEVENT $vevent
*/
private function addBulletList(IEMailTemplate $template, IL10N $l10n, $vevent) {
if ($vevent->SUMMARY) {
$template->addBodyListItem($vevent->SUMMARY, $l10n->t('Title:'),
$this->getAbsoluteImagePath('caldav/title.svg'),'','',self::IMIP_INDENT);
}
$meetingWhen = $this->generateWhenString($l10n, $vevent);
if ($meetingWhen) {
$template->addBodyListItem($meetingWhen, $l10n->t('Time:'),
$this->getAbsoluteImagePath('caldav/time.svg'),'','',self::IMIP_INDENT);
}
if ($vevent->LOCATION) {
$template->addBodyListItem($vevent->LOCATION, $l10n->t('Location:'),
$this->getAbsoluteImagePath('caldav/location.svg'),'','',self::IMIP_INDENT);
}
if ($vevent->URL) {
$url = $vevent->URL->getValue();
$template->addBodyListItem(sprintf('<a href="%s">%s</a>',
htmlspecialchars($url),
htmlspecialchars($url)),
$l10n->t('Link:'),
$this->getAbsoluteImagePath('caldav/link.svg'),
$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->getValue(), $l10n->t('Description:'),
$this->getAbsoluteImagePath('caldav/description.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 string $time
* @param string $location
* @param string $description
* @param string $url
* @param Message $iTipMessage
* @param int $lastOccurrence
* @author brad2014 on github.com
*/
private function addBulletList(IEMailTemplate $template, IL10N $l10n, $time, $location, $description, $url) {
$template->addBodyListItem($time, $l10n->t('When:'),
$this->getAbsoluteImagePath('filetypes/text-calendar.svg'));

if ($location) {
$template->addBodyListItem($location, $l10n->t('Where:'),
$this->getAbsoluteImagePath('filetypes/location.svg'));
private function addAttendees(IEMailTemplate $template, IL10N $l10n, VEvent $vevent) {
if ($this->config->getAppValue('dav', 'invitation_list_attendees', 'no') === 'no') {
Copy link
Contributor Author

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 ($description) {
$template->addBodyListItem((string)$description, $l10n->t('Description:'),
$this->getAbsoluteImagePath('filetypes/text.svg'));

if (isset($vevent->ORGANIZER)) {
/** @var Property\ICalendar\CalAddress $organizer */
$organizer = $vevent->ORGANIZER;
$organizerURI = $organizer->getNormalizedValue();
list($scheme,$organizerEmail) = explode(':',$organizerURI,2); # strip off scheme mailto:
/** @var string|null $organizerName */
$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'])) {
/** @var Parameter $partstat */
$partstat = $organizer['PARTSTAT'];
if (strcasecmp($partstat->getValue(), 'ACCEPTED') === 0) {
$organizerHTML .= ' ✔︎';
$organizerText .= ' ✔︎';
}
}
$template->addBodyListItem($organizerHTML, $l10n->t('Organizer:'),
$this->getAbsoluteImagePath('caldav/organizer.svg'),
$organizerText,'',self::IMIP_INDENT);
}
if ($url) {
$template->addBodyListItem((string)$url, $l10n->t('Link:'),
$this->getAbsoluteImagePath('filetypes/link.svg'));

$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('caldav/attendees.svg'),
implode("\n",$attendeesText),'',self::IMIP_INDENT);
}

/**
Expand Down
12 changes: 7 additions & 5 deletions apps/dav/tests/unit/CalDAV/Schedule/IMipPluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ protected function setUp(): void {

public function testDelivery() {
$this->config
->expects($this->at(1))
->method('getAppValue')
->with('dav', 'invitation_link_recipients', 'yes')
->willReturn('yes');
Expand All @@ -148,6 +149,7 @@ public function testDelivery() {

public function testFailedDelivery() {
$this->config
->expects($this->at(1))
->method('getAppValue')
->with('dav', 'invitation_link_recipients', 'yes')
->willReturn('yes');
Expand All @@ -163,6 +165,7 @@ public function testFailedDelivery() {

public function testDeliveryWithNoCommonName() {
$this->config
->expects($this->at(1))
->method('getAppValue')
->with('dav', 'invitation_link_recipients', 'yes')
->willReturn('yes');
Expand All @@ -188,9 +191,8 @@ public function testDeliveryWithNoCommonName() {
*/
public function testNoMessageSendForPastEvents(array $veventParams, bool $expectsMail) {
$this->config
->method('getAppValue')
->with('dav', 'invitation_link_recipients', 'yes')
->willReturn('yes');
->method('getAppValue')
->willReturn('yes');

$message = $this->_testMessage($veventParams);

Expand Down Expand Up @@ -228,6 +230,7 @@ public function testIncludeResponseButtons(string $config_setting, string $recip

$this->_expectSend($recipient, true, $has_buttons);
$this->config
->expects($this->at(1))
->method('getAppValue')
->with('dav', 'invitation_link_recipients', 'yes')
->willReturn($config_setting);
Expand All @@ -252,14 +255,13 @@ public function dataIncludeResponseButtons() {
public function testMessageSendWhenEventWithoutName() {
$this->config
->method('getAppValue')
->with('dav', 'invitation_link_recipients', 'yes')
->willReturn('yes');

$message = $this->_testMessage(['SUMMARY' => '']);
$this->_expectSend('frodo@hobb.it', true, true,'Invitation: Untitled event');
$this->emailTemplate->expects($this->once())
->method('addHeading')
->with('Mr. Wizard invited you to »Untitled event«');
->with('Invitation');
$this->plugin->schedule($message);
$this->assertEquals('1.1', $message->getScheduleStatus());
}
Expand Down
2 changes: 2 additions & 0 deletions apps/settings/tests/Mailer/NewUserMailHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,7 @@ public function testGenerateTemplateWithoutPasswordResetToken() {

Your username is: john


Go to TestCloud: https://example.com/
Install Client: https://nextcloud.com/install/#install-clients

Expand Down Expand Up @@ -817,6 +818,7 @@ public function testGenerateTemplateWithoutUserId() {

Welcome to your TestCloud account, you can add, protect, and share your data.


Go to TestCloud: https://example.com/
Install Client: https://nextcloud.com/install/#install-clients

Expand Down
1 change: 1 addition & 0 deletions core/img/caldav/attendees.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions core/img/caldav/description.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions core/img/caldav/link.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions core/img/caldav/location.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions core/img/caldav/organizer.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions core/img/caldav/time.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions core/img/caldav/title.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading