From 82e2f39f383595fa70d2883975b50faea27fe6ae Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 02 2017 21:42:37 +0000 Subject: [PATCH 1/5] Improve our markdown processor to link commits directly in the text This way commit hash of 7 to 40 characters are automatically transformed to link to the specified commit. --- diff --git a/pagure/pfmarkdown.py b/pagure/pfmarkdown.py index 31d81aa..394ca7a 100644 --- a/pagure/pfmarkdown.py +++ b/pagure/pfmarkdown.py @@ -52,6 +52,7 @@ COMMIT_LINK_RE = r'(?[\w]{40})' IMPLICIT_ISSUE_RE = r'[^|\w](?\w#])([a-f0-9]{7,40})' STRIKE_THROUGH_RE = r'~~(\w+)~~' @@ -236,6 +237,44 @@ class ImplicitPRPattern(markdown.inlinepatterns.Pattern): return text +class ImplicitCommitPattern(markdown.inlinepatterns.Pattern): + """ Implicit commit pattern. """ + + def handleMatch(self, m): + """ When the pattern matches, update the text. """ + + githash = markdown.util.AtomicString(m.group(2)) + text = ' %s' % githash[:7] + try: + root = flask.request.url_root + url = flask.request.url + except RuntimeError: + return text + repo = namespace = user = None + + if flask.request.args.get('user'): + user = flask.request.args.get('user') + if flask.request.args.get('namespace'): + namespace = flask.request.args.get('namespace') + if flask.request.args.get('repo'): + repo = flask.request.args.get('repo') + + if not user and not repo: + if 'fork/' in url: + user, repo = url.split('fork/')[1].split('/', 2)[:2] + else: + repo = url.split(root)[1].split('/', 1)[0] + + if pagure.lib.search_projects( + pagure.SESSION, + username=user, + namespace=namespace, + pattern=repo): + return _obj_anchor_tag(user, namespace, repo, githash, text) + + return text + + class StrikeThroughPattern(markdown.inlinepatterns.Pattern): """ ~~striked~~ pattern class. """ @@ -258,11 +297,15 @@ class PagureExtension(markdown.extensions.Extension): ]) md.inlinePatterns['mention'] = MentionPattern(MENTION_RE) + + md.inlinePatterns['implicit_commit'] = ImplicitCommitPattern( + IMPLICIT_COMMIT_RE) + md.inlinePatterns['commit_links'] = CommitLinkPattern( + COMMIT_LINK_RE) + if pagure.APP.config.get('ENABLE_TICKETS', True): md.inlinePatterns['implicit_pr'] = \ ImplicitPRPattern(IMPLICIT_PR_RE) - md.inlinePatterns['commit_links'] = \ - CommitLinkPattern(COMMIT_LINK_RE) md.inlinePatterns['explicit_fork_issue'] = \ ExplicitLinkPattern(EXPLICIT_LINK_RE) md.inlinePatterns['implicit_issue'] = \ diff --git a/tests/test_pagure_flask_ui_app.py b/tests/test_pagure_flask_ui_app.py index ede4d3a..1ebb4a2 100644 --- a/tests/test_pagure_flask_ui_app.py +++ b/tests/test_pagure_flask_ui_app.py @@ -477,6 +477,45 @@ class PagureFlaskApptests(tests.Modeltests): """ self.assertEqual(output.data, exp) + tests.create_projects(self.session) + texts = [ + 'pingou committed on test#9364354a4555ba17aa60f0dc844d70b74eb1aecd', + 'Cf commit 936435', # 6 chars - not long enough + 'Cf commit 9364354', # 7 chars - long enough + 'Cf commit 9364354a', # 8 chars - still long enough + 'Cf commit 9364354a4555ba17aa60f0dc844d70b74eb1aecd', # 40 chars + ] + expected = [ + # 'pingou committed on test#9364354a4555ba17aa60f0dc844d70b74eb1aecd', + '

pingou committed on test#9364354a4555ba17aa60f0dc844d70b74eb1aecd

', + # 'Cf commit 936435', + '

Cf commit 936435

', + # 'Cf commit 9364354', + #'

Cf commit 9364354

', + '

Cf commit 9364354

', + # 'Cf commit 9364354a', + '

Cf commit 9364354

', + # 'Cf commit 9364354a4555ba17aa60f0dc844d70b74eb1aecd', + '

Cf commit 9364354

', + ] + + with pagure.APP.app_context(): + for idx, text in enumerate(texts): + #print idx, text + data = { + 'content': text, + 'csrf_token': csrf_token, + } + output = self.app.post('/markdown/?repo=test', data=data) + self.assertEqual(output.status_code, 200) + self.assertEqual(expected[idx], output.data) + @patch('pagure.ui.app.admin_session_timedout') def test_remove_user_email(self, ast): """ Test the remove_user_email endpoint. """ From 771e6500bb516fce0fdd2b40db6360d2301d17bc Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 02 2017 21:42:37 +0000 Subject: [PATCH 2/5] Simplify the logic around retrieving information from the request --- diff --git a/pagure/pfmarkdown.py b/pagure/pfmarkdown.py index 394ca7a..0011806 100644 --- a/pagure/pfmarkdown.py +++ b/pagure/pfmarkdown.py @@ -170,12 +170,9 @@ class ImplicitIssuePattern(markdown.inlinepatterns.Pattern): return text repo = namespace = user = None - if flask.request.args.get('user'): - user = flask.request.args.get('user') - if flask.request.args.get('namespace'): - namespace = flask.request.args.get('namespace') - if flask.request.args.get('repo'): - repo = flask.request.args.get('repo') + user = flask.request.args.get('user') + namespace = flask.request.args.get('namespace') + repo = flask.request.args.get('repo') if not user and not repo: if 'fork/' in url: @@ -213,12 +210,9 @@ class ImplicitPRPattern(markdown.inlinepatterns.Pattern): return text repo = namespace = user = None - if flask.request.args.get('user'): - user = flask.request.args.get('user') - if flask.request.args.get('namespace'): - namespace = flask.request.args.get('namespace') - if flask.request.args.get('repo'): - repo = flask.request.args.get('repo') + user = flask.request.args.get('user') + namespace = flask.request.args.get('namespace') + repo = flask.request.args.get('repo') if not user and not repo: if 'fork/' in url: From be0baf78ba70b483102ec12a1ad8c94f35fbee79 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 02 2017 21:42:37 +0000 Subject: [PATCH 3/5] Check if the hash found exists in the repo and only link to it if it does --- diff --git a/pagure/pfmarkdown.py b/pagure/pfmarkdown.py index 0011806..38a537b 100644 --- a/pagure/pfmarkdown.py +++ b/pagure/pfmarkdown.py @@ -25,6 +25,7 @@ import flask import markdown.inlinepatterns import markdown.util +import pygit2 import pagure import pagure.lib @@ -238,7 +239,7 @@ class ImplicitCommitPattern(markdown.inlinepatterns.Pattern): """ When the pattern matches, update the text. """ githash = markdown.util.AtomicString(m.group(2)) - text = ' %s' % githash[:7] + text = ' %s' % githash try: root = flask.request.url_root url = flask.request.url @@ -263,8 +264,9 @@ class ImplicitCommitPattern(markdown.inlinepatterns.Pattern): pagure.SESSION, username=user, namespace=namespace, - pattern=repo): - return _obj_anchor_tag(user, namespace, repo, githash, text) + pattern=repo) \ + and _commit_exists(user, namespace, repo, githash): + return _obj_anchor_tag(user, namespace, repo, githash, text[:8]) return text @@ -345,6 +347,18 @@ def _pr_exists(user, namespace, repo, idx): return pr_obj +def _commit_exists(user, namespace, repo, githash): + """ Utility method checking if a given commit exists. """ + repo_obj = pagure.lib.get_project( + pagure.SESSION, name=repo, user=user, namespace=namespace) + if not repo_obj: + return False + + reponame = pagure.get_repo_path(repo_obj) + git_repo = pygit2.Repository(reponame) + return githash in git_repo + + def _obj_anchor_tag(user, namespace, repo, obj, text): """ Utility method generating the link to an issue or a PR. From 80e0f251b9f78ddc48433d6232e1d177626b7a68 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 02 2017 21:42:37 +0000 Subject: [PATCH 4/5] Expand the unit-tests around our markdown processor for git commit Basically with this commit we are checking its behavior when the git hash is found in the repo and when it is not. --- diff --git a/tests/test_pagure_flask_ui_app.py b/tests/test_pagure_flask_ui_app.py index 1ebb4a2..581c4a0 100644 --- a/tests/test_pagure_flask_ui_app.py +++ b/tests/test_pagure_flask_ui_app.py @@ -440,6 +440,19 @@ class PagureFlaskApptests(tests.Modeltests): output = self.app.get('/settings/') self.assertEqual(output.status_code, 302) + def patched_commit_exists(user, namespace, repo, githash): + ''' Patched version of pagure.pfmarkdown._commit_exists to enforce + returning true on some given hash without having us actually check + the git repos. + ''' + if githash in ['9364354', '9364354a', '9364354a4555ba17aa60f0dc844d70b74eb1aecd']: + return True + else: + return False + + @patch( + 'pagure.pfmarkdown._commit_exists', + MagicMock(side_effect=patched_commit_exists)) def test_markdown_preview(self): """ Test the markdown_preview endpoint. """ @@ -507,7 +520,7 @@ class PagureFlaskApptests(tests.Modeltests): with pagure.APP.app_context(): for idx, text in enumerate(texts): - #print idx, text + print idx, text data = { 'content': text, 'csrf_token': csrf_token, @@ -516,6 +529,38 @@ class PagureFlaskApptests(tests.Modeltests): self.assertEqual(output.status_code, 200) self.assertEqual(expected[idx], output.data) + @patch( + 'pagure.pfmarkdown._commit_exists', + MagicMock(side_effect=patched_commit_exists)) + def test_markdown_preview(self): + """ Test the markdown_preview endpoint. """ + + user = tests.FakeUser() + user.username = 'foo' + with tests.user_set(pagure.APP, user): + output = self.app.get('/settings/') + self.assertEqual(output.status_code, 200) + self.assertIn( + '
\n Basic Information\n' + '
', output.data) + + csrf_token = output.data.split( + 'name="csrf_token" type="hidden" value="')[1].split('">')[0] + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path), bare=True) + text = 'Cf commit 9364354a4555ba17aa60f0d' + exp = '

