Skip to content
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

feat(payment_methods): Store necessary payment method data in payment_methods table #2073

Merged
merged 10 commits into from
Sep 6, 2023

Conversation

Sarthak1799
Copy link
Contributor

@Sarthak1799 Sarthak1799 commented Sep 1, 2023

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

Currently, The Customer Payment method List API resorts to a locker call to fetch card details. The latency induced from this is reduced by storing encrypted card details in payment_methods table. This PR does that.

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

How did you test it?

Payments Create - curl --location --request POST 'http://localhost:8080/payments' \ --header 'Content-Type: application/json' \ --header 'Accept: application/json' \ --header 'api-key: dev_Ia7M1EapArhTiWcTqmQMCLJ4O8lXRbSz8vrxd2nTAZMgOIkBttrLRXFY6owwhlL7' \ --data-raw '{ "amount": 6540, "currency": "USD", "confirm": true, "capture_method": "automatic", "capture_on": "2022-09-10T10:11:12Z", "amount_to_capture": 6540, "customer_id": "StripeCustomer3", "email": "guest@example.com", "name": "John Doe", "phone": "999999999", "phone_country_code": "+1", "description": "Its my first payment request", "authentication_type": "no_three_ds", "return_url": "https://google.com", "payment_method": "card", "payment_method_type": "credit", "setup_future_usage":"off_session", "payment_method_data": { "card": { "card_number": "4242424242424242", "card_exp_month": "10", "card_exp_year": "25", "card_holder_name": "joseph Doe", "card_cvc": "124", "card_network": "Visa" } }, "billing": { "address": { "line1": "1467", "line2": "Harrison Street", "line3": "Harrison Street", "city": "San Fransico", "state": "California", "zip": "94122", "country": "US", "first_name": "john", "last_name": "Doe" }, "phone": { "number": "8056594427", "country_code": "+91" } }, "shipping": { "address": { "line1": "1467", "line2": "Harrison Street", "line3": "Harrison Street", "city": "San Fransico", "state": "California", "zip": "94122", "country": "US", "first_name": "joseph", "last_name": "Doe" }, "phone": { "number": "8056594430", "country_code": "+91" } }, "statement_descriptor_name": "joseph", "statement_descriptor_suffix": "JS", "metadata": { "udf1": "value1", "new_customer": "true", "login_date": "2019-09-10T10:11:12Z" } }'

List Payment methods for customers - curl --location --request GET 'http://localhost:8080/customers/StripeCustomer1/payment_methods' \ --header 'Accept: application/json' \ --header 'api-key: dev_Ia7M1EapArhTiWcTqmQMCLJ4O8lXRbSz8vrxd2nTAZMgOIkBttrLRXFY6owwhlL7'

image image image

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@Sarthak1799 Sarthak1799 added M-database-changes Metadata: This PR involves database schema changes A-payment-methods Area: Payment Methods C-refactor Category: Refactor labels Sep 1, 2023
@Sarthak1799 Sarthak1799 requested a review from a team as a code owner September 1, 2023 07:55
@Sarthak1799 Sarthak1799 self-assigned this Sep 1, 2023
@Sarthak1799 Sarthak1799 requested a review from a team as a code owner September 1, 2023 07:55
@Sarthak1799 Sarthak1799 linked an issue Sep 1, 2023 that may be closed by this pull request
2 tasks
@@ -144,6 +144,17 @@ pub struct PaymentMethodResponse {
pub created: Option<time::PrimitiveDateTime>,
}

#[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it required to derive Eq, PartialEq trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as the PMT derives these. It wasn't compiling before.

Copy link
Contributor

@kashif-m kashif-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of things missing here

  • Storing bank details or other payment methods info in DB
  • API calls will always be made for payment method type - BankTransfer - while listing the customer's saved PMs

crates/api_models/src/payment_methods.rs Show resolved Hide resolved
crates/diesel_models/src/payment_method.rs Outdated Show resolved Hide resolved
kashif-m
kashif-m previously approved these changes Sep 5, 2023
@Sarthak1799 Sarthak1799 changed the title feat(payment_methods): Store necessary card details in payment_methods table feat(payment_methods): Store necessary payment method data in payment_methods table Sep 5, 2023
crates/api_models/src/payment_methods.rs Outdated Show resolved Hide resolved
Comment on lines 2247 to 2253
let key_store = db
.get_merchant_key_store_by_merchant_id(
&merchant_account.merchant_id,
&db.get_master_key().to_vec().into(),
)
.await
.ok();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the key store already available in all caller contexts, so we can just pass it in as an argument to this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have done the corresponding changes

crates/router/src/core/payment_methods/cards.rs Outdated Show resolved Hide resolved
crates/router/src/core/payment_methods/cards.rs Outdated Show resolved Hide resolved
crates/router/src/core/payment_methods/cards.rs Outdated Show resolved Hide resolved
crates/router/src/core/payment_methods/cards.rs Outdated Show resolved Hide resolved
};

let card: Option<api::CardDetailFromLocker> =
if pm.payment_method == enums::PaymentMethod::Card {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write card related things as a separate function and call that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have done

@jarnura jarnura added this to the September 2023 Milestone milestone Sep 6, 2023
@jarnura jarnura added this pull request to the merge queue Sep 6, 2023
Merged via the queue into main with commit 3c93552 Sep 6, 2023
10 checks passed
@jarnura jarnura deleted the card-details-payment-methods branch September 6, 2023 14:04
jarnura added a commit that referenced this pull request Sep 11, 2023
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 C-refactor Category: Refactor M-database-changes Metadata: This PR involves database schema changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Store necessary Card details in Payment methods table
5 participants