[Documentation] PR review process

[f-odhiambo]

This will be useful to make sure we have efficacy when managing open PRs. This can go into a MD doc later

Why are code reviews important and what makes a good code review?

1. Ensure that all engineers take part in the code review process
2. Good code reviews use the same quality bar and approach for everyone, regardless of their job title, level, or when they joined the company.
3. Better code reviews pay additional attention to making the first few reviews for new joiners a great experience.
   • Reviewers are empathetic to the fact that the recent joiner might not be aware of all the coding guidelines and might be unfamiliar with parts of the code.
   • Provide relevant documentation (guidelines, style guides, etc) to help engineer improve their code.
4. Ensure no code makes it to production without a code review & tests
5. PR and reviews should be time-bound. Ensure that PRs get reviewed, approved/rejected within a scrum cycle/sprint to avoid stale code
6. Foster a good code review culture in which finding defects is viewed positively, give respectful and constructive feedback

How to approach conducting a code review

1. Prioritize review over coding
2. Integrate code review into your daily routine
3. Review at least part of the code, even if you can't do all of it
4. Review fewer than 400 lines of code at a time
5. Take your time while reviewing someone else's code, do not depend on others to catch errors
6. Do not bunch PRs for review. Instead, scatter reviews tasks throughout the working hours
7. Ensure the PRs comes with tests
8. Build and test (can be in CI tool) — before code review
9. Ensure external documents if any (API, user manual, etc.) are updated
10. Give feedback that helps (not hurts)
11. Create and follow a code-review checklist.
12. While reviewing code, be mindful of the following:
    • Security best practices
    • Manageability (readability, structure, style)
    • Architecture
    • Maintainability (logic, test coverage)
    • Correctness (functionality)
    • Invalid input/states
    • Usability (performance)
    • Re-usability
    • Object-oriented analysis and design (OOAD) principles
13. Verify that the defects -- if any -- are fixed
14. PR rejections must always include an explanation for why the PR got rejected

Guidance when opening a PR for your code to be reviewed

1. There are various git workflow, get yourself acquainted with different git workflows
2. You should annotate PRs with self-explaining comments before the review
3. Create brief change logs
4. Never self-approve a PR, always ask your peers for approval after code review
5. Before opening a PR, check your commit log and squash or rename commits as useful to clarify the content of the commit.
6. Prior to making PRs always check to see you have merged the most updated code from the main branch. Update all affected tests and or create new tests for files you have changed.
7. Remove all unnecessary console logs prior to making a PR
8. A detailed Pull Request description

A detailed Pull Request description that addresses the following:

1. What has changed between his feature branch and the dev branch
2. Checklist of prerequisites (e.g. added documentation, added tests, ready to merge, etc.)
3. How to use the feature (a GIF of the feature in action is extremely useful here)
4. How the design has changed and how it compares to the design mockups (screenshots comparing the designs to the implementation)
5. Additional notes the reviewers should be made aware of

Further, we could also look at

• [ ] Integration or unit test provided
• [ ] Deprecated code removed
• [ ] Syntax & formatting is correct (linting should help with this)
• [ ] Is the approach to the problem appropriate?
• [ ] Can anything be simplified?
• [ ] Is the code too specific to the product and needs generalization?
• [ ] Do forms need to be sanitized
• [ ] Any obvious security flaws or potential holes
• [ ] Are the naming conventions appropriate?
• [ ] Are there enough comments in line with the code?
• [ ] Is there documentation needed?
• [ ] Is there anything included that should not be?
• [ ] Are all dependencies declared?
• [ ] Are there any code smells?

[f-odhiambo]

@pld @ndegwamartin @dubdabasoduba Kindly review and update

[pld]

Cool, made some edits and added details to the header names. What is this referring to, in reviewee notes?

Create small change lists, Change Lists Should Be Complete on Their Own

[f-odhiambo]

Cool, made some edits and added details to the header names. What is this referring to, in reviewee notes?

Create small change lists, Change Lists Should Be Complete on Their Own

Change logs is what we are referring to here

[pld]

ahh ok I'll update to call it that, think it looks good

[pld]

think this was closed by https://github.com/opensrp/fhircore/pull/588