-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(connector): [Adyen] add dispute flows for adyen connector #5514
Conversation
c8f2f1c
to
7d2c714
Compare
7374cf8
to
8f005fc
Compare
82bf8da
to
a384d96
Compare
config/config.example.toml
Outdated
@@ -181,6 +181,7 @@ hash_key = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" | |||
aci.base_url = "https://eu-test.oppwa.com/" | |||
adyen.base_url = "https://checkout-test.adyen.com/" | |||
adyen.secondary_base_url = "https://pal-test.adyen.com/" | |||
adyen.third_base_url = "https://ca-test.adyen.com/ca/services/DisputeService/v30/" |
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.
Can we proper naming convention stating the usecase of the newly added url?
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.
Please don't add the url path (ca/services/DisputeService/v30/) in base_url
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.
Can we proper naming convention stating the usecase of the newly added url?
It can be used by other connectors for other use cases in future so it is better to be like that
pub cancellation_policy: Option<Vec<u8>>, | ||
pub cancellation_policy_type: Option<String>, |
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.
pub cancellation_policy_type: Option<String>, | |
pub cancellation_policy_file_type: Option<String>, |
pub cancellation_policy: Option<Vec<u8>>, | ||
pub cancellation_policy_type: Option<String>, |
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.
Please follow the same convention for all the other newly added fields
crates/router/src/connector/adyen.rs
Outdated
@@ -52,16 +50,18 @@ impl Adyen { | |||
} | |||
} | |||
} | |||
|
|||
fn get_key(auth_type: &types::ConnectorAuthType) -> CustomResult<String, errors::ConnectorError> { |
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.
Can we move this to transformers and name it more meaningful?
crates/router/src/connector/adyen.rs
Outdated
) -> CustomResult<types::AcceptDisputeRouterData, errors::ConnectorError> { | ||
Ok(types::AcceptDisputeRouterData { | ||
response: Ok(types::AcceptDisputeResponse { | ||
dispute_status: api::enums::DisputeStatus::DisputeAccepted, |
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.
We can't hardcode the status here, when we get a 200 response.
Instead, we need to consume the body and decide the status
{
"disputeServiceResult": {
"success": true
}
}
crates/router/src/connector/adyen.rs
Outdated
) -> CustomResult<types::SubmitEvidenceRouterData, errors::ConnectorError> { | ||
Ok(types::SubmitEvidenceRouterData { | ||
response: Ok(types::SubmitEvidenceResponse { | ||
dispute_status: api_models::enums::DisputeStatus::DisputeChallenged, |
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.
Please consume the response body and decide the status
} | ||
#[async_trait::async_trait] | ||
impl api::FileUpload for Adyen { | ||
fn validate_file_upload( |
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.
Can you please add validations specific to adyen connector, https://help.adyen.com/knowledge/risk/dispute-management/what-are-the-specific-defense-requirements
Ok(Self { | ||
dispute_psp_reference: item.request.connector_dispute_id.clone(), | ||
merchant_account_code, | ||
defense_reason_code: "SupplyDefenseMaterial".into(), |
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.
Can you please attach the screenshot of adyen support confirming the usage of SupplyDefenseMaterial
in all Defend dispute requests?
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.
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.
What about the production environment?
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.
Yes, We can use this for production also
defense_documents.push(DefenseDocuments { | ||
content: get_content(service_documentation), | ||
content_type: item.service_documentation_type, | ||
defense_document_type_code: "DefenseMaterial".into(), |
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.
Should we take this as an input from the merchant, or is it okay to hardcode it to be DefenseMaterial?
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.
It is okay to be DefenceMaterial
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.
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.
Yes We can use that
@@ -175,9 +175,9 @@ pub async fn retrieve_file_and_provider_file_id_from_file_id( | |||
merchant_account: &domain::MerchantAccount, | |||
key_store: &domain::MerchantKeyStore, | |||
is_connector_file_data_required: api::FileDataRequired, | |||
) -> CustomResult<(Option<Vec<u8>>, Option<String>), errors::ApiErrorResponse> { | |||
) -> CustomResult<(Option<Vec<u8>>, Option<String>, Option<String>), errors::ApiErrorResponse> { |
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.
Instead of returning a tuple with 3 elements, please create a struct for this
7d55c76
to
635ca1c
Compare
1e839ad
to
86080df
Compare
crates/api_models/src/disputes.rs
Outdated
@@ -49,6 +49,11 @@ pub struct DisputeResponse { | |||
pub merchant_connector_id: Option<String>, | |||
} | |||
|
|||
pub struct FileInfo { |
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.
Why is this struct defined in api_models? We can consider this moving into hyperswitch domain models
pub adyenplatform: ConnectorParams, | ||
#[cfg(not(feature = "payouts"))] | ||
pub adyen: ConnectorParams, | ||
pub adyen: ConnectorParamsWithThreeBaseUrls, |
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.
Previously this didn't have a secondary_base_url, why are we adding it now?
crates/router/src/connector/adyen.rs
Outdated
.response | ||
.parse_struct("AdyenDisputeResponse") | ||
.change_context(errors::ConnectorError::ResponseDeserializationFailed)?; | ||
if response.success { |
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.
Can we move this block to transformers please?
crates/router/src/connector/adyen.rs
Outdated
.response | ||
.parse_struct("AdyenDisputeResponse") | ||
.change_context(errors::ConnectorError::ResponseDeserializationFailed)?; | ||
if response.success { |
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.
Can we move this block to transformers please?
crates/router/src/connector/adyen.rs
Outdated
.parse_struct("AdyenDisputeResponse") | ||
.change_context(errors::ConnectorError::ResponseDeserializationFailed)?; | ||
|
||
if response.success { |
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.
Can we move this block to transformers please and try to reuse the same function?
Ok(Self { | ||
dispute_psp_reference: item.request.connector_dispute_id.clone(), | ||
merchant_account_code, | ||
defense_reason_code: "SupplyDefenseMaterial".into(), |
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.
What about the production environment?
defense_documents.push(DefenseDocuments { | ||
content: get_content(service_documentation), | ||
content_type: item.service_documentation_type, | ||
defense_document_type_code: "DefenseMaterial".into(), |
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.
759fe1a
to
71853aa
Compare
@@ -11,7 +11,7 @@ use serde::Deserialize; | |||
pub struct Connectors { | |||
pub aci: ConnectorParams, | |||
#[cfg(feature = "payouts")] | |||
pub adyen: ConnectorParamsWithSecondaryBaseUrl, | |||
pub adyen: ConnectorParamsWithThreeBaseUrls, | |||
pub adyenplatform: ConnectorParams, | |||
#[cfg(not(feature = "payouts"))] | |||
pub adyen: ConnectorParams, |
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.
Irrespective of payout feature being enabled, we would need this url for disputes right?
CI checks are failing, please check @KiranKBR |
fa954c1
to
29611c2
Compare
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.
LGTM
crates/router/src/connector/adyen.rs
Outdated
if (file_type.to_string().as_str() == "image/jpeg" | ||
|| file_type.to_string().as_str() == "image/jpeg") |
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.
This OR clause is checking same thing, please updated with intended check
e5597b7
to
cd93e54
Compare
@sai-harsha-vardhan Should we make FileData's |
260046d
a72e67d
to
260046d
Compare
* 'main' of github.com:juspay/hyperswitch: feat(connector): [Adyen] add dispute flows for adyen connector (#5514) chore(version): 2024.08.23.0 feat(router): [cybersource] add disable_avs and disable_cvn flag in connector metadata (#5667) feat(customer_v2): add route for customer retrieve v2 (#5516) chore(version): 2024.08.22.1 fix(router): [Adyen] prevent partial submission of billing address and add required fields for all payment methods (#5660) docs(README): Adding Contributors guide (#5184) Docs: Adding redirect url details (#5507) feat(global_id): create a `GlobalId` domain type (#5644) feat(router): collect customer address details based on business profile config regardless of connector required fields (#5418) feat: add new routes for profile level list apis (#5589) refactor(core): Refactor fallback routing behaviour in payments for v2 (#5642) refactor(router): add connector_transaction_id, send response body and use admin_api_auth_with_merchant_id for payments manual update flow (#5658)
* 'main' of github.com:juspay/hyperswitch: (134 commits) refactor(open_banking): Added merchant data update in mca update (#5655) feat: add test_mode for quickly testing payout links (#5669) refactor: introduce a domain type for profile ID (#5687) ci(cypress): update paybox configs (#5664) feat(openapi): Add open api routes for routing v2 (#5686) feat(connector): [NOVALNET] Add template code (#5670) feat(user): business email update (#5674) chore(config): add production connector-configs for netcetera external 3ds flow (#5698) chore(version): 2024.08.27.0 refactor(euclid): make the disabled node's relation as negative (#5701) feat: populate payment method details in payments response (#5661) build(deps): bump `diesel` to `2.2.3` and `sqlx` to `0.8.1` (#5688) feat(customer_v2): added list customer v2 end point (#5517) feat(business_profile): add tax_connector_id column in business_profile table (#5576) chore: create v2 route for organization (#5679) refactor(payments_response): remove setter from payments response (#5676) feat(payment_methods_v2): Payment methods v2 API models (#5564) chore(version): 2024.08.26.0 feat(connector): [Adyen] add dispute flows for adyen connector (#5514) chore(version): 2024.08.23.0 ...
Type of Change
Description
This PR will add dispute flows - accept,defend,submitEvidence to adyen connector
Additional Changes
Motivation and Context
How did you test it?
Accept:
Submit Evidence:
Accept request:
response:
Submit evidence:
response:
Checklist
cargo +nightly fmt --all
cargo clippy