-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add player age eligibility validation for leagues #38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: Add player age eligibility validation for leagues #38
Conversation
Add functions to validate if a player is eligible to participate in a league by comparing their dateOfBirth against the league's minAge and maxAge restrictions: - calculateAge: Utility to compute age from date of birth - checkPlayerEligibility: Returns validation result with errors - validatePlayerEligibility: Throws error if ineligible - isPlayerEligible: Returns boolean for eligibility Includes comprehensive tests for all edge cases including boundary conditions, missing data, and invalid formats.
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
WalkthroughThis PR introduces age-based player eligibility validation with four new utility functions (calculateAge, checkPlayerEligibility, validatePlayerEligibility, isPlayerEligible) in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant ValidateFunc as validatePlayerEligibility()
participant CheckFunc as checkPlayerEligibility()
participant CalcFunc as calculateAge()
Caller->>ValidateFunc: (dateOfBirth, minAge, maxAge, referenceDate)
ValidateFunc->>CheckFunc: (dateOfBirth, minAge, maxAge, referenceDate)
CheckFunc->>CalcFunc: (dateOfBirth, referenceDate)
CalcFunc-->>CheckFunc: age (number | null)
alt Valid age constraints
CheckFunc->>CheckFunc: Validate dateOfBirth format
CheckFunc->>CheckFunc: Check age against min/max
CheckFunc-->>ValidateFunc: ValidationCheckResult (valid)
ValidateFunc-->>Caller: Success (void)
else Invalid or age out of bounds
CheckFunc-->>ValidateFunc: ValidationCheckResult (errors)
ValidateFunc->>Caller: Throw Error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/validation.test.ts (1)
967-1006: Good coverage, but missing timezone edge case tests.The test suite covers the main scenarios well. However, it doesn't expose the timezone bug in
calculateAgebecause all tests use explicit date strings forreferenceDate(which are parsed as UTC, matching the UTC parsing ofdateOfBirth).Consider adding a test that uses the current time in a specific timezone to ensure the fix handles timezone correctly:
it('should handle timezone correctly when using current time', () => { // Test that would expose the timezone bug if not using UTC methods const birthDate = '2000-01-01'; const age = calculateAge(birthDate, new Date(2024, 0, 1, 12, 0, 0)); // January 1, 2024 noon local time expect(age).toBe(24); });This test would fail with the current implementation in timezones west of UTC.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/assets/navigation.js(1 hunks)docs/assets/search.js(1 hunks)docs/functions/calculateAge.html(1 hunks)docs/functions/checkPlayerEligibility.html(1 hunks)docs/functions/isPlayerEligible.html(1 hunks)docs/functions/validatePlayerEligibility.html(1 hunks)src/validation.ts(1 hunks)test/validation.test.ts(2 hunks)
🔇 Additional comments (12)
docs/assets/navigation.js (1)
1-1: LGTM - Generated documentation artifact.This is a generated navigation data payload that reflects the new API surface. No action required.
docs/assets/search.js (1)
1-1: LGTM - Generated documentation artifact.This is a generated search index that reflects the new API surface. No action required.
docs/functions/calculateAge.html (1)
1-5: LGTM - Generated documentation page.The documentation accurately describes the
calculateAgefunction's parameters and return type. No action required.docs/functions/checkPlayerEligibility.html (1)
1-7: LGTM - Generated documentation page.The documentation accurately describes the
checkPlayerEligibilityfunction's parameters and return type. No action required.docs/functions/validatePlayerEligibility.html (1)
1-7: LGTM - Generated documentation page.The documentation accurately describes the
validatePlayerEligibilityfunction's throwing behavior. No action required.docs/functions/isPlayerEligible.html (1)
1-7: LGTM - Generated documentation page.The documentation accurately describes the
isPlayerEligiblefunction's boolean return value. No action required.src/validation.ts (3)
523-560: LGTM - Solid validation logic.The age eligibility validation correctly handles all edge cases:
- Returns no errors when no age restrictions are set
- Validates that dateOfBirth is provided when restrictions exist
- Delegates to
calculateAgefor date parsing and age computation- Checks both minimum and maximum age constraints with clear error messages
The use of
!= nullproperly catches both null and undefined values.Note: This function depends on
calculateAge, which has a timezone issue flagged separately.
570-578: LGTM - Clean throwing wrapper.The function follows the established validation pattern in the codebase by delegating to
checkPlayerEligibilityand throwing viaresult.validate().
588-596: LGTM - Useful boolean convenience method.The function provides a clean boolean API by delegating to
checkPlayerEligibilityand converting the result to true/false based on error presence.test/validation.test.ts (3)
1008-1138: LGTM - Comprehensive test coverage.The test suite for
checkPlayerEligibilityis excellent, covering:
- No restrictions scenario
- Missing/invalid date of birth handling
- Minimum and maximum age boundaries
- Combined age range validation
- Edge cases (exact age requirements, zero minimum age)
The tests verify both the absence of errors in valid cases and the presence of specific error messages in invalid cases.
1140-1165: LGTM - Adequate coverage for throwing wrapper.The tests verify the key throwing behaviors:
- No exception for eligible players
- Correct exception messages for ineligible players (too young, too old, missing DOB)
- No exception when no restrictions exist
1167-1197: LGTM - Complete boolean wrapper coverage.The tests verify the boolean return values across all scenarios:
- True for eligible players
- False for all ineligibility reasons
- Partial restriction handling (minAge-only, maxAge-only)
Add functions to validate if a player is eligible to participate in a league by comparing their dateOfBirth against the league's minAge and maxAge restrictions:
Includes comprehensive tests for all edge cases including boundary conditions, missing data, and invalid formats.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.