I do not plan to do full review. But I would appreciate a regression test for this bug.

Sure, will add one :)

pretty please pagure-ci rebuild

What's the reason the JavaScript functions have restricted access in the first place?
This should basically be just UI stuff, no logic, so you should keep these restrictions so that the "Take"/"Drop" button displays according to who's looking at it.

Would it cause attribute errors to just remove the whole conditions you modified (maybe keeping just "if authenticated" to mitigate possible attr. errors)?

What's the reason the JavaScript functions have restricted access in the first place?

I guess this is a case of belt and suspender. Since the code shouldn't be used by people not having the permission to do so, we simply do not include the code in the page.

The reason I am asking that is that by removing the conditions you could prevent issues such as the one we just experienced. It might be worth removing them but I am not going to insist upon it.
I will go through the tests and see what's happening with the button tomorrow, did not have capacity to do that today, sorry.

No problem and thanks for looking into it :)

I recommend making this change in a separate commit since it's just a style change.

I also recommend making this change in a separate commit.

Looks fine to me! As always, recommendations are just recommendations.

The original button had hover effects which the new one does not have.
Basically, you bring incompatibility of this button with the rest of the UI so I am not sure if this is the best way to go.

@bowlofeggs: This is a separate commit. It might actually be worth merging with the "Fix the tests on view_issue with user having the ticket ACL" commit since the fixed test would fail during git bisect.

I'm fine with keeping the button as button, the real fix is including the JS code that was missing. Changing the button was an additional precaution that should be superfluous.

@pingou: please, do keep it a button. I should hope such an issue should not get into release anymore, if it does, at least it will show.

Done :)

I just realized that the second commit is against pep8, if you could possibly remove it.

There are more issues:

./pagure/api/issue.py:98:9: W503 line break before binary operator
./pagure/api/issue.py:99:9: W503 line break before binary operator
./tests/test_pagure_flask_ui_issues.py:540:54: E231 missing whitespace after ','
./tests/test_pagure_flask_ui_issues.py:541:54: E231 missing whitespace after ','
./tests/test_pagure_flask_ui_issues.py:684:51: E231 missing whitespace after ','
./tests/test_pagure_flask_ui_issues.py:685:51: E231 missing whitespace after ','

but I don't insist on fixing all of them. Just the one commit and this can be pushed.

Btw: checking pep8 compatibility of your Python code is as easy as doing
git diff -U0 master | pep8 --diff when in your branch rebased on the current master you have pulled :)

These two:

./pagure/api/issue.py:98:9: W503 line break before binary operator
./pagure/api/issue.py:99:9: W503 line break before binary operator

are actually in purpose, I like the binary operator at the start of the line which pep8 itself is fine with: http://legacy.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator (just the pep8 tool isn't).

I'll adjust the other pep8 errors though, thanks for pointing them out and thanks for the tip, I didn't know pep8 --diff :)

Did not know about the line break change, thanks!

I suppose this can now go, ACK!

Thanks for the review! :)