Feat(connector): [Fiuu] Add Card Flows#5786
Merged
Gnanasundari24 merged 6 commits intomainfrom Sep 4, 2024
Merged
Conversation
|
Review changes with SemanticDiff. Analyzed 14 of 20 files. Overall, the semantic diff is 8% smaller than the GitHub diff.
|
SamraatBansal
requested changes
Sep 3, 2024
crates/hyperswitch_connectors/src/connectors/fiuu/transformers.rs
Outdated
Show resolved
Hide resolved
crates/hyperswitch_connectors/src/connectors/fiuu/transformers.rs
Outdated
Show resolved
Hide resolved
config/development.toml
Outdated
Comment on lines
+123
to
+124
| "fiuu", | ||
| "fiuu", |
Member
There was a problem hiding this comment.
This has been repeated twice.
crates/common_utils/src/types.rs
Outdated
Comment on lines
+585
to
+589
| impl Display for StringMajorUnit { | ||
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| write!(f, "{}", self.0) | ||
| } | ||
| } |
Member
Comment on lines
+49
to
+60
| let response_str = String::from_utf8_lossy(data); | ||
| let mut map = HashMap::new(); | ||
| for line in response_str.lines() { | ||
| if let Some((key, value)) = line.split_once('=') { | ||
| map.insert(key.to_string(), value.to_string()); | ||
| } | ||
| } | ||
| let json = serde_json::to_string(&map) | ||
| .map_err(|_| errors::ConnectorError::ResponseDeserializationFailed)?; | ||
|
|
||
| let response: T = serde_json::from_str(&json) | ||
| .map_err(|_| errors::ConnectorError::ResponseDeserializationFailed)?; |
Member
There was a problem hiding this comment.
- We may be losing some information because we're calling
from_utf8_lossy(). - Can we avoid the split by
=and find a better way to deserialize the response if possible? - The 3 step process where we collect key-values to a map, serialize it as JSON, then deserialize it into
Tmust preferably be avoided, if we can find a better way to deserialize the response. - If we end sticking with this, then at least log the errors, do not ignore them.
Comment on lines
+151
to
+152
| let serialized = serde_json::to_value(&data) | ||
| .map_err(|_| common_errors::ParsingError::EncodeError("json-value"))?; |
Member
There was a problem hiding this comment.
Do not ignore errors, log them at the very least.
Comment on lines
+159
to
+162
| Value::String(s) => s.clone(), | ||
| Value::Number(n) => n.to_string(), | ||
| Value::Bool(b) => b.to_string(), | ||
| _ => "".to_string(), |
Member
There was a problem hiding this comment.
We'd end up serializing arrays and nested object with an empty string value. I'm sure we shouldn't be doing that?
Comment on lines
+205
to
+209
| #[derive(Debug, Serialize, Deserialize)] | ||
| pub enum RequestType { | ||
| REDIRECT, | ||
| RESPONSE, | ||
| } |
Member
There was a problem hiding this comment.
Nit: Can we use PascalCase names instead with serde(rename_all = "UPPERCASE") (or screaming snake case, as required)?
Comment on lines
+336
to
+337
| pub enum RefundType { | ||
| P, |
Member
There was a problem hiding this comment.
Can we preferably use a verbose name with serde(rename = "P") here, instead of using a single character enum variant?
Comment on lines
+469
to
+473
| if stat_name == "settled" || stat_name == "captured" { | ||
| Ok(enums::AttemptStatus::Charged) | ||
| } else { | ||
| Ok(enums::AttemptStatus::Authorized) | ||
| } |
Member
There was a problem hiding this comment.
Can't we use an enum instead for the stat_name field?
| }) | ||
| } | ||
| } | ||
| // Reminder |
Member
There was a problem hiding this comment.
Can you remove this comment?
SamraatBansal
approved these changes
Sep 4, 2024
AkshayaFoiger
approved these changes
Sep 4, 2024
SanchithHegde
approved these changes
Sep 4, 2024
Comment on lines
+340
to
346
| impl RefundType { | ||
| pub fn as_str(&self) -> &'static str { | ||
| match self { | ||
| Self::Partial => "P", | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
You anyway derive strum::Display, you can add #[strum(serialize = "P")] on the enum variant, and take advantage of the Display implementation for generating the signature.
Contributor
Author
There was a problem hiding this comment.
Will add the change in the next upcoming PR(for Duitnow,FPX payment Method)
Narayanbhat166
approved these changes
Sep 4, 2024
Contributor
There was a problem hiding this comment.
Please handle the redirection flows in the separate PR
other than that LGTM
likhinbopanna
approved these changes
Sep 4, 2024
pixincreate
added a commit
that referenced
this pull request
Sep 5, 2024
* 'main' of github.com:juspay/hyperswitch: feat(customer_v2): Add customer V2 delete api (#5518) chore(version): 2024.09.05.0 feat(user_roles): get user role details (#5777) feat(users): Add profile level invites (#5793) refactor(router): profile based routes for payouts (#5794) Feat(connector): [Fiuu] Add Card Flows (#5786) fix(cypress): fix fiservemea configs for cypress (#5772) fix(cypress): `api_key` check in cypress (#5787) feat(payment_methods_v2): Implemented Diesel and Domain models for v2 (#5700) fix(payout): query for getting a list of active payout IDs (#5771) refactor(router): remove admin v2 intermediate features (#5780) feat(revert): populate payment method details in payments response (#5785) chore(version): 2024.09.04.0 fix(connector): skip 3DS in `network_transaction_id` flow for cybersource (#5781) refactor(euclid): check the authenticity of profile_id being used (#5647) feat(analytics): refactor and introduce analytics APIs to accommodate OrgLevel, MerchantLevel and ProfileLevel authentication (#5729) fix(router): make customer details None in the `Psync` flow if the customer is deleted (#5732) feat(connector): [DEUTSCHE] Add template code (#5774) chore(version): 2024.09.03.1 fix(router): send post message to window.parent instead of window.top in external 3ds flow (#5778)
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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
Added Card Flows for Fiuu
Additional Changes
Motivation and Context
Card Flows for Fiuu
How did you test it?
Added Cypress test
For 3DS
Request
Response
Redirection Response(after Sync)
Checklist
cargo +nightly fmt --allcargo clippy