-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove "high level" subscription model #1079
Conversation
@@ -22,7 +22,7 @@ class TouchpointBackends( | |||
contactRepositoryOverride: Option[ContactRepository] = None, | |||
subscriptionServiceOverride: Option[SubscriptionService[Future]] = None, | |||
zuoraRestServiceOverride: Option[ZuoraRestService] = None, | |||
catalogServiceOverride: Option[Future[CatalogMap]] = None, | |||
catalogServiceOverride: Option[Future[Catalog]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Catalog is now an object containing the configured Products for each product id as well as the Product rate plans from the catalog (this makes it easier to pass around together)
@@ -60,7 +60,7 @@ class TouchpointComponents( | |||
|
|||
lazy val supporterProductDataTable = environmentConfig.getString("supporter-product-data.table") | |||
|
|||
lazy val productIds = config.SubsV2ProductIds(environmentConfig.getConfig("zuora.productIds")) | |||
lazy val productIds = config.SubsV2ProductIds.load(environmentConfig.getConfig("zuora.productIds")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to load
- reduce the use of apply
methods
_ <- sendSubscriptionCancelledEmail( | ||
request.user.primaryEmailAddress, | ||
contact, | ||
subscription.plan, | ||
subscription.plan(catalog).productType(catalog), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subscription
was a high level but is now a low level one.
Where fields no longer exist, I have created a similarly named method, which takes the catalog and generates the old object on the fly.
regarding adding .productType(catalog)
in this case we were passing in the high level rate plan object, but only using the productType, so I've narrowed the method parameter
.map { | ||
case Right(subscriptionList) => | ||
logger.info(s"Successfully retrieved payment details result for identity user: $userId") | ||
val productsResponseWrites = new ProductsResponseWrites(catalog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toJson's implicit writes (used just below) now need the catalog, so I've put the code in a class ProductsResponseWrites
and passed the catalog in.
d57021c
to
c2233c4
Compare
@@ -129,10 +127,11 @@ class AccountController( | |||
subscription.termEndDate, | |||
) | |||
result = cancellationEffectiveDate.getOrElse("now").toString | |||
catalog <- EitherT.rightT(services.futureCatalog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now there's a lot of places we have to flatmap the catalog, I did wonder whether to carry on passing the catalog in to the deserialiser, but instead of using it, just store it in a private field in the Subscription object. Then when we are reading the old fields (which are now defined on the fly) it could return the values without requiring the catalog as a parameter.
I feel like keeping the low level model clean is probably going to be cleaner and make the tests clearer. It just makes the business logic a bit cluttered.
billingPeriod <- SimpleEitherT.fromEither(contributionPlan.billingPeriod.toEither) | ||
recurringPeriod <- SimpleEitherT.fromEither(billingPeriod match { | ||
case period: RecurringPeriod => Right(period) | ||
case period: BillingPeriod.OneOffPeriod => Left(s"period $period was not recurring for contribution update") | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asInstanceOf
should not have been used as it always compiles even if the types are incompatible
I have updated it to use a pattern match which is fully type checked
currencyGlyph = currency.glyph | ||
oldPrice = contributionPlan.charges.price.prices.head.amount | ||
oldPrice = contributionPlan.totalChargesMinorUnit.toDouble / 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contribution only has one charge so the head
price is the same as the total charges
2eee502
to
bf96b89
Compare
@@ -90,11 +87,12 @@ class AccountController( | |||
Left(badRequest("Malformed request. Expected a valid reason for cancellation.")) | |||
} | |||
|
|||
def cancelSubscription(subscriptionName: memsub.Subscription.Name): Action[AnyContent] = | |||
def cancelSubscription(subscriptionNameString: String): Action[AnyContent] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inlined this because there was a method further down the file that just wrapped the string and then called this method
contact <- OptionT(EitherT(tp.contactRepository.get(identityId))) | ||
subs <- OptionT(EitherT(tp.subscriptionService.recentlyCancelled(contact)).map(Option(_))) | ||
} yield { | ||
Ok(Json.toJson(subs.map(CancelledSubscription(_, catalog)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only change here is passing in the catalog
result <- futureEitherListExistingPaymentOption.map { | ||
case Right(existingPaymentOptions) => | ||
logger.info(s"Successfully retrieved eligible existing payment options for identity user: ${maybeUserId.mkString}") | ||
Ok(Json.toJson(consolidatePaymentMethod(existingPaymentOptions.toList).map(_.toJson(catalog)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the change is to pass in the catalog here
def maybePaperDaysOfWeek(plan: RatePlan) = { | ||
val dayNames = for { | ||
charge <- plan.ratePlanCharges.list.toList | ||
.filterNot(_.pricing.isFree) // note 'Echo Legacy' rate plan has all days of week but some are zero price, this filters those out | ||
catalogZuoraPlan <- catalog.catalogMap.get(plan.productRatePlanId) | ||
dayName <- catalogZuoraPlan.productRatePlanCharges | ||
.get(charge.productRatePlanChargeId) | ||
.collect { case benefit: PaperDay => DayOfWeek.of(benefit.dayOfTheWeekIndex).getDisplayName(TextStyle.FULL, Locale.ENGLISH) } | ||
} yield dayName | ||
|
||
if (dayNames.nonEmpty) Json.obj("daysOfWeek" -> dayNames) else Json.obj() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is extracted and rewritten from the code from old line 117 below, that relied on pattern matching the high level PaperCharges.
private def billingPeriodFromInterval(interval: String): ZBillingPeriod = interval match { | ||
case "year" => ZYear | ||
case _ => ZMonth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we used to convert the low level stripe API data into our high level model, however since the high level model is removed, we need to produce the low level model instead.
(This also means adding a fake Guardian Patron to the catalog, so that it can find everything on the fly.)
@@ -85,29 +82,29 @@ class GuardianPatronService( | |||
casActivationDate = None, | |||
promoCode = None, | |||
isCancelled = subscription.isCancelled, | |||
plans = CovariantNonEmptyList( | |||
ratePlans = List( | |||
RatePlan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that RatePlan
(and RatePlanCharge
below) are totally different from before.
The old ones were the high level ones and have been deleted.
The new ones are the old low level ones and have been renamed.
bf96b89
to
780ffc6
Compare
# Conflicts: # membership-attribute-service/app/models/AccountDetails.scala # membership-attribute-service/app/services/zuora/payment/PaymentService.scala # membership-common/src/main/scala/com/gu/config/SubsV2ProductIds.scala # membership-common/src/main/scala/com/gu/memsub/subsv2/reads/ChargeListReads.scala
@@ -321,9 +335,6 @@ class AccountController( | |||
} yield validAmount | |||
} | |||
|
|||
def cancelSpecificSub(subscriptionName: String): Action[AnyContent] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these last few wrapper methods aren't needed any more as the called method now takes a string directly
case period: RecurringPeriod => Right(period) | ||
case period: BillingPeriod.OneOffPeriod => Left(s"period $period was not recurring for contribution update") | ||
}) | ||
applyFromDate = contributionPlan.chargedThroughDate.getOrElse(contributionPlan.start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was called chargedThroughDate
in the low level model (and chargedThrough
in the high level model),
The low level model matches what it's called in zuora, so I've gone with that.
.collect { case benefit: PaperDay => DayOfWeek.of(benefit.dayOfTheWeekIndex).getDisplayName(TextStyle.FULL, Locale.ENGLISH) } | ||
} yield dayName | ||
|
||
if (dayNames.nonEmpty) Json.obj("daysOfWeek" -> dayNames) else Json.obj() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of pattern matching the result of the logic that was in the ChargeListReads, we have to do it all in this function
Old Reads code was here:
members-data-api/membership-common/src/main/scala/com/gu/memsub/subsv2/reads/ChargeListReads.scala
Line 172 in 1433e4f
.ensure("No days found".wrapNel)(_.nonEmpty) |
This reverts commit 09d19fd.
@@ -86,10 +87,9 @@ object SupporterRatePlanToAttributesMapper { | |||
attributes.copy( | |||
GuardianPatronExpiryDate = Some(supporterRatePlanItem.termEndDate), | |||
) | |||
val guardianPatronProductRatePlanId = "guardian_patron" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to membership-common/src/main/scala/com/gu/memsub/subsv2/Catalog.scala in this PR
strangely, in this file, type ProductRatePlanId = String
, however elsewhere ProductRatePlanId is a value class.
I think IDs should generally be value classes so we get type checking, but this is not the PR to start changing that.
@@ -21,15 +21,15 @@ object Emails { | |||
"first_name" -> contact.firstName.getOrElse(""), | |||
"last_name" -> contact.lastName, | |||
"payment_method" -> paymentMethod.valueForEmail, | |||
"product_type" -> plan.productType, | |||
"product_type" -> productType.productTypeString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
productType is now a value class rather than a string so it needs unpacking when it's serialised
.toList | ||
.reduce((f1, f2) => | ||
PricingSummary( | ||
f1.underlying.keySet.intersect(f2.underlying.keySet).map(c => c -> Price(f1.underlying(c).amount + f2.underlying(c).amount, c)).toMap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic is from PricingSemigroup which is now deleted above - membership-common/src/main/scala/com/gu/memsub/PricingSummary.scala
c9f3f70
to
e7b7220
Compare
currency = contributionPlan.chargesPrice.prices.head.currency | ||
currencyGlyph = currency.glyph | ||
oldPrice = contributionPlan.charges.price.prices.head.amount | ||
oldPrice = contributionPlan.chargesPrice.prices.head.amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chargesPrice has the logic that was previously in the high level deserialiser (that returns the total pricing summary for all charges in the plan)
The reason it's not kept the old name is that charges is now called ratePlanCharges (so it would be ratePlanCharges.prices
) and is now a list rather than a high level ChargeList so we couldn't add a dummy prices method without using extension methods.
"price" -> plan.chargesPrice.prices.head.amount * 100, | ||
"currency" -> plan.chargesPrice.prices.head.currency.glyph, | ||
"currencyISO" -> plan.chargesPrice.prices.head.currency.iso, | ||
"billingPeriod" -> plan.billingPeriod.leftMap(e => throw new RuntimeException("no billing period: " + e)).toOption.get.noun, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would have failed to deserialise in the first place if the billing period wasn't right, now we fail when we actually try to use it (we could just leave it blank in the response alternatively)
|
||
val sortedPlans = subscription.plans.list.sortBy(_.start.toDate) | ||
"chargedThrough" -> plan.chargedThroughDate, | ||
"price" -> (plan.chargesPrice.prices.head.amount * 100).toInt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason we got a .0005p on the sixday paper which we didn't before even though the code was the same, I suspect it was adding them in a different order, and maybe addition on a float is not associative?
@@ -130,7 +134,7 @@ class AccountDetailsFromZuora( | |||
.getPaymentDetails(contactAndSubscription) | |||
.map(Right(_)) | |||
.recover { case x => | |||
Left(s"error retrieving payment details for subscription: freeOrPaidSub.name. Reason: $x") | |||
Left(s"error retrieving payment details for subscription: ${contactAndSubscription.subscription.name}. Reason: $x") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the $ sign was missed off originally
@@ -49,7 +49,7 @@ class AccountControllerAcceptanceTest extends AcceptanceTest { | |||
contactRepositoryMock = mock[ContactRepository] | |||
subscriptionServiceMock = mock[SubscriptionService[Future]] | |||
zuoraRestServiceMock = mock[ZuoraRestService] | |||
catalogServiceMock = TestCatalog() | |||
catalogServiceMock = catalog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now we use the TestCatalog from membership-common tests instead of the blank dummy one, as the code needs to access the catalog in the business logic in order to get some of the information
@@ -149,7 +164,7 @@ class AccountControllerAcceptanceTest extends AcceptanceTest { | |||
val giftSubscriptionFromSubscriptionService = TestSubscription( | |||
id = Subscription.Id(giftSubscription.Id), | |||
name = Subscription.Name(giftSubscription.Name), | |||
plans = CovariantNonEmptyList(TestPaidSubscriptionPlan(product = Product.SupporterPlus), Nil), | |||
plans = List(TestPaidSubscriptionPlan(productRatePlanId = TestCatalog.digipackPrpId)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test was a bit confusing, it was testing
- a gift supporter plus, a contribution, and a patron.
Since you can only have a gift digi sub (or GW) I changed it to test a more realistic situation
- gift digisub, a supporter plus and a patron.
} | ||
|
||
/** Low level model of a rate plan, as it appears on a subscription in Zuora | ||
*/ | ||
case class SubscriptionZuoraPlan( | ||
case class RatePlan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although this is still the low level model, now it has a load of helper methods on it to mimic the fields that were on the old high level model. The new methods take the catalog as a parameter in most cases.
@@ -28,17 +22,16 @@ case class Subscription( | |||
casActivationDate: Option[DateTime], | |||
promoCode: Option[PromoCode], | |||
isCancelled: Boolean, | |||
plans: CovariantNonEmptyList[RatePlan], | |||
ratePlans: List[RatePlan], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although using a non empty list means we can get nice errors with context from the deserialiser, it probably scares people too much. What we lose is compile time certainty that it's correct (we'd have to do .head or pattern match and throw)
override def reads(json: JsValue): JsResult[Status] = json match { | ||
case JsString("Expired") => JsSuccess(Status.Legacy) | ||
case JsString("Active") => JsSuccess(Status.Current) | ||
case JsString("NotStarted") => JsSuccess(Status.Upcoming) | ||
case a => JsError(s"Unknown status $a") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we no longer do this checking - before, the implicit deserialisers would discard any plans that were not active in the catalog. However since MDAPI is only working on existing subscriptions, we would only be looking things up rather than searching for one to create.
import scalaz.NonEmptyList | ||
import utils.Resource | ||
|
||
class SubReadsTest extends Specification { | ||
|
||
"Subscription JSON reads" should { | ||
|
||
"Discard discount rate plans when reading JSON" in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only discards them because the Pricing Summary parser
members-data-api/membership-common/src/main/scala/com/gu/memsub/PriceParser.scala
Line 12 in c2233c4
currency <- Currency.fromString(code) |
can't parse discounts
"pricingSummary": "50% discount", |
so the whole rateplan is discarded.
If it's a discount amount instead, it wil be retained. This test will have to change, probably in a later PR.
reasonForChange = | ||
s"User updated contribution via self-service MMA. Amount changed from $currencyGlyph$oldPrice to $currencyGlyph$newPrice effective from $applyFromDate" | ||
result <- SimpleEitherT( | ||
services.zuoraRestService.updateChargeAmount( | ||
subscription.name, | ||
contributionPlan.charges.subRatePlanChargeId, | ||
contributionPlan.ratePlanCharges.head.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know that ratePlanCharges has a head value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes good point, in reality there is always (exactly) one charge in the rateplan which would be the contribution.
If a broken contribution subscription came in then they would not even be able to see it in manage, let alone update the charge amount.
In the old system it would fail to deserialise the broken contribution, now it would only notice the issue when it comes to use it at this point.
It does raise a good point (along with the fact that your paper subscription found an issue). Before if there was an issue with the data it was discovered by the deserialiser and the subscription was usually silently discarded. Now if there is an issue, it will trip up when it tries to use it. I wonder how many broken subscriptions (or incomplete traits etc) we have that slip under the radar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that is a lot of changes. It looks reasonable to me as long as the discount issue is now fixed. Well done for sticking with this!
case class PaperCharges(dayPrices: Map[PaperDay, PricingSummary], digipack: Option[PricingSummary]) extends RatePlanChargeList { | ||
def benefits = NonEmptyList.fromSeq[Benefit](dayPrices.keys.head, dayPrices.keys.tail.toSeq ++ digipack.map(_ => Digipack)) | ||
def price: PricingSummary = (dayPrices.values.toSeq ++ digipack.toSeq).reduce(_ |+| _) | ||
override def billingPeriod: BillingPeriod = BillingPeriod.Month |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually not all papers are monthly, I found some migrated in Echo Legacy that are still running and are actually quarterly
billingPeriods match { | ||
case Nil => Validation.f[BillingPeriod]("No billing period found") | ||
case b :: Nil => b | ||
case _ => Validation.f[BillingPeriod]("Too many billing periods found") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked bigquery and there were only 5 active subscriptions with rateplans containing mixed billing periods, all were quarterly migrated in echo legacy subscriptions (the monthly charges were all zero)
before it would have been incorrectly hard coded as monthly, now it will end up as unknown_billing_period.
I did consider filtering the free charges out, but I found there are quite a lot of free charges in the system, mainly recurring contributions that have been set to zero, but a few digi sub and others.
val endDate: LocalDate = sortedPlans.headOption.map(_.end).getOrElse(paymentDetails.termEndDate) | ||
|
||
if (currentPlans.length > 1) logger.warn(s"More than one 'current plan' on sub with id: ${subscription.id}") | ||
val subscriptionData = new FilterPlans(subscription, catalog) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted for testability
case Product.Membership => true | ||
case Product.GuardianPatron => true | ||
case Product.Contribution => true | ||
case Product.Discounts => false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discounts used to be filtered out in the high level reads, now they must be removed before we write the plans out to the client
Seen on PROD (merged by @johnduffell 6 seconds ago) Please check your changes! Sentry Release: members-data-api |
this has been tested in PROD for a couple of 1 hour periods 24/25th July, plus since yesterday morning 29th July, with no new issues seen via fastly logs, searching the cloudwatch logs, and checking sentry. |
This PR removes the "high level" subscription model from members-data-api.
The purpose of the high level model was to combine the subscription, catalog, and product mappings into one thing. This would be guaranteed valid as a real guardian subscription and have all the info we need in a data structure.
After this PR everything is just blindly read in from zuora as a low level model, and it's up to individual bits of code to pick out what they need in conjunction with the catalog and product ids.
This new way does mean that errors that would be handled quietly in the high level reads (by discarding the plan) would then be surfaced during the business logic. If the code is looking at the wrong plan for whatever reason, this error may not be recoverable at that point. I have tried to deal with those situations on a case by case basis, but there will be a lot of holiday credits etc floating around in the business logic that weren't before.
There is probably more work to be done, especially to rename the individual fields to be closer to how they come back from zuora. Also e.g. we seem to have two fields
ProductType__c
in zuora catalog, one on the product and one on the charge, and used separately. But all this will be easier once it's all one set of objects.trello https://trello.com/c/Y7MSKlAj/243-add-to-mdapi-useful-values-for-customers-in-a-free-period
Tests all compiling and running locally fine with extensive changes made.
(Possible) Future work
PricingSummary
is only used to read from the subscription not the catalog now, so we could make it only handle a single currency which would simplify things a bit.