Bespoke PR checklists

[roryabraham]

Problem

We have a PR author and reviewer checklists to help authors and reviewers to follow our best practices, promote high code quality and consistency, and to reduce the likelihood of introducing bugs.

However, filling out this checklist is time-consuming. Spending that time is worth it when it provides us with real value, but there are many items in the checklist that are not always relevant. (i.e: if you added a component..., if you added a new file, etc...)

The current guideline is to check off items that don’t apply to the current situation (there's even an item that says "I have checked off every item on this list, even the ones that aren't relevant), but that’s not ideal because:

• It's a waste of time
• Because there are so many items on the checklist that are often not relevant, we are conditioning ourselves to ignore it

As a result, it's a time-consuming process that often provides little value.

Solution

Let's review the items on the checklist and:

• Automated + remove the items that can be reliably automated
• Update the code that generates the checklists to look at the code changes in a given PR and only include the checklist items that are relevant

This diagram shows the process we can follow for each checklist item:

[melvin-bot[bot]]

Triggered auto assignment to @NicMendonca (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot[bot]]

Auto-assign attempt failed, all eligible assignees are OOO.

[roryabraham]

Just self-assigned this to HOLD it for now. I'll be passing this off 100% to SWM to implement.

[sebryu]

I'm from an expert agency Software Mansion I and would like to work on this issue. There is already started discussion on slack

[roryabraham]

@sebryu I don't think we need a design doc for this change, but please let's discuss in slack before diving into any code. The main thing I'd like to cover is how we can dynamically generate the PR checklist and reviewer checklist. Right now they're static markdown files.

I'd like to make sure that code is open-source, so I think using JavaScript GitHub Actions to generate the checklist makes sense. We can have two actions:

• generatePRChecklist that runs whenever a new PR is created, and it updates the PR description to include the checklist
• generateReviewerChecklist, that runs based on a comment trigger. So if you comment Reviewer Checklist in a PR, that would trigger the action to generate the reviewer checklist, which would then be left as a comment on the PR.

Also, we have a custom Chrome extension called K2 for GitHub (also called KSv2, which stands for KernelSchedulerV2). It is a productivity tool inspired by kernel schedulers and built in PHP. It does a lot, but one of the things that it does is that it gives you a button (example) that can be used to download the reviewer checklist and leave it as a comment:

This could be updated to just trigger the generateReviewerChecklist GitHub Action for the PR (which would mean it should also respond to the workflow_dispatch trigger).

[sebryu]

Hi Rory!

The main thing I'd like to cover is how we can dynamically generate the PR checklist and reviewer checklist

As you have stated we could use JS Github Actions, but I want to point out that pull_request_template must remain as a static file (github limitation, there is no way of making the PR template dynamic).

I think the best way to approach it would be keeping always relevant checks in this static pull_request_template.md, while keeping additional checks in a new file dynamic_pull_request_template.md (which would also be a static file, but we would fetch it and dynamically select only relevant parts from it) . Author of a PR will see information in the bottom of the template, that checks from dynamic_pull_request_template would be automatically added when submitting PR, and I'm not sure if that is good UX, but I don't see a better way to do that.

In Github Action (generatePRChecklist) when PR is created, we will detect from changed files which groups of checks should be added. i.e if we detect change in src/components/ we would add Checks for new component group. Particular groups of checks, and rules when they should be added can be discussed case by case, and we can start with single group at first (= take only one group of checks from existing pull_request_template.md and extract that into the new file).

[roryabraham]

If I'm following you, you're suggesting that:

• Always-relevant checks are kept in the existing PULL_REQUEST_TEMPLATE.md
• After the PR is created, we check the content of the PR, determine which additional checks are relevant, and then update the PR description to include those checks.

I think that sounds like a good plan 👍🏼 I believe it also further reinforces that every checkbox added after the PR was created is definitely considered relevant.

[sebryu]

Yes, that exactly what I'm proposing. I'll create PoC with that logic on private repo (easier to test Github actions) and I will keep you posted

[sebryu]

I'm still working on PoC. In the meantime can you confirm that:

