feat: storage quota notifications #1156
Conversation
|
what is 990? If it's an issue number pls just link to it in the PR description |
| @@ -47,7 +47,7 @@ services: | |||
|
|
|||
| cluster0: | |||
| container_name: cluster0 | |||
| image: ipfs/ipfs-cluster:latest | |||
There was a problem hiding this comment.
I assume the latest version introduces some issue or is buggy?
Do we have that context?
If yes I wonder if we could add a comment and maybe a follow-up ticket to move this back to latest if/when possible?
There was a problem hiding this comment.
ipfs-cluster just went 1.0 and made a couple of breaking changes which impact us.
|
@GaryHomewood great work. I knew I would have likely struggled to get to the end of the review and tomorrow I'm not working. As stated in one of my comments my main concern is in the way we get the users on the fly for a given quota range and the performance implications that that would have. |
| END | ||
| $$; | ||
|
|
||
| CREATE TABLE IF NOT EXISTS email_notification_history |
There was a problem hiding this comment.
My instinct is to call this table email_history, rather than email_notification_history, just because it keeps it as a more generic thing. I.e. not all emails are necessarily notifications (they could be "please click this link to verify your email address", or "click this link to log in", or "here's the receipt for your account charges this month").
There was a problem hiding this comment.
Also makes sense. Made more generic.
packages/db/test/utils.js
Outdated
| const { data } = await dbClient._client | ||
| .from('user_tag') | ||
| .upsert({ | ||
| user_id: userId, | ||
| tag: options.tag || '', | ||
| value: options.value || '', | ||
| reason: options.reason || 'test reason' | ||
| }) |
There was a problem hiding this comment.
In order to be inline with the way the user_tags table is intended to work, this should really always do an insert, and set deleted_at on the previously-active row.
There was a problem hiding this comment.
Actually, we could also move this function into packages/db/index.js and then it would achieve this for us at the same time: #1195
There was a problem hiding this comment.
Have moved to the db package, now that its needed elsewhere.
packages/db/index.js
Outdated
| } from './utils.js' | ||
| import { ConstraintError, DBError } from './errors.js' | ||
|
|
||
| import { emailType } from './constants.js' |
There was a problem hiding this comment.
Can we use UPPERCASE here to hightlight is a constant?
Also pretty sure this could be written in one line
export { emailType } from './constants.js'
There was a problem hiding this comment.
Yes, have uppercased.
| * @param {string} [tag.reason] | ||
| * @returns {Promise<boolean>} | ||
| */ | ||
| async createUserTag (userId, tag = {}) { |
There was a problem hiding this comment.
This could be done as part of #1195
- It's not transactional, we might end up having deleted a tag without adding the new one.
- The function is currently doing 2 api calls, not great for performance.
- This is currently not tested
packages/cron/src/jobs/storage.js
Outdated
| }) | ||
| await emailService.sendEmails({ | ||
| users, | ||
| email: EMAIL_TYPE.Used90PercentStorage |
There was a problem hiding this comment.
Can you please double check all the email types in this file?
There was a problem hiding this comment.
Good catch, have corrected and added to the test.
|
One other thought on this is that I'm wondering whether we should skip the call to So as as we do #1178 before we actually set this going as a cron job then I guess it's fine. But it might be good to comment those lines out for safety? Edit: that would break the tests, so possibly isn't worth it. Maybe we should just make sure we do #1178 before we add the cron?! |
GaryHomewood
changed the title
This PR resolves the first 3 tasks of #990
cronpackage to call the DB functions to get the users that need emailing and save details of email sent.Task 4 about the Mailchimp email service integration is in a separate ticket #1153 and PR.