fix: add try catch condition in handlebar template compilation #2317
Conversation
jainpawan21
requested review from
davidsoderberg and
scopsy
and removed request for
davidsoderberg
| } catch (error) { | ||
| return html; | ||
| } |
There was a problem hiding this comment.
I would actually not return the html here, but throw an ApiError and pass the handlebars content as information there. This error could be watched from the calling function in send-message.email level and than create the execution logs with the actual handlebars error.
| await this.createExecutionDetails.execute( | ||
| CreateExecutionDetailsCommand.create({ | ||
| ...CreateExecutionDetailsCommand.getDetailsFromJob(command.job), | ||
| detail: DetailEnum.MESSAGE_CONTENT_SYNTAX_ERROR, | ||
| source: ExecutionDetailsSourceEnum.INTERNAL, | ||
| status: ExecutionDetailsStatusEnum.FAILED, | ||
| messageId: message._id, | ||
| isTest: false, | ||
| isRetry: false, | ||
| raw: JSON.stringify(payload), | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Here we need to throw an error and don't continue the flow execution since the recipient will get broken template
| @@ -209,6 +209,21 @@ export class SendMessageEmail extends SendMessageType { | |||
| }) | |||
| ); | |||
|
|
|||
| if (!html) { | |||
| await this.createExecutionDetails.execute( | |||
There was a problem hiding this comment.
Combining with my comment from above regarding catching in the actual send message usecase and not compile template. We need to add this execution detail to all channels, so maybe consider moving this catching to one level above?
|
Fix @scopsy comments and I think it is ready to merge :) |
|
@scopsy shifted try catch one level above and added handlebar error in execution details |
|
Hi @jainpawan21 , correct me if I'm wrong, but this still won't apply to the other channels, right? Like sms, in app, push etc |
Yes, You are right, it will handle only email syntax error |
|
I've actually been working on something similar before I noticed your PR 🙈 Would you mind if I join you on this one and push some of my changes? @jainpawan21 |
Yes sure Gali |
|
@jainpawan21 I added the try catch to also be on the first render&compile (not only on the html). And also to wrap the compiles for subject, preheader, cta etc . Also changed it for all channels. |
|
@ainouzgali what do you think also adding a validation on the client side? So we can even skip the server request if the compile message fails on the client? |
|
@scopsy Yes, I definitely want that. I've started looking into that, but it is not as straight forward. I will continue at the next cool down. Or, we could open it for the community, in case someone will be available to help on this. |
What change does this PR introduce?
Add try catch condition for handlebars template compilation
Add new detail enum value
Why was this change needed?
Closes #2229
Other information (Screenshots)