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(connector): [DATATRANS] Implement card payments #5028

Merged

Conversation

awasthi21
Copy link
Contributor

@awasthi21 awasthi21 commented Jun 18, 2024

Type of Change

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

Description

Implemented Card Payments for Datatrans

Additional Changes

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

Motivation and Context

#5027

How did you test it?

Attached Postman Collection postman/collection-json/datatrans.postman_collection.json

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

@awasthi21 awasthi21 added the A-connector-integration Area: Connector integration label Jun 18, 2024
@awasthi21 awasthi21 self-assigned this Jun 18, 2024
@awasthi21 awasthi21 requested review from a team as code owners June 18, 2024 12:14
@awasthi21 awasthi21 linked an issue Jun 18, 2024 that may be closed by this pull request
@hyperswitch-bot hyperswitch-bot bot added the M-api-contract-changes Metadata: This PR involves API contract changes label Jun 18, 2024
@awasthi21 awasthi21 force-pushed the 5027-datatrans-connector-integration branch from d2edc7e to 95ebb7b Compare June 18, 2024 12:19
@deepanshu-iiitu
Copy link
Contributor

Rename PR to: feat(connector): [DATATRANS] Implement card payments
Add the issue link #5027 in the Motivation and Context section of this PR.

Also add description in the issue #5027

crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
@awasthi21 awasthi21 changed the title feat(connector): added payment flows code feat(connector): [DATATRANS] Implement card payments Jun 21, 2024
@awasthi21 awasthi21 requested a review from a team as a code owner June 21, 2024 10:14
reason: response.reason,
code: error_code,
message: error_message.clone(),
reason: Some(error_message),
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the some here as mentioned in the previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reason is Option here
and
error_message is String

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok

.unwrap_or(consts::NO_ERROR_CODE.to_string());
let error_message = response
.error
.as_ref()
.and_then(|e| e.message.clone())
.map(|e| e.message.clone())
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this unwrap_or to the message field below. And remove the some used for reason

},
};

