Mattermost Logo
Code Review
Getting Started Server Web App Mobile Apps Redux Desktop App Mattermost Dev Talks
Mattermost Contributors
MVP
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 (weekdays, Monday through Friday, excluding statutory holidays).
    • Keep in mind that core committers are geographically distributed around the world and likely in a different time zone than your own.
    • If no interaction has occurred after 5 business days, please ping a core team member who is participating in the PR. If there are no participating members yet, please ping Jason Blais or Ben Schumacher (Hanzei).
  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 the 2: QA Review label and add the QA Review Done label 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 added the QA Review Done 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. For handling cherry-picks, please check here.

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 added the QA Review Done 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.
    • There is an automated cherry-pick process and the author of the pull request should make sure the cherry-pick succeeds. Assume this is the case unless you are explicitly asked to help cherry-pick.