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
obey to config in share mail notifications APIs #36161
Conversation
Codecov Report
@@ Coverage Diff @@
## master #36161 +/- ##
=======================================
Coverage 54.01% 54.01%
=======================================
Files 63 63
Lines 7404 7404
Branches 1309 1309
=======================================
Hits 3999 3999
Misses 3019 3019
Partials 386 386
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #36161 +/- ##
=======================================
Coverage 54.01% 54.01%
=======================================
Files 63 63
Lines 7404 7404
Branches 1309 1309
=======================================
Hits 3999 3999
Misses 3019 3019
Partials 386 386
Continue to review full report at Codecov.
|
@@ -75,6 +80,10 @@ public function __construct( | |||
* @return Result | |||
*/ | |||
public function notifyPublicLinkRecipientsByEmail($link, $recipients, $personalNote) { | |||
if ($this->config->getAppValue('core', 'shareapi_allow_public_notification', 'no') !== 'yes') { | |||
$message = "Public link mail notification is not allowed"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be inside the t
function for transifex to pick up the text properly, otherwise we might not get the translations for that message.
apps/files_sharing/tests/Controller/NotificationControllerTest.php
Outdated
Show resolved
Hide resolved
8d629c5
to
1fbf953
Compare
@jvillafanez I applied suggested changes. Please review one more time. Thanks. |
I kind of have to complain about files_sharing using configuration for core. If it's a setting for core it should be core the one managing that setting, files_sharing shouldn't care about it. We can go with this solution, but I think we should include a fixme. |
1fbf953
to
8c962f8
Compare
@jvillafanez instead of adding a fixme, I moved the controls to |
Description
We have two different admin options (public link and internal mail) for enabling/disabling share mail notifications, but APIs don't obey to this config. This pr fixes this behavior.
Related Issue
Allow users to send mail notification for shared files
#36156Motivation and Context
Fighting with bugs.
How Has This Been Tested?
Manually with Curl and unit test.
Types of changes
Checklist: