feature: mail provider backend #45383

SebastianKrupinski · 2024-05-16T23:21:20Z

Resolves: Calendar invites: Administrator shall choose if server's or user's email address is used #5080

Summary

Initial Mail Provider Implementation

TODO

Copyright
Comments
Minor code improvements

lib/private/AppFramework/Bootstrap/RegistrationContext.php

lib/private/Mail/Provider/Manager.php

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

lib/private/Mail/Provider/Address.php

lib/private/Mail/Provider/Attachment.php

lib/private/Mail/Provider/Message.php

lib/public/Mail/Provider/IAttachment.php

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

+					(($recipientName !== null) ? [$recipient => $recipientName] : [$recipient])
+				);
+				$message->setReplyTo(
+					(($senderName !== null) ? [$sender => $senderName] : [$sender])


apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

+					(($recipientName !== null) ? [$recipient => $recipientName] : [$recipient])
+				);
+				$message->setReplyTo(
+					(($senderName !== null) ? [$sender => $senderName] : [$sender])


github-advanced-security

Psalm found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

SebastianKrupinski · 2024-05-24T18:23:12Z

TODO: Separate attachment handling for local files (saved on the system) and streamed files (generate in memory)

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

lib/private/Mail/Provider/Manager.php

lib/public/Mail/Provider/Message.php

lib/public/Mail/Provider/IService.php

lib/public/Mail/Provider/Message.php

lib/private/Mail/Provider/Manager.php

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

lib/private/Mail/Provider/Manager.php

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

lib/private/Mail/Provider/Manager.php

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

apps/theming/lib/ThemingDefaults.php

-		if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $color)) {
-			return BackgroundService::DEFAULT_COLOR;
+		// try admin color
+		$defaultColor = $this->config->getAppValue(Application::APP_ID, 'primary_color', '');


apps/theming/lib/ThemingDefaults.php

 		$variables['image-login-plain'] = 'false';

-		if ($this->config->getAppValue('theming', 'color', '') !== '') {
+		if ($this->config->getAppValue('theming', 'primary_color', '') !== '') {


apps/theming/lib/Service/BackgroundService.php

 	public function setDefaultBackground(): void {
 		$this->config->deleteUserValue($this->userId, Application::APP_ID, 'background_image');
 		$this->config->deleteUserValue($this->userId, Application::APP_ID, 'background_color');
+		$this->config->deleteUserValue($this->userId, Application::APP_ID, 'primary_color');


apps/theming/lib/ThemingDefaults.php

+	 * Default background color only taking admin setting into account
+	 */
+	public function getDefaultColorBackground(): string {
+		$defaultColor = $this->config->getAppValue(Application::APP_ID, 'background_color', '');


apps/theming/lib/ImageManager.php

 			case 'background':
-				return $this->urlGenerator->linkTo(Application::APP_ID, 'img/background/' . BackgroundService::DEFAULT_BACKGROUND_IMAGE);
+				// Removing the background defines its mime as 'backgroundColor'
+				$mimeSetting = $this->config->getAppValue('theming', 'backgroundMime', '');


apps/theming/lib/Settings/Personal.php

+			/** URL of admin configured background image */
+			'backgroundImage' => $this->themingDefaults->getBackground(),
+			/** `backgroundColor` if disabled, mime type if defined and empty by default */
+			'backgroundMime' => $this->config->getAppValue('theming', 'backgroundMime', ''),


apps/theming/lib/ThemingDefaults.php

+				// If a background image is set we revert to the mean image color
+				if ($this->imageManager->hasImage('background')) {
+					$file = $this->imageManager->getImage('background');
+					$returnValue = $this->backgroundService->setGlobalBackground($file->read()) ?? '';


apps/theming/lib/Service/BackgroundService.php

+		if ($handle && $image->loadFromFileHandle($handle) !== false) {
+			$meanColor = $this->calculateMeanColor($image);
+			if ($meanColor !== false) {
+				$this->config->setAppValue(Application::APP_ID, 'background_color', $meanColor);


apps/theming/lib/ImageManager.php

 					imagedestroy($outputImage);
-				}
+				} catch (\Exception $e) {
+					if (isset($outputImage) && is_resource($outputImage) || $outputImage instanceof \GdImage) {


SebastianKrupinski · 2024-05-28T13:19:00Z

TODO: unit tests

github-actions · 2024-05-31T01:47:04Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

lib/private/Mail/Provider/Manager.php

ChristophWurst

Looks sane

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

lib/private/Mail/Provider/Manager.php

lib/public/Mail/Provider/IMessageSend.php

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php

tests/lib/Mail/Provider/ManagerTest.php

lib/public/Mail/Provider/Message.php

kesselb · 2024-07-15T10:39:34Z

IAddress.getAddress
IAddress.getLabel
IAttachment.getName
IAttachment.getType
IAttachment.getContents
IMessage.getFrom
IMessage.getReplyTo
IMessage.getTo
IMessage.getCC
IMessage.getBcc
IMessage.getSubject
IMessage.getBody
IMessage.getBodyHtml
IMessage.getBodyPlain
IMessage.getAttachments

Each of the above methods can return null.
This should be reworked.

For example, IAttachment:

What's the use case for having an attachment object with a name and a type but content's null, and what is the consumer supposed to do with such an object?

<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>TypeError</s:exception>
  <s:message>OCA\Mail\Service\Attachment\AttachmentService::addFileFromString(): Argument #4 ($fileContents) must be of type string, null given, called in /var/www/html/apps-extra/mail/lib/Provider/Command/MessageSend.php on line 47</s:message>
</d:error>

The requirements for the public api should be as strict as possible and can be extended when necessary. We don't have a use case for having a "null" attachment and therefore should not implement it. The same for every other method from the list above.

kesselb · 2024-07-15T10:42:43Z

IProvider.createService
IProvider.modifyService
IProvider.deleteService

Are unused and the implemention in https://github.com/nextcloud/mail/pull/9651/files#diff-863bf0fae4943be717ec0db9eba3ff621d28665362e192e0b6d45a2b917426f7R190-R236 does nothing and therefore should be removed.

st3iny

The API looks solid to me and I like the design overall.

However, I agree with kesselb's latest points and have some additional minor high level feedback:

Please get rid of IServiceIdentity and IServiceLocation interfaces. We can always add new interfaces once those two features are needed and ready. Currently, they seem to be unused.
All methods inside IMessage that return an array or null should only return an (empty) array. This makes consuming the API more convenient as both cases don't need to be handled separately despite being semantically identical. (As kesselb mentioned, there are some more methods inside IAttachment where this also applies.)

SebastianKrupinski · 2024-07-17T15:13:48Z

@st3iny @kesselb

I've changed the empty return type on the functions that return an array to an empty array, you have a valid point about the easier consumption of the array.

I have also removed the extra IServiceIdentity and IServiceLocation interfaces and methods, even tho I have a feeling I might be adding them back in next month from a conversion I had this week, but I save the code so that should not be a big deal.

miaulalala

Aside from the comments by Daniel and Richard this looks fine.

st3iny

Feedback has been addressed.

kesselb

#45383 (comment)

kesselb · 2024-07-23T17:29:43Z

I'm aware that Beta 1 is due on 2024-07-25, but we can always backport changes if necessary.

Most methods still accept null values for no reason. I would prefer to sort that out before merging. It's easier to make an API less strict later, than the other way around.
https://github.com/nextcloud/server/blob/cf432cf1c31908a45adde486ff4353a37254e8ed/lib/public/Mail/Provider/Message.php Please use class properties rather then $this->data.

kesselb · 2024-07-23T19:25:06Z

Sebastian and I discussed the nullable return types and agreed to keep the nullable return types.
We've learned that there's a need for api consumers to signal back that an entity is unprocessable and therefore will add an exception to ocp.

Sorted out

Signed-off-by: SebastianKrupinski <[email protected]>

ChristophWurst · 2024-07-24T14:56:17Z

@SebastianKrupinski this PR adds new APIs to lib/public aka OCP, so we need developer docs. You can take https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/groupware/calendar.html as inspiration.

susnux · 2024-08-23T16:15:45Z

Documentation was added: nextcloud/documentation#12083

SebastianKrupinski added enhancement 2. developing Work in progress feature: emails labels May 16, 2024

SebastianKrupinski requested review from ChristophWurst, miaulalala and tcitworld as code owners May 16, 2024 23:21

SebastianKrupinski marked this pull request as draft May 16, 2024 23:21

github-advanced-security bot found potential problems May 16, 2024

View reviewed changes

github-advanced-security bot found potential problems May 22, 2024

View reviewed changes

github-advanced-security bot found potential problems May 23, 2024

View reviewed changes

SebastianKrupinski mentioned this pull request May 24, 2024

feature: mail provider backend nextcloud/mail#9651

Merged

github-advanced-security bot found potential problems May 25, 2024

View reviewed changes

lib/public/Mail/Provider/Message.php Fixed Show fixed Hide fixed

lib/public/Mail/Provider/Message.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 25, 2024

View reviewed changes

lib/private/Mail/Provider/Manager.php Fixed Show fixed Hide fixed

lib/private/Mail/Provider/Manager.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 27, 2024

View reviewed changes

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 27, 2024

View reviewed changes

lib/private/Mail/Provider/Manager.php Fixed Show fixed Hide fixed

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems May 27, 2024

View reviewed changes

SebastianKrupinski force-pushed the feature/request-803 branch from b1b1fe5 to 2187c7c Compare May 27, 2024 03:56

github-actions bot added the feedback-requested label May 31, 2024

SebastianKrupinski marked this pull request as ready for review June 4, 2024 17:21

github-advanced-security bot found potential problems Jun 5, 2024

View reviewed changes

lib/private/Mail/Provider/Manager.php Fixed Show fixed Hide fixed

SebastianKrupinski force-pushed the feature/request-803 branch from 2ee01d0 to 43d6666 Compare June 7, 2024 15:34

solracsf added this to the Nextcloud 30 milestone Jun 18, 2024

ChristophWurst reviewed Jun 26, 2024

View reviewed changes

SebastianKrupinski force-pushed the feature/request-803 branch from 43d6666 to 81763fb Compare June 26, 2024 16:39

github-advanced-security bot found potential problems Jun 26, 2024

View reviewed changes

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed

apps/dav/lib/CalDAV/Schedule/IMipPlugin.php Fixed Show fixed Hide fixed

SebastianKrupinski force-pushed the feature/request-803 branch from ff4903b to e7d0dfc Compare July 13, 2024 00:45

kesselb reviewed Jul 15, 2024

View reviewed changes

st3iny reviewed Jul 15, 2024

View reviewed changes

SebastianKrupinski force-pushed the feature/request-803 branch 2 times, most recently from c3d0b64 to f4c1801 Compare July 17, 2024 14:54

SebastianKrupinski requested review from ChristophWurst, kesselb and st3iny July 17, 2024 15:14

miaulalala approved these changes Jul 23, 2024

View reviewed changes

st3iny approved these changes Jul 23, 2024

View reviewed changes

SebastianKrupinski force-pushed the feature/request-803 branch 2 times, most recently from 91f2c96 to cf432cf Compare July 23, 2024 16:27

kesselb previously requested changes Jul 23, 2024

View reviewed changes

SebastianKrupinski force-pushed the feature/request-803 branch from cf432cf to 791149f Compare July 23, 2024 20:10

feat: mail provider backend

fc0b694

Signed-off-by: SebastianKrupinski <[email protected]>

SebastianKrupinski force-pushed the feature/request-803 branch from 791149f to fc0b694 Compare July 23, 2024 20:20

SebastianKrupinski merged commit 644d490 into master Jul 23, 2024

SebastianKrupinski deleted the feature/request-803 branch July 23, 2024 20:46

ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jul 24, 2024

susnux removed the pending documentation This pull request needs an associated documentation update label Aug 23, 2024

ChristophWurst mentioned this pull request Nov 20, 2024

Appointments: eMail Sender/'From' Field is misleading - should use 'Reply to' nextcloud/calendar#6495

Open

tcitworld mentioned this pull request Dec 17, 2025

feat(sharebymail): Use initiator's email as sender if mail providers enabled #57080

Open

7 tasks

Uh oh!

feature: mail provider backend #45383

feature: mail provider backend #45383

Uh oh!

Conversation

SebastianKrupinski commented May 16, 2024

Summary

TODO

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Check notice

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski commented May 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

Check notice

SebastianKrupinski commented May 28, 2024

Uh oh!

github-actions bot commented May 31, 2024

Uh oh!

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

st3iny left a comment •

edited

Loading

kesselb commented Jul 23, 2024 •

edited

Loading

kesselb commented Jul 23, 2024 •

edited

Loading