From f36b62329d444e85fd9d0d0fe5b34aba2e1bab8f Mon Sep 17 00:00:00 2001 From: Joe Onorato Date: Tue, 21 Oct 2008 22:13:06 -0700 Subject: [PATCH] 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 | 75 +++++++++++++++---- webapp/codereview/internal/merge_service.py | 33 +++++--- webapp/codereview/views.py | 45 ++++------- webapp/templates/mails/clean_merge.txt | 2 - webapp/templates/mails/comment-email.txt | 4 + webapp/templates/mails/comment.txt | 5 -- webapp/templates/mails/missing_dependency.txt | 5 +- webapp/templates/mails/path_conflict.txt | 5 +- 8 files changed, 104 insertions(+), 70 deletions(-) create mode 100644 webapp/templates/mails/comment-email.txt diff --git a/webapp/codereview/email.py b/webapp/codereview/email.py index 95bd37ac3c..a9f68f5c1a 100644 --- a/webapp/codereview/email.py +++ b/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 diff --git a/webapp/codereview/internal/merge_service.py b/webapp/codereview/internal/merge_service.py index bf9c721f29..642981f6f0 100644 --- a/webapp/codereview/internal/merge_service.py +++ b/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) diff --git a/webapp/codereview/views.py b/webapp/codereview/views.py index 26b18afc79..88056145bf 100644 --- a/webapp/codereview/views.py +++ b/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 diff --git a/webapp/templates/mails/clean_merge.txt b/webapp/templates/mails/clean_merge.txt index de8fdf67c1..25384ef162 100644 --- a/webapp/templates/mails/clean_merge.txt +++ b/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%} diff --git a/webapp/templates/mails/comment-email.txt b/webapp/templates/mails/comment-email.txt new file mode 100644 index 0000000000..d515ac6c6f --- /dev/null +++ b/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%} diff --git a/webapp/templates/mails/comment.txt b/webapp/templates/mails/comment.txt index 32f251afd4..cc4a5a8869 100644 --- a/webapp/templates/mails/comment.txt +++ b/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%} - diff --git a/webapp/templates/mails/missing_dependency.txt b/webapp/templates/mails/missing_dependency.txt index 97f69e431a..256db31cfa 100644 --- a/webapp/templates/mails/missing_dependency.txt +++ b/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%} diff --git a/webapp/templates/mails/path_conflict.txt b/webapp/templates/mails/path_conflict.txt index 2ba59b9153..c34816bdd3 100644 --- a/webapp/templates/mails/path_conflict.txt +++ b/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