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.

Sign up for GitHub

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

Jump to bottom

feat: backend preparation for sending email forms #1458

Merged
merged 8 commits into from
Oct 7, 2024
Merged

feat: backend preparation for sending email forms #1458

merged 8 commits into from
Oct 7, 2024

Conversation

erehulka
Copy link
Collaborator

@erehulka erehulka commented Sep 26, 2024 •
edited
Loading

Adds a backend support for handling email forms:

  • If the form definition is email, it skips the NASES flow and moves into a new functionality
  • Sends an email to the recipient of the emails from the form definition
  • Send an confirmation email to the user after the form has been successfully delivered to the recipient

So far there are some TODOs:

  • Email templates
  • Paths to emails in form data from email form definitions, when the form has no email set in database (is sent by user which is not logged in)

@erehulka erehulka marked this pull request as draft September 30, 2024 06:27
@erehulka erehulka force-pushed the feat/email-forms-send branch 3 times, most recently from a71a4b5 to d090d79 Compare September 30, 2024 08:59
@erehulka erehulka changed the title WIP: email forms feat: backend preparation for sending email forms Sep 30, 2024
@erehulka erehulka marked this pull request as ready for review September 30, 2024 09:01
@erehulka erehulka marked this pull request as draft September 30, 2024 09:01
@erehulka erehulka marked this pull request as ready for review September 30, 2024 11:44
@erehulka erehulka marked this pull request as draft October 1, 2024 09:27
@erehulka erehulka marked this pull request as ready for review October 1, 2024 11:59
mpinter
mpinter requested changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@mpinter mpinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these are up for discussion.

nest-forms-backend/src/forms/forms.helper.ts Outdated Show resolved Hide resolved
nest-forms-backend/src/forms/forms.helper.ts Outdated Show resolved Hide resolved
nest-forms-backend/src/nases/nases.service.ts Outdated Show resolved Hide resolved
nest-forms-backend/src/nases/nases.service.ts Outdated Show resolved Hide resolved
nest-forms-backend/src/utils/subservices/email-forms.subservice.ts Outdated Show resolved Hide resolved
nest-forms-backend/src/nases-consumer/nases-consumer.service.ts
alertError(
`ERROR onQueueConsumption: UnprocessableEntityException - ${FormsErrorsResponseEnum.FORM_DEFINITION_NOT_SUPPORTED_TYPE} ${form.formDefinitionSlug}.`,
this.logger,
)
return new Nack(false)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logger debug below could probably be rephrased and used before line 116

it would also be nice to wrap the logic below this into function such as handleEmailForm (handle slovenskSkForm ?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it has to be used after the email check, if used before then Email forms would also throw errors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant this log

this.logger.debug(
      `All files are in final state, sending message to nases for formId: ${data.formId}`,
    )

am I missing something. I don't see that line throwing errors if moved earlier, and I guess the idea behind that log is that file check is done at that point and the queue procedes withg sending. It just needs rephrasing the 'nases' part

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I understand. I have rephrased it and moved few lines back.

nest-forms-backend/src/utils/subservices/email-forms.subservice.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added pr: needs work 🛠️ Changes requested before another review pr: needs review 🙏 and removed pr: needs review 🙏 labels Oct 2, 2024
@erehulka erehulka requested a review from mpinter October 3, 2024 08:59
mpinter
mpinter approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@mpinter mpinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the couple comments & LGTM 👍

Originally wanted to do more with isUserVerified, but we can move that forward along with some more cleanup, will leave you a slack message.

nest-forms-backend/src/nases-consumer/subservices/email-forms.subservice.ts
Comment on lines +126 to +130
alertError(
`Sending confirmation email to ${toEmail} for form ${formId} failed.`,
this.logger,
JSON.stringify(error),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we likely want an alert even when toEmail is not defined - so that we notice if this happens a lot / happens when it should not

nest-forms-backend/src/nases/nases.service.ts Outdated
Comment on lines 311 to 334
if (
!formDefinition.allowSendingByUnverifiedUsers &&
!this.isUserVerified(user)
) {
throw this.throwerErrorGuard.ForbiddenException(
NasesErrorsEnum.SEND_UNVERIFIED,
NasesErrorsResponseEnum.SEND_UNVERIFIED,
)
}

if (
!this.formsHelper.userCanSendForm(
form,
formDefinition.allowSendingByUnverifiedUsers ?? false,
userInfo,
user?.sub,
)
) {
throw this.throwerErrorGuard.ForbiddenException(
NasesErrorsEnum.FORBIDDEN_SEND,
NasesErrorsResponseEnum.FORBIDDEN_SEND,
)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two must come before form vallidation - it's a minor (security) concern, but without these checks first you could get validation data for a form you don't own, which may contain user data

@github-actions github-actions bot added pr: fix & ship 🚢 No additional review needed before merge - some work may be required, if specified in last review and removed pr: needs review 🙏 pr: needs work 🛠️ Changes requested before another review labels Oct 3, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 7, 2024 •
edited
Loading

Test build pipeline info 🚀

Changes in the code and tag info:

➡️ Changes in forms-shared: true

➡️ Changes in next: false

➡️ Changes in strapi: false

➡️ Changes in nest-forms-backend: true

➡️ Changes in nest-clamav-scanner: false

We are going to build 🚢

🔜 forms-shared together with nest-forms-backend and next as those needs to be rebuild when forms-shared are changed

🔜 nest-forms-backend

@erehulka erehulka merged commit d2ab27a into master Oct 7, 2024
16 of 18 checks passed
@erehulka erehulka deleted the feat/email-forms-send branch October 7, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpinter mpinter mpinter approved these changes

Assignees
No one assigned
Labels
pr: fix & ship 🚢 No additional review needed before merge - some work may be required, if specified in last review pr: needs review 🙏
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erehulka @mpinter