-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
iMIP email improvements (take 2) #17456
iMIP email improvements (take 2) #17456
Conversation
@skjnldsv and @nextcloud/designers |
@@ -95,6 +95,7 @@ class IMipPlugin extends SabreIMipPlugin { | |||
const METHOD_REQUEST = 'request'; | |||
const METHOD_REPLY = 'reply'; | |||
const METHOD_CANCEL = 'cancel'; | |||
const IMIP_INDENT = 15; // Enough for the length of all body bullet items, in all languages |
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 changes in IMipPlugin.php activate the new plainText formatting provided (below) EmailTemplate::addBodyListItem. Basically, the change to plainText invitations lines up all the invitation details to a common indent:
Old format:
Hello ${attendeeName},
* xxxxxxxx (When:)
* xxxxxxxxxxxx (Where:)
* xxxxxxxxxxx
xxxxxxxxx (Description:)
* xxxxxxxxxxxx (Link:)
New layout:
Invitation
Title: xxxxxxxx
When: xxxxxxxxxxxxx
Location: xxxxxxxxxxxx
Link: xxxxxxxxxxxx
Description: xxxxxxxxxxx
xxxxxxxxx
Other notes:
Calculating a reasonable indent dynamically (tough, especially in the face of localization and utf8) is TODO. In the mean time, this fixed indent degrades gracefully if, in some language, a label is too long (it just means the value will be misaligned, but still readable). Note that this doesn't handle right-to-left languages at all (another TODO).
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.
What if the label is too wide? What is done currently? Ellipsised?
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.
What if the label is too wide? What is done currently? Ellipsised?
No, it would just disturb the alignment:
Titel: xxxxxxxx
Zeit: xxxxxxxxxxxxx
Straßenbahnhaltestelle: xxxxxxxxxxxx
Link: xxxxxxxxxxxx
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.
Can we ellipsis instead? :)
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.
Can we ellipsis instead? :)
I'd advise against it. Given the choice between readability and alignment, I'd choose readability. It is quite hard, in general, across languages, to determine the printed length of a unicode plainText string (you can't simply count up the bytes), so programmatically cutting off a label without context would reduce readability without even necessarily getting the alignment fixed.
A better solution would be to advise the translator that these labels sit in a limited width, and to pick a translation that is worded, abbreviated, or elided in a way that makes it fit (Practically speaking, I suspect this will almost never be an issue).
Titel: xxxxxxxx
Zeit: xxxxxxxxxxxxx
SBHaltestelle: xxxxxxxxxxxx
Link: xxxxxxxxxxxx
$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); |
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.
Moved into the generateWhenString function, where it belongs.
$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')); |
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.
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!).
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.
👍
*/ | ||
|
||
private function addAttendees(IEMailTemplate $template, IL10N $l10n, VEvent $vevent) { | ||
if ($this->config->getAppValue('dav', 'invitation_list_attendees', 'no') === 'no') { |
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.
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.
* @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(); |
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.
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.
$this->ensureBodyListOpened(); | ||
|
||
if ($plainText === '') { | ||
$plainText = $text; | ||
$text = htmlspecialchars($text); | ||
$text = str_replace("\n", "<br/>", $text); // convert newlines to HTML breaks |
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.
This fixes an oops. Newlines in plaintext are line-breaks in HTML.
@@ -542,7 +560,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; |
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.
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.
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.
Looks really great!
On the first html screenshot, the organizer is a "contacts" icon, can we have multiple organizers in an event?
If not it should be a single-user icon :)
- There is no administrative UI to change the "include attendees" setting. Only via occ.
This is not acceptable, please create a config.php key or a config toggle in the mail settings! ;)
@georgehrke this raises a question to me, why do we have this mails in server and not in calendar?
|
It can only be one. Ideally we would show the user avatar, but an icon is a good start for now. :)
What @tcitworld said. |
Ah right, I forgot! :) |
;) I took @georgehrke 's suggestion from my last PR (#12392, 8d8bcea) that I use an app config setting rather than config.php, so the preference would be tied to the database, rather than to the server host. |
I limited myself to the svg's that are currently shipped in core/img/filetypes, which is where the icons were pulled prior. The two icons that are "not quite right" are the Organizer (a single person) and the Title (which uses the text.svg icon, same as "Description"). It seems whoever chose the original design was being opportunistic/efficient in reusing the icons available for filetypes, and I didn't want to disturb that. But if there is a currently shipping 16x16 svg icon representing a single user, let me know and I'll put it in. These icons are, semantically, bullets, very small (16x16) and marking the separate paragraphs, so it wouldn't work to use the organizer's avatar photo without changing the design of the email (which I tried last time in PR #17195, removing the icons entirely, and that did not prove too popular). For this PR, I'm trying to keep it super simple, on the assumption that a more competent person later will do a proper design refresh of the email layout. |
yes there is :) The |
Notes on 5dfbe56: @skjnldsv, I took your suggestion and used the user.svg logo, though I had to recolor it. @georgehrke, the icons used in iMip email were loaded from core/img/filetypes, as a hack (a caldav entry is not a filetype). This commit creates the folder core/img/caldav, and it contains all the images for elements of an event. These are used in iMIP email, but eventually should probably be used in any UI that refers to caldav elements (e.g. the calendar app). The images currently in the new folder are copied from the filetype images, except for "title" and "organizer" which were taken from core/img/actions and recolored to fit the existing email theme. |
Cool stuff on improving that email! :) Some design feedback, cause the main content currently has lots of different elements so that everything gets lost a bit:
What do you think? Might have more feedback after those changes are applied, but that’s it for now. Really nice that you work on improving this @brad2014! |
Good feedback, @jancborchardt, thanks! I wonder if, in name of making things "no worse", we can do these in a second PR (along with other suggestions). My main goal for this PR is that it fix clear existing breakage (ie. the plaintext is just broken, multi-line descriptions are just broken), be easy to review (i.e. minimal) and approvable, and not touch things that are currently bearable. I'd be happy to enhance more UI enhancements into a second PR, both the ones you mention and the new ones you come up with. (As a newbie to the NC community, I need help understanding how UI fixes are approved and merged - how do I separate "suggestions" from "requirements"?) Admittedly, the addition of Organizer/Attendee violates that minimalist idea, but it is clean addition that is by default off (invisible), so won't break anything by being included (and I had the code in hand). It can be enabled by default in the future if folks like it. I'll note a few caveats (some of which I learned from my previous failed attempt):
This is how it was before, and it looked poor, especially when the title was long enough to line wrap - long multiline titles containing quoted sentences are hard to read (but multi-line title in the list now formats well, I think). In my experience people often just type the description into the title field and leave the description blank (I suspect event creation dialog boxes encourage this). Also, I do think there is some value in keeping the structure of the email parallel to the structure of the event create/edit dialog in the calendar app (same order, same labels, etc) so that, when the email is not what you want, it is clear where to go in the event editor to fix it.
I'd be a bit nervous about making this label different from the label in the calendar app, and making it different from common apps like google calendar, outlook, thunderbird which all label it
The organizer is not necessarily an attendee, it is spec'd (by RFC) as the point of contact. In many contexts the organizer is the person collecting the rsvp's, and managing the attendees list. The ical RFC has separate support for marking participation ROLE, e.g. of an attendee as "Chair" (the leader of a meeting), and that could be called out in the UI, but I think it's overkill (e.g. the calendar app UI won't let you set the participation role of each attendee).
I'd be happy to do this in the follow-up PR. It looks like @nickvergessen put the italics in, is it ok to take them out?
Maybe the smile means you're kidding, but just in case... If I put this in a PR, I'd want confirmation it is really what y'all want - changing the order of buttons like these after people have gotten used them is sometimes a usability disaster - it breaks everyone's muscle memory. It would also be different from every RSVP dialog I've ever seen. |
Ping @jancborchardt, please check the answer above :) |
I'm still interested in making progress on this, if you are, @skjnldsv and @jancborchardt. Was my reply helpful? |
@brad2014 yes, sorry for the late reply! :) Sounds good to do enhancements in a follow-up pull request. (And yes, removing italics is fine – as said we do not use them anywhere as they are quite bad for readability.) |
iMIP e-mails were, as a hack, using filetypes icons for caldav elements (titles, locations). This commit creates a folder of caldav element icons. To start, they are used in iMIP emails, but eventually should be used by any app that wants to have icon labels for caldav elements. Signed-off-by: brad2014 <brad2014@users.noreply.github.com>
Done. There were no conflicts. |
Hold off briefly on merge. The static-code-analysis points to some possible errors that concern me. Checking... |
if ($plainText === '' || $plainText === true) { | ||
$plainText = $text; | ||
$text = htmlspecialchars($text); | ||
$text = str_replace("\n", "<br/>", $text); // convert newlines to HTML breaks |
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 code handled the case where plainText and plainMetaInfo were strings or false booleans, but left the true bool case undefined. This made the static-code-checker unhappy (which it should). I simply handled the true case as if it were an empty string, and updated the preceding @ var comments.
Commit 6a44945 gives clean Psalm output. It fixes one bug (which would occur if a VEVENT had no DTEND, legal but rare), and the rest is type documenting. I'm going to do some more testing tomorrow, to make sure I didn't break anything else. I'd appreciate a second pair of 👀 if you have a moment. UPDATE: testing completed. I'm satisfied. |
How do I set up my development environment so I'm running my rebased branch (off master), and also have the calendar app, for testing? When i try to install the calendar app via "occ app:install" I get:
I tried following the instructions to clone and build building the nextcloud/calendar repo ( |
@brad2014 Edit |
Thank you, @tcitworld! |
@brad2014 The PHP Codestyle check is failing. Can you please check. You can just use |
@nickvergessen @ChristophWurst @skjnldsv Can we please get a second review here? :) |
For the google: occ app:enable has a -f flag to override version checking :-)
Maybe I'll put in an issue to support the -f flag in app:install :-) |
@georgehrke I get this error. Is the 3rdparty repo missing nextcloud\codingstandard? Any pointers?
|
Did you do a |
I'll give it a go. (Sorry if this is the wrong place for these newbie questions - I've never used composer before.) |
Done. |
} elseif (isset($vevent->DURATION)) { | ||
$isFloating = $vevent->DTSTART->isFloating(); | ||
$dtend = clone $vevent->DTSTART; | ||
$endDateTime = $end->getDateTime(); | ||
$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 = $end->getDateTime(); | ||
$endDateTime = $dtend->getDateTime(); |
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.
This was actually a bug, but would only happen if an event had no DTEND (legal per RFC, but rare in the field).
CI is not happy... |
[UPDATE] ha ha. It turns out |
IMipPlugin.php Removed blank lines to make php-cs-fixer happy. Minor cleanup: bugs found by Psalm static checker IEMailTemplate: The public interface to addBodyListItem also needs to include the new plainIndent parameter. IMipPlugin: Fixes an undefined variable for events that do not have DTEND. Also use explicit string conversion for parameters and properties in several places. The new email template adds an additional blank line before "button" links in plain text, so the tests were fixed to include that additional blank line. Signed-off-by: Brad Rubenstein <brad@wbr.tech>
CI tests for this PR now pass. |
Whoop whoop! 🚀 |
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.
fix #12391
It addresses issues #12391 and #13555, with the following changes:
The plainText of iMIP emails has been upgraded as described in
issue Minor improvements to iMip mail message text #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 Minor improvements to iMip mail message text #12391.
The interface for EMailTemplate has been extended: addBodyListItem
now takes an optional
plainIndent
parameter. Existing callerssee 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 emailsunchanged. 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.
There is no administrative UI to change the "include attendees" setting. Only via occ.
Signed-off-by: Brad Rubenstein brad@wbr.tech