Skip to content

Check list for peer reviewing patches and pull requests

Jump to bottom
Luca Bösch edited this page May 13, 2024 · 13 revisions

Preface

  • Contributions to our plugins from within and from outside the german-speaking higher education Moodle community are highly appreciated. We always appreciate if developers take the time to improve and extend our plugins in any way.

  • Having said that, we have to highlight that our plugins are built with passion and contributions to the plugins should strive to match this quality level as well.

  • Every community member, regardless if he has push rights to the affected plugin repository or not, is invited to do a peer review of a submitted patch or pull request. To help with this task, we built the following checklist.

Aspects to check during peer review

The following sections explain the aspects to be checked during the review process.

  • At the end of this page, you'll find a checklist you should use for your peer review documentation, e.g. in a pull request.
  • If you are editing the following aspects please revise the checklist to reflect your respective changes

Documentation

  • Is the commit message understandable? There is no real need (but highly appreciated) to summarize the whole patch in some paragraphs within the commit message. But it is mandatory that the first line of the commit message is understandable and self-explaining so that everyone gets the idea what the patch is about. Ideally, the commit message also references the Github issue which is solved by the patch.

  • Is the patch author correct? Generally, it is highly encouraged that the author of the patch maps to a real human and an existing e-mail address. Commit authors like root <root@localhost> are not acceptable.

  • Is there a new line in CHANGES.md? Generally, a patch should include a new line in CHANGES.md which contains the commit message as well. This is especially helpful as the CHANGES.md file's content is automatically published on the moodle.org/plugins repository when a new version is released there.

  • Is there new information in README.md? Generally, if there were major changes or new functionalities, changes to README.md might be recommended or necessary. This mainly means that the "Usage & Settings" section should be changed or amended. If new settings were introduced, the setting description should match more or less the text in README.md.

  • If the README.md contains a 'Contributors' section in README.md, that the contribution is honored in the Contributors section.

  • Does the code contain enough comments? There is no mandatory code-to-comment ratio for our plugins, but generally the purpose of the plugin code should be understandable by reading just the comment lines and skimming over the PHP / Javascript lines.

  • Are the PHPDoc comments correct? Generally, there is no real need to document everything with PHPDoc in our plugins. However, at least the minimum documentation requirements which are also demanded by the Moodle PHPDoc checker (which runs within Github actions, see below) have to be fulfilled.

version.php

  • Is the $plugin->version value increased? Not every patch needs to increase the version in version.php. But there are clear situations, especially if new settings are introduced to settings.php, new strings are added to the language pack, new classes were introduced, new CSS styles or JavaScipt code was introduced when a version increase is recommended or even mandatory.

  • Is the $plugin->release value kept untouched? Generally, the release string should only be changed when an official release is made and published to the moodle.org/plugins repository.

lib.php

  • Does lib.php only contain necessary functions? The lib.php file of a plugin is loaded more or less at every page load, regardless if the plugin is needed on this page or not. This is because lib.php can contain hook functions. Against this background, functions which help the plugin to realize its own logic, should not be placed in lib.php and should be placed in a locallib.php or autoloaded class instead.

Languages

  • Are there any non-english strings in the english language pack or are the any non-english language packs? All translations are done on AMOS which means that there must not be any non-english strings in the patch.

  • Are magic strings added? There are some assets in a plugin which have to be accompanied by a magic string in the language pack, for example new capabilities, new scheduled tasks or new events. If any of these assets were added, the magic string should be added to the language pack as well.

Automated tests

  • Many of our plugins have a good coverage with Behat tests, some are covered with PHPUnit tests as well. As a general rule, patches must not break existing tests which were running successfully before. Additionally, patches which introduce new functionality should contain new automated tests which cover the new functionality as well.

Mustache templates

  • Some of our plugins ship with Mustache templates. Mustache templates have to contain a properly formatted example context (as JSON data structure) so that the template can be looked at in the Moodle template library.

CSS and styles.css

  • If the plugin ships with CSS styles or adds CSS classes to HTML elements, Bootstrap classes and styles should be used, if possible. We should always strive to use the basics which Bootstrap provides and not to re-invent the wheel.

