PR#3349: Allow to reopen pull requests - pagure

pagure

#3349 Allow to reopen pull requests

Merged 5 years ago by pingou. Opened 5 years ago by karsten.

karsten/pagure reopen_pr into master

Allow to reopen pull requests

Karsten Hopp • 5 years ago

3933fdd

pagure/lib/__init__.py

file modified

+35

		`@@ -2923,6 +2923,41 @@`
		`return output`


		`+ def reopen_pull_request(session, request, user, requestfolder):`
		`+ ''' Re-Open the provided pull request`
		`+ '''`
		`+ if request.status != 'Closed':`
		`+ raise pagure.exceptions.PagureException(`
		`+ 'Trying to reopen a pull request that is not closed'`
		`+ )`
		`+ user_obj = get_user(session, user)`
		`+ request.status = 'Open'`
		`+ session.add(request)`
		`+ session.flush()`
		`+ log_action(session, request.status.lower(), request, user_obj)`
		`+ pagure.lib.notify.notify_reopen_pull_request(request, user_obj)`
		`+ pagure.lib.git.update_git(`
		`+ request, repo=request.project, repofolder=requestfolder)`
		`+ pagure.lib.add_pull_request_comment(`
		`+ session, request,`
		`+ commit=None, tree_id=None, filename=None, row=None,`
		`+ comment='Pull-Request has been reopened by %s' % (`
		`+ user),`
		`+ user=user,`
		`+ requestfolder=requestfolder,`
		`+ notify=False, notification=True`
		`+ )`
		`+ pagure.lib.notify.log(`
		`+ request.project,`
		`+ topic='pull-request.reopened',`
		`+ msg=dict(`
		`+ pullrequest=request.to_json(public=True),`
		`+ agent=user_obj.username,`
		`+ ),`
		`+ redis=REDIS,`
		`+ )`
		`+`
		`+`
		`def close_pull_request(session, request, user, requestfolder, merged=True):`
		`''' Close the provided pull-request.`
		`'''`

pagure/lib/notify.py

file modified

+36

		`@@ -660,6 +660,42 @@`
		`)`


		`+ def notify_reopen_pull_request(request, user):`
		`+ ''' Notify the people following a project that a closed pull-request`
		`+ has been reopened.`
		`+ '''`
		`+ text = """`
		+ %s reopened a pull-request against the project: `%s` that you are following.
		`+`
		`+ Reopened pull-request:`
		`+`
		+ ``
		`+ %s`
		+ ``
		`+`
		`+ %s`
		`+ """ % (user.username,`
		`+ request.project.name,`
		`+ request.title,`
		`+ _build_url(`
		`+ pagure_config['APP_URL'],`
		`+ _fullname_to_url(request.project.fullname),`
		`+ 'pull-request',`
		`+ request.id))`
		`+ mail_to = _get_emails_for_obj(request)`
		`+`
		`+ uid = time.mktime(datetime.datetime.now().timetuple())`
		`+ send_email(`
		`+ text,`
		`+ 'PR #%s: %s' % (request.id, request.title),`
		`+ ','.join(mail_to),`
		`+ mail_id='%s/close/%s' % (request.mail_id, uid),`
		`+ in_reply_to=request.mail_id,`
		`+ project_name=request.project.fullname,`
		`+ user_from=user.fullname or user.user,`
		`+ )`
		`+`
		`+`
		`def notify_cancelled_pull_request(request, user):`
		`''' Notify the people following a project that a pull-request was`
		`cancelled in it.`

pagure/templates/pull_request.html

file modified

