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

Set recipient instance variable in Noticed::Base #180

Merged
merged 3 commits into from
Nov 19, 2021

Conversation

clinejj
Copy link
Contributor

@clinejj clinejj commented Nov 18, 2021

Fixes #179

This is a partial revert of #126 using style from #176.

In #179, issue was identified where the recipient was nil in before callbacks on delivery methods. This fixes that issue and adds appropriate tests.


There were some other issues I identified while trying to increase test case coverage for callback methods, namely:

  • Adding the following callback to CallbackExample in noticed_test would raise the argument error that recipient is nil, it is unclear if that is intended behavior:
before_deliver do
  raise ArgumentError unless recipient

  self.class.callbacks << :before_everything
end
  • Adding an around_deliver callback to Noticed::DeliveryMethods::Test would fail multiple tests, as the #deliver method would not get called

  • Adding before/around/after callbacks to CallbackExample on both everything and the database method would only call the everything callbacks, and not the method callbacks. For example, adding before_deliver and around_deliver callbacks to the example test would result in the test returning [:before_everything, :around_everything, :after_everything]

There's some unclear logic as to what callbacks get called when, and I'm not sure what is expected vs what happens in test vs what happens in an actual implementation, but happy to log to another issue or document elsewhere.

@excid3
Copy link
Owner

excid3 commented Nov 19, 2021

Thanks @clinejj! Good to have some more robust tests here for next time internals get refactored. 😅

The order of callbacks kind of vary.

We'll always run the database delivery first. Depending if the deliveries are async or not, the delivery methods will either run in parallel or in order.

@excid3 excid3 merged commit 7940adc into excid3:master Nov 19, 2021
@clinejj clinejj deleted the patch-1 branch November 19, 2021 18: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.

nil recipient in email callback after updating to 1.5+
2 participants