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(ShareAPI): Send mails for mail shares by default #48381

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

nfebe
Copy link
Contributor

@nfebe nfebe commented Sep 26, 2024

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

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix makes sense, curious how it broke though.

apps/files_sharing/lib/Controller/ShareAPIController.php Outdated Show resolved Hide resolved
@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from 6553268 to 8103393 Compare September 26, 2024 12:31
@nfebe
Copy link
Contributor Author

nfebe commented Sep 26, 2024

/backport to stable30

@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from 8103393 to 410854f Compare September 26, 2024 13:08
@nfebe nfebe enabled auto-merge September 26, 2024 13:08
@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from 410854f to 3f7528b Compare September 26, 2024 13:52
@nfebe nfebe 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
@provokateurin
Copy link
Member

Why rebase it so often?

@nfebe
Copy link
Contributor Author

nfebe commented Sep 30, 2024

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.

@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from 9f0eda3 to aab671f Compare October 7, 2024 06:57
Copy link
Contributor Author

@nfebe nfebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks "File Requests"

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.

@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from aab671f to 91a3ce1 Compare October 7, 2024 07:57
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops

@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from 91a3ce1 to e089e40 Compare October 7, 2024 08:09
@nfebe nfebe disabled auto-merge October 7, 2024 08:11
@skjnldsv
Copy link
Member

skjnldsv commented Oct 8, 2024

My fix would be to set the sendMail param to true by default, as file requests set it to false anyway 🤔

@nfebe
Copy link
Contributor Author

nfebe commented Oct 23, 2024

My fix would be to set the sendMail param to true by default, as file requests set it to false anyway 🤔

and what about other things that use the API without specifying what goes to sendMail???

@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from e089e40 to a154393 Compare October 23, 2024 21:07
@nfebe
Copy link
Contributor Author

nfebe commented Oct 23, 2024

/compile

@susnux
Copy link
Contributor

susnux commented Oct 24, 2024

and what about other things that use the API without specifying what goes to sendMail???

Agree, we can not change default behavior as this would be a breaking change.

@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch 2 times, most recently from dd82bef to c1b0f46 Compare October 24, 2024 13:13
@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from c1b0f46 to e69c1a5 Compare October 24, 2024 13:39
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 <fenn25.fn@gmail.com>
@nfebe nfebe force-pushed the fix/48012/fix-share-email-send-mail-share branch from e69c1a5 to e2d2a17 Compare October 24, 2024 13:41
@nfebe nfebe enabled auto-merge October 24, 2024 13:41
@nfebe nfebe merged commit bc6a3e8 into master Oct 24, 2024
177 checks passed
@nfebe nfebe deleted the fix/48012/fix-share-email-send-mail-share branch October 24, 2024 16:28
Copy link
Contributor

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!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🏗️ In progress
Development

Successfully merging this pull request may close these issues.

[Bug]: 30.0.0 share by email - no email is sent
5 participants