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(core): Connector specific validation for Payment Sync #2005

Merged
merged 10 commits into from
Aug 31, 2023

Conversation

Sakilmostak
Copy link
Contributor

@Sakilmostak Sakilmostak commented Aug 24, 2023

Type of Change

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

Description

connector_request_reference_id is being pass as merchant_transaction_id which will help in payment retrieval in case of connector timeout.
Whether psync call is made is now handled at connector level to decide what parameters are required to make the call
For Bluesnap, merchant_id needs to be passed in connector meta_data to enable this feature

Additional Changes

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

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

@Sakilmostak Sakilmostak added A-connector-integration Area: Connector integration C-refactor Category: Refactor R-waiting-on-L1 Review: Waiting on L1 reviewer labels Aug 24, 2023
@Sakilmostak Sakilmostak requested a review from a team as a code owner August 24, 2023 08:11
@Sakilmostak Sakilmostak self-assigned this Aug 24, 2023
@Sakilmostak Sakilmostak requested review from a team, jarnura and ashokkjag as code owners August 25, 2023 07:30
@Sakilmostak Sakilmostak changed the title refactor(connector): [Bluesnap] Use connector_request_reference_id as Transaction Id to Retrieve Payments feat(core): Connector specific validation for Payment Sync Aug 28, 2023
@Sakilmostak Sakilmostak added the A-core Area: Core flows label Aug 28, 2023
crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
crates/router/src/connector/bluesnap/transformers.rs Outdated Show resolved Hide resolved
crates/router/src/core/payments.rs Outdated Show resolved Hide resolved
@ArjunKarthik ArjunKarthik added the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Aug 28, 2023
@Sakilmostak Sakilmostak removed the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Aug 28, 2023
@@ -26,7 +26,7 @@ use crate::{
configs::settings::Connectors,
connector, consts,
core::errors::{self, CustomResult},
services::{request, ConnectorIntegration, ConnectorRedirectResponse},
services::{request, ConnectorIntegration, ConnectorRedirectResponse, ConnectorValidation},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this import?

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, since we are adding this trait to Payment, this import was made to add ConnectorValidation trait for api crate itself otherwise for all file where payment is implemented, we need to import services crate too for ConnectorValidation

crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
Copy link
Member

@NishantJoshi00 NishantJoshi00 left a comment

Choose a reason for hiding this comment

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

Just some nit picks, other than that looks good

crates/router/src/connector/bluesnap.rs Outdated Show resolved Hide resolved
None => types::ResponseId::NoResponseId,
};

// [#44]: why should response be filled during request
Copy link
Member

Choose a reason for hiding this comment

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

We should in the future, also pick this up, as currently these are just work arounds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure👍, will pick this up

crates/router/src/services/api.rs Outdated Show resolved Hide resolved
NishantJoshi00
NishantJoshi00 previously approved these changes Aug 30, 2023
crates/router/src/services/api.rs Outdated Show resolved Hide resolved
@ArjunKarthik ArjunKarthik added S-ready-for-merge and removed R-waiting-on-L1 Review: Waiting on L1 reviewer labels Aug 31, 2023
@ArjunKarthik ArjunKarthik added this pull request to the merge queue Aug 31, 2023
Merged via the queue into main with commit 098dc89 Aug 31, 2023
12 of 14 checks passed
@ArjunKarthik ArjunKarthik deleted the bluesnap_sync_refactor branch August 31, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-connector-integration Area: Connector integration A-core Area: Core flows C-refactor Category: Refactor
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

4 participants