Skip to content
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 staff membership #1072

Merged
merged 2 commits into from
May 21, 2024
Merged

remove staff membership #1072

merged 2 commits into from
May 21, 2024

Conversation

johnduffell
Copy link
Member

This follows on from #1071
and removes staff as well as friend.

Although some people still have staff membership, it doesn't give any benefit and so we can ignore it in zuora and the catalog.

All staff should get free digital subscription which would prevent supporter asks and give ad free anyway.
Events discounts are done via other means now.

The next PR will clean up all the Paid/Free distinction, to keep things easier to review.
And there will also be a PR to remove the date based downgrade-to-friend logic. (Since friend/staff are modelled as one time charges, there was more complexity in working out if people were members.)

@@ -17,8 +17,7 @@ object FeastApp {
private def isBeforeFeastLaunch(dt: LocalDate): Boolean = dt.isBefore(FeastIosLaunchDate)

def shouldGetFeastAccess(attributes: Attributes): Boolean =
attributes.isStaffTier ||
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was misleading - staff who get digi access will get feast via attributes.digitalSubscriberHasActivePlan below (although in a convoluted way - this logic needs refactoring)

Comment on lines 339 to 341
case object Supporter extends MembershipTier("Supporter", 3)
case object Partner extends MembershipTier("Partner", 4)
case object Patron extends MembershipTier("Patron", 5)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't sure whether to renumber the ordering but I don't think It'll hurt to leave it

@@ -139,7 +138,6 @@ object Benefit {
(id == Weekly.id).option(Weekly)

sealed trait MemberTier extends Benefit
sealed trait FreeMemberTier extends MemberTier
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed a few of the Free... things, but I tried to avoid doing to much in this PR, the next PR will be imminent.

@@ -1,11 +0,0 @@
package com.gu.memsub
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused

implicit def readChargeList: ChargeListReads[ChargeList] = new ChargeListReads[ChargeList] {
override def read(cat: PlanChargeMap, charges: List[ZuoraCharge]) = {
readPaidChargeList.read(cat, charges) orElse2
readFreeChargeList.read(cat, charges)
readPaidChargeList.read(cat, charges).map(identity[ChargeList])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to map with identity because functions are (co)variant but a ChargeListRead is not. This means
ChargeListReads[PaidChargeList] is not a subclass of ChargeListReads[ChargeList]

Whether it should be or not was not something I wanted to delve in to, since this function will disappear in the next PR.

Comment on lines -135 to -148
ChargeListReads.readPaidCharge[B, BillingPeriod].read(c.benefits, z.charges.list.toList).map(\/.r[FreeCharge[B]].apply) orElse
ChargeListReads.readFreeCharge[B].read(c.benefits, z.charges.list.toList).map(\/.left)
).flatMap(
_.fold(
free =>
freePlanReads(implicitly[SubPlanReads[P]], ChargeListReads.pure(free))
.read(p, z, c)
.map(identity[SubscriptionPlan[P, ChargeList with SingleBenefit[B]]]),
paid =>
paidPlanReads(implicitly[SubPlanReads[P]], ChargeListReads.pure(paid))
.read(p, z, c)
.map(identity[SubscriptionPlan[P, ChargeList with SingleBenefit[B]]]),
),
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took me a minute to work this out - I think it might be using an Either in order to keep the correct types - so we don't need this. I will revisit in the next PR when I remove Free/Paid distinction.

@@ -135,17 +135,6 @@ class ChargeListReadsTest extends AnyFlatSpec {

}

"product reads" should "not read any supporter as a Free member" in {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no such thing as a free member now. We have tests for other kinds of incorrect charge lists below so I deleted this one.

Comment on lines +34 to +36
val digipackAnnualPrpId = ProductRatePlanId("2c92c0f94bbffaaa014bc6a4212e205b")
val partnerPrpId = ProductRatePlanId("2c92c0f84c510081014c569327003593")
val supporterPrpId = ProductRatePlanId("2c92c0f84bbfeca5014bc0c5a793241d")
val supporterPlusPrpId = ProductRatePlanId("8ad08cbd8586721c01858804e3275376")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to update these tests to use S+ and digi sub, rather than friend and supporter.

Comment on lines -138 to -139
case "/subscriptions/accounts/foo" => jsonResponse("rest/plans/accounts/Friend.json")(request)
case "/subscriptions/accounts/bar" => jsonResponse("rest/plans/accounts/Friend.json")(request)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why it was reading the same subscription twice?

@@ -341,13 +314,6 @@ class SubscriptionServiceTest extends Specification {
result mustEqual \/-("idPartner")
}

"tell you you aren't a staff membership if your subscription is cancelled regardless of the date" in {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no such thing as instant change of a one time charge subscription any more. we only modelled friend and staff that way.

@@ -412,14 +359,14 @@ class SubscriptionServiceTest extends Specification {

"Leverage the soap client to fetch subs by contact ID" in {
val subs = service.current[SubscriptionPlan.AnyPlan](contact)
subs.headOption.map(_.name) mustEqual Some(memsub.Subscription.Name("subscriptionNumber")) // what is in the config file
subs.map(_.name.get).sorted mustEqual List("A-S00890520", "A-S00890521") // from the test resources jsons
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what the point of testing read the same sub twice, so I read both the annual digi and the monthly S+ and chec they both appear

Copy link
Member

@rupertbates rupertbates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@johnduffell johnduffell merged commit bb3a55c into main May 21, 2024
2 checks passed
@johnduffell johnduffell deleted the jd-remove-staff branch May 21, 2024 09:51
@prout-bot
Copy link

Seen on PROD (merged by @johnduffell 16 minutes and 22 seconds ago) Please check your changes!

Sentry Release: members-data-api

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants