docs/development/code-review-guidelines/README.md
This guide serves as a foundation on how to prepare your work for and perform code reviews for OpenProject.
We try to adhere to the Ruby community style guide as well as the AirBnB JavaScript style guide with some extensions for Angular. Rules we want to follow are expressed as either Rubocop definitions or eslint rules. Follow your linter to adhere to them.
Due to the age of our codebase, a lot of our code might not yet adhere to these style guides, but we want all new code to adhere to it. You do not 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 do not 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 do not have to fix issues with existing code. There is a list of editor plugins in the Rubocop docs. You can also use bin/dirty-rubocop to test them. Pull requests are being linted automatically through a GitHub action.
The same is true for eslint. Your editor will likely have support for eslint checks, and allows you to correct them before committing.
Lefthook
For automatically linting your files on committing them, please have a look at Lefthook. You can install these rules by using bundle exec lefthook install.
See the Git Book.
Every developer and reviewer should read the Rails Security Guide as well as the OWASP top ten.
OP##<Work package ID> or https://community.openproject.org/work_packages/ID in your pull request.Before requesting a review, double check your own changes:
Once your pull request is ready to be reviewed, convert it from a draft and add the label needs review. You can bookmark this query to always show all pull requests recently marked as reviewable
Do not explicitly request people or groups as reviewers unless you have collaborated with them already, or have a good reason to request specific feedback.
Wait for a reviewer to start. If you get feedback or requested changes, do not take them personally. Code is highly personal, and everyone might have different thoughts or ideas.
Try to respond to every feedback and resolve feedback that you addressed already. Re-request a review from the same person if you addressed all remarks.
You've found a pull request you want to review. Here is how to do it:
Reviewing code from your colleagues has higher priority than picking up more work. When you start your day, or in between working on other topics, please check the above link if there is any review requested.
If a review is left untouched, feel free to request a review from a group or link the pull request in question in the developers element channel.
If you're ready to perform a review for a pull request, do these things:
needs review label. This is a semaphore and ensures only one developer performs a reviewin review and assign yourselfAs a reviewer, your job is not to make sure that the code is what you would have written – because it will not 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.
Try to think of edge cases when testing or evaluating the code, double check the test coverage. But do not frown if you merged the pull request and something broke after all. This is the learning path to avoiding this mistake on the next attempt. Not doing a review in the first place will not move you forward either.
Keep in mind that we're all trying to do the correct thing. Be kind and honest to one another, especially since our reviews are public for everyone to see.
Verify that the appropriate tests have been added as documented above.
When testing a feature or change, check out the code test at least the happy paths according to the specification of the ticket.
If possible, add smaller documentation changes right away.
If there are breaking changes (e.g., to permissions, code relevant for developers), add them to the release notes draft for the release or create a new draft if none exists yet.
For external contributions: Check whether the author has signed a Contributor License Agreement and kindly ask for it if not. There is a GitHub workflow ensuring the contributor signed the current CLA. If the action is green, consider this point done.
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.
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 should not rush through a code review – but also, you need to do it promptly. Your coworkers are waiting for you.
Once you've completed the review, you might have left feedback for the developer. In that case:
in developmentIf there are no change requests, perform these steps:
Merge pull request button.Why merge, not squashing?
We do not use squashing to retain any commit information of the original developer, which might contain valuable information. If there are a lot of bogus commits, we squash them on the PR first and then merge them. Having all information about a change is deemed more important than a strictly linear history.
The only exception to this rule are single commit pull requests, which can be applied to dev using Rebase and merge instead.
Things everyone should do: code review