Wip.email multi event #1496
Wip.email multi event #1496
Conversation
… wip.email-multi-event
…erent email clients and browsers
There was a problem hiding this comment.
Changes introduced in this PR,
- split email html into smaller components using ejs
- separated email template code, email sending code, email content gathering code, and event specific code (massive respect for this refactor)
- rename
startDatetoissuedDate - change in definition for finished events
getEventsSummaryTriggerFinishedMailandgetFinishedEvent
Suggestions,
Note for future refactors: Let's keep the new features in a separate PR from the refactor. If there are features added or removed, it's impossible to track if we've lost some functionality.
Would it be possible to document the notifications logic in a flow chart? Feel free to commit the flow chart image in the notifications folder. So it can be used as documentation coupled with the code.
TLDR; It looks okay - I haven't executed any code in this review. See comments for the merge blocker.
services/API-service/src/api/notification/notification-content/notification-content.service.ts
Outdated
Show resolved
Hide resolved
services/API-service/src/api/notification/notification-content/notification-content.service.ts
Outdated
Show resolved
Hide resolved
services/API-service/src/api/notification/email/email-template.service.ts
Show resolved
Hide resolved
services/API-service/src/api/notification/email/email-template.service.ts
Outdated
Show resolved
Hide resolved
services/API-service/src/api/notification/email/email-template.service.ts
Outdated
Show resolved
Hide resolved
| public async getEventsSummaryTriggerFinishedMail( | ||
| countryCodeISO3: string, | ||
| disasterType: DisasterType, | ||
| ): Promise<EventSummaryCountry[]> { | ||
| const countryAdminAreaIds = await this.getCountryAdminAreaIds( | ||
| countryCodeISO3, | ||
| ); | ||
|
|
||
| // I spend quite some time on trying to figure out what is the right query to get the event finished summary for the trigger closed email | ||
| // I came up with the following query but I am not sure if it is correct and how to test it properly | ||
| const sevenDaysAgo = new Date(Date.now() - 7 * 24 * 60 * 60 * 1000); | ||
| const sixDaysAgo = new Date(Date.now() - 6 * 24 * 60 * 60 * 1000); | ||
| const whereFilters = { | ||
| endDate: Between(sevenDaysAgo, sixDaysAgo), | ||
| adminArea: In(countryAdminAreaIds), | ||
| disasterType: disasterType, | ||
| closed: true, | ||
| }; |
There was a problem hiding this comment.
the logic here is different from getFinishedEvent. I suggest to confirm this new behaviour with the team before merge. Bcoz we've discussed this I agree your interpretation, the only thing that strongly recommend is to keep the check binary instead of a range. i.e. use LessThan instead of Between.
The reason for this recommendation is that for a binary condition - we have two logical pathways. It is either LessThan or MoreThanOrEqual. If we use Between we have three logical pathways. It is either LessThan, MoreThan, or Between. This makes the code more complex and harder to understand.
There was a problem hiding this comment.
the logic here is different from getFinishedEvent. I suggest to confirm this new behaviour with the team before merge. Bcoz we've discussed this I agree your interpretation, the only thing that strongly recommend is to keep the check binary instead of a range. i.e. use LessThan instead of Between.
I did not get to checking this with the rest of the team unfortunately
services/API-service/src/api/notification/notification.service.ts
Outdated
Show resolved
Hide resolved
|
Thanks for the review! Great that you summarized the changes!
I get this, and agree that it is sub-optimal
Sound like a good idea. I am creating an extra task for this on the task board AB#28230.
I also created a separate task for this AB#28231. That probably would have been a better approach from the start. However considering the limit time to complete this work I'd rather first finish the API tests before implementing this. |
…erent email clients and browsers
No description provided.