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

Refactor notifications #199

Merged
merged 18 commits into from Jan 2, 2022
Merged

Refactor notifications #199

merged 18 commits into from Jan 2, 2022

Conversation

joemasilotti
Copy link
Owner

@joemasilotti joemasilotti commented Jan 2, 2022

This PR refactors notifications to make them a little easier to interact with. Most of the ideas implemented here come from discussions with @jayeclark in #182. So a huge thank you there!

Most importantly, notifications are now tied to a User, not a Business or Developer. This makes queries much more straightforward as current_user.notifications gets them all at once. No more building custom queries.

Second, notification logic now lives on the notification models (not records) themselves. NewMessageNotification now has methods for the conversation and message, not Notification. This is largely thanks to the discovered #to_notification method from noticed that rehydrates the instance from the params initially sent in.

Finally, it refactors how notifications are "read" by navigating to /notifications/:id and then redirecting. The second refactor enabled this to be much simpler, mostly due to the #to_notification method from noticed. If a Notification instance doesn't have a url then it is marked as read but you are redirected to /notifications (with a flash message).

A backfill was added that addresses a few things. First, it sets the recipient of all NewMessageNotification type records to the user associated with the developer or business. Second, it deletes some dangling notification records from deleted developer profiles. Finally, a separate rake task marks all existing notifications as read.

Edit: The backfill now also adds the conversation to the notification records (as params) which allows easy querying for batch updating: conversation.notifications_as_conversation grabs all the notifications triggered by sent messages in the convo. Adding this to ConversationsController means marking all (unread) notifications as read when viewing a conversation, all with a single query!


I'm going to leave this in draft tonight and revisit tomorrow morning as there's a lot going on. Especially with the backfills!

Pull request checklist

  • Your code contains tests relevant for code you modified
  • You have linted and tested the project with bin/check

@joemasilotti joemasilotti marked this pull request as ready for review January 2, 2022 16:52
@joemasilotti joemasilotti merged commit 62ceb16 into main Jan 2, 2022
5 checks passed
@joemasilotti joemasilotti deleted the refactor-notifications branch January 2, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants