#2401 Send email using celery
Opened 6 years ago by vivekanand1101. Modified 3 years ago
vivekanand1101/pagure cemale  into  master

@@ -0,0 +1,105 @@ 

+ # coding=utf-8

+ 

+ """

+  (c) 2017 - Copyright Red Hat Inc

+ 

+  Authors:

+      Pierre-Yves Chibon <pingou@pingoured.fr>

+      Vivek Anand <vivekanand1101@gmail.com>

+ 

+ """

+ 

+ import hashlib

+ import smtplib

+ 

+ from email.header import Header

+ from email.mime.text import MIMEText

+ 

+ import pagure

+ 

+ 

+ def _prepare_individual_email(

+         header, subject, subject_tag, from_email,

+         mail_id, in_reply_to, project_name, mailto, text):

+     ''' Prepares each email before sending or printing to console '''

+ 

+     from pagure.lib import notify

+     msg = MIMEText(text.encode('utf-8'), 'plain', 'utf-8')

+     msg['Subject'] = header = Header(

+         '[%s] %s' % (subject_tag, subject), 'utf-8')

+     msg['From'] = from_email

+ 

+     if mail_id:

+         msg['mail-id'] = mail_id

+         msg['Message-Id'] = '<%s>' % mail_id

+ 

+     if in_reply_to:

+         msg['In-Reply-To'] = '<%s>' % in_reply_to

+ 

+     msg['X-Auto-Response-Suppress'] = 'All'

+     msg['X-pagure'] = pagure.APP.config['APP_URL']

+     if project_name is not None:

+         msg['X-pagure-project'] = project_name

+         msg['List-ID'] = project_name

+         msg['List-Archive'] = notify._build_url(

+             pagure.APP.config['APP_URL'],

+             notify._fullname_to_url(project_name))

+ 

+     # Send the message via our own SMTP server, but don't include the

+     # envelope header.

+     if isinstance(mailto, unicode):

+         mailto = mailto.encode('utf-8')

+     msg['To'] = mailto

+     salt = pagure.APP.config.get('SALT_EMAIL')

+     if isinstance(mail_id, unicode):

+         mail_id = mail_id.encode('utf-8')

+     mhash = hashlib.sha512('<%s>%s%s' % (mail_id, salt, mailto))

+     msg['Reply-To'] = 'reply+%s@%s' % (

+         mhash.hexdigest(),

+         pagure.APP.config['DOMAIN_EMAIL_NOTIFICATIONS'])

+     msg['Mail-Followup-To'] = msg['Reply-To']

+     return (msg, header)

+ 

+ 

+ def _print_email_to_console(

+         from_email, mailto, subject, in_reply_to, mail_id, text, msg):

+     ''' Prints the email to be sent in the worker's console, invoked when

+     EMAIL_SEND is False '''

+ 

+     print('******EMAIL******')

+     print('From: %s' % from_email)

+     print('To: %s' % mailto)

+     print('Subject: %s' % subject)

+     print('in_reply_to: %s' % in_reply_to)

+     print('mail_id: %s' % mail_id)

+     print('Contents:')

+     print(text.encode('utf-8'))

+     print('*****************')

+     print(msg.as_string())

+     print('*****/EMAIL******')

+ 

+ 

+ def _send_email(smtp, from_email, mailto, msg):

+     ''' Send the prepared mail to the respective users '''

+ 

+     if smtp is None:

+         if pagure.APP.config['SMTP_SSL']:

+             smtp = smtplib.SMTP_SSL(

+                 pagure.APP.config['SMTP_SERVER'],

+                 pagure.APP.config['SMTP_PORT'])

+         else:

+             smtp = smtplib.SMTP(

+                 pagure.APP.config['SMTP_SERVER'],

+                 pagure.APP.config['SMTP_PORT'])

+     if pagure.APP.config['SMTP_USERNAME'] \

+             and pagure.APP.config['SMTP_PASSWORD']:

+         smtp.login(

+             pagure.APP.config['SMTP_USERNAME'],

+             pagure.APP.config['SMTP_PASSWORD']

+         )

+ 

+     smtp.sendmail(

+         from_email,

+         [mailto],

+         msg.as_string())

+ 

file modified
+10 -94
@@ -15,21 +15,17 @@ 

  

  

  import datetime

- import hashlib

  import json

  import logging

  import urlparse

  import re

