Merge pull request #1078 from guardian/jd-simplify-catalog-code

simplify the catalog related code

membership-attribute-service/app/components/TouchpointBackends.scala

@@ -1,5 +1,6 @@
 package components
 
+import com.gu.memsub.subsv2.services.SubscriptionService.CatalogMap
 import com.gu.memsub.subsv2.services.{CatalogService, SubscriptionService}
 import com.gu.zuora.ZuoraSoapService
 import com.typesafe.config.Config
@@ -21,7 +22,7 @@ class TouchpointBackends(
     contactRepositoryOverride: Option[ContactRepository] = None,
     subscriptionServiceOverride: Option[SubscriptionService[Future]] = None,
     zuoraRestServiceOverride: Option[ZuoraRestService] = None,
-    catalogServiceOverride: Option[CatalogService[Future]] = None,
+    catalogServiceOverride: Option[Future[CatalogMap]] = None,
     zuoraServiceOverride: Option[ZuoraSoapService with HealthCheckableService] = None,
     patronsStripeServiceOverride: Option[BasicStripeService] = None,
     chooseStripeOverride: Option[ChooseStripe] = None,

membership-attribute-service/app/components/TouchpointComponents.scala

@@ -5,15 +5,14 @@ import com.gu.config
 import com.gu.identity.IdapiService
 import com.gu.identity.auth._
 import com.gu.identity.play.IdentityPlayAuthService
-import com.gu.memsub.subsv2.Catalog
 import com.gu.memsub.subsv2.services.SubscriptionService.CatalogMap
 import com.gu.memsub.subsv2.services.{CatalogService, FetchCatalog, SubscriptionService}
 import com.gu.monitoring.SafeLogger.LogPrefix
 import com.gu.monitoring.{SafeLogging, ZuoraMetrics}
 import com.gu.okhttp.RequestRunners
 import com.gu.touchpoint.TouchpointBackendConfig
 import com.gu.zuora.rest.SimpleClient
-import com.gu.zuora.soap.ClientWithFeatureSupplier
+import com.gu.zuora.soap.Client
 import com.gu.zuora.{ZuoraSoapService, rest}
 import com.typesafe.config.Config
 import configuration.Stage
@@ -38,7 +37,7 @@ import software.amazon.awssdk.services.dynamodb.{DynamoDbAsyncClient, DynamoDbAs
 
 import java.util.concurrent.TimeUnit.SECONDS
 import scala.concurrent.duration._
-import scala.concurrent.{Await, ExecutionContext, Future}
+import scala.concurrent.{ExecutionContext, Future}
 
 class TouchpointComponents(
     stage: Stage,
@@ -48,7 +47,7 @@ class TouchpointComponents(
     contactRepositoryOverride: Option[ContactRepository] = None,
     subscriptionServiceOverride: Option[SubscriptionService[Future]] = None,
     zuoraRestServiceOverride: Option[ZuoraRestService] = None,
-    catalogServiceOverride: Option[CatalogService[Future]] = None,
+    catalogServiceOverride: Option[Future[CatalogMap]] = None,
     zuoraServiceOverride: Option[ZuoraSoapService with HealthCheckableService] = None,
     patronsStripeServiceOverride: Option[BasicStripeService] = None,
     chooseStripeOverride: Option[ChooseStripe] = None,
@@ -102,10 +101,9 @@ class TouchpointComponents(
 
   lazy val zuoraSoapService = {
     lazy val zuoraSoapClient =
-      new ClientWithFeatureSupplier(
+      new Client(
         apiConfig = backendConfig.zuoraSoap,
         httpClient = RequestRunners.configurableFutureRunner(timeout = Duration(30, SECONDS)),
-        extendedHttpClient = RequestRunners.futureRunner,
         metrics = zuoraMetrics,
       )
 
@@ -124,28 +122,21 @@ class TouchpointComponents(
   }
 
   lazy val catalogRestClient = rest.SimpleClient[Future](backendConfig.zuoraRest, RequestRunners.configurableFutureRunner(60.seconds))
-  lazy val catalogService = catalogServiceOverride.getOrElse(
-    new CatalogService(
-      productIds,
-      FetchCatalog.fromZuoraApi(catalogRestClient)(implicitly, LogPrefix.noLogPrefix),
-      Await.result(_: Future[Catalog], 60.seconds),
-      stage.value,
-    ),
+  lazy val futureCatalog: Future[CatalogMap] = catalogServiceOverride.getOrElse(
+    CatalogService
+      .read(FetchCatalog.fromZuoraApi(catalogRestClient)(implicitly, LogPrefix.noLogPrefix))
+      .recover { case error =>
+        logger.errorNoPrefix(scrub"Failed to load the product catalog from Zuora due to: $error")
+        throw error
+      },
   )
 
-  private lazy val futureCatalog: Future[CatalogMap] = catalogService.catalog
-    .map(_.fold[CatalogMap](error => { logger.errorNoPrefix(scrub"error: ${error.list.toList.mkString}"); Map() }, _.map))
-    .recover { case error =>
-      logger.errorNoPrefix(scrub"Failed to load the product catalog from Zuora due to: $error")
-      throw error
-    }
-
   lazy val subscriptionService: SubscriptionService[Future] = {
     lazy val zuoraSubscriptionService = new SubscriptionService(productIds, futureCatalog, zuoraRestClient, zuoraSoapService)
 
     subscriptionServiceOverride.getOrElse(zuoraSubscriptionService)
   }
-  lazy val paymentService: PaymentService = new PaymentService(zuoraSoapService, catalogService.unsafeCatalog.productMap)
+  lazy val paymentService: PaymentService = new PaymentService(zuoraSoapService)
 
   lazy val idapiService = new IdapiService(backendConfig.idapi, RequestRunners.futureRunner)
   lazy val tokenVerifierConfig = OktaTokenValidationConfig(

membership-attribute-service/app/services/zuora/payment/ZuoraPaymentService.scala → membership-attribute-service/app/services/zuora/payment/PaymentService.scala

@@ -3,7 +3,7 @@ package services.zuora.payment
 import com.gu.memsub.BillingSchedule.Bill
 import com.gu.memsub.Subscription._
 import com.gu.memsub.promo.LogImplicit._
-import com.gu.memsub.subsv2.{Subscription, RatePlan}
+import com.gu.memsub.subsv2.Subscription
 import com.gu.memsub.{BillingSchedule, Subscription => _, _}
 import com.gu.monitoring.SafeLogger.LogPrefix
 import com.gu.monitoring.SafeLogging
@@ -20,8 +20,7 @@ import scalaz.syntax.std.option._
 import scala.concurrent.{ExecutionContext, Future}
 import scala.util.Try
 
-class PaymentService(zuoraService: ZuoraSoapService, planMap: Map[ProductRatePlanChargeId, Benefit])(implicit ec: ExecutionContext)
-    extends SafeLogging {
+class PaymentService(zuoraService: ZuoraSoapService)(implicit ec: ExecutionContext) extends SafeLogging {
 
   def paymentDetails(
       sub: Subscription,
@@ -84,17 +83,17 @@ class PaymentService(zuoraService: ZuoraSoapService, planMap: Map[ProductRatePla
     }
 
   private def getNextBill(subId: Id, account: Account, numberOfBills: Int)(implicit logPrefix: LogPrefix): Future[Option[Bill]] =
-    zuoraService.previewInvoices(subId, numberOfBills).map { previewInvoiceItems =>
-      val maybeBillingSchedule = BillingSchedule.fromPreviewInvoiceItems(planMap, previewInvoiceItems)
-      maybeBillingSchedule.flatMap { billingSched =>
-        billingSched
-          .withCreditBalanceApplied(account.creditBalance)
-          .invoices
-          .list
-          .find(_.amount > 0)
-          .toOption
-      }
-    }
+    for {
+      previewInvoiceItems <- zuoraService.previewInvoices(subId, numberOfBills)
+    } yield for {
+      billingSched <- BillingSchedule.fromPreviewInvoiceItems(previewInvoiceItems)
+      bill <- billingSched
+        .withCreditBalanceApplied(account.creditBalance)
+        .invoices
+        .list
+        .find(_.amount > 0)
+        .toOption
+    } yield bill
 
   def getPaymentMethod(maybePaymentMethodId: Option[String], defaultMandateIdIfApplicable: Option[String] = None)(implicit
       logPrefix: LogPrefix,

membership-attribute-service/app/wiring/AppLoader.scala

@@ -2,6 +2,7 @@ package wiring
 
 import actions.CommonActions
 import ch.qos.logback.classic.LoggerContext
+import com.gu.memsub.subsv2.services.SubscriptionService.CatalogMap
 import com.gu.memsub.subsv2.services.{CatalogService, SubscriptionService}
 import com.gu.monitoring.SafeLoggerImpl
 import com.gu.zuora.ZuoraSoapService
@@ -63,7 +64,7 @@ class MyComponents(context: Context)
   lazy val contactRepositoryOverride: Option[ContactRepository] = None
   lazy val subscriptionServiceOverride: Option[SubscriptionService[Future]] = None
   lazy val zuoraRestServiceOverride: Option[ZuoraRestService] = None
-  lazy val catalogServiceOverride: Option[CatalogService[Future]] = None
+  lazy val catalogServiceOverride: Option[Future[CatalogMap]] = None
   lazy val zuoraSoapServiceOverride: Option[ZuoraSoapService with HealthCheckableService] = None
   lazy val patronsStripeServiceOverride: Option[BasicStripeService] = None
   lazy val chooseStripeOverride: Option[ChooseStripe] = None

membership-attribute-service/test/acceptance/AccountControllerAcceptanceTest.scala

@@ -1,12 +1,13 @@
 package acceptance
 
-import acceptance.data.stripe.{TestCustomersPaymentMethods, TestDynamoSupporterRatePlanItem, TestStripeSubscription}
 import acceptance.data._
+import acceptance.data.stripe.{TestCustomersPaymentMethods, TestDynamoSupporterRatePlanItem, TestStripeSubscription}
 import com.gu.i18n.Currency
 import com.gu.memsub.Product.Contribution
 import com.gu.memsub.Subscription.Name
-import com.gu.memsub.subsv2.services.{CatalogService, SubscriptionService}
-import com.gu.memsub.subsv2.{CovariantNonEmptyList, RatePlan}
+import com.gu.memsub.subsv2.CovariantNonEmptyList
+import com.gu.memsub.subsv2.services.SubscriptionService
+import com.gu.memsub.subsv2.services.SubscriptionService.CatalogMap
 import com.gu.memsub.{Product, Subscription}
 import com.gu.monitoring.SafeLogger.LogPrefix
 import com.gu.zuora.ZuoraSoapService
@@ -37,7 +38,7 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
   var contactRepositoryMock: ContactRepository = _
   var subscriptionServiceMock: SubscriptionService[Future] = _
   var zuoraRestServiceMock: ZuoraRestService = _
-  var catalogServiceMock: CatalogService[Future] = _
+  var catalogServiceMock: CatalogMap = _
   var zuoraSoapServiceMock: ZuoraSoapService with HealthCheckableService = _
   var supporterProductDataServiceMock: SupporterProductDataService = _
   var databaseServiceMock: ContributionsStoreDatabaseService = _
@@ -48,7 +49,7 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
     contactRepositoryMock = mock[ContactRepository]
     subscriptionServiceMock = mock[SubscriptionService[Future]]
     zuoraRestServiceMock = mock[ZuoraRestService]
-    catalogServiceMock = mock[CatalogService[Future]]
+    catalogServiceMock = TestCatalog()
     zuoraSoapServiceMock = mock[ZuoraSoapService with HealthCheckableService]
     supporterProductDataServiceMock = mock[SupporterProductDataService]
     databaseServiceMock = mock[ContributionsStoreDatabaseService]
@@ -63,7 +64,7 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
       override lazy val contactRepositoryOverride = Some(contactRepositoryMock)
       override lazy val subscriptionServiceOverride = Some(subscriptionServiceMock)
       override lazy val zuoraRestServiceOverride = Some(zuoraRestServiceMock)
-      override lazy val catalogServiceOverride = Some(catalogServiceMock)
+      override lazy val catalogServiceOverride = Some(Future.successful(catalogServiceMock))
       override lazy val zuoraSoapServiceOverride = Some(zuoraSoapServiceMock)
       override lazy val dbService = databaseServiceMock
       override lazy val patronsStripeServiceOverride = Some(patronsStripeServiceMock)
@@ -156,8 +157,6 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
         Some(giftSubscriptionFromSubscriptionService),
       )
 
-      catalogServiceMock.unsafeCatalog returns TestCatalog()
-
       zuoraRestServiceMock.getAccount(giftSubscriptionAccountId)(any) returns Future(\/.right(TestAccountSummary(id = giftSubscriptionAccountId)))
       zuoraRestServiceMock.getAccount(nonGiftSubscriptionAccountId)(any) returns Future(
         \/.right(TestAccountSummary(id = nonGiftSubscriptionAccountId)),
@@ -202,7 +201,6 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
       subscriptionServiceMock.current(contact)(any) was called
       zuoraRestServiceMock.getGiftSubscriptionRecordsFromIdentityId("200067388")(any) was called
       subscriptionServiceMock.get(Subscription.Name(giftSubscription.Name), isActiveToday = false)(any) was called
-      catalogServiceMock.unsafeCatalog was called
 
       zuoraRestServiceMock.getAccount(giftSubscriptionAccountId)(any) was called
       zuoraRestServiceMock.getAccount(nonGiftSubscriptionAccountId)(any) was called
@@ -218,7 +216,6 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
       contactRepositoryMock wasNever calledAgain
       subscriptionServiceMock wasNever calledAgain
       zuoraRestServiceMock wasNever calledAgain
-      catalogServiceMock wasNever calledAgain
       zuoraSoapServiceMock wasNever calledAgain
       databaseServiceMock wasNever called
 
@@ -366,7 +363,6 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
       contactRepositoryMock wasNever calledAgain
       subscriptionServiceMock wasNever calledAgain
       zuoraRestServiceMock wasNever calledAgain
-      catalogServiceMock wasNever called
       zuoraSoapServiceMock wasNever called
       databaseServiceMock wasNever called
       sendEmailMock wasNever calledAgain
@@ -479,7 +475,6 @@ class AccountControllerAcceptanceTest extends AcceptanceTest {
       contactRepositoryMock wasNever calledAgain
       subscriptionServiceMock wasNever calledAgain
       zuoraRestServiceMock wasNever calledAgain
-      catalogServiceMock wasNever called
       zuoraSoapServiceMock wasNever called
       databaseServiceMock wasNever called
       sendEmailMock wasNever calledAgain

membership-attribute-service/test/acceptance/PaymentUpdateControllerAcceptanceTest.scala

@@ -5,6 +5,7 @@ import acceptance.data._
 import acceptance.data.stripe.{TestStripeCard, TestStripeCustomer}
 import com.gu.i18n.Country
 import com.gu.memsub.Subscription
+import com.gu.memsub.subsv2.services.SubscriptionService.CatalogMap
 import com.gu.memsub.subsv2.services.{CatalogService, SubscriptionService}
 import com.gu.memsub.subsv2.{CovariantNonEmptyList, RatePlan}
 import com.gu.zuora.ZuoraSoapService
@@ -35,7 +36,7 @@ class PaymentUpdateControllerAcceptanceTest extends AcceptanceTest {
   var contactRepositoryMock: ContactRepository = _
   var subscriptionServiceMock: SubscriptionService[Future] = _
   var zuoraRestServiceMock: ZuoraRestService = _
-  var catalogServiceMock: CatalogService[Future] = _
+  var catalogServiceMock: CatalogMap = _
   var zuoraSoapServiceMock: ZuoraSoapService with HealthCheckableService = _
   var supporterProductDataServiceMock: SupporterProductDataService = _
   var databaseServiceMock: ContributionsStoreDatabaseService = _
@@ -49,7 +50,7 @@ class PaymentUpdateControllerAcceptanceTest extends AcceptanceTest {
     contactRepositoryMock = mock[ContactRepository]
     subscriptionServiceMock = mock[SubscriptionService[Future]]
     zuoraRestServiceMock = mock[ZuoraRestService]
-    catalogServiceMock = mock[CatalogService[Future]]
+    catalogServiceMock = TestCatalog()
     zuoraSoapServiceMock = mock[ZuoraSoapService with HealthCheckableService]
     supporterProductDataServiceMock = mock[SupporterProductDataService]
     databaseServiceMock = mock[ContributionsStoreDatabaseService]
@@ -70,7 +71,7 @@ class PaymentUpdateControllerAcceptanceTest extends AcceptanceTest {
       override lazy val contactRepositoryOverride = Some(contactRepositoryMock)
       override lazy val subscriptionServiceOverride = Some(subscriptionServiceMock)
       override lazy val zuoraRestServiceOverride = Some(zuoraRestServiceMock)
-      override lazy val catalogServiceOverride = Some(catalogServiceMock)
+      override lazy val catalogServiceOverride = Some(Future.successful(catalogServiceMock))
       override lazy val zuoraSoapServiceOverride = Some(zuoraSoapServiceMock)
       override lazy val dbService = databaseServiceMock
       override lazy val patronsStripeServiceOverride = Some(patronsStripeServiceMock)
@@ -149,8 +150,6 @@ class PaymentUpdateControllerAcceptanceTest extends AcceptanceTest {
         ),
       )
 
-      catalogServiceMock.unsafeCatalog returns TestCatalog()
-
       contactRepositoryMock.get("200067388")(any) returns Future(\/.right(Some(contact)))
 
       val subscription = TestSubscription(
@@ -216,7 +215,6 @@ class PaymentUpdateControllerAcceptanceTest extends AcceptanceTest {
       identityMockClientAndServer.verify(identityRequest)
       subscriptionServiceMock.current(contact)(any) was called
       contactRepositoryMock.get("200067388")(any) was called
-      catalogServiceMock.unsafeCatalog was called
       zuoraSoapServiceMock.getAccount(subscription.accountId)(any) wasCalled twice
       zuoraSoapServiceMock.getContact(account.billToId)(any) was called
       zuoraSoapServiceMock.createPaymentMethod(createPaymentMethod)(any) was called
@@ -227,7 +225,6 @@ class PaymentUpdateControllerAcceptanceTest extends AcceptanceTest {
       contactRepositoryMock wasNever calledAgain
       subscriptionServiceMock wasNever calledAgain
       zuoraRestServiceMock wasNever calledAgain
-      catalogServiceMock wasNever calledAgain
       zuoraSoapServiceMock wasNever calledAgain
       databaseServiceMock wasNever called
       sendEmailMock wasNever calledAgain
@@ -376,7 +373,6 @@ class PaymentUpdateControllerAcceptanceTest extends AcceptanceTest {
       contactRepositoryMock wasNever calledAgain
       subscriptionServiceMock wasNever calledAgain
       zuoraRestServiceMock wasNever calledAgain
-      catalogServiceMock wasNever calledAgain
       ukStripeServiceMock wasNever calledAgain
       zuoraSoapServiceMock wasNever calledAgain
       databaseServiceMock wasNever called