Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow editing of submission by the user #1690

Open
wants to merge 44 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
72eb6e8
patch for AllowEdit for Forms5
tpokorra Dec 31, 2024
53e5b68
drop DeleteSubmission, and make Clear button work for AllowEdit
tpokorra Dec 31, 2024
70eb574
fix php-cs lint issues
tpokorra Dec 31, 2024
7ad2b97
small fixes
tpokorra Dec 31, 2024
eb9f237
run prettier
tpokorra Dec 31, 2024
c9afdc2
more fixes
tpokorra Dec 31, 2024
cd35dca
fix existing tests by adding AllowEdit
tpokorra Jan 3, 2025
ede4874
fix lint issue
tpokorra Jan 3, 2025
c665372
fix existing tests by adding AllowEdit
tpokorra Jan 3, 2025
7f5ce93
fix testCanSubmit with AllowEdit=false
tpokorra Jan 3, 2025
53d00da
extend testCanSubmit by allowEditGood and allowEditNotGood
tpokorra Jan 3, 2025
4b181df
add unit test testUpdateSubmission_answers
tpokorra Jan 4, 2025
b0c4101
add unit test testGetFormWithAnswers for AllowEdit
tpokorra Jan 4, 2025
20def4d
add unit test testGetFormAllowEditWithoutAnswers
tpokorra Jan 4, 2025
48bb942
improve unit tests testGetFormAllowEditWithoutAnswers
tpokorra Jan 4, 2025
847c0a7
use existing function loadFormForSubmission instead of new function c…
tpokorra Jan 4, 2025
6d51f0e
new function canDeleteSubmission with Tests. improve tests for canDel…
tpokorra Jan 4, 2025
5cc6c07
drop function canDeleteSubmission. it is unrelated to this PR
tpokorra Jan 4, 2025
0c4cc2e
adjust label for AllowEdit
tpokorra Jan 4, 2025
18fbede
rename migration to Version 5 and current date
tpokorra Jan 9, 2025
cc208cb
drop IconDeleteSvg from Submit.vue
tpokorra Jan 9, 2025
db96895
fix error messages with multiple ors
tpokorra Jan 9, 2025
a1f5a60
use PUT for updating submission
tpokorra Jan 9, 2025
a102736
fail silently for MultipleObjectsReturnedException from findByFormAnd…
tpokorra Jan 9, 2025
7422021
updateSubmission: first check if editing is allowed and by this user
tpokorra Jan 9, 2025
1e3c1a3
add integration test testUpdateSubmission
tpokorra Jan 9, 2025
e5178cf
fix missing MultipleObjectsReturnedException for FormsService
tpokorra Jan 9, 2025
e81dfa3
testUpdateSubmission: POST files instead of PUT
tpokorra Jan 21, 2025
2779c8c
fixes for Psalm: add allowEdit
tpokorra Jan 21, 2025
e40798e
fix ApiRoute for updateSubmission
tpokorra Jan 21, 2025
97655a7
fix RespectAdminSettingsTest
tpokorra Jan 21, 2025
e69b1fc
fix openapi errors
tpokorra Jan 21, 2025
4fb8875
another fix for openapi
tpokorra Jan 21, 2025
ca30fba
extend FormsForm for psalm
tpokorra Jan 21, 2025
b2ea8d9
update openapi.json
tpokorra Jan 21, 2025
64e9747
another fix for FormsForm
tpokorra Jan 21, 2025
8366562
another fix for FormsForm
tpokorra Jan 21, 2025
e6434d8
another attempt to fix openapi issues
tpokorra Jan 21, 2025
c174a25
simplify check in updateSubmission
tpokorra Jan 22, 2025
0a3a092
testUpdateSubmission: set AllowEdit for form
tpokorra Jan 22, 2025
adacff4
testUpdateSubmission: use different http client for user1
tpokorra Jan 22, 2025
037bf85
fix password for user1
tpokorra Jan 22, 2025
43d47f2
fix testUpdateSubmission
tpokorra Jan 22, 2025
38cb8d6
fix testUpdateSubmission
tpokorra Jan 22, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/DataStructure.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ This document describes the Object-Structure, that is used within the Forms App
| isAnonymous | Boolean | | If Answers will be stored anonymously |
| state | Integer | [Form state](#form-state) | The state of the form |
| submitMultiple | Boolean | | If users are allowed to submit multiple times to the form |
| allowEdit | Boolean | | If users are allowed to edit or delete their response |
| showExpiration | Boolean | | If the expiration date will be shown on the form |
| canSubmit | Boolean | | If the user can Submit to the form, i.e. calculated information out of `submitMultiple` and existing submissions. |
| permissions | Array of [Permissions](#permissions) | Array of permissions regarding the form |
Expand All @@ -46,6 +47,7 @@ This document describes the Object-Structure, that is used within the Forms App
"expires": 0,
"isAnonymous": false,
"submitMultiple": true,
"allowEdit": false,
"showExpiration": false,
"canSubmit": true,
"permissions": [
Expand Down
157 changes: 149 additions & 8 deletions lib/Controller/ApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@
'showToAllUsers' => false,
]);
$form->setSubmitMultiple(false);
$form->setAllowEdit(false);
$form->setShowExpiration(false);
$form->setExpires(0);
$form->setIsAnonymous(false);
Expand Down Expand Up @@ -1296,7 +1297,7 @@
continue;
}

