Tests and fixes for pricing edge cases (#703)

* Better logging when creating prices

* Always pull tiers when pulling prices

* Fix helper to debug missing order

* Minor doc

* Testing edge case where billing_scheme is defined without any tiers

* Fix up user limit console helper

* Shorter log entries

* Doc on pricing tests

* Shortening log fields

* Stripe deep copy

* Allow ids to be passed to order condition check

* Safe tier comparison

* Use deep_copy everywhere

* Price reuse tests

* Missing envs

* Bug fix from deep_dup not working properly

* Log improvement

* running tests locally documentation

.env-example

@@ -8,8 +8,10 @@ STRIPE_TEST_API_KEY=sk_test_
 STRIPE_CLIENT_ID=ca_
 STRIPE_WEBHOOK_SECRET=whsec_
 
-# aws is only used for kms
+SF_CONSUMER_KEY=
+SF_CONSUMER_SECRET=
 
+# aws is only used for kms
 AWS_KMS_SALESFORCE_CREDENTIALS=
 AWS_REGION=
 AWS_KEY=

README.md

@@ -348,6 +348,7 @@ BUNDLE_DISABLE_SHARED_GEMS=1 BUNDLE_PATH=vendor/bundle bundle
     - If the above script is not granting you a new access token for your scratch org, you can also generate a new one via `sfdx force:org:open -u brennen-scratch`
     - This will have refreshed your access token for SFDX, so you can re-run the refresh-tokens script above to replace it in your ENV.
   - `NO_RESCUE=true bundle exec rails test "test/**/test*.rb"` will run the entire test suite
+  - `NO_RESCUE=true RAISE_EXCEPTIONS=true bundle exec rails test "test/**/test*.rb"` will run the entire test suite with exception handling like CI
   - `NO_RESCUE=1` to avoid autoloading pry-rescue in the test suite
 
 - SFDX:

lib/integrations/error_context.rb

@@ -118,14 +118,15 @@ def set_error_context(user: nil, stripe_resource: nil, integration_record: nil,
 
       log.set_context({
         stripe_account_id: user&.stripe_account_id,
-        salesforce_account_id: user&.salesforce_account_id,
+        sf_account_id: user&.salesforce_account_id,
         livemode: user&.livemode,
 
         stripe_resource_id: stripe_resource&.id,
         stripe_resource_type: stripe_resource&.class,
 
-        integration_record_type: integration_record&.sobject_type,
-        integration_record_id: integration_record&.Id,
+        # `translation` meaning the record which triggered the operation
+        sf_translation_type: integration_record&.sobject_type,
+        sf_translation_id: integration_record&.Id,
       }.compact)
 
       # if `set_error_context` is run from a class method, we want to display the class name

lib/integrations/log.rb

@@ -12,8 +12,8 @@
   expand_log do |tags, default_tags|
     if tags[:salesforce_object] && tags[:salesforce_object].is_a?(Restforce::SObject)
       salesforce_object = tags.delete(:salesforce_object)
-      tags[:salesforce_object_id] = salesforce_object.Id
-      tags[:salesforce_object_type] = salesforce_object.sobject_type
+      tags[:sf_object_id] = salesforce_object.Id
+      tags[:sf_object_type] = salesforce_object.sobject_type
     end
 
     if tags[:stripe_resource] && tags[:stripe_resource].respond_to?(:id)

lib/integrations/utilities/stripe_util.rb

@@ -11,6 +11,12 @@ module Integrations::Utilities::StripeUtil
   include Integrations::Log
   include Integrations::ErrorContext
 
+  # NOTE there is a PRIVATE method in ruby bindings to do this for stripe objects, I don't know why it's not public
+  # TODO https://github.com/stripe/stripe-ruby/pull/1120
+  def self.deep_copy(obj)
+    Marshal.load(Marshal.dump(obj))
+  end
+
   # if you null-out a field in a StripeObject it is still sent to the API as an empty string
   # this causes issues for a host of API surfaces in Stripe. Additionally, there is no way to remove
   # a field from the StripeObject once it has been added, thus this incredibly terrible hack

lib/stripe-force/translate/order.rb

@@ -460,7 +460,7 @@ def update_subscription_phases_from_order_amendments(contract_structure)
     ).returns(T::Array[StripeForce::Translate::ContractItemStructure])
   end
   def merge_subscription_line_items(original_aggregate_phase_items, new_phase_items)
-    # avoid mutating the input value
+    # no side effects, please!
     aggregate_phase_items = original_aggregate_phase_items.dup
 
     # TODO `termination_lines` here is what we need for credit calculation
@@ -535,7 +535,10 @@ def terminate_subscription_line_items(original_aggregate_phase_items, terminatio
           .first
 
         if fifo_remaining_line
-          log.info 'reducing quantity on line', reducing_line: fifo_remaining_line.order_line_id
+          log.info 'reducing quantity on line',
+            reducing_line: fifo_remaining_line.order_line_id,
+            quantity: fifo_remaining_line.quantity
+
           fifo_remaining_line.reduce_quantity
         else
           # this should never happen

lib/stripe-force/translate/price.rb

@@ -26,6 +26,11 @@ def create_price_from_pricebook(sf_pricebook_entry)
   end
 
   def pricebook_and_order_line_identical?(sf_pricebook_entry, sf_order_item, sf_product)
+    log.info 'comparing price with order line',
+      sf_pricebook_entry_id: sf_pricebook_entry.Id,
+      sf_order_line_id: sf_order_item.Id,
+      sf_product_id: sf_product.Id
+
     # generate the full params that would be sent to the price object and compare them
     # if they aren't *exactly* the same, this will trigger a new price to be created.
     pricebook_params = generate_price_params_from_sf_object(sf_pricebook_entry, sf_product)
@@ -281,6 +286,8 @@ def extract_tiered_price_params_from_order_line(sf_order_line)
 
     tiers_mode = PriceHelpers.transform_salesforce_consumption_schedule_type_to_tier_mode(consumption_schedule.SBQQ__Type__c)
 
+    log.info 'consumption schedule found, configuring as tiered price'
+
     {
       "tiers" => pricing_tiers,
       "tiers_mode" => tiers_mode,
@@ -347,7 +354,7 @@ def generate_price_params_from_sf_object(sf_object, sf_product)
 
     # if tiered pricing is set up, then we ignore any non-tiered pricing configuration
     if is_tiered_price
-      is_tiered_price = PriceHelpers.sanitize_price_tier_params(stripe_price)
+      stripe_price = PriceHelpers.sanitize_price_tier_params(stripe_price)
     end
 
     # `recurring` could have been partially constructed by the mapper, which is why we use a symbol-based accessor

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

@@ -13,7 +13,10 @@ module PriceHelpers
     def self.auto_archive_prices_on_subscription_schedule(user, subscription_schedule)
       expanded_subscription_schedule = Stripe::SubscriptionSchedule.retrieve({
         id: subscription_schedule.id,
-        expand: %w{phases.items.price phases.add_invoice_items.price},
+        expand: %w{
+          phases.items.price
+          phases.add_invoice_items.price
+        },
       }, user.stripe_credentials)
 
       prices_on_subscription = OrderHelpers.extract_all_items_from_subscription_schedule(expanded_subscription_schedule).map(&:price)
@@ -23,7 +26,7 @@ def self.auto_archive_prices_on_subscription_schedule(user, subscription_schedul
         if active_price.metadata[StripeForce::Utilities::Metadata.metadata_key(user, "auto_archive")]
           log.info 'archiving price', archive_price_id: active_price.id
           active_price.active = false
-          # TODO idempotency_key
+          # TODO idempotency_key, needs class method
           # active_price.save({}, generate_idempotency_key_with_credentials(user, active_price, :archive))
           active_price.save
         end
@@ -75,16 +78,73 @@ def self.recurring_price?(stripe_price)
       stripe_price.type != "one_time"
     end
 
+    sig { params(original_price_1: Stripe::Price, original_price_2: Stripe::Price).returns(T::Boolean) }
+    def self.pricing_tiers_equal?(original_price_1, original_price_2)
+      # to-be-created object:
+      #   {"flat_amount":null,"flat_amount_decimal":null,"unit_amount":3000,"unit_amount_decimal":"3000","up_to":null}
+      #
+      # existing stripe object:
+      #   {"up_to":"inf","unit_amount_decimal":"3000.0"}
+
+      # problems:
+      #   1. Tiers could be in different order
+      #   2. Tiers from stripe could have null fields
+      #   3. Tiers from stripe use `null` for `inf`
+
+      # TODO we will probably need to do some sort of normalization here on tiering amounts
+
+      # maybe the values are both nil, this should satisfy this condition
+      if original_price_1[:tiers] == original_price_2[:tiers]
+        return true
+      end
+
+      price_1 = Integrations::Utilities::StripeUtil.deep_copy(original_price_1)
+      price_1_tiers = price_1.tiers.map {|t| Integrations::Utilities::StripeUtil.delete_nil_fields_from_stripe_object(t) }
+
+      price_2 = Integrations::Utilities::StripeUtil.deep_copy(original_price_2)
+      price_2_tiers = price_2.tiers.map {|t| Integrations::Utilities::StripeUtil.delete_nil_fields_from_stripe_object(t) }
+
+      (price_1_tiers + price_2_tiers).each do |tier|
+        # this is a subhash item and sorbet doesn't have these fields typed
+        tier = T.unsafe(tier)
+
+        if tier[:unit_amount].present? && tier[:unit_amount_decimal].present?
+          Integrations::Utilities::StripeUtil.delete_field_from_stripe_object(
+            tier,
+            :unit_amount
+          )
+        end
+
+        tier[:unit_amount_decimal] = normalize_unit_amount_decimal_for_comparison(tier[:unit_amount_decimal])
+
+        if tier.up_to.nil?
+          tier.up_to = 'inf'
+        end
+      end
+
+      if price_1_tiers != price_2_tiers
+        log.info 'tiers are not equal'
+        # TODO hash diff doesn't support arrays right now
+        # diff: HashDiff::Comparison.new(price_1_tiers.map(&:to_hash), price_2_tiers.map(&:to_hash)).diff
+        false
+      else
+        true
+      end
+    end
+
     sig { params(stripe_price: Stripe::Price).returns(Stripe::Price) }
     def self.sanitize_price_tier_params(stripe_price)
       # no side effects, please!
-      stripe_price = stripe_price.dup
+      stripe_price = Integrations::Utilities::StripeUtil.deep_copy(stripe_price)
 
       # if non-tiered pricing fields are included Stripe will throw an error
       Integrations::Utilities::StripeUtil.delete_field_from_stripe_object(stripe_price, :unit_amount_decimal)
 
       # TODO are there other pricing fields we should delete? It's unclear what the requirements are here?
 
+      # if the field types are not the same, you will get the following error:
+      # Caused by ArgumentError: comparison of Stripe::StripeObject with Stripe::StripeObject failed
+
       # API also requires pricing tiers to be sorted. Wat?
       # TODO https://jira.corp.stripe.com/browse/PLATINT-1573
       stripe_price.tiers.sort! do |a, b|
@@ -120,6 +180,15 @@ def self.transform_salesforce_billing_type_to_usage_type(raw_usage_type)
       end
     end
 
+    sig { params(unit_amount_decimal: T.any(String, BigDecimal)).returns(BigDecimal) }
+    def self.normalize_unit_amount_decimal_for_comparison(unit_amount_decimal)
+      if !unit_amount_decimal.is_a?(BigDecimal)
+        unit_amount_decimal = BigDecimal(unit_amount_decimal)
+      end
+
+      unit_amount_decimal.round(MAX_STRIPE_PRICE_PRECISION)
+    end
+
     sig { params(price_1: Stripe::Price, price_2: Stripe::Price).returns(T::Boolean) }
     def self.price_billing_amounts_equal?(price_1, price_2)
       # metadata *could* have important financial data for downstream systems
@@ -157,23 +226,13 @@ def self.price_billing_amounts_equal?(price_1, price_2)
           raise StripeForce::Errors::RawUserError.new("unit_amount_decimal nil on price objects")
         end
 
-        price_1_decimal = price_1.unit_amount_decimal
-        if !price_1_decimal.is_a?(BigDecimal)
-          price_1_decimal = BigDecimal(price_1_decimal)
-        end
-        price_1_decimal = price_1_decimal.round(MAX_STRIPE_PRICE_PRECISION)
-
-        price_2_decimal = price_2.unit_amount_decimal
-        if !price_2_decimal.is_a?(BigDecimal)
-          price_2_decimal = BigDecimal(price_2_decimal)
-        end
-        price_2_decimal = price_2_decimal.round(MAX_STRIPE_PRICE_PRECISION)
+        price_1_decimal = normalize_unit_amount_decimal_for_comparison(price_1.unit_amount_decimal)
+        price_2_decimal = normalize_unit_amount_decimal_for_comparison(price_2.unit_amount_decimal)
 
         price_1_decimal == price_2_decimal
       elsif price_1.billing_scheme == 'tiered'
-        # TODO we will probably need to do some sort of normalization here on tiering amounts
         # TODO probably need to think through transformed_quantity here and if it could effect this
-        price_1.tiers == price_2.tiers && price_1.tiers_mode == price_2.tiers_mode
+        price_1[:tiers_mode] == price_2[:tiers_mode] && PriceHelpers.pricing_tiers_equal?(price_1, price_2)
       else
         raise StripeForce::Errors::RawUserError.new("Unexpected billing_scheme on price encountered #{price_1.billing_scheme}")
       end

lib/stripe-force/translate/translate.rb

@@ -337,8 +337,18 @@ def retrieve_from_stripe(stripe_class, sf_object)
     stripe_id = sf_object[prefixed_stripe_field(GENERIC_STRIPE_ID)]
     return if stripe_id.nil?
 
+    # TODO need to cast to unsafe other sorbet thinks the code below is unreachable
+    # by default tiers are not returned, but this is an important parameter for price comparison purposes
+    additional_retrieve_params = {}
+    if stripe_class == Stripe::Price
+      additional_retrieve_params = {expand: %w{tiers}}
+    end
+
     begin
-      stripe_record = stripe_class.retrieve(sf_object[prefixed_stripe_field(GENERIC_STRIPE_ID)], @user.stripe_credentials)
+      stripe_record = stripe_class.retrieve(
+        {id: sf_object[prefixed_stripe_field(GENERIC_STRIPE_ID)]}.merge(additional_retrieve_params),
+        @user.stripe_credentials
+      )
 
       # if this ID was provided to us by the user, the metadata does not exist in Stripe
       stripe_record_metadata = stripe_metadata_for_sf_object(sf_object)

scripts/console.rb

@@ -122,8 +122,9 @@ def get_all(object_name)
   sf.query("SELECT #{all_fields} FROM #{object_name}")
 end
 
-def limits
-  sf.limits.slice(*%w{DailyApiRequests DailyAsyncApexExecutions DailyBulkApiBatches DailyFunctionsApiCallLimit DailyStreamingApiEvents})
+# or `sfdx force:limits:api:display -u [email protected]`
+def user_limits(user)
+  user.sf_client.limits.slice(*%w{DailyApiRequests DailyAsyncApexExecutions DailyBulkApiBatches DailyFunctionsApiCallLimit DailyStreamingApiEvents})
 end
 
 # new scratch orgs come without pricebooks active, this causes issues with amendments
@@ -149,12 +150,18 @@ def touch_order(sf_order)
   })
 end
 
-def ensure_order_is_included_in_custom_where_clause(sf_order)
+def ensure_order_is_included_in_custom_where_clause(sf_order_or_id)
+  sf_order = if sf_order_or_id.is_a?(String)
+    sf_get(sf_order_or_id)
+  else
+    sf_order_or_id
+  end
+
   order_poller = StripeForce::OrderPoller.new(@user)
   custom_soql = order_poller.send(:user_specified_where_clause_for_object)
   results = @sf.query("SELECT Id FROM #{SF_ORDER} WHERE Id = '#{sf_order.Id}' " + custom_soql)
 
-  if results.first.empty?
+  if results.first.blank?
     puts "Order is not included in custom soql"
   else
     puts "Order is included in custom soql"

test/integration/translate/test_duplicate_price.rb

@@ -3,6 +3,7 @@
 
 require_relative '../../test_helper'
 
+# the same price id cannot be used more than once on a subscription, we have specific logic to work around this
 class Critic::DuplicatePriceTranslation < Critic::FunctionalTest
   before do
     @user = make_user(save: true)

test/integration/translate/test_price_errors.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+# typed: true
+
+require_relative '../../test_helper'
+
+class Critic::PriceReuse < Critic::FunctionalTest
+  before do
+    @user = make_user(save: true)
+  end
+
+  it 'gracefully fails when a metered price is specified without billing tiers' do
+    @user.field_defaults = {"price" => {"billing_scheme" => "tiered"}}
+    @user.save
+
+    sf_order = create_subscription_order
+
+    # before this test, this would throw a fatal exception without a nice user error
+    exception = assert_raises(Stripe::InvalidRequestError) do
+      StripeForce::Translate.perform_inline(@user, sf_order.Id)
+    end
+
+    assert_match("tiered the parameter tiers must be set", exception.message)
+  end
+end

test/integration/translate/test_price_reuse.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+# typed: true
+
+require_relative '../../test_helper'
+
+class Critic::PriceReuse < Critic::FunctionalTest
+  before do
+    @user = make_user(save: true)
+  end
+
+  it 'uses the same tiered price when there are no customizations on the order line level' do
+    sf_product_id, sf_pricebook_entry_id = create_recurring_per_unit_tiered_price
+
+    # first, translate the price twice and ensure the same ID is used, then we'll test the order line
+    StripeForce::Translate.perform_inline(@user, sf_pricebook_entry_id)
+
+    sf_pricebook_entry = sf.find(SF_PRICEBOOK_ENTRY, sf_pricebook_entry_id)
+    stripe_price_id = sf_pricebook_entry[prefixed_stripe_field(GENERIC_STRIPE_ID)]
+    refute_nil(stripe_price_id)
+
+    stripe_price = Stripe::Price.retrieve(stripe_price_id, @user.stripe_credentials)
+
+    Stripe::Price.expects(:create).never
+
+    StripeForce::Translate.perform_inline(@user, sf_pricebook_entry_id)
+
+    sf_pricebook_entry.refresh
+    assert_equal(stripe_price.id, sf_pricebook_entry[prefixed_stripe_field(GENERIC_STRIPE_ID)])
+
+    sf_order = create_subscription_order(sf_product_id: sf_product_id)
+
+    StripeForce::Translate.perform_inline(@user, sf_order.Id)
+
+    sf_order.refresh
+    stripe_id = sf_order[prefixed_stripe_field(GENERIC_STRIPE_ID)]
+    refute_nil(stripe_id)
+
+    subscription_schedule = Stripe::SubscriptionSchedule.retrieve(stripe_id, @user.stripe_credentials)
+
+    assert_equal(1, subscription_schedule.phases.count)
+    assert_equal(1, subscription_schedule.phases.first&.items&.count)
+
+    item = subscription_schedule.phases.first&.items&.first
+    assert_equal(stripe_price_id, item&.price)
+  end
+end

test/support/salesforce_factory.rb

@@ -162,6 +162,7 @@ def salesforce_standalone_product_with_price(price: 100_00)
       [product_id, pricebook_entry_id]
     end
 
+    # returns the full object, not the ID
     def create_subscription_order(sf_product_id: nil, sf_account_id: nil)
       create_salesforce_order(
         sf_product_id: sf_product_id,