+18 -1

		`@@ -30,7 +30,7 @@`
		`<div class="col-md-12">`
		`{% if pull_request %}`
		`<h3><span class="label label-default">PR#{{requestid}}</span>`
		`- {% if pull_request.status != 'Open'%}`
		`+ {% if pull_request.status != 'Open' and pull_request.status != 'Closed' %}`
		`<span class="label {{'label-success' if pull_request.status == 'Merged' else 'label-danger'}}">{{pull_request.status}}</span>`
		`{% endif %}`
		`{{ pull_request.title \| noJS(ignore="img") \| safe}}`
		`@@ -705,6 +705,19 @@`
		`<a href="{{ url_for('ui_ns.view_user', username=pull_request.closed_by.user)}} ">`
		`{{ pull_request.closed_by.user if pull_request.closed_by else ''}}`
		`</a> {{pull_request.closed_at\|humanize}}`
		`+ {% if pull_request.status == 'Closed' and g.authenticated and`
		`+ (g.repo_committer or g.fas_user.username == pull_request.user.username) %}`
		`+ <form action="{{ url_for(`
		`+ 'ui_ns.reopen_request_pull', username=username,`
		`+ namespace=repo.namespace,`
		`+ repo=repo.name, requestid=requestid) }}" method="POST">`
		`+ {{ mergeform.csrf_token }}`
		`+ <button type="submit" value="Reopen" id="reopen_pr"`
		`+ class="btn btn-sm btn-primary" title="Reopen PR">`
		`+ Re-Open`
		`+ </button>`
		`+ </form>`
		`+ {% endif %}`
		`</div>`
		`</div>`
		`{% endif %}`
		`@@ -1190,6 +1203,10 @@`
		`return window.confirm("Are you sure you want to close this requested pull?");`
		`});`

		`+ $('#reopen_pr').click(function(){`
		`+ return window.confirm("Are you sure you want to reopen this requested pull?");`
		`+ });`
		`+`
		`$( ".code_table tr" ).hover(`
		`function() {`
		`$( this ).find( ".prc_img" ).show().width(13);`

pagure/ui/fork.py

file modified

+57

		`@@ -745,6 +745,63 @@`


		`@UI_NS.route(`
		`+ '/<repo>/pull-request/<int:requestid>/reopen', methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/<namespace>/<repo>/pull-request/<int:requestid>/reopen',`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<repo>/pull-request/<int:requestid>/reopen',`
		`+ methods=['POST'])`
		`+ @UI_NS.route(`
		`+ '/fork/<username>/<namespace>/<repo>/pull-request/<int:requestid>/reopen',`
		`+ methods=['POST'])`
		`+ @login_required`
		`+ def reopen_request_pull(repo, requestid, username=None, namespace=None):`
		`+ """ Re-Open a pull request.`
		`+ """`
		`+ form = pagure.forms.ConfirmationForm()`
		`+ if form.validate_on_submit():`
		`+`
		`+ if not flask.g.repo.settings.get('pull_requests', True):`
		`+ flask.abort(404, 'No pull-requests found for this project')`
		`+`
		`+ request = pagure.lib.search_pull_requests(`
		`+ flask.g.session, project_id=flask.g.repo.id, requestid=requestid)`
		`+`
		`+ if not request:`
		`+ flask.abort(404, 'Pull-request not found')`
		`+`
		`+ if not flask.g.repo_committer \`
		`+ and not flask.g.fas_user.username == request.user.username:`
		`+ flask.abort(`
		`+ 403,`
		`+ 'You are not allowed to reopen pull-request for this project')`
		`+`
		`+ try:`
		`+ pagure.lib.reopen_pull_request(`
		`+ flask.g.session, request, flask.g.fas_user.username,`
		`+ requestfolder=pagure_config['REQUESTS_FOLDER'])`
		`+ except pagure.exceptions.PagureException as err:`
		`+ flask.flash(str(err), 'error')`
		`+`
		`+ try:`
		`+ flask.g.session.commit()`
		`+ flask.flash('Pull request reopened!')`
		`+ except SQLAlchemyError as err: # pragma: no cover`
		`+ flask.g.session.rollback()`
		`+ _log.exception(err)`
		`+ flask.flash(`
		`+ 'Could not update this pull-request in the database',`
		`+ 'error')`
		`+`
		`+ else:`
		`+ flask.flash('Invalid input submitted', 'error')`
		`+`
		`+ return flask.redirect(flask.url_for(`
		`+ 'ui_ns.request_pull', repo=repo, username=username, namespace=namespace, requestid=requestid))`
		`+`
		`+`
		`+ @UI_NS.route(`
		`'/<repo>/pull-request/<int:requestid>/merge', methods=['POST'])`
		`@UI_NS.route(`
		`'/<namespace>/<repo>/pull-request/<int:requestid>/merge',`

tests/test_pagure_flask_ui_fork.py

file modified

