Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for creating project cards from pull requests #2185

Merged
merged 1 commit into from
Jun 7, 2020

Conversation

boblangley
Copy link
Contributor

@boblangley boblangley commented Apr 29, 2020

The v3 GitHub API now supports adding Pull Requests as project cards, which was disabled when support for project cards was initially added to OctoKit. This PR reintroduces support for PRs as cards, including a new integration test.

Plan of Action

  • Write an integration test
  • Update enum to support Pull Requests
  • Test successfully passes

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #2185 into master will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #2185   +/-   ##
=======================================
  Coverage   65.88%   65.88%           
=======================================
  Files         546      546           
  Lines       14293    14293           
  Branches      838      838           
=======================================
  Hits         9417     9417           
  Misses       4718     4718           
  Partials      158      158           
Impacted Files Coverage Δ
Octokit/Models/Request/NewProjectCard.cs 27.27% <0.00%> (ø)

@boblangley
Copy link
Contributor Author

boblangley commented Apr 29, 2020

When the new integration test is run locally it fails on this line when attempting to retrieve the master branch from the newly created repository:

https://github.com/octokit/octokit.net/blob/master/Octokit.Tests.Integration/Helpers/RepositorySetupHelper.cs#L47

The error is:

Octokit.ApiException : Git Repository is empty.

In debugging I confirmed the repository is created.

When I run PullRequestsClientTests.CanCreate it passes, so I am convinced I have done something wrong writing this new test. Any ideas?

@boblangley boblangley force-pushed the pullrequest-project-cards branch from ad75b43 to ababe4f Compare May 12, 2020 21:31
@boblangley boblangley force-pushed the pullrequest-project-cards branch from ababe4f to 477b3ed Compare May 12, 2020 21:35
@boblangley
Copy link
Contributor Author

I figured out what I was doing wrong.

The tests for cards did not require auto-init, now they do.

I also had to update the NewProjectCard object to use a long instead of int for the ContentId property, since PullRequest Id's are a long and not an int like Issues.

Additionally, the helper method I was using was incorrect, since it only supported creating Issue cards, so another helper was introduced to keep the test styles consistent with the other tests.

I have squashed my commits and pushed.

@boblangley boblangley marked this pull request as ready for review May 12, 2020 21:36
@boblangley boblangley changed the title [WIP] Support pull requests as project cards Add support for creating project cards from pull requests May 12, 2020
@samhouts
Copy link
Contributor

@boblangley!!!!! I just came here to make this change!!!! ❤️❤️❤️❤️

@shiftkey shiftkey self-assigned this Jun 1, 2020
Copy link
Member

@shiftkey shiftkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice spot @boblangley - and thanks for adding that test to verify the behaviour works again!

@shiftkey shiftkey merged commit 1c026db into octokit:master Jun 7, 2020
@shiftkey
Copy link
Member

shiftkey commented Jun 7, 2020

release_notes: Add support for creating project cards from pull requests

@SzymonPobiega
Copy link

@boblangley @shiftkey there is still something off after this change. The ContentUrl of a card created from a PR seems to contain wrong URL -- with issues instead of pulls.

/cc @lailabougria @mauroservienti

@mauroservienti
Copy link

To add more context we do this:

//payload is a GitHub webhook json paylook
var prUniqueId = Convert.ToInt64(payload.pull_request.id);
var prCard = new NewProjectCard(prUniqueId, ProjectCardContentType.PullRequest);
var card = await client.Repository.Project.Card.Create(columnId, projectCard);

Once the card is created on the board is has the wrong ContentUrl property, we expect it to be https://api.github.com/repos/--org--/--repo--/pulls/--pr-number--, but instead, it is https://api.github.com/repos/--org--/--repo--/issues/--pr-number--. As @SzymonPobiega said it has issues instead of pulls.

@boblangley
Copy link
Contributor Author

The ContentUrl property returns the value returned by the GitHub API directly, with no manipulation:

image

I agree this isn't what I would intuitively expect from the API (the GitHub API has lots of weird decisions in it), it does make sense considering how they describe the relationship between PRs and Issues:

Note: GitHub's REST API v3 considers every pull request an issue, but not every issue is a pull request. For this reason, "Issues" endpoints may return both issues and pull requests in the response. You can identify pull requests by the pull_request key. Be aware that the id of a pull request returned from "Issues" endpoints will be an issue id. To find out the pull request id, use the "List pull requests" endpoint.

Since the ContentUrl reflects the URL to the API version of the content, it is in-line with how they would expect you to use the GET Issue API which handles both PRs and Issues (and is the source of the above quote).

@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed category: bug labels Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants