-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
refactor: Pass country and currency as json format in MCA #656
Conversation
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.
Other than the disable all, LGTM.
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.
Minor nit,
Looks good to merge
pub enable_only: Option<Vec<String>>, | ||
/// Type of accepted countries (disable_only, enable_only) | ||
#[serde(rename = "type")] | ||
pub accept_type: 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.
this could be an enum
#[serde(rename = "type")] | ||
pub accept_type: String, | ||
/// List of countries of the provided type | ||
pub list: Option<Vec<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.
are these 2 letter country codes?
these could probably be enum as well
@@ -472,19 +472,25 @@ fn filter_pm_country_based( | |||
) -> (Option<admin::AcceptedCountries>, Option<Vec<String>>, bool) { | |||
match (accepted_countries, req_country_list) { | |||
(None, None) => (None, None, true), | |||
(None, Some(ref r)) => (None, Some(r.to_vec()), false), | |||
(None, Some(ref r)) => ( |
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.
do we need ref
seems you should be able to consume the value instead of ref + to_owned
Type of Change
Description
Supported country and currency , the size exceed the allowed request size, refactoring how these field are passed in the MCA and in the List PM api
Additional Changes
Motivation and Context
How did you test it?
Checklist
cargo +nightly fmt --all
cargo clippy