Duplicated code

  • Has any code been duplicated from Moodle core into the plugin and been modified? If yes, then the source of the duplicated code should be mentioned with a comment like this: This code is copied and modified from original_function() in path/to/original/file.php. This way, it is made clear that the maintainer should check the code for upstream changes every now and then.

Github action

  • Is the Github action run all green? With every patch and every pull request, a Github action run is triggered which checks especially the plugin code against the Moodle coding styles and runs all automated tests. If this Github action run is not all green, the patch must not be merged and the issues have to be fixed first.

Commit history and scope

  • Does the patch consist of one single commit? One contribution to a plugin should consist of one commit. If there are multiple commits, these should be squashed during the merge.

  • Is the patch really focused on one single purpose? It might ok if a patch changes some minor things in the surroundings of the patched lines if this helps to improve the overall quality of code. However, if a patch contains completely unrelated changes like fixing typos in lines which would not be touched by the patch otherwise, these changes should be split in separate patches.

  • Are the any surplus files in the patch? If the commit author added any file by mistake or by purpose which does not relate to the patch's purpose, especially Mac OS .DS_Store files or any IDE management files, these files should be dropped before merging the patch.

Additional aspects for Boost Union

If you are reviewing a pull request for the Boost Union theme, additional aspects have to be checked:

  • Are there any usages of $theme->settings->foo in the patch? To avoid breaking Boost Union Child, settings must always be fetched with get_config('theme_boost_union', 'foo'). Otherwise, Moodle Core would search the setting in the currently active (grandchild) theme which might not re-implement this setting.

  • Is there any setting-dependent renderer / output code which is called on main pages, for example in layout files or in overwritten core renderers? If yes, this code must be built in a way that it would not break or trigger PHP Undefined variable warnings within the plugin update process, i.e. when the code is placed on disk already but the setting is not created in the DB yet. This can be often achieved by using isset().

  • Are there any binary (yes / no) admin settings introduced? If yes, then these have to be realized as admin_setting_configselect setting with the THEME_BOOST_UNION_SETTING_SELECT_NO / THEME_BOOST_UNION_SETTING_SELECT_YES constants and not as checkbox setting.

  • Are there any mustache templates copied and modified? If yes, then the modifications should be marked in the head of the mustache template like this:

This template is a modified version of theme_boost/original
Modifications compared to the original template:

In addition to that, a .upstream copy of the mustache template has to be in place.

Checklist

Use the following markdown template to document your reviews

## Review

Following https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Check-list-for-peer-reviewing-patches-and-pull-requests

### Documentation

- [ ] **Commit Message** understandable and issue referenced (via resolves keyword)
- [ ] **Patch author** correct
- [ ] **CHANGES.md** appended
- [ ] **README.md** appended
- [ ] **Credits** appended
- [ ] Appropriate **Comment quantity**
- [ ] Successful **Moodle PHPDoc check**

### version.php

- [ ] Checked if **$plugin->version** increment necessary (and increment done if necessary)
- [ ] **$plugin->release** untouched

### lib.php

- [ ] Only **necessary functions**

### Languages

- [ ] Only **english strings**
- [ ] Necessary **magic strings** added (e.g. capabilities)

### Automated tests

- [ ] **New functionality covered**
- [ ] **No failing steps or scenarios**

### Mustache templates

- [ ] **Example context exists**

### CSS and styles.css

- [ ] **Bootstrap styles** used if possible

### Duplicated code

- [ ] **Duplicated code is marked properly**

### Github action

- [ ] **All green**

### Commit history and scope

- [ ] Already **single commit**
- [ ] Focus on **single purpose**
- [ ] No **surplus files**

### Additional aspects for Boost Union

- [ ] No usage of $theme->settings
- [ ] Usage of isset() checks when processing plugin settings in renderer / output code
- [ ] Usage of admin_setting_configselect instead of admin_setting_configcheckbox
- [ ] Modified mustache templates properly marked and .upstream template in place

## Review result

* ...
Clone this wiki locally