fix(ShareAPI): Send mails for mail shares by default #48381

Fenn-CS · 2024-09-26T11:08:04Z

Summary

It looks like, the frontend it needs to provide the sendMail param for the backend to decide whether mails would be sent.

Our UI does not have that at the moment so it should default to sending emails always for mail shares.

Not exactly sure how this was handled earlier but this is a good starting point.

Resolves : #48012

Checklist

Backports requested where applicable (ex: critical bugfixes)

apps/files_sharing/lib/Controller/ShareAPIController.php

provokateurin

Fix makes sense, curious how it broke though.

apps/files_sharing/lib/Controller/ShareAPIController.php

Fenn-CS · 2024-09-26T13:08:21Z

/backport to stable30

provokateurin · 2024-09-30T11:13:43Z

Why rebase it so often?

Fenn-CS · 2024-09-30T12:35:43Z

Why rebase it so often?

Some tests that seem unrelated (cypress especially are failing consistently)...

Re-basing triggers a rerun, but also adds the latest updates on master in case something that broke the tests has been resolved. (Because a bunch of other PRs are failing tests too)...

Now taking a closer to see if something is indeed broken.

Fenn-CS

This breaks "File Requests"

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

Lines 721 to 727 in 363ba6b

    
           if (is_string($shareWith) && $shareType === IShare::TYPE_EMAIL) { 
        
           	// If sending a mail have been requested, validate the mail address 
        
           	if ($share->getMailSend() && !$this->mailer->validateMailAddress($shareWith)) { 
        
           		throw new OCSNotFoundException($this->l->t('Please specify a valid email address')); 
        
           	} 
        
           	$share->setSharedWith($shareWith); 
        
           }

Why?

File requests are sent with an empty string for shareWith which the backend uses to check if a shareWith value is available (poorly so)
The file requests shareType is 4, which is mail share and I am not sure why that is.

It looks like, the frontend it needs to provide the `sendMail` param for the backend to decide wether mails would be sent. Our UI does not have that at the moment so it should default to sending emails always for mail shares. Not exactly sure how this was handled earlier but this is a good starting point. Resolves : #48012 Signed-off-by: fenn-cs <[email protected]>

apps/files_sharing/lib/Controller/ShareAPIController.php

provokateurin

Oops

"File request" is the new "File drop"... File drop was only possible on link shares. The mails sent at the end of creating a file request is "sending the link to various emails" it does should not turn the share type to email. Signed-off-by: fenn-cs <[email protected]>

provokateurin · 2024-10-07T08:15:42Z

apps/files_sharing/src/components/NewFileRequestDialog.vue

-					shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
+					shareType: ShareType.Link,


Not sure about this, @skjnldsv what was the idea behind this?

Yes, I checked on talk and seems he is on holiday... However as far as I know "file drop" which is what was converted to "File request" was only possible on public link shares.

Leaving all this context as I won't be around as from tomorrow, however, this is an important fix. Mails not being sent out for mails shares is quite critical.

@susnux May also be able to help in the absence of Bath.

Fenn-CS · 2024-10-07T08:56:40Z

Add some more context on why the tests are failing and some related stuff with a plausible fix:

Share type setting can be changed (current solution), this seems to prevent the sendMail function for file requests as at now but since the links can be copied and shared, it's way better than breaking email shares on prod.

Fenn-CS · 2024-10-07T08:13:09Z

apps/files_sharing/src/components/NewFileRequestDialog.vue

