-
Notifications
You must be signed in to change notification settings - Fork 16
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
Adds a callable generator for Mail and Notification assertions #14
base: master
Are you sure you want to change the base?
Conversation
81d354a
to
04edf66
Compare
04edf66
to
a49d1ed
Compare
Hi @peterfox I'm going to throw a spanner in the job, sorry. :) I'm thinking that InteractsWithViews could have assertMail and assertNotification returning a ViewAssertion instance. Your initial example would then become something like Mail::fake();
Mail::to('[email protected]')->send(new ExampleMailable());
Mail::assertSent(ExampleMailable::class, function ($mail) {
$this->assertMail($mail)->contains('Laravel');
return true;
}); which I find a bit simpler and easier to read. What do you think? For example, we can have Mail::assertSent(ExampleMailable::class, function ($mail) use ($user) {
$this->assertMail($mail)->contains('Laravel');
return $mail->hasTo($user->email)
}); |
No worries, happens to me all the time on my projects.
Yeah I agree this might be the better option in the end especially as I know L7 has changed the assert methods a little so this feature might break now. Only problem is I'm not sure if this will work as well for Notifications I guess I could do something like: $this->assertNotification($notification, $notifiable)->assertContains('Laravel'); What's your thoughts on traits? Should I add these to the |
I think they can be added to the same trait for simplicity since they are still checking stuff on generated views. We can call them assertMailView and assertNotificationView if we want to highlight that they are still view related. Let's see what @nunomaduro thinks. |
@SimoTod sure, will hold off further changes until @nunomaduro takes a look |
@nunomaduro As far as I'm aware no. Mailables are a bit complicated because they can have a view or have a markdown template. Same with notifications but also in a more complicated way because they're typically a We could do like this: Mail::assertSent(ExampleMailable::class, function ($mail) use ($user) {
$this->assertView($mail)->contains('Laravel');
});
Notification::assertSentTo($user, ExampleNotification::class, function ($notification, $notifiable) use ($user) {
$this->assertView($notification->toMail($notifiable))->contains('Laravel');
}); Using |
My 2 pence A separate method would typehint Mailable and it would know exactly what to do (same for Notification). Something like (probably not complete, i did not test it) /**
* Create a new view test case.
*/
protected function assertView(string $view, array $data = [], array $mergeData = []): ViewAssertion
{
return new ViewAssertion(view($view, $data, $mergeData)->render());
}
/**
* Create a new view test case from a Mailable.
*/
protected function assertMailView(Mailable $mailable): ViewAssertion
{
return new $mailable->render();
}
/**
* Create a new view test case from a Notification.
*/
protected function assertNotificationView(Notification $notification): ViewAssertion
{
if (! method_exists($notification, 'toMail')) {
throw WhateverException('not testable');
}
return new $notification->toMail(null)->render();
} I'm not fussy about that but, as long as it's documented, different methods for this cases shouldn't cause too much confusion and the underlying code would look a bit nicer. |
@SimoTod The notification signature would have to be: /**
* Create a new view test case from a Notification.
*/
protected function assertNotificationView(Notification $notification, $notifiable): ViewAssertion
{
} Or we'd have to do: /**
* Create a new view test case from a Notification.
*/
protected function assertNotificationView($message): ViewAssertion
{
if ($message instanceOf Mailable) {
// return ViewAssertion
} else if ($message instanceOf MailMessage) {
// return ViewAssertion
}
// throw Invalid Arg
} I generally lead to your solution over @nunomaduro if only because it doesn't risk breaking compatibility with what's already there and I think most people who use it will understand they need to use the extra methods for notifications and mailables. |
It makes sense to me. |
@nunomaduro @SimoTod you guys in consensus then that it should be done as individual methods inside the trait? |
@peterfox We didn't discuss it since Nuno has been busy with pest lately but I think he preferred the single helper to keep the public API simple. |
@SimoTod I think that works fine for me. Shouldn't be too difficult, might end up being a lot of logic in one method, will have to consider how to break up some of that logic |
Implements the feature described in issue #13. Adds a static make command that generates a callable that is compatible with both Mail and Notification assertions in Laravel that then calls a provided callable with a ViewAssertion as the first parameter making it possible to test the rendered email content.