feat(connector): [Authorizedotnet] Pass surcharge in payments request#12851
Conversation
|
Review changes with Changed Files
|
XyneSpaces
left a comment
XyneSpaces
left a comment
There was a problem hiding this comment.
Code Review: Authorizedotnet Surcharge Support
I've reviewed the changes for adding surcharge support to the Authorizedotnet connector. The implementation correctly extracts surcharge details and adds them to the transaction request. However, I've identified a few areas for improvement:
Issues Found:
-
Code Duplication: The surcharge calculation logic is duplicated across 4 different
TryFromimplementations. Consider extracting this into a helper function. -
Error Context: The error message could include more diagnostic information (currency, amount) to aid debugging production issues.
-
Test Coverage: The PR checklist indicates unit tests were not added. Connector changes should include tests for the new surcharge mapping logic.
Inline Comments:
See detailed comments on specific lines below.
| ) -> Result<Self, Self::Error> { | ||
| let surcharge_amount = item | ||
| .router_data | ||
| .request |
There was a problem hiding this comment.
ℹ️ Suggestion: The error message could include more context for debugging. Consider:
.attach_printable(format!(
"Failed to convert surcharge amount to major unit float for currency: {:?}",
item.router_data.request.currency
))This would help identify issues faster in production logs.
XyneSpaces
left a comment
XyneSpaces
left a comment
There was a problem hiding this comment.
Code Review: Authorizedotnet Surcharge Support
This PR adds surcharge support to the Authorizedotnet connector. Below are my findings:
🟡 Medium: Code Duplication
The surcharge calculation logic is duplicated 4 times across different impl blocks (lines ~900, ~1048, ~1166, ~1281). Consider extracting this into a helper function to improve maintainability.
🟢 Good
- Proper use of
#[serde(skip_serializing_if = "Option::is_none")]to avoid sending empty surcharge objects - Appropriate error handling with
change_contextandattach_printable - Correct conversion from minor to major units using
to_major_unit_as_f64
ℹ️ Suggestion
The Surcharge struct could benefit from a doc comment explaining it's for Authorizedotnet's surcharge API.
| subsequent_auth_information: Option<SubsequentAuthInformation>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| surcharge: Option<Surcharge>, | ||
| } |
There was a problem hiding this comment.
📝 Documentation Suggestion
Add a doc comment to the Surcharge struct:
/// Represents a surcharge amount to be added to the transaction.
/// Used for passing fee amounts to Authorizedotnet's API.
#[derive(Debug, Serialize)]
struct Surcharge {
amount: FloatMajorUnit,
}
🔍 Automated PR Review SummaryReview Status: CommentedFile Changed: Key Findings
Recommendations
Testing NotesThe PR includes manual testing evidence showing successful payment flows with surcharge. Unit tests for the new surcharge logic would strengthen confidence. This is an automated review. Please address the findings before merging. |
github-actions
Bot
removed
the
S-test-ready
1e01987 to
74f20e4
Compare
github-actions
Bot
removed
the
S-test-ready
github-actions
Bot
removed
the
S-test-ready
6 participants
Type of Change
Description
Pass surcharge in payments request whenever applicable.
Additional Changes
Motivation and Context
How did you test it?
Payment Request:
Response:
Make a
/eligibilitycall:Response:
Response:
Setup Mandate:
Response:
MIT:
Response:
Checklist
cargo +nightly fmt --allcargo clippy