Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

miaulalala
Copy link
Contributor

  • Resolves: #

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

Signed-off-by: Anna Larch <anna@nextcloud.com>
@miaulalala miaulalala self-assigned this Dec 17, 2024
@miaulalala miaulalala added 3. to review Waiting for reviews bug labels Dec 17, 2024
@provokateurin
Copy link
Member

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
Copy link
Contributor

Antreesy commented Dec 17, 2024

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:
image

Could be done in this palce?

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants