fix: fix some todo's in core (#360)

crates/api_models/src/payment_methods.rs

@@ -1,3 +1,5 @@
+use std::collections;
+
 use common_utils::pii;
 use serde::de;
 
@@ -165,7 +167,7 @@ fn set_or_reject_duplicate<T, E: de::Error>(
     }
 }
 
-#[derive(Debug, serde::Serialize, serde::Deserialize)]
+#[derive(Eq, PartialEq, Hash, Debug, serde::Serialize, serde::Deserialize)]
 pub struct ListPaymentMethodResponse {
     pub payment_method: api_enums::PaymentMethodType,
     pub payment_method_types: Option<Vec<api_enums::PaymentMethodSubType>>,
@@ -183,7 +185,7 @@ pub struct ListPaymentMethodResponse {
 
 #[derive(Debug, serde::Serialize)]
 pub struct ListCustomerPaymentMethodsResponse {
-    pub enabled_payment_methods: Vec<ListPaymentMethodResponse>,
+    pub enabled_payment_methods: collections::HashSet<ListPaymentMethodResponse>,
     pub customer_payment_methods: Vec<CustomerPaymentMethod>,
 }
 
@@ -210,7 +212,7 @@ pub struct CustomerPaymentMethod {
     pub created: Option<time::PrimitiveDateTime>,
 }
 
-#[derive(Clone, Debug, serde::Serialize, serde::Deserialize)]
+#[derive(Eq, PartialEq, Hash, Clone, Debug, serde::Serialize, serde::Deserialize)]
 #[serde(rename_all = "snake_case")]
 #[non_exhaustive]
 pub enum PaymentExperience {

crates/router/src/core/errors.rs

@@ -56,6 +56,8 @@ pub enum StorageError {
     DuplicateValue(String),
     #[error("KV error")]
     KVError,
+    #[error("Serialization failure")]
+    SerializationFailed,
     #[error("MockDb error")]
     MockDbError,
 }

crates/router/src/core/errors/utils.rs

@@ -42,7 +42,6 @@ pub(crate) trait ConnectorErrorExt {
     fn to_verify_failed_response(self) -> error_stack::Report<errors::ApiErrorResponse>;
 }
 
-// FIXME: The implementation can be improved by handling BOM maybe?
 impl ConnectorErrorExt for error_stack::Report<errors::ConnectorError> {
     fn to_refund_failed_response(self) -> error_stack::Report<errors::ApiErrorResponse> {
         let data = match self.current_context() {

crates/router/src/core/payment_methods/cards.rs

@@ -84,11 +84,11 @@ pub async fn add_payment_method(
                 metadata: req.metadata,
                 created: Some(common_utils::date_time::now()),
                 payment_method_issuer_code: req.payment_method_issuer_code,
-                recurring_enabled: false,           //TODO
+                recurring_enabled: false,           //[#219]
-                installment_payment_enabled: false, //TODO
+                installment_payment_enabled: false, //[#219]
                 payment_experience: Some(vec![
                     api_models::payment_methods::PaymentExperience::RedirectToUrl,
-                ]), //TODO
+                ]), //[#219]
             })
         }
     }
@@ -354,7 +354,7 @@ pub async fn list_payment_methods(
     db: &dyn db::StorageInterface,
     merchant_account: storage::MerchantAccount,
     mut req: api::ListPaymentMethodRequest,
-) -> errors::RouterResponse<Vec<api::ListPaymentMethodResponse>> {
+) -> errors::RouterResponse<collections::HashSet<api::ListPaymentMethodResponse>> {
     let payment_intent = helpers::verify_client_secret(
         db,
         merchant_account.storage_scheme,
@@ -392,8 +392,8 @@ pub async fn list_payment_methods(
             error.to_not_found_response(errors::ApiErrorResponse::MerchantAccountNotFound)
         })?;
 
-    // TODO: Deduplicate payment methods
+    let mut response: collections::HashSet<api::ListPaymentMethodResponse> =
-    let mut response: Vec<api::ListPaymentMethodResponse> = Vec::new();
+        collections::HashSet::new();
     for mca in all_mcas {
         let payment_methods = match mca.payment_methods_enabled {
             Some(pm) => pm,
@@ -420,7 +420,7 @@ pub async fn list_payment_methods(
 async fn filter_payment_methods(
     payment_methods: Vec<serde_json::Value>,
     req: &mut api::ListPaymentMethodRequest,
-    resp: &mut Vec<api::ListPaymentMethodResponse>,
+    resp: &mut collections::HashSet<api::ListPaymentMethodResponse>,
     payment_intent: Option<&storage::PaymentIntent>,
     payment_attempt: Option<&storage::PaymentAttempt>,
     address: Option<&storage::Address>,
@@ -465,7 +465,7 @@ async fn filter_payment_methods(
                 };
 
                 if filter && filter2 && filter3 {
-                    resp.push(payment_method_object);
+                    resp.insert(payment_method_object);
                 }
             }
         }
@@ -586,8 +586,8 @@ pub async fn list_customer_payment_method(
             error.to_not_found_response(errors::ApiErrorResponse::MerchantAccountNotFound)
         })?;
 
-    // TODO: Deduplicate payment methods
+    let mut enabled_methods: collections::HashSet<api::ListPaymentMethodResponse> =
-    let mut enabled_methods: Vec<api::ListPaymentMethodResponse> = Vec::new();
+        collections::HashSet::new();
     for mca in all_mcas {
         let payment_methods = match mca.payment_methods_enabled {
             Some(pm) => pm,
@@ -598,7 +598,7 @@ pub async fn list_customer_payment_method(
             if let Ok(payment_method_object) =
                 serde_json::from_value::<api::ListPaymentMethodResponse>(payment_method)
             {
-                enabled_methods.push(payment_method_object);
+                enabled_methods.insert(payment_method_object);
             }
         }
     }
@@ -854,11 +854,11 @@ pub async fn retrieve_payment_method(
             payment_method_issuer_code: pm
                 .payment_method_issuer_code
                 .map(ForeignInto::foreign_into),
-            recurring_enabled: false,           //TODO
+            recurring_enabled: false,           //[#219]
-            installment_payment_enabled: false, //TODO
+            installment_payment_enabled: false, //[#219]
             payment_experience: Some(vec![
                 api_models::payment_methods::PaymentExperience::RedirectToUrl,
-            ]), //TODO,
+            ]), //[#219],
         },
     ))
 }

crates/router/src/core/payments.rs

@@ -454,7 +454,7 @@ where
     pub email: Option<masking::Secret<String, pii::Email>>,
 }
 
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct CustomerDetails {
     pub customer_id: Option<String>,
     pub name: Option<masking::Secret<String, masking::WithType>>,

crates/router/src/core/payments/helpers.rs

@@ -146,8 +146,6 @@ pub async fn get_token_for_recurring_mandate(
         .await
         .map_err(|error| error.to_not_found_response(errors::ApiErrorResponse::MandateNotFound))?;
 
-    // TODO: Make currency in payments request as Currency enum
-
     let customer = req.customer_id.clone().get_required_value("customer_id")?;
 
     let payment_method_id = {
@@ -733,7 +731,7 @@ pub async fn make_pm_data<'a, F: Clone, R>(
     let payment_method = match (request, token) {
         (_, Some(token)) => Ok::<_, error_stack::Report<errors::ApiErrorResponse>>(
             if payment_method_type == Some(storage_enums::PaymentMethodType::Card) {
-                // TODO: Handle token expiry
+                // [#196]: Handle token expiry
                 let (pm, tokenize_value2) =
                     Vault::get_payment_method_data_from_locker(state, &token).await?;
                 utils::when(
@@ -766,7 +764,7 @@ pub async fn make_pm_data<'a, F: Clone, R>(
                         field_name: "payment_method_type".to_owned(),
                     })
                 })?;
-                // TODO: Implement token flow for other payment methods
+                // [#195]: Implement token flow for other payment methods
                 None
             },
         ),

crates/router/src/core/payments/operations/payment_start.rs

@@ -92,13 +92,9 @@ impl<F: Send + Clone> GetTracker<F, PaymentData<F>, api::PaymentsStartRequest> f
         payment_intent.shipping_address_id = shipping_address.clone().map(|i| i.address_id);
         payment_intent.billing_address_id = billing_address.clone().map(|i| i.address_id);
 
-        //TODO: get customer from db?
         let customer_details = CustomerDetails {
             customer_id: payment_intent.customer_id.clone(),
-            name: None,
+            ..CustomerDetails::default()
-            email: None,
-            phone: None,
-            phone_country_code: None,
         };
 
         let connector_response = db

crates/router/src/core/webhooks.rs

@@ -173,14 +173,14 @@ async fn trigger_webhook_to_merchant(
 
     match response {
         Err(e) => {
-            // TODO: Schedule webhook for retry.
+            // [#217]: Schedule webhook for retry.
             Err(e)
                 .into_report()
                 .change_context(errors::WebhooksFlowError::CallToMerchantFailed)?;
         }
         Ok(res) => {
             if !res.status().is_success() {
-                // TODO: Schedule webhook for retry.
+                // [#217]: Schedule webhook for retry.
                 Err(errors::WebhooksFlowError::NotReceivedByMerchant).into_report()?;
             }
         }

crates/router/src/db/payment_intent.rs

@@ -52,7 +52,7 @@ mod storage {
             api,
             storage::{enums, kv, payment_intent::*},
         },
-        utils::storage_partitioning::KvStorePartition,
+        utils::{self, storage_partitioning::KvStorePartition},
     };
 
     #[async_trait::async_trait]
@@ -155,9 +155,11 @@ mod storage {
 
                     let updated_intent = payment_intent.clone().apply_changeset(this.clone());
                     // Check for database presence as well Maybe use a read replica here ?
-                    let redis_value = serde_json::to_string(&updated_intent)
+
-                        .into_report()
+                    let redis_value =
-                        .change_context(errors::StorageError::KVError)?;
+                        utils::Encode::<PaymentIntent>::encode_to_string_of_json(&updated_intent)
+                            .change_context(errors::StorageError::SerializationFailed)?;
+
                     let updated_intent = self
                         .redis_conn
                         .set_hash_fields(&key, ("pi", &redis_value))

crates/router/src/db/refund.rs

@@ -437,7 +437,6 @@ mod storage {
 
                     let updated_refund = refund.clone().apply_changeset(this.clone());
                     // Check for database presence as well Maybe use a read replica here ?
-                    // TODO: Add a proper error for serialization failure
 
                     let lookup = self
                         .get_lookup_by_lookup_id(&key)
@@ -451,7 +450,7 @@ mod storage {
                         utils::Encode::<storage_types::Refund>::encode_to_string_of_json(
                             &updated_refund,
                         )
-                        .change_context(errors::StorageError::KVError)?;
+                        .change_context(errors::StorageError::SerializationFailed)?;
 
                     self.redis_conn
                         .set_hash_fields(&key, (field, redis_value))

crates/router/src/types/storage/payment_intent.rs

@@ -36,7 +36,7 @@ impl PaymentIntentDbExt for PaymentIntent {
         let starting_after = &pc.starting_after;
         let ending_before = &pc.ending_before;
 
-        //TODO: Replace this with Boxable Expression and pass it into generic filter
+        //[#350]: Replace this with Boxable Expression and pass it into generic filter
         // when https://github.com/rust-lang/rust/issues/52662 becomes stable
         let mut filter = <Self as HasTable>::table()
             .filter(dsl::merchant_id.eq(merchant_id.to_owned()))