Add Draft PR support by kuychaco · Pull Request #1173 · microsoft/vscode-pull-request-github

kuychaco · 2019-05-23T01:18:09Z

On dotcom users can create draft pull requests to indicate that their PR is a work in progress and not yet ready for review.

With this PR, users may submit draft pull requests using the GitHub Pull Requests: Create Draft Pull Request command. When viewing the description of a draft PR in the editor, a Draft badge is displayed in the header.

This PR also includes upgrades of @octokit/rest and typescript. In order to use the Draft Pull Request rest API we needed to upgrade octokit, which required a typescript upgrade.

Fixes #1129
Fixes #1190

TODO:

Upgrade octokit and typescript
Create draft PR using GitHub Pull Requests: Create Draft Pull Request
Add Draft badge to PR description header
Indicate if PR is draft in PR list
"Ready for Review", disable "Merge pull request" button
~~Handle Unprocessable Entity error when clicking "Approve" and "Request changes" buttons while in draft mode~~
Style
Add telemetry
Tests

Co-authored-by: Ashi Krishnan <ashi@queerlyamorous.org>

Co-authored-by: Ashi Krishnan <ashi@queerlyamorous.org>clear

msftclas · 2019-05-23T01:18:31Z

All CLA requirements met.

kuychaco · 2019-05-24T01:19:07Z

Current UI/UX below...

Feedback is welcome!

queerviolet

I think we should have some UI for making the draft not a draft ("Ready for Review" on dotcom).

Also, when I try to Approve or Request Changes, it fails with Unprocessable Entity. Are those things we can do to Draft PRs? If not, we should remove the buttons.

preview-src/header.tsx

queerviolet · 2019-05-24T18:04:41Z

One UX thing and a small code nit, otherwise LG!

Co-authored-by: Ashi Krishnan <queerviolet@github.com>

... since REST API doesn't actually allow this. The documentation says it's supported, but turns out only in that it returns the `draft` field. The patch request doesn't take a draft param

This reverts commit d5e4fa5.

kuychaco · 2019-05-30T05:54:20Z

Also, when I try to Approve or Request Changes, it fails with Unprocessable Entity. Are those things we can do to Draft PRs? If not, we should remove the buttons.

Looks like this is due to the fact that a PR author is trying to approve/request changes on their own PR. Which is not allowed. I checked master and this is a pre-existing bug. So let's call this out-of-scope and tackle it separately

Due to lack of graphql support

Strangely the extension was working even without it listed explicitly. It was listed in the yarn.lock, and I guess that was getting us by... In any case, we started seeing this error with the stack trace pointing to lodash: ``` ```Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'nonce-i53qcjvFaMb4cikgwqc758pL4aWAtfGk'".``` ```

smashwilson

Random drive-by diff comments 👀

smashwilson · 2019-05-31T12:28:33Z

package.json

    "graphql": "^14.0.2",
    "iconv-lite": "0.4.23",
    "js-base64": "^2.4.9",
+    "lodash": "^4.17.11",


I'm actually depending on lodash.isequal in #1169. I'll have to consolidate after this is merged so it isn't redundant 🙈

smashwilson · 2019-05-31T12:29:08Z

src/github/pullRequestOverview.ts

 			<html lang="en">
 			<head>
 				<meta charset="UTF-8">
-				<meta http-equiv="Content-Security-Policy" content="default-src 'none'; img-src vscode-resource: https:; script-src 'unsafe-eval' 'nonce-${nonce}'; style-src vscode-resource: 'unsafe-inline' http: https: data:;">


Ohhh I bet this is the change that caused that EvalError you were seeing yesterday ☝️

It's a very good change to make, though.

Heh yup, and totally!

RMacfarlane

This is cool!

RMacfarlane · 2019-06-03T17:34:25Z

package.json

          "command": "pr.merge",
          "when": "config.git.enabled && github:inReviewMode"
        },
+        {


we should also scope pr.createDraft to be when you're not in review mode. Looks like something funky is happening with pr.create and that context key right now, the bang operator should work but I still see the command when checked out. Changing the syntax to github:inReviewMode == 'false' fixes it

Good to know about that funkiness! Made the fix 👍

preview-src/merge.tsx

RMacfarlane · 2019-06-03T17:37:50Z

src/github/credentials.ts

 			agent,
 			baseUrl,
-			headers: { 'user-agent': 'GitHub VSCode Pull Requests' }
+			headers: { 'user-agent': 'GitHub VSCode Pull Requests', Accept: 'application/vnd.github.shadow-cat-preview+json' }


just curious, why is this change needed?

Good question! It's required for access to the Draft PR API while it's in preview -- https://developer.github.com/v3/previews/#draft-pull-requests

I'll add a comment to the code to increase clarity 👍

RMacfarlane · 2019-06-03T17:39:57Z

src/github/interface.ts

 }

 export interface PullRequest {
+	id?: string;


should this be a required property?

That would certainly be ideal! However, as long as we fall back to using REST, this field must be optional. id refers specifically to the graphql object id and is required for making mutations (such as updating draft status).

And unfortunately the REST API doesn't currently support updating the draft status of PRs (and the GitHub pull requests team doesn't seem to have plans to add support). So for now users simply cannot convert from draft status to ready-for-review unless graphql is supported https://github.com/microsoft/vscode-pull-request-github/pull/1173/files#diff-4c377281588b3e01e164412d5a23724aR1229)

I feel like we dealt with this in some way—making this id either the GraphQL node id, or the database ID as returned by the REST API. @RMacfarlane do you recall?

