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_v2): implement a barebones version of list customer payment methods v2 #6649

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

SanchithHegde
Copy link
Member

Type of Change

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

Description

This PR implements a very basic version of the list customer payment methods v2 route, including some refactors made along the way.

This PR includes the following changes:

  • Modifies the created field to be a required field in CustomerPaymentMethod. I've also fixed a minor error in the documentation comment for that field.
  • Updates the get_pm_list_context() function to match on the payment_method_data field, which simplifies code to some extent.
  • Updates the list customer payment methods v2 list code to find customer by global ID instead of by merchant reference ID.
  • Refactors the PaymentMethodListContext type to be an enum (only in v2), since this helps simplify some logic.
  • Adds a TODO comment around perform_surcharge_ops() call in list_customer_payment_method(), since that hasn't been implemented yet.
  • Updates the payment_method_data field in the PaymentMethod v2 domain model to use OptionalEncryptableJsonType type alias. (This addresses a comment on my previous PR: refactor(payment_methods_v2): rename payment_method and payment_method_type fields and use concrete type for payment_method_data #6555 (comment).)

Additionally, I've included these fixes I identified when testing my code changes:

  • Fixed a few errors in payment methods v2 code due to incorrect cell ID being passed.
  • Fixed customers list v2 not being recognized as a valid route.
  • Removed redundant v2 specific authentication implementations. These implementations became redundant after the AuthenticationDataV2 type was renamed to AuthenticationData.
  • Fixed payment methods routes not being recognized as valid ones when run with v2 features enabled.

Testing this involved some manual insertion of data with SQL queries, and fiddling of data in the database. Additionally, I could not test the scenario where the API call is associated with a payment, because there are a couple of panics in that code path, and I'm not completely sure what the implementations of those functions should be.

Feel free to review the PR one commit at a time, if that helps.

Additional Changes

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

This PR makes the created field to be a required field in CustomerPaymentMethod type, and updates the documentation comment for the same field.

Motivation and Context

Partially implements a new route related to payment methods v2.

How did you test it?

Testing this on hosted environments is not directly possible, since this involves inserting data into the database using SQL queries rather than with APIs.

  • To test the changes in this PR, I had to manually insert some data into the database using SQL queries, since the save payment methods v2 route is not completely implemented.

    • This involved inserting encrypted payment method data that is tied to a specific merchant account (and thus a specific merchant key store / data encryption key) created on my machine.
  • Once the data was inserted, I tried the endpoint by using a request as follows (the response is also attached):

    $ curl --location 'http://localhost:8080/v2/customers/customer123/saved_payment_methods' \
      --header 'x-profile-id: pro_bJzILfZDpOPo3IcNqN8J' \
      --header 'api-key: dev_5yalCEaTm4ABIAmJi30wHXmfnI5tcAs94pxIaU6F6WPbt4PeTpCI5Dh60tIdw9gt'
    {
      "customer_payment_methods": [
        {
          "payment_token": null,
          "payment_method_id": "global_id_pm_Il3WGMpoX6MCjQ0LfBi9",
          "customer_id": "customer123",
          "payment_method_type": "card",
          "payment_method_subtype": "credit",
          "recurring_enabled": false,
          "payment_method_data": {
            "card": {
              "issuer_country": null,
              "last4_digits": "4242",
              "expiry_month": "10",
              "expiry_year": "25",
              "card_holder_name": "joseph Doe",
              "card_fingerprint": null,
              "nick_name": null,
              "card_network": null,
              "card_isin": "424242",
              "card_issuer": null,
              "card_type": null,
              "saved_to_locker": true
            }
          },
          "bank": null,
          "created": "2024-11-14T14:37:57.934Z",
          "surcharge_details": null,
          "requires_cvv": false,
          "last_used_at": "2024-11-14T14:37:57.934Z",
          "is_default": false,
          "billing": {
            "address": {
              "city": "San Fransico",
              "country": "US",
              "line1": "1467",
              "line2": "Harrison Street",
              "line3": "Harrison Street",
              "zip": "94122",
              "state": "California",
              "first_name": "joseph",
              "last_name": "Doe"
            },
            "phone": {
              "number": "8888888888",
              "country_code": "+91"
            },
            "email": "[email protected]"
          }
        }
      ],
      "is_guest_customer": null
    }

Also, I could not test the scenario where the API call is associated with a payment, because there are a couple of panics in that code path, and I'm not completely sure what the implementations of those functions should be.

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

@SanchithHegde SanchithHegde added C-feature Category: Feature request or enhancement S-waiting-on-review Status: This PR has been implemented and needs to be reviewed A-payment-methods Area: Payment Methods C-refactor Category: Refactor M-api-contract-changes Metadata: This PR involves API contract changes api-v2 labels Nov 24, 2024
@SanchithHegde SanchithHegde added this to the November 2024 Release milestone Nov 24, 2024
@SanchithHegde SanchithHegde self-assigned this Nov 24, 2024
@SanchithHegde SanchithHegde changed the title feature(payment_methods_v2): implement a barebones version of list customer payment methods v2 feat(payment_methods_v2): implement a barebones version of list customer payment methods v2 Nov 24, 2024
@SanchithHegde SanchithHegde marked this pull request as ready for review November 25, 2024 05:28
@SanchithHegde SanchithHegde requested review from a team as code owners November 25, 2024 05:28
ThisIsMani
ThisIsMani previously approved these changes Nov 26, 2024
Copy link
Contributor

@ThisIsMani ThisIsMani left a comment

Choose a reason for hiding this comment

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

Dashboard specific changes looks fine.

Some(payment_methods::PaymentMethodsData::BankDetails(bank_details)) => {
let get_bank_account_token_data =
|| -> errors::CustomResult<payment_methods::BankAccountTokenData, errors::ApiErrorResponse> {
let connector_details = bank_details
Copy link
Member

Choose a reason for hiding this comment

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

why we need to take first here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to be related to payment method authentication flows.

Looping in @Sarthak1799.

Copy link
Contributor

@Sarthak1799 Sarthak1799 Nov 26, 2024

Choose a reason for hiding this comment

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

The types and flow here seem specific to PM auth flows in v1.
The first connector details are stored in redis for payment token flow (This would later on be fetched in payments confirm for calling connector).

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-feature Category: Feature request or enhancement C-refactor Category: Refactor M-api-contract-changes Metadata: This PR involves API contract changes S-waiting-on-review Status: This PR has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants