Tim Hunt
Posts made by Tim Hunt
Thanks Matt. I am really glad that something is being done to address the worrying trend review queue, that can be seen in in the Integration exposed summary each week. And, I think this proposal is sensible (for what that is worth).
I am a big fan of Kanban principles, and one of the principles there is about 'work-in-progress' limits. Lots of stuff sitting around that has been stared but not completely finished does nobody any favours, and is hugely inefficient (a trivial example in this case is rebaseing contributions for weeks on end while they are not looked at). Should set ourselves an aspirtation? For example, that we would like to see an integration queue that is shorter than, say, twice the average number of issues that are being successfully merged each week? If that was the case, then, on average that would mean that isses get integrated within a couple of weeks, which would be very nice. Obviously, it would take some time to get there from where we are now.
While there is a lot of promise in this change, there is one obvious risk: if you have more less experienced people doing reviews, then there is a danger that changes get through that would previously have been correctly sent back for further work. For a lot of small changes that flow through integration, this is not a problem at all. They are simple and obviously good change (e.g. a bug fix) so it is just about the code quality. The real risk is in big and complex changes, and in a product as large as Moodle, we all need to be involved in a constant battle to keep the complexity under control. We will have to see if the changed process is capable of dealing with this, but it was one of hte things the small, but highly skilled and experienced itegration team did very well: work to ensure Moodle stayed consistent and not unnecessaily complex.
So, in relation to that, can I pass on one bit of advice I often give to people when introducing them to doing code review. There are acutally three possible outcomes to a code review, the obvious two:
- This is a good change: I understand this change; I can see it is right for Moodle; and it follows all the required code code quality standards. This can move forwards.
- This needs more work: I can see these apects of the change, as it is currently written, which need to be improved before it could be merged.
But, there is a third outcome you should allow yourself:
- I don't know: I partially understand what is going on here, but I can see that there are implications that I don't fully understand. I think it would be good for someone more experience than me to have a look.
I hope we encourage people doing reviewes to consider this third option - particularly as people are new and building experience. However, I have been doing CLR for quiz and question bank issues for years, and there are still changes I look and and find myself in this third outcome. Until now I could send back to integration. Obviously the specifics of that will now change, but I hope the principle remains. (And, watching what comments the more experience reviewer makes when they look at the issue is a really good way to learn.)
I hope these are some useful thoughts.
2) In the new world, the key table for random questions is question_set_references, and that link to quizzes via quiz_slots. And example of some SQL that does that join is https://github.com/moodle/moodle/blob/024e36be173aa99c290d7e91ed088c645e523ee6/mod/quiz/classes/question/bank/qbank_helper.php#L164