From c292442d75f8c88ff65f3220ff07b3ea15460e43 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Nov 24 2017 08:35:42 +0000 Subject: Allow tagging PR using the same tag list as used for issues. This will allow projects to use tags on PR (tags which here can be updated by the person who opened a PR even if they are not committers). Fixes #2442 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/templates/pull_request.html b/pagure/templates/pull_request.html index 1c59530..d6ef1fe 100644 --- a/pagure/templates/pull_request.html +++ b/pagure/templates/pull_request.html @@ -707,8 +707,9 @@
- {% if authenticated and mergeform and pull_request.status == 'Open' and g.repo_committer %} -
- -
- {{ pull_request.assignee.username or 'Unassigned' }} -
- - {% if authenticated and mergeform and pull_request.status == 'Open' and g.repo_committer %} +
+ +
+ {{ pull_request.assignee.username or 'Unassigned' }} +
+
+ + {% if authenticated and ( + g.repo_user + or g.fas_user.username == pull_request.user.user) %} +
+ + +
+ {% endif%} + +
+ +

+ {% for tag in pull_request.tags %} + + {{ tag.tag }} + + {% endfor %} +

+
+ + {% if authenticated and mergeform and pull_request.status == 'Open' + and (g.repo_committer + or g.fas_user.username == pull_request.user.user) %} Cancel
@@ -1444,6 +1477,23 @@ $(document).ready(function () { $("#comment").atwho(issueAndPrConfig); $("#initial_comment").atwho(issueAndPrConfig); }); + + var available_tags = []; + {%for tog in tag_list %} + available_tags.push("{{tog.tag}}"); + {%endfor%} + var items = available_tags.map(function(x) { return { item: x }; }); + + $('#tag').selectize({ + delimiter: ',', + options: items, + persist: false, + create: false, + labelField: "item", + valueField: "item", + searchField: ["item"], + }); + } ); $(window).on('hashchange', updateHighlight); diff --git a/pagure/ui/fork.py b/pagure/ui/fork.py index b3de389..b7f0afe 100644 --- a/pagure/ui/fork.py +++ b/pagure/ui/fork.py @@ -255,6 +255,7 @@ def request_pull(repo, requestid, username=None, namespace=None): diff=diff, mergeform=form, subscribers=pagure.lib.get_watch_list(SESSION, request), + tag_list=pagure.lib.get_tags_of_project(SESSION, repo), ) @@ -896,19 +897,19 @@ def refresh_request_pull(repo, requestid, username=None, namespace=None): @APP.route( - '//pull-request//assign', methods=['POST']) + '//pull-request//update', methods=['POST']) @APP.route( - '///pull-request//assign', + '///pull-request//update', methods=['POST']) @APP.route( - '/fork///pull-request//assign', + '/fork///pull-request//update', methods=['POST']) @APP.route( - '/fork////pull-request//assign', + '/fork////pull-request//update', methods=['POST']) @login_required -def set_assignee_requests(repo, requestid, username=None, namespace=None): - ''' Assign a pull-request. ''' +def update_pull_requests(repo, requestid, username=None, namespace=None): + ''' Update the metadata of a pull-request. ''' repo = flask.g.repo if not repo.settings.get('pull_requests', True): @@ -923,22 +924,55 @@ def set_assignee_requests(repo, requestid, username=None, namespace=None): if request.status != 'Open': flask.abort(403, 'Pull-request closed') - if not flask.g.repo_committer: - flask.abort(403, 'You are not allowed to assign this pull-request') + if not flask.g.repo_committer \ + and flask.g.fas_user.username != request.user.username: + flask.abort(403, 'You are not allowed to update this pull-request') form = pagure.forms.ConfirmationForm() if form.validate_on_submit(): + tags = [ + tag.strip() + for tag in flask.request.form.get('tag', '').strip().split(',') + if tag.strip()] + + messages = set() try: - # Assign or update assignee of the ticket - message = pagure.lib.add_pull_request_assignee( - SESSION, - request=request, - assignee=flask.request.form.get('user', '').strip() or None, - user=flask.g.fas_user.username, - requestfolder=APP.config['REQUESTS_FOLDER'],) - if message: + # Adjust (add/remove) tags + msgs = pagure.lib.update_tags( + SESSION, request, tags, + username=flask.g.fas_user.username, + gitfolder=APP.config['TICKETS_FOLDER'], + ) + messages = messages.union(set(msgs)) + + if flask.g.repo_committer: + # Assign or update assignee of the ticket + msg = pagure.lib.add_pull_request_assignee( + SESSION, + request=request, + assignee=flask.request.form.get( + 'user', '').strip() or None, + user=flask.g.fas_user.username, + requestfolder=APP.config['REQUESTS_FOLDER'], + ) + if msg: + messages.add(msg) + + if messages: + # Add the comment for field updates: + not_needed = set(['Comment added', 'Updated comment']) + pagure.lib.add_metadata_update_notif( + session=SESSION, + obj=request, + messages=messages - not_needed, + user=flask.g.fas_user.username, + gitfolder=APP.config['REQUESTS_FOLDER'] + ) + messages.add('Metadata fields updated') + SESSION.commit() - flask.flash(message) + for message in messages: + flask.flash(message) except pagure.exceptions.PagureException as err: SESSION.rollback() flask.flash(err.message, 'error') diff --git a/tests/test_pagure_flask_ui_fork.py b/tests/test_pagure_flask_ui_fork.py index 990c9f5..b7717bd 100644 --- a/tests/test_pagure_flask_ui_fork.py +++ b/tests/test_pagure_flask_ui_fork.py @@ -20,7 +20,7 @@ import time import os import pygit2 -from mock import patch +from mock import patch, MagicMock sys.path.insert(0, os.path.join(os.path.dirname( os.path.abspath(__file__)), '..')) @@ -1240,10 +1240,10 @@ index 0000000..2a552bb '\n Pull request canceled!', output.data) - @patch('pagure.lib.notify.send_email') - def test_set_assignee_requests(self, send_email): - """ Test the set_assignee_requests endpoint. """ - send_email.return_value = True + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_update_pull_requests_assign(self): + """ Test the update_pull_requests endpoint when assigning a PR. + """ tests.create_projects(self.session) tests.create_projects_git( @@ -1254,15 +1254,15 @@ index 0000000..2a552bb user.username = 'pingou' with tests.user_set(pagure.APP, user): # No such project - output = self.app.post('/foo/pull-request/1/assign') + output = self.app.post('/foo/pull-request/1/update') self.assertEqual(output.status_code, 404) - output = self.app.post('/test/pull-request/100/assign') + output = self.app.post('/test/pull-request/100/update') self.assertEqual(output.status_code, 404) # Invalid input output = self.app.post( - '/test/pull-request/1/assign', follow_redirects=True) + '/test/pull-request/1/update', follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( 'PR#1: PR from the feature branch - test\n - ' @@ -1285,7 +1285,7 @@ index 0000000..2a552bb # No CSRF output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( @@ -1305,7 +1305,7 @@ index 0000000..2a552bb } output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( @@ -1327,14 +1327,14 @@ index 0000000..2a552bb user.username = 'foo' with tests.user_set(pagure.APP, user): output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 403) user.username = 'pingou' with tests.user_set(pagure.APP, user): output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 200) self.assertIn( @@ -1356,7 +1356,145 @@ index 0000000..2a552bb self.session.commit() output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 403) + + # Project w/o pull-request + repo = pagure.get_authorized_project(self.session, 'test') + settings = repo.settings + settings['pull_requests'] = False + repo.settings = settings + self.session.add(repo) + self.session.commit() + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 404) + + + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_update_pull_requests_tag(self): + """ Test the update_pull_requests endpoint when tagging a PR. + """ + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'requests'), bare=True) + self.set_up_git_repo(new_project=None, branch_from='feature') + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + output = self.app.get('/test/pull-request/1') + self.assertEqual(output.status_code, 200) + + csrf_token = self.get_csrf(output=output) + + data = { + 'tag': 'black', + } + + # No CSRF + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + '<title>PR#1: PR from the feature branch - test\n - ' + 'Pagure', output.data) + self.assertIn( + '

