...
In order to achieve this, there are a few different things the reviewer will bear in mind.
Generic Guidelines
Reviewer/Reviewee
In general, a review will be carried out by a developer who has more experience (either in the framework, module, or both) than the developer who carried out the work. However this won’t necessarily always be the case, and the context of who did the work and who is reviewing can be useful for gauging the best way for both reviewer and developer to take something from the interaction.
Context
Understanding the context of a code change is important. A change in isolation may seem like a good one, swapping out a destructured prop name from thisIsAPorp to thisIsAProp everywhere it’s used within the module. However if that module is a shared library such as stripes-erm-components, then it’s possible that the property is in use elsewhere, and it represents a breaking change.
This is just an example, but the layers of context can change what seems like a perfectly reasonable change into one that causes issues. The layers in particular:
Framework - is it react, grails, inside or outside FOLIO?
Module - is the change in mod-agreements or mod-serials-interaction, does that make a difference to code practices?
Location - Are the changes in the correct place? Should this be centralised or localised?
Function - Code for displaying something to a user will need different attention to code fetching something from the backend, for example.
Code - Final layer of context is the code itself. Are there any antipatterns, does the logic make sense at a glance? Are comments in place?
Testing
There are guidelines in place around FOLIO that a testing threshold of 80% is expected. This is in placfe to try and ensure a good and responsible test coverage across all modules, but is often substituted for the same. In reality it is up to the developer to ensure that tests are in place for any functionality, or bug fix to ensure that changes elsewhere would not impact the component or class at hand.
From a review standpoint, the main criteria are whether or not tests exist and are passing, but a cursory glance can help identify cases where a developer has potentially missed something important that ought to be tested.
Are tests needed?
If a change has been made to code, that probably should break tests.
If a change has happened but no tests are changed or added, either the fix/feature is not tested – which should be asked of the developer, or the tests in place do not actually test the functionality as they should - which should again go back to the developer.
What sort of tests are required?
Changes in frontend will require cypress tests and/or jest tests
Jest tests should do more than just check the interior stripes components render and work as expected (filling out fields etc)
In general, if a component “does something”, like check if one field is filled to give a warning on another field, that should be tested
Cypress tests should set up and then delete all the data needed for them to run, so they can run on any system without clogging it up entirely (Still don’t run against live production builds)
Communication
Developers can obviously be contacted both directly via the PR/JIRA ticket or through other means such as Slack etc. In general, a problem and worked through solution should be trackable in future, so it is important that at the very least a record of decision making and questions/answers is available somewhere. The Jira ticket and/or the PR are probably the best places for those, with any particularly complicated or “weird” decisions possibly warranting a comment in the code itself for the next developer.
...
In a similar fashion, if code in one place is known by the reviewer to match or have impacts elsewhere, a comment should be made on the PR to check the other places. This should indicate that a comment is needed in the code to point the developer at those hidden dependencies, and tests ought to catch and fail at situations like this.
Time crunch
Certain decisions can be made in context that would otherwise be advised against. Tests can be skipped, slightly wonky code can be allowed through, solutions that should be centralised can be left local.
If up against tight time or resourcing constraints, sometimes it’s the right thing to do to allow these things through the net. However it is then up to the reviewer to raise them with the team at meetings and ensure that follow up work to recover the tech debt is not lost.
Module Specific guidelines
TBC - ui-serials has centralised testing etc etc