$this->storeAnswersForQuestion($form, $submission->getId(), $questions[$questionIndex], $answerArray);
$this->storeAnswersForQuestion($form, $submission->getId(), $questions[$questionIndex], $answerArray, false);
}

$this->formMapper->update($form);
Expand All @@ -1311,6 +1312,87 @@
return new DataResponse(null, Http::STATUS_CREATED);
}

/**
* Update an existing submission
*
* @param int $formId the form id
* @param int $submissionId the submission id
* @param array<string, list<string>> $answers [question_id => arrayOfString]
* @param string $shareHash public share-hash -> Necessary to submit on public link-shares.
* @return DataResponse<Http::STATUS_OK, int, array{}>
* @throws OCSBadRequestException Can only update submission if AllowEdit is set and the answers are valid
* @throws OCSForbiddenException Can only update your own submission
*
* 200: the id of the updated submission
*/
#[CORS()]
#[NoAdminRequired()]
#[NoCSRFRequired()]
#[PublicPage()]
#[ApiRoute(verb: 'PUT', url: '/api/v3/forms/{formId}/submissions/{submissionId}')]
public function updateSubmission(int $formId, int $submissionId, array $answers, string $shareHash = ''): DataResponse {
$this->logger->debug('Updating submission: formId: {formId}, answers: {answers}, shareHash: {shareHash}', [
'formId' => $formId,
'answers' => $answers,
'shareHash' => $shareHash,
]);

$form = $this->loadFormForSubmission($formId, $shareHash);

if (!$form->getAllowEdit()) {
throw new OCSBadRequestException('Can only update if AllowEdit is set');

Check warning on line 1343 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1343

Added line #L1343 was not covered by tests
}

$questions = $this->formsService->getQuestions($formId);
// Is the submission valid
$isSubmissionValid = $this->submissionService->validateSubmission($questions, $answers, $form->getOwnerId());
if (is_string($isSubmissionValid)) {
throw new OCSBadRequestException($isSubmissionValid);

Check warning on line 1350 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1350

Added line #L1350 was not covered by tests
}
if ($isSubmissionValid === false) {
throw new OCSBadRequestException('At least one submitted answer is not valid');

Check warning on line 1353 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1353

Added line #L1353 was not covered by tests
}

// get existing submission of this user
try {
$submission = $this->submissionMapper->findByFormAndUser($form->getId(), $this->currentUser->getUID());
} catch (DoesNotExistException $e) {
throw new OCSBadRequestException('Cannot update a non existing submission');

Check warning on line 1360 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1359-L1360

Added lines #L1359 - L1360 were not covered by tests
}

if ($submissionId != $submission->getId()) {
throw new OCSForbiddenException('Can only update your own submissions');

Check warning on line 1364 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1364

Added line #L1364 was not covered by tests
}

$submission->setTimestamp(time());
$this->submissionMapper->update($submission);

if (empty($answers)) {
// Clear Answers
foreach ($questions as $question) {
$this->storeAnswersForQuestion($form, $submission->getId(), $question, [''], true);

Check warning on line 1373 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1372-L1373

Added lines #L1372 - L1373 were not covered by tests
}
} else {
// Process Answers
foreach ($answers as $questionId => $answerArray) {
// Search corresponding Question, skip processing if not found
$questionIndex = array_search($questionId, array_column($questions, 'id'));
if ($questionIndex === false) {
continue;
}

$question = $questions[$questionIndex];

$this->storeAnswersForQuestion($form, $submission->getId(), $question, $answerArray, true);
}
}

//Create Activity
$this->formsService->notifyNewSubmission($form, $submission);

return new DataResponse($submissionId);
}

