feat(air-discount-scheme): Add myflug + add more logs to ads #16655
Conversation
WalkthroughThe changes in this pull request include the addition of the airline Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)charts/islandis/values.prod.yaml (1)
The secret path Let's verify the consistency of airline API key configurations: ✅ Verification successfulThe MYFLUG_API_KEY secret path follows the consistent pattern across all environments The verification confirms that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify all airline API key secret paths follow the same pattern
# Expected: All airline API keys should follow the pattern /k8s/air-discount-scheme/backend/*_API_KEY
rg "air-discount-scheme/backend/.*_API_KEY" -A 1 -B 1
Length of output: 3752 charts/islandis/values.dev.yaml (1)
The addition of the MYFLUG_API_KEY secret follows the established pattern for airline API keys in the air-discount-scheme-backend service configuration. This aligns with the PR objective of adding Myflug as a new airline. 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
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
libs/air-discount-scheme/consts/src/lib/flight.ts (1)
Line range hint
1-19: Consider adding TypeScript enum or union type for Airlines.To improve type safety and autocompletion, consider defining a TypeScript type for the airlines.
Add this type definition above the constants:
export type Airline = 'ernir' | 'icelandair' | 'norlandair' | 'myflug'; // Then update the Airlines const to use the type export const Airlines: Record<Airline, Airline> = { ernir: 'ernir', icelandair: 'icelandair', norlandair: 'norlandair', myflug: 'myflug', }apps/air-discount-scheme/backend/src/app/modules/common/test/unit/auth.guard.spec.ts (1)
45-55: LGTM! Consider enhancing test description consistency.The test case for Myflug follows the established pattern and correctly validates the API key integration. The implementation is clean and well-structured.
For consistency with other test descriptions, consider using the airline's official name "Mýflug" in the test description:
- it('Api key should work for Myflug', async () => { + it('Api key should work for Mýflug', async () => {apps/air-discount-scheme/backend/infra/air-discount-scheme-backend.ts (1)
Line range hint
1-67: Consider enhancing observability configuration.Since one of the PR objectives is to enhance logging, consider adding structured logging configuration to the service setup. This could include:
- Log aggregation configuration
- Log level configuration per environment
- Metrics endpoint configuration for monitoring
Would you like assistance in implementing these observability enhancements?
libs/service-portal/air-discount/src/lib/messages.ts (1)
58-61: LGTM! Consider adding JSDoc comment for better documentation.The new error message follows the established pattern and integrates well with the existing messages. The ID and default message are consistent with the naming conventions.
Consider adding a JSDoc comment to document the usage context:
+/** + * Message displayed when the discount code generation fails. + * Used in the AirDiscountOverview component for error feedback. + */ codeGenFailed: { id: 'sp.air-discount:code-generation-fail', defaultMessage: 'Ekki tókst að útbúa kóða', },apps/air-discount-scheme/README.md (1)
24-24: Consider documenting the API key configuration.Based on the changes mentioned in the summary, there's a new
MYFLUG_API_KEYconfiguration. Consider adding this to the development setup section to ensure completeness of the documentation.Here's a suggested addition to the Development section:
1. Fetch the environment secrets: ```bash yarn get-secrets air-discount-scheme-api yarn get-secrets air-discount-scheme-backend yarn get-secrets air-discount-scheme-web + +# Required environment variables include: +# - MYFLUG_API_KEY: API key for Myflug airline integration</blockquote></details> <details> <summary>apps/air-discount-scheme/backend/src/app/modules/nationalRegistry/nationalRegistry.service.ts (1)</summary><blockquote> Line range hint `1-359`: **Consider architectural improvements for maintainability and security.** A few suggestions to improve the overall service architecture: 1. Move `TEST_USERS` array to a separate test data file 2. Add input validation for `authUser` parameter 3. Consider implementing caching for `getUser` results since the injected `cacheManager` is unused Would you like me to provide specific implementation details for any of these suggestions? </blockquote></details> <details> <summary>libs/service-portal/air-discount/src/screens/AirDiscountOverview/AirDiscountOverview.tsx (1)</summary><blockquote> `194-194`: **Consider adding error logging for failed code generation.** The fallback text provides good user feedback, but given the PR's objective to enhance logging, consider adding error logging when code generation fails. ```diff secondaryText={ item.user.fund?.credit === 0 ? undefined : item.discountCode ? item.discountCode - : formatMessage(m.codeGenFailed) + : (() => { + console.error('ADS: Failed to generate discount code for user', { + nationalId: item.nationalId, + userName: item.user.name + }); + return formatMessage(m.codeGenFailed); + })() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
apps/air-discount-scheme/README.md(1 hunks)apps/air-discount-scheme/backend/infra/air-discount-scheme-backend.ts(1 hunks)apps/air-discount-scheme/backend/seeders/20211108100835-development.js(1 hunks)apps/air-discount-scheme/backend/src/app/modules/common/test/unit/auth.guard.spec.ts(1 hunks)apps/air-discount-scheme/backend/src/app/modules/nationalRegistry/nationalRegistry.service.ts(1 hunks)apps/air-discount-scheme/backend/src/environments/environment.ts(2 hunks)apps/air-discount-scheme/web/screens/Admin/consts.ts(1 hunks)charts/islandis/values.dev.yaml(1 hunks)charts/islandis/values.prod.yaml(1 hunks)charts/islandis/values.staging.yaml(1 hunks)libs/air-discount-scheme/consts/src/lib/flight.ts(1 hunks)libs/portals/admin/air-discount-scheme/src/screens/Overview/consts.ts(1 hunks)libs/service-portal/air-discount/src/lib/messages.ts(1 hunks)libs/service-portal/air-discount/src/screens/AirDiscountOverview/AirDiscountOverview.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
apps/air-discount-scheme/README.md (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/air-discount-scheme/backend/infra/air-discount-scheme-backend.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/air-discount-scheme/backend/seeders/20211108100835-development.js (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/air-discount-scheme/backend/src/app/modules/common/test/unit/auth.guard.spec.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/air-discount-scheme/backend/src/app/modules/nationalRegistry/nationalRegistry.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/air-discount-scheme/backend/src/environments/environment.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/air-discount-scheme/web/screens/Admin/consts.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/air-discount-scheme/consts/src/lib/flight.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/portals/admin/air-discount-scheme/src/screens/Overview/consts.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/service-portal/air-discount/src/lib/messages.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/service-portal/air-discount/src/screens/AirDiscountOverview/AirDiscountOverview.tsx (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 (12)
libs/air-discount-scheme/consts/src/lib/flight.ts (1)
18-18: LGTM! Verify usage of the new airline constant.
The addition follows the existing pattern and naming convention.
Let's verify the usage of this new airline constant:
✅ Verification successful
The new airline constant is properly integrated across the codebase
The verification shows that the new myflug airline constant is:
- Correctly referenced in admin UI components with proper labeling as 'Mýflug'
- Properly configured in backend environment with API key setup
- Included in authentication tests
- Used consistently through ValueOf type references
No switch statements or hardcoded airline strings were found that would need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any potential issues with the new airline constant usage
# Test 1: Check if there are any hardcoded airline strings that should use the constant
rg -i "myflug|mýflug" --type ts --type tsx
# Test 2: Look for switch statements or if-else chains that might need updating
ast-grep --pattern 'switch ($expr) { $$$ case $airline: $$$ }'
Length of output: 137
Script:
#!/bin/bash
# Let's try a different approach to verify the usage
# Check for any references to myflug in TypeScript files
rg -i "myflug|mýflug" --type ts
# Look for switch statements in TypeScript files that handle airlines
rg "switch.*airlines?" --type ts -B 1 -A 5
# Check for any type definitions or interfaces that might need to be updated
rg "type.*Airlines|interface.*Airlines" --type ts -A 3
# Look for any test files that might need updating for the new airline
fd "test|spec" -e ts -x rg -i "airlines?" {} -A 2
Length of output: 5544
apps/air-discount-scheme/web/screens/Admin/consts.ts (1)
31-34: LGTM! Verify Airlines enum includes myflug.
The addition of Mýflug airline follows the existing pattern and maintains alphabetical order in the options list.
Let's verify the Airlines enum includes the myflug value:
libs/portals/admin/air-discount-scheme/src/screens/Overview/consts.ts (2)
31-34: LGTM! The new airline entry follows established patterns.
The addition of Mýflug maintains consistency with other airline entries and follows proper alphabetical ordering.
31-34: Verify the Airlines enum definition.
Let's ensure that the Airlines.myflug value is properly defined in the imported constants.
✅ Verification successful
Airlines.myflug is properly defined in the constants
The verification confirms that Airlines.myflug is correctly defined in libs/air-discount-scheme/consts/src/lib/flight.ts with the value 'myflug'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Airlines enum includes the myflug definition
# Test: Search for myflug in the air-discount-scheme constants
# Expect: Definition of myflug in the Airlines enum
ast-grep --pattern 'export const Airlines = {
$$$
myflug: $_,
$$$
}'
Length of output: 516
apps/air-discount-scheme/backend/src/environments/environment.ts (1)
12-12: LGTM! Development configuration for Myflug follows established patterns.
The addition of Myflug to the development configuration maintains consistency with other airlines and properly utilizes the Airlines enum.
apps/air-discount-scheme/backend/seeders/20211108100835-development.js (1)
36-37: Verify test coverage with the new airline addition.
The addition of 'myflug' to the airlines array is correct and aligns with the PR objectives. However, since this seeder is used for generating test data, we should verify that existing tests aren't making assumptions about specific airlines.
✅ Verification successful
Let me check the Airlines enum or constant definition to understand how airlines are handled in the codebase.
The seeder changes are safe and aligned with the codebase
The addition of 'myflug' to the seeder is correct as 'myflug' is already properly defined in the Airlines constant (libs/air-discount-scheme/consts/src/lib/flight.ts). The test files use the Airlines enum for type safety rather than hardcoded strings, which means they will automatically support the new airline. The only direct airline string references in tests are for API authorization tokens, which are independent of the seeder's random airline selection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests that might have hardcoded airline expectations
rg -g "*.test.*" -g "*.spec.*" "ernir|norlandair|icelandair" apps/air-discount-scheme/
Length of output: 4341
Script:
#!/bin/bash
# Search for Airlines enum or constant definition
ast-grep --pattern 'enum Airlines {
$$$
}'
# Also search for any other airline-related constant definitions
rg -g "*.ts" "const.*Airlines.*=.*{" -A 5
Length of output: 196
Script:
#!/bin/bash
# Search for Airlines type/enum/const definition with fixed regex
rg -g "*.ts" "Airlines.*=.*" -B 2 -A 2
# Also search for airline-related imports and types
rg -g "*.ts" "import.*Airlines.*from"
Length of output: 3378
Script:
#!/bin/bash
# Let's check the full content of the Airlines constant
cat libs/air-discount-scheme/consts/src/lib/flight.ts
Length of output: 422
apps/air-discount-scheme/backend/infra/air-discount-scheme-backend.ts (1)
10-10: LGTM! API key configuration follows established patterns.
The addition of MYFLUG_API_KEY follows the consistent pattern used for other airline API keys and maintains alphabetical ordering.
Let's verify the secret configuration across environments:
✅ Verification successful
API key configuration verified and follows established patterns
The MYFLUG_API_KEY configuration is correctly implemented:
- Follows the same path pattern
/k8s/air-discount-scheme/backend/as other airline API keys (ICELANDAIR, ERNIR, NORLANDAIR) - Maintains consistent naming convention with other airline API keys
- Is grouped together with other airline-related API keys in the configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that MYFLUG_API_KEY is properly configured across all environments
# Test: Check if the secret is configured in values files for all environments
echo "Checking secret configuration in values files..."
rg "MYFLUG_API_KEY" "charts/islandis/values*.yaml"
# Test: Ensure the secret path follows the established pattern
echo "Verifying secret path pattern..."
rg "/k8s/air-discount-scheme/backend/[A-Z_]+_API_KEY" "apps/air-discount-scheme/backend/infra/*.ts"
Length of output: 487
Script:
#!/bin/bash
# Let's try a different approach using fd to find the relevant files first
# Find all yaml files that might contain values
echo "Looking for values files..."
fd -e yaml values
# Find the infrastructure files
echo "Looking for infrastructure files..."
fd -e ts infra
# Once we find the files, let's check the API key patterns
echo "Checking API key patterns in found files..."
fd -e ts infra -x rg -l "API_KEY"
# Check for other airline API keys to compare patterns
echo "Checking other airline API keys..."
fd -e ts -x rg "/k8s.*API_KEY"
Length of output: 3041
apps/air-discount-scheme/README.md (1)
24-24: LGTM: Addition of Myflug to supported airlines list.
The change accurately reflects the addition of Myflug as a new supported airline.
libs/service-portal/air-discount/src/screens/AirDiscountOverview/AirDiscountOverview.tsx (1)
201-201: LGTM: Button visibility logic is well-implemented.
The condition correctly combines both credit check and code availability, preventing interaction when no valid code is available.
charts/islandis/values.staging.yaml (1)
188-188: LGTM: Myflug API key secret configuration added correctly
The addition of MYFLUG_API_KEY secret follows the established pattern used for other airline API keys (like ERNIR_API_KEY, ICELANDAIR_API_KEY, etc.) in the air-discount-scheme-backend service. This change aligns with the PR objective of adding Myflug as a new airline.
charts/islandis/values.prod.yaml (1)
180-180: LGTM: New Myflug API key secret follows established pattern
The addition of the MYFLUG_API_KEY secret follows the same path pattern as other airline API keys in the air-discount-scheme-backend service.
✅ Verification successful
✅ API key path follows consistent pattern across environments
The MYFLUG_API_KEY secret path /k8s/air-discount-scheme/backend/MYFLUG_API_KEY follows the exact same pattern as other airline API keys (Icelandair, Ernir, Norlandair) across all environments (dev, staging, prod) and infrastructure code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the secret path follows the same pattern as other airline API keys
rg "air-discount-scheme/backend/.*_API_KEY" -A 1 -B 1
Length of output: 3752
charts/islandis/values.dev.yaml (1)
188-188: LGTM! The Myflug API key secret is properly configured.
The new MYFLUG_API_KEY secret follows the established pattern for airline API keys in the air-discount-scheme-backend service. The secret path is consistent with other airline configurations.
Let's verify the configuration consistency across environments:
✅ Verification successful
Configuration verified: MYFLUG_API_KEY is consistently defined across all environments
The verification confirms that the MYFLUG_API_KEY secret is properly configured with the same path /k8s/air-discount-scheme/backend/MYFLUG_API_KEY across all environment value files:
- values.dev.yaml
- values.staging.yaml
- values.prod.yaml
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that Myflug API key is consistently configured across environments
# Test: Check if MYFLUG_API_KEY exists in all environment value files with the same path pattern
echo "Checking MYFLUG_API_KEY configuration across environments..."
rg "MYFLUG_API_KEY.*'/k8s/air-discount-scheme/backend/MYFLUG_API_KEY'" charts/islandis/values.*.yaml
Length of output: 621
apps/air-discount-scheme/backend/src/app/modules/nationalRegistry/nationalRegistry.service.ts
Show resolved
Hide resolved
* Add myflug * No code fallback. Improve logging when relation endpoint fails * Seq revert --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Add myflug + add more logs to ads
Why
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Localization
User Interface Improvements