|
| 1 | +--- |
| 2 | +title: Code review tips |
| 3 | +tags: |
| 4 | + - Processes |
| 5 | + - Integration |
| 6 | + - Code |
| 7 | + - Code review |
| 8 | +sidebar_position: 5 |
| 9 | +--- |
| 10 | + |
| 11 | +:::info About this page |
| 12 | + |
| 13 | +This guide provides tips for code review in Moodle. It is intended for developers who are reviewing code at any stage of the development process (Peer review, CLR or Integration review). |
| 14 | + |
| 15 | +Thanks [Safat](https://moodle.org/user/profile.php?id=3375956) for sharing your experience and knowledge with the community! |
| 16 | + |
| 17 | +::: |
| 18 | + |
| 19 | +## General |
| 20 | + |
| 21 | +### Zero trust |
| 22 | + |
| 23 | +As bad as this term sounds, this is a rule of thumb while reviewing any patch or tracker. Even if the patch comes from the most experienced developer in the world, you should put zero trust in the patch and look at it like all other patches. While we should respect the opinions and approaches of more experienced engineers, as a reviewer, it's your responsibility to make sure that the patch is clean. Humans make mistakes, regardless of their experience in any product. |
| 24 | + |
| 25 | +### Error log |
| 26 | + |
| 27 | +It's not a hard requirement, but for patches that are touching critical APIs, try to check the error log after going through the testing instructions. It helps finding patches that have error logs recorded that have not been tested by CI or any other automated systems. |
| 28 | + |
| 29 | +### Reproduce the patch |
| 30 | + |
| 31 | +Simple rule, if you can't reproduce it, it's not an issue. If the reporter is having issues in their system which you don't have access to, that's not your business anymore. They will have to provide a proper docker environment or steps for an issue to be reproduced. Although it should be caught in the triaging stage, it's always good to double-check. |
| 32 | + |
| 33 | +### Evaluate the feasibility of an issue |
| 34 | + |
| 35 | +Anyone can create a tracker and push it. That doesn't necessarily mean that is something should be included in Moodle LMS core. While they are also a user, we will have to make sure the patch for an improvement is beneficial enough for everyone. Proactively engage the UX team to evaluate feature feasibility from a user perspective. |
| 36 | + |
| 37 | +### Understand the impact |
| 38 | + |
| 39 | +An important point to remember is that, the main purpose of code review is not just to review code quality and find syntax errors, coding style, documentation issues, etc. While we have to do that as we don't have a tighter tools yet, our main goal is to evaluate the impact of a patch, how it will behave in the wild with other APIs, and whether this is a breaking change. A patch might work great isolated in its own even PHPUnit and behat passing, but it might create issues somewhere else. While it's not always possible to understand all the impacts and we can't just keep digging a patch for 7 days, keeping the impact in mind will help mitigate risks upfront. |
| 40 | + |
| 41 | +### Evaluate the approach |
| 42 | + |
| 43 | +A regular example of this can be drinking water. You can drink water, in a cup, in a glass, in a jug, in a bottle, or even in a plastic container you took out of your dishwasher while it was shining clean. But which is the most appropriate one to use? A glass! But that's not always possible, right? You might drink using your hand when there is nothing available, or you might use a wine glass when you loaded your dishwasher with all the glasses and you have nothing left. Same way, one patch can be done too many different ways, while some ways might be wrong, some might be right, and some might be half and half right. You will have to evaluate the best possible approach to fix that patch. The impact of a patch mentioned above will play a big role here. This kinda thinking will improve with time, but it ties up to the fact that, as a reviewer, you will have to put your brain in front of the patch to confirm the approach rather than trusting the approach of the given patch entirely and just reviewing as is. |
| 44 | + |
| 45 | +### Testing instructions review |
| 46 | + |
| 47 | +Leverage your developer expertise to anticipate potential ambiguities in testing instructions. Then, test as if you were a user with no prior knowledge, focusing on identifying gaps and areas requiring clarification. This process can also reveal underlying UX issues. |
| 48 | + |
| 49 | +### Involve UI/UX people |
| 50 | + |
| 51 | +As we have a lot of help in terms of UI/UX at the moment, always involve them in string changes, UI changes, etc. That endorses the work and your review as well then accepting a patch. The `ux_review_required` and `ux_writing` [labels](https://moodledev.io/general/development/tracker/labels) are there for a reason, use them. |
| 52 | + |
| 53 | +### Pushing code to help |
| 54 | + |
| 55 | +While reviewing, pushing a patch to help the assignee is a great one! That's particularly helpful when the change needed is significant or to show some code to explain a different approach. |
| 56 | + |
| 57 | +### Always cross-check Moodle's policy |
| 58 | + |
| 59 | +Moodle has a bunch of policies, about component communication, upgrades, and some more. These are independent of the coding style or PHP-related policy we have. Always review them and make sure they are not violated in a patch. |
| 60 | + |
| 61 | +:::tip |
| 62 | + |
| 63 | +Please note, as a reviewer, you will get many many different types of patches, some will be easy, some will be complex, some will make you crazy, and some might make you put your head in a bucket full of water and stop breathing, regardless, never be afraid to review a tracker and ask for help. Being a reviewer is a rewarding experience, and to be honest, reviewing something will help you learn faster than writing something. Good luck reviewers! |
| 64 | + |
| 65 | +::: |
| 66 | + |
| 67 | +## CLR specific |
| 68 | + |
| 69 | +### Pushing a patch to IR |
| 70 | + |
| 71 | +Pushing a patch to IR to someone else is not a shame or a topic of fear or skills. You looked at a patch and you didn't feel comfortable pushing it and didn't feel comfortable posting your comments, not a problem. Add your comments as a restricted note in the tracker or your own, then cross-check with the person who reviewed it. There are two ways to learn, making a mistake and learning from it, and learning then not making a mistake, you decide! |
| 72 | + |
| 73 | +### Rejecting a patch |
| 74 | + |
| 75 | +Patches can be rejected, and it's not a big deal. They can be pushed back to IR with comments but, sometimes, you can easily say if a patch is wrong and the reported issue is not an issue. When in doubt, take advantage of the internal channels and discuss these when needed instead of rejecting it or pushing it back to IR. |
| 76 | + |
| 77 | +### Pushing back |
| 78 | + |
| 79 | +When you review a tracker and there is a possibility that it will come back with a large amount of change or the entire approach is not right, you can always push back and ask them to take time, do it properly PR again and send back to CLR/IR. But it's a judgment call. |
| 80 | + |
| 81 | +### Be mindful about pushing with critical information |
| 82 | + |
| 83 | +There will be patches that will include developer docs, mention that in your comment while pushing so that whoever merges that, can also merge the developer docs and never forget about that. In cases where the Assignee field is empty, which typically occurs with a user's first issue, please explicitly mention that within your comment. This ensures awareness for adding them manually to the proper Tracker group once the issue is closed. |
0 commit comments