Top Menu

Jump to content
Home
    • Projects
    • Work packages
    • News
    • Getting started
    • Introduction video
      Welcome to OpenProject Community
      Get a quick overview of project management and team collaboration with OpenProject. You can restart this video from the help menu.

    • Help and support
    • User guides
    • Videos
    • Shortcuts
    • Community forum
    • Professional support

    • Additional resources
    • Data privacy and security policy
    • Digital accessibility (DE)
    • OpenProject website
    • Security alerts / Newsletter
    • OpenProject blog
    • Release notes
    • Report a bug
    • Development roadmap
    • Add and edit translations
    • API documentation
  • Sign in
      Forgot your password?
      Create a new account

      or sign in with your existing account

      Google

Side Menu

  • Overview
  • Activity
  • Roadmap
  • Work packages
  • Calendars
  • Team planners
  • Boards
  • Forums
  • Wiki
    • Table of contents
      • Expanded. Click to collapseCollapsed. Click to showDeveloper
        • Hierarchy leafAccessibility Checklist
        • Hierarchy leafCode Review Guidelines
        • Expanded. Click to collapseCollapsed. Click to showContribution
          • Hierarchy leafGit Workflow
          • Hierarchy leafTranslations
        • Expanded. Click to collapseCollapsed. Click to showDeveloping Plugins
          • Hierarchy leafDeveloping an OmniAuth Authentication Plugin
        • Hierarchy leafRelease Process
        • Hierarchy leafReport a bug
        • Hierarchy leafSecurity
        • Hierarchy leafSetting up an OpenLDAP server for testing
        • Hierarchy leafTheme Features
      • Hierarchy leafDownload
      • Expanded. Click to collapseCollapsed. Click to showFeature tour
        • Hierarchy leafRelease Notes OpenProject 30
        • Expanded. Click to collapseCollapsed. Click to showRelease Notes OpenProject 30 - Overview
          • Hierarchy leafGlossary
          • Hierarchy leafRelease Notes - Accessibility
          • Hierarchy leafRelease Notes - Accessibility changes
          • Hierarchy leafRelease Notes - Add work package queries as menu items to sidebar
          • Hierarchy leafRelease Notes - Copy projects based on Templates
          • Hierarchy leafRelease Notes - Design changes
          • Hierarchy leafRelease Notes - Fixed Bugs
          • Hierarchy leafRelease Notes - Keyboard Shortcuts
          • Hierarchy leafRelease Notes - Project settings
          • Hierarchy leafRelease Notes - Ruby&Rails Update
          • Hierarchy leafRelease Notes - Security
          • Hierarchy leafRelease Notes - Timelines
          • Hierarchy leafRelease Notes - Work packages
      • Hierarchy leafHowto create animated gifs
      • Hierarchy leafMigration Squashing
      • Hierarchy leafMod security
      • Hierarchy leafNew work package page
      • Hierarchy leafOP3 to OP4 Debian upgrade
      • Hierarchy leafOP4 Ubuntu1404 Stable with MySQL in production
      • Hierarchy leafOpenProject 40 Development Setup
      • Expanded. Click to collapseCollapsed. Click to showOpenProject Foundation
        • Hierarchy leafBoards
        • Hierarchy leafMembers
        • Hierarchy leafOPF-Meetings
        • Hierarchy leafStatutes
      • Expanded. Click to collapseCollapsed. Click to showRelease Notes
        • Hierarchy leafOpenProject released on Bitnami
      • Expanded. Click to collapseCollapsed. Click to showRelease Notes OpenProject 40 - Overview
        • Hierarchy leafRelease Notes OpenProject 40 - Accessibility improvements
        • Hierarchy leafRelease Notes OpenProject 40 - Column header functions in work package table
        • Hierarchy leafRelease Notes OpenProject 40 - Improved Design
        • Hierarchy leafRelease Notes OpenProject 40 - Integrated query title on work package page
        • Hierarchy leafRelease Notes OpenProject 40 - Integrated toolbar on work package page
        • Hierarchy leafRelease Notes OpenProject 40 - OmniAuth integration for OpenProject
        • Hierarchy leafRelease Notes OpenProject 40 - Work package details pane
      • Expanded. Click to collapseCollapsed. Click to showSecurity and privacy
        • Hierarchy leafFAQ
      • Expanded. Click to collapseCollapsed. Click to showSupport
        • Expanded. Click to collapseCollapsed. Click to showDownload and Installation
          • Hierarchy leafInstallation MacOS
          • Expanded. Click to collapseCollapsed. Click to showInstallation OpenProject 3 0
            • Hierarchy leafDebian Stable with MySQL in production
            • Hierarchy leafInstallation Ubuntu
            • Hierarchy leafInstallation Windows
            • Hierarchy leafInstallation on Centos 65 x64 with Apache and PostgreSQL 93
          • Expanded. Click to collapseCollapsed. Click to showInstallation OpenProject 40
            • Hierarchy leafOP4 Debian Stable with MySQL in production
          • Expanded. Click to collapseCollapsed. Click to showMigration paths
            • Hierarchy leafFrom Chilliproject to OpenProject
            • Hierarchy leafMigration 15 to 30
            • Hierarchy leafMigration 24 to 30
            • Hierarchy leafMigration Redmine 2x › OpenProject 30
            • Hierarchy leafOpenProject 3 Migration
          • Hierarchy leafOpenProject 40
        • Expanded. Click to collapseCollapsed. Click to showNews
          • Hierarchy leafNew OpenProject Translations Plugin
          • Hierarchy leafNew Plugin on OpenProjectorg Local Avatars
          • Hierarchy leafNew design for OpenProject
          • Hierarchy leafNews Accessibility workshop for OpenProject
          • Hierarchy leafNews Glossary for OpenProject
          • Hierarchy leafNews Heartbleed fixed
          • Hierarchy leafNews Icon Fonts
          • Hierarchy leafNews OpenProject 30 Release
          • Hierarchy leafNews Release GitHub Integration Plugin
          • Hierarchy leafNews Success Story Deutsche Telekom
          • Hierarchy leafNews Timelines
          • Hierarchy leafOpenProject 3013 released
          • Hierarchy leafOpenProject 3017 released
          • Hierarchy leafOpenProject 40 released
          • Hierarchy leafOpenProject 40 will be coming soon
          • Hierarchy leafOpenProject 405 released
          • Hierarchy leafOpenProject and pkgrio
          • Hierarchy leafOpenProject news moved to a new blog
          • Hierarchy leafOpenProjectBitnami
          • Hierarchy leafPackager version with plugins released ("Community edition")
          • Hierarchy leafRegistration OpenProject-Foundation
          • Hierarchy leafRelease OpenProject AuthPlugins
          • Hierarchy leafUpdates on OpenProject
          • Hierarchy leafWe need your feedback for the the new fullscreen view for work packages
        • Hierarchy leafOpenProject Plug-Ins
      • Hierarchy leafWiki
