Skip to content

Tech Review

What is the goal of Tech Review?

  • Spot errors/bugs.
  • Check the logic/implementation.
  • Check if the solution can be done in a better way.
  • Ensure a consistent code style.
  • Give the developers new opportunities to learn.
  • Run the code and test that the changes really work.
  • Make sure to understand where the changes are used and what effects they can have on code that is not in the PR.
  • If you are not sure why a change was done, ask the developer.
  • Check that migrations are backward compatible, so the old version of the code still works while deploying — see DB Migration Strategy.

How do we tech review?

Process

  1. The Ticket Owner (= Developer working on the ticket) moves the ticket to "Tech Review" and opens a Pull Request (PR).
  2. The Ticket Owner asks for a code review from one of their fellow Developers (in the chat — group or private — or during the Daily).
  3. The Reviewer (= Developer doing the Tech Review) checks the PR and ensures the code quality (see "What is the goal of Tech Review?" above).

N.B.: other optional Reviewers can be added to the PR if so wished.

Pull Request Principles

  • If the Reviewer and the Ticket Owner are not able to reach a consensus on a PR comment, Florian Gilde will come in as Moderator. The Moderator will hear the functional arguments on both sides, and make the decision.
  • If there isn't any argument or official standard to be able to make the decision on a technical basis, then it is a matter of taste and the Ticket Owner shall have the last word.
  • If the Tech Review, PO Review or Testing highlights a bug/problem that must be fixed, the PR shall stay open and not be merged; instead, the ticket shall be moved to "Reopened" and worked on again.
  • If an extra feature change/addition comes up but is outside the scope of the ticket, it shouldn't be added to the PR; a new ticket should be written to work on it later.
  • No refactoring shall happen anymore once the Tech Review is started (= when the Ticket Owner decides that the ticket has entered its code review phase and has moved the ticket to "Tech Review"). If more work is needed, discuss with the Reviewer and put the PR back to draft. If needed, wait until the Reviewer has completed their review before pushing any changes.
  • Each PR shall be a functional piece of software that can be executed on its own.
  • Before Tech Review:
    • PO Review is done.
    • The PR shall have completed all the automated checks besides the Reviewer approval (e.g. CI, linked ticket, etc.).
  • No comment shall be left uncommented (so we can make sure that they weren't resolved on purpose).
  • No comment shall stay unresolved, and the Reviewer shall resolve their comments, not the Ticket Owner.
  • If a comment has been discussed externally (e.g. via a call), the result shall be summed up as an answer to the comment.
  • A small call is better than a long comment/thread.

How this is enforced in this repo

  • The PR description carries the Owner checklist (.github/pull_request_template.md). Until its PO Review point is ticked the PR wears the Needs PO-Review label; Build & Test only run once every Owner point is ticked.
  • Once the Owner side is complete, the bot posts the Reviewer checklist as a comment. The Tech review checklist required check blocks the merge until both sides are fully ticked.
  • PRs that touch migrations additionally get the migration checklist comment — see DB Migration Strategy and Development Workflow.