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

shouldSend does not get called or change outcome of "assertNotSent" or other notification assertions #38909

Closed
mrpritchett opened this issue Sep 22, 2021 · 2 comments · Fixed by #38979

Comments

@mrpritchett
Copy link

  • Laravel Version: 8.61.0
  • PHP Version: 8.0.3
  • Database Driver & Version: mysql 5.7

Description:

It looks like the PR:

#37930 (comment)

that gave us shouldSend() on Notification does not give us that behavior when checking assertions like assertNotSent.

Steps To Reproduce:

  • Create a new notification
  • Set shouldSend to return false
  • Create a test where that notification should send without shouldSend and try to assertNotSent based on shouldSend.
  • Test fails because notification is sent regardless of shouldSend
@gbradley
Copy link
Contributor

gbradley commented Sep 22, 2021

There is some slightly mixed use of "sent" in the Notification classes. The assertSent, assertNotSent methods on the Notification facade more accurately assert the notification was / not passed to the ChannelManager via notify(), send() or sendNow().

What it doesn't assert is that the underlying Illuminate\Notifications\NotificationSender class decided to actually send the notification to the relevant channel(s), which happens in shouldSendNotification(). The same behaviour existed prior to #37930 when listening for the NotificationSending event and returning false.

I do think this is confusing & happy to submit a PR to include the shouldSendNotification() check inside the assertion code, if someone from the team thinks this is the right way to go (rather than creating separate assertions on the NotificationSender itself).

@mrpritchett
Copy link
Author

I'd be happy to submit a PR, but would like to get some team guidance on if this is something that is desired or if another solution is preferred.

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 a pull request may close this issue.

2 participants