Skip to content
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

docs: notification database schema #168

Merged
merged 8 commits into from
Apr 19, 2023

Conversation

johnhooks
Copy link
Collaborator

@johnhooks johnhooks commented Mar 16, 2023

What?

Redesigns the database schema in markdown.

Why?

Improve the notification system to be fast and flexible.

  • Add channels and subscriptions.
  • Use a join table to provide a many-to-many relationships between messages and users.
  • Use a join table to manage user channel subscriptions.

Continues Discussion #114

Issues with the previous schema

  • Looking up messages by recipient, which could be either a role or users email, limits the system to messages targeting only a single user or a role. If more possibilities are added, it will only continue to complicate the query to find a users messages and enlarge the database index. The more flexible the system the slower it becomes.
  • In the new system the logic and database queries to determine who should receive a message is done once, when the message is emitted, not on every query for a user's notifications.
  • There is no mechanism to change the status of a single users message if the recipient is a role. Toggling the status would change the state of the notification for every user of that role. To maintain a status of every user in a single table would require duplicating the message for every user.

How?

This PR does not implement the schema, it is for documentation and planning.

@johnhooks johnhooks changed the title Feature/notify database schema feature: notification database schema Mar 16, 2023
@johnhooks johnhooks force-pushed the feature/notify-database-schema branch from 0c66a7e to ea94bd6 Compare March 16, 2023 19:14
@johnhooks johnhooks changed the title feature: notification database schema docs: notification database schema Mar 16, 2023
@erikyo erikyo added the documentation Improvements or additions to documentation label Mar 16, 2023
@erikyo erikyo requested a review from Sephsekla March 16, 2023 19:37
docs/database-schema.md Outdated Show resolved Hide resolved
docs/database-schema.md Outdated Show resolved Hide resolved
@johnhooks
Copy link
Collaborator Author

johnhooks commented Mar 16, 2023

Some ideas for core WordPress channels

  • For each role, Admin, Editor etc.

    Alert only the Admins of out of date plugins.

  • For post actions

    Could be used to alert users of modifications or a request to review a post or page. This could create a helpful history.

The final message render could be hookable, and filters applied to it. This would provide the hook function with the data about the user and the notification, allowing personalized messages.

@johnhooks
Copy link
Collaborator Author

johnhooks commented Mar 16, 2023

The properties missing from the original, mostly because I didn't understand the translation aspect,

  • recipient_key - I don't believe this is important in the new system of channels and subscriptions.
  • sender_key - this is similar to channel source in the new system. It could be label sender_key if preferred. A channel also has a name_key.
  • action_url - I can add this in, or it could be kept in the meta property. It's data that would never be queried against.

@johnhooks johnhooks force-pushed the feature/notify-database-schema branch from ea94bd6 to 3f0e344 Compare March 17, 2023 05:03
@johnhooks
Copy link
Collaborator Author

What should the character limit for the translation key columns be? Is there a character limit in WordPress that we can match?

@Sephsekla
Copy link
Collaborator

What should the character limit for the translation key columns be? Is there a character limit in WordPress that we can match?

Hmm I don't think this needs to be massive, since the message key is essentially namespaced by the sender key. I'll have a look at WP Core and see if something else does exist that would be helpful 🤔

@johnhooks
Copy link
Collaborator Author

johnhooks commented Mar 29, 2023

@JasonTheAdams Does this look like a good start? I made it in MySQL Workbench. I looked at the option you linked in Slack, but it looked like it was only for paid subscription (I could be wrong, just didn't research further).

Screen Shot 2023-03-31 at 6 39 29 AM

@johnhooks
Copy link
Collaborator Author

johnhooks commented Apr 5, 2023

@Sephsekla it would be nice to have find some resolution to Issue #219 (even if the solution is a half measure to support implementing a functional API to experiment with). At the moment I haven't found a performant way to looking up the translations by key/slug.

Edit: To enable implementing features while we consider the translation system, we will store messages in the database in the translated language at time a notification is emitted. Though with a strong understanding it will need further consideration and most like require modifications.

@johnhooks
Copy link
Collaborator Author

johnhooks commented Apr 10, 2023

Screen Shot 2023-04-10 at 4 17 34 AM

This updated V2 schema makes adjustments based on Issue #209 and PR #251.

Notifications are to be translated at the time of emission and stored this way in the notifications table.

The subscription table is removed, in preference for in code channel registration and discovery. Subscriptions are linked to registered channels by the channel_name column, which is a key/slug/name (if anyone has a preference on how what to call this, please advise, this is simular in concept to block type name).

Considerations

  • 32 characters might be two little for the channel name, especially considering it will be namespaced by a plugin slug. It will need to be indexed in the subscriptions table.
  • TINYTEXT has a limit of 255 characters, does this seem appropriate for notification title and message? One concern I have is in regard to unicode characters and if the space requirements aren't considered, translations could be cut off if/when using more bytes than the original.
  • Would it be common to query notifications by channel_name? Probably. Should it be indexed?

@johnhooks johnhooks force-pushed the feature/notify-database-schema branch from 2ef593a to c55fd20 Compare April 19, 2023 15:08
Copy link
Collaborator

@erikyo erikyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@johnhooks johnhooks merged commit ba82dba into WordPress:develop Apr 19, 2023
@johnhooks johnhooks deleted the feature/notify-database-schema branch April 19, 2023 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants