-
-
Notifications
You must be signed in to change notification settings - Fork 468
chore: refactor notifications send in batch #4583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
onearmy-community-platform
|
||||||||||||||||||||||||||||||||||||||||
| Project |
onearmy-community-platform
|
| Branch Review |
feat/refactor-notifications-batch
|
| Run status |
|
| Run duration | 10m 05s |
| Commit |
|
| Committer | Mário Nunes |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
1
|
|
|
1
|
|
|
0
|
|
|
0
|
|
|
93
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
src/integration/research/read.spec.ts • 1 failed test • ci-chrome
| Test | Artifacts | |
|---|---|---|
| ... > [Visible to everyone] |
Test Replay
Screenshots
Video
|
|
src/integration/settings.spec.ts • 1 flaky test • ci-chrome
| Test | Artifacts | |
|---|---|---|
| [Settings] > Can create member |
Test Replay
Screenshots
Video
|
|
| token?: string; | ||
| token_hash?: string; | ||
| token_new?: string; | ||
| token_hash_new?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these auth focused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are not being used as far as I can see.
benfurber
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good.
My worry about the responsibility of the SQL function current is that it's hard to follow. Can't the validating the user email preferences stay a TS service focused?
(I'm sure it's on your list already but looks like a bunch of code can be removed from the edge function folder.)
Before we were doing 1 query per user (and was still an sql function because needs access to auth.users).
But I don't see a benefit to that, the 2nd query especially would be longer: But I'll look into making it more readable. |
…m/ONEARMY/community-platform into feat/refactor-notifications-batch
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the new behavior?
Emails are sent in batches of 100.
This should solve some emails not being sent due to hitting resend limits.
The new approach brings the resend code to our server, instead of using the edge function.
Does this PR introduce a DB Schema Change or Migration?
Git Issues
Closes #