From a8fef88e754439c4b900167f8bc8296deeeb551e Mon Sep 17 00:00:00 2001 From: Adam Williamson Date: Apr 09 2018 15:10:38 +0000 Subject: Fix new commit notification mails with non-ASCII (#1814) This is an alternative to #3140. As @puiterwijk figured out, when a new commit notification mail would contain non-ASCII text - i.e. when the commit author's full name or the commit topic contain non-ASCII text - sending the mail would fail. In 3.x at least, this resulted in the service that sends the mails silently getting stuck; we don't know how the service behaves when this happens in current git (it was changed from trollius to celery), but the underlying bug is still there. It fails because of unicode conversion issues in the new commit notification function. `send_email` expects the body text to be provided as a `unicode` instance, but `notify_new_commits` works with `str` instances throughout (including the commit author and commit topic values; these ultimately come in as UTF-8 encoded strings, from the `pagure.git.read_output` function). Ultimately it produces a UTF-8 encoded `str` which `send_email` then tries to encode again, and that crashes. This approach fixes the problem in a way that's as consistent as possible with the other functions in the file - the body text is produced by `%s`-style string formatting of a unicode, and we ensure the things that get put into it are also unicode type, for the ones which can contain non-ASCII text. I also added some comments about this area, noted the expected type for `text` in the `send_email` docstring, and added a test. Fixes https://pagure.io/pagure/issue/1814 Signed-off-by: Adam Williamson --- diff --git a/pagure/lib/notify.py b/pagure/lib/notify.py index 8aa5044..e9ad0b0 100644 --- a/pagure/lib/notify.py +++ b/pagure/lib/notify.py @@ -265,6 +265,7 @@ def send_email(text, subject, to_mail, ''' Send an email with the specified information. :arg text: the content of the email to send + :type text: unicode :arg subject: the subject of the email :arg to_mail: a string representing a list of recipient separated by a comma @@ -790,37 +791,38 @@ def notify_new_commits(abspath, project, branch, commits): ''' Notify the people following a project's commits that new commits have been added. ''' + # string note: abspath, project and branch can only contain ASCII + # by policy (pagure and/or gitolite) commits_info = [] for commit in commits: commits_info.append({ 'commit': commit, - 'author': pagure.lib.git.get_author(commit, abspath), - 'subject': pagure.lib.git.get_commit_subject(commit, abspath) + # we want these to be unicodes (read_output gives us str) + 'author': pagure.lib.git.get_author(commit, abspath).decode('utf-8'), + 'subject': pagure.lib.git.get_commit_subject(commit, abspath).decode('utf-8') }) - commits_string = '\n'.join('{0} {1} {2}'.format( + # make sure this is unicode + commits_string = u'\n'.join(u'{0} {1} {2}'.format( commit_info['commit'], commit_info['author'], commit_info['subject']) for commit_info in commits_info) commit_url = _build_url( pagure_config['APP_URL'], _fullname_to_url(project.fullname), 'commits', branch) - email_body = ''' -The following commits were pushed to the repo "{repo}" on branch -"{branch}", which you are following: -{commits} + email_body = u''' +The following commits were pushed to the repo %s on branch +%s, which you are following: +%s To view more about the commits, visit: -{commit_url} -''' - email_body = email_body.format( - repo=project.fullname, - branch=branch, - commits=commits_string, - commit_url=commit_url - ) +%s +''' % (project.fullname, + branch, + commits_string, + commit_url) mail_to = _get_emails_for_commit_notification(project) send_email( diff --git a/tests/test_pagure_lib_notify_email.py b/tests/test_pagure_lib_notify_email.py index 7f3500f..00ab204 100644 --- a/tests/test_pagure_lib_notify_email.py +++ b/tests/test_pagure_lib_notify_email.py @@ -117,6 +117,7 @@ https://pagure.org/test/issue/1 # Mail text should be as expected. self.assertEqual(args[0], exptext) + self.assertTrue(isinstance(args[0], unicode)) # Mail subject should be as expected. self.assertEqual(args[1], u'Issue #1: issue') @@ -153,6 +154,7 @@ https://pagure.org/somenamespace/test3/issue/1 # Mail text should be as expected. self.assertEqual(args[0], exptext) + self.assertTrue(isinstance(args[0], unicode)) # Mail subject should be as expected. self.assertEqual(args[1], u'Issue #1: namespaced project issue') @@ -184,6 +186,7 @@ https://pagure.org/fork/foo/test/issue/1 # Mail text should be as expected. self.assertEqual(args[0], exptext) + self.assertTrue(isinstance(args[0], unicode)) # Mail subject should be as expected. self.assertEqual(args[1], u'Issue #1: forked project issue') @@ -201,6 +204,43 @@ https://pagure.org/fork/foo/test/issue/1 # Mail should be from user1 (who submitted the issue). self.assertEqual(kwargs['user_from'], self.user2.fullname) + @mock.patch('pagure.lib.notify.send_email') + # for non-ASCII testing, we mock these return values + @mock.patch('pagure.lib.git.get_author', return_value="Cecil Cõmmîttër") + @mock.patch('pagure.lib.git.get_commit_subject', return_value="We love Motörhead") + def test_notify_new_commits(self, _, __, fakemail): # pylint: disable=invalid-name + """Test for notification on new commits, especially when + non-ASCII text is involved. + """ + exptext = u""" +The following commits were pushed to the repo test on branch +master, which you are following: +abcdefg Cecil Cõmmîttër We love Motörhead + + + +To view more about the commits, visit: +https://pagure.org/test/commits/master +""" + # first arg (abspath) doesn't matter and we can use a commit + # ID that doesn't actually exist, as we are mocking + # the get_author and get_commit_subject calls anyway + pagure.lib.notify.notify_new_commits('/', self.project1, 'master', ['abcdefg']) + (_, args, kwargs) = fakemail.mock_calls[0] + + # Mail text should be as expected. + self.assertEqual(args[0], exptext) + self.assertTrue(isinstance(args[0], unicode)) + + # Mail subject should be as expected. + self.assertEqual(args[1], u'New Commits To "test" (master)') + + # Mail doesn't actually get sent to anyone by default + self.assertEqual(args[2], '') + + # Project name should be...project (full) name. + self.assertEqual(kwargs['project_name'], self.project1.fullname) + # Add more tests to verify that correct mails are sent to correct people here if __name__ == '__main__':