-
Notifications
You must be signed in to change notification settings - Fork 5
2743: Note is not included in confirmation of receipt #2746
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
base: main
Are you sure you want to change the base?
2743: Note is not included in confirmation of receipt #2746
Conversation
b9689d3 to
c8871a0
Compare
|
Just a general question to mailDev. |
f1sh1918
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The related issue is fixed! 👍 Good work
Maybe some additional testing instructions would be good
I think its important to check that a multiline note with line breaks work, because just a single sentence did work before afaics. Just to ensure testers will test this issue properly.
| RegionsRepository.findRegionById(regionId).apply { | ||
| this.activatedForApplication = activatedForApplication | ||
| activatedForCardConfirmationMail = activatedForConfirmationMail | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RegionsRepository.findRegionById(regionId).apply { | |
| this.activatedForApplication = activatedForApplication | |
| activatedForCardConfirmationMail = activatedForConfirmationMail | |
| } | |
| Regions.update({ Regions.id eq regionId }) { | |
| it[this.activatedForApplication] = activatedForApplication | |
| it[activatedForCardConfirmationMail] = activatedForConfirmationMail | |
| } |
🙃 can be changed to 1 query instead of 2
but it would be nice to check the return value in this case (updated == 1) to make sure the region was found
@f1sh1918 I think the |
seluianova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Okay then I don't understand the docker config since there is the incoming user mapped. |
Short Description
The regions
applicationConfirmationMailNoteis not included in the initial applicant mail. This is because our mailer DSL is very error prone. When not including the initial unary+method in theParagraphbuilder block, the DSL will silently drop a text fragment. Additionally, there was an error in theParagraph.plain()builder method.Proposed Changes
maildevservice to the docker-compose file, for easier mail debugging.RegionRepositoryused inRegionMutationController, which where pointless. Also pull out code from transactions that does not need to execute in a transaction.t()method to theParagraphbuilder class and use it instead of an unary+method.Paragraph.plain().Open questions
The text for the notification is mixed into the standard text we generate by default. The way regions seem to use this feature, creates really weird text flows with multiple salutations, a kind of mail in mail situation.
Testing
Resolved Issues
Fixes: #2743