Skip to content

Code review process

The code review process from the reviewer's side. The author's side is in Contributing.

Code review best practices

Before conducting code reviews please read this article about code review best practices.

Essential practices for merge requests (MRs)

  • Assign the MR to a reviewer pool (merge_request_pool) rather than to individual reviewers, so any available, qualified reviewer can pick it up.
  • The pipeline must pass. MRs with a failed pipeline are not merged. If it fails for a reason you cannot fix, ask technical support or your guild.
  • A Draft MR is for early feedback only. It is not merged until it is marked ready and meets all other criteria.

Reviewer guidelines

  • The goal is to raise the quality of the codebase, so be thorough.
  • Check if the code match our code style guidelines for either FE or BE.
  • Be constructive towards the author.
  • Ask questions if you are not sure about something.
  • If possible, do the code review with the developer who created the MR.
  • Spend one hour a day doing code review.
  • If MR is assigned to you, you are responsible for doing the code review.

Reviews of BE and FE code

In case you are BE developer you should not review FE code and vice versa. FE developers should tag their MRs using tags like vue or javascript, depending on the case. BE developers tag their code with tags such as bug, feature or change.

People responsible for merging

All developers are responsible for merging and releasing merge requests. Check the merge request rules before you merge.

Common errors to watch out for

New external dependencies

External dependencies should always be locked to a fixed version.

# example
gem "audited", "5.1.0"

Rails find

ApplicationController in Easy8 registers a global rescue_from(ActiveRecord::RecordNotFound) that renders a 404 page. There is no need to rescue locally around .find in controllers.

Prefer a private memoized getter over a find_* before-action:

# Good: let the global handler produce 404
def my_model
  @my_model ||= MyModel.visible.find(params[:id])
end

For optional lookups where nil is a valid outcome, use .find_by and handle nil explicitly:

# Good: nil is intentional here
@issue = Issue.find_by(id: params[:id])
@project = @issue&.project

Avoid wrapping .find in a local rescue inside controllers:

# Avoid: redundant, the global handler already covers this
begin
  @issue ||= Issue.find(params[:id])
rescue ActiveRecord::RecordNotFound
  render_404
end

Nils

The most common problem. A variable or a parameter can be nil.

# On Hash
hash[:key1]
hash[:key1][:key2]

# On Array
[].flatten!.map(&:to_i)

# On model
@issue.assigned_to.name

Reload in after_save

Method .reload should not be used in callbacks, because it resets attributes in Active Model Dirty.

Magic constants

Don't use numbers or strings as status/type/property identifiers. Define constants that describe their meaning, and use them throughout the code in place of some meaningless symbols.