+92

		`@@ -1478,6 +1478,98 @@`
		`'</button>\n Pull request canceled!',`
		`output_text)`

		`+ @patch('pagure.lib.notify.send_email')`
		`+ def test_reopen_request_pull(self, send_email):`
		`+ """ Test the reopen_request_pull endpoint. """`
		`+ send_email.return_value = True`
		`+`
		`+ 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', mtype='merge')`
		`+`
		`+ user = tests.FakeUser()`
		`+ with tests.user_set(self.app.application, user):`
		`+ output = self.app.post('/test/pull-request/1/reopen')`
		`+ self.assertEqual(output.status_code, 302)`
		`+`
		`+ output = self.app.post(`
		`+ '/test/pull-request/1/reopen', follow_redirects=True)`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ '<title>PR#1: PR from the feature branch - test\n - Pagure</title>', output_text)`
		`+ self.assertIn(`
		`+ #'</button>\n Pull request reopened!',`
		`+ 'return window.confirm("Are you sure you want to reopen this requested pull?")',`
		`+ output_text)`
		`+`
		`+ output = self.app.get('/test/pull-request/1')`
		`+ self.assertEqual(output.status_code, 200)`
		`+`
		`+ csrf_token = self.get_csrf(output=output)`
		`+`
		`+ data = {`
		`+ 'csrf_token': csrf_token,`
		`+ }`
		`+`
		`+ # Invalid project`
		`+ output = self.app.post(`
		`+ '/foo/pull-request/1/reopen', data=data,`
		`+ follow_redirects=True)`
		`+ self.assertEqual(output.status_code, 404)`
		`+`
		`+ # Invalid PR id`
		`+ output = self.app.post(`
		`+ '/test/pull-request/100/reopen', data=data,`
		`+ follow_redirects=True)`
		`+ self.assertEqual(output.status_code, 404)`
		`+`
		`+ # Invalid user for this project`
		`+ output = self.app.post(`
		`+ '/test/pull-request/1/reopen', data=data,`
		`+ follow_redirects=True)`
		`+ self.assertEqual(output.status_code, 403)`
		`+`
		`+ user.username = 'pingou'`
		`+ with tests.user_set(self.app.application, user):`
		`+ # Project w/o pull-request`
		`+ repo = pagure.lib.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/reopen', data=data,`
		`+ follow_redirects=True)`
		`+ self.assertEqual(output.status_code, 404)`
		`+`
		`+ # Project w/ pull-request`
		`+ repo = pagure.lib.get_authorized_project(self.session, 'test')`
		`+ settings = repo.settings`
		`+ settings['pull_requests'] = True`
		`+ repo.settings = settings`
		`+ self.session.add(repo)`
		`+ self.session.commit()`
		`+`
		`+ output = self.app.post(`
		`+ '/test/pull-request/cancel/1', data=data,`
		`+ follow_redirects=True)`
		`+ output = self.app.post(`
		`+ '/test/pull-request/1/reopen', data=data,`
		`+ follow_redirects=True)`
		`+ self.assertEqual(output.status_code, 200)`
		`+ output_text = output.get_data(as_text=True)`
		`+ self.assertIn(`
		`+ '<title>PR#1: PR from the feature branch - test\n - '`
		`+ 'Pagure</title>', output_text)`
		`+ self.assertIn(`
		`+ 'return window.confirm("Are you sure you want to reopen this requested pull?")',`
		`+ output_text)`
		`+`
		`@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.`

karsten commented 5 years ago

Fixes: https://pagure.io/pagure/issue/2643

Signed-off-by: Karsten Hopp karsten@redhat.com

ngompa commented 5 years ago

:thumbsup: :100:

karsten commented 5 years ago

Two issues where I could use some hints:
- I'd like to get back to the reopened PR when the button was clicked, not to the overview over all PRs. How can I do that ?

I don't like the button with the open envelope. do you have a better suggestion ?

pingou commented on line 42 of pagure/ui/fork.py 5 years ago

 return flask.redirect(flask.url_for(
         'ui_ns.request_pull', repo=repo, username=username, namespace=namespace, requestid=request.id))

karsten commented 5 years ago

perfect, thanks!

1 new commit added

return to reopened PR

5 years ago

pingou commented 5 years ago

I don't like the button with the open envelope. do you have a better suggestion ?

I'm fixing the tests for #3344 and I'll check this

1 new commit added

fix whitespace

5 years ago

karsten commented 5 years ago

Pretty please pagure-ci rebuild

pingou commented on line 13 of pagure/templates/pull_request.html 5 years ago

Note that you open a form here without closing it later on :)

pingou commented on line 13 of pagure/lib/__init__.py 5 years ago

Where are these two fields coming from?

pingou commented on line 22 of pagure/lib/__init__.py 5 years ago

I'm confused a little bit here, can req_status ever be something else than 'reopened' ?

