feat(coupons): check coupons apply entire plan interval

[StaberindeZA]

Because

• Currently monthly repeating coupons can be used even though they could
  be applied to only part of the plan interval. (eg. Coupon duration of
  3 months, applied to a 6 month plan.) This leads to possible confusion
  for the customer, and should not be allowed.

This pull request

• Checks if the coupon duration will apply to the entire interval for
  a given plan
• If the coupon duration does not apply to the entire interval, report an
  error to Sentry.

Issue that this pull request solves

Closes: #12015

Checklist

Put an x in the boxes that apply

• My commit is GPG signed.
• If applicable, I have modified or added tests which pass locally.
• I have added necessary documentation (if appropriate).
• I have verified that my changes render correctly in RTL (if appropriate).

[IvoJP]

Code looks good, test coverage looks good. I left one question and one comment about a variable name - neither of which require action necessarily. I'll leave it to you discretion. The question regards string comparisons but it might be outside the scope of this ticket, the second is about if the variable name might be confusing for us in the future - I am not sure it will be and debated leaving the comment.

I am going to approve the PR and leave it to you discretion if either of the comments need to be addressed. LGTM.

[IvoJP]

I am not sure if we do this anywhere else or if we enforce this, but should we be doing case insensitive string comparisons? Is the couponDuration property serialized by stripe or do we have anything that works with the stripe response and guarantees that its always all lower case?

[IvoJP]

this will likely seem like a pedantic (right adjective?) comment but i think it might go a long way in terms of readability/understandability in the future when we have to revisit any of this code. so this method (checkPromotionCodeForPlan) is called from both methods to retreive a promo code - the one that checks the valid promo code for plan which is used by previewInvoice and the method that returns the coupon info regardless of validity so that we can determine whether a coupon is expired or redeemed or whatever.

should we consider changing the name of this variable? up to you, the name of the promo codes is already validPromoCodes but im wondering if in the future we might wonder why a method that retrieves invalid coupon calls into this and has a variabe called valid which could be true even for a coupon that is invalid in the stripe sense of validity.

[StaberindeZA]

Thank you for your comments, and I agree a better name would make it more readable.

It also got me to thinking that this is actually the wrong place for this check to happen, since it actually validates the Coupon (coupon duration in this case), and doesn't actually have anything to do with the Promotion Code and shouldn't inhibit retrieving the Promotion Code for a plan.

I was thinking a better place could be the verifyPromotionAndCoupon method. What do you think?

[IvoJP]

Good catch, I agree with the above, and took a look at the other commit for the alternative approach. I am not sure when any of these methods will be used in any other context but you're right about the appropriateness of where things are called and it should at least help us with debugging/call stacks later on.

Feel free to pull those changes into here and just ping me if I need to re-approve. I have already taken a look though, so if my approval stays after the change, feel free to merge when you're ready.