/**
* Delete a specific submission
*
Expand Down Expand Up @@ -1524,14 +1606,23 @@
// private functions

/**
* Insert answers for a question
* Insert or update answers for a question
*
* @param Form $form
* @param int $submissionId
* @param array $question
* @param string[]|array<array{uploadedFileId: string, uploadedFileName: string}> $answerArray
* @param bool $update
*/
private function storeAnswersForQuestion(Form $form, $submissionId, array $question, array $answerArray): void {
private function storeAnswersForQuestion(Form $form, int $submissionId, array $question, array $answerArray, bool $update): void {
// get stored answers for this question
$storedAnswers = [];
if ($update) {
$storedAnswers = $this->answerMapper->findBySubmissionAndQuestion($submissionId, $question['id']);
}

$newAnswerTexts = [];

foreach ($answerArray as $answer) {
$answerEntity = new Answer();
$answerEntity->setSubmissionId($submissionId);
Expand All @@ -1548,6 +1639,33 @@
} elseif (!empty($question['extraSettings']['allowOtherAnswer']) && strpos($answer, Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX) === 0) {
$answerText = str_replace(Constants::QUESTION_EXTRASETTINGS_OTHER_PREFIX, '', $answer);
}

if (!array_key_exists($question['id'], $newAnswerTexts)) {
$newAnswerTexts[$question['id']] = [];
}
$newAnswerTexts[$question['id']][] = $answerText;

// has this answer already been stored?
$foundAnswer = false;
foreach ($storedAnswers as $storedAnswer) {
if ($storedAnswer->getText() == $answerText) {

Check warning on line 1651 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1651

Added line #L1651 was not covered by tests
// nothing to be changed
$foundAnswer = true;
break;

Check warning on line 1654 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1653-L1654

Added lines #L1653 - L1654 were not covered by tests
}
}
if (!$foundAnswer) {
if ($answerText === '') {
continue;
}
// need to add answer
$answerEntity = new Answer();
$answerEntity->setSubmissionId($submissionId);
$answerEntity->setQuestionId($question['id']);
$answerEntity->setText($answerText);
$this->answerMapper->insert($answerEntity);
}

} elseif ($question['type'] === Constants::ANSWER_TYPE_FILE) {
$uploadedFile = $this->uploadedFileMapper->getByUploadedFileId($answer['uploadedFileId']);
$answerEntity->setFileId($uploadedFile->getFileId());
Expand All @@ -1567,20 +1685,43 @@
$file->move($folder->getPath() . '/' . $name);

$answerText = $name;

$answerEntity->setText($answerText);
$this->answerMapper->insert($answerEntity);
} else {
$answerText = $answer; // Not a multiple-question, answerText is given answer
}

if ($answerText === '') {
continue;
if (!empty($storedAnswers)) {
$answerEntity = $storedAnswers[0];
$answerEntity->setText($answerText);
$this->answerMapper->update($answerEntity);

Check warning on line 1697 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1695-L1697

Added lines #L1695 - L1697 were not covered by tests
} else {
if ($answerText === '') {
continue;

Check warning on line 1700 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1700

Added line #L1700 was not covered by tests
}
$answerEntity = new Answer();
$answerEntity->setSubmissionId($submissionId);
$answerEntity->setQuestionId($question['id']);
$answerEntity->setText($answerText);
$this->answerMapper->insert($answerEntity);
}
}

$answerEntity->setText($answerText);
$this->answerMapper->insert($answerEntity);
if ($uploadedFile) {
$this->uploadedFileMapper->delete($uploadedFile);
}
}