pingou commented on line 35 of pagure/lib/__init__.py 5 years ago

Similar question here: can merged ever be something else than 'False'?
Also, I'm not sure I understand the idea, is it to indicate someone re-opened a merged PR? Do we want to allow it?

pingou commented on line 35 of pagure/lib/__init__.py 5 years ago

Do we want to allow it?

The first two lines of this method seem to indicate we do not want to allow it, which makes sense to me, but then I don't understand these lines here

pingou commented on line 54 of pagure/ui/fork.py 5 years ago

Fails on me with: UnboundLocalError: local variable 'request' referenced before assignment

pingou commented on line 12 of pagure/templates/pull_request.html 5 years ago

The CSRF token is defined outside of the form, so it won't be passed to the form :)

pingou commented 5 years ago

From an UI point of view, what do you think of:

diff --git a/ pagure/templates/pull_request.html b/ pagure/templates/pull_request.html
index f9cc89e3..bfb4e2b3 100644
--- a/ pagure/templates/pull_request.html       
+++ b/ pagure/templates/pull_request.html       
@@ -34,18 +34,6 @@
   <span class="label {{'label-success' if pull_request.status == 'Merged' else 'label-danger'}}">{{pull_request.status}}</span>
   {% endif %}
   {{ pull_request.title | noJS(ignore="img") | safe}}
-  {% if pull_request.status == 'Closed' and g.authenticated and
-      (g.repo_committer or g.fas_user.username == pull_request.user.username) %}
-      {{ mergeform.csrf_token }}
-         <form style="display:inline;" action="{{ url_for(
-            'ui_ns.reopen_request_pull', username=username,
-            namespace=repo.namespace,
-            repo=repo.name, requestid=requestid) }}" method="POST">
-          <button type="submit" value="Reopen" id="reopen_pr"
-                  class="btn btn-sm btn-primary" title="Reopen PR">
-            <span class="fa fa-envelope-open"></span>
-          </button>
-  {% endif %}
   {% if g.authenticated and (g.fas_user.username == pull_request.user.username
     or g.repo_committer) and pull_request.status == 'Open'%}
      <span class="pull-xs-right">
@@ -717,6 +705,19 @@
           <a href="{{ url_for('ui_ns.view_user', username=pull_request.closed_by.user)}} ">
             {{ pull_request.closed_by.user if pull_request.closed_by else ''}}
           </a> {{pull_request.closed_at|humanize}}
+        {% if pull_request.status == 'Closed' and g.authenticated and
+           (g.repo_committer or g.fas_user.username == pull_request.user.username) %}
+         <form action="{{ url_for(
+                'ui_ns.reopen_request_pull', username=username,
+                namespace=repo.namespace,
+                repo=repo.name, requestid=requestid) }}" method="POST">
+          {{ mergeform.csrf_token }}
+          <button type="submit" value="Reopen" id="reopen_pr"
+                  class="btn btn-sm btn-primary" title="Reopen PR">
+            Re-Open
+          </button>
+        </form>
+        {% endif %}
        </div>
       </div>
       {% endif %}

karsten commented on line 13 of pagure/lib/__init__.py 5 years ago

The initial idea was to have the reopening info in the DB. Later on I've dismissed that idea as that info will be available in the comments anyway. I'll delete these lines

karsten commented on line 22 of pagure/lib/__init__.py 5 years ago

This is just to create a correct sentence, otherwise we would have 'Pull-Request has been open by karsten'

karsten commented on line 22 of pagure/lib/__init__.py 5 years ago

I think I know what you mean. Replace request.status.lower() by the fixed string 'reopened'.
I'll do that as right above that line i set request.status to 'Open', so it cannot be anything else.

karsten commented on line 35 of pagure/lib/__init__.py 5 years ago

I'll drop it, it makes no sense here. At this point request.status is always 'Open' . If the PR has already been merged we'd open a can of worms if we'd allow thesame PR again. Its better to create a completely new PR in that case.

karsten commented on line 54 of pagure/ui/fork.py 5 years ago

It should have aborted in line 29 if request was not set.
Can you describe what you did to make that happen ?

pingou commented on line 54 of pagure/ui/fork.py 5 years ago

If the CSRF token is missing, which is the case in the current template, the form doesn't validate and you end up here where request is undefined :)

karsten commented 5 years ago

I like your solution with the Re-Open button on the right sidebar.
Actually I think that even the 'Cancel' button should be moved from the title to the right side.

2 new commits added