Peer reviewing
Process
Peer review process helps to prepare the issue for integration. Peer reviewer is another developer who was not involved in the development process on the issue and therefore can take a fresh look and notice something that original developer might have forgotten during development. It is important to check that the bug actually is present and the code fixes it without creating new regressions.
Internal peer review
When the code came from HQ developer or external developer who has been contributing a lot to Moodle and is well acquainted with Moodle standards, coding and commit styles, peer review basically consists of checking the code according to the Checklist below.
If everything is fine, peer reviewer submits the issue for integration.
If some additional work is needed or author specifically asked not to submit for integration yet, peer reviewer clicks on “Finish peer review” and issue state returns to “Development in progress”. Usually the name of the peer reviewer stays on the issue and if the second peer review is requested it is expected that the same Peer reviewer picks it up. If peer reviewer is not able to do the second review, he should remove his name and comment about it. Otherwise the issue does not appear on “waiting for peer review” dashboard.
External peer review
When the code has come from external developer peer reviewer will also help the developer to lead the issue to integration. In this case peer reviewer should not use “Finish peer review” button.
If the issue needs additional work, peer reviewer comments about the suggested changes but does not change the status of the issue and it remains as “Peer review in progress”. If the author of the patch does not reply in 4 days, peer reviewer may select either to complete the patch himself or reopen the issue. The decision should be based on the amount of work required to complete the solution, for example, changing coding style or commit message, rebasing, backporting, adding testing instructions, etc. should be performed by the peer reviewer. This may require picking the commits into reviewer’s git branch.
Very important to give the credit to the author of the code by either keeping his/her authorship on the commit or adding “Thanks to XXXX for providing the patch” to the commit message.
There could be situations when the patch is incomplete, does not fix the original issue, creates regressions or otherwise is not correct. As stated above peer reviewer should comment about it and wait for the feedback from the author. If there is no feedback, or author can not work on this issue any more, and the peer reviewer also does not find it easy and important enough to complete, the issue needs to be reopen. Button “Fail peer review” (available to HQ developers only) will reset the issue status to “Reopened”. On this screen peer reviewer should remove assignee, peer reviewer and “patch” label from the issue. This way issue will become available again for anybody who want to work on it. All communication will remain in comments and issue history.
Checklist
These are points to consider while peer-reviewing issues. Further explanation below.
[] Syntax [] Whitespace [] Output [] Language [] Databases [] Testing (instructions and automated tests) [] Security [] Documentation [] Git [] Third party code [] Sanity check [] Icons
Acceptable check-marks are Y (for yes), N (for no) or - (for not applicable). All N check-marks should be accompanied by an explanation of the problem that still needs to be addressed.
Syntax
To allow the community of Moodle developers to work together, conventions should be followed.
- The code is easy to understand and, where it isn't, comments have been provided.
- Variables are named correctly (all lower case, no camel-case, no underscores).
- Functions are named correctly (all lower case, no camel-case, underscores allowed).
- PHP DocBlocks have been updated and adhere to coding style guide.
- Where functions are being removed, the Deprecation process is followed.
- The code doesn't use deprecated functions.
- $_GET, $_POST, $_REQUEST, $_COOKIE, and $_SESSION are never used.
See the Coding style guide for details.
Whitespace
Unnecessary whitespace changes can cause conflicts when committing code.
Ensure that:
- there are no unnecessary blank lines in the new code;
- blank lines do not contain spaces;
- there are no unnecessary changes to whitespace in other areas on the file;
Output
Output needs to be controlled by renderers to achieve consistency and correct application of themes.
Ensure that:
- output renders are used to generate output strings, including HTML tags;
- HTML output is valid XHTML;
- no inline styles have been used in HTML output (everything has to be in CSS);
- CSS has been added to the appropriate CSS files (base, specific area, sometimes canvas); and
- the code doesn't use buffered output unless absolutely necessary.
- all visual output has a RTL alternative included
feedback any notices (E_STRICT, etc) seen into the MDL.
Language
To achieve appropriate internationalisation of Moodle, language strings must be managed correctly.
Ensure that:
- new language strings are named correctly (all lower case, no camel-case, underscores are permissible in some cases);
- help strings are named and formatted as described in Help strings;
- language strings are used instead of hard-coded strings for text output;
- language strings have not been removed or renamed, had their meaning changed or had their parameters changed in stable branches (permitted only in master); and
- AMOS commands have been specified when moving or copying language strings.
Databases
DB calls are the greatest performance bottleneck in Moodle.
If there is SQL code you can test quickly then do so.
Ensure that:
- there are minimal DB calls (no excessive use of the DB); and
- the code uses SQL compatible with all the supported DB engines (check all selected fields appear in an 'order by' clause).
Testing instructions and automated tests
It is the developer's responsibility to test code before integration. Issues should not be sent for peer review without tests so that the peer reviewer can assess their quality and use them to consider the scope of the issue.
Ensure that:
- there are specific testing instructions that state how, as well as what, to test. Please ensure that the testing instructions are in the correct format: Behat gherkin;
- new unit tests have been added when there is a change in functionality; and
- unit tests pass for related areas where changes have been made.
- Behat tests pass for related areas where changes have been made, especially when it involved UI changes.
Security
The user community relies on Moodle being responsibly secure.
Ensure that:
- user login is checked where an identity is needed;
- sesskey values are checked before all write actions where appropriate (some read actions as well);
- capabilities are checked where roles differ; and
- if the issue itself is a security issue, the security process is being followed.
- Ensure that the fix is not available in a public repository (ie. a personal Github account); stand-alone patches should be provided instead.
- The issue will not be integrated until just before the next minor version release.
Documentation
Work does not stop when code is integrated.
Ensure that:
- Appropriate labels have been added when there has been a function change, particularly
- qa_test_required (significant functional change),
- docs_required (any functional change, usually paired with ui_change),
- dev_docs_required (any change to APIs, usually paired with api_chage),
- ui_change (any functional change, usually paired with docs_required, except ui_change remains permanetly), and
- api_change (any change to APIs that devs will need to know about, usually paired with dev_docs_required, except api_change remains permanetly).
Git
Ensure that:
- the commit matches the Coding style
- the Git history is clean and the work has been rebased to logical commits; and
- the original author of the work provided as a patch has been given credit within the commit (as author of in the commit message if changes were made).
Third party code
Does the change contain third party code? If so, ensure that:
- The code is licensed under a GPL compatible license.
- The instructions for upgrading/importing the library and contained within a readme_moodle.txt file.
- The library is recorded in a thirdparty.xml file, including licensing information.
- Third party code has been scanned to check for url accessible entry points that could be exploited. These should either be disabled, or if required functionality they should be checked for security weaknesses.
- Does not duplicate the functionality of any existing api or third party library in core.
Sanity check
Ensure that:
- the code seems to solve the described problem completely within its reported scope (and further issues have been created to resolve remaining parts or further refactoring);
- the code makes sense in relation to the broader codebase (look at the whole function, not just the altered code); and
- the developer has searched for and fixed other areas that may also have been affected by the same problem.
- if any version numbers have been changed in version.php files, then the changes follow the rule for updating version numbers in core.
Icons
Are new icons being introduced? If so, ensure that:
- the icons abide by our icon guidelines with regards to size, design and format
- the icons are do not unnecessarily add new ways of expressing existing concepts
- the icons are in a pix folder that makes sense
