Reviewing and approving Pull Requests
All contributions to the PrestaShop project are subject to review. The main objective of this process is to make sure that proposed changes are desirable, coherent with the project’s objectives, and that its inclusion won’t produce undesired side effects.
Approving a Pull Request is a meaningful act. It carries multiple messages:
The submitted code is correct. This is the most obvious requirement; a Pull Request containing invalid code will never be merged.
The resulting behavior is desired. Some Pull Requests might be technically correct, but are not merged because they aren’t considered useful for the project. For example a Pull Request implementing support for the XCF format for images is likely to be rejected as this esoteric image format is rarely used for the web.
Maintainers and committers agree to maintain the added code. Code that is merged inside the project becomes part of it. It means the project maintainers agree to maintain, manage, test, document and update this code as if it was their own.
Everybody in the community can and is encouraged to review submitted Pull Requests. However, only some project members have the ability to accept or reject contributions.
In order to be merged by project members, contributions must undergo up to three stages of approval: Code review, Product / UX / Wording review (if necessary), and QA verification.
The code review process is generally regarded as a good practice and is adopted by most software projects around the world. It provides many benefits:
- it helps spot errors in the code, because we all make mistakes, and it can be hard to find one’s own mistakes.
- it helps improve code quality, not only by ensuring the code is readable and understandable, but also by pinpointing design, performance or security issues that may have been missed by the author.
- it helps spread knowledge of the codebase, because reviewers will learn how the code works too.
The goal of the code review is to find the best solution by sharing different points of view through constructive conversation.
Pull Requests submitted to the PrestaShop Core repository require approval from at least two different maintainers. Other repositories require approval from at least one maintainer.
During review, Pull Requests should be checked against these requirements:
- The change provides enough value to be worth merging, and is coherent with the rest of the project.
- The is code clear enough, and it includes all the necessary comments to understand it.
- The PR complies with the contribution guidelines, including:
- The PR targets right branch.
- The nature of the change and the author’s reasoning is clear.
- The PR title is clear enough to be added as a line in the Changelog.
- The change address the issue the PR is intended to fix/implement.
- The PR stays within scope to address only that issue.
- The PR provides clear instructions to verify that the change works as expected.
- All the files include the right licence headers.
- Any and all breaking changes have been described in the PR body.
- Any and all deprecations have been described in the code and in the PR body.
- The changes are covered by automated tests (where possible), and those tests cover all relevant edge cases.
- The changes don’t introduce evident regressions nor degrade the software quality in any way. If a compromise is being done, it has been documented.
- If the change introduces or modifies a feature, the expected behavior of the feature been documented and approved by the project’s Product Managers.
Not fulfilling the above requirements usually blocks a contribution from being accepted. However, the opposite does not automatically mean that such a contribution must be accepted.
This rule can be overridden in special cases, for example when a critical or security issue cannot be fixed in a backward compatible manner.
Here are some examples of “red flags” that should not be approved during code review:
- Anti-patterns or code that clearly violates software development best practices such as SOLID principles.
- Code that does not comply with current PrestaShop architecture, unless for good reasons (example: view logic inside the model layer).
- Code that performs and/or scales poorly.
- Code that is very hard to read and consequently less maintainable.
- Code that is missing standard features or that is not generic enough (example: logic that only makes sense for a specific country or region).
- Code that is missing a key need of PrestaShop’s users (example: CSS that is not RTL-compliant).
- Code that is not secure.
More details available here.
Product, UX & Wording review
Besides the maintainers' code review, additional approval from other members is required in the following cases:
- Product Managers, when the change introduces a new feature or changes in existing features (eg: a new page, adding a button, modifying a business rule…).
- UX Managers, when the change introduces visual changes (eg: changing a layout, modifying a color…).
- Wording Managers, when the change introduces new wordings.
Whenever any of cases above applies, the Pull Request cannot proceed to QA verification until at least one of each of the required project members have given their approval.
If necessary, these people can be reached out on the Slack chat.
After the Pull Request has been approved, the change must go through a Quality Assurance (QA) verification. Software testers will proceed to manually test the modified software to make that it works as expected.
As a general rule, all Pull Requests undergo the QA verification, except for the following cases:
- Version branch merges
- Changes that have no functional impact (e.g. code comments), cannot be manually verified, or are verified by automated tests.
The Pull Request can proceed to merge once the QA verification is successful.
Be polite and respectful. Remember that you’re reviewing the hard work of other people. The Symfony project provides good tips on providing respectful review comments.
The reviewer’s job is to evaluate whether the proposed solution is appropriate, not to reverse-engineer it. If a change appears as cryptical or difficult to understand, ask the author to clarify their intent and/or justify their choice of implementation.
PrestaShop is complex software, and it’s not unusual to be unaware of the details of all components or subsystems. When in doubt, ask other maintainers for their opinion. If a PR introduces significant changes (a new dependency, a new extensibility mechanism), consider asking the opinion of multiple maintainers.
Note that architecture related changes require submitting a formal request for comments, which are subject to a vote from maintainers, and are approved by majority.
Pull Requests may be closed after 30 days of inactivity following a request for modifications.
Merging Pull Requests
A Pull Request may only be merged after the following requirements have been fulfilled:
- The change has received all the required approvals.
- The change does not have any outstanding merge conflicts.
- Automated checks (including automated tests) are passing.
On the PrestaShop Core repository), two approvals are required.
After merging a Pull Request on the Core Repository, maintainers must make sure to:
- Link the Pull Request to the right milestone. The milestone to choose is the next target release.
- Add the label if the Pull Request must be mentioned in the Release Note.
- Add the label if the Pull Request introduces a BC Break.
- Add the label if the Pull Request introduces changes that need to be documented in DevDocs.
- Add the label if the Pull Request introduces changes in configuration or data structure and existing installs need to be modified.
- Add the label if the Pull Request introduces changes that need theme modification to work.
These actions are very important as they will be key to writing a good Release Note and ChangeLog for the next version.
Some Pull Request contain changes that need an update of the developer documentation.
Example of such Pull Requests:
- If it contains BC breaks or deprecations, they must be listed, including recommended workarounds (if any).
- If it introduces or modifies a component or subsystem — especially when theme or module developers are expected to use it.
- If the Pull Request is related to an issue, check whether the issue is fixed and closed and whether it should be: some issues can get closed automatically even though the Pull Request only addresses part of the issue.
- Thank the Pull Request author and anybody else who invested notable energy into the Pull Request (code review, code suggestions, QA validation, usecase specification …).
Labeling helps project members keep track of the status of Pull Requests.
Two bots monitor the project’s Issues and Pull Requests on GitHub to help automate common tasks.
- Prestonbot will evaluate whether there are missing/invalid items in Pull Requests' descriptions and label them accordingly.
- Issuebot helps sync the status of Issues with its linked Pull Request by applying labels and performing kaban board automation.
The following labels are added when an action is required:
|When||Who is concerned||Label
|The PR is waiting for the author to address feedback||The PR’s author||(no label)|
|The PR must be rebased
(because of merge conflicts or wrong base branch)
|The PR’s author||(no label)|
|The PR introduced functional changes||Product Managers|
|The PR introduced visual changes||UX Designers|
|The PR introduced wording changes||Wording Managers|
|The PR is ready for QA verification||Software Testers|
|The PR needs to be documented in DevDocs||Maintainers|
|The PR needs PR for autoupgrade module||The PR’s author||(no label)|
|The PR needs PR for default theme||The PR’s author||(no label)|
The following labels provide metadata and are essentially informative:
|This PR fixes a bug|
|This PR introduces a new feature|
|This PR is an improvement (e.g. performance)|
|This PR is a refactoring|
|Destination branch (e.g. 1.7.8.x, develop, …)|
|This PR includes breaking changes|
|This PR includes a feature to be highlighted in this release|
|This PR is a work in progress|
|This PR is about UI automated tests|
|This PR is about the Symfony migration project|
Following the above items, here is an example of a PR workflow
Upon opening a PR and you are the first reviewer, please verify:
- Is the The Pull Request form correctly filled with relevant informations, especially
How to test?
- Does the CI pass?
- When relevant, is there a linked GitHub issue (for bugfixes or new features)?
If any of the above items are missing, you can ask the PR author to provide them. Else, you can review the code.
While reviewing, an action might be required, then add the right label (see above section
Action required), for example
- If the code needs changes, you can add
Waiting for authorlabel.
- If the PR introduced visual changes, you can add
Waiting for UXlabel.
If no action is needed and the code is correct, you can approve the PR.
If it has the right number of approvals (Pull Requests submitted to the PrestaShop Core repository require approval from at least two different maintainers while other repositories require approval from at least one maintainer), the last step for most PRs is QA validation.
You can add
Waiting for QA label to request a validation from QA team. Some PRs do not need QA testing, for example fixing a typo or a code change that only impacts CI.
If the PR behavior is confirmed by QA Team, the PR can be merged. See above section
Merging Pull Requests for the different actions needed following the merge.