Cf commit 9364354a4555ba17aa60f0d

' + + with pagure.APP.app_context(): + data = { + 'content': text, + 'csrf_token': csrf_token, + } + output = self.app.post('/markdown/?repo=test', data=data) + self.assertEqual(output.status_code, 200) + self.assertEqual(exp, output.data) + @patch('pagure.ui.app.admin_session_timedout') def test_remove_user_email(self, ast): """ Test the remove_user_email endpoint. """ From 30383f1ebf39b07d7056604be845273d99837b7f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 02 2017 21:42:37 +0000 Subject: [PATCH 5/5] And again some more code simplification thanks @bowlofeggs --- diff --git a/pagure/pfmarkdown.py b/pagure/pfmarkdown.py index 38a537b..525dba1 100644 --- a/pagure/pfmarkdown.py +++ b/pagure/pfmarkdown.py @@ -169,7 +169,6 @@ class ImplicitIssuePattern(markdown.inlinepatterns.Pattern): url = flask.request.url except RuntimeError: return text - repo = namespace = user = None user = flask.request.args.get('user') namespace = flask.request.args.get('namespace') @@ -209,7 +208,6 @@ class ImplicitPRPattern(markdown.inlinepatterns.Pattern): url = flask.request.url except RuntimeError: return text - repo = namespace = user = None user = flask.request.args.get('user') namespace = flask.request.args.get('namespace') @@ -245,14 +243,10 @@ class ImplicitCommitPattern(markdown.inlinepatterns.Pattern): url = flask.request.url except RuntimeError: return text - repo = namespace = user = None - - if flask.request.args.get('user'): - user = flask.request.args.get('user') - if flask.request.args.get('namespace'): - namespace = flask.request.args.get('namespace') - if flask.request.args.get('repo'): - repo = flask.request.args.get('repo') + + user = flask.request.args.get('user') + namespace = flask.request.args.get('namespace') + repo = flask.request.args.get('repo') if not user and not repo: if 'fork/' in url: