laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.32k stars 10.95k forks source link

Notification test doesn't use preferredLocale #44923

Closed nikosid closed 1 year ago

nikosid commented 1 year ago

Description:

When I run test with sending notification to User which haspreferredLocale()method, it ignored the user's language. This is a test for command.

Steps To Reproduce:

Test can be something like this:

$user = User::factory()->create(['lang' => 'uk']);

Notification::fake();
artisan(NotifyCommand::class)
        ->assertExitCode(0);
Notification::assertSentTo($user, BirthdayNotification::class, function ($notification, $channels) use ($user) {
        $mailData = $notification->toMail($user);

        return $mailData->subject === "Birthdays for {$user->name} on ".today()->format('d.m.Y');
    });

BirthdayNotification looks like this:

public function toMail(User $notifiable): MailMessage
    {
        return (new MailMessage)
            ->subject(__(
                'Birthdays for :teamName on :date',
                [
                    'teamName' => $this->team->name,
                    'date' => Carbon::today()->format('d.m.Y'),
                ]
            ))
            ->markdown('emails.birthdays.daily', [
                'teamTitle' => $this->team->name,
                'todaysBirthdays' => $this->todaysBirthdays,
            ]);
    }

In current example test must fail, because user has 'lang' => 'uk' lang and in test we compare title with English sentence.

driesvints commented 1 year ago

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

eduance commented 1 year ago

This is also an issue with mailables.

driesvints commented 1 year ago

Closing until a repo is provided.

nikosid commented 1 year ago

Let's try. https://github.com/nikosid/bug-report/commit/c4b508c1119a3147c00a7b8cc1894e4a47e97343

My test fails php artisan test --filter="DailyMailTest"

Expected that subject will be translated to user's preferred locale.

driesvints commented 1 year ago

Confirmed. We'd appreciate a PR for this 👍

eduance commented 1 year ago

@nikosid Your tests are failing for a different reason, you're asserting that the subject is translated, but Laravel doesn't automatically translate your subject nor makes that promise anywhere, only the contents within the mail should be translated. This content is properly translated when called to() or send() which is not called when calling assertQueued/assertSent as this never hits the render method.

@driesvints Mailables can be tested in two ways, one: for checking whether they are sent properly. Second, for testing the contents. When testing content, a note should be added to the documentation that locale() has to be added manually. Another way to do this would to do this in the constructor of the Mailable.

See: https://laravel.com/docs/9.x/mail#testing-mailable-sending

We can:

  1. Express more clearly that assertQueued or assertSent shouldn't be used for testing the contents.

  2. Make assertQueued/assertSent better to also be able to test the contents within, this is possible but would be a new feature; I like this idea more as it would save a bunch of wasted hours and has no backwards problems directly.

  3. Simply add a more obvious documentation note which explains the usage of assertQueued/assertSents and testing the mailable contents directly.

In my opinion, 2 seems like a harmless, yet really beautiful improvement to the testing of mailables. We could as a bonus also instantly see whether we can make the subject directly translatable.

Willing to spend some time on this improvement.

driesvints commented 1 year ago

@eduance that's a very good remark. Maybe we can indeed improve this. But I think for now better docs would help here. We'd appreciate a PR for that 👍