:thumbsup: :100:

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 ?

perfect, thanks!

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

Pretty please pagure-ci rebuild

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

Where are these two fields coming from?

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

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?

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

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

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

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 %}

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

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

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.

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.

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

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 :)

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.

I'd suggest to raise an exception here, so we can catch it in the controller and show an error message to the user

I guess this can be dropped now

I think this can also be dropped

If this is closing the {% if .. %} block from above, the former indentation looks more inline

if this needed now?

This should be the PR page no?

I disagree. '(% if' starts in column 6 and '{% endif %}' starts in column 5 (fresh git clone).
But I'll revert it, this should'nt be part of this PR.

You likely want: if request.status != 'Closed': rather than a is not equal :)

This raises exceptions now :)

One empty line too many :)

Should we remove the lines above?

Commit 130d0e7d fixes this pull-request