feat: mail provider settings #48134

SebastianKrupinski · 2024-09-17T11:48:27Z

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

apps/settings/lib/AppInfo/Application.php

apps/settings/lib/Listener/MailSettingsEventListener.php

apps/settings/lib/AppInfo/Application.php

apps/settings/lib/Listener/MailSettingsEventListener.php

SebastianKrupinski · 2024-09-17T16:18:17Z

New setting in administration section

SebastianKrupinski · 2024-09-17T16:19:16Z

@marcoambrosini

kesselb

config.sample.php needs an update

kesselb · 2024-09-17T17:20:14Z

apps/settings/lib/Listener/MailSettingsEventListener.php

+			'section_id' => 'server',
+			'storage_type' => DeclarativeSettingsTypes::STORAGE_TYPE_EXTERNAL,
+			'title' => $this->l->t('System Mails'),
+			'description' => $this->l->t('System e-mails are messages generated automatically by Nextcloud. They are sent for example when a share is created or when inviting attendees to a calendar event..'),


They are sent for example when a share is created

I find it weird to use "share is created" as an example because we don't use the mail provider there.

This is what design wanted...

I'm quite sure that design wanted a description of what the feature actually does. I can't imagine that design asked you to mention in the description that this feature toggle affects the behavior of the sharebyemail provider, whereas it doesn't affect it.

Yep, if we don't use the mail provider there than the example is wrong and we should change it to something that is actually affected by the setting

kesselb · 2024-09-17T17:28:49Z

apps/settings/lib/Listener/MailSettingsEventListener.php

