[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Ezio Melotti added the comment: This seems to work, so I'm going to close it. -- status: testing -> resolved ___ PSF Meta Tracker

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Anish Shah added the comment: Yep. It might be because of permissions issue. ___ PSF Meta Tracker ___ _

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Anish, yes that's intentional. But you should not have access rights to edit anything there. ___ PSF Meta Tracker ___

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Anish Shah added the comment: Hi, on this link http://bugs.python.org/pull_request, I am getting a textbox where I can delete/edit entries. Do we want this? I am not sure. ___ PSF Meta Tracker

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Ezio Melotti added the comment: I committed the latest patch with a few changes (fixed a bug in the url generation and removed the [remove] buttons) in https://hg.python.org/tracker/python-dev/rev/f2fa6e43904a I'll leave the issue open for another day in case something comes up. Thanks Anish an

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Uploading fixed patch with Ezio's changes. ___ PSF Meta Tracker ___diff --git a/detectors/pull_request

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Uploading final version using just the number. ___ PSF Meta Tracker ___diff --git a/detectors/pull_req

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: The alternate version with pr number and repository fields. ___ PSF Meta Tracker ___diff --git a/detec

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: I've added slight changes to previous patch, fixing the look of the PRs table and the class in hgrepos table header (it had typo, Header vs header). ___ PSF Meta Tracker

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Permissions problem should be addressed as well, since I've added option to unlink, rather than remove. Similarly to what is available for files. ___ PSF Meta Tracker

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Apparently I can't leave title set to none, I had to explicitly set it to empty if none is set. ___ PSF Meta Tracker

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Addressed 1, 2, 4, 5. I'll handle 3rd along with title as a followup issue. ___ PSF Meta Tracker ___d

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Ezio Melotti added the comment: I've looked at the latest patch (2016-07-21.16:13:52) and the only major issue I've found so far is that any user can unlink (remove) any PR, even the ones created by other users. This is likely because of wrong permissions in schema.py. There are also a few m

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: LGTM ___ PSF Meta Tracker ___ ___ Tracker-discuss mailing

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Anish Shah added the comment: I have updated the patch. I have removed pr_no field entirely. ___ PSF Meta Tracker ___commit 299d794e5e7

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Anish, actually there is one serious problem that grows from previous comment. Generally your current solution blocks adding single PR to multiple issues, because of the key. Generally, I'd advise you to drop the key entirely and do it similarly to how hgrepos

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: Anish before accepting (if there are no other objections) there's still one issue. If I add a PR, remove it and re-add it I'm hitting the detector's "GitHub PR already added to an issue". Can you fix it, please? _

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Berker Peksag added the comment: The API call in my patch was just an example. GitHub does provide a search API. However, the problem with my approach is that it ignores rate limiting of GitHub API: https://api.github.com/rate_limit So Anish's patch might be more reliable than mine. _

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

R David Murray added the comment: I anticipate that even if we improve our patch commit rate we'll still have 1000+ open PRs. I don't think pounding on github's infrastructure for every page render on our bug tracker is very friendly, not to mention probably not very performant on our end. U

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Berker Peksag added the comment: I don't have an objection to a general purpose webhook (I wrote such webhooks for GitHub before.) Using a webhook for a simple task like this will only make the implementation more complicated. I don't see a reason to store pull request URLs in database. We've

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Anish Shah added the comment: I have updated the patch according to reviews suggested by @maciej.szulik and @berker.peksag Description:- This patch adds a new table "pull_request" which is used to save GitHub PR URLs. URL can be added manually using "GitHub PR" field on issue's page. --- GitH

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Anish Shah added the comment: @berker.peksag I looked at your patch and screenshot. What you are doing is calling GitHub API using JS each time an issue page is loaded and showing PRs which have a string like "Issue #NN" in its title. I have just added a description in issue 589 (sorry for no

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Berker Peksag added the comment: I'm not sure I explained myself clearly so here is a quick and dirty patch, including a screenshot: https://dl.dropboxusercontent.com/u/166024/gh-pr.png Since I don't have meaningful data I chose Django as an example. ___

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Maciej Szulik added the comment: @anish.shah > I'm reconstructing because URL might contain fragment identifier/query > string. What do you think? Doesn't matter, the parsed_url is just an object representation of that url. > I will change this. But, I will need to change this in all the pat

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

R David Murray added the comment: Berker: I presume the point here is that this creates the field, and issue 589 fills it in automatically by having github call roundup when a PR is created. I haven't looked at either patch closely enough to know if I'm right, but that's what I'd expect the i

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Berker Peksag added the comment: Issue 589 is a different feature and it should be discussed there (by the way it would be really nice if you could give more information about the feature and the patch -- I had to read the whole patch to understand what it does). The problem with this implemen

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Anish Shah added the comment: @berker.peksag Thank you for the review. I will make those changes. > Also, I'm not fan of such a manual work. I would prefer to have a JSON > endpoint > which queries open pull requests on python/cpython and match if > the title of a > pull request contains the

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Anish Shah added the comment: > Why do you need to reconstruct the url in this case? Why not just assigning > the one you verified is ok? I'm reconstructing because URL might contain fragment identifier/query string. What do you think? > Make shorter, 'github_prs' is enough. > 'id', 'no', 'p

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Berker Peksag added the comment: Also, I'm not fan of such a manual work. I would prefer to have a JSON endpoint which queries open pull requests on python/cpython and match if the title of a pull request contains the issue number. You can see https://code.djangoproject.com/ticket/25227 for an

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

Berker Peksag added the comment: +url = newvalues.get('url', '') You should also return early if there is no 'url' key in newvalues. i18n:translate="" can be removed. I don't think we are going to support a non-English language. I would also prefer a 'remove' link instead of 'edit'. This

[Tracker-discuss] [issue586] Add GitHub Pull Request URL on issue page

New submission from Maciej Szulik: Anish here are my review comments: 1. detectors/github_pullrequest_url.py Add an empty line before imports. > if parsed_url.scheme not in ('http', 'https'): You're expecting http or https, but what if I paste github.com/python/cpython/pull/41 which is perfe