Fix price comparison logic (#681)

* Extract out nil field removal as a helper

* Fixing price equality logic

* Fixing typing

lib/integrations/utilities/stripe_util.rb

@@ -16,22 +16,37 @@ module Integrations::Utilities::StripeUtil
   # a field from the StripeObject once it has been added, thus this incredibly terrible hack
   # TODO https://jira.corp.stripe.com/browse/PLATINT-1572
   sig { params(stripe_object: Stripe::StripeObject, stripe_field_name: Symbol).void }
-  def delete_field_from_stripe_object(stripe_object, stripe_field_name)
+  def self.delete_field_from_stripe_object(stripe_object, stripe_field_name)
     stripe_object.instance_eval do
       # the top-level keys of `@values` seem to be symbolized
       @values.delete(stripe_field_name)
     end
   end
 
-  module_function :delete_field_from_stripe_object
+  # for cleansing objects pulled *from* the Stripe API before sending data *to* the API
+  # this is also helpful for removing nonsense data when doing comparison between objects
+  sig { params(stripe_object: Stripe::StripeObject).returns(Stripe::StripeObject) }
+  def self.delete_nil_fields_from_stripe_object(stripe_object)
+    stripe_object
+      .keys
+      # all fields that are nil from the API should be removed before sending to the API
+      .select {|field_sym| stripe_object.send(field_sym).nil? }
+      .each do |field_sym|
+        Integrations::Utilities::StripeUtil.delete_field_from_stripe_object(
+          stripe_object,
+          field_sym
+        )
+      end
+
+    stripe_object
+  end
 
   # not specific to Stripe, but exclusively used in translating data to Stripe
-  def is_integer_value?(value)
+  sig { params(value: T.any(String, Integer, BigDecimal, Float)).returns(T::Boolean) }
+  def self.is_integer_value?(value)
     value.to_i == value.to_f
   end
 
-  module_function :is_integer_value?
-
   def stripe_class_from_id(stripe_object_id, raise_on_missing: true)
     # Setting raise_on_missing to false should only be used on internal
     # support tooling.

lib/stripe-force/translate/order/helpers.rb

@@ -57,6 +57,7 @@ def self.transform_payment_terms_to_days_until_due(raw_days_until_due)
       end
     end
 
+    # TODO this should move to the price helpers
     sig do
       params(
         user: StripeForce::User,
@@ -144,16 +145,7 @@ def self.sanitize_subscription_schedule_phase_params(original_phases)
       # You can't pass back the phase in it's original format, it must be modified to avoid:
       # 'You passed an empty string for 'phases[0][collection_method]'. We assume empty values are an attempt to unset a parameter; however 'phases[0][collection_method]' cannot be unset. You should remove 'phases[0][collection_method]' from your request or supply a non-empty value.'
       phases.each do |phase|
-        phase
-          .keys
-          # all fields that are nil from the API should be removed before sending to the API
-          .select {|field_sym| phase.send(field_sym).nil? }
-          .each do |field_sym|
-            Integrations::Utilities::StripeUtil.delete_field_from_stripe_object(
-              phase,
-              field_sym
-            )
-          end
+        Integrations::Utilities::StripeUtil.delete_nil_fields_from_stripe_object(phase)
       end
 
       # (Status 400) (Request req_6sXw1ulKg8naEO) You may only specify one of these parameters: end_date, iterations.>

lib/stripe-force/translate/price.rb

@@ -86,7 +86,7 @@ def create_price_for_order_item(sf_order_item)
       generated_stripe_price = generate_price_params_from_sf_object(sf_pricebook_entry, sf_product)
 
       # this should never happen if our identical check is correct, unless the data in Salesforce is mutated over time
-      if PriceHelpers.price_billing_amounts_equal?(existing_stripe_price, generated_stripe_price)
+      if !PriceHelpers.price_billing_amounts_equal?(existing_stripe_price, generated_stripe_price)
         raise Integrations::Errors::UnhandledEdgeCase.new("expected generated prices to be equal, but they differed")
       end
 
@@ -325,6 +325,8 @@ def generate_price_params_from_sf_object(sf_object, sf_product)
       extract_tiered_price_params_from_order_line(sf_object)
     end
 
+    # NOTE not using `tiered_price?` helper here since this method
+    #      constructs the params that the helper uses
     is_tiered_price = !tiered_pricing_params.empty?
 
     # omitting price param here, this should be defined upstream

lib/stripe-force/translate/price/helpers.rb

@@ -119,13 +119,14 @@ def self.transform_salesforce_billing_type_to_usage_type(raw_usage_type)
       end
     end
 
-    # right now, this is NOT used everywhere there is a price comparison, we should leverage this more broadly across the codebase
+    # TODO right now, this is NOT used everywhere there is a price comparison, we should leverage this more broadly across the codebase
     sig { params(price_1: Stripe::Price, price_2: Stripe::Price).returns(T::Boolean) }
     def self.price_billing_amounts_equal?(price_1, price_2)
-      billing_amounts_equal = BigDecimal(price_1.unit_amount_decimal.to_s) != BigDecimal(price_2.unit_amount_decimal.to_s) ||
-        price_1.recurring.present? != price_2.recurring.present? ||
-        price_1.recurring.interval != price_2.recurring[:interval] ||
-        price_1.recurring.interval_count != price_2.recurring[:interval_count]
+      # TODO in some cases the `unit_amount_decimal` is already a bigd, we should handle this more cleanly
+      billing_amounts_equal = BigDecimal(price_1.unit_amount_decimal.to_s) == BigDecimal(price_2.unit_amount_decimal.to_s) &&
+        price_1.recurring.present? == price_2.recurring.present? &&
+        price_1.recurring.interval == price_2.recurring[:interval] &&
+        price_1.recurring.interval_count == price_2.recurring[:interval_count]
 
       if !billing_amounts_equal
         log.info 'price not equal', diff: HashDiff::Comparison.new(price_1.to_hash, price_2.to_hash).diff