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

refactor(FRM): refactor frm configs #4581

Merged
merged 22 commits into from
May 17, 2024
Merged

refactor(FRM): refactor frm configs #4581

merged 22 commits into from
May 17, 2024

Conversation

srujanchikke
Copy link
Contributor

@srujanchikke srujanchikke commented May 8, 2024

Type of Change

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

Description

This PR contains

  1. Depricating FRM action and rule setting on hyperswitch end.
  2. revising the new FRM configs
  3. Taking actions based on frm status .
  4. Few bugs of previous PR described here.
  5. provided backward compatability for previous configs.

Additional Changes

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

Previously frm configuration when creating merchant connector for FRM connector used to be like

"frm_configs": [
        {
            "gateway": "stripe",
            "payment_methods": [
                {
                    "payment_method": "card",
                    "payment_method_types": [
                        {
                            "payment_method_type": "credit",
                            "card_networks": [
                                "Visa"
                            ],
                            "flow": "post",
                            "action": "cancel_txn"
                        },
                        {
                            "payment_method_type": "debit",
                            "card_networks": [
                                "Visa"
                            ],
                            "flow": "post",
                            "action": "manual_review"
                        }
                    ]
                }
            ]
        }
    ]

This has been modified to

"frm_configs": [
        {
            "gateway": "stripe",
            "payment_methods": [
                {
                    "payment_method": "card",
                    "flow": "post"
                },
                {
		                "payment_method": "wallet",
                    "flow": "pre"
                }
            ]
        }
    ]

Motivation and Context

Frm action is depricated because the levarage of manual review now has been moved to frm connector. We consume webhooks from frm connectors and take appropriate action. We also depricated merchant to configure rules from hyperswitch end since he already sets up rules in frm, so we solely depends on FRM status to take action.

PreAuth flow :

  • FRAUD -> Cancel Payment
  • LEGIT -> Continue transaction
  • MANUAL_REVIEW -> We don't support Manual review since there will be no customer for completing the payment.

PostAuth flow :

  • FRAUD -> Cancel Payment
  • LEGIT -> continue transaction
  • MANUAL_REVIEW -> Approve/ Decline based on webhook recieved from frm connector.

How did you test it?

Test merchant account create with both new and old frm configs.

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

@srujanchikke srujanchikke added S-waiting-on-review Status: This PR has been implemented and needs to be reviewed C-refactor Category: Refactor M-api-contract-changes Metadata: This PR involves API contract changes labels May 8, 2024
@srujanchikke srujanchikke self-assigned this May 8, 2024
@srujanchikke srujanchikke requested review from a team as code owners May 8, 2024 08:16
kashif-m
kashif-m previously approved these changes May 8, 2024
@srujanchikke srujanchikke requested review from a team as code owners May 10, 2024 10:05
Copy link
Member

@Narayanbhat166 Narayanbhat166 left a comment

Choose a reason for hiding this comment

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

Can you add some test coverage for FRM? can be taken up in subsequent PRs

///payment method types(credit, debit) that can be used in the payment. This field is deprecated. It has not been removed to provide backward compatibility.
pub payment_method_types: Option<Vec<FrmPaymentMethodType>>,
///frm flow type to be used...can be pre/post
#[schema(value_type = FrmPreferredFlowTypes)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[schema(value_type = FrmPreferredFlowTypes)]
#[schema(value_type = Option<FrmPreferredFlowTypes>)]

pub payment_method_types: Vec<FrmPaymentMethodType>,
///payment method types(credit, debit) that can be used in the payment. This field is deprecated. It has not been removed to provide backward compatibility.
pub payment_method_types: Option<Vec<FrmPaymentMethodType>>,
///frm flow type to be used...can be pre/post
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
///frm flow type to be used...can be pre/post
///frm flow type to be used, can be pre/post

@@ -1038,6 +1037,7 @@ impl PaymentCreate {
session_expiry: Some(session_expiry),
request_external_three_ds_authentication: request
.request_external_three_ds_authentication,
frm_metadata: request.frm_metadata.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

do we validate the value here? or any serde_json::value can be put in the column?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't validate here, validation is there at connector level. Signifyd & Riskified expects few fields from frm_metadata.

@@ -0,0 +1,2 @@
-- Your SQL goes here
ALTER TABLE payment_intent ADD COLUMN frm_metadata JSONB DEFAULT NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ALTER TABLE payment_intent ADD COLUMN frm_metadata JSONB DEFAULT NULL;
ALTER TABLE payment_intent ADD COLUMN IF NOT EXISTS frm_metadata JSONB DEFAULT NULL;

@@ -0,0 +1,2 @@
-- This file should undo anything in `up.sql`
ALTER TABLE payment_intent DROP COLUMN frm_metadata;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ALTER TABLE payment_intent DROP COLUMN frm_metadata;
ALTER TABLE payment_intent DROP COLUMN IF EXISTS frm_metadata;

@srujanchikke srujanchikke added the M-database-changes Metadata: This PR involves database schema changes label May 17, 2024
Copy link
Contributor

@apoorvdixit88 apoorvdixit88 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 are fine!

@likhinbopanna likhinbopanna added this pull request to the merge queue May 17, 2024
Merged via the queue into main with commit 853f3b4 May 17, 2024
16 checks passed
@likhinbopanna likhinbopanna deleted the frm_configs branch May 17, 2024 10:37
@SanchithHegde SanchithHegde removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label May 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants