Remove subscription guesswork in SaasPurchaseNotificationService

The following discussion from !12341 should be addressed:

• @vitallium started a discussion: (+1 comment)

  While changes look good, I think we should spend some time checking the zuora_order_subscription method in this class. I see there is a comment:

  The subscription object is loaded via SubscriptionsFinder.new().by_id_or_name(<SUBSCRIPTION_NAME>), this could return the incorrect version. Which isn't a big deal for other emails given the main plan is a paid plan. This method ensures that the correct subscription version is used for the standalone add-on email.

  This comment lacks an important point - it doesn't answer the question of why it does not matter what version we use here. If this service requires a specific version of a subscription, then why can't we provide it outside?

  After looking at zuora_order_subscription method, I could be wrong, but its idea is to get the latest subscription for the given order, which can be achieved by calling this method Zuora::Local::Subscription.find_by(name: subscription.name). Of course, calling the Zuora layer here directly is a code smell, so we can move that to the Subscription class directly:

  class Subscription
    def latest_version?
      @zuora_subscription.is_latest_version
    end
  end

  Additionally, zuora_order_subscription is a bit of a tricky variable name to understand, in my opinion. We have subscription and zuora_order_subscription in this class. What's the difference between them? Why do we need both? And so on. I think we can reduce the complexity of this class by checking the following key points:

  • See if we can pass the correct subscription through params.
  • See if we can get rid of the zuora_order_subscription method.

  This is a non-blocking thought, of course.

  @aish.sub added:

Agree with the points mentioned here.

Adding a few more points:

After looking at zuora_order_subscription method, I could be wrong, but its idea is to get the latest subscription for the given order, which can be achieved by calling this method Zuora::Local::Subscription.find_by(name: subscription.name)

A subscription can have multiple versions, and it doesn't looks like we want the latest subscription version here and therefore cannot use Zuora::Local::Subscription.find_by(name: subscription.name).

Rather, we are trying to fetch the subscription associated with an Order. I would say the latest is not as important here because in our current usage a Zuora Order can only be associated with one Subscription version.

---

At best, we could to pass the appropriate subscription to the service instead of leaving a risk to getting the wrong version.

Alternatively, we could modify @subscription as the following in the initializer. Which is basically: use the subscription associated with the order if order_number is present or fallback to subscription passed to the service.

def initialize(amendment_types:, order:, subscription:, z_order_number: nil)
  ...
  @subscription = subscription_for_order(order) || subscription
end

And, non-blocking comment of course! I'll spin up a follow-up so we can continue the conversations and refactor as appropriate.