if (in_array($question['type'], Constants::ANSWER_TYPES_PREDEFINED)) {
// drop all answers that are not in new set of answers
foreach ($storedAnswers as $storedAnswer) {
$questionId = $storedAnswer->getQuestionId();

Check warning on line 1718 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1718

Added line #L1718 was not covered by tests

if (empty($newAnswerTexts[$questionId]) || !in_array($storedAnswer->getText(), $newAnswerTexts[$questionId])) {
$this->answerMapper->delete($storedAnswer);

Check warning on line 1721 in lib/Controller/ApiController.php

View check run for this annotation

Codecov / codecov/patch

lib/Controller/ApiController.php#L1720-L1721

Added lines #L1720 - L1721 were not covered by tests
}
}
}
}

private function loadFormForSubmission(int $formId, string $shareHash): Form {
Expand Down
20 changes: 20 additions & 0 deletions lib/Db/AnswerMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,26 @@
return $this->findEntities($qb);
}

/**
* @param int $submissionId
* @param int $questionId
* @throws \OCP\AppFramework\Db\DoesNotExistException if not found
* @return Answer[]
*/

public function findBySubmissionAndQuestion(int $submissionId, int $questionId): array {
$qb = $this->db->getQueryBuilder();

Check warning on line 52 in lib/Db/AnswerMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/AnswerMapper.php#L51-L52

Added lines #L51 - L52 were not covered by tests

$qb->select('*')
->from($this->getTableName())
->where(
$qb->expr()->eq('submission_id', $qb->createNamedParameter($submissionId, IQueryBuilder::PARAM_INT)),
$qb->expr()->eq('question_id', $qb->createNamedParameter($questionId, IQueryBuilder::PARAM_INT))
);

Check warning on line 59 in lib/Db/AnswerMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/AnswerMapper.php#L54-L59

Added lines #L54 - L59 were not covered by tests

return $this->findEntities($qb);

Check warning on line 61 in lib/Db/AnswerMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/AnswerMapper.php#L61

Added line #L61 was not covered by tests
}

