pull-request: warn against attaching pull-requests to others' issues

[wking]

You can only attach a pull request to your own issues 1. If there is
something wrong with a pull request 2 and the user was trying to attach it
to an existing issue, warn the user about this limitation.

I was expecting something along these lines after @mislav's comment in #189 (3),
but I don't think a clearer error message made it into the trunk yet, so I added it myself.

[mislav]

It's a good start, but 422 doesn't always mean that this was the problem. How about not guessing, but using GitHub's API to fetch the issue in question and check ownership?

Also, I believe you can turn any issue into pull request on a project that you have commit rights to, not just those that are owned by you.

[wking]

On Thu, Dec 13, 2012 at 10:02:48AM -0800, Mislav Marohnić wrote:

It's a good start, but 422 doesn't always mean that this was the
problem.

The warning doesn't claim that it is the problem, it just warns that
it might be the problem. I wonder if we can get GitHub to give better
warnings on 422. Trying to guess why they've refused the pull request
is brittle.

How about not guessing, but using GitHub's API to fetch the issue in
question and check ownership?

If we go down that road, we'll end up with “is this pull request
valid?” code that tries to duplicate whatever GitHub does internally.
I think it's easier to just print out collected wisdom (“in the past,
people have seen this error when …”) than it would be to do anything
more rigorous. I'd be happy to reroll with an ownership-check if you
don't agree ;).

Also, I believe you can turn any issue into pull request on a
project that you have commit rights to, not just those that are
owned by you.

Ah, I will definitely add this to the warning message/ownership-check
on reroll. It would be nice if these limitations were documented in
1. I'll bump support@ and see if they want to chime in.

[pengwynn]

You should be getting a hash of validation errors back on a 422. Please share your (sanitized) output if you find otherwise.

There are basically two checks for creating a PR from an otherwise valid Issue

• Issue number can't already be a PR type
• Authenticating user must own the Issue or have push access to the repository

[mislav]

@pengwynn Thanks for the clarification. I already show validation errors on 422s but in these cases such errors aren't displayed. I was convinced that GitHub doesn't set proper validation errors in 422 JSON response. However, it might be a bug in hub. I will look into it

[mislav]

@pengwynn Yes I'm getting JSON validation errors. However the error code is vague (just says "invalid"). I will have to improve how I report these errors in the UI.

{
  "errors": [
    {
      "field": "issue",
      "value": 269,
      "resource": "PullRequest",
      "code": "invalid"
    }
  ],
  "message": "Validation Failed"
}

@wking Closing this without pulling. I will handle the error message elsewhere.

[pengwynn]

Bah, we should be more descriptive there. I'll open an internal ticket, too.

[wking]

On Mon, Dec 17, 2012 at 11:30:09AM -0800, Wynn Netherland wrote:

You should be getting a hash of errors back in the JSON
response. Are you finding otherwise?

I don't think I got any JSON back with my 422:

$ cat data
{
"issue": "189",
"head": "wking:test",
"base": "master"
}
$ wget --header 'Authorization: token REDACTED' --post-file data -S -O out https://api.github.com/repos/defunkt/hub/pulls
--2012-12-17 16:38:35-- https://api.github.com/repos/defunkt/hub/pulls
Resolving api.github.com... 207.97.227.243
Connecting to api.github.com|207.97.227.243|:443... connected.
HTTP request sent, awaiting response...
HTTP/1.1 422 Unprocessable Entity
Server: nginx
Date: Mon, 17 Dec 2012 21:38:27 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Status: 422 Unprocessable Entity
Content-Length: 154
X-Accepted-OAuth-Scopes: repo, public_repo
X-RateLimit-Remaining: 4998
X-RateLimit-Limit: 5000
Cache-Control:
X-GitHub-Media-Type: github.beta
X-Content-Type-Options: nosniff
X-OAuth-Scopes: repo
2012-12-17 16:38:35 ERROR 422: Unprocessable Entity.
$ ls -l out
-rw-r--r-- 1 wking wking 0 Dec 17 16:38 out