HOFF-869 -Refactor notify component to enable email attachments #472
Conversation
There was a problem hiding this comment.
Looks good , just a couple of suggestions (see comments). Could you also please :
- Add the jira ticket number in the commit?
- Use the new pr description template for your pr description:
What
Why
How
Testing
Screenshots(optional)
Anything else(optional)
Check list
- I have reviewed my own pull request for linting issues (e.g. adding new lines)
- I have written tests (if relevant)
- I have created a JIRA number for my branch
- I have created a JIRA number for my commit
- I have followed the chris beams method for my commit https://cbea.ms/git-commit/
here is an example commit - Ensure drone builds are green especially tests
- I will squash the commits before merging
components/notify/notify.js
Outdated
| personalisation: personalisation, | ||
| reference }); | ||
| } catch (error) { | ||
| console.error('Error sending email:', error.response ? error.response.data : error.message); |
There was a problem hiding this comment.
Could we use hof's built in logger here? It provides more info when logging than console.error
There was a problem hiding this comment.
I have refactored to use the hof logger component
components/notify/notify.js
Outdated
| send(email) { | ||
| let personalisation = { | ||
| 'email-subject': email.subject, | ||
| 'email-body': email.body | ||
| }; | ||
|
|
||
| if (email.attachment && email.attachment !== undefined) { | ||
| personalisation = Object.assign({'email-attachment': | ||
| this.notifyClient.prepareUpload(email.attachment)}, personalisation); | ||
| } | ||
|
|
||
| this.sendEmail(this.notifyTemplate, email.recipient, personalisation); |
There was a problem hiding this comment.
I think it would be good to make this function async as well and to use a try/catch block
There was a problem hiding this comment.
I have changed it to async
| console.error('Error sending email:', error.response ? error.response.data : error.message); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
should we also have a separate sendEmailWithAttachment function?
There was a problem hiding this comment.
Did some investigation and it is will good but it is out of scope for this bit of work - Please raise it with the project team, thanks
What
Refactor notify component to enable email attachments. HOFF-869
Why
The notify component in the hof framework is not enabled to send attachments with gov.notify emails, which is a requirement of the ROTM service, please HOFF-814.
How
Refactor the code to upload an attachment, without affecting other services that using the notify component to send emails.
Test
Test on two or more services using the hof notify component in local and branch
Screenshots(optional)
Anything else(optional)
Check list