#1538 Use fullname not name when constructing URLs for emails
Merged 7 years ago by pingou. Opened 7 years ago by adamwill.
adamwill/pagure mail-url-fullname  into  master

file modified
+17 -8
@@ -177,6 +177,15 @@ 

      return '/'.join(items)

  

  

+ def _fullname_to_url(fullname):

+     ''' For forked projects, fullname is 'forks/user/...' but URL is

+     'fork/user/...'. This is why we can't have nice things.

+     '''

+     if fullname.startswith('forks/'):

+         fullname = fullname.replace('forks', 'fork', 1)

+     return fullname

+ 

+ 

  def send_email(text, subject, to_mail,

                 mail_id=None, in_reply_to=None,

                 project_name=None, user_from=None):  # pragma: no cover
@@ -292,7 +301,7 @@ 

         REPLY_MSG,

         _build_url(

             pagure.APP.config['APP_URL'],

-            comment.issue.project.name,

+            _fullname_to_url(comment.issue.project.fullname),

             'issue',

             comment.issue.id))

      mail_to = _get_emails_for_obj(comment.issue)
@@ -331,7 +340,7 @@ 

         REPLY_MSG,

         _build_url(

             pagure.APP.config['APP_URL'],

-            issue.project.name,

+            _fullname_to_url(issue.project.fullname),

             'issue',

             issue.id))

      mail_to = _get_emails_for_obj(issue)
@@ -364,7 +373,7 @@ 

         user.username,

         _build_url(

             pagure.APP.config['APP_URL'],

-            issue.project.name,

+            _fullname_to_url(issue.project.fullname),

             'issue',

             issue.id))

      mail_to = _get_emails_for_obj(issue)
@@ -401,7 +410,7 @@ 

         user.username,

         _build_url(

             pagure.APP.config['APP_URL'],

-            request.project.name,

+            _fullname_to_url(request.project.fullname),

             'pull-request',

             request.id))

      mail_to = _get_emails_for_obj(request)
@@ -440,7 +449,7 @@ 

         REPLY_MSG,

         _build_url(

             pagure.APP.config['APP_URL'],

-            request.project.name,

+            _fullname_to_url(request.project.fullname),

             'pull-request',

             request.id))

      mail_to = _get_emails_for_obj(request)
@@ -474,7 +483,7 @@ 

         request.title,

         _build_url(

             pagure.APP.config['APP_URL'],

-            request.project.name,

+            _fullname_to_url(request.project.fullname),

             'pull-request',

             request.id))

      mail_to = _get_emails_for_obj(request)
@@ -510,7 +519,7 @@ 

         request.title,

         _build_url(

             pagure.APP.config['APP_URL'],

-            request.project.name,

+            _fullname_to_url(request.project.fullname),

             'pull-request',

             request.id))

      mail_to = _get_emails_for_obj(request)
@@ -545,7 +554,7 @@ 

         REPLY_MSG,

         _build_url(

             pagure.APP.config['APP_URL'],

-            comment.pull_request.project.name,

+            _fullname_to_url(comment.pull_request.project.fullname),

             'pull-request',

             comment.pull_request.id))

      mail_to = _get_emails_for_obj(comment.pull_request)

@@ -0,0 +1,208 @@ 

+ # -*- coding: utf-8 -*-

+ 

+ """

+  (c) 2016 - Copyright Red Hat Inc

+ 

+  Authors:

+    Adam Williamson <awilliam@redhat.com>

+ 

+ """

+ 

+ import unittest

+ import sys

+ import os

+ 

+ import mock

+ 

+ sys.path.insert(0, os.path.join(os.path.dirname(

+     os.path.abspath(__file__)), '..'))

+ 

+ import pagure.lib           # pylint: disable=wrong-import-position

+ import pagure.lib.model     # pylint: disable=wrong-import-position

+ import pagure.lib.notify    # pylint: disable=wrong-import-position

+ import tests                # pylint: disable=wrong-import-position

+ 

+ 

+ class PagureLibNotifyEmailtests(tests.Modeltests):

+     """ Some tests for the various email construction functions. In

+     their own class so they can have some shared fixtures.

+     """

+ 

+     def setUp(self):

+         """ Override setUp to add more fixtures used for many tests. """

+         super(PagureLibNotifyEmailtests, self).setUp()

+         pagure.SESSION = self.session

+         tests.create_projects(self.session)

+ 

+         # we don't want to send any mails while setting up

+         patcher = mock.patch('pagure.lib.notify.send_email')

+         patcher.start()

+ 

+         self.user1 = pagure.lib.get_user(self.session, 'pingou')

+         self.user2 = pagure.lib.get_user(self.session, 'foo')

+         self.project1 = pagure.lib.get_project(self.session, 'test')

+         self.project2 = pagure.lib.get_project(self.session, 'test2')

+         self.project3 = pagure.lib.get_project(self.session, 'test3', namespace='somenamespace')

+ 

+         # Create a forked repo, should be project #4

+         # Not using fork_project as it tries to do a git clone

