refactor(stripe): return all the missing fields in a request (#935)

Co-authored-by: jeeva <[email protected]>
Co-authored-by: Sanchith Hegde <[email protected]>
Co-authored-by: ItsMeShashank <[email protected]>

.gitignore

@@ -256,3 +256,5 @@ loadtest/*.tmp/
 
 # Nix output
 result*
+
+.idea/

crates/common_utils/Cargo.toml

@@ -37,7 +37,6 @@ ring = "0.16.20"
 serde = { version = "1.0.160", features = ["derive"] }
 serde_json = "1.0.96"
 serde_urlencoded = "0.7.1"
-signal-hook-tokio = { version = "0.3.1", features = ["futures-v0_3"], optional = true }
 signal-hook = { version = "0.3.15", optional = true }
 tokio = { version = "1.27.0", features = ["macros", "rt-multi-thread"], optional = true }
 thiserror = "1.0.40"
@@ -48,6 +47,9 @@ md5 = "0.7.0"
 masking = { version = "0.1.0", path = "../masking" }
 router_env = { version = "0.1.0", path = "../router_env", features = ["log_extra_implicit_fields", "log_custom_entries_to_extra"], optional = true }
 
+[target.'cfg(not(target_os = "windows"))'.dependencies]
+signal-hook-tokio = { version = "0.3.1", features = ["futures-v0_3"], optional = true }
+
 [dev-dependencies]
 fake = "2.5.0"
 proptest = "1.1.0"

crates/common_utils/src/signals.rs

@@ -1,13 +1,16 @@
 //! Provide Interface for worker services to handle signals
 
+#[cfg(not(target_os = "windows"))]
 use futures::StreamExt;
+#[cfg(not(target_os = "windows"))]
 use router_env::logger;
 use tokio::sync::mpsc;
 
 ///
 /// This functions is meant to run in parallel to the application.
 /// It will send a signal to the receiver when a SIGTERM or SIGINT is received
 ///
+#[cfg(not(target_os = "windows"))]
 pub async fn signal_handler(mut sig: signal_hook_tokio::Signals, sender: mpsc::Sender<()>) {
     if let Some(signal) = sig.next().await {
         logger::info!(
@@ -31,9 +34,47 @@ pub async fn signal_handler(mut sig: signal_hook_tokio::Signals, sender: mpsc::S
     }
 }
 
+///
+/// This functions is meant to run in parallel to the application.
+/// It will send a signal to the receiver when a SIGTERM or SIGINT is received
+///
+#[cfg(target_os = "windows")]
+pub async fn signal_handler(_sig: DummySignal, _sender: mpsc::Sender<()>) {}
+
 ///
 /// This function is used to generate a list of signals that the signal_handler should listen for
 ///
+#[cfg(not(target_os = "windows"))]
 pub fn get_allowed_signals() -> Result<signal_hook_tokio::SignalsInfo, std::io::Error> {
     signal_hook_tokio::Signals::new([signal_hook::consts::SIGTERM, signal_hook::consts::SIGINT])
 }
+
+///
+/// This function is used to generate a list of signals that the signal_handler should listen for
+///
+#[cfg(target_os = "windows")]
+pub fn get_allowed_signals() -> Result<DummySignal, std::io::Error> {
+    Ok(DummySignal)
+}
+
+///
+/// Dummy Signal Handler for windows
+///
+#[cfg(target_os = "windows")]
+#[derive(Debug, Clone)]
+pub struct DummySignal;
+
+#[cfg(target_os = "windows")]
+impl DummySignal {
+    ///
+    /// Dummy handler for signals in windows (empty)
+    ///
+    pub fn handle(&self) -> Self {
+        self.clone()
+    }
+
+    ///
+    /// Hollow implementation, for windows compatibility
+    ///
+    pub fn close(self) {}
+}

crates/router/Cargo.toml

@@ -69,7 +69,6 @@ serde_path_to_error = "0.1.11"
 serde_qs = { version = "0.12.0", optional = true }
 serde_urlencoded = "0.7.1"
 serde_with = "2.3.2"
-signal-hook-tokio = { version = "0.3.1", features = ["futures-v0_3"] }
 signal-hook = "0.3.15"
 strum = { version = "0.24.1", features = ["derive"] }
 thiserror = "1.0.40"
@@ -94,6 +93,9 @@ aws-sdk-s3 = "0.25.0"
 aws-config = "0.55.1"
 infer = "0.13.0"
 
+[target.'cfg(not(target_os = "windows"))'.dependencies]
+signal-hook-tokio = { version = "0.3.1", features = ["futures-v0_3"]}
+
 [build-dependencies]
 router_env = { version = "0.1.0", path = "../router_env", default-features = false }
 

crates/router/src/connector/stripe/transformers.rs

@@ -1,14 +1,14 @@
 use api_models::{self, enums as api_enums, payments};
 use base64::Engine;
-use common_utils::{errors::CustomResult, fp_utils, pii};
+use common_utils::{errors::CustomResult, pii};
 use error_stack::{IntoReport, ResultExt};
 use masking::{ExposeInterface, ExposeOptionInterface, Secret};
 use serde::{Deserialize, Serialize};
 use url::Url;
 use uuid::Uuid;
 
 use crate::{
-    consts,
+    collect_missing_value_keys, consts,
     core::errors,
     services,
     types::{self, api, storage::enums},
@@ -429,30 +429,21 @@ fn validate_shipping_address_against_payment_method(
     payment_method: &StripePaymentMethodType,
 ) -> Result<(), error_stack::Report<errors::ConnectorError>> {
     if let StripePaymentMethodType::AfterpayClearpay = payment_method {
-        fp_utils::when(shipping_address.name.is_none(), || {
-            Err(errors::ConnectorError::MissingRequiredField {
-                field_name: "shipping.address.first_name",
-            })
-        })?;
-
-        fp_utils::when(shipping_address.line1.is_none(), || {
-            Err(errors::ConnectorError::MissingRequiredField {
-                field_name: "shipping.address.line1",
-            })
-        })?;
-
-        fp_utils::when(shipping_address.country.is_none(), || {
-            Err(errors::ConnectorError::MissingRequiredField {
-                field_name: "shipping.address.country",
-            })
-        })?;
+        let missing_fields = collect_missing_value_keys!(
+            ("shipping.address.first_name", shipping_address.name),
+            ("shipping.address.line1", shipping_address.line1),
+            ("shipping.address.country", shipping_address.country),
+            ("shipping.address.zip", shipping_address.zip)
+        );
 
-        fp_utils::when(shipping_address.zip.is_none(), || {
-            Err(errors::ConnectorError::MissingRequiredField {
-                field_name: "shipping.address.zip",
+        if !missing_fields.is_empty() {
+            return Err(errors::ConnectorError::MissingRequiredFields {
+                field_names: missing_fields,
             })
-        })?;
+            .into_report();
+        }
     }
+
     Ok(())
 }
 
@@ -1799,3 +1790,192 @@ pub struct DisputeObj {
     pub dispute_id: String,
     pub status: String,
 }
+
+#[cfg(test)]
+mod test_validate_shipping_address_against_payment_method {
+    #![allow(clippy::unwrap_used)]
+    use api_models::enums::CountryCode;
+    use masking::Secret;
+
+    use crate::{
+        connector::stripe::transformers::{
+            validate_shipping_address_against_payment_method, StripePaymentMethodType,
+            StripeShippingAddress,
+        },
+        core::errors,
+    };
+
+    #[test]
+    fn should_return_ok() {
+        // Arrange
+        let stripe_shipping_address = create_stripe_shipping_address(
+            Some("name".to_string()),
+            Some("line1".to_string()),
+            Some(CountryCode::AD),
+            Some("zip".to_string()),
+        );
+
+        let payment_method = &StripePaymentMethodType::AfterpayClearpay;
+
+        //Act
+        let result = validate_shipping_address_against_payment_method(
+            &stripe_shipping_address,
+            payment_method,
+        );
+
+        // Assert
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn should_return_err_for_empty_name() {
+        // Arrange
+        let stripe_shipping_address = create_stripe_shipping_address(
+            None,
+            Some("line1".to_string()),
+            Some(CountryCode::AD),
+            Some("zip".to_string()),
+        );
+
+        let payment_method = &StripePaymentMethodType::AfterpayClearpay;
+
+        //Act
+        let result = validate_shipping_address_against_payment_method(
+            &stripe_shipping_address,
+            payment_method,
+        );
+
+        // Assert
+        assert!(result.is_err());
+        let missing_fields = get_missing_fields(result.unwrap_err().current_context()).to_owned();
+        assert_eq!(missing_fields.len(), 1);
+        assert_eq!(missing_fields[0], "shipping.address.first_name");
+    }
+
+    #[test]
+    fn should_return_err_for_empty_line1() {
+        // Arrange
+        let stripe_shipping_address = create_stripe_shipping_address(
+            Some("name".to_string()),
+            None,
+            Some(CountryCode::AD),
+            Some("zip".to_string()),
+        );
+
+        let payment_method = &StripePaymentMethodType::AfterpayClearpay;
+
+        //Act
+        let result = validate_shipping_address_against_payment_method(
+            &stripe_shipping_address,
+            payment_method,
+        );
+
+        // Assert
+        assert!(result.is_err());
+        let missing_fields = get_missing_fields(result.unwrap_err().current_context()).to_owned();
+        assert_eq!(missing_fields.len(), 1);
+        assert_eq!(missing_fields[0], "shipping.address.line1");
+    }
+
+    #[test]
+    fn should_return_err_for_empty_country() {
+        // Arrange
+        let stripe_shipping_address = create_stripe_shipping_address(
+            Some("name".to_string()),
+            Some("line1".to_string()),
+            None,
+            Some("zip".to_string()),
+        );
+
+        let payment_method = &StripePaymentMethodType::AfterpayClearpay;
+
+        //Act
+        let result = validate_shipping_address_against_payment_method(
+            &stripe_shipping_address,
+            payment_method,
+        );
+
+        // Assert
+        assert!(result.is_err());
+        let missing_fields = get_missing_fields(result.unwrap_err().current_context()).to_owned();
+        assert_eq!(missing_fields.len(), 1);
+        assert_eq!(missing_fields[0], "shipping.address.country");
+    }
+
+    #[test]
+    fn should_return_err_for_empty_zip() {
+        // Arrange
+        let stripe_shipping_address = create_stripe_shipping_address(
+            Some("name".to_string()),
+            Some("line1".to_string()),
+            Some(CountryCode::AD),
+            None,
+        );
+        let payment_method = &StripePaymentMethodType::AfterpayClearpay;
+
+        //Act
+        let result = validate_shipping_address_against_payment_method(
+            &stripe_shipping_address,
+            payment_method,
+        );
+
+        // Assert
+        assert!(result.is_err());
+        let missing_fields = get_missing_fields(result.unwrap_err().current_context()).to_owned();
+        assert_eq!(missing_fields.len(), 1);
+        assert_eq!(missing_fields[0], "shipping.address.zip");
+    }
+
+    #[test]
+    fn should_return_error_when_missing_multiple_fields() {
+        // Arrange
+        let expected_missing_field_names: Vec<&'static str> =
+            vec!["shipping.address.zip", "shipping.address.country"];
+        let stripe_shipping_address = create_stripe_shipping_address(
+            Some("name".to_string()),
+            Some("line1".to_string()),
+            None,
+            None,
+        );
+        let payment_method = &StripePaymentMethodType::AfterpayClearpay;
+
+        //Act
+        let result = validate_shipping_address_against_payment_method(
+            &stripe_shipping_address,
+            payment_method,
+        );
+
+        // Assert
+        assert!(result.is_err());
+        let missing_fields = get_missing_fields(result.unwrap_err().current_context()).to_owned();
+        for field in missing_fields {
+            assert!(expected_missing_field_names.contains(&field));
+        }
+    }
+
+    fn get_missing_fields(connector_error: &errors::ConnectorError) -> Vec<&'static str> {
+        if let errors::ConnectorError::MissingRequiredFields { field_names } = connector_error {
+            return field_names.to_vec();
+        }
+
+        vec![]
+    }
+
+    fn create_stripe_shipping_address(
+        name: Option<String>,
+        line1: Option<String>,
+        country: Option<CountryCode>,
+        zip: Option<String>,
+    ) -> StripeShippingAddress {
+        StripeShippingAddress {
+            name: name.map(Secret::new),
+            line1: line1.map(Secret::new),
+            country,
+            zip: zip.map(Secret::new),
+            city: Some(String::from("city")),
+            line2: Some(Secret::new(String::from("line2"))),
+            state: Some(Secret::new(String::from("state"))),
+            phone: Some(Secret::new(String::from("pbone number"))),
+        }
+    }
+}