Improve seedNotificationType #7
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CarlosQ96
reviewed
Oct 20, 2022
| href: Joi.string().required(), | ||
| }); | ||
|
|
||
| export const SEGMENT_METADATA_SCHEMA_VALIDATOR: { |
CarlosQ96
reviewed
Oct 20, 2022
test/pre-test-scripts.ts
Outdated
| @@ -48,7 +51,12 @@ async function runMigrations() { | |||
| } | |||
|
|
|||
| const seedDb = async () => { | |||
| // | |||
| new ThirdParty(); | |||
There was a problem hiding this comment.
Why instanciate an empty class here? Can we remove it? I don't think it's doing anything.
There was a problem hiding this comment.
Oh my mistake, I deleted that line
CarlosQ96
reviewed
Oct 20, 2022
| }, | ||
| DRAFTED_PROJECT_SAVED: { | ||
| name: 'The project saved as draft', | ||
| description: 'The project saved as draftd', |
There was a problem hiding this comment.
Typo here, remove the d of the word draftd
CarlosQ96
reviewed
Oct 20, 2022
|
|
||
| // https://github.com/Giveth/notification-center/issues/6 , https://gist.github.com/MohammadPCh/24434d50bc9ccd9b74905c271ee05482 |
There was a problem hiding this comment.
We are missing the welcome notification type. I think we removed it on another PR. We should re add it, since it's part of the logic.
CarlosQ96
reviewed
Oct 20, 2022
| }, | ||
| { | ||
| type: 'a', | ||
| content: 'clik here', |
CarlosQ96
reviewed
Oct 20, 2022
| }, | ||
| ], | ||
| content: | ||
| 'Pssst! your rewards are ready to claim 😉\nCheckout GIVeconomey to claim your rewards.', |
CarlosQ96
reviewed
Oct 20, 2022
| { type: 'br' }, | ||
| { | ||
| type: 'a', | ||
| content: 'Clik here', |
CarlosQ96
reviewed
Oct 20, 2022
| content: 'Your project update was successful.', | ||
| }, | ||
| ], | ||
| content: 'Your project update was succesful.', |
CarlosQ96
reviewed
Oct 20, 2022
| pushNotifierService: null, | ||
| title: 'Donation has been failed', | ||
| title: 'Project posted an update', |
CarlosQ96
approved these changes
Oct 20, 2022
|
@CarlosQ96 Thanks for your great review and made changes |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
related to #6
@CarlosQ96
We had to add some HTML templates as https://gist.github.com/MohammadPCh/24434d50bc9ccd9b74905c271ee05482 so after merging we should delete some tables and migrations, to seed them again in staging DB, I added something to validate notificaiton's metadata as you wrote for segment data, and wrote lots of test cases to cover all notification types.
PS After this we should change impact-graph to send needed metadata when sending notifications and maybe calling notification-center in more places like when someone like a project