Code Review Checklist - Simon-Initiative/oli-torus GitHub Wiki
Torus PR Code Review Checklist
Quick Summary
- Purpose and Clarity: PRs should have a clear, defined purpose and a detailed description of what was changed and why
- Organization, Documentation: Code changes should be well organized and include comments
- Functional Correctness: Manual testing by the reviewer AND unit tests prove that the feature works or the bug is fixed
- Error Handling and Logging: Exceptional returns and errors and handled at appropriate levels, logging is present and useful
- Performance and Scalability: All performance and scalability best practices are followed
Detailed Guidelines
-
Purpose and Clarity
- Is the purpose of the PR clearly explained in the description?
- For Bug Fix PRs, ensure the PR description includes:
- A detailed explanation of the diagnosed root cause.
- A clear description of what was done to fix the issue.
- A description like "This fixes a bug in course preview" is not acceptable.
- For Hot Fix PRs, ensure the PR is targeted to the correct branch (for instance,
hotfix-v0.XX.X
), as specified in the JIRA ticket (source of truth)
-
Organization, Documentation
- Is the code well-organized and easy to read?
- Are functions appropriately named, concise, and follow consistent naming conventions?
- Is there unnecessary commented-out code, debug logs, or obsolete code included?
- Are there any instances of cyclic dependencies between modules? Or
Oli
code usingOliWeb
code? - Are modules, contexts, or components organized logically?
- Are there any opportunities for further modularization or code reuse?
-
Functional Correctness
- Have you the reviewer run the code locally and manually verified that it works?
- Are there unit tests and do they adequately cover the feature or bug fix?
- For LiveView UI based unit tests, are these truly necessary? Are they being used to verify non UI functionality?
-
Error Handling and Logging
- Does the code gracefully handle possible errors or edge cases?
- Are descriptive error messages returned where appropriate?
- Are useful logging messages at an appropriate logging level present?
-
Performance and Scalability
- Consider if a single, custom query could be written to replace reusing a series of existing queries
- For queries that are overly complex, consider breaking it down to multiple, simpler queries
- There should be no instances of DB queries being made within an
Enum.map
or any other type of loop - Aggregrated tables (
ResourceSummary
,ResponseSummary
) queries should be used in nearly all cases instead of querying acrossActivityAttempt
orPartAttempt
records - For Delivery code, the
SectionResourceDepot
cache should be used instead of directly running resolver and section resource queries for things like page titles, course hierarchy, schedule information, page details, etc. - LiveViews should be optimizing their load time by using async assigns
- LiveViews should only be storing in their assigns the information needed to render the view.