(IMO this isn't a blocker, as going from an optional field to a required field is always non-breaking.)

Oh yeah! On comments we have an id property and a graphNodeId property. id is the database id, graphNodeId is the GraphQL node id. Both are always present, the REST API had a node_id property that we used to fill in the graphNodeId. I think we still have some mixed usage of REST and GraphQL when dealing with comments, even when GraphQL is supported

RMacfarlane · 2019-06-03T18:16:50Z

package.json

  },
  "dependencies": {
-    "@octokit/rest": "^15.15.1",
+    "@octokit/rest": "^16.21.0",


thanks for updating this!

src/github/pullRequestManager.ts

The bang operator doesn't seem to be working as expected, hence using the more verbose `github:inReviewMode == 'false'` syntax Co-authored-by: Rachel Macfarlane <ramacfar@microsoft.com>

Co-authored-by: Rachel Macfarlane <ramacfar@microsoft.com>

Looks like there's a bug in VS Code. Currently the pr.create and pr.createDraft commands always show up regardless of review mode. I'll go ahead and ship and hope that the bug gets fixed soon. Version: 1.35.0-insider Commit: 2737cde4facc8411982862742f5c6cd3dabef323 Date: 2019-05-31T05:16:11.646Z Electron: 3.1.8 Chrome: 66.0.3359.181 Node.js: 10.2.0 V8: 6.6.346.32 OS: Darwin x64 17.7.0

alif1993 · 2020-06-12T19:07:42Z

That's weird that I cannot find create draft pull request command!!!!
Any clue??

kuychaco and others added 7 commits May 22, 2019 15:40

Add pr.createDraft command

03b6be9

Co-authored-by: Ashi Krishnan <ashi@queerlyamorous.org>

Add accept header to access rest Draft Pull Request API

2a793fc

Co-authored-by: Ashi Krishnan <ashi@queerlyamorous.org>

Display "draft" status in PR overview header

7e978bc

Add property draft to PullRequest

cd17c59

Upgrade @octokit/rest to version that supports draft parameter

61f1c2b

Co-authored-by: Ashi Krishnan <ashi@queerlyamorous.org>

WIP?

46355eb

Upgrade typescript and octokit

bf3c972

Co-authored-by: Ashi Krishnan <ashi@queerlyamorous.org>clear

kuychaco added 4 commits May 23, 2019 16:13

👕

23a6c3a

Display Draft badge in PR description header

f1ce53c

Indicate draft status in PR list

334ef68

🔥 console.logs

9647ce2

kuychaco marked this pull request as ready for review May 24, 2019 01:01

kuychaco requested a review from queerviolet May 24, 2019 01:01

queerviolet suggested changes May 24, 2019

View reviewed changes

preview-src/header.tsx Outdated Show resolved Hide resolved

kuychaco and others added 8 commits May 28, 2019 15:17

Type should be lowercase

dcd3f99

Co-authored-by: Ashi Krishnan <queerviolet@github.com>

Update PR as ready to review using octokit

93abb55

Mark PR as ready for review using graphql

445f8e8

... since REST API doesn't actually allow this. The documentation says it's supported, but turns out only in that it returns the `draft` field. The patch request doesn't take a draft param

Disable "Ready for Review" button while mutation is in progress

52bbd06

Update PR to reflect non-draft status

7fa6f2e

Disable approve and request changes buttons for draft PR

d5e4fa5

Revert "Disable approve and request changes buttons for draft PR"

5ad0ec8

This reverts commit d5e4fa5.

Fix error message for when submitting review fails

b0790c7

kuychaco added 3 commits May 30, 2019 16:52

Style "ready for review" section

39efd81

Add pr.create and pr.createDraft telemetry events

02aa70f

Clean up

57127c2

kuychaco added 3 commits May 30, 2019 18:05

Fix typo

250fdca

Clarify when setting ready for review fails

07c1cf9

Due to lack of graphql support

kuychaco requested a review from queerviolet May 31, 2019 02:45

smashwilson reviewed May 31, 2019

View reviewed changes

RMacfarlane approved these changes Jun 3, 2019

View reviewed changes

kuychaco and others added 5 commits June 3, 2019 16:09

Scope PR creation to be when not in review mode

36a8d73

The bang operator doesn't seem to be working as expected, hence using the more verbose `github:inReviewMode == 'false'` syntax Co-authored-by: Rachel Macfarlane <ramacfar@microsoft.com>

Improve ternary format

7e992d9

Co-authored-by: Rachel Macfarlane <ramacfar@microsoft.com>

Clarify use of Draft PR API preview header

dc0b9dc

Co-authored-by: Rachel Macfarlane <ramacfar@microsoft.com>

Clarify why PullRequest id field is optional

ce33c65

Co-authored-by: Rachel Macfarlane <ramacfar@microsoft.com>

👕

1a6bb97

Co-authored-by: Rachel Macfarlane <ramacfar@microsoft.com>

queerviolet approved these changes Jun 4, 2019

View reviewed changes

kuychaco added 2 commits June 4, 2019 16:17

Add both graphNodeId and databaseId to PullRequestModel

c598e89

kuychaco force-pushed the kuychaco/draft-prs branch from ca80c6f to 14b067e Compare June 4, 2019 23:18

kuychaco merged commit df2a052 into master Jun 5, 2019

kuychaco deleted the kuychaco/draft-prs branch June 5, 2019 00:05

smashwilson mentioned this pull request Jun 5, 2019

Expand test coverage #1169

Merged

11 tasks

IllusionMH mentioned this pull request Jan 13, 2020

WIP / draft Pull Request should look different #1241

Closed

Conversation

kuychaco commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuychaco commented May 24, 2019

Uh oh!

queerviolet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

queerviolet commented May 24, 2019

Uh oh!

kuychaco commented May 30, 2019

Uh oh!

smashwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RMacfarlane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alif1993 commented Jun 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kuychaco commented May 23, 2019 •

edited

Loading

msftclas commented May 23, 2019 •

edited

Loading