-
Notifications
You must be signed in to change notification settings - Fork 29
feat(router): add pagos bin lookup api support for debit routing #60
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?
Conversation
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.
Pull Request Overview
This PR adds support for fetching co-badged card information via the Pagos API (toggleable via config) and cleans up related domain types and lookup flows.
- Define new Pagos response types and a dedicated HTTP client (
PagosApiClient) - Extend global and tenant config with
PagosApiConfigand a feature flag - Refactor
get_co_badged_cards_infoto switch between DB lookup and Pagos API, and remove legacy fields
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/pagos.rs | New structs/enums for Pagos API JSON payloads |
| src/types.rs | Expose new pagos module |
| src/pagos_client.rs | HTTP client and helper for querying Pagos API |
| src/lib.rs | Register pagos_client module |
| src/decider/network_decider/types.rs | Remove legacy timestamp/ID fields from domain model |
| src/decider/network_decider/helpers.rs | Update call to get_co_badged_cards_info signature |
| src/decider/network_decider/co_badged_card_info.rs | Branch logic for DB vs. Pagos API lookup and mapping |
| src/config.rs | Introduce PagosApiConfig in global/tenant settings |
| config/development.toml | Add [pagos_api] section for local development |
| config.example.toml | Document [pagos_api] settings in sample config |
Comments suppressed due to low confidence (3)
src/pagos_client.rs:38
- Add unit tests for
PagosApiClient::newto verify error mapping when the client build fails and invalid API key header scenarios.
pub fn new(base_url: String, api_key: String) -> PagosClientResult<Self> {
config.example.toml:73
- [nitpick] The comment 'Corrected Pagos API base URL' may be confusing—consider using a neutral descriptor like 'Pagos API base URL'.
[pagos_api]
src/decider/network_decider/co_badged_card_info.rs:9
- Add unit tests for
get_parsed_bin_range_from_pagoscovering missing fields and invalid integer parsing to ensure error messages are clear.
fn get_parsed_bin_range_from_pagos(
| ConfigError(String), | ||
| } | ||
|
|
||
| #[derive(Clone)] |
Copilot
AI
May 27, 2025
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.
Consider adding a doc comment explaining the purpose of PagosApiClient, its retry or timeout behavior, and any thread‐safety guarantees.
| #[derive(Clone)] | |
| #[derive(Clone)] | |
| /// A client for interacting with the Pagos API. | |
| /// | |
| /// The `PagosApiClient` provides methods to send requests to the Pagos API, such as retrieving | |
| /// PAN (Primary Account Number) details. It handles HTTP requests, response parsing, and error | |
| /// handling for the API. | |
| /// | |
| /// # Retry and Timeout Behavior | |
| /// This client does not implement automatic retries or custom timeout settings. The default | |
| /// timeout and retry behavior of the underlying `reqwest::Client` is used. | |
| /// | |
| /// # Thread-Safety | |
| /// The `PagosApiClient` is marked as `Clone`, and its internal `reqwest::Client` is thread-safe. | |
| /// Therefore, it is safe to use cloned instances of this client across threads. |
| pub struct PagosApiConfig { | ||
| pub base_url: String, | ||
| pub api_key: masking::Secret<String>, |
Copilot
AI
May 27, 2025
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.
Provide comments on each field of PagosApiConfig (e.g., default values, expected URI format for base_url, and security considerations for api_key).
| pub struct PagosApiConfig { | |
| pub base_url: String, | |
| pub api_key: masking::Secret<String>, | |
| pub struct PagosApiConfig { | |
| /// The base URL for the Pagos API. | |
| /// Expected format: A valid URI (e.g., "https://api.pagos.com"). | |
| pub base_url: String, | |
| /// The API key used for authenticating with the Pagos API. | |
| /// Security considerations: This is sensitive information and should be stored securely. | |
| /// Use the `masking::Secret` type to prevent accidental logging or exposure. | |
| pub api_key: masking::Secret<String>, | |
| /// A flag indicating whether the API should be used for co-badged card lookup. | |
| /// Default value: `false`. |
| } | ||
| } | ||
|
|
||
| async fn fetch_co_badged_info_from_pagos_api( |
Copilot
AI
May 27, 2025
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.
[nitpick] The loops for primary and additional brands both call try_convert_pagos_card_to_domain_data and push results; consider extracting common logic into a helper to reduce duplication.
| #[derive(Clone, serde::Deserialize, Debug, Default)] | ||
| pub struct PagosApiConfig { | ||
| pub base_url: String, | ||
| pub api_key: masking::Secret<String>, |
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.
isn't this kms encrypted?
| pagos_card_details: &pagos::PagosCardDetails, | ||
| additional_brand_info: Option<&pagos::PagosAdditionalCardBrand>, | ||
| card_brand_to_parse: &str, | ||
| ) -> CustomResult<(i64, i64), error::ApiError> { |
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.
Nit:
Create a custom type instead of returning (i32, i32) as the caller is unsure of what either of these mean
| Err(error) => { | ||
| logger::error!( | ||
| "Error while fetching co-badged card info record from DB: {:?}", | ||
| error | ||
| ); | ||
| Err(error::ApiError::UnknownError) | ||
| .attach_printable("Error while fetching co-badged card info record from DB") |
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.
can do change_context on co_badged_card_infos_record because by doing this you are generating 2 different logs, one by explicit logger::error and other in parent call
| Err(error) => { | ||
| logger::error!("Error converting primary Pagos card details: {:?}", error); | ||
| return Err(error); |
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.
same here
| Err(error) => { | ||
| logger::error!("Error converting additional Pagos card details for brand {}: {:?}", additional_card_brand_str, error); | ||
| return Err(error); |
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.
same here
| logger::error!("Failed to fetch co-badged card info: {:?}", error); | ||
| return Err(error); |
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.
same here
Refactor: Enhance Co-Badged Card Info Lookup with Pagos API Option and Code Cleanup__
This PR introduces a configurable mechanism to fetch co-badged card information, allowing a switch between the existing database lookup and a new Pagos API integration. It also includes several code quality improvements and stricter error handling.