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

Add notification_methods for adding methods to the Notification model #362

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

excid3
Copy link
Owner

@excid3 excid3 commented Jan 15, 2024

This introduces a notification_methods block for Notifiers to add methods to the Noticed::Notification records. This works by adding STI to Noticed::Notifications, dynamically creating a Notification class for each Notifier and applying methods to it.

Without this feature, it is tricky to load notifications from the database and call methods on the Notifier that require the notification or recipient:

<div>
  <% @user.notifications.includes(:event).each do |notification| %>
    <%= link_to notification.event.message_for(notification)  %>
  <% end %>
</div>
class NewCommentNotifier < Noticed::Event
  # ...

  def message_for(notification)
    "#{notification.recipient.name}, you have a new comment to review"
  end
end

While this is fine, it feels incomplete.

By introducing notification_methods, we can refactor the above to this:

<div>
  <% @user.notifications.includes(:event).each do |notification| %>
    <%= link_to notification.message  %>
  <% end %>
</div>
class NewCommentNotifier < Noticed::Event
  # ...

  notification_methods do
    def message
      "#{recipient.name}, you have a new comment to review"
    end
end

This is considerably cleaner and provides a handy way of extending the Notification class while also making the methods unique to each Notifier. If we added the message method to Noticed::Notification directly, it would share that method across all Notifiers.

See #360 for the discussion leading to this.

@excid3 excid3 merged commit 5ec9617 into main Jan 15, 2024
45 checks passed
@excid3 excid3 deleted the notification-methods branch January 15, 2024 20:17
@jon-sully
Copy link
Contributor

🙌

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