From dd03ab5886f9e9cd40f22b91105bfbab266f3933 Mon Sep 17 00:00:00 2001 From: mbianco-stripe <45374579+mbianco-stripe@users.noreply.github.com> Date: Thu, 18 Aug 2022 09:52:25 -0700 Subject: [PATCH] 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 | 25 ++++++++++++++++----- lib/stripe-force/translate/order/helpers.rb | 12 ++-------- lib/stripe-force/translate/price.rb | 4 +++- lib/stripe-force/translate/price/helpers.rb | 11 ++++----- 4 files changed, 31 insertions(+), 21 deletions(-) diff --git a/lib/integrations/utilities/stripe_util.rb b/lib/integrations/utilities/stripe_util.rb index 3d4a4f0563..01f345204b 100644 --- a/lib/integrations/utilities/stripe_util.rb +++ b/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. diff --git a/lib/stripe-force/translate/order/helpers.rb b/lib/stripe-force/translate/order/helpers.rb index 2bd7122f2b..dd4438972e 100644 --- a/lib/stripe-force/translate/order/helpers.rb +++ b/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.> diff --git a/lib/stripe-force/translate/price.rb b/lib/stripe-force/translate/price.rb index 2f2c6c186d..8ee736e2ea 100644 --- a/lib/stripe-force/translate/price.rb +++ b/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 diff --git a/lib/stripe-force/translate/price/helpers.rb b/lib/stripe-force/translate/price/helpers.rb index b888e8797b..bafe138652 100644 --- a/lib/stripe-force/translate/price/helpers.rb +++ b/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