fix(notifications): Rework the notification handling

- Don't generate notifications live on resetAllSignatures but use a cron job - Don't create notifications for users that signed any terms already - Don't create notifications when enabling the app, but only after terms have been added Signed-off-by: Joas Schilling <[email protected]>
nextcloud · Oct 25, 2024 · 03222a5 · 03222a5
1 parent 083f3b6
commit 03222a5
Show file tree

Hide file tree

Showing 6 changed files with 123 additions and 84 deletions.
diff --git a/appinfo/info.xml b/appinfo/info.xml
@@ -36,15 +36,10 @@ This product includes GeoLite2 data created by MaxMind, available from [maxmind.
 		<nextcloud min-version="28" max-version="30" />
 	</dependencies>
 
-	<repair-steps>
-		<install>
-			<step>OCA\TermsOfService\Migration\CreateNotifications</step>
-		</install>
-	</repair-steps>
-    <commands>
-        <command>OCA\TermsOfService\Command\SetTermsCommand</command>
-        <command>OCA\TermsOfService\Command\DeleteTermsCommand</command>
-    </commands>
+	<commands>
+		<command>OCA\TermsOfService\Command\SetTermsCommand</command>
+		<command>OCA\TermsOfService\Command\DeleteTermsCommand</command>
+	</commands>
 
 	<settings>
 		<admin>OCA\TermsOfService\Settings\Admin</admin>

diff --git a/lib/BackgroundJobs/CreateNotifications.php b/lib/BackgroundJobs/CreateNotifications.php
@@ -0,0 +1,89 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\TermsOfService\BackgroundJobs;
+
+use OCA\TermsOfService\AppInfo\Application;
+use OCA\TermsOfService\Db\Mapper\SignatoryMapper;
+use OCA\TermsOfService\Db\Mapper\TermsMapper;
+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\BackgroundJob\QueuedJob;
+use OCP\IConfig;
+use OCP\IUser;
+use OCP\IUserManager;
+use OCP\Notification\IManager;
+use OCP\Notification\INotification;
+use Psr\Log\LoggerInterface;
+
+class CreateNotifications extends QueuedJob {
+	public const BATCH_SIZE = 1000;
+	protected ?INotification $notification = null;
+	protected int $currentBatch = 0;
+
+	public function __construct(
+		protected IUserManager $userManager,
+		protected IManager $notificationsManager,
+		protected TermsMapper $termsMapper,
+		protected SignatoryMapper $signatoryMapper,
+		protected IConfig $config,
+		protected LoggerInterface $logger,
+		ITimeFactory $time,
+	) {
+		parent::__construct($time);
+	}
+
+	protected function run($argument): void {
+		if ($this->config->getAppValue(Application::APPNAME, 'sent_notifications', 'no') === 'yes') {
+			$this->logger->debug('ToS Notifications have already been sent');
+			return;
+		}
+
+		$terms = $this->termsMapper->getTerms();
+		if (empty($terms)) {
+			$this->logger->debug('No terms available to sign');
+			return;
+		}
+
+		$this->config->setAppValue(Application::APPNAME, 'sent_notifications', 'yes');
+
+		$this->notification = $this->notificationsManager->createNotification();
+		$this->notification->setApp('terms_of_service')
+			->setSubject('accept_terms')
+			->setObject('terms', '1');
+
+		// Mark all notifications as processed …
+		$this->notificationsManager->markProcessed($this->notification);
+
+		// … before generating new ones.
+		$this->notification->setDateTime(new \DateTime());
+
+		$this->notificationsManager->defer();
+		$this->currentBatch = 0;
+		$this->userManager->callForSeenUsers(\Closure::fromCallable([$this, 'callForSeenUsers']));
+		$this->notificationsManager->flush();
+	}
+
+	public function callForSeenUsers(IUser $user): void {
+		if ($this->signatoryMapper->hasSignedByUser($user)) {
+			// User already signed in the meantime
+			$this->logger->debug('User ' . $user->getUID() . ' already signed ToS');
+			return;
+		}
+
+		$this->notification->setUser($user->getUID());
+		$this->notificationsManager->notify($this->notification);
+
+		// Make sure we don't create a too huge batch for the push notifications
+		$this->currentBatch++;
+		if ($this->currentBatch === self::BATCH_SIZE) {
+			$this->notificationsManager->flush();
+			$this->notificationsManager->defer();
+			$this->currentBatch = 0;
+		}
+	}
+}
diff --git a/lib/Controller/SigningController.php b/lib/Controller/SigningController.php
@@ -7,10 +7,12 @@
 namespace OCA\TermsOfService\Controller;
 
 use OCA\TermsOfService\AppInfo\Application;
+use OCA\TermsOfService\BackgroundJobs\CreateNotifications;
 use OCA\TermsOfService\Db\Entities\Signatory;
 use OCA\TermsOfService\Db\Mapper\SignatoryMapper;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\OCSController;
+use OCP\BackgroundJob\IJobList;
 use OCP\IConfig;
 use OCP\IRequest;
 use OCP\ISession;
@@ -47,7 +49,8 @@ public function __construct(
 		IUserManager $userManager,
 		IConfig $config,
 		ISession $session,
-		IEventDispatcher $eventDispatcher
+		IEventDispatcher $eventDispatcher,
+		protected IJobList $jobList,
 	) {
 		parent::__construct($appName, $request);
 		$this->userId = $UserId;
@@ -113,21 +116,9 @@ public function resetAllSignatories(): DataResponse {
 		$this->signatoryMapper->deleteAllSignatories();
 		$this->config->setAppValue(Application::APPNAME, 'term_uuid', uniqid());
 
-		$notification = $this->notificationsManager->createNotification();
-		$notification->setApp('terms_of_service')
-			->setSubject('accept_terms')
-			->setObject('terms', '1');
-
-		// Mark all notifications as processed …
-		$this->notificationsManager->markProcessed($notification);
-
-		$notification->setDateTime(new \DateTime());
-
-		// … so we can create new ones for every one, also users which already accepted.
-		$this->userManager->callForSeenUsers(function(IUser $user) use ($notification) {
-			$notification->setUser($user->getUID());
-			$this->notificationsManager->notify($notification);
-		});
+		// Schedule a job to generate notifications
+		$this->config->deleteAppValue(Application::APPNAME, 'sent_notifications');
+		$this->jobList->add(CreateNotifications::class);
 
 		$event = $this->resetAllSignaturesEvent();
 		$this->eventDispatcher->dispatchTyped($event);

diff --git a/lib/Controller/TermsController.php b/lib/Controller/TermsController.php
@@ -7,6 +7,7 @@
 namespace OCA\TermsOfService\Controller;
 
 use OCA\TermsOfService\AppInfo\Application;
+use OCA\TermsOfService\BackgroundJobs\CreateNotifications;
 use OCA\TermsOfService\Checker;
 use OCA\TermsOfService\CountryDetector;
 use OCA\TermsOfService\Db\Entities\Terms;
@@ -18,6 +19,7 @@
 use OCP\AppFramework\OCSController;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
+use OCP\BackgroundJob\IJobList;
 use OCP\IConfig;
 use OCP\IRequest;
 use OCP\L10N\IFactory;
@@ -55,7 +57,8 @@ public function __construct(string $appName,
 								CountryDetector $countryDetector,
 								Checker $checker,
 								IConfig $config,
-								IEventDispatcher $eventDispatcher
+								IEventDispatcher $eventDispatcher,
+								protected IJobList $jobList,
 	) {
 		parent::__construct($appName, $request);
 		$this->factory = $factory;
@@ -152,6 +155,7 @@ public function create(string $countryCode,
 			$this->termsMapper->update($terms);
 		} else {
 			$this->termsMapper->insert($terms);
+			$this->jobList->add(CreateNotifications::class);
 		}
 
 		$event = $this->createTermsCreatedEvent();

diff --git a/lib/Db/Mapper/SignatoryMapper.php b/lib/Db/Mapper/SignatoryMapper.php
@@ -65,6 +65,24 @@ public function getSignatoriesByUser(IUser $user): array {
 		return $entities;
 	}
 
+	/**
+	 * Check if a user has signed the terms
+	 */
+	public function hasSignedByUser(IUser $user): bool {
+		$query = $this->db->getQueryBuilder();
+		$query->select($query->expr()->literal(1))
+			->from(self::TABLENAME)
+			->where($query->expr()->eq('user_id', $query->createNamedParameter($user->getUID())))
+			->setMaxResults(1);
+
+		$entities = [];
+		$result = $query->executeQuery();
+		$hasSigned = (bool)$result->fetchOne();
+		$result->closeCursor();
+
+		return $hasSigned;
+	}
+
 	/**
 	 * Update the signer of an entry
 	 *

diff --git a/lib/Migration/CreateNotifications.php b/lib/Migration/CreateNotifications.php