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 --- lib/private/Share20/Share.php | 11 +++++++++++ lib/public/Share/IShare.php | 16 ++++++++++++++++ 2 files changed, 27 insertions(+) (limited to 'lib') diff --git a/lib/private/Share20/Share.php b/lib/private/Share20/Share.php index b01d05ca4b9..92f801e3fc1 100644 --- a/lib/private/Share20/Share.php +++ b/lib/private/Share20/Share.php @@ -72,6 +72,8 @@ class Share implements IShare { private $nodeCacheEntry; /** @var bool */ private $hideDownload = false; + /** @var bool */ + private $reminderSent = false; private bool $noExpirationDate = false; @@ -613,4 +615,13 @@ class Share implements IShare { public function getHideDownload(): bool { return $this->hideDownload; } + + public function setReminderSent(bool $reminderSent): IShare { + $this->reminderSent = $reminderSent; + return $this; + } + + public function getReminderSent(): bool { + return $this->reminderSent; + } } diff --git a/lib/public/Share/IShare.php b/lib/public/Share/IShare.php index fe5e7eb47dd..c2843c078e3 100644 --- a/lib/public/Share/IShare.php +++ b/lib/public/Share/IShare.php @@ -617,4 +617,20 @@ interface IShare { * @since 15.0.0 */ public function getHideDownload(): bool; + + /** + * Sets a flag that stores whether a reminder via email has been sent + * + * @return self The modified object + * @since 31.0.0 + */ + public function setReminderSent(bool $reminderSent): IShare; + + /** + * Gets a flag that stores whether a reminder via email has been sent + * + * @return bool + * @since 31.0.0 + */ + public function getReminderSent(): bool; } -- 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 'lib') 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 'lib') 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