feat: Add confirmation email for form respondents#3098
feat: Add confirmation email for form respondents#3098dtretyakov wants to merge 2 commits intonextcloud:mainfrom
Conversation
|
@dtretyakov thanks for you contribution :) Please have a look at the failing checks and fix them :) |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
dtretyakov
force-pushed
the
main
branch
3 times, most recently
from
52d72a9 to
3f94699
Compare
|
@Chartman123 done, it seems that approval is required to re-run remaining checks |
dtretyakov
force-pushed
the
main
branch
6 times, most recently
from
e61e929 to
ec01c84
Compare
|
@Chartman123 the tests are green. I’m not sure who could help reviewing that to proceed further. |
|
@dtretyakov I'll have a closer look in the next days :) |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
@Chartman123 hopefully you are doing well. Do you need an additional information to proceed further with the review? |
|
@dtretyakov just being busy at the moment... the same applies to my co-maintainer. so just give us some more time. :) Some questions: what do you do if there's more than one e-mail field? also no error checking in the frontend if there's none. |
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
Looks quite clean but did not review properly yet.
Will do soon!
Chartman123
added
enhancement
Please use the configured system mail account for sending the mails for privacy reasons, but also for making sure the mail gets sent at all. Most mailservers will check if the sender's address corresponds to the logged on user. |
|
Please don't use merge commits, just rebase your branch on main :) |
|
@Chartman123 thank you for the review. I rebased the changes on top of the main branch.
Now we have the validation when no e-mail fields are present and multiple e-mail fields are available:
Done, now it uses system mail account |
|
@susnux or @Chartman123 do you have any other ideas to address before the merge? |
|
@dtretyakov Could you please move your changes to a branch other than |
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
Overall this makes sense to me, but from experience with other tables like this it turned out that its not ideal for scaling.
The problem is that many things in the table are only used by few tables but cause huge size of the table like "submission message" or from this PR "confirmation mail body".
Its not a problem introduced with this PR - so we can also do this later!
But just for some feedback here cc @Chartman123
I wonder if we should first - or later - split the table to get some "forms metadata" or similar so that we can put data not needed for every action there.
That would reduce database select latency a lot for large instances (e.g. this information is only needed on edit or insert submit but not for the majority of requests which would just load the form for submitting.
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
So for the pull request itself I think it is quite nice! Good work :)
Just about the email field I am not sure if we should not better explicitly let the creator flag the input field to use.
| } | ||
|
|
||
| $emailValue = $answerMap[$questionId][0]; | ||
| if ($this->mailer->validateMailAddress($emailValue)) { |
There was a problem hiding this comment.
Should rather use \OCP\Mail\IEmailValidator for validation
| // If no email found, cannot send confirmation | ||
| if (empty($recipientEmails)) { | ||
| $this->logger->debug('No valid email address found in submission for confirmation email', [ | ||
| 'formId' => $form->getId(), | ||
| 'submissionId' => $submission->getId(), | ||
| ]); | ||
| return; | ||
| } | ||
|
|
||
| if (count($recipientEmails) > 1) { | ||
| $this->logger->debug('Multiple email fields found in submission for confirmation email, using first match', [ | ||
| 'formId' => $form->getId(), | ||
| 'submissionId' => $submission->getId(), | ||
| 'emailCount' => count($recipientEmails), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
I am not sure if this is the best way to configure the mail.
I can imagine multiple valid scenarios where you ask for multiple e-mail addresses-
Maybe we can set a flag on such a question? So the form creator can configure which field to use?
There was a problem hiding this comment.
Added a dedicated flag, stored in extra settings
Could be part of the |
- Allow form creators to explicitly select which email field to use for confirmations via the `confirmationEmailRecipient` property. - Update `FormsService` to rely on the explicit recipient field when multiple email fields are present. - Display relevant warnings in the settings tab if recipient fields are misconfigured. - Replace basic email validation with Nextcloud's internal `\OCP\Mail\IEmailValidator`. - Expand unit tests to verify explicit recipient selection and multi-email field validation logic.
dtretyakov
force-pushed
the
main
branch
2 times, most recently
from
cdbd2d0 to
0620a2d
Compare
Implements issue nextcloud#525 - Send confirmation emails to form respondents after submission. Features: - Add confirmation email settings (enabled, subject, body) to Form entity. - Implement email sending with placeholder replacement: - {formTitle}, {formDescription} - Field placeholders based on name or label (e.g. {name}, {email}). - Allow form creators to explicitly select the recipient email field when multiple email fields are present. - Add UI in Settings sidebar to configure confirmation emails and recipient selection. - Replace basic email validation with Nextcloud's internal IEmailValidator. - Integration with activity notifications and background jobs for file syncing. Technical changes: - Database migration for new Form properties. - Enhanced FormsService with email sending logic and validation. - Extensive unit and integration tests covering the new functionality. - Updated API documentation and OpenAPI spec. Signed-off-by: Dmitry Tretyakov <dtretyakov@gmail.com>
@Chartman123 I created a draft PR with the same changes #3289
Added the dedicated flag |
|
@dtretyakov ok, so we can close this one? |
|
Closed in favor of #3289 |
3 participants
Implements issue #525 - Send confirmation emails to form respondents after submission.
Features:
Technical changes:
The feature requires at least one email-validated short text question in the form. Email sending failures are logged but don't break the submission process.