The code review process is generally regarded as a good practice and is adopted by hundreds of software projects around the world.
It provides lot of benefits:
Maintainers are responsible for the Pull Requests they approve. The main objective of this process is to make sure that the change being reviewed does not produce undesired side effects and that it’s inclusion is coherent with the project’s objectives.
Here are the rules for approving a Pull Request:
Pull Requests introduce into the codebase many kinds of changes.
Some Pull Requests do fix bugs. Sometimes the fix is done in a simple way, and the review is easy, sometimes the solution is more complex, and the reviewer must evaluate whether the submitted solution is good or if a better (simpler?) is desirable. It is recommended in such cases to ask for the opinion of other maintainers.
Some Pull Requests do introduce new behavior changes (a new button, a new Back-Office page, a new feature…) . If the change is impactful, such Pull Requests cannot be merged without the Product Team approval. It can be asked by marking the Pull Request and/or the related Issue with label “Waiting for PM” and Product Team will mark validated changes with label “PM ✓”.
Some Pull Requests do introduce new UX changes (changing a layout, modifying a color…) . If the change is impactful, such Pull Requests cannot be merged without the UX Team approval. It can be asked by marking the Pull Request and/or the related Issue with label “Waiting for UX” and UX Team will mark validated changes with label “UX ✓”.
Some Pull Requests do introduce new wording changes. Usually, Prestonbot will detect such Pull Requests and automatically add the “Waiting for wording” label. The Wording Team will review the Pull Request and work with the author until it is valid. At this moment the Pull Request is labelled “Wording ✓”. Without this label, such Pull Requests cannot be merged.
Some Pull Requests do introduce new technical changes (a new dependency, a new extensibility mechanism)… If the change is impactful, it is recommended for Pull Requests to ask for the opinion of multiple maintainers. For changes that are important, a Voting phase might be needed.
All these Teams can be reached out on the Slack channel.
During review, maintainers can ask questions to the Pull Request author (for example to better understand a technical choice). Simply writing the question as a comment is enough.
When doing so, it is recommended to add the label “Waiting for author” on the Pull Request. This helps other maintainers to know this Pull Request state.
There are multiple bots that monitor the Issues and Pull Requests on GitHub.
Prestonbot will try to add relevant informations on new Pull Requests and evaluate whether there are missing/invalid items. Following the Pull Request template, Prestonbot will add labels on the Pull Requests.
Here is a list of things that should not be approved in a Pull Request
More details available here.
Since PrestaShop follows SemVer, we should not accept Pull Requests introducing Breaking Changes unless they will be delivered in a new Major version.
Exceptions to this rule can however be made, for good reasons only such as:
Approving a Pull Request is actually a meaningful act. It carries multiple messages:
1. The submitted code is correct and its quality meets our expectations.
This is obviously a requirement for the Pull Request to be approved.
2. The outcome of this Pull Request is desirable.
Some Pull Requests are correct but are not merged because they do not benefit the project. For example a Pull Request that enables the support of XCF format for images is likely to be rejected as this image format is very rarely used in eCommerce.
3. We accept to introduce this new code into our scope.
Code that is merged inside the project becomes part of its scope. It means the maintainers team agree to maintain, manage, test, document and update this code as if it was their own.
Part of the Pull Request might also be integrated into PrestaShop public API which must evolve in a backward compatible manner to comply with SemVer. This means that once that it is released, it is quite frozen and must be preserved.
A Pull Request may only be merged after the following requirements have been fulfilled:
When merging a Pull Request on the Core Repository, maintainer must do the following, if it has not been done by someone else:
The items above are very important as they will be key to writing a good Release Note and ChangeLog for the next version.
It is also recommended to :
Pull Requests may be closed after 30 days of inactivity following a request for modifications.