Notifications for PR review

[dhruvkb]

Given the pace of the project, it's easy to forget to review PRs. We've all noticed that PRs opened all week always get reviewed once they're called out in the next meeting, which isn't ideal because it slows down work and blocks progress.

The two-pronged approach to improving the system is

• tagging PRs with "priority" labels
  • each levels of priority is assigned a time period in which the review is expected
• notifying reviewers after a time period (decided based on the priority label)
  • preferably in the DMs or by tagging them in a comment on said PR

This approach ensures that our PRs get timely reviews. This discussion aims to reach a good agreement on the following aspects of the project.

• levels of priority and meaning of each level
• ETAs associated with each level
• number of notifications
• fraction of ETA at which to send notifications
• medium of notification
• implementation of the notifications
• worst case scenario / last resort

Details on these are listed below in specific comments. Please reply with your ideas, criticisms of submitted ideas and general up or down votes.

[dhruvkb]

Levels of priority and meaning of each level

Currently, this is the 4-level priority system we have.

Label	Meaning	Color
🟥 priority: critical	Must be fixed ASAP	#b60205
🟧 priority: high	Stalls work on the project or its dependents	#ff9f1c
🟨 priority: medium	Not blocking but should be fixed soon	#ffcc00
🟩 priority: low	Low priority and doesn't need to be rushed	#cfda2c

I feel very good about this existing system but we can definitely expand the meanings of each label to make it clearer when each should be applied and how much urgency is expected from a reviewer when a PR has a particular level.

[dhruvkb]

ETAs associated with each level

This is referring to the time in which a PR with a particular priority should definitely get merged. Any PR reviews requesting changes are expected to be submitted well within the time so adequate time is given for changes to be implemented.

[dhruvkb]

This is what I was thinking. Assuming critical to be the priority where something is blocked in prod, or something blocks the deploy, it would need to be fixed immediately with a 1d wiggle room to compensate for the TZ spread of the team. Similarly low, which is neither urgent nor very important can easily be pushed for up to two weeks (after which it can get a little sad for the author; and introduce merge conflicts, which is also saddening).

Label	ETA
🟥 priority: critical	1d
🟧 priority: high	3-5d
🟨 priority: medium	7d
🟩 priority: low	14d

[sarayourfriend]

Given the current definition of "low" and "medium" I think these don't make sense.

Most PRs will fall under medium and low unless we expand the definition of "blocking" to include like "this is part of a stream of work" or something, in which case almost every PR will be critical or high (despite not actually having the same "urgency" as something like a production bug fix for example)

I personally think that a week for any PR that is ready for review is far too long to without at least some kind of attention. If I was a new contributor especially, I would be (and have been) especially discouraged if my PR got labelled "low priority" and then ignored for 2 weeks.

Depending on the area a PR is targeting 2 weeks would introduce a lot of merge conflicts, which you mentioned, but I think is more likely to happen and more frustrating than we give credence to.

I guess I don't totally understand why we need these wide spread ETAs? 5 days to wait for any PR review for a small (though intricate) project with a dedicated team of 5/6+ professional developers who are spending full time hours dedicated to that project seems... weird? I've never experienced waiting for 5 days for a review, much less two weeks at any job.

I'm not sure what the thresholds should be exactly for each priority but to me 5 work days (taking into account the weekend when a PR is open on any day but Monday) would be the maximum. If I had to propose a priority structure it would be something like this:

Label	ETA/Review urgency
critical	1d
high	1d (but review it after critical PRs and if you run out of time put this at the top of your list for the next day)
medium	5d (but try to review it as soon as you can and definitely before low priority PRs)
low	5d (but review these last unless they are at the fifth day, in which case delay other work to review any PRs that have reached threshold including medium and low)

[zackkrida]

Here's how I've been using the labels. Not saying this is good, just how they seem to me in practice:

Label	Description
🟥 priority: critical	Bug affecting production that warrants a hotfix and deployment
🟧 priority: high	Work that blocks other PRs from being started, or is related to a hard deadline
🟨 priority: medium	Average work that should be part of the current or next sprint
🟩 priority: low	Work that'd be okay if it was never done

I still think 72hrs is a quite reasonable low-end for PR reviews. 5+ days seems way too far out for me, given our staffing.

[krysal]

We still have to divide the developers' team between four or five project repos, which together are not small, having to get context, write documentation, maintain automation, etc., by PRs. In my mind, these are the max days expected for a PR to be reviewed by priorities:

Label	Max days to get a review
🟥 priority: critical	1d
🟧 priority: high	3d
🟨 priority: medium	5d
🟩 priority: low	7d or anytime

Reading again, seems that the confusion is in what we are estimating for, @dhruvkb said:

time in which a PR with a particular priority should definitely get merged

I was thinking about these estimates on the time the second notification would be sent (the first one is the native from GitHub) and I think @sarayourfriend probably thought the same.

[zackkrida]

We should revisit these timings, @sarayourfriend's suggestions are the most reasonable and a good 'compromise' between other proposed solutions.

I don't think the goal here is to improve merge time, necessarily, but to reduce developer feedback time. I think this will organically improve merge time but it's not a goal on it's own. This is all to say, I think these times should refer to how quickly a PR is reviewed, not merged.

[dhruvkb]

Number of notifications

How many notifications for PR review to send to any one reviewer?

[dhruvkb]

