feat: Add database foundation for end-user self-subscription to status pages#6998
feat: Add database foundation for end-user self-subscription to status pages#6998catsdev wants to merge 2 commits intolouislam:masterfrom
Conversation
|
Hello and thanks for lending a paw to Uptime Kuma! 🐻👋 |
a77d9a6 to
eec3e77
Compare
eec3e77 to
585f384
Compare
CommanderStorm
left a comment
There was a problem hiding this comment.
Seems resonable. I seem to have forgot to submit my review.
Hard to review something which is not integrated into the rest of the system at all.
| table.increments("id").primary(); | ||
| table.string("email", 255).notNullable().unique(); | ||
| table.string("unsubscribe_token", 255).unique(); | ||
| table.datetime("created_at").notNullable().defaultTo(knex.fn.now()); |
There was a problem hiding this comment.
(applies in more than one place)
please use knex' timestamp function instead. This way we get created_at and updated_on
There was a problem hiding this comment.
Excellent... didn't know about it.
| return ( | ||
| knex.schema | ||
| // Create subscriber table | ||
| .createTable("subscriber", (table) => { |
There was a problem hiding this comment.
(applies in more than one place)
please name all tables status_page_ to make it clearer for future devs what this table contains
| // Create subscription table (links subscribers to status pages and components) | ||
| .createTable("subscription", (table) => { | ||
| table.increments("id").primary(); | ||
| table.integer("subscriber_id").unsigned().notNullable(); |
There was a problem hiding this comment.
(applies in more than one place)
please use foreign key constraints + on delete cascade. This way we can be tighter in terms of data modeling.
You currently are using an index which is kind of weird
| table.increments("id").primary(); | ||
| table.integer("subscriber_id").unsigned().notNullable(); | ||
| table.integer("status_page_id").unsigned().notNullable(); | ||
| table.integer("component_id").unsigned(); // NULL means subscribe to all components |
There was a problem hiding this comment.
what is a component? please add a knex comment or a foreign key to what this is.
There was a problem hiding this comment.
Good catch. My subscription UI called them components, thus the component id. But, in reality these are the publicMonitorList aka publicGroupList of the status page, and the ids como from the table "group". I'll rename to group_id and reference the group table
| // Create notification queue table | ||
| .createTable("notification_queue", (table) => { | ||
| table.increments("id").primary(); | ||
| table.integer("subscriber_id").unsigned().notNullable(); |
There was a problem hiding this comment.
should this not be subscription_id?
There was a problem hiding this comment.
Hmmm, not really, once a determination has been made that the subscriber needs to be notified, there is no need for the subscription that triggered the notification. But the contact details are in the subscriber table.
| table.string("notification_type", 50).notNullable(); // 'incident', 'incident_update', 'maintenance', 'status_change' | ||
| table.string("subject", 255).notNullable(); | ||
| table.text("data").notNullable(); | ||
| table.string("status", 50).defaultTo("pending"); // 'pending', 'sent', 'failed' |
There was a problem hiding this comment.
(also applies elsewere)
for things that are an enum, please use knex' .enu(..) functionality
| table.index("status", "notification_queue_status"); | ||
| table.index("created_at", "notification_queue_created_at"); |
There was a problem hiding this comment.
these two indexes seem kind of strange. Can you explain this?
| toPublicJSON() { | ||
| return { | ||
| id: this.id, | ||
| email: this.email, |
There was a problem hiding this comment.
I'd like to not leak users emails
| email: this.email, |
| table.boolean("notify_maintenance").defaultTo(true); | ||
| table.boolean("notify_status_changes").defaultTo(false); | ||
| table.boolean("verified").defaultTo(false); | ||
| table.string("verification_token", 255); |
There was a problem hiding this comment.
please go over your nullability.
I for example think theat verified being null is kind of weird, same as most of the other collumns.
There was a problem hiding this comment.
Don't want to waste your time, but this is a feature that would be very difficult to implement, and the scope is very large. We should discuss it first.
Basically, what you are trying to build is a whole open source email subscription system.
Like this one:
https://github.com/Billionmail/BillionMail
In the real world, if a company needs newsletter subscription function, no one would implement such thing from scratch, because we have Sendgrid, mailchimp etc., to deal with the problem perfectly.
1. Sending mass emails is a big topic
Just google why.
2. Performance is questionable, especially on SQLite
By looking at #7040.
Assume that you have 5000 subscription emails.
When a monitor went down, it add 5000 records to notification_queue table, each record contains full email content (full html code).
It could lock SQLite for a long period to insert them all, because SQLite only have one file writer at the same time.
During the period, other monitors could fail to insert heartbeat records, and go down too because of this.... And them another 5000 emails will be queued to the notification queue.... so on....
3. The UI of email subscription management
Another big part that you have to implement from scratch.
My advice
Drop all tables. Add massmail_provider (or similar).
Let Sendgrid, mailchimp, Billionmail handle it.
When a monitor went down, you just need to send one API call to them.
|
@louislam I like that direction. From your perspective, there wouldn’t be a need for customers to subscribe directly to the status page. Instead, the status page could be configured by admins to send alerts to a service (e.g., webhook, email list), and customers would subscribe to those services instead. Is that correct? If so, we would only need to add alert configuration to the status pages (UI and database), along with the logic to trigger alerts based on lifecycle events for groups and maintenance events. Am I reading you correctly? |
From public users' UI perspective, they still need to input their email address in our status page, in order to subscribe. They won't know which mass mail provider we are using. Here is pseudo code of my imagination, not correct, but you should get the idea. MassMailProvider {
name: string;
config: {};
subscribe(email: string) {}
unsubscribe(email: string) {}
send(msg : string) {}
}
SendGrid extends MassMailProvider {
name = "SendGrid";
// Get config from db
config = {
apiKey,
emailListName
}
subscribe(email: string) {
// Call API to add `email` to `emailListName`
}
unsubscribe(email: string) {
// Call API to remove `email` from `emailListName`
}
send(msg : string) {
// Call API to send mass email using `emailListName`
}
}Ideally, for example, after an admin added new Incident post, it should call: const massMailProvider = statusPage.getMassMailProvider()If it is not null, use it to call: massMailProvider.send("Oh no! My server is down. I am fixing the issue!") |
|
But first thing first, if you have no experience with those mass mail services (SendGrid, mailchimp), you should get familiar with them first. They should have free tier for you to try. Also just re-read #916, Twist is supporting multiple subscription methods. So maybe it is a many-to-many relation (statusPage vs massMailProvider). I personally don't need this function, I probably won't go deeper, but I hope my comments help get you guys back on the right track. You may also need to ask in #916 if it is what they need (also is it what you want to do too?). |
Summary
This pull request introduces the following changes:
Three new tables have been added to the schema to support end-user status page subscriptions.
subscriber: Stores individual subscriber information (email, etc.).
subscriptions: Stores associations between subscribers and specific status pages.
notification_queue: Manages and queues email notifications to offload the processing of potentially many of emails.
Relates to Email Subscription on Status Page #916 link
Please follow this checklist to avoid unnecessary back and forth (click to expand)