- import smtplib

  import time

  import warnings

  

  import flask

  import pagure

  

- from email.header import Header

- from email.mime.text import MIMEText

- 

+ from pagure.lib import tasks

  

  _log = logging.getLogger(__name__)

  
@@ -224,95 +220,15 @@ 

      :kwarg project_name: if defined, the name of the project

  

      '''

-     if not to_mail:

-         return

- 

-     from_email = pagure.APP.config.get(

-         'FROM_EMAIL', 'pagure@fedoraproject.org')

-     if user_from:

-         header = Header(user_from, 'utf-8')

-         from_email = '%s <%s>' % (header, from_email)

- 

-     if project_name is not None:

-         subject_tag = project_name

-     else:

-         subject_tag = 'Pagure'

- 

-     smtp = None

-     for mailto in to_mail.split(','):

-         msg = MIMEText(text.encode('utf-8'), 'plain', 'utf-8')

-         msg['Subject'] = header = Header(

-             '[%s] %s' % (subject_tag, subject), 'utf-8')

-         msg['From'] = from_email

- 

-         if mail_id:

-             msg['mail-id'] = mail_id

-             msg['Message-Id'] = '<%s>' % mail_id

- 

-         if in_reply_to:

-             msg['In-Reply-To'] = '<%s>' % in_reply_to

- 

-         msg['X-Auto-Response-Suppress'] = 'All'

-         msg['X-pagure'] = pagure.APP.config['APP_URL']

-         if project_name is not None:

-             msg['X-pagure-project'] = project_name

-             msg['List-ID'] = project_name

-             msg['List-Archive'] = _build_url(

-                 pagure.APP.config['APP_URL'],

-                 _fullname_to_url(project_name))

- 

-         # Send the message via our own SMTP server, but don't include the

-         # envelope header.

-         if isinstance(mailto, unicode):

-             mailto = mailto.encode('utf-8')

-         msg['To'] = mailto

-         salt = pagure.APP.config.get('SALT_EMAIL')

-         if isinstance(mail_id, unicode):

-             mail_id = mail_id.encode('utf-8')

-         mhash = hashlib.sha512('<%s>%s%s' % (mail_id, salt, mailto))

-         msg['Reply-To'] = 'reply+%s@%s' % (

-             mhash.hexdigest(),

-             pagure.APP.config['DOMAIN_EMAIL_NOTIFICATIONS'])

-         msg['Mail-Followup-To'] = msg['Reply-To']

-         if not pagure.APP.config.get('EMAIL_SEND', True):

-             print('******EMAIL******')

-             print('From: %s' % from_email)

-             print('To: %s' % to_mail)

-             print('Subject: %s' % subject)

-             print('in_reply_to: %s' % in_reply_to)

-             print('mail_id: %s' % mail_id)

-             print('Contents:')

-             print(text.encode('utf-8'))

-             print('*****************')

-             print(msg.as_string())

-             print('*****/EMAIL******')

-             continue

-         try:

-             if smtp is None:

-                 if pagure.APP.config['SMTP_SSL']:

-                     smtp = smtplib.SMTP_SSL(

-                         pagure.APP.config['SMTP_SERVER'],

-                         pagure.APP.config['SMTP_PORT'])

-                 else:

-                     smtp = smtplib.SMTP(

-                         pagure.APP.config['SMTP_SERVER'],

-                         pagure.APP.config['SMTP_PORT'])

-             if pagure.APP.config['SMTP_USERNAME'] \

-                     and pagure.APP.config['SMTP_PASSWORD']:

-                 smtp.login(

-                     pagure.APP.config['SMTP_USERNAME'],

-                     pagure.APP.config['SMTP_PASSWORD']

-                 )

- 

-             smtp.sendmail(

-                 from_email,

-                 [mailto],

-                 msg.as_string())

-         except smtplib.SMTPException as err:

-             _log.exception(err)

-     if smtp:

-         smtp.quit()

-     return msg

+     tasks.send_email.delay(

+         text=text,

+         subject=subject,

+         to_mail=to_mail,

+         mail_id=mail_id,

+         in_reply_to=in_reply_to,

+         project_name=project_name,

+         user_from=user_from,

+     )

  

  

  def notify_new_comment(comment, user=None):

file modified
+104 -10
@@ -22,12 +22,16 @@ 

  import six

  

  import logging

+ import smtplib

  

  import pagure

  from pagure import APP

  import pagure.lib

  import pagure.lib.git

  import pagure.lib.git_auth

+ from pagure.lib import mail_helpers

+ 

+ from email.header import Header

  

  logging.config.dictConfig(APP.config.get('LOGGING') or {'version': 1})

  _log = logging.getLogger(__name__)
@@ -286,8 +290,13 @@ 

                                             content, message, userobj, email)

  

      session.remove()

-     return ret('view_commits', repo=project.name, username=user,

-                namespace=namespace, branchname=branchto)

+     return ret(

+         'view_commits',

+         repo=project.name,

+         username=user,

+         namespace=namespace,

+         branchname=branchto

+     )

  

  

  @conn.task
@@ -306,7 +315,8 @@ 

              _log.exception(err)

  

      session.remove()

-     return ret('view_repo', repo=name, namespace=namespace, username=user)

+     return ret(

+         'view_repo', repo=name, namespace=namespace, username=user)

  

  

  @conn.task
@@ -412,12 +422,17 @@ 

      gc_clean()

  

      if editfile is None:

-         return ret('view_repo', repo=name, namespace=namespace,

-                    username=user_forker)

+         return ret(

+             'view_repo', repo=name, namespace=namespace, username=user_forker)

      else:

-         return ret('edit_file', repo=name, namespace=namespace,

-                    username=user_forker, branchname=editbranch,

-                    filename=editfile)

+         return ret(

+             'edit_file',

+             repo=name,

+             namespace=namespace,

+             username=user_forker,

+             branchname=editbranch,

+             filename=editfile

+         )

  

  

  @conn.task
@@ -463,7 +478,8 @@ 

      refresh_pr_cache.delay(name, namespace, user)

      session.remove()

      gc_clean()

-     return ret('view_repo', repo=name, username=user, namespace=namespace)

+     return ret(

+         'view_repo', repo=name, username=user, namespace=namespace)

  

  

  @conn.task
@@ -510,7 +526,8 @@ 

      session.remove()

      gc_clean()

  

-     return ret('view_repo', repo=name, username=user, namespace=namespace)

+     return ret(

+         'view_repo', repo=name, username=user, namespace=namespace)

  

  

  @conn.task
@@ -544,3 +561,80 @@ 

  

      session.remove()

      gc_clean()

+ 

+ 

+ @conn.task

+ def send_email(text, subject, to_mail,

+                mail_id=None, in_reply_to=None,

+                project_name=None, user_from=None):  # pragma: no cover

+     ''' Send an email with the specified information.

