From 78ba11ddde3bcbfa42b7bef165d9e865950e033b Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 30 2018 08:09:38 +0000 Subject: [PATCH 1/7] Move _get_parent_repo_path to pagure.utils as get_parent_repo_path Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index 70e5e4b..a59ebba 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -33,24 +33,13 @@ import pagure.lib.tasks import pagure.forms from pagure.config import config as pagure_config from pagure.ui import UI_NS -from pagure.utils import login_required, __get_file_in_tree +from pagure.utils import ( + login_required, __get_file_in_tree, get_parent_repo_path) _log = logging.getLogger(__name__) -def _get_parent_repo_path(repo): - """ Return the path of the parent git repository corresponding to the - provided Repository object from the DB. - """ - if repo.parent: - parentpath = os.path.join( - pagure_config['GIT_FOLDER'], repo.parent.path) - else: - parentpath = os.path.join(pagure_config['GIT_FOLDER'], repo.path) - - return parentpath - def _get_parent_request_repo_path(repo): """ Return the path of the parent git repository corresponding to the @@ -326,7 +315,7 @@ def request_pull_to_diff_or_patch( else: repo_from = request.project_from repopath = pagure.utils.get_repo_path(repo_from) - parentpath = _get_parent_repo_path(repo_from) + parentpath = get_parent_repo_path(repo_from) repo_obj = pygit2.Repository(repopath) orig_repo = pygit2.Repository(parentpath) @@ -1105,7 +1094,7 @@ def new_request_pull( repo_obj = flask.g.repo_obj if not project_to: - parentpath = _get_parent_repo_path(repo) + parentpath = get_parent_repo_path(repo) orig_repo = pygit2.Repository(parentpath) else: p_namespace = None diff --git a/pagure/utils.py b/pagure/utils.py index 2b7e673..057ddeb 100644 --- a/pagure/utils.py +++ b/pagure/utils.py @@ -376,3 +376,16 @@ def split_project_fullname(project_name): _, user, namespace, project_name = project_items return (user, namespace, project_name) + + +def get_parent_repo_path(repo): + """ Return the path of the parent git repository corresponding to the + provided Repository object from the DB. + """ + if repo.parent: + parentpath = os.path.join( + pagure_config['GIT_FOLDER'], repo.parent.path) + else: + parentpath = os.path.join(pagure_config['GIT_FOLDER'], repo.path) + + return parentpath From 11d0199fbadcca873936285e277b0d8bfb9fce11 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 30 2018 08:09:38 +0000 Subject: [PATCH 2/7] Add the possibility to link issues to pull-requests This commit add this linking at the database level as well as in the UI. Fixes https://pagure.io/pagure/issue/2705 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/hooks/files/default_hook.py b/pagure/hooks/files/default_hook.py index 4694560..00e13fd 100644 --- a/pagure/hooks/files/default_hook.py +++ b/pagure/hooks/files/default_hook.py @@ -149,6 +149,10 @@ def inform_pull_request_urls( # Link to existing PRs if there are any seen = len(prs) != 0 for pr in prs: + # Link tickets with pull-requests if the commit mentions it + pagure.lib.tasks.link_pr_to_ticket.delay(pr.uid) + + # Inform the user about the PR print('View pull-request for %s' % refname) print(' %s/%s/pull-request/%s' % ( _config['APP_URL'].rstrip('/'), diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 0d9f1e1..f020ccb 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -1773,6 +1773,8 @@ def new_pull_request(session, branch_from, pagure.lib.git.update_git( request, repo=request.project, repofolder=requestfolder) + pagure.lib.tasks.link_pr_to_ticket.delay(request.uid) + log_action(session, 'created', request, user_obj) if notify: @@ -5055,3 +5057,28 @@ def get_project_family(session, project): ) return [parent] + query.all() + + +def link_pr_issue(session, issue, request): + ''' Associate the specified issue with the specified pull-requets. + + :arg session: The SQLAlchemy session to use + :type session: sqlalchemy.orm.session.Session + :arg issue: The issue mentionned in the commits of the pull-requests to + be associated with + :type issue: pagure.lib.model.Issue + :arg request: A pull-request to associate the specified issue with + :type request: pagure.lib.model.PullRequest + + ''' + + + associated_issue = [iss.uid for iss in request.related_issues] + if issue.uid not in associated_issue: + obj = model.PrToIssue( + pull_request_uid=request.uid, + issue_uid=issue.uid + ) + session.add(obj) + session.flush() + session.commit() diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 533e450..b9c1e35 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -1312,6 +1312,28 @@ class IssueToIssue(BASE): primary_key=True) +class PrToIssue(BASE): + """ Stores the associations between issues and pull-requests. + + Table -- pr_to_issue + """ + + __tablename__ = 'pr_to_issue' + + pull_request_uid = sa.Column( + sa.String(32), + sa.ForeignKey( + 'pull_requests.uid', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True) + issue_uid = sa.Column( + sa.String(32), + sa.ForeignKey( + 'issues.uid', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True) + + class IssueComment(BASE): """ Stores the comments made on a commit/file. @@ -1765,6 +1787,14 @@ class PullRequest(BASE): viewonly=True ) + related_issues = relation( + "Issue", + secondary="pr_to_issue", + primaryjoin="pull_requests.c.uid==pr_to_issue.c.pull_request_uid", + secondaryjoin="pr_to_issue.c.issue_uid==issues.c.uid", + backref=backref("related_prs", order_by="pull_requests.c.id.desc()") + ) + def __repr__(self): return 'PullRequest(%s, project:%s, user:%s, title:%s)' % ( self.id, self.project.name, self.user.user, self.title diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 8a329d1..35e132e 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -32,9 +32,11 @@ from sqlalchemy.exc import SQLAlchemyError import pagure.lib import pagure.lib.git import pagure.lib.git_auth +import pagure.lib.link import pagure.lib.repo import pagure.utils from pagure.config import config as pagure_config +from pagure.utils import get_parent_repo_path # logging.config.dictConfig(pagure_config.get('LOGGING') or {'version': 1}) _log = logging.getLogger(__name__) @@ -850,3 +852,65 @@ def commits_history_stats(self, session, repopath): dates[arrow.get(commit.commit_time).date().isoformat()] += 1 return [(key, dates[key]) for key in sorted(dates)] + + +@conn.task(bind=True) +@pagure_task +def link_pr_to_ticket(self, session, pr_uid): + """ Link the specified pull-request against the ticket(s) mentioned in + the commits of the pull-request + + """ + _log.info( + 'LINK_PR_TO_TICKET: Linking ticket(s) to PR for: %s' % pr_uid) + + request = pagure.lib.get_request_by_uid(session, pr_uid) + if not request: + _log.info('LINK_PR_TO_TICKET: Not PR found for: %s' % pr_uid) + return + + if request.remote: + repopath = pagure.utils.get_remote_repo_path( + request.remote_git, request.branch_from) + parentpath = pagure.utils.get_repo_path(request.project) + else: + repo_from = request.project_from + repopath = pagure.utils.get_repo_path(repo_from) + parentpath = get_parent_repo_path(repo_from) + + repo_obj = pygit2.Repository(repopath) + orig_repo = pygit2.Repository(parentpath) + + diff_commits = pagure.lib.git.diff_pull_request( + session, request, repo_obj, orig_repo, + requestfolder=pagure_config['REQUESTS_FOLDER'], with_diff=False) + + _log.info( + 'LINK_PR_TO_TICKET: Found %s commits in that PR' % len(diff_commits)) + + name = request.project.name + namespace = request.project.namespace + user = request.project.user.user \ + if request.project.is_fork else None + branch = request.branch_from + + for line in pagure.lib.git.read_git_lines( + ['log', '--no-walk'] + + [c.oid.hex for c in diff_commits] + + ['--'], repopath): + + line = line.strip() + for issue in pagure.lib.link.get_relation( + session, name, user, namespace, line, 'fixes', + include_prs=False): + _log.info( + 'LINK_PR_TO_TICKET: Link ticket %s to PRs %s' % ( + issue, request)) + pagure.lib.link_pr_issue(session, issue, request) + + for issue in pagure.lib.link.get_relation( + session, name, user, namespace, line, 'relates'): + _log.info( + 'LINK_PR_TO_TICKET: Link ticket %s to PRs %s' % ( + issue, request)) + pagure.lib.link_pr_issue(session, issue, request) diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html index aee9f69..e2ab290 100644 --- a/pagure/templates/issue.html +++ b/pagure/templates/issue.html @@ -365,6 +365,32 @@ {% endif %} + {% if issue.related_prs %} +
+
+
+ +
+
    + {% for pr in issue.related_prs %} +
  • + #{{pr.id}} + {{ pr.status if pr.status != 'Open' else 'Last updated' + }} {{ pr.last_updated | humanize }} +
    • + {% endfor %} +
+
+
+
+
+ {% endif %} + {% if repo.issue_keys %}
diff --git a/tests/__init__.py b/tests/__init__.py index 520e6d3..4eec5b0 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -476,31 +476,37 @@ class FakeUser(object): # pylint: disable=too-few-public-methods return self.dic[key] -def create_projects(session): +def create_projects(session, is_fork=False, user_id=1, hook_token_suffix=''): """ Create some projects in the database. """ item = pagure.lib.model.Project( - user_id=1, # pingou + user_id=user_id, # pingou name='test', + is_fork=is_fork, + parent_id=1 if is_fork else None, description='test project #1', - hook_token='aaabbbccc', + hook_token='aaabbbccc' + hook_token_suffix, ) item.close_status = ['Invalid', 'Insufficient data', 'Fixed', 'Duplicate'] session.add(item) item = pagure.lib.model.Project( - user_id=1, # pingou + user_id=user_id, # pingou name='test2', + is_fork=is_fork, + parent_id=2 if is_fork else None, description='test project #2', - hook_token='aaabbbddd', + hook_token='aaabbbddd' + hook_token_suffix, ) item.close_status = ['Invalid', 'Insufficient data', 'Fixed', 'Duplicate'] session.add(item) item = pagure.lib.model.Project( - user_id=1, # pingou + user_id=user_id, # pingou name='test3', + is_fork=is_fork, + parent_id=3 if is_fork else None, description='namespaced test project', - hook_token='aaabbbeee', + hook_token='aaabbbeee' + hook_token_suffix, namespace='somenamespace', ) item.close_status = ['Invalid', 'Insufficient data', 'Fixed', 'Duplicate'] diff --git a/tests/test_pagure_flask_ui_issue_pr_link.py b/tests/test_pagure_flask_ui_issue_pr_link.py new file mode 100644 index 0000000..a82e9a8 --- /dev/null +++ b/tests/test_pagure_flask_ui_issue_pr_link.py @@ -0,0 +1,160 @@ +# -*- coding: utf-8 -*- + +""" + (c) 2018 - Copyright Red Hat Inc + + Authors: + Pierre-Yves Chibon + +""" + +__requires__ = ['SQLAlchemy >= 0.8'] +import pkg_resources + +import datetime +import json +import unittest +import re +import shutil +import sys +import tempfile +import time +import os + +import pygit2 +from mock import ANY, patch, MagicMock + +sys.path.insert(0, os.path.join(os.path.dirname( + os.path.abspath(__file__)), '..')) + +import pagure +import pagure.lib +import tests +from pagure.lib.repo import PagureRepo + + +class PagureFlaskPrIssueLinkTest(tests.Modeltests): + """ Tests pagure when linking PRs to tickets """ + + maxDiff = None + + def setUp(self): + """ Set up the environnment, ran before every tests. """ + super(PagureFlaskPrIssueLinkTest, self).setUp() + + tests.create_projects(self.session) + tests.create_projects( + self.session, is_fork=True, user_id=2, hook_token_suffix='bar') + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + tests.create_projects_git(os.path.join( + self.path, 'repos', 'forks', 'foo'), bare=True) + + repo = pagure.lib.get_authorized_project(self.session, 'test') + + # Create issues to play with + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title=u'tést íssüé', + content='We should work on this', + user='pingou', + ticketfolder=None + ) + self.session.commit() + self.assertEqual(msg.title, u'tést íssüé') + + msg = pagure.lib.new_issue( + session=self.session, + repo=repo, + title=u'tést íssüé #2', + content='We should still work on this', + user='foo', + ticketfolder=None + ) + self.session.commit() + self.assertEqual(msg.title, u'tést íssüé #2') + + # Add a commit to the fork + + newpath = tempfile.mkdtemp(prefix='pagure-fork-test') + repopath = os.path.join(newpath, 'test') + clone_repo = pygit2.clone_repository(os.path.join( + self.path, 'repos', 'forks', 'foo', 'test.git'), repopath) + + # Create a file in that git repo + with open(os.path.join(repopath, 'sources'), 'w') as stream: + stream.write('foo\n bar') + clone_repo.index.add('sources') + clone_repo.index.write() + + try: + com = repo.revparse_single('HEAD') + prev_commit = [com.oid.hex] + except: + prev_commit = [] + + # Commits the files added + tree = clone_repo.index.write_tree() + author = pygit2.Signature( + 'Alice Author', 'alice@authors.tld') + committer = pygit2.Signature( + 'Cecil Committer', 'cecil@committers.tld') + clone_repo.create_commit( + 'refs/heads/master', # the name of the reference to update + author, + committer, + 'Add sources file for testing\n\n Relates to #2', + # binary string representing the tree object ID + tree, + # list of binary strings representing parents of the new commit + prev_commit + ) + refname = 'refs/heads/master:refs/heads/master' + ori_remote = clone_repo.remotes[0] + PagureRepo.push(ori_remote, refname) + + # Create the corresponding PR + + repo = pagure.lib.get_authorized_project(self.session, 'test') + fork_repo = pagure.lib.get_authorized_project( + self.session, 'test', user='foo') + + request = pagure.lib.new_pull_request( + self.session, + branch_from='master', + repo_to=repo, + branch_to='master', + title='test PR', + user='foo', + requestfolder=None, + initial_comment=None, + repo_from=fork_repo, + ) + self.session.commit() + + pagure.lib.tasks.link_pr_to_ticket(request.uid) + self.assertEqual(request.id, 3) + + def test_ticket_no_link(self): + """ Test that no Related PR(s) block is showing in the issue page. + """ + output = self.app.get('/test/issue/1') + self.assertEqual(output.status_code, 200) + self.assertNotIn( + u'Related PR(s)', + output.data.decode('utf-8')) + + def test_ticket_link(self): + """ Test that no Related PR(s) block is showing in the issue page. + """ + time.sleep(1) + output = self.app.get('/test/issue/2') + print output.data.decode('utf-8') + self.assertEqual(output.status_code, 200) + self.assertIn( + u'Related PR(s)', + output.data.decode('utf-8')) + + +if __name__ == '__main__': + unittest.main(verbosity=2) From 88c073a070fbb697d4e9019c72b83010838bb987 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 30 2018 08:09:38 +0000 Subject: [PATCH 3/7] Add an alembic migration to create the pr_to_issue table Signed-off-by: Pierre-Yves Chibon --- diff --git a/alembic/versions/369deb8c8b63_add_the_pr_to_issue_table.py b/alembic/versions/369deb8c8b63_add_the_pr_to_issue_table.py new file mode 100644 index 0000000..12b920c --- /dev/null +++ b/alembic/versions/369deb8c8b63_add_the_pr_to_issue_table.py @@ -0,0 +1,42 @@ +"""Add the pr_to_issue table + +Revision ID: 369deb8c8b63 +Revises: eab41ce5f92a +Create Date: 2018-03-12 11:38:00.955252 + +""" + +# revision identifiers, used by Alembic. +revision = '369deb8c8b63' +down_revision = 'eab41ce5f92a' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ''' Create the pr_to_issue table. ''' + + op.create_table( + 'pr_to_issue', + sa.Column( + 'pull_request_uid', + sa.String(32), + sa.ForeignKey( + 'pull_requests.uid', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True), + sa.Column( + 'issue_uid', + sa.String(32), + sa.ForeignKey( + 'issues.uid', ondelete='CASCADE', onupdate='CASCADE', + ), + primary_key=True) + ) + + +def downgrade(): + ''' Drop the pr_to_issue table. ''' + + op.drop_table('pr_to_issue') From c0a26729cf20bc22f0d8833f063a9555a096134c Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 30 2018 08:09:38 +0000 Subject: [PATCH 4/7] Ensure variables are instantiated before trying to use them Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 35e132e..1d64b3a 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -295,6 +295,7 @@ def create_project(self, session, username, namespace, name, add_readme, with open(http_clone_file, 'w') as stream: pass + docrepo = None if pagure_config.get('DOCS_FOLDER'): docrepo = os.path.join( pagure_config['DOCS_FOLDER'], project.path) @@ -309,13 +310,15 @@ def create_project(self, session, username, namespace, name, add_readme, _log.debug('Create git repo at: %s', docrepo) pygit2.init_repository(docrepo, bare=True) + ticketrepo = None if pagure_config.get('TICKETS_FOLDER'): ticketrepo = os.path.join( pagure_config['TICKETS_FOLDER'], project.path) if os.path.exists(ticketrepo): if not ignore_existing_repo: shutil.rmtree(gitrepo) - shutil.rmtree(docrepo) + if docrepo: + shutil.rmtree(docrepo) session.remove() raise pagure.exceptions.RepoExistsException( 'The tickets repo "%s" already exists' % @@ -332,8 +335,10 @@ def create_project(self, session, username, namespace, name, add_readme, if os.path.exists(requestrepo): if not ignore_existing_repo: shutil.rmtree(gitrepo) - shutil.rmtree(docrepo) - shutil.rmtree(ticketrepo) + if docrepo: + shutil.rmtree(docrepo) + if ticketrepo: + shutil.rmtree(ticketrepo) session.remove() raise pagure.exceptions.RepoExistsException( 'The requests repo "%s" already exists' % From 5a0b9ae2df1cfeb14ad4d9316067bb03be2182ad Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 30 2018 08:09:38 +0000 Subject: [PATCH 5/7] Flake8 fixes Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index f020ccb..5a3cd23 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -5072,7 +5072,6 @@ def link_pr_issue(session, issue, request): ''' - associated_issue = [iss.uid for iss in request.related_issues] if issue.uid not in associated_issue: obj = model.PrToIssue( diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 1d64b3a..61bfed0 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -897,7 +897,6 @@ def link_pr_to_ticket(self, session, pr_uid): namespace = request.project.namespace user = request.project.user.user \ if request.project.is_fork else None - branch = request.branch_from for line in pagure.lib.git.read_git_lines( ['log', '--no-walk'] diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index a59ebba..1adbea4 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -40,7 +40,6 @@ from pagure.utils import ( _log = logging.getLogger(__name__) - def _get_parent_request_repo_path(repo): """ Return the path of the parent git repository corresponding to the provided Repository object from the DB. From 8584f338bed69b6ec01ee2ea3e9a87060f1f2eeb Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 30 2018 08:09:38 +0000 Subject: [PATCH 6/7] Fix typo, variable name and restructure when we commit to the DB These are suggestions made by @bowlofeggs during the review Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index 5a3cd23..3eb7203 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -5064,7 +5064,7 @@ def link_pr_issue(session, issue, request): :arg session: The SQLAlchemy session to use :type session: sqlalchemy.orm.session.Session - :arg issue: The issue mentionned in the commits of the pull-requests to + :arg issue: The issue mentioned in the commits of the pull-requests to be associated with :type issue: pagure.lib.model.Issue :arg request: A pull-request to associate the specified issue with @@ -5072,12 +5072,11 @@ def link_pr_issue(session, issue, request): ''' - associated_issue = [iss.uid for iss in request.related_issues] - if issue.uid not in associated_issue: + associated_issues = [iss.uid for iss in request.related_issues] + if issue.uid not in associated_issues: obj = model.PrToIssue( pull_request_uid=request.uid, issue_uid=issue.uid ) session.add(obj) session.flush() - session.commit() diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py index 61bfed0..ce97237 100644 --- a/pagure/lib/tasks.py +++ b/pagure/lib/tasks.py @@ -918,3 +918,9 @@ def link_pr_to_ticket(self, session, pr_uid): 'LINK_PR_TO_TICKET: Link ticket %s to PRs %s' % ( issue, request)) pagure.lib.link_pr_issue(session, issue, request) + + try: + session.commit() + except SQLAlchemyError: + _log.exception('Could not link ticket to PR :(') + session.rollback() From 189430007fb2bcbbf5ed1309baaca8bc0eb4d6c7 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Mar 30 2018 08:09:38 +0000 Subject: [PATCH 7/7] Rely on the list of branches rather than the ``.empty`` attribute There is a risk sometime that the git repo is no longer empty (because we added a specific head for a PR for example) so ``.empty`` returns False while there is actually no content in the git repo. In this case, there will be no branches in that git repo. So use this mechanism to determine if the git repo is empty or not. Fixes https://pagure.io/pagure/issue/3090 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/lib/git.py b/pagure/lib/git.py index b15e8ee..040ac83 100644 --- a/pagure/lib/git.py +++ b/pagure/lib/git.py @@ -1395,7 +1395,8 @@ def get_diff_info(repo_obj, orig_repo, branch_from, branch_to, prid=None): branch = None if branch_to: branch = orig_repo.lookup_branch(branch_to) - if not branch and not orig_repo.is_empty: + local_branches = orig_repo.listall_branches(pygit2.GIT_BRANCH_LOCAL) + if not branch and local_branches: raise pagure.exceptions.BranchNotFoundException( 'Branch %s could not be found in the target repo' % branch_to )