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

Customizable notification email mailable #7

Merged
merged 13 commits into from
Aug 23, 2024
Merged

Customizable notification email mailable #7

merged 13 commits into from
Aug 23, 2024

Conversation

CodeWithDennis
Copy link
Contributor

@CodeWithDennis CodeWithDennis commented Aug 23, 2024

Since the email is sent through a notification, we couldn’t customize it when trying to publish the view because there wasn't one. I’ve added a new view for the email with a default layout (see screenshot). Let me know your thoughts or if you have any feedback.

Since the Blade view is a markdown file, we shouldn't format it, as that could cause it to break.

TLDR: Package users can now override the view

Scherm­afbeelding 2024-08-23 om 11 11 01

@CodeWithDennis
Copy link
Contributor Author

Perhaps adding the option in the config to override the subject? Or, if you want to take it a step further, create a custom mailable class.

@Baspa Baspa self-assigned this Aug 23, 2024
@Baspa
Copy link
Member

Baspa commented Aug 23, 2024

Perhaps adding the option in the config to override the subject? Or, if you want to take it a step further, create a custom mailable class.

Nice PR! Creating a custom mailable class would be the nicest solution imo. Can you take a look at it, or do you want me to try? @CodeWithDennis

@CodeWithDennis
Copy link
Contributor Author

Perhaps adding the option in the config to override the subject? Or, if you want to take it a step further, create a custom mailable class.

Nice PR! Creating a custom mailable class would be the nicest solution imo. Can you take a look at it, or do you want me to try? @CodeWithDennis

I will take a look, i will let you know if i can pull it off today.

@CodeWithDennis
Copy link
Contributor Author

CodeWithDennis commented Aug 23, 2024

@Baspa I'm not entirely sure if this will work because the Notification is supposed to return a MailMessage. And besides that moving it to the config would also mean you'd need to bring over the getTwoFactorCode method.

@Baspa
Copy link
Member

Baspa commented Aug 23, 2024

Hmm according to the documentation it should be possible: https://laravel.com/docs/11.x/notifications#using-mailables

If needed, you may return a full mailable object from your notification's toMail method. When returning a Mailable instead of a MailMessage, you will need to specify the message recipient using the mailable object's to method:

I'm going to think about the getTwoFactorCode part. @CodeWithDennis

@CodeWithDennis
Copy link
Contributor Author

Hmm according to the documentation it should be possible: https://laravel.com/docs/11.x/notifications#using-mailables

If needed, you may return a full mailable object from your notification's toMail method. When returning a Mailable instead of a MailMessage, you will need to specify the message recipient using the mailable object's to method:

I'm going to think about the getTwoFactorCode part. @CodeWithDennis

Oh nice find, i will refactor it.

@Baspa
Copy link
Member

Baspa commented Aug 23, 2024

Maybe the user should make sure their mailable can accept the two factor code as argument? Then we could do something like this:

    public function toMail(object $notifiable): Mailable
    {
        $mailable = config('filament-two-factor-auth.mail_mailable') ?? SendTwoFactor::class;

        $mail = new $mailable($this->getTwoFactorCode($notifiable));

        $mail->to($notifiable->email);

        return $mail
     }

@Baspa Baspa added the enhancement New feature or request label Aug 23, 2024
@Baspa Baspa changed the title Created a view for the two-factor authentication email code Customizable notification email mailable Aug 23, 2024
@CodeWithDennis
Copy link
Contributor Author

Maybe the user should make sure their mailable can accept the two factor code as argument? Then we could do something like this:

    public function toMail(object $notifiable): Mailable
    {
        $mailable = config('filament-two-factor-auth.mail_mailable') ?? SendTwoFactor::class;

        $mail = new $mailable($this->getTwoFactorCode($notifiable));

        $mail->to($notifiable->email);

        return $mail
     }

That might work... The changes have been pushed. @Baspa

@Baspa
Copy link
Member

Baspa commented Aug 23, 2024

Great! Code is looking good, thank you for your effort. I will merge it @CodeWithDennis

@CodeWithDennis
Copy link
Contributor Author

Great! Code is looking good, thank you for your effort. I will merge it @CodeWithDennis

Thanks! Maybe you could bring up the option to customize the email as a discussion or issue for a future feature. I can see that some people might want more control, even though they can already change the view.

@Baspa Baspa merged commit 65c4cc3 into vormkracht10:main Aug 23, 2024
9 checks passed
@CodeWithDennis CodeWithDennis deleted the feature/add-mail-view branch August 23, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants