feature: mail provider backend #9651

SebastianKrupinski · 2024-05-16T23:14:34Z

Initial Mail Provider Implementation

nextcloud/server#5080

SebastianKrupinski · 2024-05-16T23:16:34Z

Todo:

Copyright
Comments
Minor code improvements

SebastianKrupinski · 2024-05-24T18:17:11Z

This PR requires the server PR first...

nextcloud/server#45383

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

TODO: Separate attachment handling for file system files (saved on the system) and string data files (created or loaded in memory)

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

TODO: unit tests

github-actions · 2024-06-02T02:11:20Z

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!

SebastianKrupinski · 2024-07-09T15:14:45Z

Testing process:

Pull this PR and server PR - nextcloud/server#45383

Make sure you have an email account setup in the mail app.

Then create calendar event with an attendee.

Invitation email should be sent from personal mail account

lib/Provider/Command/MessageSend.php

lib/Provider/MailService.php

tests/Unit/Provider/MailProviderTest.php

tests/Unit/Provider/MailServiceTest.php

lib/Db/MailAccountMapper.php

lib/AppInfo/Application.php

miaulalala

Please remove all @since comments. Also, comments that only describe what the variable name / return type already documents can be removed, too.

lib/Db/MailAccountMapper.php

lib/Provider/Command/MessageSend.php

lib/Provider/MailProvider.php

lib/Provider/MailService.php

lib/Service/AccountService.php

miaulalala · 2024-07-15T12:24:19Z

lib/Service/AccountService.php

