Reviewing

As an Odoo employee, how to review external (and internal) pull requests:

What to Review

• List of PR from the community:
  • odoo/odoo
  • odoo/enterprise

Targeting stable?

• See Stability Guidelines
• Not breaking anything is not enough, a patch must fix a bug
• Some customisations or modules (officials or not) may rely on a behaviour, changing it will break these modules
  • No new feature
  • No change of field definition
  • No new SQL constraint
  • No change of _order
  • No change of signature on public methods
  • No hook method (helping to modify standard behaviour)
  • No code cleaning or cosmetic changes
  • No change of translatable content (labels, templates, messages,...)
• The older the version is, the stricter you must be
  • The older a version is, the bigger is the risk of somebody relying on a weird behaviour

Targeting master?

• New features and change of behaviour are accepted
• No need to worry about translations (no changes in .po or .pot files)
• Use the Security Checklist

Commit message?

• See Commit Message Guidelines
• In 90% of the time, the commit message does not respect our guidelines, don't be lazy, change it!
• Explain the why, not what

CLA failing?

• See sign the CLA
• The verification is based on the last commit email address
  • Add .patch at the end of the URL to find it easily
• The verification is based on the source branch, not target, make sure it is rebased

Translations?

• See translation guidelines
• Never change .po files
  • to be done on Transifex
  • a few exceptions are allowed like localisations or branches/languages not on Transifex
• Adapt the .pot if adding or removing terms (in stable)
• Modifying existing term will make it untranslated for many people

Fetching the code

The code is probably on a new remote, to test it (and adapt it if needed) you have to add the remote and branch.

• To help you, add the get-br alias to your ~/.gitconfig
• $ git get-br remote-name:branch-name
• $ git commit --amend modify the commit message
• $ git rebase odoo/12.0 rebase branch (and squash?)
• $ git push -f push force (or consider --force-with-lease) on target branch
  • in most cases you have the rights to do it (allowed by default)
  • if not you have to create a branch on odoo-dev with the amended code, create pr,...
• @robodoo r+
• Remove the added remote and branch
  • $ git branch -D branch-name
  • $ git remote remove remote-name
  • or, if you are lazy, use the rem-br alias
    • $ git rem-br remote-name:branch-name

Be nice

Last but not least, be nice to people!

Somebody took the time to create a contribution to Odoo. They may not be professional developers nor they know our specific guidelines. They may not know all the corner cases or how to use git properly.

Most of the time the contributions will not be good enough on the first try or may even be rejected. Explain why you can not accept a patch.

See also

• Contributing Guidelines
• Google's guide on How to do a code review