+
+		switch ($event->getFieldId()) {
+			case 'mail_providers_disabled':
+				$this->config->setSystemValue('mail_providers_disabled', $event->getValue());


I don't know how you planned the feature with @ChristophWurst, but changing values in config.php via web admin is tricky.

Examples:

config_is_read_only = true

multiple config files (the additional configuration files overwrite the values in config.php, setSystemValue modifies only config.php not the additional configuration files)

The unfinished and undocumented "overwrite configuration variables via env" feature may overload the variable as well.

That's all unrelated to this pull request, but the incomplete handling in our configuration classes is the reason that we usually don't change config.php via web admin.

I recommend against adding a ui with system config as backend.

This feature functions identical to the new Windows support feature that was put in last month

kesselb · 2024-09-17T17:31:01Z

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

-			if ($user !== null) {
-				// retrieve appropriate service with the same address as sender
-				$mailService = $this->mailManager->findServiceByAddress($user->getUID(), $sender);
+			if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {


Suggested change

if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {

if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {

Must be documented in https://github.com/nextcloud/server/blob/master/config/config.sample.php.

https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html is also generated from the config.sample.php.

kesselb · 2024-09-17T17:32:07Z

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

-			if ($user !== null) {
-				// retrieve appropriate service with the same address as sender
-				$mailService = $this->mailManager->findServiceByAddress($user->getUID(), $sender);
+			if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {


Suggested change

if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {

if ($this->config->getSystemValueBool('mail_providers_disabled', false)) {

kesselb · 2024-09-17T17:33:57Z

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

-			if ($user !== null) {
-				// retrieve appropriate service with the same address as sender
-				$mailService = $this->mailManager->findServiceByAddress($user->getUID(), $sender);
+			if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {


Easier to use/read: mail_providers_enabled with default true

This is what design wanted...

provokateurin · 2024-09-17T18:35:36Z

apps/settings/lib/Listener/MailSettingsEventListener.php

Please at least use https://github.com/nextcloud/server/blob/master/apps/testing/lib/Settings/DeclarativeSettingsForm.php if you really want to do this.

apps/settings/lib/Listener/SystemMailSettingsListener.php

+			return;
+		}
+
+		if ($event instanceof DeclarativeSettingsSetValueEvent) {


Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

SebastianKrupinski · 2024-09-24T01:30:53Z

Latest version

marcoambrosini

✅ from design

kesselb · 2024-09-24T08:47:31Z

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

-			if ($user !== null) {
-				// retrieve appropriate service with the same address as sender
-				$mailService = $this->mailManager->findServiceByAddress($user->getUID(), $sender);
+			if ($this->config->getValueInt('core', 'mail_providers_disabled', 0) === 0) {


Use a boolean?

I would also recommend using mail_providers_enabled true/false rather than mail_providers_disabled true/false because it's easier to read.

Example:

if (!$this->config->getValueBool('core', 'mail_providers_disabled', false))

vs.

if ($this->config->getValueBool('core', 'mail_providers_enabled', true))

kesselb · 2024-09-24T09:13:21Z

apps/settings/lib/Settings/Admin/SystemMail.php

+			'section_type' => DeclarativeSettingsTypes::SECTION_TYPE_ADMIN,
+			'section_id' => 'server',
+			'storage_type' => DeclarativeSettingsTypes::STORAGE_TYPE_EXTERNAL,
+			'title' => $this->l->t('System Mails'),


I would prefer to use Mail Providers instead of System Mails. System Mails can mean anything.

kesselb · 2024-09-24T09:17:16Z

apps/settings/lib/Settings/Admin/SystemMail.php

+			'section_id' => 'server',
+			'storage_type' => DeclarativeSettingsTypes::STORAGE_TYPE_EXTERNAL,
+			'title' => $this->l->t('System Mails'),
+			'description' => $this->l->t('System emails are automatically generated by Nextcloud when people perform actions like inviting others to calendar events.'),


What do you think about:

Nextcloud 30 introduced a feature called mail provider to send emails through the user's personal email account when possible. Currently, it only works with calendar invitations. You need Nextcloud 30, Nextcloud Mail 4.1 and there must be an email account in Nextcloud Mail matching the email address of the user in Nextcloud.

I think we should try to explain what's necessary to make the feature work. The requirements Mail 4.0 + a matching email account should be explained somewhere, how should a user otherwise know how it works?

We should have some documentation for https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/email_configuration.html

kesselb · 2024-09-24T09:28:36Z

apps/settings/lib/Settings/Admin/SystemMail.php

+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors


Suggested change

* SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors

* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors

kesselb · 2024-09-24T09:28:43Z

apps/settings/lib/Listener/SystemMailSettingsListener.php

+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors


Suggested change

* SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors

* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors

kesselb · 2024-09-24T09:36:34Z

apps/settings/lib/Listener/SystemMailSettingsListener.php

+
+	private function handleGetValue(DeclarativeSettingsGetValueEvent $event): void {
+
+		$event->setValue(


Why so complicated? ;)

We only have one field right now.

if ($event->getFieldId() === 'mail_providers_disabled') { $event->setValue($this->config->getValueInt('core', 'mail_providers_disabled', 0)); return; }

kesselb · 2024-09-24T09:37:33Z

apps/settings/lib/Listener/SystemMailSettingsListener.php

+
+	private function handleSetValue(DeclarativeSettingsSetValueEvent $event): void {
+
+		switch ($event->getFieldId()) {


Same here ;)

if ($event->getFieldId() === 'mail_providers_disabled') { $this->config->setValueInt('core', 'mail_providers_disabled', (int)$event->getValue()); $event->stopPropagation(); return; }

kesselb · 2024-09-24T10:19:04Z

The Linter is failing.

SebastianKrupinski marked this pull request as draft September 17, 2024 11:48

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

SebastianKrupinski self-assigned this Sep 17, 2024

SebastianKrupinski added the 3. to review Waiting for reviews label Sep 17, 2024

SebastianKrupinski marked this pull request as ready for review September 17, 2024 13:28

SebastianKrupinski requested review from ChristophWurst, miaulalala and tcitworld as code owners September 17, 2024 13:28

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

susnux reviewed Sep 17, 2024

View reviewed changes

apps/settings/lib/Listener/MailSettingsEventListener.php Outdated Show resolved Hide resolved

susnux reviewed Sep 17, 2024

View reviewed changes

apps/settings/lib/Listener/MailSettingsEventListener.php Outdated Show resolved Hide resolved

SebastianKrupinski requested a review from susnux September 17, 2024 14:02

github-advanced-security bot found potential problems Sep 17, 2024

View reviewed changes

apps/settings/lib/Listener/MailSettingsEventListener.php Fixed Show fixed Hide fixed

kesselb requested changes Sep 17, 2024

View reviewed changes

provokateurin reviewed Sep 17, 2024

View reviewed changes

github-advanced-security bot found potential problems Sep 18, 2024

View reviewed changes

SebastianKrupinski force-pushed the feat/mail-provider-settings branch from 04d3529 to cce92cc Compare September 18, 2024 15:56

SebastianKrupinski requested review from provokateurin and kesselb September 19, 2024 09:53

SebastianKrupinski added 8 commits September 23, 2024 21:27

feat: mail provider settings

926ed33

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! feat: mail provider settings

b73f96b

fixup! feat: mail provider settings

46a3acb

fixup! feat: mail provider settings

ac59598

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! feat: mail provider settings

93d64b0

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! feat: mail provider settings

9471871

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! feat: mail provider settings

8de14f8

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

fixup! feat: mail provider settings

da9179b

Signed-off-by: SebastianKrupinski <krupinskis05@gmail.com>

SebastianKrupinski force-pushed the feat/mail-provider-settings branch from 9d2d55c to da9179b Compare September 24, 2024 01:29

marcoambrosini approved these changes Sep 24, 2024

View reviewed changes

kesselb reviewed Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: mail provider settings #48134

feat: mail provider settings #48134

SebastianKrupinski commented Sep 17, 2024

SebastianKrupinski commented Sep 17, 2024

SebastianKrupinski commented Sep 17, 2024

kesselb left a comment

kesselb Sep 17, 2024

SebastianKrupinski Sep 17, 2024

kesselb Sep 17, 2024

marcoambrosini Sep 19, 2024

kesselb Sep 17, 2024

SebastianKrupinski Sep 17, 2024

kesselb Sep 17, 2024

kesselb Sep 17, 2024

kesselb Sep 17, 2024

SebastianKrupinski Sep 17, 2024

provokateurin Sep 17, 2024

SebastianKrupinski commented Sep 24, 2024

marcoambrosini left a comment

kesselb Sep 24, 2024

kesselb Sep 24, 2024

kesselb Sep 24, 2024 •

edited

Loading

kesselb Sep 24, 2024

kesselb Sep 24, 2024

kesselb Sep 24, 2024

kesselb Sep 24, 2024

kesselb Sep 24, 2024

kesselb commented Sep 24, 2024

	if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {
	if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {

	if ($this->config->getSystemValueInt('mail_providers_disabled', 0) === 0) {
	if ($this->config->getSystemValueBool('mail_providers_disabled', false)) {

	* SPDX-FileCopyrightText: 2023 Nextcloud GmbH and Nextcloud contributors
	* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors

	* SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors
	* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors


		private function handleGetValue(DeclarativeSettingsGetValueEvent $event): void {

		$event->setValue(


		private function handleSetValue(DeclarativeSettingsSetValueEvent $event): void {

		switch ($event->getFieldId()) {

feat: mail provider settings #48134

Are you sure you want to change the base?

feat: mail provider settings #48134

Conversation

SebastianKrupinski commented Sep 17, 2024

SebastianKrupinski commented Sep 17, 2024

SebastianKrupinski commented Sep 17, 2024

kesselb left a comment

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

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

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 commented Sep 24, 2024

marcoambrosini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kesselb Sep 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kesselb commented Sep 24, 2024

kesselb Sep 24, 2024 •

edited

Loading