+	 *
+	 * @throws ClientException
+	 */
+	public function findByUserIdAndAddress(string $userId, string $address): array {


Suggested change

public function findByUserIdAndAddress(string $userId, string $address): array {

public function findByUserIdAndAddress(string $userId, string $email): array {

kesselb · 2024-09-03T20:47:16Z

lib/Provider/Command/MessageSend.php

+	 *
+	 * @param array<int,IAddress> $addresses collection of IAddress objects
+	 *
+	 * @return array<int,array{email: string, label?: string}>


Suggested change

* @return array<int,array{email: string, label?: string}>

* @return array<int,array{email: ?string, label?: ?string}>

This didn't work, caused more Psalm errors.

ERROR: InvalidArgument - lib/Provider/Command/MessageSend.php:109:5 - Argument 3 of OCA\Mail\Service\OutboxService::saveMessage expects array<int, array<array-key, string>>, but array<int, array{email: null|string, label?: string}> provided (see https://psalm.dev/004) $to, ERROR: InvalidArgument - lib/Provider/Command/MessageSend.php:110:5 - Argument 4 of OCA\Mail\Service\OutboxService::saveMessage expects array<int, array<array-key, string>>, but array<int, array{email: null|string, label?: string}> provided (see https://psalm.dev/004) $cc, ERROR: InvalidArgument - lib/Provider/Command/MessageSend.php:111:5 - Argument 5 of OCA\Mail\Service\OutboxService::saveMessage expects array<int, array<array-key, string>>, but array<int, array{email: null|string, label?: string}> provided (see https://psalm.dev/004) $bcc,

#10114

/** * Converts IAddress objects collection to plain array * * @since 4.0.0 * * @param array<int,IAddress> $addresses collection of IAddress objects * * @return array<int,array{email: ?string, label?: string}> */ protected function convertAddressArray(array $addresses): array { return array_map(static function (IAddress $address) { $label = $address->getLabel(); return !empty($label) ? ['email' => $address->getAddress(), 'label' => $label] : ['email' => $address->getAddress()]; }, $addresses); }

Please update the function as above.

The annotated return type for label is always a string.

It's required to introduce a local variable for label, otherwise psalm is unable to detect.

kesselb · 2024-09-03T20:54:37Z

lib/Provider/Command/MessageSend.php

+			}
+		}
+		// determine if required To address is set
+		if (empty($message->getTo()) || empty($message->getTo()[0]->getAddress())) {


Easiest way to fix the psalm warning about possibly null: Introduce a local variable for to.

A bit nicer: Move the validation to a function.

/** * @param IAddress[] $addresses * @throws SendException */ private function validateTo(array $addresses): void { if (count($addresses) === 0 || empty($addresses[0]->getAddress())) { throw new SendException('Invalid Message Parameter: MUST contain at least one TO address with a valid address'); } }

ChristophWurst

These seem to be the important changes remaining

lib/Provider/Command/MessageSend.php

ChristophWurst · 2024-09-04T07:40:27Z

lib/Provider/MailProvider.php

+	public function listServices(string $userId): array {
+		// retrieve service(s) details from data store
+		try {
+			$accounts = $this->accountService->findByUserId($userId);


findByUserId does not throw anything

Yes, findByUserId it self does not throw an error. But wouldn't it be better to catch any and all errors and log them? That way the mail manager does not have to handle any/all the different errors that might occur with the different providers.

There is no reason for a catch-all right now.

lib/Provider/MailProvider.php

ChristophWurst · 2024-09-04T07:44:03Z

lib/Provider/Command/MessageSend.php

+						$entry->getType(),
+						$entry->getContents()
+					)->jsonSerialize();
+				} catch (\Throwable $e) {


Suggested change

} catch (\Throwable $e) {

} catch (UploadException $e) {

Wouldn't it be better to catch any and all errors and present them as one type? That way the consuming app/service does not have to handle any/all the different errors that might occur. For instance, the addFileFromString function currently does not thrown a proper UploadException for any database errors, so if the filename is too long or the mime type is too long, this would cause a different error.

ChristophWurst · 2024-09-04T07:44:24Z

tests/Unit/Provider/Command/MessageSendTest.php

+				$this->mailAttachment->getType(),
+				$this->mailAttachment->getContents()
+			)->will(
+				$this->throwException(new \Exception('Something went wrong'))


Suggested change

$this->throwException(new \Exception('Something went wrong'))

$this->throwException(new UploadException('Something went wrong'))

ChristophWurst · 2024-09-04T07:50:44Z

lib/Service/AccountService.php

+			return array_map(static function ($a) {
+				return new Account($a);
+			}, $this->mapper->findByUserIdAndAddress($userId, $address));
+		} catch (DoesNotExistException $e) {


@SebastianKrupinski just remove the try-catch. There is no exception to expect from this method. The db exception is not an expected runtime exception either, hence not documented in the mapper method.

tests/Unit/Provider/Command/MessageSendTest.php

kesselb · 2024-09-06T16:41:42Z

The branch is a bit behind main.
Please merge main (no rebase, no squash) to have a CI run against the latest main.

ChristophWurst · 2024-09-09T06:39:24Z

Please merge main (no rebase, no squash) to have a CI run against the latest main.

@SebastianKrupinski to provide the reason for this: it allows us to follow your changes. Every time you change the git history with an amend or a rebase, github is no longer able to diff the changes between our last visit and your last push. Every time you change history, we have to review the full diff. In this PR, that's 1200 LOC.

Hope this makes sense.

Please avoid rebases in PRs like this where reviewers left request for changes.

ChristophWurst

The remaining must haves

ChristophWurst · 2024-09-09T13:44:34Z

lib/Provider/Command/MessageSend.php

+				$bcc,
+				$attachments
+			);
+		} catch (\Throwable $e) {


I could not find any documented exception to be thrown by saveMessage. Remove the try-catch.

I left this in one in on purpose, the saveMessage function actually spawns a Imap connection, so I though it would be a good idea to remove the local attachments if the function fails to prevent ghost attachment entries in the database.

Removed

ChristophWurst · 2024-09-09T13:46:11Z

tests/Unit/Provider/Command/MessageSendTest.php

+	private $attachmentService;
+	/** @var MessageSend */
+	private $commandSend;
+


This is spamming the logs on CI and when you run tests locally. I strongly suggest to fix this in this PR. The required effort is low.

tests/Unit/Provider/Command/MessageSendTest.php

SebastianKrupinski · 2024-09-09T13:53:32Z

Please merge main (no rebase, no squash) to have a CI run against the latest main.

@SebastianKrupinski to provide the reason for this: it allows us to follow your changes. Every time you change the git history with an amend or a rebase, github is no longer able to diff the changes between our last visit and your last push. Every time you change history, we have to review the full diff. In this PR, that's 1200 LOC.

Hope this makes sense.

Please avoid rebases in PRs like this where reviewers left request for changes.

Sorry, i did a merge the first time and it messed up the app, couldn't get it to run, so after trying to fix it, I had to delete it re-clone it and redo the changes.

kesselb · 2024-09-09T15:21:08Z

lib/Provider/Command/MessageSend.php

+	 */
+	protected function purgeSavedAttachments(array $attachments): void {
+		foreach ($attachments as $attachment) {
+			$this->attachmentService->deleteAttachment($attachment->getUserId(), $attachment->getId());


getUserId and getId only work when you pass an LocalAttachment object. You are calling jsonSerialize in Line 95, and therefore we have a plain array here.

I prefer working with real objects rather than plain arrays and therefore recommend removing jsonSerialize in Line 95, add the phpdoc back for purgeSavedAttachmends, and use array_map(static fn(LocalAttachment $attachment) => $attachment->jsonSerialize(), $attachments) in Line 109.

Done. I fixed the oversight with the LocalAttachment. But i left he phpdoc as paslm is still not happy and I don't want miss another deadline because of paslm.

@param array<int, LocalAttachment> $attachments collection of local attachment objects

SebastianKrupinski · 2024-09-09T15:26:01Z

@ChristophWurst

These two comments...

#9651 (comment)
#9651 (comment)

From the selected code block, I think you are looking at a old version of the PR the code was already changed in this commit.

7f85f0d#diff-b2fd4b9c63f5641101d4ec474c50008f95fc16af2c64ae2972a22e607f88932d

ChristophWurst · 2024-09-09T15:38:15Z

From the selected code block, I think you are looking at a old version of the PR the code was already changed in this commit.

See https://github.com/nextcloud/mail/actions/runs/10755754956/job/29827858067?pr=9651. This is the latest CI run.

SebastianKrupinski · 2024-09-09T17:29:41Z

From the selected code block, I think you are looking at a old version of the PR the code was already changed in this commit.

See nextcloud/mail/actions/runs/10755754956/job/29827858067?pr=9651. This is the latest CI run.

Okay, I found the confusion it was a different variable then what was highlighted. Fixed.

the phpdoc is correct now

ChristophWurst · 2024-09-11T16:32:22Z

@SebastianKrupinski please rebase & squash into one commit

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

SebastianKrupinski · 2024-09-11T17:13:34Z

@ChristophWurst done

SebastianKrupinski added enhancement 2. developing feature: mail provider labels May 16, 2024

SebastianKrupinski requested a review from ChristophWurst as a code owner May 16, 2024 23:14

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

github-actions bot added the feedback-requested label Jun 2, 2024

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

SebastianKrupinski force-pushed the feature/request-803 branch from ac83ff7 to db9dc0e Compare June 7, 2024 15:28

SebastianKrupinski force-pushed the feature/request-803 branch from b3a5ab4 to 35075d7 Compare June 26, 2024 17:00

SebastianKrupinski added 3. to review and removed 2. developing labels Jul 9, 2024

SebastianKrupinski requested review from miaulalala and kesselb July 9, 2024 14:07

SebastianKrupinski force-pushed the feature/request-803 branch from 3a1d94a to ece67ba Compare July 9, 2024 15:09

SebastianKrupinski mentioned this pull request Jul 9, 2024

feature: mail provider backend nextcloud/server#45383

Merged

kesselb reviewed Jul 12, 2024

View reviewed changes

SebastianKrupinski force-pushed the feature/request-803 branch 2 times, most recently from 221745f to 20dc43a Compare July 13, 2024 10:44

miaulalala reviewed Jul 15, 2024

View reviewed changes

lib/AppInfo/Application.php Outdated Show resolved Hide resolved

miaulalala requested changes Jul 15, 2024

View reviewed changes

SebastianKrupinski force-pushed the feature/request-803 branch from ade51c2 to a9898e6 Compare July 17, 2024 15:08

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

SebastianKrupinski force-pushed the feature/request-803 branch from a9898e6 to d15711e Compare July 23, 2024 21:01

kesselb reviewed Sep 3, 2024

View reviewed changes

ChristophWurst requested changes Sep 4, 2024

View reviewed changes

kesselb mentioned this pull request Sep 6, 2024

docs: document the expected format for convertToRecipient #10114

Merged

kesselb reviewed Sep 6, 2024

View reviewed changes

tests/Unit/Provider/Command/MessageSendTest.php Show resolved Hide resolved

SebastianKrupinski force-pushed the feature/request-803 branch from 167374a to 7f85f0d Compare September 8, 2024 00:38

ChristophWurst requested changes Sep 9, 2024

View reviewed changes

kesselb previously requested changes Sep 9, 2024

View reviewed changes

SebastianKrupinski requested review from ChristophWurst and kesselb September 9, 2024 17:37

kesselb mentioned this pull request Sep 9, 2024

fix: set content type parameters for attachments #10057

Merged

1 task

ChristophWurst approved these changes Sep 10, 2024

View reviewed changes

ChristophWurst added 4. to release and removed 3. to review labels Sep 11, 2024

ChristophWurst removed request for st3iny and GretaD September 11, 2024 16:34

feat: mail provider backend

ce653d0

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

SebastianKrupinski force-pushed the feature/request-803 branch from fd7ceff to ce653d0 Compare September 11, 2024 17:12

ChristophWurst enabled auto-merge September 11, 2024 17:14

ChristophWurst merged commit 18f93fd into main Sep 11, 2024
33 of 34 checks passed

ChristophWurst deleted the feature/request-803 branch September 11, 2024 18:32

	public function findByUserIdAndAddress(string $userId, string $address): array {
	public function findByUserIdAndAddress(string $userId, string $email): array {

	* @return array<int,array{email: string, label?: string}>
	* @return array<int,array{email: ?string, label?: ?string}>

	$this->throwException(new \Exception('Something went wrong'))
	$this->throwException(new UploadException('Something went wrong'))

feature: mail provider backend #9651

feature: mail provider backend #9651

Conversation

SebastianKrupinski commented May 16, 2024 • edited by ChristophWurst Loading

SebastianKrupinski commented May 16, 2024

SebastianKrupinski commented May 24, 2024

SebastianKrupinski commented May 24, 2024

SebastianKrupinski commented May 28, 2024

github-actions bot commented Jun 2, 2024

SebastianKrupinski commented Jul 9, 2024

miaulalala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianKrupinski Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianKrupinski Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianKrupinski Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kesselb commented Sep 6, 2024

ChristophWurst commented Sep 9, 2024

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianKrupinski Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianKrupinski commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianKrupinski commented Sep 9, 2024

ChristophWurst commented Sep 9, 2024

SebastianKrupinski commented Sep 9, 2024

ChristophWurst commented Sep 11, 2024

SebastianKrupinski commented Sep 11, 2024

SebastianKrupinski commented May 16, 2024 •

edited by ChristophWurst

Loading

SebastianKrupinski Sep 5, 2024 •

edited

Loading

SebastianKrupinski Sep 5, 2024 •

edited

Loading

SebastianKrupinski Sep 5, 2024 •

edited

Loading

SebastianKrupinski Sep 9, 2024 •

edited

Loading