AB#16252 - Email notifications next #1005
Conversation
… emails and graphql resolvers
…ase 1), send individual in progress
* legacy styling implemented * Createdat, modifiedat, persisting --------- Co-authored-by: SianaDicheva <sianadicheva@gmail.com> Co-authored-by: Morgan Gowans <morgan.g@adappt.ai>
|
Hi @AntoineRelief , @MwanPygmay, @matheus-relief The following changes have been implemented:
|
AntoineRelief
changed the title
There was a problem hiding this comment.
Hi @HasnatL-Adapptlimited,
Review done there too. The only thing that seems possibly critical to me is the permissions issue.
The naming changes on DB structure should also be done as early as possible to avoid the need of migrations.
Please let me know if any of my comments is unclear.
All the rest looks mostly good, thank you for your work!
| case 'inthelast': { | ||
| if ([...DATE_TYPES, ...DATETIME_TYPES].includes(type)) { | ||
| const now = Date.now(); | ||
| const withinTheLastMs = value * 60 * 1000; |
There was a problem hiding this comment.
Unclear variable naming to replace by "withinTheLastMinutes"
| * @param userValue The user's input value | ||
| * @returns A string where all non-alphanumeric and non-hyphen characters are replaced with a whitespace. | ||
| */ | ||
| export const replaceUnderscores = (userValue: string): string => { |
There was a problem hiding this comment.
Bad naming, should be "replaceByWhitespaces" or similar
| recipients: { | ||
| distributionListName: String, | ||
| To: [{ type: String }], | ||
| Cc: [{ type: String }], | ||
| Bcc: [{ type: String }], | ||
| }, |
There was a problem hiding this comment.
HIGH PRIORITY
I don't understand why the naming "recipients" is used there. Would it be possible to change it to "distributionList" directly, to keep the naming consistent and more straightfoward?
Then rename "distributionListName" to simply "name". The changed should be reflected everywhere needed in the code of course.
(putting as high priority because waiting before implementing this change could make it more troublesome to solve)
| /** | ||
| * | ||
| */ | ||
| interface AddCustomNotificationArgs { |
There was a problem hiding this comment.
Naming is wrong
There was a problem hiding this comment.
Why is it "editAndGet"?
We have the separate folder "query" which contains the query to "get" the emailNotification. So I'm not sure of this file's naming?
If this is to signify that you return the updated object after edition, it makes sense. However if it's the only reason, please edit it so it sticks to the convention that we already use with the other mutations, just starting with "edit".
src/schema/types/dataSet.type.ts
Outdated
| // console.log('type resolver'); | ||
| // console.log(parent.records); |
| export const RecipientsType = new GraphQLObjectType({ | ||
| name: 'Recipients', | ||
| fields: () => ({ | ||
| distributionListName: { type: GraphQLString }, | ||
| To: { type: new GraphQLList(GraphQLString) }, | ||
| Cc: { type: new GraphQLList(GraphQLString) }, | ||
| Bcc: { type: new GraphQLList(GraphQLString) }, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Naming to fix as described in previous comment
There was a problem hiding this comment.
@AntoineRelief Is think this part of the code is/will be changed by a fix that Matthieu developed for the "today" function, you might have some merge conflict
| const formattedPastDate = pastDate.toLocaleDateString('en-US', { | ||
| year: 'numeric', | ||
| month: 'long', | ||
| day: '2-digit', | ||
| }); | ||
|
|
||
| // Past Date to time (hh:mm) | ||
| const formattedPastTime = pastDate.toLocaleTimeString('en-US', { | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| }); | ||
|
|
||
| // Current Date to date (mm/dd/yyyy) | ||
| const formattedCurrentDate = currentDate.toLocaleDateString('en-US', { | ||
| year: 'numeric', | ||
| month: 'long', | ||
| day: '2-digit', | ||
| }); | ||
|
|
||
| // Current Date to time (hh:mm) | ||
| const formattedCurrentTime = currentDate.toLocaleTimeString('en-US', { | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| }); |
There was a problem hiding this comment.
Could we make those localeFormats constants so they can be changed easily if required?
| newHeader = `From ${formattedPastDate} ${formattedCurrentTime} UTC as of ${formattedCurrentDate} ${formattedCurrentTime} UTC`; | ||
| } else { | ||
| newHeader = `From ${formattedPastDate} ${formattedPastTime} UTC as of ${formattedCurrentDate} ${formattedCurrentTime} UTC`; |
There was a problem hiding this comment.
I didn't check by running the code, so I'm not too sure where they are displayed/used, but they probably should be translated when needed.
…ectly. * Handle arrays of objects in formatDates * Fix DB values showing instead of text * Fix single string data being replaced (radionuclear)
AB#16252 - Email Notifications Phase 1 Fixes
fix: record validation would not be done correctly for some fields / …
## [2.10.3-rc.1](v2.10.2...v2.10.3-rc.1) (2024-06-06) ### Bug Fixes * record validation would not be done correctly for some fields / requests, and make them fail AB[#95404](https://github.com/ReliefApplications/ems-backend/issues/95404) AB[#95398](https://github.com/ReliefApplications/ems-backend/issues/95398) ([2ecfe0f](2ecfe0f))
## [2.10.3-rc.2](v2.10.3-rc.1...v2.10.3-rc.2) (2024-06-06) ### Bug Fixes * allow to skip validation of records ([35fe868](35fe868))
feat: allow null values in aggregation
# [2.11.0-rc.1](v2.10.3-rc.2...v2.11.0-rc.1) (2024-06-11) ### Bug Fixes * Deleting resource breaks schema ([#1099](#1099)) ([595f294](595f294)) ### Features * allow null values in aggregation ([afe4656](afe4656)) ### Reverts * Revert "bug: Deleting resource breaks schema (#1099)" (#1101) ([e99b0dc](e99b0dc)), closes [#1099](#1099) [#1101](#1101)
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Useful links
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
Screenshots
Please include screenshots of this change. If this issue is only back-end related, and does not involve any visual change of the platform, you can skip this part.
Checklist:
( * == Mandatory )
More explanation
https://www.loom.com/share/05a716d61b9744faaf51fb304c21d1e5?sid=f87cf896-582a-4f76-93ae-8ceed801b145