Optimization method FormsService::canSubmit

Signed-off-by: ailkiv <[email protected]>

lib/Controller/ApiController.php

@@ -1161,7 +1161,7 @@ public function insertSubmission(int $formId, array $answers, string $shareHash
 
 		// Ensure the form is unique if needed.
 		// If we can not submit anymore then the submission must be unique
-		if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) {
+		if (!$this->formsService->canSubmit($form) && $this->submissionMapper->hasMultipleFormSubmissionsByUser($form, $submission->getUserId())) {
 			$this->submissionMapper->delete($submission);
 			throw new OCSForbiddenException('Already submitted');
 		}

lib/Db/SubmissionMapper.php

@@ -86,36 +86,40 @@ public function findById(int $id): Submission {
 	}
 
 	/**
-	 * @param int $formId
-	 * @throws DoesNotExistException if not found
-	 * @return array
+	 * Сhecks if there are multiple form submissions by user
+	 * @param Form $form of the form to count submissions
+	 * @param string $userId ID of the user to count submissions
 	 */
-	public function findParticipantsByForm(int $formId): array {
-		$qb = $this->db->getQueryBuilder();
-
-		$qb->select('user_id')
-			->from($this->getTableName())
-			->where(
-				$qb->expr()->eq('form_id', $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT))
-			);
-
-		$submissionEntities = $this->findEntities($qb);
+	public function hasMultipleFormSubmissionsByUser(Form $form, string $userId): bool {
+		return $this->countSubmissionsWithFilters($form->getId(), $userId, 2) >= 2;
+	}
 
-		// From array of submissionEntities produce array of userIds.
-		$userIds = array_map(function ($submissionEntity) {
-			return $submissionEntity->getUserId();
-		}, $submissionEntities);
+	/**
+	 * Сhecks if there are form submissions by user
+	 * @param Form $form of the form to count submissions
+	 * @param string $userId ID of the user to count submissions
+	 */
+	public function hasFormSubmissionsByUser(Form $form, string $userId): bool {
+		return (bool)$this->countSubmissionsWithFilters($form->getId(), $userId, 1);
+	}
 
-		return $userIds;
+	/**
+	 * Count submissions by form
+	 * @param int $formId ID of the form to count submissions
+	 * @throws \Exception
+	 */
+	public function countSubmissions(int $formId): int {
+		return $this->countSubmissionsWithFilters($formId, null, -1);
 	}
 
 	/**
-	 * Count submissions by form and optionally also by userId
-	 * @param int $formId ID of the form to count submissions for
+	 * Count submissions by form with optional filters
+	 * @param int $formId ID of the form to count submissions
 	 * @param string|null $userId optionally limit submissions to the one of that user
+	 * @param int $limit allows to limit the query selection. If -1, the restriction is ignored
 	 * @throws \Exception
 	 */
-	public function countSubmissions(int $formId, ?string $userId = null): int {
+	protected function countSubmissionsWithFilters(int $formId, ?string $userId = null, int $limit = -1): int {
 		$qb = $this->db->getQueryBuilder();
 
 		$query = $qb->select($qb->func()->count('*', 'num_submissions'))
@@ -124,6 +128,9 @@ public function countSubmissions(int $formId, ?string $userId = null): int {
 		if (!is_null($userId)) {
 			$query->andWhere($qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)));
 		}
+		if ($limit !== -1) {
+			$query->setMaxResults($limit);
+		}
 
 		$result = $query->executeQuery();
 		$row = $result->fetch();

lib/Migration/Version040300Date20240708163401.php

@@ -0,0 +1,52 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2024 Andrii Ilkiv <[email protected]>
+ *
+ * @author Andrii Ilkiv <[email protected]>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OCA\Forms\Migration;
+
+use Closure;
+use OCP\DB\ISchemaWrapper;
+use OCP\Migration\IOutput;
+use OCP\Migration\SimpleMigrationStep;
+
+class Version040300Date20240708163401 extends SimpleMigrationStep {
+	/**
+	 * @param IOutput $output
+	 * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
+	 * @param array $options
+	 * @return null|ISchemaWrapper
+	 */
+	public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
+		/** @var ISchemaWrapper $schema */
+		$schema = $schemaClosure();
+
+		$table = $schema->getTable('forms_v2_submissions');
+		if (!$table->hasIndex('forms_submissions_form_user')) {
+			$table->addIndex(['form_id', 'user_id'], 'forms_submissions_form_user');
+		}
+
+		return $schema;
+	}
+}

lib/Service/FormsService.php

@@ -335,13 +335,11 @@ public function canSubmit(Form $form): bool {
 		}
 
 		// Refuse access, if SubmitMultiple is not set and user already has taken part.
-		if (!$form->getSubmitMultiple()) {
-			$participants = $this->submissionMapper->findParticipantsByForm($form->getId());
-			foreach ($participants as $participant) {
-				if ($participant === $this->currentUser->getUID()) {
-					return false;
-				}
-			}
+		if (
+			!$form->getSubmitMultiple() &&
+			$this->submissionMapper->hasFormSubmissionsByUser($form, $this->currentUser->getUID())
+		) {
+			return false;
 		}
 
 		return true;

lib/Service/SubmissionService.php

@@ -165,13 +165,6 @@ public function getSubmissions(int $formId): array {
 		}
 	}
 
-	/**
-	 * Validate the new submission is unique
-	 */
-	public function isUniqueSubmission(Submission $newSubmission): bool {
-		return $this->submissionMapper->countSubmissions($newSubmission->getFormId(), $newSubmission->getUserId()) === 1;
-	}
-
 	/**
 	 * Export Submissions to Cloud-Filesystem
 	 *

tests/Unit/Db/SubmissionMapperTest.php

@@ -0,0 +1,139 @@
+<?php
+
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2024 Abdrii Ilkiv <[email protected]>
+ *
+ * @author Abdrii Ilkiv <[email protected]>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+namespace OCA\Forms\Tests\Unit\Db;
+
+use OCA\Forms\Db\Form;
+use OCA\Forms\Db\SubmissionMapper;
+
+use Test\TestCase;
+
+class SubmissionMapperTest extends TestCase {
+
+	private SubmissionMapper $mockSubmissionMapper;
+
+
+	public function setUp(): void {
+		parent::setUp();
+
+		$this->mockSubmissionMapper = $this->getMockBuilder(SubmissionMapper::class)
+					 ->disableOriginalConstructor()
+					 ->setMethods(['countSubmissionsWithFilters'])
+					 ->getMock();
+	}
+
+	/**
+	 * @dataProvider dataHasMultipleFormSubmissionsByUser
+	 */
+	public function testHasMultipleFormSubmissionsByUser(int $numberOfSubmissions, bool $expected) {
+		$this->mockSubmissionMapper->expects($this->once())
+			 ->method('countSubmissionsWithFilters')
+			 ->will($this->returnValue($numberOfSubmissions));
+
+		$form = new Form();
+		$form->setId(1);
+
+		$this->assertEquals($expected, $this->mockSubmissionMapper->hasMultipleFormSubmissionsByUser($form, 'user1'));
+	}
+
+	public function dataHasMultipleFormSubmissionsByUser() {
+		return [
+			[
+				'numberOfSubmissions' => 0,
+				'expected' => false,
+			],
+			[
+				'numberOfSubmissions' => 1,
+				'expected' => false,
+			],
+			[
+				'numberOfSubmissions' => 2,
+				'expected' => true,
+			],
+			[
+				'numberOfSubmissions' => 3,
+				'expected' => true,
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider dataHasFormSubmissionsByUser
+	 */
+	public function testHasFormSubmissionsByUser(int $numberOfSubmissions, bool $expected) {
+		$this->mockSubmissionMapper->expects($this->once())
+			 ->method('countSubmissionsWithFilters')
+			 ->will($this->returnValue($numberOfSubmissions));
+
+		$form = new Form();
+		$form->setId(1);
+
+		$this->assertEquals($expected, $this->mockSubmissionMapper->hasFormSubmissionsByUser($form, 'user1'));
+	}
+
+	public function dataHasFormSubmissionsByUser() {
+		return [
+			[
+				'numberOfSubmissions' => 0,
+				'expected' => false,
+			],
+			[
+				'numberOfSubmissions' => 1,
+				'expected' => true,
+			],
+			[
+				'numberOfSubmissions' => 2,
+				'expected' => true,
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider dataCountSubmissions
+	 */
+	public function testCountSubmissions(int $numberOfSubmissions, int $expected) {
+		$this->mockSubmissionMapper->expects($this->once())
+			 ->method('countSubmissionsWithFilters')
+			 ->will($this->returnValue($numberOfSubmissions));
+
+		$this->assertEquals($expected, $this->mockSubmissionMapper->countSubmissions(1));
+	}
+
+	public function dataCountSubmissions() {
+		return [
+			[
+				'numberOfSubmissions' => 0,
+				'expected' => 0,
+			],
+			[
+				'numberOfSubmissions' => 1,
+				'expected' => 1,
+			],
+			[
+				'numberOfSubmissions' => 20,
+				'expected' => 20,
+			],
+		];
+	}
+}

tests/Unit/Service/FormsServiceTest.php

@@ -802,25 +802,25 @@ public function dataCanSubmit() {
 			'allowFormOwner' => [
 				'ownerId' => 'currentUser',
 				'submitMultiple' => false,
-				'participantsArray' => ['currentUser'],
+				'hasFormSubmissionsByUser' => true,
 				'expected' => true
 			],
 			'submitMultipleGood' => [
 				'ownerId' => 'someUser',
 				'submitMultiple' => false,
-				'participantsArray' => ['notCurrentUser'],
+				'hasFormSubmissionsByUser' => false,
 				'expected' => true
 			],
 			'submitMultipleNotGood' => [
 				'ownerId' => 'someUser',
 				'submitMultiple' => false,
-				'participantsArray' => ['notCurrentUser', 'currentUser'],
+				'hasFormSubmissionsByUser' => true,
 				'expected' => false
 			],
 			'submitMultiple' => [
 				'ownerId' => 'someUser',
 				'submitMultiple' => true,
-				'participantsArray' => ['currentUser'],
+				'hasFormSubmissionsByUser' => true,
 				'expected' => true
 			]
 		];
@@ -830,10 +830,10 @@ public function dataCanSubmit() {
 	 *
 	 * @param string $ownerId
 	 * @param bool $submitMultiple
-	 * @param array $participantsArray
+	 * @param bool $hasFormSubmissionsByUser
 	 * @param bool $expected
 	 */
-	public function testCanSubmit(string $ownerId, bool $submitMultiple, array $participantsArray, bool $expected) {
+	public function testCanSubmit(string $ownerId, bool $submitMultiple, bool $hasFormSubmissionsByUser, bool $expected) {
 		$form = new Form();
 		$form->setId(42);
 		$form->setAccess([
@@ -844,9 +844,9 @@ public function testCanSubmit(string $ownerId, bool $submitMultiple, array $part
 		$form->setSubmitMultiple($submitMultiple);
 
 		$this->submissionMapper->expects($this->any())
-			->method('findParticipantsByForm')
-			->with(42)
-			->willReturn($participantsArray);
+			->method('hasFormSubmissionsByUser')
+			->with($form, 'currentUser')
+			->willReturn($hasFormSubmissionsByUser);
 
 		$this->assertEquals($expected, $this->formsService->canSubmit($form));
 	}