[WIP] notification of share through email moved to background job #217
Conversation
sharidas
force-pushed
the
email-notification-background
branch
from
adbbc02 to
f575504
Compare
sharidas
force-pushed
the
email-notification-background
branch
from
f575504 to
dde984e
Compare
Codecov Report
@@ Coverage Diff @@
## master #217 +/- ##
============================================
- Coverage 89.55% 89.07% -0.48%
- Complexity 134 161 +27
============================================
Files 18 19 +1
Lines 536 604 +68
============================================
+ Hits 480 538 +58
- Misses 56 66 +10
Continue to review full report at Codecov.
|
It is better to move the notification of share using email to background job. This prevents timeout or long time wait to notify the user(s). Signed-off-by: Sujith H <[email protected]>
sharidas
force-pushed
the
email-notification-background
branch
from
dde984e to
a3aae6b
Compare
|
Do we want to move all these notifications to the background job? I expected to use the background job only for big groups... |
@jvillafanez Only email notifications are moved to background jobs. When user tries to share a file/folder to a group/user the notification to the user(s) would be received normally ( as that part is not touched ). |
| * @param string $shareFullId | ||
| * @return null | ||
| */ | ||
| public function emailNotify($shareFullId) { |
| $this->mailer = $mailer ? $mailer : \OC::$server->getMailer(); | ||
| $this->config = $config ? $config : \OC::$server->getConfig(); | ||
|
|
||
| $optionsStorage = new OptionsStorage($this->config); |
| * @param $notificationURL | ||
| * @return string | ||
| */ | ||
| public function fixURL($notificationURL) { |
| * | ||
| * @package OCA\Notifications\BackgroundJob | ||
| */ | ||
| class MailNotificationSender extends Job { |
There was a problem hiding this comment.
either extend the (private) QueuedJob class or implement the public interface.
| /** | ||
| * @param $shareFullId | ||
| */ | ||
| public function sendNotify($shareFullId) { |
There was a problem hiding this comment.
unless there is a reason to call this method from the outside, keep the method private
| $users = []; | ||
| if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) { | ||
| //Notify all the group members | ||
| $group = $this->groupManager->get($share->getSharedWith()); |
There was a problem hiding this comment.
what if the group doesn't exist? it might have been deleted after the notification was generated
| $users = $group->getUsers(); | ||
| } elseif ($share->getShareType() === \OCP\Share::SHARE_TYPE_USER) { | ||
| $users[] = $this->userManager->get($share->getSharedWith()); | ||
| } |
There was a problem hiding this comment.
Abort the function if it doesn't match any of those share types? This way is clearer that we are handling this case.
|
|
||
| foreach ($users as $user) { | ||
| if ($user->getEMailAddress() === null) { | ||
| \OC::$server->getLogger()->warning("Mail notification can not be sent to " . $user->getUID() . " for share " . $share->getName() . " as email for user is not set"); |
| $acceptAction->setLink($fileLink, 'POST'); | ||
| $notification->addAction($acceptAction); | ||
|
|
||
| if (\count($notificationList) < $maxCountSendNotification) { |
There was a problem hiding this comment.
does it make a difference to send the emails like this? It took a while to understand what you're doing here. Sending the emails one by one would be easier, so unless there is a real advantage I'd prefer to go through the easy route.
| } else { | ||
| $this->sendNotify($this->getArgument()['shareFullId']); | ||
| } | ||
| \OC::$server->getJobList()->removeById($this->getId()); |
There was a problem hiding this comment.
As said, better use the QueuedJob and let it handle this by itself.
It is better to move the notification of share using
email to background job. This prevents timeout or long
time wait to notify the user(s).
Signed-off-by: Sujith H [email protected]