fix(licenses): pkpass status on machine license #16679
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseMapper.ts (3)
12-13: Add type safety and documentation to the compareDates helper.The function logic is correct, but could benefit from additional type safety and documentation.
Consider applying these improvements:
+/** + * Compares two dates and returns the later one + * @param newDate - The date to compare + * @param latestDate - Optional reference date + * @returns The later date between the two + */ -const compareDates = (newDate: Date, latestDate?: Date) => +const compareDates = (newDate: Date | string, latestDate?: Date | string) => { + const newDateTime = new Date(newDate) + const latestDateTime = latestDate ? new Date(latestDate) : undefined + + if (isNaN(newDateTime.getTime())) { + throw new Error('Invalid newDate') + } + if (latestDateTime && isNaN(latestDateTime.getTime())) { + throw new Error('Invalid latestDate') + } + + return !latestDateTime || newDateTime > latestDateTime ? newDateTime : latestDateTime +}
20-30: Enhance type safety and readability of findLatestExpirationDate.The refactored implementation is cleaner and more maintainable.
Consider these improvements:
-export const findLatestExpirationDate = (license: VinnuvelaDto) => { +export const findLatestExpirationDate = (license: VinnuvelaDto): string | null => { if (!license.vinnuvelaRettindi) { return null } let latestDate: Date | undefined - for (const right of license.vinnuvelaRettindi) { + license.vinnuvelaRettindi.forEach((right) => { if (right.stjorna) { latestDate = compareDates(new Date(right.stjorna), latestDate) } if (right.kenna) { latestDate = compareDates(new Date(right.kenna), latestDate) } - } + }) return latestDate ? latestDate.toISOString() : null }
Line range hint
1-116: Add explicit return types to exported functions.The code demonstrates good TypeScript practices and reusability. However, some exported functions would benefit from explicit return type annotations.
Add return types to these functions:
-export const parseRightsForPkpassInput = ( +export const parseRightsForPkpassInput = ( rights?: Array<VinnuvelaRettindiDto>, locale?: Locale, -) => { +): string => { -export const createPkPassDataInput = ( +export const createPkPassDataInput = ( license: VinnuvelaDto, nationalId: string, locale?: Locale, -) => { +): Array<{ identifier: string; value: string }> | null => {libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.service.ts (1)
49-51: Add type safety for expiration date.The expirationDate variable lacks type information, which could lead to runtime issues.
- const expirationDate = licenseInfo + const expirationDate: string | null = licenseInfo ? findLatestExpirationDate(licenseInfo) : null
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.service.ts(2 hunks)libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseMapper.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseMapper.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (1)
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.service.ts (1)
49-62: Verify the impact of updated license validation logic.
The changes to license validation logic might affect other parts of the codebase that rely on this validation.
✅ Verification successful
The search results show that the LicensePkPassAvailability enum is consistently used across multiple license client services (hunting, firearm, machine, ADR, driving, and disability) with the same pattern of returning Unknown, NotAvailable, or Available based on license validation. The changes in the machine license client follow this same pattern and maintain consistency with other implementations.
Let me verify the specific implementation pattern by searching for the actual validation logic:
License validation changes maintain consistent behavior
The changes to the license validation logic in machineLicenseClient.service.ts follow the established pattern across all license client services. The findLatestExpirationDate function is properly scoped to the machine license module and is used consistently within its service. The validation logic maintains the same LicensePkPassAvailability enum states (Unknown, NotAvailable, Available) as other license client implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of LicensePkPassAvailability and potential impacts
# Test 1: Find all usages of LicensePkPassAvailability enum
echo "Checking LicensePkPassAvailability usage:"
rg "LicensePkPassAvailability\." -A 3
# Test 2: Find all implementations of checkLicenseValidityForPkPass
echo -e "\nChecking checkLicenseValidityForPkPass implementations:"
ast-grep --pattern 'checkLicenseValidityForPkPass($_) { $$$ }'
Length of output: 11570
Script:
#!/bin/bash
# Search for license validation implementations
echo "Checking license validation patterns:"
ast-grep --pattern 'checkLicenseValidityForPkPass($_) {
$$$
}'
# Search for findLatestExpirationDate usage
echo -e "\nChecking findLatestExpirationDate usage:"
rg "findLatestExpirationDate" -A 3
Length of output: 3293
...lients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.service.ts
Show resolved
Hide resolved
...lients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.service.ts
Show resolved
Hide resolved
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseMapper.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16679 +/- ##
==========================================
- Coverage 36.67% 36.63% -0.04%
==========================================
Files 6858 6872 +14
Lines 142787 143113 +326
Branches 40703 40788 +85
==========================================
+ Hits 52364 52436 +72
- Misses 90423 90677 +254
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 29 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Affected services are: api,license-api, Deployed services: api,license-api,consultation-portal,web,services-bff-portals-admin. |
Datadog ReportAll test runs ✅ 3 Total Test Services: 0 Failed, 2 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
…achine-license' into fix/pkpass-status-machine-license
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.spec.ts (1)
10-10: Consider using a more reasonable test date.The test uses a hardcoded date far in the future (2080). While this works, it's better to use a more reasonable date range to prevent the test from becoming outdated or misleading.
-const expectedDate = new Date('2080-01-01T00:00:00Z').toISOString() +const expectedDate = new Date(Date.now() + 365 * 24 * 60 * 60 * 1000).toISOString() // One year from now
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/clients/license-client/src/lib/clients/machine-license-client/machineLicenseClient.spec.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
| describe('get latest license qualification expiration date', () => { | ||
| it('should return the latest license right date', async () => { | ||
| const result = mapper.findLatestExpirationDate(ValidMachineLicense) | ||
|
|
||
| const expectedDate = new Date('2080-01-01T00:00:00Z').toISOString() | ||
| const expectedDate = new Date('2080-01-01T00:00:00Z').toISOString() | ||
|
|
||
| expect(result).toMatch(expectedDate) | ||
| }) | ||
| it('should return undefined if no license info', async () => { | ||
| const result = mapper.findLatestExpirationDate({}) | ||
| expect(result).toMatch(expectedDate) | ||
| }) | ||
| it('should return undefined if no license info', async () => { | ||
| const result = mapper.findLatestExpirationDate({}) | ||
|
|
||
| expect(result).toBeNull | ||
| expect(result).toBeNull | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional test cases.
The current test suite could benefit from additional test cases to ensure robust functionality:
- Test with multiple dates to verify the latest date is correctly identified
- Test with invalid date formats
- Test with empty date fields
Here's a suggested addition to improve coverage:
it('should handle multiple dates and return the latest', () => {
const licenseWithMultipleDates = {
...ValidMachineLicense,
qualifications: [
{ expiryDate: '2023-01-01T00:00:00Z' },
{ expiryDate: '2023-12-31T00:00:00Z' },
{ expiryDate: '2023-06-01T00:00:00Z' }
]
}
const result = mapper.findLatestExpirationDate(licenseWithMultipleDates)
expect(result).toBe('2023-12-31T00:00:00Z')
})
it('should handle invalid date formats gracefully', () => {
const licenseWithInvalidDate = {
...ValidMachineLicense,
qualifications: [
{ expiryDate: 'invalid-date' }
]
}
const result = mapper.findLatestExpirationDate(licenseWithInvalidDate)
expect(result).toBeNull()
})| @@ -1,34 +1,21 @@ | |||
| import * as mapper from './machineLicenseMapper' | |||
| import ExpiredMachineLicense from '../__mock_data/expiredMachineLicense.json' | |||
| import ValidMachineLicense from '../__mock_data/validMachineLicense.json' | |||
|
|
|||
| describe('license-service/client/machine-license', () => { | |||
| describe('check if expired', () => { | |||
There was a problem hiding this comment.
Update the describe block description.
The describe block "check if expired" is misleading as the tests now focus on finding the latest expiration date rather than checking if a license is expired.
-describe('check if expired', () => {
+describe('license expiration date handling', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('check if expired', () => { | |
| describe('license expiration date handling', () => { |
|
|
||
| expect(result).toBeNull | ||
| expect(result).toBeNull |
There was a problem hiding this comment.
Fix the assertion syntax error.
The assertion is missing parentheses which makes it ineffective. The test is currently checking if the toBeNull matcher exists rather than if the value is null.
-expect(result).toBeNull
+expect(result).toBeNull()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(result).toBeNull | |
| expect(result).toBeNull() |
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
checkLicenseExpirationDatefunction and simplified date handling in thefindLatestExpirationDatefunction.