From 5e4a166365a601bf0133a97fda968a85744dfef5 Mon Sep 17 00:00:00 2001 From: Stefan Cherniakov Date: Thu, 22 Aug 2024 06:43:43 +0200 Subject: feat(files_sharing): reminder for link shares with expiration date Signed-off-by: Stefan Cherniakov --- apps/files_sharing/appinfo/info.xml | 3 +- .../composer/composer/autoload_classmap.php | 2 + .../composer/composer/autoload_static.php | 2 + .../Migration/Version31000Date20240821142813.php | 36 +++ apps/files_sharing/lib/SharesReminderJob.php | 261 +++++++++++++++++++++ apps/files_sharing/tests/SharesReminderJobTest.php | 196 ++++++++++++++++ 6 files changed, 499 insertions(+), 1 deletion(-) create mode 100644 apps/files_sharing/lib/Migration/Version31000Date20240821142813.php create mode 100644 apps/files_sharing/lib/SharesReminderJob.php create mode 100644 apps/files_sharing/tests/SharesReminderJobTest.php (limited to 'apps/files_sharing') diff --git a/apps/files_sharing/appinfo/info.xml b/apps/files_sharing/appinfo/info.xml index 6979893d7e6..dcf6621b183 100644 --- a/apps/files_sharing/appinfo/info.xml +++ b/apps/files_sharing/appinfo/info.xml @@ -14,7 +14,7 @@ Turning the feature off removes shared files and folders on the server for all share recipients, and also on the sync clients and mobile apps. More information is available in the Nextcloud Documentation. - 1.23.0 + 1.23.1 agpl Michael Gapczynski Bjoern Schiessle @@ -33,6 +33,7 @@ Turning the feature off removes shared files and folders on the server for all s OCA\Files_Sharing\DeleteOrphanedSharesJob OCA\Files_Sharing\ExpireSharesJob + OCA\Files_Sharing\SharesReminderJob OCA\Files_Sharing\BackgroundJob\FederatedSharesDiscoverJob diff --git a/apps/files_sharing/composer/composer/autoload_classmap.php b/apps/files_sharing/composer/composer/autoload_classmap.php index e4aa9c7b16a..e3c94a7ac1a 100644 --- a/apps/files_sharing/composer/composer/autoload_classmap.php +++ b/apps/files_sharing/composer/composer/autoload_classmap.php @@ -76,6 +76,7 @@ return array( 'OCA\\Files_Sharing\\Migration\\Version22000Date20210216084241' => $baseDir . '/../lib/Migration/Version22000Date20210216084241.php', 'OCA\\Files_Sharing\\Migration\\Version24000Date20220208195521' => $baseDir . '/../lib/Migration/Version24000Date20220208195521.php', 'OCA\\Files_Sharing\\Migration\\Version24000Date20220404142216' => $baseDir . '/../lib/Migration/Version24000Date20220404142216.php', + 'OCA\\Files_Sharing\\Migration\\Version31000Date20240821142813' => $baseDir . '/../lib/Migration/Version31000Date20240821142813.php', 'OCA\\Files_Sharing\\MountProvider' => $baseDir . '/../lib/MountProvider.php', 'OCA\\Files_Sharing\\Notification\\Listener' => $baseDir . '/../lib/Notification/Listener.php', 'OCA\\Files_Sharing\\Notification\\Notifier' => $baseDir . '/../lib/Notification/Notifier.php', @@ -87,6 +88,7 @@ return array( 'OCA\\Files_Sharing\\ShareBackend\\Folder' => $baseDir . '/../lib/ShareBackend/Folder.php', 'OCA\\Files_Sharing\\SharedMount' => $baseDir . '/../lib/SharedMount.php', 'OCA\\Files_Sharing\\SharedStorage' => $baseDir . '/../lib/SharedStorage.php', + 'OCA\\Files_Sharing\\SharesReminderJob' => $baseDir . '/../lib/SharesReminderJob.php', 'OCA\\Files_Sharing\\Updater' => $baseDir . '/../lib/Updater.php', 'OCA\\Files_Sharing\\ViewOnly' => $baseDir . '/../lib/ViewOnly.php', ); diff --git a/apps/files_sharing/composer/composer/autoload_static.php b/apps/files_sharing/composer/composer/autoload_static.php index 36a2622f56e..597e65d96a2 100644 --- a/apps/files_sharing/composer/composer/autoload_static.php +++ b/apps/files_sharing/composer/composer/autoload_static.php @@ -91,6 +91,7 @@ class ComposerStaticInitFiles_Sharing 'OCA\\Files_Sharing\\Migration\\Version22000Date20210216084241' => __DIR__ . '/..' . '/../lib/Migration/Version22000Date20210216084241.php', 'OCA\\Files_Sharing\\Migration\\Version24000Date20220208195521' => __DIR__ . '/..' . '/../lib/Migration/Version24000Date20220208195521.php', 'OCA\\Files_Sharing\\Migration\\Version24000Date20220404142216' => __DIR__ . '/..' . '/../lib/Migration/Version24000Date20220404142216.php', + 'OCA\\Files_Sharing\\Migration\\Version31000Date20240821142813' => __DIR__ . '/..' . '/../lib/Migration/Version31000Date20240821142813.php', 'OCA\\Files_Sharing\\MountProvider' => __DIR__ . '/..' . '/../lib/MountProvider.php', 'OCA\\Files_Sharing\\Notification\\Listener' => __DIR__ . '/..' . '/../lib/Notification/Listener.php', 'OCA\\Files_Sharing\\Notification\\Notifier' => __DIR__ . '/..' . '/../lib/Notification/Notifier.php', @@ -102,6 +103,7 @@ class ComposerStaticInitFiles_Sharing 'OCA\\Files_Sharing\\ShareBackend\\Folder' => __DIR__ . '/..' . '/../lib/ShareBackend/Folder.php', 'OCA\\Files_Sharing\\SharedMount' => __DIR__ . '/..' . '/../lib/SharedMount.php', 'OCA\\Files_Sharing\\SharedStorage' => __DIR__ . '/..' . '/../lib/SharedStorage.php', + 'OCA\\Files_Sharing\\SharesReminderJob' => __DIR__ . '/..' . '/../lib/SharesReminderJob.php', 'OCA\\Files_Sharing\\Updater' => __DIR__ . '/..' . '/../lib/Updater.php', 'OCA\\Files_Sharing\\ViewOnly' => __DIR__ . '/..' . '/../lib/ViewOnly.php', ); diff --git a/apps/files_sharing/lib/Migration/Version31000Date20240821142813.php b/apps/files_sharing/lib/Migration/Version31000Date20240821142813.php new file mode 100644 index 00000000000..b063902a380 --- /dev/null +++ b/apps/files_sharing/lib/Migration/Version31000Date20240821142813.php @@ -0,0 +1,36 @@ +getTable('share'); + $table->addColumn('reminder_sent', Types::BOOLEAN, [ + 'notnull' => false, + 'default' => false, + ]); + return $schema; + } + +} diff --git a/apps/files_sharing/lib/SharesReminderJob.php b/apps/files_sharing/lib/SharesReminderJob.php new file mode 100644 index 00000000000..214e555f173 --- /dev/null +++ b/apps/files_sharing/lib/SharesReminderJob.php @@ -0,0 +1,261 @@ +setInterval(3600); + } + + + /** + * Makes the background job do its work + * + * @param array $argument unused argument + * @throws Exception if a database error occurs + */ + public function run(mixed $argument): void { + $shares = $this->getShares(); + [$foldersByEmail, $langByEmail] = $this->prepareReminders($shares); + $this->sendReminders($foldersByEmail, $langByEmail); + } + + /** + * Finds all folder shares of type user or email with expiration dates within the specified timeframe. + * This method returns only those shares that have not yet received the reminder. + * + * @return array + * @throws Exception if a database error occurs + */ + private function getShares(): array { + $minDate = new \DateTime(); + $maxDate = new \DateTime(); + $maxDate->setTimestamp($maxDate->getTimestamp() + self::SECONDS_BEFORE_REMINDER); + + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'share_type') + ->from('share') + ->where( + $qb->expr()->andX( + $qb->expr()->orX( + $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_USER)), + $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_EMAIL)) + ), + $qb->expr()->eq('item_type', $qb->expr()->literal('folder')), + $qb->expr()->gte('expiration', $qb->createNamedParameter($minDate->format('Y-m-d H:i:s'))), + $qb->expr()->lt('expiration', $qb->createNamedParameter($maxDate->format('Y-m-d H:i:s'))), + $qb->expr()->eq('reminder_sent', $qb->createNamedParameter( + false, IQueryBuilder::PARAM_BOOL + )) + ) + ); + + $sharesResult = $qb->executeQuery(); + $shares = []; + while ($share = $sharesResult->fetch()) { + if ((int)$share['share_type'] === IShare::TYPE_EMAIL) { + $id = "ocMailShare:$share[id]"; + } else { + $id = "ocinternal:$share[id]"; + } + + try { + $shares[] = $this->shareManager->getShareById($id); + } catch (ShareNotFound) { + $this->logger->error("Share with ID $id not found."); + } + } + $sharesResult->closeCursor(); + return $shares; + } + + /** + * Checks if the user should be reminded about this share. + * If so, it will retrieve and return all the necessary data for this. + * It also updates the reminder sent flag for the affected shares (to avoid multiple reminders). + * + * @param array $shares Shares that were obtained with {@link getShares} + * @return array A tuple consisting of two dictionaries: folders and languages by email + * @throws Exception if the reminder sent flag could not be saved + */ + private function prepareReminders(array $shares): array { + // This dictionary stores email addresses as keys and folder lists as values. + // It is used to ensure that each user receives no more than one email notification. + // The email will include the names and links of the folders that the user should be reminded of. + $foldersByEmail = []; + // Similar to the previous one, this variable stores the language for each email (if provided) + $langByEmail = []; + + /** @var IShare $share */ + foreach ($shares as $share) { + if (!$this->shouldRemindOfThisShare($share)) { + continue; + } + + $sharedWith = $share->getSharedWith(); + if ($share->getShareType() == IShare::TYPE_USER) { + $user = $this->userManager->get($sharedWith); + $mailTo = $user->getEMailAddress(); + $lang = $this->l10nFactory->getUserLanguage($user); + $link = $this->urlGenerator->linkToRouteAbsolute('files.view.index', [ + 'dir' => $share->getTarget() + ]); + } else { + $mailTo = $sharedWith; + $lang = ''; + $link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare', [ + 'token' => $share->getToken() + ]); + } + if (empty($mailTo)) { + continue; + } + + if (!empty($lang)) { + $langByEmail[$mailTo] ??= $lang; + } + if (!isset($foldersByEmail[$mailTo])) { + $foldersByEmail[$mailTo] = []; + } + $foldersByEmail[$mailTo][] = ['name' => $share->getNode()->getName(), 'link' => $link]; + + $share->setReminderSent(true); + $qb = $this->db->getQueryBuilder(); + $qb->update('share') + ->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId()))) + ->set('reminder_sent', $qb->createNamedParameter($share->getReminderSent())) + ->execute(); + } + + return [$foldersByEmail, $langByEmail]; + } + + /** + * Checks if user has write permission and folder is empty + * + * @param IShare $share Share to check + * @return bool + */ + private function shouldRemindOfThisShare(IShare $share): bool { + try { + $folder = $share->getNode(); + $fileCount = count($folder->getDirectoryListing()); + } catch (NotFoundException) { + $id = $share->getFullId(); + $this->logger->debug("File by share ID $id not found."); + return false; + } + $permissions = $share->getPermissions(); + $hasCreatePermission = ($permissions & Constants::PERMISSION_CREATE) === Constants::PERMISSION_CREATE; + return ($fileCount == 0 && $hasCreatePermission); + } + + /** + * This method accepts data obtained by {@link prepareReminders} and sends reminder emails. + * + * @param array $foldersByEmail + * @param array $langByEmail + * @return void + */ + private function sendReminders(array $foldersByEmail, array $langByEmail): void { + $instanceName = $this->defaults->getName(); + $from = [Util::getDefaultEmailAddress($instanceName) => $instanceName]; + foreach ($foldersByEmail as $email => $folders) { + $l = $this->l10nFactory->get('files_sharing', $langByEmail[$email] ?? null); + $emailTemplate = $this->generateEMailTemplate($l, $folders); + $message = $this->mailer->createMessage(); + $message->setFrom($from); + $message->setTo([$email]); + $message->useTemplate($emailTemplate); + $errorText = "Sending email with share reminder to $email failed."; + try { + $failedRecipients = $this->mailer->send($message); + if (count($failedRecipients) > 0) { + $this->logger->error($errorText); + } + } catch (\Exception) { + $this->logger->error($errorText); + } + } + } + + /** + * Returns the reminder email template + * + * @param IL10N $l + * @param array $folders Folders the user should be reminded of + * @return IEMailTemplate + */ + private function generateEMailTemplate(IL10N $l, array $folders): IEMailTemplate { + $emailTemplate = $this->mailer->createEMailTemplate('files_sharing.SharesReminder', [ + 'folders' => $folders, + ]); + + $emailTemplate->addHeader(); + if (count($folders) == 1) { + $emailTemplate->setSubject( + $l->t('Remember to upload the files to %s', [$folders[0]['name']]) + ); + $emailTemplate->addBodyText($l->t( + 'We would like to kindly remind you that you have not yet uploaded any files to the shared folder.' + )); + } else { + $emailTemplate->setSubject( + $l->t('Remember to upload the files to shared folders') + ); + $emailTemplate->addBodyText($l->t( + 'We would like to kindly remind you that you have not yet uploaded any files to the shared folders.' + )); + } + + foreach ($folders as $folder) { + $emailTemplate->addBodyButton( + $l->t('Open "%s"', [$folder['name']]), + $folder['link'] + ); + } + $emailTemplate->addFooter(); + return $emailTemplate; + } +} diff --git a/apps/files_sharing/tests/SharesReminderJobTest.php b/apps/files_sharing/tests/SharesReminderJobTest.php new file mode 100644 index 00000000000..d35d88786f2 --- /dev/null +++ b/apps/files_sharing/tests/SharesReminderJobTest.php @@ -0,0 +1,196 @@ +db = \OC::$server->get(IDBConnection::class); + $this->shareManager = \OC::$server->get(IManager::class); + $this->userManager = \OC::$server->get(IUserManager::class); + $this->mailer = $this->createMock(IMailer::class); + + // Clear occasional leftover shares from other tests + $this->db->executeUpdate('DELETE FROM `*PREFIX*share`'); + + $this->user1 = $this->getUniqueID('user1_'); + $this->user2 = $this->getUniqueID('user2_'); + + $user1 = $this->userManager->createUser($this->user1, 'longrandompassword'); + $user2 = $this->userManager->createUser($this->user2, 'longrandompassword'); + $user1->setSystemEMailAddress('user1@test.com'); + $user2->setSystemEMailAddress('user2@test.com'); + + \OC::registerShareHooks(\OC::$server->get(SystemConfig::class)); + + $this->job = new SharesReminderJob( + \OC::$server->get(ITimeFactory::class), + $this->db, + \OC::$server->get(IManager::class), + $this->userManager, + \OC::$server->get(LoggerInterface::class), + \OC::$server->get(IURLGenerator::class), + \OC::$server->get(IFactory::class), + $this->mailer, + \OC::$server->get(Defaults::class), + ); + } + + protected function tearDown(): void { + $this->db->executeUpdate('DELETE FROM `*PREFIX*share`'); + + $userManager = \OC::$server->get(IUserManager::class); + $user1 = $userManager->get($this->user1); + if ($user1) { + $user1->delete(); + } + $user2 = $userManager->get($this->user2); + if ($user2) { + $user2->delete(); + } + + $this->logout(); + + parent::tearDown(); + } + + public function dataSharesReminder() { + $someMail = 'test@test.com'; + $noExpirationDate = null; + $today = new \DateTime(); + // For expiration dates, the time is always automatically set to zero by ShareAPIController + $today->setTime(0, 0); + $nearFuture = new \DateTime(); + $nearFuture->setTimestamp($today->getTimestamp() + 86400 * 1); + $farFuture = new \DateTime(); + $farFuture->setTimestamp($today->getTimestamp() + 86400 * 2); + $permissionRead = Constants::PERMISSION_READ; + $permissionCreate = $permissionRead | Constants::PERMISSION_CREATE; + $permissionUpdate = $permissionRead | Constants::PERMISSION_UPDATE; + $permissionDelete = $permissionRead | Constants::PERMISSION_DELETE; + $permissionAll = Constants::PERMISSION_ALL; + + return [ + // No reminders for folders without expiration date + [$noExpirationDate, '', false, $permissionRead, false], + [$noExpirationDate, '', false, $permissionCreate, false], + [$noExpirationDate, '', true, $permissionDelete, false], + [$noExpirationDate, '', true, $permissionCreate, false], + [$noExpirationDate, $someMail, false, $permissionUpdate, false], + [$noExpirationDate, $someMail, false, $permissionCreate, false], + [$noExpirationDate, $someMail, true, $permissionRead, false], + [$noExpirationDate, $someMail, true, $permissionAll, false], + // No reminders for folders with expiration date in the far future + [$farFuture, '', false, $permissionRead, false], + [$farFuture, '', false, $permissionCreate, false], + [$farFuture, '', true, $permissionDelete, false], + [$farFuture, '', true, $permissionCreate, false], + [$farFuture, $someMail, false, $permissionUpdate, false], + [$farFuture, $someMail, false, $permissionCreate, false], + [$farFuture, $someMail, true, $permissionRead, false], + [$farFuture, $someMail, true, $permissionAll, false], + /* Should send reminders for folders with expiration date in the near future + if the folder is empty and the user has write permission */ + [$nearFuture, '', false, $permissionRead, false], + [$nearFuture, '', false, $permissionCreate, false], + [$nearFuture, '', true, $permissionDelete, false], + [$nearFuture, '', true, $permissionCreate, true], + [$nearFuture, $someMail, false, $permissionUpdate, false], + [$nearFuture, $someMail, false, $permissionCreate, false], + [$nearFuture, $someMail, true, $permissionRead, false], + [$nearFuture, $someMail, true, $permissionAll, true], + ]; + } + + /** + * @dataProvider dataSharesReminder + * + * @param \DateTime|null $expirationDate Share expiration date + * @param string $email Share with this email. If empty, the share is of type TYPE_USER and the sharee is user2 + * @param bool $isEmpty Is share folder empty? + * @param int $permissions + * @param bool $shouldBeReminded + */ + public function testSharesReminder( + \DateTime|null $expirationDate, string $email, bool $isEmpty, int $permissions, bool $shouldBeReminded + ): void { + $this->loginAsUser($this->user1); + + $user1Folder = \OC::$server->get(IRootFolder::class)->getUserFolder($this->user1); + $testFolder = $user1Folder->newFolder('test'); + + if (!$isEmpty) { + $testFolder->newFile("some_file.txt"); + } + + $share = $this->shareManager->newShare(); + + $share->setNode($testFolder) + ->setShareType(($email ? IShare::TYPE_EMAIL : IShare::TYPE_USER)) + ->setPermissions($permissions) + ->setSharedBy($this->user1) + ->setSharedWith(($email ?: $this->user2)) + ->setExpirationDate($expirationDate); + $share = $this->shareManager->createShare($share); + + $this->logout(); + $messageMock = $this->createMock(IMessage::class); + $this->mailer->method('createMessage')->willReturn($messageMock); + $this->mailer + ->expects(($shouldBeReminded ? $this->once() : $this->never())) + ->method('send') + ->with($messageMock); + $messageMock + ->expects(($shouldBeReminded ? $this->once() : $this->never())) + ->method('setTo') + ->with([$email ?: $this->userManager->get($this->user2)->getSystemEMailAddress()]); + $this->assertSame(false, $share->getReminderSent()); + $this->job->run([]); + $qb = $this->db->getQueryBuilder(); + $reminderSent = $qb + ->select('reminder_sent') + ->from('share') + ->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId()))) + ->executeQuery() + ->fetch()["reminder_sent"]; + $this->assertEquals($shouldBeReminded, $reminderSent); + } +} -- cgit v1.2.3 From d633b9bce641846c3925613c04c6e6489c008248 Mon Sep 17 00:00:00 2001 From: Stefan Cherniakov Date: Tue, 27 Aug 2024 09:29:21 +0200 Subject: fix(files_sharing): Make share reminders more stable & fix issues Signed-off-by: Stefan Cherniakov --- apps/files_sharing/lib/SharesReminderJob.php | 218 ++++++++------------- apps/files_sharing/tests/SharesReminderJobTest.php | 10 +- apps/sharebymail/lib/ShareByMailProvider.php | 4 +- lib/private/Share20/DefaultShareProvider.php | 6 +- lib/private/Share20/Share.php | 3 +- 5 files changed, 96 insertions(+), 145 deletions(-) (limited to 'apps/files_sharing') diff --git a/apps/files_sharing/lib/SharesReminderJob.php b/apps/files_sharing/lib/SharesReminderJob.php index 214e555f173..4b19ee72d18 100644 --- a/apps/files_sharing/lib/SharesReminderJob.php +++ b/apps/files_sharing/lib/SharesReminderJob.php @@ -32,6 +32,7 @@ use Psr\Log\LoggerInterface; */ class SharesReminderJob extends TimedJob { private const SECONDS_BEFORE_REMINDER = 86400; + private const CHUNK_SIZE = 1000; public function __construct( ITimeFactory $time, @@ -56,43 +57,51 @@ class SharesReminderJob extends TimedJob { * @throws Exception if a database error occurs */ public function run(mixed $argument): void { - $shares = $this->getShares(); - [$foldersByEmail, $langByEmail] = $this->prepareReminders($shares); - $this->sendReminders($foldersByEmail, $langByEmail); + foreach ($this->getShares() as $share) { + $reminderInfo = $this->prepareReminder($share); + $this->sendReminder($reminderInfo); + } } /** - * Finds all folder shares of type user or email with expiration dates within the specified timeframe. - * This method returns only those shares that have not yet received the reminder. + * Finds all shares of empty folders, for which the user has write permissions. + * The returned shares are of type user or email only, have expiration dates within the specified time frame + * and have not yet received a reminder. * - * @return array + * @return array|\Iterator * @throws Exception if a database error occurs */ - private function getShares(): array { + private function getShares(): array|\Iterator { $minDate = new \DateTime(); $maxDate = new \DateTime(); $maxDate->setTimestamp($maxDate->getTimestamp() + self::SECONDS_BEFORE_REMINDER); $qb = $this->db->getQueryBuilder(); - $qb->select('id', 'share_type') - ->from('share') + $qb->select('s.id', 's.share_type') + ->from('share', 's') + ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('f.parent', 's.file_source')) ->where( $qb->expr()->andX( $qb->expr()->orX( - $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_USER)), - $qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_EMAIL)) + $qb->expr()->eq('s.share_type', $qb->expr()->literal(IShare::TYPE_USER)), + $qb->expr()->eq('s.share_type', $qb->expr()->literal(IShare::TYPE_EMAIL)) ), - $qb->expr()->eq('item_type', $qb->expr()->literal('folder')), - $qb->expr()->gte('expiration', $qb->createNamedParameter($minDate->format('Y-m-d H:i:s'))), - $qb->expr()->lt('expiration', $qb->createNamedParameter($maxDate->format('Y-m-d H:i:s'))), - $qb->expr()->eq('reminder_sent', $qb->createNamedParameter( + $qb->expr()->eq('s.item_type', $qb->expr()->literal('folder')), + $qb->expr()->gte('s.expiration', $qb->createNamedParameter($minDate->format('Y-m-d H:i:s'))), + $qb->expr()->lt('s.expiration', $qb->createNamedParameter($maxDate->format('Y-m-d H:i:s'))), + $qb->expr()->eq('s.reminder_sent', $qb->createNamedParameter( false, IQueryBuilder::PARAM_BOOL - )) + )), + $qb->expr()->eq( + $qb->expr()->bitwiseAnd('s.permissions', Constants::PERMISSION_CREATE), + $qb->createNamedParameter(Constants::PERMISSION_CREATE, IQueryBuilder::PARAM_INT) + ), + $qb->expr()->isNull('f.fileid') ) - ); + ) + ->setMaxResults(SharesReminderJob::CHUNK_SIZE); $sharesResult = $qb->executeQuery(); - $shares = []; while ($share = $sharesResult->fetch()) { if ((int)$share['share_type'] === IShare::TYPE_EMAIL) { $id = "ocMailShare:$share[id]"; @@ -101,122 +110,78 @@ class SharesReminderJob extends TimedJob { } try { - $shares[] = $this->shareManager->getShareById($id); + yield $this->shareManager->getShareById($id); } catch (ShareNotFound) { $this->logger->error("Share with ID $id not found."); } } $sharesResult->closeCursor(); - return $shares; } /** - * Checks if the user should be reminded about this share. - * If so, it will retrieve and return all the necessary data for this. + * Retrieves and returns all the necessary data before sending a reminder. * It also updates the reminder sent flag for the affected shares (to avoid multiple reminders). * - * @param array $shares Shares that were obtained with {@link getShares} - * @return array A tuple consisting of two dictionaries: folders and languages by email - * @throws Exception if the reminder sent flag could not be saved + * @param IShare $share Share that was obtained with {@link getShares} + * @return array|null Info needed to send a reminder */ - private function prepareReminders(array $shares): array { - // This dictionary stores email addresses as keys and folder lists as values. - // It is used to ensure that each user receives no more than one email notification. - // The email will include the names and links of the folders that the user should be reminded of. - $foldersByEmail = []; - // Similar to the previous one, this variable stores the language for each email (if provided) - $langByEmail = []; - - /** @var IShare $share */ - foreach ($shares as $share) { - if (!$this->shouldRemindOfThisShare($share)) { - continue; - } - - $sharedWith = $share->getSharedWith(); - if ($share->getShareType() == IShare::TYPE_USER) { - $user = $this->userManager->get($sharedWith); - $mailTo = $user->getEMailAddress(); - $lang = $this->l10nFactory->getUserLanguage($user); - $link = $this->urlGenerator->linkToRouteAbsolute('files.view.index', [ - 'dir' => $share->getTarget() - ]); - } else { - $mailTo = $sharedWith; - $lang = ''; - $link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare', [ - 'token' => $share->getToken() - ]); - } - if (empty($mailTo)) { - continue; - } - - if (!empty($lang)) { - $langByEmail[$mailTo] ??= $lang; - } - if (!isset($foldersByEmail[$mailTo])) { - $foldersByEmail[$mailTo] = []; - } - $foldersByEmail[$mailTo][] = ['name' => $share->getNode()->getName(), 'link' => $link]; - - $share->setReminderSent(true); - $qb = $this->db->getQueryBuilder(); - $qb->update('share') - ->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId()))) - ->set('reminder_sent', $qb->createNamedParameter($share->getReminderSent())) - ->execute(); + private function prepareReminder(IShare $share): array|null { + $sharedWith = $share->getSharedWith(); + $reminderInfo = []; + if ($share->getShareType() == IShare::TYPE_USER) { + $user = $this->userManager->get($sharedWith); + $reminderInfo['email'] = $user->getEMailAddress(); + $reminderInfo['userLang'] = $this->l10nFactory->getUserLanguage($user); + $reminderInfo['folderLink'] = $this->urlGenerator->linkToRouteAbsolute('files.view.index', [ + 'dir' => $share->getTarget() + ]); + } else { + $reminderInfo['email'] = $sharedWith; + $reminderInfo['folderLink'] = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare', [ + 'token' => $share->getToken() + ]); + } + if (empty($reminderInfo['email'])) { + return null; } - return [$foldersByEmail, $langByEmail]; - } - - /** - * Checks if user has write permission and folder is empty - * - * @param IShare $share Share to check - * @return bool - */ - private function shouldRemindOfThisShare(IShare $share): bool { try { - $folder = $share->getNode(); - $fileCount = count($folder->getDirectoryListing()); + $reminderInfo['folderName'] = $share->getNode()->getName(); } catch (NotFoundException) { $id = $share->getFullId(); - $this->logger->debug("File by share ID $id not found."); - return false; + $this->logger->error("File by share ID $id not found."); } - $permissions = $share->getPermissions(); - $hasCreatePermission = ($permissions & Constants::PERMISSION_CREATE) === Constants::PERMISSION_CREATE; - return ($fileCount == 0 && $hasCreatePermission); + $share->setReminderSent(true); + $this->shareManager->updateShare($share); + return $reminderInfo; } /** - * This method accepts data obtained by {@link prepareReminders} and sends reminder emails. + * This method accepts data obtained by {@link prepareReminder} and sends reminder email. * - * @param array $foldersByEmail - * @param array $langByEmail + * @param array $reminderInfo * @return void */ - private function sendReminders(array $foldersByEmail, array $langByEmail): void { + private function sendReminder(array $reminderInfo): void { $instanceName = $this->defaults->getName(); $from = [Util::getDefaultEmailAddress($instanceName) => $instanceName]; - foreach ($foldersByEmail as $email => $folders) { - $l = $this->l10nFactory->get('files_sharing', $langByEmail[$email] ?? null); - $emailTemplate = $this->generateEMailTemplate($l, $folders); - $message = $this->mailer->createMessage(); - $message->setFrom($from); - $message->setTo([$email]); - $message->useTemplate($emailTemplate); - $errorText = "Sending email with share reminder to $email failed."; - try { - $failedRecipients = $this->mailer->send($message); - if (count($failedRecipients) > 0) { - $this->logger->error($errorText); - } - } catch (\Exception) { + $l = $this->l10nFactory->get('files_sharing', $reminderInfo['userLang'] ?? null); + $emailTemplate = $this->generateEMailTemplate($l, [ + 'link' => $reminderInfo['folderLink'], 'name' => $reminderInfo['folderName'] + ]); + + $message = $this->mailer->createMessage(); + $message->setFrom($from); + $message->setTo([$reminderInfo['email']]); + $message->useTemplate($emailTemplate); + $errorText = "Sending email with share reminder to $reminderInfo[email] failed."; + try { + $failedRecipients = $this->mailer->send($message); + if (count($failedRecipients) > 0) { $this->logger->error($errorText); } + } catch (\Exception) { + $this->logger->error($errorText); } } @@ -224,37 +189,24 @@ class SharesReminderJob extends TimedJob { * Returns the reminder email template * * @param IL10N $l - * @param array $folders Folders the user should be reminded of + * @param array $folder Folder the user should be reminded of * @return IEMailTemplate */ - private function generateEMailTemplate(IL10N $l, array $folders): IEMailTemplate { + private function generateEMailTemplate(IL10N $l, array $folder): IEMailTemplate { $emailTemplate = $this->mailer->createEMailTemplate('files_sharing.SharesReminder', [ - 'folders' => $folders, + 'folder' => $folder, ]); - $emailTemplate->addHeader(); - if (count($folders) == 1) { - $emailTemplate->setSubject( - $l->t('Remember to upload the files to %s', [$folders[0]['name']]) - ); - $emailTemplate->addBodyText($l->t( - 'We would like to kindly remind you that you have not yet uploaded any files to the shared folder.' - )); - } else { - $emailTemplate->setSubject( - $l->t('Remember to upload the files to shared folders') - ); - $emailTemplate->addBodyText($l->t( - 'We would like to kindly remind you that you have not yet uploaded any files to the shared folders.' - )); - } - - foreach ($folders as $folder) { - $emailTemplate->addBodyButton( - $l->t('Open "%s"', [$folder['name']]), - $folder['link'] - ); - } + $emailTemplate->setSubject( + $l->t('Remember to upload the files to %s', [$folder['name']]) + ); + $emailTemplate->addBodyText($l->t( + 'We would like to kindly remind you that you have not yet uploaded any files to the shared folder.' + )); + $emailTemplate->addBodyButton( + $l->t('Open "%s"', [$folder['name']]), + $folder['link'] + ); $emailTemplate->addFooter(); return $emailTemplate; } diff --git a/apps/files_sharing/tests/SharesReminderJobTest.php b/apps/files_sharing/tests/SharesReminderJobTest.php index d35d88786f2..edb348de51b 100644 --- a/apps/files_sharing/tests/SharesReminderJobTest.php +++ b/apps/files_sharing/tests/SharesReminderJobTest.php @@ -184,13 +184,7 @@ class SharesReminderJobTest extends \Test\TestCase { ->with([$email ?: $this->userManager->get($this->user2)->getSystemEMailAddress()]); $this->assertSame(false, $share->getReminderSent()); $this->job->run([]); - $qb = $this->db->getQueryBuilder(); - $reminderSent = $qb - ->select('reminder_sent') - ->from('share') - ->where($qb->expr()->eq('id', $qb->createNamedParameter($share->getId()))) - ->executeQuery() - ->fetch()["reminder_sent"]; - $this->assertEquals($shouldBeReminded, $reminderSent); + $share = $this->shareManager->getShareById($share->getFullId()); + $this->assertEquals($shouldBeReminded, $share->getReminderSent()); } } diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index e6bda058aaa..ca1546a3752 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -756,6 +756,7 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider ->set('hide_download', $qb->createNamedParameter((int)$share->getHideDownload(), IQueryBuilder::PARAM_INT)) ->set('attributes', $qb->createNamedParameter($shareAttributes)) ->set('mail_send', $qb->createNamedParameter((int)$share->getMailSend(), IQueryBuilder::PARAM_INT)) + ->set('reminder_sent', $qb->createNamedParameter((int)$share->getReminderSent(), IQueryBuilder::PARAM_INT)) ->executeStatement(); if ($originalShare->getNote() !== $share->getNote() && $share->getNote() !== '') { @@ -987,7 +988,7 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider } /** - * Create a share object from an database row + * Create a share object from a database row * * @throws InvalidShare * @throws ShareNotFound @@ -1012,6 +1013,7 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider $share->setLabel($data['label']); $share->setSendPasswordByTalk((bool)$data['password_by_talk']); $share->setHideDownload((bool)$data['hide_download']); + $share->setReminderSent((bool)$data['reminder_sent']); if ($data['uid_initiator'] !== null) { $share->setShareOwner($data['uid_owner']); diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 9268d575c85..a5f5ca715ce 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -97,6 +97,8 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv if ($expirationDate !== null) { $qb->setValue('expiration', $qb->createNamedParameter($expirationDate, 'datetime')); } + + $qb->setValue('reminder_sent', $qb->createNamedParameter($share->getReminderSent() ? 1 : 0, IQueryBuilder::PARAM_INT)); } elseif ($share->getShareType() === IShare::TYPE_GROUP) { //Set the GID of the group we share with $qb->setValue('share_with', $qb->createNamedParameter($share->getSharedWith())); @@ -223,6 +225,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv ->set('expiration', $qb->createNamedParameter($expirationDate, IQueryBuilder::PARAM_DATE)) ->set('note', $qb->createNamedParameter($share->getNote())) ->set('accepted', $qb->createNamedParameter($share->getStatus())) + ->set('reminder_sent', $qb->createNamedParameter($share->getReminderSent())) ->execute(); } elseif ($share->getShareType() === IShare::TYPE_GROUP) { $qb = $this->dbConn->getQueryBuilder(); @@ -1006,7 +1009,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv } /** - * Create a share object from an database row + * Create a share object from a database row * * @param mixed[] $data * @return \OCP\Share\IShare @@ -1068,6 +1071,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv $share->setProviderId($this->identifier()); $share->setHideDownload((int)$data['hide_download'] === 1); + $share->setReminderSent((int)$data['reminder_sent'] === 1); return $share; } diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index 92f801e3fc1..1c0b21b8038 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -72,8 +72,7 @@ class Share implements IShare { private $nodeCacheEntry; /** @var bool */ private $hideDownload = false; - /** @var bool */ - private $reminderSent = false; + private bool $reminderSent = false; private bool $noExpirationDate = false; -- cgit v1.2.3 From 457eaee2b31c1baa7ec7386f1b53f84e8a87f146 Mon Sep 17 00:00:00 2001 From: Stefan Cherniakov Date: Mon, 2 Sep 2024 15:02:50 +0200 Subject: fix(files_sharing): Add missing check for null & use bool instead of int for reminder_sent field Signed-off-by: Stefan Cherniakov --- apps/files_sharing/lib/SharesReminderJob.php | 4 +++- apps/files_sharing/tests/SharesReminderJobTest.php | 2 +- apps/sharebymail/lib/ShareByMailProvider.php | 2 +- lib/private/Share20/DefaultShareProvider.php | 6 +++--- 4 files changed, 8 insertions(+), 6 deletions(-) (limited to 'apps/files_sharing') diff --git a/apps/files_sharing/lib/SharesReminderJob.php b/apps/files_sharing/lib/SharesReminderJob.php index 4b19ee72d18..6f7a379ac36 100644 --- a/apps/files_sharing/lib/SharesReminderJob.php +++ b/apps/files_sharing/lib/SharesReminderJob.php @@ -59,7 +59,9 @@ class SharesReminderJob extends TimedJob { public function run(mixed $argument): void { foreach ($this->getShares() as $share) { $reminderInfo = $this->prepareReminder($share); - $this->sendReminder($reminderInfo); + if ($reminderInfo !== null) { + $this->sendReminder($reminderInfo); + } } } diff --git a/apps/files_sharing/tests/SharesReminderJobTest.php b/apps/files_sharing/tests/SharesReminderJobTest.php index edb348de51b..5f01f4850e9 100644 --- a/apps/files_sharing/tests/SharesReminderJobTest.php +++ b/apps/files_sharing/tests/SharesReminderJobTest.php @@ -158,7 +158,7 @@ class SharesReminderJobTest extends \Test\TestCase { $testFolder = $user1Folder->newFolder('test'); if (!$isEmpty) { - $testFolder->newFile("some_file.txt"); + $testFolder->newFile('some_file.txt'); } $share = $this->shareManager->newShare(); diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index ca1546a3752..74483771762 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -756,7 +756,7 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider ->set('hide_download', $qb->createNamedParameter((int)$share->getHideDownload(), IQueryBuilder::PARAM_INT)) ->set('attributes', $qb->createNamedParameter($shareAttributes)) ->set('mail_send', $qb->createNamedParameter((int)$share->getMailSend(), IQueryBuilder::PARAM_INT)) - ->set('reminder_sent', $qb->createNamedParameter((int)$share->getReminderSent(), IQueryBuilder::PARAM_INT)) + ->set('reminder_sent', $qb->createNamedParameter($share->getReminderSent(), IQueryBuilder::PARAM_BOOL)) ->executeStatement(); if ($originalShare->getNote() !== $share->getNote() && $share->getNote() !== '') { diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index a5f5ca715ce..3ea429dfe3d 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -98,7 +98,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv $qb->setValue('expiration', $qb->createNamedParameter($expirationDate, 'datetime')); } - $qb->setValue('reminder_sent', $qb->createNamedParameter($share->getReminderSent() ? 1 : 0, IQueryBuilder::PARAM_INT)); + $qb->setValue('reminder_sent', $qb->createNamedParameter($share->getReminderSent(), IQueryBuilder::PARAM_BOOL)); } elseif ($share->getShareType() === IShare::TYPE_GROUP) { //Set the GID of the group we share with $qb->setValue('share_with', $qb->createNamedParameter($share->getSharedWith())); @@ -225,7 +225,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv ->set('expiration', $qb->createNamedParameter($expirationDate, IQueryBuilder::PARAM_DATE)) ->set('note', $qb->createNamedParameter($share->getNote())) ->set('accepted', $qb->createNamedParameter($share->getStatus())) - ->set('reminder_sent', $qb->createNamedParameter($share->getReminderSent())) + ->set('reminder_sent', $qb->createNamedParameter($share->getReminderSent(), IQueryBuilder::PARAM_BOOL)) ->execute(); } elseif ($share->getShareType() === IShare::TYPE_GROUP) { $qb = $this->dbConn->getQueryBuilder(); @@ -1071,7 +1071,7 @@ class DefaultShareProvider implements IShareProviderWithNotification, IShareProv $share->setProviderId($this->identifier()); $share->setHideDownload((int)$data['hide_download'] === 1); - $share->setReminderSent((int)$data['reminder_sent'] === 1); + $share->setReminderSent((bool)$data['reminder_sent']); return $share; } -- cgit v1.2.3 From 2685501231821be2f7ca72568bedd5571fe0eef2 Mon Sep 17 00:00:00 2001 From: Stefan Cherniakov Date: Mon, 2 Sep 2024 21:58:03 +0200 Subject: fix(files_sharing): Add check for null Signed-off-by: Stefan Cherniakov --- apps/files_sharing/lib/SharesReminderJob.php | 3 +++ 1 file changed, 3 insertions(+) (limited to 'apps/files_sharing') diff --git a/apps/files_sharing/lib/SharesReminderJob.php b/apps/files_sharing/lib/SharesReminderJob.php index 6f7a379ac36..9682f7fc770 100644 --- a/apps/files_sharing/lib/SharesReminderJob.php +++ b/apps/files_sharing/lib/SharesReminderJob.php @@ -132,6 +132,9 @@ class SharesReminderJob extends TimedJob { $reminderInfo = []; if ($share->getShareType() == IShare::TYPE_USER) { $user = $this->userManager->get($sharedWith); + if ($user === null) { + return null; + } $reminderInfo['email'] = $user->getEMailAddress(); $reminderInfo['userLang'] = $this->l10nFactory->getUserLanguage($user); $reminderInfo['folderLink'] = $this->urlGenerator->linkToRouteAbsolute('files.view.index', [ -- cgit v1.2.3