Feat/fraud 3.1 #2681
Feat/fraud 3.1 #2681
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts (1)
Line range hint
26-45: Consider enhancing error monitoring for fraud detectionGiven this is part of a fraud detection system, consider adding structured logging or monitoring for:
- API version usage patterns
- Fraud detection rates
- Common error patterns
This would help track the effectiveness of different API versions and fraud detection rules.
api/src/pdc/providers/carpool/providers/CarpoolStatusService.ts (1)
Line range hint
1-71: Consider documenting API versioning strategyThe introduction of API versioning for fraud detection suggests a need for a clear versioning strategy. Consider:
- Documenting the differences between v3 and v3.1 fraud detection
- Adding migration guides for clients
- Planning for future version deprecation
api/src/pdc/providers/carpool/repositories/CarpoolLabelRepository.ts (1)
Line range hint
36-40: Consider improving parameter placement and type safety
- The
api_versionparameter would be more conventional as the last parameter.- Consider using a more specific type or enum for
api_versionto prevent invalid version strings.async findFraudByOperatorJourneyId( - api_version: string, operator_id: number, operator_journey_id: string, - client?: PoolClient, + client?: PoolClient, + api_version: ApiVersion, // Consider creating an enum or type ): Promise<Array<CarpoolLabel>>api/src/pdc/providers/carpool/repositories/CarpoolLabelRepository.integration.spec.ts (4)
14-24: LGTM! Consider grouping constants into objects for better maintainability.The label constants are well-organized by type. To improve maintainability, consider grouping them into objects by category.
- const anomaly_label_1 = "temporal_overlap_anomaly"; - const terms_label_1 = "distance_too_short"; - const fraud_label_1_V3 = "interoperator_fraud"; - const fraud_label_1_V3_1 = "interoperator_overlap_trip"; - const fraud_label_2_V3_1 = "interoperator_too_many_trips_by_day"; + const ANOMALY_LABELS = { + TEMPORAL_OVERLAP: "temporal_overlap_anomaly", + }; + const TERMS_LABELS = { + DISTANCE_TOO_SHORT: "distance_too_short", + }; + const FRAUD_LABELS = { + V3: { + INTEROPERATOR: "interoperator_fraud", + }, + V3_1: { + OVERLAP_TRIP: "interoperator_overlap_trip", + TOO_MANY_TRIPS: "interoperator_too_many_trips_by_day", + }, + };
28-55: Add error handling to database operations.The helper functions are well-structured, but they should include error handling for database operations to make debugging test failures easier.
const insertTermLabels = async (carpool_id: number) => { + try { await db.connection.getClient().query( sql`INSERT INTO ${raw(labelRepository.termsTable)} (carpool_id, labels) VALUES (${carpool_id}, ${[ terms_label_1, ]})`, ); + } catch (error) { + console.error(`Failed to insert terms label for carpool_id ${carpool_id}:`, error); + throw error; + } };
Line range hint
98-111: Remove duplicate test case for empty fraud labels.There are two nearly identical test cases checking for empty fraud labels. Consider combining them using test parameters for different versions.
- it("Should read carpool fraud label and returns empty array if none for api v3", async () => { - // Arrange - await repository.register({ ...insertableCarpool, operator_journey_id: "operator_journey_id-4" }); - // Act - const result = await labelRepository.findFraudByOperatorJourneyId( - "v3", - insertableCarpool.operator_id, - "operator_journey_id-4", - ); - // Assert - assertEquals(result, []); - }); - it("Should read carpool fraud label and returns empty array if none for api v3", async () => { - // Arrange - await repository.register({ ...insertableCarpool, operator_journey_id: "operator_journey_id-4" }); - // Act - const result = await labelRepository.findFraudByOperatorJourneyId( - "v3.1", - insertableCarpool.operator_id, - "operator_journey_id-4", - ); - // Assert - assertEquals(result, []); - }); + const versions = ["v3", "v3.1"]; + for (const version of versions) { + it(`Should return empty array for non-existent fraud labels (${version})`, async () => { + // Arrange + await repository.register({ ...insertableCarpool, operator_journey_id: "operator_journey_id-4" }); + // Act + const result = await labelRepository.findFraudByOperatorJourneyId( + version, + insertableCarpool.operator_id, + "operator_journey_id-4", + ); + // Assert + assertEquals(result, []); + }); + }Also applies to: 137-150
114-122: Standardize API version format.The test uses "v3.0" while other tests use "v3". Standardize the version format across all tests.
const result = await labelRepository.findFraudByOperatorJourneyId( - "v3.0", + "v3", insertableCarpool.operator_id, insertableCarpool.operator_journey_id, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
api/src/pdc/providers/carpool/providers/CarpoolStatusService.ts(2 hunks)api/src/pdc/providers/carpool/repositories/CarpoolLabelRepository.integration.spec.ts(2 hunks)api/src/pdc/providers/carpool/repositories/CarpoolLabelRepository.ts(3 hunks)api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts(2 hunks)api/src/shared/acquisition/status.contract.ts(2 hunks)
🔇 Additional comments (5)
api/src/shared/acquisition/status.contract.ts (2)
42-42: LGTM: Simple formatting improvement
The formatting change to the signature constant declaration improves readability while maintaining functionality.
4-4: Verify handling of breaking change in API contracts
The addition of a required api_version property is a breaking change that will affect all consumers of this interface. While the typing is correctly constrained to specific versions, we should ensure proper migration paths exist.
Let's verify the impact:
Consider:
- Adding migration documentation
- Implementing runtime validation to provide helpful error messages
- Consider making this optional with a default value to ease migration
api/src/pdc/services/acquisition/actions/StatusJourneyAction.ts (2)
1-1: LGTM: Import changes are consistent with new requirements
The addition of ContextType and updated contract imports properly support the new context-aware handler implementation.
Also applies to: 7-7
26-26: Verify handler usage across the codebase
The addition of the context parameter is a breaking change that could affect existing callers.
api/src/pdc/providers/carpool/providers/CarpoolStatusService.ts (1)
41-44: Verify integration with CarpoolLabelRepository
The api_version parameter is now used in fraud detection. Please ensure:
- All callers of this method are updated to provide the version
- The fraud detection logic in CarpoolLabelRepository correctly handles all version cases
✅ Verification successful
Let me gather more specific information about the fraud detection integration.
Based on the results, I can now provide a final response:
Integration with CarpoolLabelRepository is properly implemented
The api_version parameter is correctly integrated:
- The method is properly implemented in
CarpoolLabelRepositorywith version support - Integration tests demonstrate handling of different API versions (v3, v3.0, v3.1)
- The fraud detection is properly scoped to the specific journey through operator_id and operator_journey_id parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all calls to findByOperatorJourneyId to ensure they're updated
ast-grep --pattern 'findByOperatorJourneyId($$$)'
# Verify the implementation in CarpoolLabelRepository
ast-grep --pattern 'findFraudByOperatorJourneyId(api_version: $_, $$$)'
Length of output: 5120
Script:
#!/bin/bash
# Search for the implementation and usage of findFraudByOperatorJourneyId
rg "findFraudByOperatorJourneyId" -A 5
# Find all references to CarpoolLabelRepository
rg "CarpoolLabelRepository" -A 3
# Look for status service implementations that might call this method
ast-grep --pattern 'class $_ implements CarpoolStatusInterface {
$$$
}'
Length of output: 6864
| const { operator_journey_id, operator_id } = params; | ||
| const result = await this.statusService.findByOperatorJourneyId( | ||
| operator_id, | ||
| operator_journey_id, | ||
| context.call?.api_version_range || "3.1", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider centralizing API version configuration
The hardcoded version string "3.1" should be moved to a central configuration. This would make version management more maintainable and reduce the risk of inconsistencies.
Consider creating a shared constants file:
// src/shared/constants/api.ts
export const API_VERSIONS = {
DEFAULT: '3.1',
V3: 'v3',
V3_1: 'v3.1',
} as const;Then update the code:
- context.call?.api_version_range || "3.1",
+ context.call?.api_version_range || API_VERSIONS.DEFAULT,
api/src/pdc/providers/carpool/providers/CarpoolStatusService.ts
Outdated
Show resolved
Hide resolved
api/src/pdc/providers/carpool/repositories/CarpoolLabelRepository.ts
Outdated
Show resolved
Hide resolved
| // fraud V3.1 | ||
| it("Should read carpool fraud label and returns 2 labels for api v3.1", async () => { | ||
| const result = await labelRepository.findFraudByOperatorJourneyId( | ||
| "v3.1", | ||
| insertableCarpool.operator_id, | ||
| insertableCarpool.operator_journey_id, | ||
| ); | ||
| assertEquals(result, [ | ||
| { label: fraud_label_1_V3_1 }, | ||
| { label: fraud_label_2_V3_1 }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for edge cases in version handling.
The tests cover happy paths but are missing important edge cases for version handling.
Consider adding test cases for:
- Invalid version formats
- Unsupported versions
- Case sensitivity in version strings
Example:
it("Should handle invalid version format gracefully", async () => {
await assertRejects(
() => labelRepository.findFraudByOperatorJourneyId(
"invalid_version",
insertableCarpool.operator_id,
insertableCarpool.operator_journey_id,
),
Error,
"Invalid version format",
);
});
api/src/pdc/providers/carpool/repositories/CarpoolLabelRepository.integration.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
api/src/pdc/proxy/HttpTransport.ts (1)
Line range hint
553-556: Review rate limit configuration for security.The current rate limit of 20,000 requests per minute for the journey creation endpoint seems high, especially for a fraud-sensitive endpoint. Consider:
- Reducing the limit to a more conservative value
- Implementing additional fraud prevention measures like IP-based rate limiting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
api/src/pdc/proxy/HttpTransport.ts(1 hunks)
🔇 Additional comments (1)
api/src/pdc/proxy/HttpTransport.ts (1)
557-557: Verify client compatibility with the updated response format.
The change to rpcAnswerOnSuccess: true aligns with other routes in the acquisition group and improves consistency. However, this change will affect how successful responses are formatted, which may impact existing clients.
Let's verify the consistency of this change with other acquisition routes:
✅ Verification successful
Change is consistent with other acquisition routes
The verification confirms that the rpcAnswerOnSuccess: true setting is consistent with other acquisition routes in the codebase. Specifically:
- The
/journeys/:operator_journey_id/cancelPOST route usesrpcAnswerOnSuccess: true - The
/journeysPOST route (the one being modified) now matches this pattern
This standardization of response formats within the acquisition routes group is a positive change that maintains API consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check response format configuration across acquisition routes
# Expected: Most acquisition routes should use rpcAnswerOnSuccess: true
# Search for all acquisition route configurations
rg -B 5 -A 5 'acquisition:[a-z]+.+method: .POST.'
Length of output: 51
Script:
#!/bin/bash
# Let's try a different approach to find acquisition-related routes
# Search for registerAcquisitionRoutes method and its context
rg -B 10 -A 20 "registerAcquisitionRoutes"
Length of output: 3953
Script:
#!/bin/bash
# Let's search for all route registrations in the HttpTransport class
# specifically looking for POST methods and their rpcAnswerOnSuccess configuration
rg -B 5 -A 10 'method: "POST",' api/src/pdc/proxy/HttpTransport.ts
Length of output: 3810
A merger avec le refactor sur la couche HttpTransport
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests