fix(files_sharing): Make share reminders more stable & fix issues

Signed-off-by: Stefan Cherniakov <luka-sama@pm.me>

5 files changed, 96 insertions, 145 deletions

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<IShare>
+	 * @return array<IShare>|\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<IShare> $shares Shares that were obtained with {@link getShares}
-	 * @return array<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<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;