Code Reviews

Mandatory GitHub code reviews of Pull Requests ensure high-quality code, adherence to best practices, and consistency. They help identify issues, foster feedback, and enhance code quality, ultimately streamlining development and maintaining robust solutions. Email notifications from GitHub notify the person submitting the code review of any review activity.

Code reviews are mandatory. For in-depth detail, refer to this general GitHub wiki.

Original Presentation

How It Works

Have a GitHub Pull Request (PR)?

Post the PR’s URL in the #code-reviews Slack channel with a concise mention of what it is, how long you think it will take, and tagging the relevant group to look at the relative code you need feedback on:

@sass-feedback for CSS, SASS.
@php-feedback for PHP.
@js-feedback for JavaScript, jQuery.
@plugin-feedback in #webteam for Plugin Feedback.
@framework-feedback in #id-product-development for Responsive Framework feedback.

Once your PR is approved with minimal feedback, you can safely merge the PR into the develop branch.

Taking on a code review?

  1. Click through [in browser] to the PR and assign yourself as a Reviewer.
  2. Select the Files Changed tab. Filter out [top left of UI] any files that don’t pertain to the code language you’re reviewing.
  3. When done adding comments, click the green Review Changes button and select either the Comment, Approve, or Request Changes option (including any feedback).
  4. When complete: click Submit Review.
  5. [Back in the #code-reviews Slack channel] Locate and apply a reaction sticker with the relevant language you reviewed:

:sass: for CSS, SCSS code reviewed.
:php: for PHP code reviewed.
:js: for JavaScript, jQuery code reviewed.

What if I reviewed everything in the PR?

If approved (with or without minor feedback), then react to the Slack message with a :done: sticker.

Do not include :done: if you Requested Changes. The person submitting the PR will make necessary adjustments or clarify, at which point they will bump their PR with adjustments or ping the reviewer.

Need to bump a PR up because no one has taken it on?

Copy a link to the original message and post it again.

Abbreviated PR Setup

For smaller projects, bug fixes, or enhancements, we should use the following abbreviated process:

  1. Be sure that you are working in a specific repository branch ( ex: bug/this-is-the-bug-fix ) that will address the work being done
  2. Commit your changes to this branch, whether they be stylist or logic
  3. Create a Pull Request to merge your working branch into the “develop” branch or the related repository
  4. Assign yourself as the owner of the Pull Request, and assign other team members as reviewers
  5. Send a message to those you have assigned as reviewers AND / OR add a message in the “code-reviews” Slack channel, and flag one or more of the following: @sass-feedback, @js-feedback, or @php-feedback to help better target the audience that is notified

Create a Pull Request

  1. Push Your Branch
    • Commit your code changes to your local branch.
    • Push the branch to the remote repository using Git or a tool like Tower.
  2. Create a Pull Request (PR)
    • Navigate to the project’s repository on GitHub.
    • Click Create Pull Request from your branch.
    • Fill out the PR form with a clear title and concise description of your changes. See “PR Example” section below.
    • If the PR form doesn’t auto-generate content, manually add the required details.
    • Click Create Pull Request to submit for review.

Best Practices for PRs

  • Be Clear: Keep your PR title and description straightforward, summarizing what was changed and why.
  • Reference Issues: Link any related issues (e.g., close #11).
  • Tag Reviewers: Mention relevant team members to streamline the review process.

Review Phases

Large Projects

  • Review 1:
    Cursory check during the implementation phase to confirm it’s on track.
  • Review 2:
    Pre-launch review to evaluate the final Quality Assurance (QA) aspects.

Smaller Projects, Post-Launch/Follow-Up

For smaller projects, bug fixes, or enhancements:

  1. Work in a specific repository branch (e.g., bug/this-is-the-bug-fix) that addresses the task.
  2. Commit your changes to the branch, whether they involve styles or functionality.
  3. Create a Pull Request to merge your working branch into the develop branch or related repository.
  4. Assign yourself as the owner and designate team members as reviewers.
  5. Notify reviewers via Slack in the #code-reviews channel, tagging relevant groups like @sass-feedback, @js-feedback, or @php-feedback.

    Code Review Guidelines

    Depending on the code in the PR, ensure you have the appropriate person reviewing when you place the request in the #code-reviews Slack channel:

    • SASS/CSS/Markup: should be reviewed by a Designer.
    • PHP/jQuery/JS/Markup: should be reviewed by a Developer.

    If You’re a Reviewer:

    • Use reaction stickers to mark the type of code you’re reviewing (e.g., SASS, JS). Do this the moment you pick up a PR for review.

    Responding to Feedback

    • If you receive code feedback, make the necessary optimizations.
    • Commit your changes to the pull request branch and push the updated code.

    PR Example

    ## Description

    Describe what’s been coded. Provide as much detail as necessary.

    ### Mockup[if applicable]/Staging Link(s)

    – [Mockup or Staging Link](#url)

    ### Sandbox Links You can see my changes in the links below:

    – [ ] [Sandbox Link](#url)

    —

    ### Browser support

    I have tested my changes in the following browsers:<br/>

    <sub><sup>*This is rare: If your project requires browser testing beyond the latest two versions, such as IE9, please note this below.*</sup></sub>

    – [ ] Chrome

    – [ ] Firefox

    – [ ] Safari

    – [ ] Edge

    ### Pre-review checklist for designers and developers

    – [ ] I have tested my changes in my sandbox, and they are working correctly.

    – [ ] I have tested my changes in the browsers required for my project, and they are working correctly.

    – [ ] To the best of my knowledge, this code is stable and ready to be deployed to production or staging at any time.

    – [ ] My code follows [BU Coding Standards](https://github.com/bu-ist/coding-standards).

    —

    ## Information for reviewers

    – [ActiveCollab project (track time here)](#url-to-active-collab-project-code-reviews-tab)

    – [ ] This work is non-billable<br/>

    <sub><sup>*Work is only non-billable if it is a bug fix that occurs after the site has launched, or if the project has an unusual billing arrangement.*</sup></sub>