#[derive(Default, Debug, Clone, Serialize, Deserialize)]
pub struct DatatransErrorResponse {
pub error: Option<DatatransError>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this made option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed That

#[derive(Serialize, Deserialize, Clone, Debug)]
#[serde(rename_all = "camelCase")]
pub struct PlainCardDetails {
#[serde(rename = "type", default = "default_res_type")]
Copy link
Contributor

Choose a reason for hiding this comment

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

This hard coding should've been done in the try_from.

Comment on lines 104 to 110
pub enum CardType {
#[serde(rename = "card")]
PLAIN(PlainCardDetails),
PlainCardDetails,
ALIAS,
NetworkTOKEN,
DeviceTOKEN,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this unused enum

crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
_ => common_enums::AttemptStatus::Failure,
},
DataTransCaptureResponse::Error(error) => {
if error.message == *"already settled" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if error.message == *"already settled" {
if error.message == "already settled".to_string() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

warning: this creates an owned instance just for comparison
--> crates/router/src/connector/datatrans/transformers.rs:517:37
|
517 | if error.message == "already settled".to_string() {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: *"already settled"

Was getting this warning while running the 
cargo clippy --all-features --all-targets

{
"transaction already canceled" => common_enums::AttemptStatus::Voided,
_ => common_enums::AttemptStatus::Failure,
if error.message == *"transaction already canceled" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if error.message == *"transaction already canceled" {
if error.message == "transaction already canceled".to_string() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Please maintain constants for such strings

@deepanshu-iiitu
Copy link
Contributor

We also need to implement the new amount conversion framework.
Please reference the noon changes from this PR and implement the same in Datatrans
#4843

@awasthi21 awasthi21 force-pushed the 5027-datatrans-connector-integration branch 2 times, most recently from 54fed06 to 30f1c61 Compare June 22, 2024 13:58
crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
Comment on lines 297 to 301
Ok(format!(
"{}v1/transactions/{}",
self.base_url(connectors),
connector_payment_id
))
Copy link
Contributor

Choose a reason for hiding this comment

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

here

crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans.rs Outdated Show resolved Hide resolved
crates/router/src/connector/datatrans/transformers.rs Outdated Show resolved Hide resolved
{
"transaction already canceled" => common_enums::AttemptStatus::Voided,
_ => common_enums::AttemptStatus::Failure,
if error.message == *"transaction already canceled" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please maintain constants for such strings

@awasthi21 awasthi21 force-pushed the 5027-datatrans-connector-integration branch from 28afc6f to abd9739 Compare July 5, 2024 10:19
@awasthi21 awasthi21 force-pushed the 5027-datatrans-connector-integration branch from 5bcafa8 to dd2b801 Compare July 8, 2024 10:12
AkshayaFoiger
AkshayaFoiger previously approved these changes Jul 9, 2024
SamraatBansal
SamraatBansal previously approved these changes Jul 11, 2024
SanchithHegde
SanchithHegde previously approved these changes Jul 11, 2024
@awasthi21 awasthi21 force-pushed the 5027-datatrans-connector-integration branch from dd2b801 to 029c4b1 Compare July 11, 2024 10:25
@Gnanasundari24 Gnanasundari24 added this pull request to the merge queue Jul 12, 2024
Merged via the queue into juspay:main with commit f24a407 Jul 12, 2024
11 of 13 checks passed
@@ -23,6 +24,7 @@ const connectorDetails = {
paypal: paypalConnectorDetails,
stripe: stripeConnectorDetails,
trustpay: trustpayConnectorDetails,
datatrans:datatransConnectorDetails
Copy link
Member

@pixincreate pixincreate Jul 14, 2024

Choose a reason for hiding this comment

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

  • Was this connector tested?
  • Since this is a new connector, tests against other connectors needs to be run as well just to see if it affects other connectors although it shouldn't be. Was that done?
  • Looks like lints were not fixed either
  • And yes, creds are not updated

pixincreate added a commit that referenced this pull request Jul 16, 2024
* 'main' of github.com:juspay/hyperswitch: (25 commits)
  fix(logs): ignore request headers while logging (#5273)
  feat(webhooks): add support for custom outgoing webhook http headers (#5275)
  fix(payment_methods): set `requires_cvv` to false when either `connector_mandate_details` or `network_transaction_id` is present during MITs (#5331)
  chore: create justfile for running commands for v1 and v2 migrations (#5325)
  fix(routing): do not update `perform_session_flow_routing` output if the `SessionRoutingChoice` is none (#5336)
  fix(database): modified_at updated for every state change for Payment Attempts (#5312)
  feat(mca): Added recipient connector call for open banking connectors (#3758)
  chore(version): 2024.07.16.0
  refactor(connector): [Mifinity] add a field language_preference in payment request for mifinity payment method data (#5326)
  fix(router): store `customer_acceptance` in payment_attempt, use it in confirm flow for delayed authorizations like external 3ds flow (#5308)
  feat(proxy): add support to pass proxy bypass urls from configs (#5322)
  Docs: Updating Error codes in API-ref (#5296)
  feat(core): [Payouts] Add retrieve flow for payouts (#4936)
  fix(connector): [AUTHORIZEDOTNET] Populate error reason for failure transactions (#5319)
  chore(version): 2024.07.15.0
  feat(logging): Emit a setup error when a restricted keys are used for logging default keys (#5185)
  feat(payment_methods): add support to migrate existing customer PMs from processor to hyperswitch (#5306)
  feat(connector): [DATATRANS] Implement card payments (#5028)
  chore: making of function create_encrypted_data (#5251)
  fix(payments): populate merchant order ref id in list (#5310)
  ...
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 M-api-contract-changes Metadata: This PR involves API contract changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(connector): [DATATRANS] add Connector Payment Flows Code
8 participants