You are here:
  • Developer
  • Code Review Guidelines

Content

Code Review Guidelines

  • More
    • Print
    • Table of contents

Code Review Guidelines

Correctness

As a reviewer, your job isn’t to make sure that the code is what you would have written - because it won’t be. Your job as a reviewer of a piece of code is to make sure that the code as written by its author is correct.

Coding Style

We try to adhere to the Ruby community styleguide. At some point we’ll have to make decisions about some rules that are not clear or defined within that styleguide. In that case, we should either fork it or note all decisions here.

Most of our code does not yet adhere to this styleguide, but we want all new code to adhere to it. You don’t have to improve existing code when making changes, but we encourage it. If you do, please do all improvements in a separate commit from the actual change, so the improvements don’t hide your actual code changes in a diff.

Before committing, please run your new code through Rubocop. It detects deviations from a lot of things in the style guide and things that are bad practice in general. You obviously don’t have to fix issues with existing code. There’s a list of editor plugins in the Rubocop readme.

When reviewing code and you think the author hasn’t run the code throug Rubocop, please ask them to.

Commit Messages

  • First line: less than 72 characters, this is when GitHub shows ‘…’
  • Blank line
  • Detailed description of the change, wrapped to 72 characters so the text is readable in git log

See the Git Book: http://git-scm.com/book/en/Distributed-Git-Contributing-to-a-Project#Commit-Guidelines

Testing

  • All tests must be green on continuous integration
  • Appropriate unit and integration tests
    • e.g. test application logic via rspec tests and its integration with the user interface via Cucumber tests
  • Test JavaScript code, e.g. via Jasmine or Selenium/Cucumber
    • but: Cucumber tests that don’t test JavaScript must not have the @javascript tag, otherwise they run slower
  • Look for sufficient coverage of both Ruby and JavaScript code
    • CI will provide at least Ruby line coverage information at some point in the future
  • Forms: besides testing for success response, also test whether values are actually saved
    • e.g. read form or look into database
  • Bugfixes: must contain a test which detects the bug they fix to prevent regressions
  • Translations: Never use a specific translation string in a test. We might want to change them in the future and don’t want to fix tests when we do.

Security

Every developer and reviewer should read the Rails Security Guide.

  • Rails 3.x Security Guide

Changelog

  • As of OpenProject Stable Release 3.0.8 all changes made to the OpenProject software are documented via work packages on openproject.org. There is no
  • The Roadmap view gives a corresponding overview.
  • To prevent inconsistencies and avoid redundant work there is there is no additional change log in the source code.

See History of Changes section of the Release Process

Other

  • For external contributions: Check whether the author has signed a Contributor_License_Agreement and kindly ask for it if not
  • Copyright notice: When new files are added, make sure they contain the OpenProject copyright notice (copy from any file in OpenProject)
  • Adding Gems: When adding gems, make sure not only the Gemfile is updated, but also the Gemfile.lock
  • No Trailing Whitespace
  • Single newline at the end of a file

Accessibility

General

  • Readability and Understandability: The reviewer should understand the code without explanations outside the code.

There is never anything wrong with just saying “Yup, looks good”. If you constantly go hunting to try to find something to criticize, then all that you accomplish is to wreck your own credibility.

You shouldn’t rush through a code review - but also, you need to do it promptly. Your coworkers are waiting for you.

Citations from: http://scientopia.org/blogs/goodmath/2011/07/06/things-everyone-should-do-code-review/

http://beust.com/weblog/2006/06/22/why-code-reviews-are-good-for-you/
https://developer.mozilla.org/en/Code_Review_FAQ

Loading...