PR#1\n' + ' PR from the feature branch\n', output.data) + self.assertNotIn( + '\n Request assigned', + output.data) + + # Tag the PR + data = { + 'csrf_token': csrf_token, + 'tag': 'black', + } + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#1: PR from the feature branch - test\n - ' + 'Pagure', output.data) + self.assertIn( + '

PR#1\n' + ' PR from the feature branch\n', output.data) + self.assertIn( + '\n Pull-request tagged with: black', + output.data) + self.assertIn( + 'title="comma separated list of tags"\n ' + 'value="black" />', output.data) + + # Try as another user + user.username = 'foo' + with tests.user_set(pagure.APP, user): + # Tag the PR + data = { + 'csrf_token': csrf_token, + 'tag': 'blue, yellow', + } + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 403) + + # Make the PR be from foo + repo = pagure.get_authorized_project(self.session, 'test') + req = repo.requests[0] + req.user_id = 2 + self.session.add(req) + self.session.commit() + + # Re-try to tag the PR + data = { + 'csrf_token': csrf_token, + 'tag': 'blue, yellow', + } + + output = self.app.post( + '/test/pull-request/1/update', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'PR#1: PR from the feature branch - test\n - ' + 'Pagure', output.data) + self.assertIn( + '

PR#1\n' + ' PR from the feature branch\n', output.data) + self.assertIn( + '\n ' + 'Pull-request **un**tagged with: black', + output.data) + self.assertIn( + '\n ' + 'Pull-request tagged with: blue, yellow', + output.data) + self.assertIn( + 'title="comma separated list of tags"\n ' + 'value="blue,yellow" />', output.data) + + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + # Pull-Request closed + repo = pagure.get_authorized_project(self.session, 'test') + req = repo.requests[0] + req.status = 'Closed' + req.closed_by_in = 1 + self.session.add(req) + self.session.commit() + + output = self.app.post( + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 403) @@ -1369,7 +1507,7 @@ index 0000000..2a552bb self.session.commit() output = self.app.post( - '/test/pull-request/1/assign', data=data, + '/test/pull-request/1/update', data=data, follow_redirects=True) self.assertEqual(output.status_code, 404)