+ 

+     :arg text: the content of the email to send

+     :type: str

+     :arg subject: the subject of the email

+     :type: str

+     :arg to_mail: a string representing a list of recipients separated by a

+         comma

+     :type: str

+     :kwarg mail_id: if defined, the header `mail-id` is set with this value

+     :type: None or str

+     :kwarg in_reply_to: if defined, the header `In-Reply-To` is set with

+         this value

+     :type: None or str

+     :kwarg project_name: if defined, the name of the project

+     :type: None or str

+     :kwarg user_from: if defined, the email id of the user in the name of whom

+         the email is being sent.

+     :type: None or str

+ 

+     '''

+ 

+     if not to_mail:

+         return

+ 

+     from_email = pagure.APP.config.get(

+         'FROM_EMAIL', 'pagure@fedoraproject.org')

+     if user_from:

+         header = Header(user_from, 'utf-8')

+         from_email = '%s <%s>' % (header, from_email)

+ 

+     if project_name is not None:

+         subject_tag = project_name

+     else:

+         subject_tag = 'Pagure'

+ 

+     smtp = None

+     for mailto in to_mail.split(','):

+         msg, header = mail_helpers._prepare_individual_email(

+             header=header,

+             subject=subject,

+             subject_tag=subject_tag,

+             from_email=from_email,

+             mail_id=mail_id,

+             in_reply_to=in_reply_to,

+             project_name=project_name,

+             mailto=mailto,

+             text=text,

+         )

+ 

+         if not pagure.APP.config.get('EMAIL_SEND', True):

+             # This will be viewed on the console where the worker is running

+             mail_helpers._print_email_to_console(

+                 from_email=from_email,

+                 mailto=mailto,

+                 subject=subject,

+                 in_reply_to=in_reply_to,

+                 mail_id=mail_id,

+                 text=text,

+                 msg=msg,

+             )

+             continue

+         # Send it to user if EMAIL_SEND is Enabled

+         try:

+             mail_helpers._send_email(

+                 smtp=smtp, from_email=from_email, mailto=mailto, msg=msg)

+         except smtplib.SMTPException as err:

+             _log.exception(err)

+     if smtp:

+         smtp.quit()

+ 

rebased

6 years ago

i will update the docs when you confirm this is correct

s/coma/comma/ (hope none of the recipients get a coma! ☺)

It would be good to document the user_from argument. It would also be good to document the types of these arguments, though I assume they are all basestrings?

For PEP-8, this should be at the top of the file.

I realize you are just moving this code from one place to another place, but I suggest considering breaking the above function into a few helpers so it's not quite so long. It'll make it easier to understand, and also easier to test.

This looks good to me, though I suggest testing the change if feasible.

That's because, i have to import this file (lib.tasks) in lib.notify :/

2 new commits added

  • send_email: 'To' field should contain individual email id in each mail
  • Refactor code for send_email
6 years ago

3 new commits added

  • send_email: 'To' field should contain individual email id in each mail
  • Refactor code for send_email
  • Send email using celery
6 years ago

Circular imports are a sign that there is an organizational problem in the code. If module A needs module B and module B needs module A, it is likely that module A and B should either be the same module, or that some item or items from them should be brought into a third module C that they both use. I suggest refactoring to fix this.

okay. I tried moving all the helper functions to different file - pagure.lib.task_helpers still i am facing circular imports :/ . There are two methods of lib.notify which are being used in task_helpers. Those two methods are also being used in lib.notify(more often). :/

methods are: _build_url and _fullname_to_url (i thought of making it public but that's different.)

To clarify, I'm not demanding and not necessarily even recommending fixing the circular import problem in this PR, unless this PR is introducing the circular nature (and even then I'm not demanding it ☺). If the circular nature pre-exists this PR, I say it's best left as a separate concern (and a separate PR). However, if this PR is introducing circular dependencies I highly recommend addressing that before merging as it indicates an organizational problem.

To clarify, I'm not demanding and not necessarily even recommending fixing the circular import problem in this PR, unless this PR is introducing the circular nature (and even then I'm not demanding it ☺). If the circular nature pre-exists this PR, I say it's best left as a separate concern (and a separate PR). However, if this PR is introducing circular dependencies I highly recommend addressing that before merging as it indicates an organizational problem.

Chill, i take your words seriously.

rebased

6 years ago

1 new commit added

  • send_email: move import of lib.notify inside the method
6 years ago

Modules don't need a shebang :)

Editor just puts it there, i will remove

4 new commits added

  • send_email: move import of lib.notify inside the method
  • send_email: 'To' field should contain individual email id in each mail
  • Refactor code for send_email
  • Send email using celery
6 years ago

This shouldn't be there, if you re-configure the logger while it was already configured before, you're going to loose all the info (been there done that)

Not sure I would have moved these two functions (and then this file would be a mailer_helper maybe?)

Couple of comments but this is looking nice.

My only concern is about the workers themselves, I've noticed it sometime takes a while to get a PR merged for example and I think that's because the workers are just a little overloaded, so I would prefer to wait a little before merging this PR so we put in place some monitoring of the workers' load and see if we can spin up some more. Otherwise receiving emails might end up being delayed.

1 new commit added

  • send mail: only mail helpers in differnt file
6 years ago

5 new commits added

  • send mail: only mail helpers in differnt file
  • send_email: move import of lib.notify inside the method
  • send_email: 'To' field should contain individual email id in each mail
  • Refactor code for send_email
  • Send email using celery
6 years ago

5 new commits added

  • send mail: only mail helpers in differnt file
  • send_email: move import of lib.notify inside the method
  • send_email: 'To' field should contain individual email id in each mail
  • Refactor code for send_email
  • Send email using celery
6 years ago

5 new commits added

  • send mail: only mail helpers in differnt file
  • send_email: move import of lib.notify inside the method
  • send_email: 'To' field should contain individual email id in each mail
  • Refactor code for send_email
  • Send email using celery
6 years ago

rebased

6 years ago

rebased onto ae14103

5 years ago

rebased onto 32e8623

5 years ago

@vivekanand1101 is this something you are still interesting to bring in?

Sorry for dropping the ball on you :(

@pingou Oh man, this is probably a really good idea to do, how difficult would it be to rebase this and bring it in now?