-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor(payment_methods): refactor customer payment methods list v2 code to follow better code practices #6433
Open
SanchithHegde
wants to merge
14
commits into
main
Choose a base branch
from
customer-payment-method-list-v2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…on_enabled` field in profile types
…`SavedPMLPaymentsInfo`
…ng_payment` in `SavedPMLPaymentsInfo` and populate it from profile
… during update to be required fields
…it within `form_payments_info()`
…er` crate to `diesel_models` crate
…Value` for `connector_mandate_details` field
…move database call happening in a loop
…when storing payment method token in Redis
…` with a correct `apply_changeset()` implementation
… not actively being used in v1 There does not seem to be any useful information in this column in any of our hosted environments either.
…o `Result<Vec, _>`
SanchithHegde
added
S-waiting-on-review
Status: This PR has been implemented and needs to be reviewed
M-database-changes
Metadata: This PR involves database schema changes
A-payment-methods
Area: Payment Methods
C-refactor
Category: Refactor
M-api-contract-changes
Metadata: This PR involves API contract changes
api-v2
labels
Oct 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-payment-methods
Area: Payment Methods
api-v2
C-refactor
Category: Refactor
M-api-contract-changes
Metadata: This PR involves API contract changes
M-database-changes
Metadata: This PR involves database schema changes
S-waiting-on-review
Status: This PR has been implemented and needs to be reviewed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
This PR includes refactors around the customer payment methods list v2 code to follow some of the practices we've started enforcing in the recent few months. Additionally, I've picked up some minor refactors along the way.
This PR contains the following changes:
should_collect_cvv_during_payment
column to the business profile v2 table to replace the{merchant_id}_requires_cvv
config we used to read previously (and still do in v1) from theconfigs
table.business_profile
field toprofile
in theSavedPMLPaymentsInfo
struct. (This follows the changes done in refactor: Rename business profile to profiles in api, diesel, domain, interface and error types #5877.)requires_cvv
field tocollect_cvv_during_payment
inSavedPMLPaymentsInfo
struct, to follow the new column added in the business profile v2 table.form_payments_info()
method instead of fetching it within the method. Eventually, we'd want to obtain the profile from the authentication layer instead, which would be passed forward.PaymentsMandateReference
from therouter
crate to thediesel_models
crate, and updates theconnector_mandate_details
field in the payment methods table to usePaymentsMandateReference
instead ofserde_json::Value
in v2 code. (This should help reduce / prevent invalid data being stored in this column of the database.)get_mca_status()
function to avoid a possible database call (to fetch merchant connector accounts associated with a merchant / profile) happening within an iterator.metadata
column from the payment methods table and API models in v2 code, since it isn't being actively used in v1 code anyway. Neither do any of our hosted environments have any useful data stored in that column.The other minor refactors I took up along the way include:
is_network_tokenization_enabled
field in profile API models.ExtendedCardInfoUpdate
,ConnectorAgnosticMitUpdate
andNetworkTokenizationUpdate
enum variants of theProfileUpdate
domain model enum to have required fields instead of optional ones. (This is because these variants either enable something or not, and we always have the value to be updated in all cases.)serialize_and_set_key_with_expiry()
when storing payment method token in Redis instead of explicit JSON serialization happening in business logic.create_payment_method()
(which only seemed to be updating the metadata field, and was thus confusing) with a correctapply_changeset()
implementation. This change only affects MockDb implementation.The PR can be reviewed one commit at a time, if that helps ease reviews.
Additional Changes
Motivation and Context
This PR contains only a subset of the changes / improvements that can help us in arriving at the final implementation of the customer payment methods list v2 API. There are still a few more refactors pending, which would be taken up in subsequent PRs.
How did you test it?
Haven't been able to test these changes, there are still a couple of panics remaining in the customer payment methods list v2 code.
Checklist
cargo +nightly fmt --all
cargo clippy