/**
* @param int $submissionId
*/
Expand Down
6 changes: 6 additions & 0 deletions lib/Db/Form.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
* @method void setIsAnonymous(bool $value)
* @method int getSubmitMultiple()
* @method void setSubmitMultiple(bool $value)
* @method int getAllowEdit()
* @method void setAllowEdit(bool $value)
* @method int getShowExpiration()
* @method void setShowExpiration(bool $value)
* @method int getLastUpdated()
Expand All @@ -58,6 +60,7 @@ class Form extends Entity {
protected $expires;
protected $isAnonymous;
protected $submitMultiple;
protected $allowEdit;
protected $showExpiration;
protected $submissionMessage;
protected $lastUpdated;
Expand All @@ -71,6 +74,7 @@ public function __construct() {
$this->addType('expires', 'integer');
$this->addType('isAnonymous', 'boolean');
$this->addType('submitMultiple', 'boolean');
$this->addType('allowEdit', 'boolean');
$this->addType('showExpiration', 'boolean');
$this->addType('lastUpdated', 'integer');
$this->addType('state', 'integer');
Expand Down Expand Up @@ -140,6 +144,7 @@ public function setAccess(array $access): void {
* expires: int,
* isAnonymous: bool,
* submitMultiple: bool,
* allowEdit: bool,
* showExpiration: bool,
* lastUpdated: int,
* submissionMessage: ?string,
Expand All @@ -160,6 +165,7 @@ public function read() {
'expires' => (int)$this->getExpires(),
'isAnonymous' => (bool)$this->getIsAnonymous(),
'submitMultiple' => (bool)$this->getSubmitMultiple(),
'allowEdit' => (bool)$this->getAllowEdit(),
'showExpiration' => (bool)$this->getShowExpiration(),
'lastUpdated' => (int)$this->getLastUpdated(),
'submissionMessage' => $this->getSubmissionMessage(),
Expand Down
23 changes: 23 additions & 0 deletions lib/Db/SubmissionMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,29 @@
return $this->findEntities($qb);
}

/**
* @param int $formId
* @param string $userId
*
* @return Submission
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException if more than one result
* @throws \OCP\AppFramework\Db\DoesNotExistException if not found
*/
public function findByFormAndUser(int $formId, string $userId): Submission {
$qb = $this->db->getQueryBuilder();

Check warning on line 59 in lib/Db/SubmissionMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/SubmissionMapper.php#L58-L59

Added lines #L58 - L59 were not covered by tests

$qb->select('*')
->from($this->getTableName())
->where(
$qb->expr()->eq('form_id', $qb->createNamedParameter($formId, IQueryBuilder::PARAM_INT)),
$qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))
)
//Newest submissions first
->orderBy('timestamp', 'DESC');

Check warning on line 68 in lib/Db/SubmissionMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/SubmissionMapper.php#L61-L68

Added lines #L61 - L68 were not covered by tests

return $this->findEntity($qb);

Check warning on line 70 in lib/Db/SubmissionMapper.php

View check run for this annotation

Codecov / codecov/patch

lib/Db/SubmissionMapper.php#L70

Added line #L70 was not covered by tests
}

/**
* @param int $id
* @return Submission
Expand Down
1 change: 1 addition & 0 deletions lib/FormsMigrator.php
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ public function import(IUser $user, IImportSource $importSource, OutputInterface
$form->setExpires($formData['expires']);
$form->setIsAnonymous($formData['isAnonymous']);
$form->setSubmitMultiple($formData['submitMultiple']);
$form->setAllowEdit($formData['allowEdit']);
$form->setShowExpiration($formData['showExpiration']);

$this->formMapper->insert($form);
Expand Down
42 changes: 42 additions & 0 deletions lib/Migration/Version050000Date20250109201500.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Forms\Migration;

use Closure;
use OCP\DB\ISchemaWrapper;
use OCP\DB\Types;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;

class Version050000Date20250109201500 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 {

Check warning on line 26 in lib/Migration/Version050000Date20250109201500.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version050000Date20250109201500.php#L26

Added line #L26 was not covered by tests
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();
$table = $schema->getTable('forms_v2_forms');

Check warning on line 29 in lib/Migration/Version050000Date20250109201500.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version050000Date20250109201500.php#L28-L29

Added lines #L28 - L29 were not covered by tests

if (!$table->hasColumn('allow_edit')) {
$table->addColumn('allow_edit', Types::BOOLEAN, [
'notnull' => false,
'default' => 0,
]);

Check warning on line 35 in lib/Migration/Version050000Date20250109201500.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version050000Date20250109201500.php#L31-L35

Added lines #L31 - L35 were not covered by tests

return $schema;

Check warning on line 37 in lib/Migration/Version050000Date20250109201500.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version050000Date20250109201500.php#L37

Added line #L37 was not covered by tests
}

return null;

Check warning on line 40 in lib/Migration/Version050000Date20250109201500.php

View check run for this annotation

Codecov / codecov/patch

lib/Migration/Version050000Date20250109201500.php#L40

Added line #L40 was not covered by tests
}
}
4 changes: 4 additions & 0 deletions lib/ResponseDefinitions.php
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
* isAnonymous: bool,
* lastUpdated: int,
* submitMultiple: bool,
* allowEdit: bool,
* showExpiration: bool,
* canSubmit: bool,
* permissions: list<FormsPermission>,
Expand All @@ -119,6 +120,9 @@
* shares: list<FormsShare>,
* submissionCount?: int,
* submissionMessage: ?string,
* answers?: array<string,mixed>,
* newSubmission?: bool,
* submissionId?: int,
* }
*
* @psalm-type FormsUploadedFile = array{
Expand Down
Loading
Loading