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(router): add payments incremental authorization api #3038

Merged
merged 18 commits into from
Dec 4, 2023

Conversation

sai-harsha-vardhan
Copy link
Contributor

@sai-harsha-vardhan sai-harsha-vardhan commented Dec 2, 2023

Type of Change

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

Description

add payments incremental authorization api and add support for cybersource connector

Additional Changes

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

Motivation and Context

How did you test it?

Tested Manually

  1. send request_incremental_authorization as false in payments request, to see incremental_authorization_allowed being sent as false in response
    image

  2. now, try doing an increment authorization for the above payment to see not allowed error
    CURL
    curl --location --request POST '{baseUrl}/payments/{payment_id}/incremental_authorization' \ --header 'Content-Type: application/json' \ --header 'Accept: application/json' \ --header 'api-key: {API_KEY}' \ --data-raw '{ "amount": 11005 }'
    image

  3. try sending request_incremental_authorization as true for payment for which capture_method is automatic to see not allowed error
    image

  4. now send request_incremental_authorization as true for payment with capture_method manual to see incremental_authorization_allowed being true in response
    image

  5. now, for above payment_id try doing an increment authorization with amount less than original amount to see now allowed error
    image

  6. now, do the increment authorization with amount greater than original amount to see final authorized amount
    image

  7. all the authorization details along with authorization_count can be seen in the response

image image
  1. retrieve the payment to see authorization details
image

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

@sai-harsha-vardhan sai-harsha-vardhan added A-connector-integration Area: Connector integration A-core Area: Core flows C-feature Category: Feature request or enhancement S-waiting-on-review Status: This PR has been implemented and needs to be reviewed M-database-changes Metadata: This PR involves database schema changes M-api-contract-changes Metadata: This PR involves API contract changes labels Dec 2, 2023
@sai-harsha-vardhan sai-harsha-vardhan self-assigned this Dec 2, 2023
@sai-harsha-vardhan sai-harsha-vardhan requested review from a team as code owners December 2, 2023 16:25
@sai-harsha-vardhan sai-harsha-vardhan changed the title feat(router): add incremental authorization api feat(router): add payments incremental authorization api Dec 2, 2023
Comment on lines 2300 to 2301
pub code: Option<String>,
pub message: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Is this only for error_code and error_message, or some other code and message value can be present here? Can you also add doc comments to the fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be any code and message sent by connector

crates/diesel_models/src/payment_attempt.rs Outdated Show resolved Hide resolved
crates/diesel_models/src/payment_intent.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
-- Your SQL goes here
ALTER TABLE payment_intent ADD COLUMN IF NOT EXISTS authorization_count INTEGER;
Copy link
Member

Choose a reason for hiding this comment

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

All the previous payments should have this value as zero, will adding a default as 0 help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to distinguish for normal payments with incremental auth performed payments

&self,
_authorization: storage::AuthorizationNew,
) -> CustomResult<storage::Authorization, errors::StorageError> {
// TODO: Implement function for `MockDb`
Copy link
Member

Choose a reason for hiding this comment

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

Can you create good first issues for these? The issue can be linked to the parent issue #172

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +45 to +55
async fn find_all_authorizations_by_merchant_id_payment_id(
&self,
merchant_id: &str,
payment_id: &str,
) -> CustomResult<Vec<storage::Authorization>, errors::StorageError> {
let conn = connection::pg_connection_read(self).await?;
storage::Authorization::find_by_merchant_id_payment_id(&conn, merchant_id, payment_id)
.await
.map_err(Into::into)
.into_report()
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be an olap operation, can you include the feature flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using this in get_trackers of PaymentsRetrieve

Comment on lines 99 to 107
Box::pin(payment_response_update_tracker(
db,
payment_id,
payment_data,
router_data,
storage_scheme,
))
.await
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you have a separate post_update_tracker for this operation? Adding all of the update trackers in a single functions makes it bloated.

.store
.insert_authorization(authorization_new.clone())
.await
.to_not_found_response(errors::ApiErrorResponse::InternalServerError)
Copy link
Member

Choose a reason for hiding this comment

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

This should be to_duplicate_response, and the error can be GenericDuplicateError

Comment on lines +167 to +168
error_code,
error_message,
Copy link
Member

Choose a reason for hiding this comment

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

why will a success response have error code and message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case of 2xx failures, we need error_code and error_message in IncrementalAuthorizationResponse

Comment on lines +198 to +206
let authorizations = db
.store
.find_all_authorizations_by_merchant_id_payment_id(
&router_data.merchant_id,
&payment_data.payment_intent.payment_id,
)
.await
.to_not_found_response(errors::ApiErrorResponse::InternalServerError)
.attach_printable("failed while retrieving authorizations")?;
Copy link
Member

Choose a reason for hiding this comment

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

Is this to update the status of latest authorization?

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, this is to fetch all the authorizations and send back in the incremental auth response

deepanshu-iiitu
deepanshu-iiitu previously approved these changes Dec 4, 2023
Copy link
Contributor

@deepanshu-iiitu deepanshu-iiitu left a comment

Choose a reason for hiding this comment

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

Cybersource changes look good.

Comment on lines 775 to 782
incremental_authorization_allowed: if payment_data
.incremental_authorization_details
.is_none()
{
Some(false)
} else {
None
},
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this?

@likhinbopanna likhinbopanna added this pull request to the merge queue Dec 4, 2023
Copy link
Contributor

@ArjunKarthik ArjunKarthik left a comment

Choose a reason for hiding this comment

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

@sai-harsha-vardhan I reviewed connector code, it looks good to me

Merged via the queue into main with commit a0cfdd3 Dec 4, 2023
10 of 12 checks passed
@likhinbopanna likhinbopanna deleted the add-incremental-authorization-api branch December 4, 2023 13:18
@pixincreate pixincreate removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label Dec 15, 2023
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-feature Category: Feature request or enhancement 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.

6 participants