When gerrit sends emails from itself for a change (as opposed to

on behalf of someone), those messages should also go into the
change history.

webapp/codereview/email.py

@@ -34,6 +34,8 @@ def _encode_safely(s):
   return s
 
 def _to_email_string(obj):
+  if not obj:
+    return None
   if isinstance(obj, str):
     return obj
   elif isinstance(obj, unicode):
@@ -50,12 +52,18 @@ def _to_email_string(obj):
     result = str(email)
   return _encode_safely(result)
 
+def _make_to_strings(to):
+  return [x for x in [_to_email_string(e) for e in to] if x]
+
+
 def send(sender, to, subject, template, template_args):
   """Sends an email based on a template.
 
   All email address parameters can be: strings, unicode, db.Email users.User
   or models.Account objects.
 
+  Returns a Message object, suitable for keeping in the db.
+
   Args:
     sender:   The From address.  Null if it should be sent from the role acct.
     to:       An email address or a list of email address to be in the To field.
@@ -63,16 +71,16 @@ def send(sender, to, subject, template, template_args):
     template: The name of the template file to use from the mail dir.
     template_args:  A map of args to be pasaed to the template
   """
-  if not sender:
-    sender_string = get_default_sender()
-    if not sender_string:
-      logging.warn('not sending email because there is no from address')
-      return 'not sending email because there is no from address'
-  else:
-    sender_string = _to_email_string(sender)
-  to_string = [_to_email_string(e) for e in to]
+  sender_string = _to_email_string(sender)
+  if not sender_string:
+    return 'no from address'
+  to_strings = _make_to_strings(to)
+  if not to_strings:
+    return 'no to addresses'
   body = django.template.loader.render_to_string(template, template_args)
-  mail.send_mail(sender=sender_string, to=to_string, subject=subject, body=body)
+  mail.send_mail(sender=sender_string, to=to_strings, subject=subject,
+                  body=body)
+
 
 def make_change_subject(change):
   subject = "(%s) %s" % (change.dest_project.name, change.subject)
@@ -80,14 +88,49 @@ def make_change_subject(change):
     subject = 'Re: ' + subject
   return subject
 
-def send_change_message(request, change, template, template_args):
+def send_change_message(request, change, template, template_args,
+                        sender, send_email=True, email_template=None):
+  # sender
+  if not sender:
+    sender = get_default_sender()
+  sender_string = _to_email_string(sender)
+
+  # to
   to_users = set([change.owner] + change.reviewers + change.cc)
+  if sender in to_users:
+    to_users.remove(sender)
+  to_strings = _make_to_strings(to_users)
+  to_emails = [db.Email(s) for s in to_strings]
+
+  # subject
   subject = make_change_subject(change)
-  args = {
-      'url': library.change_url(change)
-    }
-  if template_args:
-    args.update(template_args)
-  email.send(request.user, to_users, subject, template, args)
+
+  # body
+  uri = library.change_url(change)
+  if not email_template:
+    email_template = template
+  body = django.template.loader.render_to_string(email_template, template_args)
+
+  # don't send emails without all of these fields
+  if not sender_string or not to_strings or not subject or not body:
+    return None
+
+  # send the email
+  if send_email:
+    message_body = "%s\n--\n%s\n" % (body, uri)
+    mail.send_mail(sender=sender_string, to=to_strings, subject=subject,
+                    body=message_body)
+  
+  # make and return the email
+  if email_template != template:
+    body = django.template.loader.render_to_string(
+                                          template, template_args)
+  msg = models.Message(change=change,
+                       subject=subject,
+                       sender=sender_string,
+                       recipients=to_emails,
+                       text=db.Text(body),
+                       parent=change)
+  return msg
 
 

webapp/codereview/internal/merge_service.py

@@ -29,22 +29,27 @@ from codereview import email
 
 def _send_clean_merge_email(http_request, change):
   if not change.emailed_clean_merge:
-    email.send_change_message(http_request, change,
-                              "mails/clean_merge.txt", None)
+    msg = email.send_change_message(http_request, change,
+                              "mails/clean_merge.txt", None, None)
     change.emailed_clean_merge = True
+    return msg
+  return None
   
 def _send_missing_dependency_merge_email(http_request, change):
   if not change.emailed_clean_merge:
-    email.send_change_message(http_request, change,
-                              "mails/missing_dependency.txt", None)
+    msg = email.send_change_message(http_request, change,
+                              "mails/missing_dependency.txt", None, None)
     change.emailed_missing_dependency = True
+    return msg
+  return None
 
 def _send_path_conflict_email(http_request, change):
   if not change.emailed_clean_merge:
-    email.send_change_message(http_request, change,
-                              "mails/path_conflict.txt", None)
+    msg = email.send_change_message(http_request, change,
+                              "mails/path_conflict.txt", None, None)
     change.emailed_path_conflict = True
-
+    return msg
+  return None
 
 class InvalidBranchStatusError(Exception):
   """The branch cannot be updated in this way at this time."""
