nextcloud
feature: mail provider backend
Initial Mail Provider Implementation
https://github.com/nextcloud/server/issues/5080
This PR requires the server PR first...
https://github.com/nextcloud/server/pull/45383
TODO: Separate attachment handling for file system files (saved on the system) and string data files (created or loaded in memory)
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!
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Testing process:
Pull this PR and server PR - https://github.com/nextcloud/server/pull/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
"OCA\\Mail\\Controller\\OutboxController::update(): Argument #5 ($editorBody) must be of type string, null given, called in /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php on line 208 in file '/var/www/nextcloud/apps/mail/lib/Controller/OutboxController.php' line 178"when using "Send now" from the Outbox
@miaulalala
So setting the editor body in the message does let the message be opened in the GUI.
BUT there is a side effect...
- After opening the message pressing send destroys the formatting of the original message.
- After opening the message pressing close destroys the attachment and prevents the message from sending.
I would recommend we disable this for now.
"OCA\\Mail\\Controller\\OutboxController::update(): Argument #5 ($editorBody) must be of type string, null given, called in /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php on line 208 in file '/var/www/nextcloud/apps/mail/lib/Controller/OutboxController.php' line 178"when using "Send now" from the Outbox@miaulalala
So setting the editor body in the message does let the message be opened in the GUI.
BUT there is a side effect...
1. After opening the message pressing send destroys the formatting of the original message. 2. After opening the message pressing close destroys the attachment and prevents the message from sending.I would recommend we disable this for now.
this is not about opening the message in the gui - it's about the "Send now" button in the three dot menu in the outbox.
A message that is sent through the outbox should behave exactly as a message sent through mail would. Disabling features is not an option.
@SebastianKrupinski as discussed in our team call, please use fixup commits. It's much easier to review your changes than because GitHub shows only the new changes. I'm aware that I actually asked you for the opposite, sorry about that.
@SebastianKrupinski as discussed in our team call, please use fixup commits. It's much easier to review your changes than because GitHub shows only the new changes. I'm aware that I actually asked you for the opposite, sorry about that.
Yes, I re based and squashed out of habit, which wiped the previous commits. Appologies.
"OCA\\Mail\\Controller\\OutboxController::update(): Argument #5 ($editorBody) must be of type string, null given, called in /var/www/nextcloud/lib/private/AppFramework/Http/Dispatcher.php on line 208 in file '/var/www/nextcloud/apps/mail/lib/Controller/OutboxController.php' line 178"when using "Send now" from the Outbox@miaulalala So setting the editor body in the message does let the message be opened in the GUI. BUT there is a side effect...
1. After opening the message pressing send destroys the formatting of the original message. 2. After opening the message pressing close destroys the attachment and prevents the message from sending.I would recommend we disable this for now.
this is not about opening the message in the gui - it's about the "Send now" button in the three dot menu in the outbox.
A message that is sent through the outbox should behave exactly as a message sent through mail would. Disabling features is not an option.
@miaulalala I agree the "Send now" should work. But these messages are different, because they are automated.
The problem is that this API is used by the server and other apps to send automated messages, that do NOT use the front end editor, therefor there is no editorBody.
So forcing the editorBody to contain content that it can not handle properly causes more issues, as I have stated above. The better solution to this is to fix the "Send Now" action to properly handle automated messages in a followup PR.
It's new that we have messages in the outbox that were not created by a user, and therefore we don't have an editor body.
That we store the editor body in addition to the actual message has the reasoning that CKEditor is an HTML editor for websites and documents, but we are using it for composing emails. The generated HTML is then "transformed" to an HTML for emails. That only works one-way, and therefore we store the actual source as well to make editing possible.
For our current use case, sending the calendar invitation from the user account, it seems unnecessary/unwanted to edit the outgoing message. I assume we need to extend our outbox logic to have mutable and immutable messages. As a start, it might be enough to hide messages in the outbox view with editor body = null. A bit nicer would be to show at least our usual message preview instead of the editor, but I cannot tell how much work that is.
Please align the final approach with Anna and Christoph.
It's new that we have messages in the outbox that were not created by a user, and therefore we don't have an editor body.
That we store the editor body in addition to the actual message has the reasoning that CKEditor is an HTML editor for websites and documents, but we are using it for composing emails. The generated HTML is then "transformed" to an HTML for emails. That only works one-way, and therefore we store the actual source as well to make editing possible.
For our current use case, sending the calendar invitation from the user account, it seems unnecessary/unwanted to edit the outgoing message. I assume we need to extend our outbox logic to have mutable and immutable messages. As a start, it might be enough to hide messages in the outbox view with editor body = null. A bit nicer would be to show at least our usual message preview instead of the editor, but I cannot tell how much work that is.
Please align the final approach with Anna and Christoph.
I agree.
Keeping editorBody null and not allowing edits for automated emails is an acceptable workaround for me :+1:
@ChristophWurst
Done. I will create a separate ticket to alter the outbox logic.
@miaulalala
Outbox send logic was broken. Its fixed now.
https://github.com/nextcloud/mail/pull/10059
The downside is that it take up to 5 minutes until the invitation is delivered. Invite someone to your Google Calendar, Outlook, etc. and the invitation is right there.
I agree this is a downside but at the moment I don't see a better option for a better balance between performance and functionality.
A) I'm not aware about performance issues with many attendees. I assume calendar and external clients do have a loading >animation to signal back that the request is beeing processed.
There are a couple of tickets stating that inviting 10+ attendees fails. At the moment our UI does not show a "Processing animation" or even disable the save buttons, when I insert a debug break point, to stop the save process you can actually re-click and start another save process.
B) Filing the invitation into the outbox is nice because a user could retry if sending failed for temporary reasons. However I don't >see how a failing mail delivery would make the entire PUT command fail.
We can talk about this in a call, but a general overview of the save process (PUT Request) is this...
-> User Generates a PUT Request (Clicks Save) -> Server Validates Data -> Server Compares Events (On update) -> Server Finds Attendees -> Server Generates iTip Messages for Attendees -> Server Saves Event for Attendees (Local NC Users Only) -> Server Generates Emails (For all attendees) -> Server Sends Emails (For all attendees) -> Server Saves Event for Organizer -> Server Returns Response
So the issue of a response that takes too long is to fold, A) the user might re-click B) the client might give up waiting for a response and re-try.
The other issue is that an error during the SMTP send process, on one of the accounts might actual cause the event save process to fail and the event is never saved for the organizer.
Ultimately its @ChristophWurst that has to make the choice... Attempt to send instantly or no
There are a couple of tickets stating that inviting 10+ attendees fails.
Link?
At the moment our UI does not show a "Processing animation" or even disable the save buttons, when I insert a debug break point, to stop the save process you can actually re-click and start another save process.
We should have one, plase log an issue for calendar: https://github.com/nextcloud/calendar
The other issue is that an error during the SMTP send process, on one of the accounts might actual cause the event save process to fail and the event is never saved for the organizer.
An SMTP error should not interrupt the process. Please make sure the mail provider part for the imip plugin handle such a situation properly. I recommend another test case for IMipPluginTest to make sure the code works as expected if an exception is thrown.
Please don't rebase/squash this pull request until approved because that may rewrite the commit hashes. GitHub cannot show the incoming changes then and I have to review everything from the start. If you need to pull the latest changes from main, merge main into this branch.
An SMTP error should not interrupt the process. Please make sure the mail provider part for the imip plugin handle such a situation properly. I recommend another test case for IMipPluginTest to make sure the code works as expected if an exception is thrown.
Sure I can do that, but that will be a separate PR on the server. This is not in scope for the mail app
@kesselb
The downside is that it take up to 5 minutes until the invitation is delivered. Invite someone to your Google Calendar, Outlook, etc. and the invitation is right there.
What if we make this a configurable option? In the mail app settings... we can add a "Bypass outbox for system messages" or "Send system messages instantly"
The other issue is that an error during the SMTP send process, on one of the accounts might actual cause the event save process to fail and the event is never saved for the organizer.
Ultimately its @ChristophWurst that has to make the choice... Attempt to send instantly or no
We currently send instantly, so let's keep this behavior for the Mail provider too.
For fault tolerance I suggest we first add all the emails to the outbox, then we trigger sending. Any messages that run into an (recoverable) error will remain in the outbox and get retried by cron.
In the future we can discuss async sending. With many attendees and slow SMTP it's indeed a bit of a bad UX and potential for a timeout. But fixing this here is out of scope.
The branch is a bit behind main. Please merge main (no rebase, no squash) to have a CI run against the latest main.
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.
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.
@ChristophWurst
These two comments...
https://github.com/nextcloud/mail/pull/9651#discussion_r1750300381 https://github.com/nextcloud/mail/pull/9651#discussion_r1750301662
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.
https://github.com/nextcloud/mail/pull/9651/commits/7f85f0d8408c8336de4b5f9d09723f66a2bc1bb9#diff-b2fd4b9c63f5641101d4ec474c50008f95fc16af2c64ae2972a22e607f88932d
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.