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

Implement Email Notification Consumer #395

Merged
merged 1 commit into from Aug 28, 2019

Conversation

@aquibbaig
Copy link
Collaborator

aquibbaig commented Jul 24, 2019

Fixes #393

TODO:

  • Decide the email template
  • Fix not able to send mails until flush
@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch 2 times, most recently from ae5c3c2 to 284a98c Jul 24, 2019
<body>
<div class="container content">
<h4 class="salutation">Hi {{ user }},</h4>
<p class="body">You have a new notification from Librecores website. {{ content }}</p>

This comment has been minimized.

Copy link
@agathver

agathver Jul 31, 2019

Collaborator

Put content in a separate DIV.

Use this: https://github.com/rodriguezcommaj/salted as a base for all emails. We already use it in password reset emails

@@ -12,26 +13,78 @@
*/
class EmailNotificationConsumer extends AbstractNotificationConsumer
{
const LIBRECORES_MAIL_ID = "abcd@gmail.com";

This comment has been minimized.

Copy link
@agathver

agathver Jul 31, 2019

Collaborator

Should go into parameters.yaml

$message = (new \Swift_Message($this->notification->getSubject()))
->setFrom(self::LIBRECORES_MAIL_ID)
->setTo($userEmail)
->setBody($this->twig->render('template/email_template.html.twig', [

This comment has been minimized.

Copy link
@agathver

agathver Jul 31, 2019

Collaborator

Always add a plain text version as well

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 7, 2019

Author Collaborator

@agathver I'm sorry but what do we intend by doing that ?

This comment has been minimized.

Copy link
@imphil

imphil Aug 7, 2019

Contributor

Emails can contain two versions, a HTML version and a plaintext version. Some people have configured their email client to always view the plaintext version. That's why it is needed. Look at the emails we send out for email address confirmation for an example how to do that.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 12, 2019

Author Collaborator

Swiftmailer provides an addPart() option to set an alternate body. Currently I've set only the notification message inside it. Do we need to add anything else?

This comment has been minimized.

Copy link
@imphil

imphil Aug 12, 2019

Contributor

Please us a template also for the plaintext version so that we can modify the surrounding text. We'll need to do that to add sender and unsubscribe information.

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 31, 2019

Screenshot 2019-07-31 at 10 14 22 AM

@imphil This is how it looks currently

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 284a98c to da76536 Jul 31, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Jul 31, 2019

@imphil @agathver I have added the template from https://github.com/rodriguezcommaj/salted with some minor tweaks(Haven't used translations, yet. Do we need to do that?). Also, setting up configuration for Swiftmailer in parameter.yml needs a bit of discussion as well. I think we have to get $transport and $spool from container and force a $spool->flush() to send emails actually which won't be sent until the crontab actually terminates. check this Stack Overflow link. Thoughts?

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Jul 31, 2019

We don't use SwiftMailer spooling today, so no explicit flush is necessary. But it's probably a good idea to add a comment to the parameter.yml.j2 file giving a link to #398 in case this parameter is changed.

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from da76536 to 8063e8a Aug 1, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 2, 2019

@imphil There are a lot of issues in production regarding mails not being sent via swiftmailer, so we need to carefully configure things here.

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 3, 2019

@imphil Currently, when I send emails, I get the following exception

Exception occurred while flushing email queue: Connection could not be established with host smtp.gmail.com [phip_network_getaddresses: getaddrinfo failed: Temporary failure in name resolution #0]

So, I went on to do a ping smtp.gmail.com and ping google.com inside the vm. For both the commands I got an Unknown host google/smtp.gmail.com

Is there some reason why the DNS is not getting resolved or is it a temporary issue?

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 4, 2019

Seems like

@imphil Currently, when I send emails, I get the following exception

Exception occurred while flushing email queue: Connection could not be established with host smtp.gmail.com [phip_network_getaddresses: getaddrinfo failed: Temporary failure in name resolution #0]

So, I went on to do a ping smtp.gmail.com and ping google.com inside the vm. For both the commands I got an Unknown host google/smtp.gmail.com

Is there some reason why the DNS is not getting resolved or is it a temporary issue?

Seems like a temporary issue, just restarted the VM. It's gone (Phew). Probably it was due to me doing a git checkout <branch> without restarting the VM to restore swiftmailer config that I had changed in this branch

@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 4, 2019

We need a bit of discussion to switch to some other kind of email service for our app because for SwiftMailer, you need to set allow least secure apps to ON here. Else it doesn't work. Should we switch to some other service such as SendGrid/PhpMailer? I tried changing the post from 465 to 587 and so on but no joy!

Currently, PHPMailer seems to be failing in the build, idk if it would be good enough but we can try something other than SwiftMailer I suppose. Do you have any suggestions? @imphil

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 8063e8a to 2bbcb61 Aug 4, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 6, 2019

Probably, setting up an app password for a google account will solve this for us.

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 2bbcb61 to 2b0850f Aug 6, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 6, 2019

Probably, setting up an app password for a google account will solve this for us.

It does solve our problem. However, in some cases, the email can hit your spam folder too, if setFrom() field doesn't match the sender address in parameters.yml

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 2b0850f to 92350be Aug 6, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 6, 2019

Configuration needs to be set up in dev-vagrant.secrets.yml as follows:

  1. Set your Email in site_smtp_user (this is already there when you create a VM)
  2. For site_smtp_password use a google app password preferably otherwise, it doesn't send emails to accounts with 2 step verification/allow less secure apps not turned on. See SwiftMailer documentation
  3. Host: smtp.gmail.com
  4. Transport: smtp
  5. Port: 465
    Other configurations are default. Spooling is disabled.
$message = (new \Swift_Message($this->notification->getSubject()))
->setFrom(self::LIBRECORES_MAIL_ID)
->setTo($userEmail)
->setBody($this->twig->render('template/email_template.html.twig', [

This comment has been minimized.

Copy link
@imphil

imphil Aug 7, 2019

Contributor

Emails can contain two versions, a HTML version and a plaintext version. Some people have configured their email client to always view the plaintext version. That's why it is needed. Look at the emails we send out for email address confirmation for an example how to do that.

site/src/Consumer/EmailNotificationConsumer.php Outdated Show resolved Hide resolved
/**
* @var Environment
*/
protected $twig;

This comment has been minimized.

Copy link
@imphil

imphil Aug 7, 2019

Contributor

twig -> twigEnvironment

This comment has been minimized.

Copy link
@imphil

imphil Aug 12, 2019

Contributor

You resolved this, but didn't change the code?

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 92350be to 76cf2d8 Aug 8, 2019
@aquibbaig aquibbaig marked this pull request as ready for review Aug 8, 2019
@aquibbaig aquibbaig requested a review from imphil Aug 8, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch 2 times, most recently from b20f889 to b196ea2 Aug 12, 2019
/**
* @var Environment
*/
protected $twig;

This comment has been minimized.

Copy link
@imphil

imphil Aug 12, 2019

Contributor

You resolved this, but didn't change the code?

protected function shouldHandle()
{
$type = $this->notification->getType();
// TODO: Check subscription status also from database, after merge of #387

This comment has been minimized.

Copy link
@imphil

imphil Aug 12, 2019

Contributor

It's merged now.

$message = (new \Swift_Message($this->notification->getSubject()))
->setFrom(self::LIBRECORES_MAIL_ID)
->setTo($userEmail)
->setBody($this->twig->render('template/email_template.html.twig', [

This comment has been minimized.

Copy link
@imphil

imphil Aug 12, 2019

Contributor

Please us a template also for the plaintext version so that we can modify the surrounding text. We'll need to do that to add sender and unsubscribe information.

<table width="100%" border="0" cellspacing="0" cellpadding="0" align="center">
<tr>
<td style="padding: 20px 0px 20px 0px;">
<!-- UNSUBSCRIBE COPY -->

This comment has been minimized.

Copy link
@imphil

imphil Aug 12, 2019

Contributor

You need to include a way to unsubscribe from these emails. Include a link to the settings page.

This comment has been minimized.

Copy link
@aquibbaig

aquibbaig Aug 12, 2019

Author Collaborator

Sure

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch 4 times, most recently from d05c76f to f8f4bb9 Aug 13, 2019
@aquibbaig aquibbaig added the gsoc label Aug 17, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Aug 19, 2019

I'll make the URL available in #415

@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Aug 19, 2019

On the configuration:

  • Create a new file app/config/packages/notifications.yml.
  • Set something like

            from_email:
                address:        'notifications@librecores.org'
                sender_name:    'LibreCores'
  • Import this file in app/config/config.yml
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 19, 2019

On the configuration:

  • Create a new file app/config/packages/notifications.yml.
  • Set something like

            from_email:
                address:        'notifications@librecores.org'
                sender_name:    'LibreCores'
  • Import this file in app/config/config.yml

Was just having a research on this. Turns out we can't do stuff this way as per Symfony standards. We have to register an extension first and configure the loader to actually access custom parameters in Symfony.

If we create a new file in packages and import it in app/config.yml, it throws us the following:

There is no extension able to load the configuration for "notifications" (in /var/www/lc/site/app/config/packages/notifications.yml). Looked for namespace "notifications", found "framework", "security", "twig", "monolog", "swiftmailer", "doctrine", "sensio_framework_extra", "knp_markdown", "old_sound_rabbit_mq", "fos_user", "hwi_oauth", "exercise_html_purifier", "doctrine_migrations", "httplug", "algolia_search", "webpack_encore", "mgilet_notification", "debug", "web_profiler", "sensio_distribution", "doctrine_fixtures", "easy_admin" in /var/www/lc/site/app/config/packages/notifications.yml (which is being imported from "/var/www/lc/site/app/config/config.yml").

@imphil Please refer to this link

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 701a803 to 1a64f3a Aug 19, 2019
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Aug 20, 2019

@aquibbaig you can now find the URL in the parameters.yml file (after you update master and re-run vagrant provision).


    app.librecores_domain: 'librecores.devel'
    app.librecores_url: 'http://www.librecores.devel'

You can use a similar technique to set the notification email from in parameters.yml.

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 1a64f3a to 8891594 Aug 20, 2019
@aquibbaig aquibbaig changed the title Email Notification Consumer Implement Email Notification Consumer Aug 20, 2019
@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 8891594 to fb091b8 Aug 20, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 21, 2019

@aquibbaig you can now find the URL in the parameters.yml file (after you update master and re-run vagrant provision).


    app.librecores_domain: 'librecores.devel'
    app.librecores_url: 'http://www.librecores.devel'

You can use a similar technique to set the notification email from in parameters.yml.

Okay, Philipp

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch 2 times, most recently from 0e44c24 to 725d00c Aug 21, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 22, 2019

@imphil made certain changes. Please have a look! :D

Copy link
Contributor

imphil left a comment

Thanks for your update @aquibbaig, we're almost there!

ansible/roles/web/templates/parameters.yml.j2 Outdated Show resolved Hide resolved
ansible/vars/dev-vagrant.yml Outdated Show resolved Hide resolved
<td align="left" style="font-size: 25px; font-family: Helvetica, Arial, sans-serif; color: rgba(63, 176, 172, 1); padding-top: 30px;" class="padding-copy">Hi {{ username }},</td>
</tr>
<tr>
<td align="left" style="font-size:14px;padding: 20px 0 5px 0;border-bottom: 2px solid #e2e2e2">You have a new Notification from <b>LibreCores</b>.</td>

This comment has been minimized.

Copy link
@imphil

imphil Aug 22, 2019

Contributor

Notification -> notification

And it's easier to read if you limit the line to 100 characters.

site/src/Consumer/EmailNotificationConsumer.php Outdated Show resolved Hide resolved
site/src/Consumer/EmailNotificationConsumer.php Outdated Show resolved Hide resolved
site/src/Consumer/EmailNotificationConsumer.php Outdated Show resolved Hide resolved
site/src/Consumer/EmailNotificationConsumer.php Outdated Show resolved Hide resolved
@imphil

This comment has been minimized.

Copy link
Contributor

imphil commented Aug 22, 2019

On the logo: Please use a colored version, and name it something like "logo_email_notification.png" so that we remember in the future where this logo is used when we modify it.

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 725d00c to 7a41ff5 Aug 22, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 22, 2019

@imphil Thanks for your time for the review. Just made some changes. Please have a look

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 7a41ff5 to 2960d07 Aug 24, 2019
site/web/static/img/logo_email_notification.svg Outdated Show resolved Hide resolved
You can unsubscribe from these notifications in the
{{ notificationSettingsUrl }} of your LibreCores account.

--

This comment has been minimized.

Copy link
@imphil

imphil Aug 26, 2019

Contributor

The separator here is "-- " with a space at the end.

This comment has been minimized.

Copy link
@imphil

imphil Aug 28, 2019

Contributor

That doesn't seem to be resolved yet?


# Notification mailer
notification_from_address: "stage.librecores.org"
notification_from_name: "LibreCores Notifications"

This comment has been minimized.

Copy link
@imphil

imphil Aug 26, 2019

Contributor

You also need to update production.yml and staging.yml. And use the right domain for each of them, with librecores.devel being the domain for dev-vagrant.

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch 2 times, most recently from b306f12 to 7016c86 Aug 26, 2019
@@ -26,3 +26,7 @@ site_user_email_confirmation_enabled: true

# Algolia
site_algolia_search_prefix: 'staging_'

# Notification mailer
notification_from_address: "noreply@librecores.devel"

This comment has been minimized.

Copy link
@imphil

imphil Aug 28, 2019

Contributor
@@ -26,3 +26,7 @@ site_user_email_confirmation_enabled: true

# Algolia
site_algolia_search_prefix: 'production_'

# Notification mailer
notification_from_address: "noreply@librecores.org"

This comment has been minimized.

Copy link
@imphil
@@ -30,3 +30,7 @@ site_user_email_confirmation_enabled: false

# Algolia
site_algolia_search_prefix: 'dev_'

# Notification mailer
notification_from_address: "noreply@librecores.devel"

This comment has been minimized.

Copy link
@imphil
You can unsubscribe from these notifications in the
{{ notificationSettingsUrl }} of your LibreCores account.

--

This comment has been minimized.

Copy link
@imphil

imphil Aug 28, 2019

Contributor

That doesn't seem to be resolved yet?

$message = (new \Swift_Message($this->notification->getSubject()))
// Set sender's email from parameters.yml
->setFrom($this->container->getParameter('app.notification_from_address'))

This comment has been minimized.

Copy link
@imphil

imphil Aug 28, 2019

Contributor

You need to set both the email address and the name here.


{{ content }}

You are receiving this notification because you are subscribed to email notifications from LibreCores.

This comment has been minimized.

Copy link
@imphil

imphil Aug 28, 2019

Contributor

Keep this line below 72 characters.

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from 7016c86 to fe3eb20 Aug 28, 2019
@aquibbaig

This comment has been minimized.

Copy link
Collaborator Author

aquibbaig commented Aug 28, 2019

@imphil I have pushed the required changes

@aquibbaig aquibbaig force-pushed the aquibbaig:email-notifications branch from fe3eb20 to 8cfefab Aug 28, 2019
@imphil
imphil approved these changes Aug 28, 2019
Copy link
Contributor

imphil left a comment

Thanks for the constant stream of updates, @aquibbaig. That's merged now!

@imphil imphil merged commit 1705101 into librecores:master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.