POC of subscriptions for notifiable instances

[adamsosterics]

Issue #, if available: #202

Summary

This is a proof-of-concept for implementing subscriptions for specific notifiable instances.
Please let me know what you think, I can make it a proper implementaiton if you like the solution direction.

Other Information

[coveralls]

coverage: 99.438% (-0.3%) from 99.734%
when pulling 9bf78ee on adamsosterics:instance-subscription
into b8d8e26 on simukappu:master.

[coveralls]

coverage: 99.438% (-0.3%) from 99.734%
when pulling 9bf78ee on adamsosterics:instance-subscription
into b8d8e26 on simukappu:master.

[coveralls]

coverage: 99.438% (-0.3%) from 99.734%
when pulling 9bf78ee on adamsosterics:instance-subscription
into b8d8e26 on simukappu:master.

[simukappu]

Hi @adamsosterics, thanks for putting this POC together — the use case in #202 is a solid one and something we'd like to support. I've reviewed the diff and have some feedback on the current approach before we move toward a proper implementation.

Subscription check moved out of generate_notification

Moving the subscribes_to_notification? check from generate_notification into notify means the notify_to → generate_notification path no longer checks subscriptions at all. The guard you added in receive_notification_of covers one entry point, but calling Notification.notify_to directly would bypass it entirely. I'd prefer to keep the check in generate_notification (or at least ensure all paths are covered) and layer the instance subscription logic on top.

Duplicate notifications

subscribed_targets.append(*notifiable.instance_subscription_targets(target_type)) can produce duplicates when a target is already in the list from notification_targets. A .uniq (or set-based merge) is needed here.

N+1 queries

The loop in notify issues a separate subscription query per target via subscribes_to_notification?. For large target sets this will be expensive. Ideally we'd batch-load the relevant subscriptions and filter in memory, or push the check into a single joined query.

Unique constraint on subscriptions

The existing unique index is [:target_type, :target_id, :key]. With instance subscriptions, a target needs multiple subscription records for the same key (one per notifiable instance). The migration adds the notifiable polymorphic columns but doesn't update this index — inserts would fail at the DB level. We'd need something like [:target_type, :target_id, :key, :notifiable_type, :notifiable_id], plus updating the ActiveRecord model validation (uniqueness: { scope: :target }) accordingly.

notify_later path not covered

The async path (notify_later via ActiveJob) doesn't include the instance subscription logic, so notifications queued through jobs would ignore instance subscriptions.

Multi-ORM support

instance_subscription_targets calls Subscription.where(...) directly, which works for ActiveRecord but hasn't been verified against Mongoid or Dynamoid. We'd need to make sure it works (or is adapted) for all supported ORMs.

Test coverage

Coverage dropped from 99.734% to 99.438%. A full implementation would need tests for the new subscription type — creation, filtering, the notify flow, edge cases around duplicates, and the async path.

---

Overall the direction makes sense. My suggestion for a full implementation would be:

1. Add notifiable_type / notifiable_id as nullable columns on subscriptions (as you've done).
2. Update the unique index to include those columns.
3. Keep the subscription check inside generate_notification and extend it to also consider instance-level subscriptions, rather than splitting the logic across notify and generate_notification.
4. Handle deduplication explicitly.
5. Cover the notify_later path.
6. Verify across all three ORMs.

Happy to discuss the design further if you'd like to iterate on this. I'll also be working on a design for this feature on our side — will share it once it's ready so we can align on the approach. Thanks again for kicking this off!

[simukappu]

Closing this PR as we've implemented the feature independently based on the direction discussed here. The implementation is tracked in #202 and will be included in v2.6.0. Thanks again for the POC — it helped shape the final design!