• if there are some changes to PR (new file added) we should add relevant checks to the description - I think that's reasonable, but should we also post new comment by bot stating "We've detected that we should add new checks for this PR, please see updated description"
• reverse situation - if we detected that we should apply some checks for PR, but then author removed part of the code and checks are no longer valid - should we remove them (also adding bot comment stating what happened)?

[roryabraham]

@sebryu that all sounds good, except rather than just leaving a comment, I think a more robust, less noisy DX would be to just re-run the Author Checklist and Reviewer Checklist PR checks from the check suite for the PR any time the checklists are modified

[sebryu]

Other things to implement:

• if some checks were added, but then should be removed
• replace existing authorChecklist (move dynamicChecklist there)
• start with typescript check
• consider nested checklists
• review checklist
• for future phases - check for using eslint to detect some checks categories (e.g. if file contains function component)

[roryabraham]

@sebryu and I sat down today and put together a minimal POC: https://github.com/Expensify/App/pull/26397/files

There's still a number of things to do to get an initial PR ready to put dynamic checklists in place, but it should be doable sometime next week.

Also, since this feature does not affect the app itself it does not need to be subject to the merge freeze.

[roryabraham]

@sebryu is OOO taking care of some sick kiddos

[sebryu]

Hi @roryabraham. I've created new PR here: https://github.com/Expensify/App/pull/27532 (I couldn't push into your branch). From the previous POC I've changed:

• Checks that are no longer relevant would be removed
• migrated logic into authorChecklist action
• some refactoring and fixes

[roryabraham]

@sebryu is actively working through the POC

[sebryu]

Hey @roryabraham . My PR is open for review. Some matters open for discussion:

1. Currently I've migrated "If new component is created" to dynamic list by removing it from PULL_REQUEST_TEMPLATE and adding it into array in newComponentCategory.js (changed wording a little to get rid of extra nesting). However what we can do is to keep those checks in both PULL_REQUEST_TEMPLATE and newComponentCategory.js - if PR doesn't meet criteria it will remove those checks from PR description.
2. my code is checking only newly added files in a PR. If some component would be added to previously existing file GH action will not check it - I'm not sure if that's needed, but to get AST from a file I need whole file (not only modified part), and if I would scan each modified file for existence of a react component any small change to a file with react component would trigger new checkboxes.

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @sebryu, @NicMendonca, @roryabraham eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[roryabraham]

Moving this back to weekly. Left a big review on the PR. I'm excited to get this first POC landed, but even more excited to iterate and hopefully make our checklists more valuable

[roryabraham]

First POC merged and announced. @sebryu next step is to get a dynamic reviewer checklist as well. I'm realizing that probably should have been included in the V1, so let's try to move quickly towards "checklist parity".

I think the easiest way to implement this will actually be to just include the reviewer checklist collapsed under a <details> tag in the PR description. We'll need to update both the authorChecklist job to ignore the empty reviewer checklist in the PR description, and also to update the reviewerChecklist job to generate and check for dynamic checks as well.

I'll also need to update our K2 chrome extension accordingly very soon after the changes to the reviewer checklist are made

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.99-0 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/27532

If no regressions arise, payment will be issued on 2023-11-22. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [ ] External issue reporter
• [ ] Contributor that fixed the issue
• [ ] Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @sebryu does not require payment (Contractor)

[melvin-bot[bot]]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@sebryu] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [ ] [@NicMendonca] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[NicMendonca]

No payment needed for this one!

[roryabraham]

I'm going to reopen this, because while we have a POC this project isn't really done. Minimally we need dynamic checklists for reviewer checklists as well as author ones for the POC to be considered complete. plus there's a lot more items that could be automated or made dynamic which @sebryu and I went over in person when I was in Poland back in late August / early September.

[roryabraham]

I'll be reviewing these myself w/o C+ input though, and @sebryu works for SWM, so we don't need a BZ assignee here. Thanks @NicMendonca!

[roryabraham]

We have a partially complete POC here, but at this point I'm going to close this issue to focus on more urgent roadmap priorities.