@@ -284,7 +284,7 @@ export default defineComponent({
 				const request = await axios.post<OCSResponse>(shareUrl, {
 					// Always create a file request, but without mail share
 					// permissions, only a share link will be created.
-					shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
+					shareType: ShareType.Link,


File requests, should always be link share? The concept of mail shares is different.

Fenn-CS · 2024-10-07T09:02:48Z

apps/files_sharing/src/components/NewFileRequestDialog.vue

-					shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
+					shareType: ShareType.Link,


Yes, I checked on talk and seems he is on holiday... However as far as I know "file drop" which is what was converted to "File request" was only possible on public link shares.

Leaving all this context as I won't be around as from tomorrow, however, this is an important fix. Mails not being sent out for mails shares is quite critical.

Fenn-CS added 3. to review Waiting for reviews feature: sharing backport-request 30-feedback labels Sep 26, 2024

Fenn-CS requested review from artonge, skjnldsv, sorbaugh and come-nc September 26, 2024 11:08

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 33e46ad to 6553268 Compare September 26, 2024 11:10

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

View reviewed changes

apps/files_sharing/lib/Controller/ShareAPIController.php Fixed Show fixed Hide fixed

provokateurin approved these changes Sep 26, 2024

View reviewed changes

apps/files_sharing/lib/Controller/ShareAPIController.php Outdated Show resolved Hide resolved

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 6553268 to 8103393 Compare September 26, 2024 12:31

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 8103393 to 410854f Compare September 26, 2024 13:08

Fenn-CS enabled auto-merge September 26, 2024 13:08

come-nc approved these changes Sep 26, 2024

View reviewed changes

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 410854f to 3f7528b Compare September 26, 2024 13:52

Fenn-CS mentioned this pull request Sep 26, 2024

Add "Send mail" toggle in sharing details ui for new shares #48397

Open

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch 3 times, most recently from e2fa7e1 to 9f0eda3 Compare September 30, 2024 11:01

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 9f0eda3 to aab671f Compare October 7, 2024 06:57

Fenn-CS commented Oct 7, 2024

View reviewed changes

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from aab671f to 91a3ce1 Compare October 7, 2024 07:57

provokateurin approved these changes Oct 7, 2024

View reviewed changes

apps/files_sharing/lib/Controller/ShareAPIController.php Outdated Show resolved Hide resolved

provokateurin requested changes Oct 7, 2024

View reviewed changes

Fenn-CS force-pushed the fix/48012/fix-share-email-send-mail-share branch from 91a3ce1 to e089e40 Compare October 7, 2024 08:09

Fenn-CS disabled auto-merge October 7, 2024 08:11

provokateurin reviewed Oct 7, 2024

View reviewed changes

Fenn-CS commented Oct 7, 2024

View reviewed changes

thbemme mentioned this pull request Oct 7, 2024

[Bug]: 30.0.0 share by email - no email is sent #48012

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ShareAPI): Send mails for mail shares by default #48381

fix(ShareAPI): Send mails for mail shares by default #48381

Fenn-CS commented Sep 26, 2024

provokateurin left a comment

Fenn-CS commented Sep 26, 2024

provokateurin commented Sep 30, 2024

Fenn-CS commented Sep 30, 2024 •

edited

Loading

Fenn-CS left a comment

provokateurin left a comment

provokateurin Oct 7, 2024

Fenn-CS Oct 7, 2024

Fenn-CS Oct 7, 2024

Fenn-CS commented Oct 7, 2024

Fenn-CS Oct 7, 2024

Fenn-CS Oct 7, 2024

	if (is_string($shareWith) && $shareType === IShare::TYPE_EMAIL) {
	// If sending a mail have been requested, validate the mail address
	if ($share->getMailSend() && !$this->mailer->validateMailAddress($shareWith)) {
	throw new OCSNotFoundException($this->l->t('Please specify a valid email address'));
	}
	$share->setSharedWith($shareWith);
	}

		shareType: sharingConfig.isMailShareAllowed ? ShareType.Email : ShareType.Link,
		shareType: ShareType.Link,

fix(ShareAPI): Send mails for mail shares by default #48381

Are you sure you want to change the base?

fix(ShareAPI): Send mails for mail shares by default #48381

Conversation

Fenn-CS commented Sep 26, 2024

Summary

Checklist

provokateurin left a comment

Choose a reason for hiding this comment

Fenn-CS commented Sep 26, 2024

provokateurin commented Sep 30, 2024

Fenn-CS commented Sep 30, 2024 • edited Loading

Fenn-CS left a comment

Choose a reason for hiding this comment

provokateurin left a comment

Choose a reason for hiding this comment

provokateurin Oct 7, 2024

Choose a reason for hiding this comment

Fenn-CS Oct 7, 2024

Choose a reason for hiding this comment

Fenn-CS Oct 7, 2024

Choose a reason for hiding this comment

Fenn-CS commented Oct 7, 2024

Fenn-CS Oct 7, 2024

Choose a reason for hiding this comment

Fenn-CS Oct 7, 2024

Choose a reason for hiding this comment

Fenn-CS commented Sep 30, 2024 •

edited

Loading