Skip to content

Commit 7d667ff

Browse files
AIlkivAIlkiv
committed
Optimization method FormsService::canSubmit
Signed-off-by: ailkiv <a.ilkiv.ye@gmail.com>
1 parent 8c4a6b2 commit 7d667ff

8 files changed

Lines changed: 234 additions & 82 deletions

File tree

‎lib/Controller/ApiController.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1161,7 +1161,8 @@ public function insertSubmission(int $formId, array $answers, string $shareHash
11611161

11621162
// Ensure the form is unique if needed.
11631163
// If we can not submit anymore then the submission must be unique
1164-
if (!$this->formsService->canSubmit($form) && !$this->submissionService->isUniqueSubmission($submission)) {
1164+
if (!$this->formsService->canSubmit($form) && $this->submissionMapper->hasMultipleFormSubmissionsByUser($form, $submission->getUserId())
1165+
) {
11651166
$this->submissionMapper->delete($submission);
11661167
throw new OCSForbiddenException('Already submitted');
11671168
}

‎lib/Db/SubmissionMapper.php‎

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -86,36 +86,39 @@ public function findById(int $id): Submission {
8686
}
8787

8888
/**
89-
* @param int $formId
90-
* @throws DoesNotExistException if not found
91-
* @return array
89+
* Сhecks if there are multiple form submissions by user
90+
* @param Form $form of the form to count submissions
91+
* @param string $userId ID of the user to count submissions
9292
*/
93-
public function findParticipantsByForm(int $formId): array {
94-
$qb = $this->db->getQueryBuilder();
95-
96-
$qb->select('user_id')
97-
->from($this->getTableName())
98-
->where(
99-
$qb->expr()->eq('form_id', $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT))
100-
);
101-
102-
$submissionEntities = $this->findEntities($qb);
93+
public function hasMultipleFormSubmissionsByUser(Form $form, string $userId): bool {
94+
return $this->countSubmissionsWithFilters($form->getId(), $userId, 2) >= 2;
95+
}
10396

104-
// From array of submissionEntities produce array of userIds.
105-
$userIds = array_map(function ($submissionEntity) {
106-
return $submissionEntity->getUserId();
107-
}, $submissionEntities);
97+
/**
98+
* Сhecks if there are form submissions by user
99+
* @param Form $form of the form to count submissions
100+
* @param string $userId ID of the user to count submissions
101+
*/
102+
public function hasFormSubmissionsByUser(Form $form, string $userId): bool {
103+
return (bool)$this->countSubmissionsWithFilters($form->getId(), $userId, 1);
104+
}
108105

109-
return $userIds;
106+
/**
107+
* Count submissions by form
108+
* @param int $formId ID of the form to count submissions
109+
* @throws \Exception
110+
*/
111+
public function countSubmissions(int $formId): int {
112+
return $this->countSubmissionsWithFilters($formId, null, -1);
110113
}
111114

112115
/**
113-
* Count submissions by form and optionally also by userId
114-
* @param int $formId ID of the form to count submissions for
116+
* Count submissions by form with optional filters
117+
* @param int $formId ID of the form to count submissions
115118
* @param string|null $userId optionally limit submissions to the one of that user
116119
* @throws \Exception
117120
*/
118-
public function countSubmissions(int $formId, ?string $userId = null): int {
121+
protected function countSubmissionsWithFilters(int $formId, ?string $userId = null, int $limit = -1): int {
119122
$qb = $this->db->getQueryBuilder();
120123

121124
$query = $qb->select($qb->func()->count('*', 'num_submissions'))
@@ -124,14 +127,16 @@ public function countSubmissions(int $formId, ?string $userId = null): int {
124127
if (!is_null($userId)) {
125128
$query->andWhere($qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)));
126129
}
130+
if ($limit !== -1) {
131+
$query->setMaxResults($limit);
132+
}
127133

128134
$result = $query->executeQuery();
129135
$row = $result->fetch();
130136
$result->closeCursor();
131137

132138
return (int) ($row['num_submissions'] ?? 0);
133139
}
134-
135140
/**
136141
* Delete the Submission, including answers.
137142
* @param int $id of the submission to delete

‎lib/Migration/Version040300Date20240708163401.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* @copyright Copyright (c) 2024 Andrii Ilkiv <ailkiv@users.noreply.github.com>
7+
*
8+
* @author Andrii Ilkiv <ailkiv@users.noreply.github.com>
9+
*
10+
* @license GNU AGPL version 3 or any later version
11+
*
12+
* This program is free software: you can redistribute it and/or modify
13+
* it under the terms of the GNU Affero General Public License as
14+
* published by the Free Software Foundation, either version 3 of the
15+
* License, or (at your option) any later version.
16+
*
17+
* This program is distributed in the hope that it will be useful,
18+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
19+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
20+
* GNU Affero General Public License for more details.
21+
*
22+
* You should have received a copy of the GNU Affero General Public License
23+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
24+
*
25+
*/
26+
27+
namespace OCA\Forms\Migration;
28+
29+
use Closure;
30+
use OCP\DB\ISchemaWrapper;
31+
use OCP\Migration\IOutput;
32+
use OCP\Migration\SimpleMigrationStep;
33+
34+
class Version040300Date20240708163401 extends SimpleMigrationStep {
35+
/**
36+
* @param IOutput $output
37+
* @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
38+
* @param array $options
39+
* @return null|ISchemaWrapper
40+
*/
41+
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
42+
/** @var ISchemaWrapper $schema */
43+
$schema = $schemaClosure();
44+
45+
if ($schema->hasTable('forms_v2_submissions')) {
46+
$schema->getTable('forms_v2_submissions')
47+
->addIndex(['form_id', 'user_id'], 'forms_submissions_form_user');
48+
}
49+
50+
return $schema;
51+
}
52+
}

‎lib/Service/FormsService.php‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -333,15 +333,12 @@ public function canSubmit(Form $form): bool {
333333
if ($this->currentUser->getUID() === $form->getOwnerId()) {
334334
return true;
335335
}
336-
337336
// Refuse access, if SubmitMultiple is not set and user already has taken part.
338-
if (!$form->getSubmitMultiple()) {
339-
$participants = $this->submissionMapper->findParticipantsByForm($form->getId());
340-
foreach ($participants as $participant) {
341-
if ($participant === $this->currentUser->getUID()) {
342-
return false;
343-
}
344-
}
337+
if (
338+
!$form->getSubmitMultiple()
339+
&& $this->submissionMapper->hasFormSubmissionsByUser($form, $this->currentUser->getUID())
340+
) {
341+
return false;
345342
}
346343

347344
return true;

‎lib/Service/SubmissionService.php‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -165,13 +165,6 @@ public function getSubmissions(int $formId): array {
165165
}
166166
}
167167

168-
/**
169-
* Validate the new submission is unique
170-
*/
171-
public function isUniqueSubmission(Submission $newSubmission): bool {
172-
return $this->submissionMapper->countSubmissions($newSubmission->getFormId(), $newSubmission->getUserId()) === 1;
173-
}
174-
175168
/**
176169
* Export Submissions to Cloud-Filesystem
177170
*

‎tests/Unit/Db/SubmissionMapperTest.php‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* @copyright Copyright (c) 2024 Abdrii Ilkiv <ailkiv@users.noreply.github.com>
6+
*
7+
* @author Abdrii Ilkiv <ailkiv@users.noreply.github.com>
8+
*
9+
* @license AGPL-3.0-or-later
10+
*
11+
* This program is free software: you can redistribute it and/or modify
12+
* it under the terms of the GNU Affero General Public License as
13+
* published by the Free Software Foundation, either version 3 of the
14+
* License, or (at your option) any later version.
15+
*
16+
* This program is distributed in the hope that it will be useful,
17+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
18+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
19+
* GNU Affero General Public License for more details.
20+
*
21+
* You should have received a copy of the GNU Affero General Public License
22+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
23+
*
24+
*/
25+
namespace OCA\Forms\Tests\Unit\Db;
26+
27+
use OCA\Forms\Db\SubmissionMapper;
28+
use OCA\Forms\Db\Form;
29+
30+
use Test\TestCase;
31+
32+
class SubmissionMapperTest extends TestCase {
33+
34+
private SubmissionMapper $mockSubmissionMapper;
35+
36+
37+
public function setUp(): void {
38+
parent::setUp();
39+
40+
$this->mockSubmissionMapper = $this->getMockBuilder(SubmissionMapper::class)
41+
->disableOriginalConstructor()
42+
->setMethods(['countSubmissionsWithFilters'])
43+
->getMock();
44+
}
45+
46+
/**
47+
* @dataProvider dataHasMultipleFormSubmissionsByUser
48+
*/
49+
public function testHasMultipleFormSubmissionsByUser(int $numberOfSubmissions, bool $expected) {
50+
$this->mockSubmissionMapper->expects($this->once())
51+
->method('countSubmissionsWithFilters')
52+
->will($this->returnValue($numberOfSubmissions));
53+
54+
$form = new Form();
55+
$form->setId(1);
56+
57+
$this->assertEquals($expected, $this->mockSubmissionMapper->hasMultipleFormSubmissionsByUser($form, 'user1'));
58+
}
59+
60+
public function dataHasMultipleFormSubmissionsByUser() {
61+
return [
62+
[
63+
'numberOfSubmissions' => 0,
64+
'expected' => false,
65+
],
66+
[
67+
'numberOfSubmissions' => 1,
68+
'expected' => false,
69+
],
70+
[
71+
'numberOfSubmissions' => 2,
72+
'expected' => true,
73+
],
74+
[
75+
'numberOfSubmissions' => 3,
76+
'expected' => true,
77+
],
78+
];
79+
}
80+
81+
/**
82+
* @dataProvider dataHasFormSubmissionsByUser
83+
*/
84+
public function testHasFormSubmissionsByUser(int $numberOfSubmissions, bool $expected) {
85+
$this->mockSubmissionMapper->expects($this->once())
86+
->method('countSubmissionsWithFilters')
87+
->will($this->returnValue($numberOfSubmissions));
88+
89+
$form = new Form();
90+
$form->setId(1);
91+
92+
$this->assertEquals($expected, $this->mockSubmissionMapper->hasFormSubmissionsByUser($form, 'user1'));
93+
}
94+
95+
public function dataHasFormSubmissionsByUser() {
96+
return [
97+
[
98+
'numberOfSubmissions' => 0,
99+
'expected' => false,
100+
],
101+
[
102+
'numberOfSubmissions' => 1,
103+
'expected' => true,
104+
],
105+
[
106+
'numberOfSubmissions' => 2,
107+
'expected' => true,
108+
],
109+
];
110+
}
111+
112+
/**
113+
* @dataProvider dataCountSubmissions
114+
*/
115+
public function testCountSubmissions(int $numberOfSubmissions, int $expected) {
116+
$this->mockSubmissionMapper->expects($this->once())
117+
->method('countSubmissionsWithFilters')
118+
->will($this->returnValue($numberOfSubmissions));
119+
120+
$this->assertEquals($expected, $this->mockSubmissionMapper->countSubmissions(1));
121+
}
122+
123+
public function dataCountSubmissions() {
124+
return [
125+
[
126+
'numberOfSubmissions' => 0,
127+
'expected' => 0,
128+
],
129+
[
130+
'numberOfSubmissions' => 1,
131+
'expected' => 1,
132+
],
133+
[
134+
'numberOfSubmissions' => 20,
135+
'expected' => 20,
136+
],
137+
];
138+
}
139+
}

‎tests/Unit/Service/FormsServiceTest.php‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -802,25 +802,25 @@ public function dataCanSubmit() {
802802
'allowFormOwner' => [
803803
'ownerId' => 'currentUser',
804804
'submitMultiple' => false,
805-
'participantsArray' => ['currentUser'],
805+
'hasFormSubmissionsByUser' => true,
806806
'expected' => true
807807
],
808808
'submitMultipleGood' => [
809809
'ownerId' => 'someUser',
810810
'submitMultiple' => false,
811-
'participantsArray' => ['notCurrentUser'],
811+
'hasFormSubmissionsByUser' => false,
812812
'expected' => true
813813
],
814814
'submitMultipleNotGood' => [
815815
'ownerId' => 'someUser',
816816
'submitMultiple' => false,
817-
'participantsArray' => ['notCurrentUser', 'currentUser'],
817+
'hasFormSubmissionsByUser' => true,
818818
'expected' => false
819819
],
820820
'submitMultiple' => [
821821
'ownerId' => 'someUser',
822822
'submitMultiple' => true,
823-
'participantsArray' => ['currentUser'],
823+
'hasFormSubmissionsByUser' => true,
824824
'expected' => true
825825
]
826826
];
@@ -830,10 +830,10 @@ public function dataCanSubmit() {
830830
*
831831
* @param string $ownerId
832832
* @param bool $submitMultiple
833-
* @param array $participantsArray
833+
* @param bool $hasFormSubmissionsByUser
834834
* @param bool $expected
835835
*/
836-
public function testCanSubmit(string $ownerId, bool $submitMultiple, array $participantsArray, bool $expected) {
836+
public function testCanSubmit(string $ownerId, bool $submitMultiple, bool $hasFormSubmissionsByUser, bool $expected) {
837837
$form = new Form();
838838
$form->setId(42);
839839
$form->setAccess([
@@ -844,9 +844,9 @@ public function testCanSubmit(string $ownerId, bool $submitMultiple, array $part
844844
$form->setSubmitMultiple($submitMultiple);
845845

846846
$this->submissionMapper->expects($this->any())
847-
->method('findParticipantsByForm')
848-
->with(42)
849-
->willReturn($participantsArray);
847+
->method('hasFormSubmissionsByUser')
848+
->with($form, 'currentUser')
849+
->willReturn($hasFormSubmissionsByUser);
850850

851851
$this->assertEquals($expected, $this->formsService->canSubmit($form));
852852
}

0 commit comments

Comments
 (0)