[WIP] notification of share through email moved to background job by sharidas · Pull Request #217 · owncloud/notifications

sharidas · 2018-07-30T14:14:52Z

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 sharidasan@owncloud.com

codecov-io · 2018-07-31T06:02:07Z

Codecov Report

Merging #217 into master will decrease coverage by 0.47%.
The diff coverage is 85.71%.

@@             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

Flag	Coverage Δ	Complexity Δ
#phpunit	`89.07% <85.71%> (-0.48%)`	`161 <28> (+27)`

Impacted Files	Coverage Δ	Complexity Δ
lib/App.php	`100% <100%> (ø)`	`5 <2> (+1)`	⬆️
lib/BackgroundJob/MailNotificationSender.php	`84.37% <84.37%> (ø)`	`26 <26> (?)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00abb1d...a3aae6b. Read the comment docs.

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 <sharidasan@owncloud.com>

jvillafanez · 2018-07-31T07:02:08Z

Do we want to move all these notifications to the background job? I expected to use the background job only for big groups...

sharidas · 2018-07-31T07:19:15Z

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 ).

jvillafanez · 2018-07-31T07:02:46Z

+	 * @param string $shareFullId
+	 * @return null
+	 */
+	public function emailNotify($shareFullId) {


currently unused

jvillafanez · 2018-07-31T07:03:40Z

+		$this->mailer = $mailer ? $mailer : \OC::$server->getMailer();
+		$this->config = $config ? $config : \OC::$server->getConfig();
+
+		$optionsStorage = new OptionsStorage($this->config);


better rely on DI

jvillafanez · 2018-07-31T07:04:06Z

+	 * @param $notificationURL
+	 * @return string
+	 */
+	public function fixURL($notificationURL) {


why is this needed?

jvillafanez · 2018-07-31T07:09:02Z

+ *
+ * @package OCA\Notifications\BackgroundJob
+ */
+class MailNotificationSender extends Job {


either extend the (private) QueuedJob class or implement the public interface.

jvillafanez · 2018-07-31T07:09:47Z

+	/**
+	 * @param $shareFullId
+	 */
+	public function sendNotify($shareFullId) {


unless there is a reason to call this method from the outside, keep the method private

jvillafanez · 2018-07-31T07:10:21Z

+		$users = [];
+		if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) {
+			//Notify all the group members
+			$group = $this->groupManager->get($share->getSharedWith());


what if the group doesn't exist? it might have been deleted after the notification was generated

jvillafanez · 2018-07-31T07:15:28Z

+			$users = $group->getUsers();
+		} elseif ($share->getShareType() === \OCP\Share::SHARE_TYPE_USER) {
+			$users[] = $this->userManager->get($share->getSharedWith());
+		}


Abort the function if it doesn't match any of those share types? This way is clearer that we are handling this case.

jvillafanez · 2018-07-31T07:15:55Z

+
+		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");


Use the injected logger

jvillafanez · 2018-07-31T07:32:17Z

+			$acceptAction->setLink($fileLink, 'POST');
+			$notification->addAction($acceptAction);
+
+			if (\count($notificationList) < $maxCountSendNotification) {


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.

jvillafanez · 2018-07-31T07:33:27Z

+		} else {
+			$this->sendNotify($this->getArgument()['shareFullId']);
+		}
+		\OC::$server->getJobList()->removeById($this->getId());


As said, better use the QueuedJob and let it handle this by itself.

CLAassistant · 2026-04-13T10:12:01Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

sharidas self-assigned this Jul 30, 2018

sharidas added the 2 - Developing label Jul 30, 2018

sharidas added this to the development milestone Jul 30, 2018

sharidas force-pushed the email-notification-background branch from adbbc02 to f575504 Compare July 30, 2018 16:27

sharidas mentioned this pull request Jul 30, 2018

[WIP] email share notification should be done in the background owncloud/core#32125

Closed

11 tasks

sharidas force-pushed the email-notification-background branch from f575504 to dde984e Compare July 31, 2018 06:02

notification of share through email moved to background job

a3aae6b

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 <sharidasan@owncloud.com>

sharidas force-pushed the email-notification-background branch from dde984e to a3aae6b Compare July 31, 2018 06:45

jvillafanez reviewed Jul 31, 2018

View reviewed changes

PVince81 modified the milestones: development, backlog Aug 21, 2018

PVince81 assigned sharidasan and unassigned sharidas Oct 23, 2018

Conversation

sharidas commented Jul 30, 2018

Uh oh!

codecov-io commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jvillafanez commented Jul 31, 2018

Uh oh!

sharidas commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jul 31, 2018 •

edited

Loading