fix(license-service): check app version #16855
Conversation
WalkthroughThe changes in this pull request involve the addition of user agent handling across several components of the license service. This includes updates to method signatures in various classes to accept an optional 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 (6)
libs/api/domains/license-service/src/lib/utils/appCompatibilityMode.ts (2)
5-7: Add debug logging for version comparison failuresConsider adding debug logging when version comparison fails due to missing parameters. This will help with troubleshooting in production.
+ // Return false if either version is missing if (!version || !versionToCompare) { + console.debug('Version comparison failed: missing version parameters', { + version, + versionToCompare, + }); return false }
9-12: Consider using a dedicated version comparison libraryWhile
localeCompareworks for basic version comparison, consider using a dedicated library likesemverfor more robust version comparison handling. This would provide better handling of edge cases and pre-release versions.+import * as semver from 'semver'; + export const enableAppCompatibilityMode = ( version?: string, versionToCompare?: string, ): boolean => { if (!version || !versionToCompare) { return false } - const isVersionLater = version.localeCompare(versionToCompare, undefined, { - numeric: true, - sensitivity: 'base', - }) - return isVersionLater > 0 + return semver.gt(version, versionToCompare) }libs/api/domains/license-service/src/lib/resolvers/licenseCollection.resolver.ts (1)
73-73: Consider extracting the hardcoded license typeThe default value
['DriversLicense']should be extracted into a constant or configuration to improve maintainability and reusability.+const DEFAULT_LICENSE_TYPES = ['DriversLicense'] as const; + @Query(() => LicenseCollection, { name: 'genericLicenseCollection', }) @Audit() async genericLicenseCollection( @CurrentUser() user: User, @ParsedUserAgent() agent: UserAgent, @Args('locale', { type: () => String, nullable: true }) locale: Locale = 'is', @Args('input') input: GetGenericLicensesInput, ) { const licenseCollection = await this.licenseServiceService.getLicenseCollection( user, locale, { ...input, - includedTypes: input?.includedTypes ?? ['DriversLicense'], + includedTypes: input?.includedTypes ?? DEFAULT_LICENSE_TYPES, excludedTypes: input?.excludedTypes, force: input?.force, onlyList: input?.onlyList, }, agent, ) return licenseCollection }libs/api/domains/license-service/src/lib/licenceService.type.ts (1)
149-149: Well-structured interface modification!The optional
userAgentparameter is a good addition:
- Maintains backward compatibility by being optional
- Properly typed using the imported
UserAgenttype- Aligns with the PR's objective of implementing version checking
Consider documenting the parameter's purpose in a JSDoc comment to help other developers understand when and why they should provide the user agent information.
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts (1)
Line range hint
108-132: Reduce code duplication in properties mapping.The properties mapping logic is duplicated with only minor differences in field type and visibility. This increases maintenance burden.
Consider refactoring to reduce duplication:
- properties && enableAppCompatibility - ? { - type: GenericLicenseDataFieldType.Group, - hideFromServicePortal: true, - label: formatMessage(m.firearmProperties), - fields: (properties.properties ?? []).map((property) => ({ - type: GenericLicenseDataFieldType.Category, - fields: this.parseProperties( - property, - formatMessage, - )?.filter(isDefined), - })), - } - : null, - properties - ? { - type: GenericLicenseDataFieldType.Table, - label: formatMessage(m.firearmProperties), - fields: (properties.properties ?? []).map((property) => ({ - type: GenericLicenseDataFieldType.Category, - fields: this.parseProperties( - property, - formatMessage, - )?.filter(isDefined), - })), - } - : null, + properties + ? { + type: enableAppCompatibility + ? GenericLicenseDataFieldType.Group + : GenericLicenseDataFieldType.Table, + ...(enableAppCompatibility && { hideFromServicePortal: true }), + label: formatMessage(m.firearmProperties), + fields: (properties.properties ?? []).map((property) => ({ + type: GenericLicenseDataFieldType.Category, + fields: this.parseProperties( + property, + formatMessage, + )?.filter(isDefined), + })), + } + : null,libs/api/domains/license-service/src/lib/licenseService.service.ts (1)
145-145: Consider using consistent parameter namingThe parameter name 'agent' differs from 'userAgent' used in getLicenseCollection. Consider using consistent naming across methods for better maintainability.
- agent?: UserAgent, + userAgent?: UserAgent,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
libs/api/domains/license-service/src/lib/licenceService.type.ts(2 hunks)libs/api/domains/license-service/src/lib/licenseService.service.ts(5 hunks)libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts(2 hunks)libs/api/domains/license-service/src/lib/resolvers/licenseCollection.resolver.ts(2 hunks)libs/api/domains/license-service/src/lib/utils/appCompatibilityMode.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/api/domains/license-service/src/lib/licenceService.type.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/api/domains/license-service/src/lib/licenseService.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/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.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/api/domains/license-service/src/lib/resolvers/licenseCollection.resolver.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/api/domains/license-service/src/lib/utils/appCompatibilityMode.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 (13)
libs/api/domains/license-service/src/lib/utils/appCompatibilityMode.ts (1)
14-14: Verify usage patterns across the codebase
Let's ensure this function is being used consistently across the codebase and that the version comparison behavior meets all use cases.
✅ Verification successful
Version comparison logic is correctly implemented and used
The implementation and usage of enableAppCompatibilityMode is consistent and appropriate:
- The function is used in
firearmLicenseMapper.tsto enable specific UI features for app versions later than '1.4.7' - The version comparison using
localeComparewithnumeric: trueis the correct approach for semantic version comparison - The function properly returns
trueonly when the provided version is greater than the comparison version - No other version comparison implementations exist in the codebase that could conflict with this approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of enableAppCompatibilityMode
echo "Searching for function usage patterns..."
rg "enableAppCompatibilityMode" -A 2
# Search for other version comparison implementations
echo "Checking for other version comparison implementations..."
rg "localeCompare.*version" --type typescript
# Look for version-related constants or configurations
echo "Searching for version-related constants..."
rg "VERSION|version" --type typescript --glob "libs/api/domains/license-service/**/*"
Length of output: 1499
Script:
#!/bin/bash
# Search for the implementation of enableAppCompatibilityMode
echo "Checking the implementation:"
cat libs/api/domains/license-service/src/lib/utils/appCompatibilityMode.ts
# Search for usage in firearmLicenseMapper
echo -e "\nChecking usage in mapper:"
cat libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts
# Search for version comparisons using ripgrep without type filter
echo -e "\nChecking other version comparisons:"
rg "localeCompare.*version"
# Search for version-related constants without type filter
echo -e "\nSearching version constants:"
rg -g "*.{ts,js}" "VERSION|version" libs/api/domains/license-service/
Length of output: 10657
libs/api/domains/license-service/src/lib/resolvers/licenseCollection.resolver.ts (3)
12-12: LGTM! Import changes are well-structured
The new imports follow TypeScript best practices and maintain the modular architecture by using internal island.is packages.
Also applies to: 18-19
26-29: LGTM! Constructor follows DI best practices
The logger injection is properly implemented using dependency injection with appropriate access modifiers.
62-62: Verify user agent handling implementation
The changes add user agent support to the license collection flow. Let's verify the implementation across the service.
Also applies to: 68-79
✅ Verification successful
User agent handling is properly implemented across the service
The verification shows that UserAgent is consistently implemented:
- UserAgent type is imported from '@island.is/nest/core' across multiple files
- It's properly passed through from resolver to service layer
- It's handled in relevant mappers and type definitions
- The implementation is consistent across the license collection flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent user agent handling across the license service
# Check if UserAgent is properly handled in the service layer
echo "Checking service layer implementation..."
ast-grep --pattern 'getLicenseCollection($$$, agent: UserAgent)' libs/api/domains/license-service/src/lib/licenseService.service.ts
# Check if UserAgent type is properly defined
echo "Checking UserAgent type definition..."
rg -t typescript "export (type|interface) UserAgent" libs/api/domains/license-service/src
# Check for other methods that might need user agent support
echo "Checking for similar methods that might need user agent support..."
rg -t typescript "getLicenses|fetchLicenses|retrieveLicenses" libs/api/domains/license-service/src
Length of output: 715
Script:
#!/bin/bash
# Let's try a different approach to verify user agent handling
# Check for UserAgent usage in the codebase
echo "Checking UserAgent usage..."
rg "UserAgent" libs/api/domains/license-service/src
# Check for methods related to license collection
echo "Checking license collection methods..."
rg "getLicenseCollection|licenseCollection" libs/api/domains/license-service/src
# Look for files that might contain the UserAgent type definition
echo "Looking for potential UserAgent type files..."
fd -e ts -e tsx . libs/api/domains/license-service/src
Length of output: 5941
libs/api/domains/license-service/src/lib/licenceService.type.ts (1)
5-5: LGTM! Clean import from internal package.
The import statement follows TypeScript best practices and maintains good code organization by importing from the shared core package.
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts (3)
24-25: LGTM: Import statements are correctly placed and utilized.
The new imports are properly organized and are actively used within the code.
33-33: LGTM: Method signature updated correctly.
The optional userAgent parameter is properly typed and aligns with the interface changes.
Line range hint 24-132: Verify the impact on existing clients.
The changes introduce version-based conditional rendering of firearm properties. Please ensure that:
- Existing clients continue to see the Table view
- Only clients with version >= 1.4.7 see the Group view
✅ Verification successful
Let me generate additional scripts to verify the handling of both views (Table and Group) in the client code.
Changes are safely implemented with proper version handling
The implementation correctly handles both views:
- The Group view is marked with
hideFromServicePortal: trueand only shown for app version >= 1.4.7 - The Table view remains as the default view for existing clients and service portal
- Other license types (driving, machine, ADR) using Group view are unaffected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts on existing clients
# Search for other uses of GenericLicenseDataFieldType.Table
rg "GenericLicenseDataFieldType.Table" -A 5
# Search for other firearm property renderers
rg "firearmProperties" -A 5
Length of output: 4092
Script:
#!/bin/bash
# Check how GenericLicenseDataFieldType.Group is handled in the client
rg "GenericLicenseDataFieldType.Group" -A 5
# Check the version compatibility logic implementation
ast-grep --pattern 'enableAppCompatibilityMode($_, $_)'
Length of output: 4146
libs/api/domains/license-service/src/lib/licenseService.service.ts (5)
50-50: LGTM: Import statement is correctly placed
The addition of the UserAgent import from the nest/core package is appropriate for the new user agent handling functionality.
100-100: LGTM: Method signature updated correctly
The optional userAgent parameter is properly typed and follows TypeScript conventions for optional parameters.
112-112: LGTM: Proper parameter forwarding
The userAgent parameter is correctly forwarded to the getLicensesOfType method, ensuring proper propagation of user agent information through the service.
193-193: LGTM: Proper parameter forwarding to mapper
The agent parameter is correctly forwarded to the parsePayload method of the mapper, ensuring proper propagation of user agent information.
Line range hint 100-193: Verify backward compatibility with existing clients
The addition of optional user agent parameters should be verified to ensure it doesn't affect existing clients that don't provide this information.
✅ Verification successful
Let me analyze the usage patterns to verify backward compatibility. I'll check the actual calls to see if they pass the user agent parameter.
Optional user agent parameter is safely implemented and backward compatible
Based on the resolver implementations found, the changes are backward compatible because:
- The user agent parameter is optional in all method signatures
- The resolver implementations don't explicitly pass user agent, which verifies the service works without it
- The parameter is forwarded through the chain only when available, with no default values that could cause issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing service usage patterns to ensure backward compatibility
# Search for direct service usage
rg -A 2 "new LicenseService\("
# Search for service injection patterns
rg -A 2 "LicenseService\)"
# Search for method calls that might be affected
rg -A 2 "getLicenseCollection\(|getLicensesOfType\("
Length of output: 6000
Script:
#!/bin/bash
# Check the implementation of methods that call getLicenseCollection or getLicensesOfType
# Get the resolver implementations
ast-grep --pattern 'async $FUNC($$$) {
$$$
getLicenseCollection($$$)
$$$
}'
# Get the controller implementations
rg -B 5 -A 5 "getLicenseCollection\(" apps/services/license-api/src/app/modules/license/
Length of output: 174
Script:
#!/bin/bash
# Let's try a different approach to find the actual implementations
# Check resolver implementations
rg -B 5 -A 5 "getLicenseCollection\(" "libs/api/domains/license-service/src/lib/resolvers/"
# Check for any GraphQL query definitions that might show the parameter structure
ast-grep --pattern '@Query($$$)
$$$
async $FUNC($$$) {
$$$
getLicenseCollection($$$)
$$$
}'
# Look for test files that might show usage patterns
rg -B 5 -A 5 "getLicenseCollection\(" ".*\.spec\.ts$"
Length of output: 2843
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16855 +/- ##
==========================================
+ Coverage 35.66% 35.71% +0.04%
==========================================
Files 6947 6927 -20
Lines 147519 147005 -514
Branches 41942 41924 -18
==========================================
- Hits 52612 52499 -113
+ Misses 94907 94506 -401
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 216 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
Affected services are: api, Deployed services: api,consultation-portal,web,services-bff-portals-admin,services-bff-portals-my-pages. |
Datadog ReportAll test runs ✅ 3 Total Test Services: 0 Failed, 2 Passed |
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
libs/clients/firearm-license/src/lib/services/firearmApi.services.ts (2)
Line range hint
58-70: Remove commented-out code blocks.The presence of commented-out API calls across multiple methods creates technical debt and makes the code harder to maintain. If this is temporary for testing, consider using a proper mocking system or feature flags instead.
Consider these alternatives:
- Use dependency injection to swap implementations
- Implement a proper mock service
- Use feature flags to toggle between real and mock data
Also applies to: 90-102, 134-145
Line range hint
1-15: Type safety and structure look good, consider enhancing error types.The code follows TypeScript best practices and maintains good reusability. The error handling structure is particularly well-designed.
Consider extracting the error codes and messages into an enum or constant object for better maintainability:
export enum FirearmApiErrorCode { SERVICE_FAILURE = 13, UNKNOWN_ERROR = 99, } export const FirearmApiErrorMessage = { [FirearmApiErrorCode.SERVICE_FAILURE]: 'Service failure', [FirearmApiErrorCode.UNKNOWN_ERROR]: 'Unknown error', } as const;libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts (2)
27-27: Document the version constant's significance.Add a comment explaining why version 1.4.8 is significant for the firearm properties display compatibility.
+// Version 1.4.8 introduces changes in how firearm properties are displayed in the table format const APP_VERSION_CUTOFF = '1.4.8'
Line range hint
111-134: Reduce code duplication in property mapping.The property mapping logic is duplicated with only the type and hideFromServicePortal flag being different. Consider extracting the common logic.
+ const createPropertyFields = (type: GenericLicenseDataFieldType, hideFromServicePortal?: boolean) => ({ + type, + ...(hideFromServicePortal && { hideFromServicePortal }), + label: formatMessage(m.firearmProperties), + fields: (properties.properties ?? []).map((property) => ({ + type: GenericLicenseDataFieldType.Category, + fields: this.parseProperties(property, formatMessage)?.filter(isDefined), + })), + }) properties && enableAppCompatibility - ? { - type: GenericLicenseDataFieldType.Group, - hideFromServicePortal: true, - label: formatMessage(m.firearmProperties), - fields: (properties.properties ?? []).map((property) => ({ - type: GenericLicenseDataFieldType.Category, - fields: this.parseProperties(property, formatMessage)?.filter(isDefined), - })), - } + ? createPropertyFields(GenericLicenseDataFieldType.Group, true) : null, properties - ? { - type: GenericLicenseDataFieldType.Table, - label: formatMessage(m.firearmProperties), - fields: (properties.properties ?? []).map((property) => ({ - type: GenericLicenseDataFieldType.Category, - fields: this.parseProperties(property, formatMessage)?.filter(isDefined), - })), - } + ? createPropertyFields(GenericLicenseDataFieldType.Table) : null,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts(2 hunks)libs/clients/firearm-license/src/lib/services/firearmApi.services.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.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/firearm-license/src/lib/services/firearmApi.services.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 (2)
libs/clients/firearm-license/src/lib/services/firearmApi.services.ts (1)
72-84:
Security: Remove sensitive information from mock data.
The hardcoded mock data contains several concerns:
- Sensitive PII (SSN, names, addresses)
- Firearm-related information (serial numbers, license details)
- Non-realistic test data that could cause confusion
This poses security risks if accidentally deployed and makes testing less reliable.
Also applies to: 104-129, 147-163
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts (1)
34-35: LGTM! Method signature updated correctly.
The optional userAgent parameter is properly typed and follows the interface definition.
libs/api/domains/license-service/src/lib/mappers/firearmLicenseMapper.ts
Outdated
Show resolved
Hide resolved
…sion-mode' into fix/firearm-app-version-mode
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation