As a user, I want reminders and metrics to ignore pull requests in "Draft" status

abinoda commented 5 years ago

GitHub released a new feature for "Draft" pull requests. This is a great standardization of WIP.

[x] Exclude Draft pull requests in reminders by default
[x] Exclude Draft pull requests in "PRs" list as well as "Open PRs" analytics page (but make this an option)

A "nice to have" if GitHub can provide a ready_for_review_at timestamp would be to make "Merge time" analytics based on when a pull request was ready for review versus when it was opened

abinoda commented 5 years ago

@lukehefson

I've run into an API issue while adding support for Draft pull requests in Pull Reminders. It appears that pull_request webhook events are not triggered when pull requests are changed from "Draft" to "Ready for Review". As an API consumer, this means it is impossible to keep "Draft" state accurately synced in my GitHub application.

Any chance the pull_request/edited webhook can be made to fire when the draft state changes?

P.S. Thank you to you and the rest of the team for the quick turnaround on API support as well as being so responsive on Twitter last week! I really appreciate it ❤️ Also, kudos on a great feature.

lukehefson commented 5 years ago

Hey @abinoda 👋

P.S. Thank you to you and the rest of the team for the quick turnaround on API support as well as being so responsive on Twitter last week! I really appreciate it ❤️ Also, kudos on a great feature.

You're very welcome. Thanks for being an amazing partner!!

Any chance the pull_request/edited webhook can be made to fire when the draft state changes?

I'll take this to the team for investigation and we'll dig in!

abinoda commented 5 years ago

@lukehefson I previously requested having the pull_request/edited webhook fire when draft status is changed. However, I actually think it'd be extremely beneficial for draft status to be its own event action type, ie. pull_request/draft_status_changed. I'll describe a simple use case below, but I think this type of scenario is and will be relevant to many other Apps and GitHub Actions.

tldr; There needs to be a straightforward way for an App or Action to know when a pull request has gone from draft to non-draft.

Use case:

I want to send a Slack notification when a pull request is opened AND ready for review, and therefore actionable to users.

In order to do this I'd watch for:

pull_request/opened events where draft=false, and
pull_request/draft_status_changed events

It'd be difficult and much less reliable to do this without a pull_request/draft_status_changed event since I'd need to "detect" the change in draft state by checking every pull_request/edited event and comparing it with cached data.

abinoda commented 5 years ago

@lukehefson Just a heads up (in case it is helpful) that this issue affected a customer today.

@ivanaveliskova wrote in:

Github just recently release a draft PRs feature which allows you to open a "Work in Progress" pr to keep from people reviewing it while you are still making updates. I recently used this feature and then took my PR off of being a draft PR but the notification for the reminder in our slack channel, that we set up to remind people what PRs are currently open, didn't seem to show up.

This issue occurred because there's no webhook that fires when the PR is turned into a non-draft.

lukehefson commented 5 years ago

Thanks @abinoda (and sorry @ivanaveliskova) – having that use case is really helpful for us!

A "nice to have" if GitHub can provide a ready_for_review_at timestamp would be to make "Merge time" analytics based on when a pull request was ready for review versus when it was opened

Talking to the dev team, the timestamp should be available via the API by fetching the timeline for the PR. Is that enough for you to achieve what you'd like?

abinoda commented 5 years ago

Talking to the dev team, the timestamp should be available via the API by fetching the timeline for the PR. Is that enough for you to achieve what you'd like?

@lukehefson I actually hadn't looked deeply into this piece yet because fixing the webhook syncing issue has been a higher priority.

I'll answer your question in two parts, (1) my general opinion on why ready_for_review_at should be added as a timestamp, and (2) the viability of your suggestion within the context of the webhook stuff we've been discussing previously.

In both cases, keep in mind that I'd like to have ready_for_review_at synced in real-time via webhooks, so only leveraging the event timeline API endpoint isn't viable. I'm not sure if that's what you were implying, though...

Generally speaking...

In the GitHub API, it seems like the philosophy has been to include important timestamps as properties like pull request created_at, closed_at, merged_at, or the pushed_at property for repos.

All of these timestamps could be inferred from the event timeline but as an API consumer it is significantly easier and more reliable to receive them as properties. Parsing the event timeline to determine timestamps is a tedious and expensive process (I do this now for certain things). Relying only on the event timeline also doesn't solve the problem of real-time syncing, which could be solved with the webhook event we're discussing separately and I'll talk about again in the next section.

It seems like the new Draft feature and associated "ready for review" event is core to the pull request workflow and important enough to deserve a property similar to the other timestamp properties I've referenced above.

Specifically in this case...

If you guys implement an independently identifiable event action, ie. pull_request/draft_status_changed versus firing pull_request/edited on draft status change, then it's pretty easy for me to log the timestamp from the draft_status_change event in order to capture the ready_for_review_at timestamp in real-time as it occurs.

Assuming the same draft_status_changed event would also show up in the event timeline, I could use this as a fallback syncing mechanism for when webhooks fail.

So to answer your original question:

Talking to the dev team, the timestamp should be available via the API by fetching the timeline for the PR. Is that enough for you to achieve what you'd like?

Yes, this would work for me, assuming that you guys are planning on adding a webhook and API event like pull_request/draft_status_changed. But all things equal, having a ready_for_review_at property in the pull request object would be nice 😄

abinoda commented 5 years ago

@lukehefson Hi Luke, was curious to know if the team was working on a solution to the webhooks issue? I've had several more customer incidents with out-of-sync draft status, similar to the one reported earlier in this thread.

lukehefson commented 5 years ago

Hey @abinoda and sorry to hear that your customers are having issues. It's in the team's backlog, so we're actively planning to get to it, but unfortunately I can't give you an ETA at this time. Hopefully very soon!

abinoda commented 5 years ago

@lukehefson Thanks for the help with this.

Just integrated with the new ready_for_review webhook which resolves the issue here 👏

pullreminders / backlog