fix(sharing): send share emails for internal users too

[miaulalala]

• Resolves: fix(sharing): send share emails for internal users too #49898

Summary

Based on the config example, sending emails for shares defaults to true.

Currently, when the frontend doesn't provide a value we break this for internal users and only send emails for external shares.

If there is no value provided from the frontend, this PR now checks the config option, and determines if the type is an internal share type or an external share via email (TYPE_EMAIL).

For external shares, the share email is sent regardless of the config option.

TODO

• ...

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[provokateurin]

According to the docks the system config is about emails for new incoming shares. I don't think this is the right place to disable that? It should be set on the share, but rather when the incoming share is added and the email should be sent.

I can't make sense of the config and your change, maybe you can explain it in more depth to help me understand it.
Also you should add a test for this ;)

[Antreesy]

For the context: change of default beaviour for sending email about the share was introduced here: #48381

• Share with TYPE_EMAIL sends email by default
• Others (like TYPE_USER) don't do it

Ideally, this algorithm should respect user preferences set in Personal Settings -> Notifications:

Could be done in this palce?

server/apps/files_sharing/lib/Controller/ShareAPIController.php

Lines 675 to 682 in e2d2a17

	if ($shareType === IShare::TYPE_USER) {
	// Valid user is required to share
	if ($shareWith === null || !$this->userManager->userExists($shareWith)) {
	throw new OCSNotFoundException($this->l->t('Please specify a valid account to share with'));
	}
	$share->setSharedWith($shareWith);
	$share->setPermissions($permissions);
	} elseif ($shareType === IShare::TYPE_GROUP) {

[miaulalala]

According to the docks the system config is about emails for new incoming shares. I don't think this is the right place to disable that? It should be set on the share, but rather when the incoming share is added and the email should be sent.

I can't make sense of the config and your change, maybe you can explain it in more depth to help me understand it. Also you should add a test for this ;)

Sure thing.

Due to the changes in the frontend, the sendMail param is not always set.
#48381 changed the share- API to only handle explicit values, and if the value is null, the share only sent via mail for TYPE_EMAIL, i. e. external users. This means, internal users never get a share notification.

I know the config option is a bit weird but it exists. This PR is not a fix for the missing value in the frontend, but it restores the previous behaviour from before 30.0.2, where mails were sent while still handling null values for the frontend.

[kesselb]

Thanks @miaulalala for taking care 🙏

This issue was also brought to my attention last week. In Nextcloud 29, creating a new user share triggers a new share notification as expected. However, this behavior no longer works in Nextcloud 30.0.4.

As mentioned before, this seems to be related to the mailSend parameter. In Nextcloud 29, the create share response shows 1, whereas in Nextcloud 30, it shows 0.

Especially on configurations with 'sharing.enable_share_accept' = true,, it's a bit annoying that the notification is gone because the user's need to check the pending shares section.

29

Screencast.From.2024-12-21.16-43-49.webm

30

Screencast.From.2024-12-21.16-44-15.webm

[kesselb]

Personal Settings -> Notifications:

It is confusing that there are two different ways to inform a user about a new share.

This PR specifically targets the notification sent by the sharebymail app. However, the screenshot provided refers to the configuration for the activity app. The notification from the sharebymail app is sent immediately, while the activity notification offers more flexibility—for example, sending notifications once per day or only via push.

I'm unsure about Maksim's idea to respect the configuration from the activity app within the sharebymail app.

[Antreesy]

I checked the behaviour with Joas, activity app settings should have nothing to do with current issue. So forget my suggestion 🙄

[nfebe]

One inline comment.

[tigernero79]

Is it possible to expect this fix for version 30.0.5?

[nfebe]

There were some requests to have some kind of control over this and flagging this from the frontend as such : #50064 works but the user has explicitly activated the toggle.