+         item = pagure.lib.model.Project(

+             user_id=2,  # foo

+             name='test',

+             description='test project #1',

+             is_fork=True,

+             parent_id=1,

+             hook_token='aaabbbyyy',

+         )

+         self.session.add(item)

+         self.session.commit()

+         self.forkedproject = pagure.lib.get_project(self.session, 'test', user='foo')

+ 

+         # Report an issue on project #1

+         self.issue1 = pagure.lib.new_issue(

+             session=self.session,

+             repo=self.project1,

+             title='issue',

+             content='a bug report',

+             user='pingou',

+             ticketfolder=None,

+         )

+ 

+         # Add a comment on the issue

+         pagure.lib.add_issue_comment(

+             self.session,

+             self.issue1,

+             comment='Test comment',

+             user='pingou',

+             ticketfolder=None,

+         )

+         self.comment1 = pagure.lib.get_issue_comment(self.session, self.issue1.uid, 1)

+ 

+         # Report an issue on project #3 (namespaced)

+         self.issue2 = pagure.lib.new_issue(

+             session=self.session,

+             repo=self.project3,

+             title='namespaced project issue',

+             content='a bug report on a namespaced project',

+             user='pingou',

+             ticketfolder=None,

+         )

+ 

+         # report an issue on foo's fork of project #1

+         self.issue3 = pagure.lib.new_issue(

+             session=self.session,

+             repo=self.forkedproject,

+             title='forked project issue',

+             content='a bug report on a forked project',

+             user='pingou',

+             ticketfolder=None,

+         )

+ 

+         patcher.stop()

+ 

+     @mock.patch('pagure.lib.notify.send_email')

+     def test_notify_new_comment(self, fakemail):

+         """Simple test for notification about new comment."""

+         exptext = u"""

+ pingou added a new comment to an issue you are following:

+ ``

+ Test comment

+ ``

+ 

+ To reply, visit the link below

+ https://pagure.org/test/issue/1

+ """

+         pagure.lib.notify.notify_new_comment(self.comment1)

+         (_, args, kwargs) = fakemail.mock_calls[0]

+ 

+         # Mail text should be as expected.

+         self.assertEqual(args[0], exptext)

+ 

+         # Mail subject should be as expected.

+         self.assertEqual(args[1], u'Issue #1 `issue`')

+ 

+         # Mail should be sent to user #1.

+         self.assertEqual(args[2], self.user1.default_email)

+ 

+         # Mail ID should be comment #1's mail ID...

+         self.assertEqual(kwargs['mail_id'], self.comment1.mail_id)

+ 

+         # In reply to issue #1's mail ID.

+         self.assertEqual(kwargs['in_reply_to'], self.issue1.mail_id)

+ 

+         # Project name should be...project (full) name.

+         self.assertEqual(kwargs['project_name'], self.project1.fullname)

+ 

+         # Mail should be from user1 (who wrote the comment).

+         self.assertEqual(kwargs['user_from'], self.user1.fullname)

+ 

+     @mock.patch('pagure.lib.notify.send_email')

+     def test_notify_new_issue_namespaced(self, fakemail):   # pylint: disable=invalid-name

+         """Test for notifying of a new issue, namespaced project."""

+         exptext = u"""

+ pingou reported a new issue against the project: `test3` that you are following:

+ ``

+ a bug report on a namespaced project

+ ``

+ 

+ To reply, visit the link below

+ https://pagure.org/somenamespace/test3/issue/1

+ """

+         pagure.lib.notify.notify_new_issue(self.issue2)

+         (_, args, kwargs) = fakemail.mock_calls[0]

+ 

+         # Mail text should be as expected.

+         self.assertEqual(args[0], exptext)

+ 

+         # Mail subject should be as expected.

+         self.assertEqual(args[1], u'Issue #1 `namespaced project issue`')

+ 

+         # Mail should be sent to user #1.

+         self.assertEqual(args[2], self.user1.default_email)

+ 

+         # Mail ID should be issue's mail ID.

+         self.assertEqual(kwargs['mail_id'], self.issue2.mail_id)

+ 

+         # Project name should be...project (full) name.

+         self.assertEqual(kwargs['project_name'], self.project3.fullname)

+ 

+         # Mail should be from user1 (who submitted the issue).

+         self.assertEqual(kwargs['user_from'], self.user1.fullname)

+ 

+     @mock.patch('pagure.lib.notify.send_email')

+     def test_notify_assigned_issue_forked(self, fakemail):  # pylint: disable=invalid-name

+         """Test for notifying re-assignment of issue on forked project.

+         'foo' reassigns issue on his fork of 'test' to 'pingou'.

+         """

+         exptext = u"""

+ The issue: `forked project issue` of project: `test` has been assigned to `pingou` by foo.

+ 

+ https://pagure.org/fork/foo/test/issue/1

+ """

+         pagure.lib.notify.notify_assigned_issue(self.issue3, self.user1, self.user2)

+         (_, args, kwargs) = fakemail.mock_calls[0]

+ 

+         # Mail text should be as expected.

+         self.assertEqual(args[0], exptext)