I'd recommend 2 notifications as a personal preference, just in case I miss the first one, or am caught up in some other task.

[sarayourfriend]

1 Notification please! These will already be duplicating GitHub notifications. Individuals should be able to manage notifications so they don't get lost (mistakes happen of course!). Worst case someone uses slack or another tool to ad-hoc configure a reminder to poke them later.

[AetherUnbound]

+1 to one notification 🙂 I try to stay on top of things using GitHub's notifications, so a single separate notification through a secondary channel would be sufficient for me.

[krysal]

Same, 1 notification seems appropriate in my view.

[dhruvkb]

Fraction of ETA at which to send notifications

Considering an open PR with a certain ETA for merge, at what fraction of the ETA should notifications be sent? It's easier to talk in fractions of the ETA because these ETAs will vary with priority.

[dhruvkb]

Since I preferred 2 notifications, the fractions I prefer would be at 0 (sent by GitHub in the notifications itself), 0.5 (a lenient fyi-ish one) and 0.75 (a slightly pressing icymi-one).

[AetherUnbound]

Based on my preference of 1 notification, I think a 0.75 ICYMI one would be best.

[dhruvkb]

Medium of notification

The mechanism for sending notifications. Currently I can think of two approaches to this:

• Slack DM: This is a bit tricky to pull off but there's hardly anything more targeted than a Slack DM. They're hard to miss, easy to keep unread till you can act on them and you can even set reminders in Slack to attend to these reminders (reminderception).
• GitHub tags: Mentioning someone in GitHub comments is also pretty effective as it sends a GitHub notification to the recipient and also adds activity to the PR bringing it to the top and to the attention of other maintainers who might want to jump in.

[dhruvkb]

My vote goes for Slack messages although it is a bit harder to implement. I'm way more likely to read and respond to Slack messages (that also come with push notifications) as compared to my GitHub notifications that are always in a state of overflow.

[sarayourfriend]

GitHub has a few distinct advantages:

1. Not hidden from folks watching or interested in this PR
2. We could include dedicated community members in the future

[Slack] also come[s] with push notifications

If folks like to receive work notifications on their personal devices, GitHub has a smartphone app for iOS and Android that will send you push notifications for review requests. You can also receive email notifications as well which are super powered in that you can interact with them straight from your email inbox without having to sign into GitHub.

[AetherUnbound]

Sara brings up a good point about dedicated community members. I am personally partial to a Slack DM though. I try to check my GitHub notifications regularly, but Slack is something that I'm monitoring constantly. I'm more likely to see and immediately address a Slack message than I am for a GitHub notification.

[sarayourfriend]

Just to note: if we use Slack we would need to use the WordPress make slack, so we'll have to coordinate with folks in the #meta channel to ensure this is even possible before exploring it further.

[zackkrida]

My preference is GitHub, even if it's ultimately used as a stepping-stone towards Slack notifications.

Regarding Slack, it sounds really difficult to map all contributors to GitHub users (I already know for a fact that some of our contributors have non-matching Slack and GitHub names and like we'd have to manually update that list every so often.

A github bot would just need to be able to read all Openverse PRs and comment on the PR the handle of each reviewer who hasn't reviewed in X days.

[dhruvkb]

Implementation of the notifications

How will this notification system work? Where will it be hosted? How will it be scheduled and triggered? All these (and more)
questions need to be answered to determine the system's implementation.

Are these tools, plugins or services that can do this for us? We do not want to overengineer it, keeping in mind that the end-goal is to build Openverse and not a PR notification system.

[dhruvkb]

GitHub actions does not support scheduling tasks at a given interval after an event, it's either immediately in response to an event or at fixed intervals/times. Running a workflow when the review fraction set for an a PR has elapsed would not be supported natively.

[zackkrida]

I think this would need to be a daily cron action, that lists all open Openverse PRs, then uses the https://docs.github.com/en/rest/reference/pulls#reviews endpoint to get the status of reviews on each one, then comments the notifications in Slack.

[dhruvkb]

Worst case scenario / last resort

What happens if PRs are not reviewed in time? This can vary by level as I assume PRs with critical priority would have short ETAs and need merging even without a review and PRs with low priority wouldn't have an ETA in the first place.

[dhruvkb]

We don't want PRs to be merged without a review so how about asking in a Slack channel requesting for a review (while checking the "Also send to ...") so that someone else might do so?

For an urgent bug fix or something blocking deployment, it might be fair to merge and also leave a message inviting reviews that can be addressed and improved in a future iteration.

[sarayourfriend]

If PRs aren't reviewed in time then I would say ping the openverse-developers alias to cast a wider net. Whomever notices that ping should prioritize reviewing that PR above anything else.

[zackkrida]

Some documentation about the built-in notification features in GitHub for teams and individuals:

• https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-github-user-account/managing-your-membership-in-organizations/managing-your-scheduled-reminders
• https://docs.github.com/en/organizations/organizing-members-into-teams/managing-scheduled-reminders-for-your-team

[zackkrida]

The WordPress GitHub organization doesn't allow users to connect Slack to a WordPress instance, so without that permission this isn't yet possible. If anyone has any other orgs they belong to whose repos they'd like to see in a slack instance, you can look into setting this up here:

https://github.com/settings/reminders

[AetherUnbound]

This discussion has been converted into an issue, and is being locked: #158

[zackkrida]

Details on the solution can be found here:
#158 (comment)