fix(connector): [Bluesnap] Throw proper error message for redirection scenario #1367
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
srujanchikke
added
A-connector-integration
github-actions
bot
added
the
S-conventions-not-followed
srujanchikke
changed the title
github-actions
bot
removed
the
S-conventions-not-followed
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
jagan-jaya
requested changes
Jun 7, 2023
| }) | ||
| .map( | ||
| |checkout_status| payments::CallConnectorAction::StatusUpdate { | ||
| status: checkout_status.into(), |
There was a problem hiding this comment.
use from here
Suggested change
| status: checkout_status.into(), | |
| status: <Status>::from(checkout_status), |
crates/router/src/connector/utils.rs
Outdated
| &self, | ||
| ) -> Result<(Secret<String>, Secret<String>), errors::ConnectorError> { | ||
| let card_holder_name = self.card_holder_name.peek(); | ||
| let words: Vec<&str> = card_holder_name.split_whitespace().collect(); |
There was a problem hiding this comment.
it works for " " too
crates/router/src/connector/utils.rs
Outdated
| words.first().unwrap_or(&"").to_string() | ||
| }); | ||
| let last_name = Secret::new(if words.len() > 1 { | ||
| words.last().unwrap_or(&"").to_string() |
There was a problem hiding this comment.
Suggested change
| words.last().unwrap_or(&"").to_string() | |
| words.last().unwrap_or_default() |
There was a problem hiding this comment.
we can't use unwrap_or_default() here as the trait bound &&str: std::default::Default is not satisfied
crates/router/src/connector/utils.rs
Outdated
| let first_name = Secret::new(if words.len() > 1 { | ||
| words[..words.len() - 1].join(" ") | ||
| } else { | ||
| words.first().unwrap_or(&"").to_string() |
There was a problem hiding this comment.
Suggested change
| words.first().unwrap_or(&"").to_string() | |
| words.first().unwrap_or_default() |
crates/router/src/services/api.rs
Outdated
| let response = ErrorResponse { | ||
| code: code.unwrap_or_else(|| consts::NO_ERROR_MESSAGE.to_string()), | ||
| message: message.unwrap_or_else(|| consts::NO_ERROR_MESSAGE.to_string()), | ||
| status_code: 200, |
There was a problem hiding this comment.
Add a comment justifying why 200 is needed here
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
jagan-jaya
previously approved these changes
Jun 8, 2023
jagan-jaya
added
R-waiting-on-L2
ArjunKarthik
requested changes
Jun 8, 2023
crates/router/src/services/api.rs
Outdated
| payments::CallConnectorAction::StatusUpdate(status) => { | ||
| payments::CallConnectorAction::StatusUpdate { | ||
| status, | ||
| code, |
There was a problem hiding this comment.
rename this to error_code & error_ message
crates/router/src/services/api.rs
Outdated
| router_data.status = status; | ||
| let error_response = code.zip(message).map(|(error_code, error_message)| { |
There was a problem hiding this comment.
This has to changed to or condition, zip will return tuple of values only if both the values are Some
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
ArjunKarthik
previously approved these changes
Jun 9, 2023
Signed-off-by: chikke srujan <121822803+srujanchikke@users.noreply.github.com>
ArjunKarthik
approved these changes
Jun 9, 2023
jagan-jaya
approved these changes
Jun 9, 2023
jagan-jaya
added
S-ready-for-merge
R-L2-completed
vspecky
approved these changes
Jun 9, 2023
NarsGNA
approved these changes
Jun 9, 2023
lsampras
approved these changes
Jun 9, 2023
| error_message: redirection_result | ||
| .info | ||
| .as_ref() | ||
| .and_then(|info| info.errors.as_ref().and_then(|error| error.first())) |
There was a problem hiding this comment.
Should we have rather joined the errors into a single string?
SanchithHegde
removed
S-ready-for-merge
R-L1-completed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
This PR has following changes
1)
CardHolderInfofield is added in payment request of Bluesnap2)Update error message and error code in statusUpdate in CallConnectorAction for redirection response
Additional Changes
Motivation and Context
Previously for connector bluesnap we were not sending proper error message when 3ds is disabled in dashboard . This pr modifies statusUpdate in CallConnectorAction by making it as struct which contains error message and error code as well . This PR also have
cardHolderInfofield added in payment request for bluesnap .How did you test it?
i tested it locally and ran unit tests for bluesnap
Checklist
cargo +nightly fmt --allcargo clippy