remove the paid/free distinction in the reads #1073
Conversation
| @@ -261,7 +261,7 @@ class AccountController( | |||
| .map(subs => subscriptionSelector(subscriptionName, s"the sfUser $contact", subs)), | |||
| ) | |||
| contributionPlan <- SimpleEitherT.fromEither(subscription.plan match { | |||
| case p: SubscriptionPlan.Contributor @unchecked /* extra guard needed due to type erasure --> */ if p.product == Contribution => Right(p) | |||
| case p if p.product == Contribution => Right(p) | |||
There was a problem hiding this comment.
before the fact that the check had passed was remembered by the types, now it's a runtime only check.
| @@ -8,7 +8,7 @@ import com.gu.memsub.Product.GuardianPatron | |||
| import com.gu.memsub.Subscription._ | |||
| import com.gu.memsub._ | |||
| import com.gu.memsub.subsv2.ReaderType.Direct | |||
| import com.gu.memsub.subsv2.{CovariantNonEmptyList, PaidCharge, PaidSubscriptionPlan, Subscription} | |||
| import com.gu.memsub.subsv2.{CovariantNonEmptyList, SingleCharge, Subscription, SubscriptionPlan} | |||
There was a problem hiding this comment.
PaidSubscriptionPlan is called SubscriptionPlan
PaidCharge is called SingleCharge (to make it more easily searchable I didn't just go for "Charge")
feel free to make other suggestions though as it's easy enough to tweak right now
There was a problem hiding this comment.
Or RatePlan and RatePlanCharge to map better to what Zuora calls them? This would probably be my preference actually as it's what we call them in other code bases.
There was a problem hiding this comment.
I will do it in another PR as it's going to cause a lot of conflicts as i've got 2 more PRs lined up
| def getProductDescription(subscription: Subscription[SubscriptionPlan.AnyPlan]) = if (subscription.asMembership.isDefined) { | ||
| s"${subscription.plan.productName} membership" | ||
| } else if (subscription.asContribution.isDefined) { |
There was a problem hiding this comment.
asMembership (and asContribution) are deleted elsewhere in this PR, but in the end it was just checking the product and downcasting in the vein of a pattern match, so now I've just made it do the check and not worry about the type system as it was just doing option.isDefined anyway
| @@ -217,10 +216,9 @@ object FrontendId { | |||
| case object Quarterly extends FrontendId { val name = "Quarterly" } | |||
| case object Yearly extends FrontendId { val name = "Yearly" } | |||
| case object Introductory extends FrontendId { val name = "Introductory" } | |||
| case object Free extends FrontendId { val name = "Free" } | |||
There was a problem hiding this comment.
frontend id is a special field in the catalog so that we can distinguish between different rateplans in the same product more reliably but in a way that's agnostic between DEV/(UAT)/PROD. In more modern codebases we just have a Map of the product rate plan IDs in the codebase.
| type Member = CatalogPlan[Product.Membership, ChargeList with SingleBenefit[MemberTier], Current] | ||
| type Supporter[+B <: BillingPeriod] = CatalogPlan[Product.Membership, SingleCharge[Supporter.type, B], Current] | ||
| type Partner[+B <: BillingPeriod] = CatalogPlan[Product.Membership, SingleCharge[Partner.type, B], Current] | ||
| type Patron[+B <: BillingPeriod] = CatalogPlan[Product.Membership, SingleCharge[Patron.type, B], Current] | ||
|
|
||
| type PaidMember[+B <: BillingPeriod] = CatalogPlan[Product.Membership, PaidCharge[PaidMemberTier, B], Current] |
There was a problem hiding this comment.
Member and PaidMember were unused and deleted, not sure why the diff is so messed up, the rest just changed from PaidCharge to SingleCharge
| type RecurringContentSubscription[+B <: BillingPeriod] = CatalogPlan[Product.ContentSubscription, PaidCharge[Benefit, B], Current] | ||
| type RecurringPlan[+B <: BillingPeriod] = CatalogPlan[Product, PaidCharge[Benefit, B], Current] |
There was a problem hiding this comment.
these two were unused
johnduffell
force-pushed
the
jd-remove-paid-classes
branch
from
070c7fb
to
965c9dc
Compare
| sealed trait SingleBenefit[+B <: Benefit] { | ||
| def benefit: B | ||
| } |
There was a problem hiding this comment.
SingleBenefit was not used any more. We can probably get rid of benefit for subscriptions at runtime completely, although it's still needed for catalog (unless we pull in a copy of the Map from product to product rate plan id, so that we deserialise based on IDs rather than shape)
| chargedThrough: Option[LocalDate], // this is None if the sub hasn't been billed yet (on a free trial) | ||
| chargedThrough: Option[ | ||
| LocalDate, | ||
| ], // this is None if the sub hasn't been billed yet (on a free trial) or if you have been billed it is the date at which you'll next be billed |
There was a problem hiding this comment.
I think this comment got split up so I've reassembled it again
| type Paid = PaidSubscriptionPlan[Product, PaidChargeList] | ||
| type Free = FreeSubscriptionPlan[Product, FreeChargeList] | ||
|
|
||
| type Member = SubscriptionPlan[Product.Membership, ChargeList with SingleBenefit[MemberTier]] | ||
| type PaidMember = PaidSubscriptionPlan[Product.Membership, PaidCharge[Benefit.PaidMemberTier, BillingPeriod]] | ||
|
|
||
| type Supporter = PaidSubscriptionPlan[Product.Membership, PaidCharge[Benefit.Supporter.type, BillingPeriod]] | ||
| type Partner = PaidSubscriptionPlan[Product.Membership, PaidCharge[Benefit.Partner.type, BillingPeriod]] | ||
| type Patron = PaidSubscriptionPlan[Product.Membership, PaidCharge[Benefit.Patron.type, BillingPeriod]] | ||
|
|
||
| type ContentSubscription = PaidSubscriptionPlan[Product.ContentSubscription, PaidChargeList] | ||
| type Digipack = PaidSubscriptionPlan[Product.ZDigipack, PaidCharge[Benefit.Digipack.type, BillingPeriod]] | ||
| type SupporterPlus = PaidSubscriptionPlan[Product.SupporterPlus, PaidCharge[Benefit.SupporterPlus.type, BillingPeriod]] | ||
| type Delivery = PaidSubscriptionPlan[Product.Delivery, PaperCharges] | ||
| type Voucher = PaidSubscriptionPlan[Product.Voucher, PaperCharges] | ||
| type DigitalVoucher = PaidSubscriptionPlan[Product.DigitalVoucher, PaperCharges] | ||
|
|
||
| type WeeklyZoneA = PaidSubscriptionPlan[Product.WeeklyZoneA, PaidCharge[Weekly.type, BillingPeriod]] | ||
| type WeeklyZoneB = PaidSubscriptionPlan[Product.WeeklyZoneB, PaidCharge[Weekly.type, BillingPeriod]] | ||
| type WeeklyPlan = PaidSubscriptionPlan[Product.Weekly, PaidCharge[Weekly.type, BillingPeriod]] | ||
|
|
||
| type Contributor = PaidSubscriptionPlan[Product.Contribution, PaidCharge[Benefit.Contributor.type, BillingPeriod]] |
There was a problem hiding this comment.
I made all these unused, which paves the way to remove the type parameter completely from Subscription and all the subscriptionService.current and other methods throughtout the codebase. Then the type system will no longer be aware of what type of subscription is being passed around (it only really needed to know it for change contribution amount code)
| if (plan.product == Product.Contribution) | ||
| contributorPlanCancelled(_ => !sub.isCancelled && !plan.lastChangeType.contains("Remove")) | ||
| else if (plan.product == Product.Digipack && plan.charges.billingPeriod == OneTimeChargeBillingPeriod) | ||
| digipackGiftEnded(_ => sub.termEndDate >= dateToCheck) | ||
| else | ||
| paidPlanEnded(_ => plan.end >= dateToCheck) |
There was a problem hiding this comment.
now that it's all runtime checks we can use if statements instead of pattern match
| implicit val legacy: CatPlanReads[Legacy] = new CatPlanReads[Legacy] { | ||
| override def read(p: ProductIds, c: CatalogZuoraPlan): ValidationNel[String, Legacy] = | ||
| (c.status == Status.legacy).option(Status.legacy).toSuccess(s"Needed legacy, got ${c.status}".wrapNel) | ||
| } | ||
|
|
||
| implicit val statusReads: CatPlanReads[Status] = new CatPlanReads[Status] { | ||
| override def read(p: ProductIds, c: CatalogZuoraPlan): ValidationNel[String, Status] = | ||
| legacy.read(p, c).map(identity[Status]) orElse currentReads.read(p, c).map(identity[Status]) | ||
| } |
| @@ -43,11 +43,6 @@ object ChargeListReads { | |||
|
|
|||
| type PlanChargeMap = Map[ProductRatePlanChargeId, Benefit] | |||
|
|
|||
| def pure[A](a: A) = new ChargeListReads[A] { | |||
| @@ -66,11 +61,6 @@ object ChargeListReads { | |||
| contributor: ProductId, | |||
| ) | |||
|
|
|||
| // shorthand syntax for creating new ChargeListReads | |||
| def apply[A](f: (PlanChargeMap, List[ZuoraCharge]) => ValidationNel[String, A]) = new ChargeListReads[A] { | |||
| weeklyDomesticReads.read(p, z, c) orElse2 | ||
| weeklyRestOfWorldReads.read(p, z, c).withTrace("paperReads") | ||
| weeklyRestOfWorldReads.read(p, z, c)).withTrace("paperReads") |
There was a problem hiding this comment.
the brackets were wrong in a couple of places in this file, no functionality issue but the logs would be less clear
| @@ -114,38 +114,12 @@ class SubscriptionService[M[_]: Monad](pids: ProductIds, futureCatalog: => M[Cat | |||
| val allSubscriptionsForSubscriberName = subJson.flatMap { jsValue => | |||
| SubscriptionTransform.getSubscription[P](catalog, pids)(jsValue).withTrace("getAllValidSubscriptionsFromJson") | |||
| } | |||
| warnOnMissingChargedThroughDate(allSubscriptionsForSubscriberName) | |||
There was a problem hiding this comment.
I don't think anyone was looking at this warning
|
Seen on PROD (merged by @johnduffell 6 minutes and 24 seconds ago) Please check your changes! Sentry Release: members-data-api |
follows on and is currently branched from #1072
Sorry it's so many changes, but actually it's pretty mechanical on the whole. I've gone through and given a commentary on the changes so it should be more clear what the reasoning is.
This PR simplifies the subscription hierarchy by removing the Paid/Free distinction completely and any associated classes.
I also did some related tidyup including reducing the amount of implicits where they were unused or could be non implicit.