pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
307 stars 447 forks source link

Consider integration of Laravel Mail Service #7141

Closed Vitaliy-1 closed 3 years ago

Vitaliy-1 commented 3 years ago

Describe the problem you would like to solve

  1. Support for modern mail drivers
  2. Reduce the burden of maintaining own mail managing solution
  3. Integrate mail sending API with other parts of the Application, e.g., Queues

Describe the solution you'd like Register Illuminate Mailer. Review whether additional tweaks/adaptations are required to implement its interface

Who is asking for this feature? Community

Additional information This issue is coupled with email template variables refactoring and improvements to email workflow in general.

Vitaliy-1 commented 3 years ago

Made it work locally with our mail templates. Some tweaks are required regarding ways to pass data (variables) with native methods. In general, this going to replace our Mail class and its children with Laravel Mailable classes. As an example, let's say there is UserRegistered event its listener MailRegisteredUser and class that extends Laravel's Mailable MailRegisteredUser. The representation of an email looks like:

class MailRegisteredUser extends Mailable
{

    // All all template variables associated with arguments of the constructor are automatically available in the template  
    public function __construct(PKPSubmission $submission, array $recipients, ReviewAssignment $reviewAssignment)
    {
        $this->submission = $submission;
        $this->recipients = $recipients;
        $this->reviewAssignment = $reviewAssignment;
    }

    public function build()
    {
         // passing email key to view (instead of path to the Laravel's Blade template), using method chaining to set data
         return $this->view('USER_REGISTERED')
                           ->replyTo($this->context->getContactEmail())
                           ->with(['additionalVariable => $this->submission->getData('somedata')]) // Supply template with additional variable 
    }

    // as an alternative to method chaining
    public function from()
    {
        $user = PKPAplication::get()->getRequest()->getUser();
        $emailAddress = user->getData('email');
        $name = $user->getLocalizedData('givenName');
        return $this->setAddress($emailAddress, $name, 'from');
    }
}

Email data can be overridden from the listener, which sends an email, e.g.:

use mail/.../MailRegisteredUser as Mailable
class MailRegisteredUser
{
    /**
     * @param \PKP\observers\events\UserRegistered
     * @return void
     */
    public function handle(UserRegisteredAssigned $event)
    {
        $mail = new Mailable($event->submission, [$event->reviewer], $event->reviewAssignment);
        Mail::send($mail->view('ANOTHER_TEMPLATE'));
        // or directly changing message data passing a Closure as a 3rd parameter to the send method
    }
}

Also, it seems that Laravel's interface for the SwiftMailer doesn't support all SMTP configs that are currently in OJS, like default envelope sender and associated. Here are parameters that can be passed to the SMTP config:

Vitaliy-1 commented 3 years ago

Mailable class and observer can be combined as the only task of the latter is sending an email.

Vitaliy-1 commented 3 years ago

I think, only message localization is left and it's ready; Laravel uses its own implementation.

@asmecher, after this, what is a preferable way of going further, make a PR with just the work on Mail Service registration or wait until the whole work on refactoring, including template variables, email-associated events and replacing the old mail-related code is done (and make a big PR that includes all this)?

Vitaliy-1 commented 3 years ago

List of modifications from original Illuminate Mail Service:

NateWr commented 3 years ago

@Vitaliy-1 I'd like to see Mailable decoupled from our email templates. Instead of passing an email template key, we should pass in the rendered body.

I think the same is true with things like the from() method. We don't want to take data from the request at this level. We want to decouple this from the request as much as possible.

I wonder if we need class MailRegisteredUser extends Mailable at all. Can we construct the email in the event listener itself?

class MailRegisteredUser
{
    /**
     * @param \PKP\observers\events\UserRegistered
     * @return void
     */
    public function handle(UserRegisteredAssigned $event)
    {

        $emailTemplate = Repo::emailTemplate()->get('EMAIL_TEMPLATE');

        // Generate body/subject with params
        $body = $emailTemplate->getBody([...]);
        $subject = $emailTemplate->getSubject([...]);

        // Generate the email
        $mail = new Mailable();
        $mail->to($event->user->getName(), $event->user->getEmail())
            ->from($event->context->getName(), $event->context->getEmail())
            ->subject($subject)
            ->body($body);

        // Send the email
        Mail::send($mail->view();
    }
}
Vitaliy-1 commented 3 years ago

I wonder if we need class MailRegisteredUser extends Mailable at all.

The idea is to parse its constructor to determine template variables that can be assigned to this email. Also, I'm finding it a good idea to associate an email with its specific representation in the code, rather than have a generic Mailable class.

I'd like to see Mailable decoupled from our email templates

The actual rendering is done inside Laravel's Mailer class, Mailable is just a storage of data in this case.

What about combining listener and email in a different way, something like

class MailRegisteredUser extend Mailable
{
    /**
     * @param \PKP\observers\events\UserRegistered
     * @return void
     */
    public function handle(UserRegisteredAssigned $event)
    {

        $emailTemplate = Repo::emailTemplate()->get('EMAIL_TEMPLATE');

        // Generate body/subject with params
        $body = $emailTemplate->getBody([...]);
        $subject = $emailTemplate->getSubject([...]);

        // Generate the email
        $this->to($event->user->getName(), $event->user->getEmail())
            ->from($event->context->getName(), $event->context->getEmail())
            ->subject($subject)
            ->body($body)
            ->variables($event->context, $event->submission);

        // Send the email
        $this->send();
    }
}
Vitaliy-1 commented 3 years ago

According to Laravel's Mail Service logic, view is the message body (that may require additional rendering). Mailer either treats it as a simple HTML or transfers to the View Service for rendering, e.g., if it's path to the template. In my adaptation, Mailer renders the view and substitutes variables with real data.

@NateWr, In your example you suggest that EmailTemplate should be responsible for compiling data (variables)?

Vitaliy-1 commented 3 years ago

I mean this part: https://github.com/Vitaliy-1/pkp-lib/blob/f7a09d229b582eeebda57886c643e5fc888ed1c8/classes/mail/Mailer.inc.php#L55

NateWr commented 3 years ago

One thing you'll need to account for is the ability to intercept all send requests. The current mail system offers a hook, Mail::send. Laravel probably has a technique for this (to support third-party senders). But we'll also need to support cases like the MailLogger plugin.

Vitaliy-1 commented 3 years ago

As part of email templates refactoring, to make variables more universal and allow their handling in custom templates associated with a certain email/event, variables are going to be unified. Right now there are variables with different names but with the same meaning and vice versa. I keep the info about variables and renames here, proposals are welcome

asmecher commented 3 years ago

Thanks, @Vitaliy-1! Is it necessary to use submissionUrlByUserRole? Couldn't we just use submissionUrl?

Vitaliy-1 commented 3 years ago

PRs pkp-lib: https://github.com/pkp/pkp-lib/pull/7221 OJS: https://github.com/pkp/ojs/pull/3169

Vitaliy-1 commented 3 years ago

@NateWr, can you take a look?

Vitaliy-1 commented 3 years ago

@NateWr, it's ready for the second round of review.

Vitaliy-1 commented 3 years ago

PRs OMP: https://github.com/pkp/omp/pull/1006 OPS: https://github.com/pkp/ops/pull/178 OJS: https://github.com/pkp/ojs/pull/3169 pkp-lib: https://github.com/pkp/pkp-lib/pull/7221

NateWr commented 3 years ago

This looks really great, @Vitaliy-1. I didn't look over all the code again, but I reviewed the new sender/recipient stuff and just had two small comments.

I've been thinking about how to document this and share it with the team. A rough outline is here: https://github.com/pkp/pkp-docs/issues/825