+ 

+         # Mail subject should be as expected.

+         self.assertEqual(args[1], u'Issue #1 `forked project issue`')

+ 

+         # Mail should be sent to user #1.

+         # NOTE: Not sent to user #2...

+         self.assertEqual(args[2], self.user1.default_email)

+ 

+         # Mail ID should contain issue's mail ID and '/assigned/'

+         self.assertIn("{0}/assigned/".format(self.issue3.mail_id), kwargs['mail_id'])

+ 

+         # Project name should be...project (full) name.

+         self.assertEqual(kwargs['project_name'], self.forkedproject.fullname)

+ 

+         # Mail should be from user1 (who submitted the issue).

+         self.assertEqual(kwargs['user_from'], self.user2.fullname)

+ 

+ # Add more tests to verify that correct mails are sent to correct people here

+ 

+ if __name__ == '__main__':

+     SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureLibNotifyEmailtests)

+     unittest.TextTestRunner(verbosity=2).run(SUITE)

This corrects the URLs for namespaced projects. It should also
work for forks, though I think we don't expect to send email
for forks.

Note: re-constructing these URLs here feels quite ugly. Should
the models not all have url as one of their attributes?

Signed-off-by: Adam Williamson awilliam@redhat.com

So I did a lot of boilerplate work for the tests here with the intent of adding several to test each function and variations on the URL (and anything else that looks interesting to test, in the email contents).

But before I do that I'd like someone's blessing for the general approach, so I don't waste any more time on it if you don't like it. In a few ways the test comes a bit close to just reproducing the tested function, it's a tough call where to just use the same attribute the tested function will pass and where to include its expected value (e.g. user names and email addresses and such). If anyone has thoughts...

If we wanted to focus the tests a bit more strictly on just checking the URLs are correct, an alternative approach (the one I started with) is to mock send_email with a function that just stores the args it's called with into the test class, then we can trivially check that the expected URL is in the email text and not bother about the rest of it.

rebased

7 years ago

okay, so now I understand the design I see why we can't trivially include the URLs for the models in that code - the db code has no access to the app so we can't simply use the app routes. We'd have to effectively duplicate them into the db code, which is not exactly a win.

I guess whether it's worth doing anything about changing the URL construction depends on if there are any other bits of Pagure which are doing something similar and trying to reconstruct URLs for DB models from their properties; if so we could throw some convenience functions into pagure.lib or something.

Maybe the expected text could be entirely hard-coded no?

One comment for the test but otherwise looks good to me as is.

I agree, it's fine to hardcode tests, especially when the test is deterministic as this appears to be.

LGTM. I think whether you choose to hardcode the tests or not is optional, but I defer to @pingou on that. If it were me, I'd just hardcode it to keep it simpler and more readable.

the only issue with hardcoding it, really, is if the fixtures change. but then it shouldn't be too hard to adapt the test. so I guess I'll do that. but I'm working on the other PR atm.

rebased

7 years ago

So, here's another version. All changes are to the tests.

The tests are now somewhat more hardcoded, and I tweaked the way the fixtures are set up. I also split out the assert_called_with into individual checks for specific args, as it's much easier to see what's wrong when the test fails this way. I added enough tests to cover the possible URL variants (so far). We could add more tests to check stuff like the correct mail recipients in various cases, but I've already spent a lot of time on this and can't really justify spending more on that.

I also did some pylint cleanup.

pylint points up one interesting issue I was reluctant to 'fix'. tests/__init__.py does this:

self.session = pagure.lib.model.create_tables(
        DB_PATH, acls=pagure.APP.config.get('ACLS', {}))

That does not actually return a session instance, but a factory function that SQLAlchemy calls a 'registry'. This is discussed here and documented upstream.

Through some wizardry doing self.session.add(), self.session.commit() etc. works, but it confuses pylint, which complains that instances of scoped_session don't have that member, because technically they don't.

We could 'solve' this by calling the factory function when assigning self.session; then we actually get a session instance which really has the members in question, so pylint is happy. However, I'm not sure of the precise implications of doing this, so I'm not going to do it in this PR. It's worth someone with a better grasp of the SQL and threading stuff looking at it, though. Note that upstream's example does:

Session = scoped_session(session_factory)
some_session = Session()

our code is kinda missing that second step.

Obviously, if we were gonna change this somehow, we'd probably want to change the similar bits in the actual Pagure code, not just the tests...

rebased

7 years ago

I think we could also stand to add a forked project to the 'fixtures' you get from tests.create_projects() and adjust all the tests that use a forked project to use that (or as many of them as feasible), but again, I can't really spare the time to do that myself, sorry...

I'm going to merge this PR as is, many thanks for working on this and spending that much time on it!

Your suggestions around the session and the tests are probably worth putting in their own ticket so we're just they do not get lost. Would you like to do it? Otherwise I will :)

Thanks again!

Pull-Request has been merged by pingou

7 years ago

After poking around at the scoped_session stuff some more I think I think it's not really wrong, so instead, I came up with a PR to shut pylint up in a better way than we were previously doing it: #1565