From 7c5e53fe9a67623ccd627817935bf99722897dc1 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Dec 06 2018 20:12:25 +0000 Subject: Match the entire URL so we can act on the right project Before this we were just matching the project name, so we missed namespaces, fork information and all. This with the fact that if even when we matched the project named we did not update the project object on which we were working, so we ended up working on the wrong ticket/PR. This can be seen in the ticket reported upstream: https://pagure.io/pagure/issue/4052 Fixes https://pagure.io/pagure/issue/4052 --- diff --git a/pagure/lib/link.py b/pagure/lib/link.py index e86df26..dc89116 100644 --- a/pagure/lib/link.py +++ b/pagure/lib/link.py @@ -15,37 +15,38 @@ from __future__ import unicode_literals import re import pagure.lib.query import pagure.exceptions +import pagure.utils +from pagure.config import config as pagure_config FIXES = [ - re.compile(r"(?:.*\s+)?fixe?[sd]?:?\s*?#(\d+)", re.I), - re.compile( - r"(?:.*\s+)?fixe?[sd]?:?\s*?https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)" - r"/(?:issue|pull-request)/(\d+)", - re.I, - ), - re.compile(r"(?:.*\s+)?merge?[sd]?:?\s*?#(\d+)", re.I), - re.compile( - r"(?:.*\s+)?merge?[sd]?:?\s*?https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)" - r"/(?:issue|pull-request)/(\d+)", - re.I, - ), - re.compile(r"(?:.*\s+)?close?[sd]?:?\s*?#(\d+)", re.I), + re.compile(r"(?:.*\s+)?{0}?[sd]?:?\s*?#(\d+)".format(kw), re.I) + for kw in ["fixe", "merge", "close"] +] +FIXES += [ re.compile( - r"(?:.*\s+)?close?[sd]?:?\s*?https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)" - r"/(?:issue|pull-request)/(\d+)", + r"(?:.*\s+)?{0}?[sd]?:?\s*?{1}" + r"(/.*?/(?:issue|pull-request)/\d+)".format( + kw, pagure_config["APP_URL"].rstrip("/") + ), re.I, - ), + ) + for kw in ["fixe", "merge", "close"] ] + RELATES = [ - re.compile(r"(?:.*\s+)?relate[sd]?:?\s*?(?:to)?\s*?#(\d+)", re.I), - re.compile(r"(?:.*\s+)?relate[sd]?:?\s?#(\d+)", re.I), + re.compile(r"(?:.*\s+)?{0}?[sd]?:?\s*?(?:to)?\s*?#(\d+)".format(kw), re.I) + for kw in ["relate"] +] +RELATES += [ re.compile( - r"(?:.*\s+)?relate[sd]?:?\s*?(?:to)?\s*?" - r"https?://.*/([a-zA-z0-9_][a-zA-Z0-9-_]*)/issue/(\d+)", + r"(?:.*\s+)?{0}?[sd]?:?\s*?(?:to)?\s*?{1}(/.*?/issue/\d+)".format( + kw, pagure_config["APP_URL"].rstrip("/") + ), re.I, - ), + ) + for kw in ["relate"] ] @@ -87,12 +88,22 @@ def get_relation( for motif in regex: relid = None project = None - if motif.match(text): - if len(motif.match(text).groups()) >= 2: - relid = motif.match(text).group(2) - project = motif.match(text).group(1) - else: - relid = motif.match(text).group(1) + got_match = motif.match(text) + if got_match: + relid = got_match.group(1) + if not relid.isdigit(): + ( + username, + namespace, + reponame, + objtype, + relid, + ) = pagure.utils.parse_path(relid) + repo = pagure.lib.query.get_authorized_project( + session, reponame, user=username, namespace=namespace + ) + if not repo: + continue if relid: relation = pagure.lib.query.search_issues( diff --git a/tests/test_pagure_hooks_pagure_hook.py b/tests/test_pagure_hooks_pagure_hook.py new file mode 100644 index 0000000..aa768ef --- /dev/null +++ b/tests/test_pagure_hooks_pagure_hook.py @@ -0,0 +1,174 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2018 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +from __future__ import unicode_literals + +import unittest +import sys +import os + +import mock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure.hooks.pagure_hook +import tests + + +class PagureHooksPagureHooktests(tests.SimplePagureTest): + """ Tests for pagure.hooks.pagure_hook """ + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureHooksPagureHooktests, self).setUp() + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + + # Add one issue to each projects otherwise we won't be able to find + project = pagure.lib.query.get_authorized_project(self.session, 'test') + msg = pagure.lib.query.new_issue( + session=self.session, + repo=project, + title='Test issue', + content='We should work on this', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + project = pagure.lib.query.get_authorized_project( + self.session, 'test2') + msg = pagure.lib.query.new_issue( + session=self.session, + repo=project, + title='Test issue on test2', + content='We should work on this, really', + user='foo', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue on test2') + + # Create a fork of test for foo with its own ticket + item = pagure.lib.model.Project( + user_id=2, # foo + name='test', + is_fork=True, + parent_id=1, + description='test project #1', + hook_token='aaabbbccc_foo', + ) + item.close_status = [ + 'Invalid', 'Insufficient data', 'Fixed', 'Duplicate'] + self.session.add(item) + self.session.commit() + project = pagure.lib.query.get_authorized_project( + self.session, 'test', user="foo") + msg = pagure.lib.query.new_issue( + session=self.session, + repo=project, + title='Test issue on fork/foo/test', + content='We should work on this, really', + user='foo', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue on fork/foo/test') + + self.folder = os.path.join(self.path, 'repos', 'test.git') + + # Add a README to the git repo - First commit + tests.add_readme_git_repo(self.folder) + + @mock.patch("pagure.hooks.pagure_hook.fixes_relation") + def test_generate_revision_change_log_short_url(self, fixes_relation): + """ Test generate_revision_change_log when the comment contains + a short link to the same project. + """ + + # Add a commit with an url in the commit message + tests.add_content_to_git( + self.folder, branch='master', filename='sources', content='foo', + message='Test commit message\n\nFixes #1' + ) + + project = pagure.lib.query.get_authorized_project(self.session, 'test') + + pagure.hooks.pagure_hook.generate_revision_change_log( + session=self.session, + project=project, + username=None, + repodir=self.folder, + new_commits_list=['HEAD'] + ) + fixes_relation.assert_called_once_with( + mock.ANY, None, mock.ANY, project.issues[0], + 'http://localhost.localdomain/') + + @mock.patch("pagure.hooks.pagure_hook.fixes_relation") + def test_generate_revision_change_log_full_url(self, fixes_relation): + """ Test generate_revision_change_log when the comment contains + a full link to another project. + """ + + # Add a commit with an url in the commit message + tests.add_content_to_git( + self.folder, branch='master', filename='sources', content='foo', + message='Test commit message\n\n' + 'Fixes http://localhost.localdomain/test2/issue/1' + ) + + project = pagure.lib.query.get_authorized_project( + self.session, 'test') + project2 = pagure.lib.query.get_authorized_project( + self.session, 'test2') + + pagure.hooks.pagure_hook.generate_revision_change_log( + session=self.session, + project=project, + username=None, + repodir=self.folder, + new_commits_list=['HEAD'] + ) + fixes_relation.assert_called_once_with( + mock.ANY, None, mock.ANY, project2.issues[0], + 'http://localhost.localdomain/') + + @mock.patch("pagure.hooks.pagure_hook.fixes_relation") + def test_generate_revision_change_log_full_url_fork(self, fixes_relation): + """ Test generate_revision_change_log when the comment contains + a full link to a fork. + """ + + # Add a commit with an url in the commit message + tests.add_content_to_git( + self.folder, branch='master', filename='sources', content='foo', + message='Test commit message\n\n' + 'Fixes http://localhost.localdomain/fork/foo/test/issue/1' + ) + + project = pagure.lib.query.get_authorized_project( + self.session, 'test') + project_fork = pagure.lib.query.get_authorized_project( + self.session, 'test', user="foo") + + pagure.hooks.pagure_hook.generate_revision_change_log( + session=self.session, + project=project, + username=None, + repodir=self.folder, + new_commits_list=['HEAD'] + ) + fixes_relation.assert_called_once_with( + mock.ANY, None, mock.ANY, project_fork.issues[0], + 'http://localhost.localdomain/') + + +if __name__ == '__main__': + unittest.main(verbosity=2) diff --git a/tests/test_pagure_lib_link.py b/tests/test_pagure_lib_link.py index d5e7b35..cb7df6e 100644 --- a/tests/test_pagure_lib_link.py +++ b/tests/test_pagure_lib_link.py @@ -33,9 +33,9 @@ COMMENTS = [ 'Did you see #1?', 'This is a duplicate of #2', 'This is a fixes #3', - 'Might be worth looking at https://fedorahosted.org/pagure/tests2/issue/4', + 'Might be worth looking at http://localhost.localdomain/pagure/tests2/issue/4', 'This relates to #5', - 'Could this be related to https://fedorahosted.org/pagure/tests2/issue/6', + 'Could this be related to http://localhost.localdomain/test/issue/6', ] @@ -186,21 +186,23 @@ class PagureLibLinktests(tests.Modeltests): self.assertEqual( str(link), '[Issue(3, project:test, user:pingou, title:issue 3)]') + self.assertEqual(len(link), 1) + self.assertEqual(link[0].project.fullname, 'test') else: self.assertEqual(link, []) def test_relates_regex(self): ''' Test the relates regex present in pagure.lib.link. ''' - text = 'relates to http://localhost/fork/pingou/test/issue/1' + text = 'relates to http://localhost.localdomain/fork/pingou/test/issue/1' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) - text = 'relates http://209.132.184.222/fork/pingou/test/issue/1' + text = 'relates http://localhost.localdomain/fork/pingou/test/issue/1' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) @@ -213,16 +215,16 @@ class PagureLibLinktests(tests.Modeltests): self.assertEqual(regex.match(text), None) text = 'Could this be related to '\ - ' https://fedorahosted.org/pagure/tests2/issue/6' + ' http://localhost.localdomain/pagure/tests2/issue/6' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) - text = 'relates https://localhost/SSSD/ding-libs/issue/31' + text = 'relates http://localhost.localdomain/SSSD/ding-libs/issue/31' for index, regex in enumerate(pagure.lib.link.RELATES): - if index == 2: + if index == 1: self.assertNotEqual(regex.match(text), None) else: self.assertEqual(regex.match(text), None) @@ -238,34 +240,44 @@ class PagureLibLinktests(tests.Modeltests): if match: break self.assertNotEqual(match, None) - self.assertEqual(len(match.groups()), 2) + self.assertEqual(len(match.groups()), 1) self.assertEqual(match.groups(), groups) data = [ # [string, groups] ] - project_match('fixes http://localhost/fork/pingou/test/issue/1', - ('test', '1')) - project_match('fix http://209.132.184.222/fork/pingou/test/issue/1', - ('test', '1')) - project_match('Could this be fixes ' - ' https://fedorahosted.org/pagure/tests2/issue/6', - ('tests2', '6')) - project_match('merged https://pagure.io/myproject/pull-request/70', - ('myproject', '70')) - project_match('Now we merge https://pagure.io/myproject/pull-request/99', - ('myproject', '99')) - project_match('Merges http://localhost/fork/pingou/test/issue/1', - ('test', '1')) - project_match('Merges: http://localhost/fork/pingou/test/issue/12', - ('test', '12')) - project_match('Merged http://localhost/fork/pingou/test/issue/123#', - ('test', '123')) - project_match('Merge: http://localhost/fork/pingou/test/issue/1234#foo', - ('test', '1234')) - project_match('Merges: https://localhost/SSSD/ding-libs/pull-request/3188', - ('ding-libs', '3188')) + project_match( + 'fixes ' + 'http://localhost.localdomain/fork/pingou/test/issue/1', + ('/fork/pingou/test/issue/1',)) + project_match( + 'Could this be fixes ' + ' http://localhost.localdomain/pagure/tests2/issue/6', + ('/pagure/tests2/issue/6',)) + project_match( + 'merged http://localhost.localdomain/myproject/pull-request/70', + ('/myproject/pull-request/70',)) + project_match( + 'Now we merge http://localhost.localdomain/myproject/pull-request/99', + ('/myproject/pull-request/99',)) + project_match( + 'Merges http://localhost.localdomain/fork/pingou/test/issue/1', + ('/fork/pingou/test/issue/1',)) + project_match( + 'Merges: http://localhost.localdomain/fork/pingou/test/issue/12', + ('/fork/pingou/test/issue/12',)) + project_match( + 'Merged http://localhost.localdomain/fork/pingou/test/issue/123#', + ('/fork/pingou/test/issue/123',)) + project_match('Merge: http://localhost.localdomain/fork/pingou/test/issue/1234#foo', + ('/fork/pingou/test/issue/1234',)) + project_match( + 'Merges: http://localhost.localdomain/SSSD/ding-libs/pull-request/3188', + ('/SSSD/ding-libs/pull-request/3188',)) + project_match( + 'Fixes: http://localhost.localdomain/fedpkg/issue/220', + ('/fedpkg/issue/220',)) # issue matches def issue_match(text, issue):