-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(payment_methods): populate connector_customer during customer creation step in payment methods migrate flow #8319
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
Conversation
…eation step in payment methods migrate flow
Changed Files
|
WalkthroughThis change introduces support for handling connector-specific customer details during customer creation and migration. New data structures encapsulate connector customer information, and function signatures are updated to accept and propagate these details. The migration and customer creation flows now accommodate optional connector customer IDs, enabling more comprehensive customer data management. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Migration/Creation Flow
participant Core as core::customers
participant Domain as domain::Customer
participant DB as StorageInterface
Caller->>Core: create_customer(state, merchant_context, customer_data, connector_customer_details)
Core->>Core: create_domain_model_from_request(connector_customer_details, ...)
Core->>Domain: Map fields, attach connector_customer_details if present
Core->>DB: Persist new Customer with connector details
DB-->>Core: Confirmation
Core-->>Caller: Result
Possibly related issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
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.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
crates/router/src/routes/customers.rs (1)
24-29
:⚠️ Potential issueMissing the new argument – will not compile when
customer_v2
is ON
create_customer
now expects(state, merchant_context, req, Option<ConnectorCustomerDetails>)
, but the customer-v2 path still supplies only three arguments.- create_customer(state, merchant_context, req) + create_customer(state, merchant_context, req, None)Without this change the build will fail for
--features "v2,customer_v2"
(or at runtime if a different overload is picked).
Please update this call (and any similar occurrences) to keep all feature combinations green.
🧹 Nitpick comments (6)
crates/router/src/routes/payment_methods.rs (2)
20-24
: Nit: redundantcore::customers
import guardBoth the
use crate::core::customers
line and the call-site below are behind the same feature flags.
If you ever broaden either side, remember to mirror the flags, otherwise dead-code / missing-import errors creep in. No action needed now.
372-377
: Allocate-less mapping
req.iter().map(...).collect()
creates an intermediateVec
.
customers::migrate_customers
takes aVec
, so this is unavoidable, but you can pre-size it to avoid a second allocation:- req.iter() - .map(|e| payment_methods::PaymentMethodCustomerMigrate::from((e.clone(), merchant_id.clone()))) - .collect(), + { + let mut v = Vec::with_capacity(req.len()); + for rec in &req { + v.push(payment_methods::PaymentMethodCustomerMigrate::from(( + rec.clone(), + merchant_id.clone(), + ))); + } + v + },Tiny optimisation, optional.
crates/api_models/src/payment_methods.rs (3)
2567-2571
: Derive missing traits & add OpenAPI schema forConnectorCustomerDetails
ConnectorCustomerDetails
is transported across service boundaries (later embedded in API requests).
Please derive the common trait set (Debug
,Eq
,PartialEq
,ToSchema
) to keep it in line with other DTOs and to ensure it appears in generated spec files:-#[derive(Debug, Clone, serde::Deserialize, serde::Serialize)] +#[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize, serde::Serialize, ToSchema)]
2573-2578
: ExposePaymentMethodCustomerMigrate
in OpenAPI & reuse existing typesSame as the previous comment – this struct is now part of the public contract between the migrate-flow and the customer module, yet it lacks
ToSchema
.
Please add it and, if possible, re-export the struct behind the same feature flags that gatecustomers::CustomerRequest
to prevent compilation under unsupported builds.
2753-2788
: Minor optimisation & clarity inFrom
impl
The
.zip()
call consumes bothOption
s, which is fine, but it also moves the values out ofrecord
; after that point those fields become un-usable.
As we do not use them later this is correct, but adding an inline comment would prevent accidental re-use in future edits.You allocate an
AddressDetails
every time, even when all address lines areNone
.
A small micro-optimisation is to construct thepayments::Address
only when at least one of the fields is present, e.g.:-let address = Some(payments::AddressDetails { ... }); +let address = { + let details = payments::AddressDetails { ... }; + if details.is_empty() { None } else { Some(details) } +};(assuming an
is_empty
helper exists – if not, keep as-is).Nothing blocking, just a heads-up.
crates/router/src/core/customers.rs (1)
1351-1352
: Redundant parameter in update flowSimilar to the create path,
connector_customer_details
is supplied to the v2 update implementation but never used. If updates are not supposed to touchconnector_customer
, drop the argument here to avoid confusion; otherwise implement the necessary merge logic.- connector_customer_details: &'a Option<payment_methods_api::ConnectorCustomerDetails>, + _connector_customer_details: &'a Option<payment_methods_api::ConnectorCustomerDetails>,or integrate the field as appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/api_models/src/payment_methods.rs
(3 hunks)crates/router/src/compatibility/stripe/customers.rs
(1 hunks)crates/router/src/core/customers.rs
(13 hunks)crates/router/src/routes/customers.rs
(1 hunks)crates/router/src/routes/payment_methods.rs
(2 hunks)crates/router/src/types/api/payment_methods.rs
(1 hunks)
⏰ Context from checks skipped due to timeout of 900000ms (6)
- GitHub Check: Check compilation on MSRV toolchain (ubuntu-latest)
- GitHub Check: Check compilation for V2 features
- GitHub Check: Run tests on stable toolchain (ubuntu-latest)
- GitHub Check: Run Cypress tests
- GitHub Check: Check wasm build
- GitHub Check: Validate generated OpenAPI spec file
🔇 Additional comments (3)
crates/router/src/types/api/payment_methods.rs (1)
21-34
: Import list looks good, but keep an eye oncfg
divergenceThe new types are only available when
not(feature = "payment_methods_v2")
.
Down-stream code that always expectsConnectorCustomerDetails
/PaymentMethodCustomerMigrate
will fail to compile whenpayment_methods_v2
is enabled. Make sure every call-site behind other feature flags is also conditionally guarded (or re-export the aliases behind a common facade).crates/router/src/compatibility/stripe/customers.rs (1)
60-65
: OK – 4ᵗʰ parameter added correctlyThe extra
None
matches the updated signature
create_customer(state, merchant_context, req, Option<ConnectorCustomerDetails>)
.
Nothing else to flag here.crates/router/src/routes/customers.rs (1)
58-63
: Consistent with new signatureHere the extra
None
is already added – good catch.
connector_customer_details: &'a Option<payment_methods_api::ConnectorCustomerDetails>, | ||
_db: &'a dyn StorageInterface, |
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.
🛠️ Refactor suggestion
connector_customer_details
parameter is accepted but silently ignored
The customer_v2
implementation of CustomerCreateBridge::create_domain_model_from_request
now receives connector_customer_details
, yet the body never references it. This will:
- trigger an “unused variable” warning (hidden only by the feature gate),
- break functional parity with the V1 path, leaving
connector_customer
alwaysNone
for v2 customers.
Either wire the data into the domain::Customer
(preferred) or prefix the argument with _
to silence warnings until support is added.
🤖 Prompt for AI Agents
In crates/router/src/core/customers.rs around lines 230 to 231, the parameter
connector_customer_details is accepted but not used, causing unused variable
warnings and breaking functional parity with the V1 path. To fix this, either
integrate connector_customer_details into the domain::Customer model within the
create_domain_model_from_request function or rename the parameter to
_connector_customer_details to suppress the warning until full support is
implemented.
b7d77b0
to
928b9d5
Compare
…connector_customer-creation-during-payment-methods-migration
11f021f
to
6dbe5ba
Compare
…connector_customer-creation-during-payment-methods-migration
Type of Change
Description
This PR introduces populating
connector_customer
field incustomers
table during payment method migration.Additional Changes
Motivation and Context
Allows merchants to populate
connector_customer
permerchant_connector_id
based onconnector_customer_id
passed in the request.How did you test it?
Steps for testing -
Create a merchant account
Create a merchant's connector account
3. Run /migrate-form API
cURL
Response
Look for connector_customer in customers table
Find CSV data below
Checklist
cargo +nightly fmt --all
cargo clippy
Summary by CodeRabbit
New Features
Bug Fixes
Chores