-
Notifications
You must be signed in to change notification settings - Fork 4.6k
feat(connector): [Novalnet] Add zero auth mandate #6631
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
Conversation
Changed Files
|
df85bb8 to
4fdf8c1
Compare
crates/hyperswitch_connectors/src/connectors/novalnet/transformers.rs
Outdated
Show resolved
Hide resolved
crates/hyperswitch_connectors/src/connectors/novalnet/transformers.rs
Outdated
Show resolved
Hide resolved
crates/hyperswitch_connectors/src/connectors/novalnet/transformers.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Narayanbhat166 @sai-harsha-vardhan Should we consider passing MerchantConnectorAccountId here, instead of the connector name? Basically update the create_webhook_url() function as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's always recommended to use MerchantConnectorAccountId in webhook url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sai-harsha-vardhan @SanchithHegde I made a new fn create_webhook_url_using_mca_id ,but only used it in line 226 (crates/router/src/core/payments/transformers.rs:226) . In the other places where create_webhook_url() is currently used the MerchantConnectorAccountId value is Optional, so we cant replace connector name with simply MerchantConnectorAccountId.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we will always have the merchant_connector_id when constructing the router data. So if it is not found you can raise an internal server error.
I do not see any harm in updating the existing webhook function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the existing create_webhook_url fn
4fdf8c1 to
0b20c37
Compare
0b20c37 to
73667aa
Compare
0fd4b9a to
64d9202
Compare
4c94106
4c94106 to
004a8b6
Compare
004a8b6 to
9f96ab9
Compare
c595f10
a3cf7e0 to
c595f10
Compare
e742315 to
62920e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to do to_string if we are taking only the reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here create_webhook_url() is accepting &String , and get_string_repr() returns &str that is why used
to_string()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can update the create_webhook_url() function to accept &str, if not done already.
62920e9 to
a13dbf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| merchant_connector_id_or_connector_name: &String, | |
| merchant_connector_id_or_connector_name: &str, |
Then we can avoid the to_string() calls.
a13dbf6 to
48a0de4
Compare
Type of Change
Description
create_webhook_urlfn can acceptmerchant_connector_account_idinstead ofconnector_nameAdditional Changes
Motivation and Context
How did you test it?
Note:
Skipping no3ds test cases as it is not supported by novalnet cc: @Likhin Bopanna
Tested manually
https://docs.google.com/document/d/1i4SSX837Dh9ipfOz24bDwJF8osR_UPmfPLWUbLP3bh0/edit?usp=sharing
Webhooks working as expected for /payments. Tested locally.
Checklist
cargo +nightly fmt --allcargo clippy