Mattermost Logo
Edit on GitHub

Code Review

All changes to the product must be reviewed.

If you are a community member seeking a review 

  1. Submit your pull request.
  2. Wait for a reviewer to be assigned.
    • Product managers are on the lookout for new pull requests, and usually handle this for you automatically.
    • If you have been working alongside a core committer, feel free to ping them directly.
    • When in doubt, ask for help in the Developers channel on our community server.
  3. Wait for a review.
    • The ticket author, a core committer, or a product manager will assign reviewers and label your pull request appropriately.
    • Expect some interaction with at least one reviewer within 5 business days.
    • Keep in mind that core committers are geographically distributed around the world and likely in a different time zone than your own.
    • Please ping Jason Blais or Hanzei after 5 business days if no interaction has occurred.
  4. Make any necessary changes.
    • If a reviewer requests changes, your pull request will disappear from their queue of reviews.
    • Once you’ve addressed the concerns, reach out with another mention.
  5. Wait for your code to be merged.
    • Larger pull requests may require more time to review.
    • Once all reviewers have approved your changes, they will handle merging your code.

If you are a core committer seeking a review 

  1. Submit your pull request.
  2. Assign a product manager to your review and label your pull request with 1: PM Review.
    • If your changes do not affect the user experience, you may skip this step.
    • Product managers ensure the changes meet user experience guidelines.
    • Wait for their review before continuing so as to avoid churn if changes are requested.
    • Note that product managers may assign core committers after completing their own review.
    • The PM should remove 1: PM Review when their review is done.
  3. Assign two core committers to your review and label your pull request with 2: Dev Review.
    • When picking your first core committer, consider someone with domain expertise relative to your changes. Sometimes GitHub will recommend a recent editor of the code, but often you must rely on your own intuition from past interactions.
    • When picking your second core committer, consider someone unrelated to your changes. A fresh and unbiased set of eyes can be invaluable, and exposing the team to new parts of the code helps spread out domain knowledge.
    • Don’t be afraid to pick someone who gives “hard” reviews. Code review feedback is never a personal attack: it should “sharpen” the skills of both the author and the reviewers, not to mention improving the quality of the product.
    • If you are Mattermost staff, try to take into account the timeoff calendar.
    • Try to avoid assigning the same person to all of your reviews unless they are related.
    • When in doubt, ask for recommendations on our community server.
  4. Assign a QA tester and label your pull request with 2: QA Review.
    • It is the PM or QA tester’s responsibility to determine the scope of required testing, if any.
    • Reviews by QA may occur at the same time as review by core committers. Be sure to ask for a second review as needed when changes are made.
    • The QA tester should remove 2: QA Review when their review is done, or if a review is deemed not necessary.
  5. Apply additional labels as necessary:
    • CherryPick/Approved: Apply this if the pull request is meant for a quality or patch release.
    • Do Not Merge/Awaiting PR: Apply this if the pull request depends on another (e.g. server changes)
    • Setup Test Server: Apply this to create a test server with your changes for review.
    • See labels for more details.
  6. Assign a milestone as necessary.
    • Most issues are targeted for an upcoming release, and should be assigned the corresponding milestone.
    • The milestone is mandatory for bug fixes that must be cherry-picked.
  7. Wait for a review.
    • Expect some interaction with each of your reviewers within 2 business days.
    • There is no need to explicitly mention them on the pull request or to explicitly reach out on our community server.
    • Core committers and QA testers are expected to have the GitHub plugin installed to automate notifications and to trigger a daily review of their outstanding requested reviews.
  8. Make any necessary changes.
    • If a reviewer requests changes, your pull request will disappear from their queue of reviews.
    • Once you’ve addressed the concerns, assign them as a reviewer again to put your pull request back in their queue.
  9. Merge the pull request.
    • Do not merge until the reviewing product manager has approved the changes and removed the 1: PM Review label.
    • Do not merge until the reviewing QA tester has approved the changes and removed the 2: QA Review label.
    • Do not merge until at least two core committers have approved and all concerns have been addressed.
    • Remove any remaining 2: Dev Review label and assign the 3: Reviews Complete label.
    • Merge your pull request and delete the branch if not from a fork.
    • Note that the last core committer to approve your changes may do this on your behalf.
    • If your pull request depends on other pull requests, consider assigning the Do Not Merge/Awaiting PR label.
  10. Handle any cherry-picks.
    • Cherry-pick the commit from master to the affected releases.
    • Push your changes directly to the remote branch.
    • No new pull request is required unless there are substantial merge conflicts.
    • Remove the CherryPick/Approved label and apply the CherryPick/Done label.

If you are a core committer asked to give a review 

  1. Respond promptly to requested reviews.
    • Assume the requested review is urgent and blocking unless explicitly stated otherwise.
    • Try to interact with the author within 2 business days.
    • Configure the GitHub plugin to automate notifications.
    • Review your outstanding requested reviews daily to avoid blocking authors.
    • Prioritize earlier milestones when reviewing to help with the release process.
    • Responding quickly doesn’t necessarily mean reviewing quickly! Just don’t leave the author hanging.
  2. Feel free to clarify expectations with the author.
    • If the code is experimental, they may need only a cursory glance and thumbs up to proceed with productizing their changes.
    • If the review is large or complex, additional time may be required to complete your review. Be upfront with the author.
    • If you are not comfortable reviewing the code, avoid “rubber stamping” the review. Be honest with the author and ask them to consider another core committer.
  3. Never rush a review.
    • Take the time necessary to review the code thoroughly.
    • Don’t be afraid to ask for changes repeatedly until all concerns are addressed.
    • Feel free to challenge assumptions and timelines. Rushing a change into a patch release may cause more harm than good.
  4. Merge the pull request.
    • Do not merge until the reviewing product manager has approved the changes and removed the 1: PM Review label.
    • Do not merge until the reviewing QA tester has approved the changes and removed the 2: QA Review label.
    • Do not merge until at least two core committers have approved and all concerns have been addressed.
    • Remove any remaining 2: Dev Review label and assign the 3: Reviews Complete label.
    • Merge the pull request, and delete the branch if not from a fork.
    • Some changes are spread out across multiple pull requests that should be merged at the same time. Look out for the Do Not Merge/Awaiting PR label. When in doubt, leave the merging of the pull request to the author.
  5. Handle any cherry-picks.
    • Typically, the author of the pull request should handle cherry-picks. Assume this is the case unless you are explicitly asked to help cherry-pick.