I recently found out about the Moodle Peer reviewing checklist when its results where added to issues that I had submitted for integration. There is also the Integration review documentation, but most of this is about principles than a checklist that might guide a developer on what is expected of them.
Since the OU has a similar checklist, and there was some discussion at the recent developer meeting in Perth about review processes, I thought it would be a good idea to compare the two lists and see if we can learn anything from each other.
The OU process is for peer review and integration to usually be done by the same person, a lead developer. Though sometimes peer review is done by any developer, integration is always done by a lead. We expect the handover checklist to be completed by the developer and checked by the reviewer so the list acts as an aide memoire for developers that they have done everything they need to. Our checklist is a series of statements with sample answers e.g.
Y - Tested.
N/A - no course-related data in database or filesystem.
N/A - data is only stored in existing backed-up locations.
N/A - data does not need to be backed up.
Our system of sample answers allows for a little more flexibility on the part of the integrator to accept things which are not perfect, with the integrator using their judgement on how important compliance would be in each situation.
There are a number of checks which only appear in the OU checklist:
- support for groups needed
- basic accessibility check
- cross browser support
- consideration of mobile devices
- install & upgrade checked
I wonder where in HQ’s processes these things are being picked up? Presumably by the integrators, though that isn’t clear anywhere. Should it be? Does it make sense for extra checks to take place on integration than at peer review – personally I think not.
Some of the steps in the HQ checklist are covered by our one code checker entry. I wonder if it would be of benefit to the entire developer community if the code checker were committed into core?
Perhaps of more interest to me, the following are only in the HQ checklist:
- provision of testing instructions
- provision of new unit tests
- sanity check that similar problems throughout the code have been fixed
- git commits
- security – sesskeys, login
- cross db engines
- AMOS commands when moving/copying/deleting lang strings
- renderers, inline style, buffered output, use of appropriate css files
- _GET, _POST etc not used, moodleisms such as correct api calls used
- deprecated functions check, deprecation process
- PHP docblocks present
We’ve talked about testing instructions before at the OU, and generally think its a good idea. That said, for new features where our handover checklist is most used, this should be in the specification document.
The OU does not require new unit tests, only gives brownie points if they are present. That’s a reflection on how busy we often are – which is a poor excuse for not having tests really!
Git commits are the big difference if you work on both core and OU moodle. We flatten work-in-progress branches into a single commit on integration. The HQ way seems more time consuming and complex to me, and if I haven’t got my head round it yet, will some of our less able developers ever manage it? I’m not clear what the benefit of all the extra work is either?
I think adding a security check to our list would be a good idea. It is easy for our developers to think that the entire system is log-in protected, but when the code is used on OpenLearn as well, this is not always the case.
Similarly I think we could add some checks about renderers and CSS styling, and also about not using $_ variables or deprecated functions but using proper moodle APIs where available.
There are a couple of entries that I think are less applicable to us, unless we are offering our modules as contrib – particularly cross DB engine support, AMOS support, deprecation process.
Changes to the OU checklist are agreed by consensus of lead developers. I’m hoping this will spark some discussion about things we could usefully add without overloading people.