Skip to content

Commit 15e1a4e

Browse files
dtretyakovdtretyakov
committed
refactor: move confirmation email recipient to Form entity
- Relocate confirmationEmailRecipient from Question extraSettings to Form - Centralize email question validation in Question model and FormsService - Simplify SettingsSidebarTab component logic - Ensure correct recipient ID mapping during form cloning - Add comprehensive unit tests for validation and notification logic
1 parent 61f3e69 commit 15e1a4e

15 files changed

Lines changed: 587 additions & 197 deletions

File tree

‎docs/API_v3.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ Returns the full-depth object of the requested form (without submissions).
178178
"confirmationEmailEnabled": false,
179179
"confirmationEmailSubject": null,
180180
"confirmationEmailBody": null,
181+
"confirmationEmailRecipient": null,
181182
"permissions": [
182183
"edit",
183184
"results",

‎docs/DataStructure.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ This document describes the Object-Structure, that is used within the Forms App
2424
| confirmationEmailEnabled | Boolean | | If enabled, send a confirmation email to the respondent after submission |
2525
| confirmationEmailSubject | String | max. 255 ch. | Optional confirmation email subject template (supports placeholders) |
2626
| confirmationEmailBody | String | | Optional confirmation email body template (plain text, supports placeholders) |
27+
| confirmationEmailRecipient | Integer | | The id of the question belonging to the form, that holds the email address of the respondent |
2728
| created | unix timestamp | | When the form has been created |
2829
| access | [Access-Object](#access-object) | | Describing access-settings of the form |
2930
| expires | unix-timestamp | | When the form should expire. Timestamp `0` indicates _never_ |
@@ -52,6 +53,7 @@ This document describes the Object-Structure, that is used within the Forms App
5253
"confirmationEmailEnabled": false,
5354
"confirmationEmailSubject": null,
5455
"confirmationEmailBody": null,
56+
"confirmationEmailRecipient": null,
5557
"created": 1611240961,
5658
"access": {},
5759
"expires": 0,

‎lib/Constants.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,6 @@ class Constants {
149149
];
150150

151151
public const EXTRA_SETTINGS_SHORT = [
152-
'confirmationEmailRecipient' => ['boolean'],
153152
'validationType' => ['string'],
154153
'validationRegex' => ['string'],
155154
];

‎lib/Controller/ApiController.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,12 +208,15 @@ public function newForm(?int $fromId = null): DataResponse {
208208
$formData['showExpiration'] = false;
209209
$formData['expires'] = 0;
210210
$formData['isAnonymous'] = false;
211+
$formData['confirmationEmailRecipient'] = null;
211212

212213
$form = Form::fromParams($formData);
213214
$this->formMapper->insert($form);
214215

215216
// Get Questions, set new formId, reinsert
216217
$questions = $this->questionMapper->findByForm($oldForm->getId());
218+
$oldConfirmationEmailRecipient = $oldForm->getConfirmationEmailRecipient();
219+
217220
foreach ($questions as $oldQuestion) {
218221
$questionData = $oldQuestion->read();
219222

@@ -222,6 +225,12 @@ public function newForm(?int $fromId = null): DataResponse {
222225
$newQuestion = Question::fromParams($questionData);
223226
$this->questionMapper->insert($newQuestion);
224227

228+
// Map the confirmation email recipient if this question matches
229+
if ($oldConfirmationEmailRecipient === $oldQuestion->getId()) {
230+
$form->setConfirmationEmailRecipient($newQuestion->getId());
231+
$this->formMapper->update($form);
232+
}
233+
225234
// Get Options, set new QuestionId, reinsert
226235
$options = $this->optionMapper->findByQuestion($oldQuestion->getId());
227236
foreach ($options as $oldOption) {
@@ -335,6 +344,14 @@ public function updateForm(int $formId, array $keyValuePairs): DataResponse {
335344
unset($keyValuePairs['fileId']);
336345
unset($keyValuePairs['fileFormat']);
337346

347+
if (array_key_exists('confirmationEmailRecipient', $keyValuePairs)) {
348+
try {
349+
$this->formsService->validateConfirmationEmailRecipient($form, $keyValuePairs['confirmationEmailRecipient']);
350+
} catch (\InvalidArgumentException $e) {
351+
throw new OCSBadRequestException('Invalid confirmationEmailRecipient, will not update.');
352+
}
353+
}
354+
338355
// Create FormEntity with given Params & Id.
339356
foreach ($keyValuePairs as $key => $value) {
340357
$method = 'set' . ucfirst($key);
@@ -651,6 +668,11 @@ public function updateQuestion(int $formId, int $questionId, array $keyValuePair
651668
throw new OCSBadRequestException('Invalid extraSettings, will not update.');
652669
}
653670

671+
if ($form->getConfirmationEmailRecipient() === $question->getId()
672+
&& !$question->isEmailType($keyValuePairs['type'] ?? null, $keyValuePairs['extraSettings'] ?? null)) {
673+
$form->setConfirmationEmailRecipient(null);
674+
}
675+
654676
// Create QuestionEntity with given Params & Id.
655677
$question = Question::fromParams($keyValuePairs);
656678
$question->setId($questionId);
@@ -722,6 +744,10 @@ public function deleteQuestion(int $formId, int $questionId): DataResponse {
722744
}
723745
}
724746

747+
if ($form->getConfirmationEmailRecipient() === $questionId) {
748+
$form->setConfirmationEmailRecipient(null);
749+
}
750+
725751
$this->formMapper->update($form);
726752

727753
return new DataResponse($questionId);

‎lib/Db/Question.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,4 +100,22 @@ public function read(): array {
100100
'extraSettings' => $this->getExtraSettings(),
101101
];
102102
}
103+
104+
/**
105+
* Check if the question is a short text question with email validation.
106+
*/
107+
public function isEmailType(?string $type = null, ?array $extraSettings = null): bool {
108+
return self::isEmailTypeStatic(
109+
$type ?? $this->getType(),
110+
$extraSettings ?? $this->getExtraSettings()
111+
);
112+
}
113+
114+
/**
115+
* Static version of isEmailType to be used with arrays.
116+
*/
117+
public static function isEmailTypeStatic(string $type, array $extraSettings): bool {
118+
return $type === \OCA\Forms\Constants::ANSWER_TYPE_SHORT
119+
&& ($extraSettings['validationType'] ?? null) === 'email';
120+
}
103121
}

‎lib/FormsMigrator.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ public function export(IUser $user, IExportDestination $exportDestination, Outpu
6969
$formData['confirmationEmailEnabled'] ??= false;
7070
$formData['confirmationEmailSubject'] ??= null;
7171
$formData['confirmationEmailBody'] ??= null;
72+
$formData['confirmationEmailRecipient'] ??= null;
7273
$formData['questions'] = $this->formsService->getQuestions($formData['id']);
7374
$formData['submissions'] = $this->submissionService->getSubmissions($formData['id']);
7475

@@ -155,6 +156,7 @@ public function import(IUser $user, IImportSource $importSource, OutputInterface
155156
$form->setConfirmationEmailEnabled($formData['confirmationEmailEnabled'] ?? false);
156157
$form->setConfirmationEmailSubject($formData['confirmationEmailSubject'] ?? null);
157158
$form->setConfirmationEmailBody($formData['confirmationEmailBody'] ?? null);
159+
$form->setConfirmationEmailRecipient(null); // Set to null initially, updated after questions are imported
158160

159161
$this->formMapper->insert($form);
160162

@@ -183,6 +185,12 @@ public function import(IUser $user, IImportSource $importSource, OutputInterface
183185
}
184186
}
185187

188+
if (($formData['confirmationEmailRecipient'] ?? null) !== null
189+
&& isset($questionIdMap[$formData['confirmationEmailRecipient']])) {
190+
$form->setConfirmationEmailRecipient($questionIdMap[$formData['confirmationEmailRecipient']]);
191+
$this->formMapper->update($form);
192+
}
193+
186194
foreach ($formData['submissions'] as $submissionData) {
187195
$submission = new Submission();
188196
$submission->setFormId($form->getId());

‎lib/Migration/Version050300Date20260413233000.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,13 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt
5353
]);
5454
}
5555

56+
if (!$table->hasColumn('confirmation_email_recipient')) {
57+
$table->addColumn('confirmation_email_recipient', Types::INTEGER, [
58+
'notnull' => false,
59+
'default' => null,
60+
]);
61+
}
62+
5663
return $schema;
5764
}
5865
}

‎lib/ResponseDefinitions.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
* dateMax?: int,
2727
* dateMin?: int,
2828
* dateRange?: bool,
29-
* confirmationEmailRecipient?: bool,
3029
* maxAllowedFilesCount?: int,
3130
* maxFileSize?: int,
3231
* optionsHighest?: 2|3|4|5|6|7|8|9|10,

‎lib/Service/FormsService.php‎

Lines changed: 44 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -800,7 +800,7 @@ private function sendConfirmationEmail(Form $form, Submission $submission): void
800800
$answerMap[$questionId][] = $answer->getText();
801801
}
802802

803-
$recipientQuestion = $this->getConfirmationEmailRecipientQuestion($questions);
803+
$recipientQuestion = $this->getConfirmationEmailRecipientQuestion($form, $questions);
804804
if ($recipientQuestion === null) {
805805
$this->logger->debug('No confirmation email recipient question is available', [
806806
'formId' => $form->getId(),
@@ -893,49 +893,39 @@ private function sendConfirmationEmail(Form $form, Submission $submission): void
893893
* @param list<FormsQuestion> $questions
894894
* @return FormsQuestion|null
895895
*/
896-
private function getConfirmationEmailRecipientQuestion(array $questions): ?array {
897-
$emailQuestions = array_values(array_filter(
898-
$questions,
899-
fn (array $question): bool => $this->isConfirmationEmailQuestion($question),
900-
));
901-
902-
if ($emailQuestions === []) {
896+
private function getConfirmationEmailRecipientQuestion(Form $form, array $questions): ?array {
897+
$recipientQuestionId = $form->getConfirmationEmailRecipient();
898+
if ($recipientQuestionId === null) {
903899
return null;
904900
}
905901

906-
$explicitRecipients = array_values(array_filter(
907-
$emailQuestions,
908-
function (array $question): bool {
909-
$extraSettings = (array)($question['extraSettings'] ?? []);
910-
return !empty($extraSettings['confirmationEmailRecipient']);
911-
},
912-
));
902+
foreach ($questions as $questionData) {
903+
if (($questionData['id'] ?? null) !== $recipientQuestionId) {
904+
continue;
905+
}
913906

914-
if (count($explicitRecipients) === 1) {
915-
return $explicitRecipients[0];
916-
}
907+
if ($this->isConfirmationEmailQuestion($questionData)) {
908+
return $questionData;
909+
}
917910

918-
if (count($explicitRecipients) > 1) {
911+
$this->logger->debug('Configured confirmation email recipient question is invalid', [
912+
'formId' => $form->getId(),
913+
'recipientQuestionId' => $recipientQuestionId,
914+
]);
919915
return null;
920916
}
921917

922-
if (count($emailQuestions) === 1) {
923-
return $emailQuestions[0];
924-
}
925-
926918
return null;
927919
}
928920

929921
/**
930922
* @param FormsQuestion $question
931923
*/
932924
private function isConfirmationEmailQuestion(array $question): bool {
933-
if (($question['type'] ?? null) !== Constants::ANSWER_TYPE_SHORT) {
934-
return false;
935-
}
936-
937-
$extraSettings = (array)($question['extraSettings'] ?? []);
938-
return ($extraSettings['validationType'] ?? null) === 'email';
925+
return Question::isEmailTypeStatic(
926+
$question['type'] ?? '',
927+
(array)($question['extraSettings'] ?? [])
928+
);
939929
}
940930

941931
/**
@@ -1072,11 +1062,6 @@ public function areExtraSettingsValid(array $extraSettings, string $questionType
10721062

10731063
// Special handling of short input for validation
10741064
} elseif ($questionType === Constants::ANSWER_TYPE_SHORT) {
1075-
if (!empty($extraSettings['confirmationEmailRecipient'])
1076-
&& ($extraSettings['validationType'] ?? null) !== 'email') {
1077-
return false;
1078-
}
1079-
10801065
if (!isset($extraSettings['validationType'])) {
10811066
return true;
10821067
}
@@ -1221,4 +1206,29 @@ public function getTemporaryUploadedFilePath(Form $form, Question $question): st
12211206
private static function normalizeFileName(string $fileName): string {
12221207
return trim(str_replace(Constants::FILENAME_INVALID_CHARS, '-', $fileName));
12231208
}
1209+
1210+
/**
1211+
* @throws \InvalidArgumentException
1212+
*/
1213+
public function validateConfirmationEmailRecipient(Form $form, mixed $recipientId): void {
1214+
if ($recipientId === null) {
1215+
return;
1216+
}
1217+
1218+
if (!is_int($recipientId)) {
1219+
throw new \InvalidArgumentException('Invalid confirmationEmailRecipient');
1220+
}
1221+
1222+
try {
1223+
$question = $this->questionMapper->findById($recipientId);
1224+
} catch (IMapperException $e) {
1225+
throw new \InvalidArgumentException('Invalid confirmationEmailRecipient');
1226+
}
1227+
1228+
if ($question->getFormId() !== $form->getId()
1229+
|| $question->getOrder() === 0
1230+
|| !$question->isEmailType()) {
1231+
throw new \InvalidArgumentException('Invalid confirmationEmailRecipient');
1232+
}
1233+
}
12241234
}

‎openapi.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -494,9 +494,6 @@
494494
"dateRange": {
495495
"type": "boolean"
496496
},
497-
"confirmationEmailRecipient": {
498-
"type": "boolean"
499-
},
500497
"maxAllowedFilesCount": {
501498
"type": "integer",
502499
"format": "int64"

0 commit comments

Comments
 (0)