Repository navigation
Fix bugs found in code review - #579
Merged
Merged
Conversation
* Notifier Notification classes inherited from a top-level ::Notification model in the host app because const_defined?/const_get searched ancestors * Ephemeral bulk delivery ignored the notifier's bulk_deliver_by config * Ephemeral notifiers skipped before_enqueue and validate!, rejected job options (deliver(recipients, wait: ...)), and deliver_later was bound to the ActiveRecord deliver body * Ephemeral notification_methods were not inherited by subclasses * Ephemeral wait/queue/priority lambdas now run in the Notification context (Event for bulk) instead of the recipient, matching persisted notifiers * Notifier.with(params) mutated the caller's hash * Noticed::DeliveryMethods::Discord inherited BulkDeliveryMethod and crashed * BulkDeliveryMethod#fetch_constant passed the value instead of the name to evaluate_option, returning nil for procs and constants * DeliverBy#validate! rejected required options set to false * rails g noticed:install could not be resolved (class was ModelGenerator) * has_noticed_notifications added a public current_adapter method to every model and read ActiveRecord::Base's adapter instead of the event model's
excid3
force-pushed
the
fix-review-batch-1
branch
from
September 30, 2026 20:41
3098105 to
44bbe03
Compare
* deliver_later delegates to deliver instead of alias_method, so subclasses that override deliver (Ephemeral) get it for free * ephemeral_perform_later requires context: explicitly; computed_options dups once instead of every caller * Drop tests that only asserted a deleted method or duplicated end-to-end coverage * Trim CHANGELOG
2 of 3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Batch of fixes from a full read-through of the library. Every item under "confirmed" was reproduced with a failing test before fixing; each now has a regression test (15 new tests, 144 total).
Confirmed bugs
Notificationclasses inherited from a top-level::Notificationmodel in the host app (const_defined?searchedObject), so any app withapp/models/notification.rbgot the wrong superclass/table.bulk_deliver_byconfig entirely (@config = overrides).before_enqueueandvalidate!, raised ondeliver(recipients, wait: ...), anddeliver_laterwas aliased to the ActiveRecorddeliverbody.Notifier.with(params)mutated the caller's hash.Noticed::DeliveryMethods::DiscordinheritedBulkDeliveryMethodsodeliver_by :discordcrashed.rails g noticed:install/noticed:modelcould not be found.From reading
BulkDeliveryMethod#fetch_constantreturnednilfor proc/constant values.DeliverBy#validate!rejected required options set tofalse.notification_methodswere not inherited by subclasses.has_noticed_notificationsadded a publiccurrent_adaptermethod to every model and usedActiveRecord::Base's adapter rather than the event model's connection.Behaviour change (noted in CHANGELOG as breaking): ephemeral
wait/wait_until/queue/prioritylambdas now evaluate in the Notification context (Event for bulk) instead of the bare recipient, matching persisted notifiers.Test plan
standardrbcleanrails teston main Gemfile, Rails main, 7.1, 8.1 — 144 runs, 0 failuresrails g noticed:installresolves and copies migrations