@@ -115,22 +120,30 @@ class MergeServiceImp(MergeService, InternalAPI):
           change.put()
 
         return True
+
       if db.run_in_transaction(chg_trans, ps.change.key()):
         if sc == MergeResultItem.CLEAN_MERGE:
-          _send_clean_merge_email(self.http_request, ps.change)
+          msg = _send_clean_merge_email(self.http_request, ps.change)
           ps.change.put()
+          if msg:
+            msg.put()
 
         elif sc == MergeResultItem.ALREADY_MERGED:
           success.append(ps)
 
         elif sc == MergeResultItem.MISSING_DEPENDENCY:
-          _send_missing_dependency_merge_email(self.http_request, ps.change)
+          msg = _send_missing_dependency_merge_email(self.http_request,
+                                                     ps.change)
           ps.change.put()
+          if msg:
+            msg.put()
           defer.append(ps)
 
         elif sc == MergeResultItem.PATH_CONFLICT:
-          _send_path_conflict_email(self.http_request, ps.change)
+          msg = _send_path_conflict_email(self.http_request, ps.change)
           ps.change.put()
+          if msg:
+            msg.put()
           fail.append(ps)
       else:
         fail.append(ps)

webapp/codereview/views.py

@@ -1294,20 +1294,7 @@ def _get_draft_details(request, comments):
 def _make_comment_message(request, change, lgtm, verified, message,
                   comments=None, send_mail=False):
   """Helper to create a Message instance and optionally send an email."""
-  # Decide who should receive mail
-  my_email = db.Email(request.user.email())
-  to = [db.Email(change.owner.email())] + change.reviewers
-  cc = change.cc[:]
-  reply_to = to + cc
-  if my_email in to and len(to) > 1:  # send_mail() wants a non-empty to list
-    to.remove(my_email)
-  if my_email in cc:
-    cc.remove(my_email)
-  subject = email.make_change_subject(change)
-  if comments:
-    details = _get_draft_details(request, comments)
-  else:
-    details = ''
+
   prefix = ''
   if lgtm:
     prefix = prefix + [y for (x,y) in models.LGTM_CHOICES
@@ -1316,26 +1303,24 @@ def _make_comment_message(request, change, lgtm, verified, message,
     prefix = prefix + 'Verified.\n'
   if prefix:
     prefix = prefix + '\n'
+
   message = message.replace('\r\n', '\n')
   message = prefix + message
-  text = ((message.strip() + '\n\n' + details.strip())).strip()
-  msg = models.Message(change=change,
-                       subject=subject,
-                       sender=my_email,
-                       recipients=reply_to,
-                       text=db.Text(text),
-                       parent=change)
 
-  if send_mail:
-    to_users = set([change.owner] + change.reviewers + cc)
-    template_args = {
-        'message': message,
-        'details': details,
-      }
-    email.send_change_message(request, change,
-                              'mails/comment.txt', template_args)
+  if comments:
+    details = _get_draft_details(request, comments)
+  else:
+    details = ''
 
-  return msg
+  template_args = {
+      'message': message,
+      'details': details,
+    }
+
+  return email.send_change_message(
+                              request, change,
+                              'mails/comment.txt', template_args, request.user,
+                              send_mail, 'mails/comment-email.txt')
 
 
 @xsrf_required

webapp/templates/mails/clean_merge.txt

@@ -3,8 +3,6 @@ Hi.
 
 Your change has been successfully merged into the git repository.
 
-For more details, see {%autoescape off%}{{url}}{%endautoescape%}
-
 -Your friendly git merger
 
 {%endautoescape%}

webapp/templates/mails/comment-email.txt

@@ -0,0 +1,4 @@
+{%autoescape off%}{%if message%}{{message|wordwrap:"72"}}
+
+{%endif%}{%if details%}{{details|wordwrap:"72"}}
+{%endif%}{%endautoescape%}

webapp/templates/mails/comment.txt

@@ -1,7 +1,2 @@
 {%autoescape off%}{%if message%}{{message|wordwrap:"72"}}
-
-{%endif%}{%if details%}{{details|wordwrap:"72"}}
 {%endif%}{%endautoescape%}
---
-To respond, visit {%autoescape off%}{{url}}{%endautoescape%}
-

webapp/templates/mails/missing_dependency.txt

@@ -2,12 +2,9 @@
 Hi.
 
 Your change could not be merged because of a missing dependency.  As
-soon as change XXX is submitted, your change will be submitted
+soon as its dependencies are submitted, your change will be submitted
 automatically.
 
-Your change: {%autoescape off%}{{url}}{%endautoescape%}
-The missing change: {%autoescape off%}{{url}}{%endautoescape%}
-
 -Your friendly git merger
 
 {%endautoescape%}

webapp/templates/mails/path_conflict.txt

@@ -1,9 +1,8 @@
 {%autoescape off%}
 Hi.
 
-Your change has not been successfully merged into the git repository.
-
-You need to resolve the following files before it can be submitted:
+Your change has not been successfully merged into the git repository
+because of a path conflict.
 
 -Your friendly git merger