APSR-1788 - Handle apps already being subscribed to an api when PPNS …

…is added to the api (#114) * APSR-1788 - Sort out mock module use to make test maintenance easier * APIS-1788 - WIP * APSR-1788 - Rework upsert and tests * APSR-1788 - Tidy tests * APSR-1788 - Tidy tests * APSR-1788: Do not update PPNS callback URL if it has not changed * APSR-1788 - Rework small section of code * APSR-1788 - PR Bot comments * APSR-1788: Fix some merge errors --------- Co-authored-by: John Green <[email protected]>
hmrc · Feb 27, 2024 · 7d681cd · 7d681cd
1 parent c1f9389
commit 7d681cd
Show file tree

Hide file tree

Showing 19 changed files with 561 additions and 246 deletions.
diff --git a/app/uk/gov/hmrc/apisubscriptionfields/connector/JsonFormatters.scala b/app/uk/gov/hmrc/apisubscriptionfields/connector/JsonFormatters.scala
@@ -17,12 +17,10 @@
 package uk.gov.hmrc.apisubscriptionfields.connector
 
 import play.api.libs.json._
-import uk.gov.hmrc.apiplatform.modules.common.domain.models._
 
 import uk.gov.hmrc.apisubscriptionfields.model.{BoxId, SubscriptionFieldsId}
 
 trait JsonFormatters {
-  implicit val clientIdJF: Format[ClientId]                         = Json.valueFormat[ClientId]
   implicit val boxIdJF: Format[BoxId]                               = Json.valueFormat[BoxId]
   implicit val subscriptionFieldsIdJF: Format[SubscriptionFieldsId] = Json.valueFormat[SubscriptionFieldsId]
 

diff --git a/app/uk/gov/hmrc/apisubscriptionfields/connector/PushPullNotificationServiceConnector.scala b/app/uk/gov/hmrc/apisubscriptionfields/connector/PushPullNotificationServiceConnector.scala
@@ -60,13 +60,15 @@ class PushPullNotificationServiceConnector @Inject() (http: HttpClient, appConfi
       }
   }
 
-  def updateCallBackUrl(clientId: ClientId, boxId: BoxId, callbackUrl: FieldValue)(implicit hc: HeaderCarrier): Future[PPNSCallBackUrlValidationResponse] = {
+  def updateCallBackUrl(clientId: ClientId, boxId: BoxId, callbackUrl: FieldValue)(implicit hc: HeaderCarrier): Future[Either[String, Unit]] = {
     val payload = UpdateCallBackUrlRequest(clientId, callbackUrl)
     http
       .PUT[UpdateCallBackUrlRequest, UpdateCallBackUrlResponse](s"$externalServiceUri/box/${boxId.value.toString}/callback", payload)
       .map(response =>
-        if (response.successful) PPNSCallBackUrlSuccessResponse
-        else response.errorMessage.fold(PPNSCallBackUrlFailedResponse("Unknown Error"))(PPNSCallBackUrlFailedResponse)
+        if (response.successful)
+          Right(())
+        else
+          Left(response.errorMessage.getOrElse("Unknown Error"))
       )
       .recover { case NonFatal(e) =>
         throw new RuntimeException(s"Unexpected response from $externalServiceUri: ${e.getMessage}")

diff --git a/app/uk/gov/hmrc/apisubscriptionfields/controller/SubscriptionFieldsController.scala b/app/uk/gov/hmrc/apisubscriptionfields/controller/SubscriptionFieldsController.scala
@@ -92,11 +92,9 @@ class SubscriptionFieldsController @Inject() (cc: ControllerComponents, service:
           .upsert(clientId, apiContext, apiVersionNbr, payload.fields)
           .map(_ match {
             case NotFoundSubsFieldsUpsertResponse                             => BadRequest(Json.toJson("reason" -> "field definitions not found")) // TODO
-            case FailedValidationSubsFieldsUpsertResponse(fieldErrorMessages) =>
-              BadRequest(Json.toJson(fieldErrorMessages))
+            case FailedValidationSubsFieldsUpsertResponse(fieldErrorMessages) => BadRequest(Json.toJson(fieldErrorMessages))
             case SuccessfulSubsFieldsUpsertResponse(response, true)           => Created(Json.toJson(response))
-            case SuccessfulSubsFieldsUpsertResponse(response, false)          =>
-              Ok(Json.toJson(response))
+            case SuccessfulSubsFieldsUpsertResponse(response, false)          => Ok(Json.toJson(response))
           })
           .recover(recovery)
       }

diff --git a/.../apisubscriptionfields/model/Topics.scala → ...c/apisubscriptionfields/model/BoxId.scala b/.../apisubscriptionfields/model/Topics.scala → ...c/apisubscriptionfields/model/BoxId.scala
diff --git a/app/uk/gov/hmrc/apisubscriptionfields/model/Responses.scala b/app/uk/gov/hmrc/apisubscriptionfields/model/Responses.scala
@@ -35,10 +35,6 @@ sealed trait SubsFieldValidationResponse
 case object ValidSubsFieldValidationResponse                                          extends SubsFieldValidationResponse
 case class InvalidSubsFieldValidationResponse(errorResponses: Map[FieldName, String]) extends SubsFieldValidationResponse
 
-sealed trait PPNSCallBackUrlValidationResponse
-case object PPNSCallBackUrlSuccessResponse                 extends PPNSCallBackUrlValidationResponse
-case class PPNSCallBackUrlFailedResponse(errorMsg: String) extends PPNSCallBackUrlValidationResponse
-
 sealed trait ErrorCode
 
 object ErrorCode {

diff --git a/app/uk/gov/hmrc/apisubscriptionfields/service/PushPullNotificationService.scala b/app/uk/gov/hmrc/apisubscriptionfields/service/PushPullNotificationService.scala
@@ -17,39 +17,28 @@
 package uk.gov.hmrc.apisubscriptionfields.service
 
 import javax.inject.{Inject, Singleton}
-import scala.concurrent.{ExecutionContext, Future}
+import scala.concurrent.Future
 
 import uk.gov.hmrc.apiplatform.modules.common.domain.models._
 import uk.gov.hmrc.http.HeaderCarrier
 
 import uk.gov.hmrc.apisubscriptionfields.connector.PushPullNotificationServiceConnector
-import uk.gov.hmrc.apisubscriptionfields.model.Types.FieldValue
-import uk.gov.hmrc.apisubscriptionfields.model.{FieldDefinition, _}
+import uk.gov.hmrc.apisubscriptionfields.model.BoxId
+import uk.gov.hmrc.apisubscriptionfields.model.Types.{FieldName, FieldValue}
 
 @Singleton
-class PushPullNotificationService @Inject() (ppnsConnector: PushPullNotificationServiceConnector)(implicit ec: ExecutionContext) {
+class PushPullNotificationService @Inject() (ppnsConnector: PushPullNotificationServiceConnector) {
 
-  def makeBoxName(apiContext: ApiContext, apiVersionNbr: ApiVersionNbr, fieldDefinition: FieldDefinition): String = {
+  def makeBoxName(apiContext: ApiContext, apiVersionNbr: ApiVersionNbr, fieldName: FieldName): String = {
     val separator = "##"
-    s"${apiContext.value}${separator}${apiVersionNbr.value}${separator}${fieldDefinition.name.value}"
+    s"${apiContext.value}${separator}${apiVersionNbr.value}${separator}${fieldName.value}"
   }
 
-  def subscribeToPPNS(
-      clientId: ClientId,
-      apiContext: ApiContext,
-      apiVersionNbr: ApiVersionNbr,
-      oFieldValue: Option[FieldValue],
-      fieldDefinition: FieldDefinition
-    )(implicit
-      hc: HeaderCarrier
-    ): Future[PPNSCallBackUrlValidationResponse] = {
-    for {
-      boxId  <- ppnsConnector.ensureBoxIsCreated(makeBoxName(apiContext, apiVersionNbr, fieldDefinition), clientId)
-      result <- oFieldValue match {
-                  case Some(value) => ppnsConnector.updateCallBackUrl(clientId, boxId, value)
-                  case None        => Future.successful(PPNSCallBackUrlSuccessResponse)
-                }
-    } yield result
+  def ensureBoxIsCreated(clientId: ClientId, apiContext: ApiContext, apiVersionNbr: ApiVersionNbr, fieldName: FieldName)(implicit hc: HeaderCarrier): Future[BoxId] = {
+    ppnsConnector.ensureBoxIsCreated(makeBoxName(apiContext, apiVersionNbr, fieldName), clientId)
   }
 
+  def updateCallbackUrl(clientId: ClientId, boxId: BoxId, fieldValue: FieldValue)(implicit hc: HeaderCarrier): Future[Either[String, Unit]] = {
+    ppnsConnector.updateCallBackUrl(clientId, boxId, fieldValue)
+  }
 }
diff --git a/app/uk/gov/hmrc/apisubscriptionfields/service/SubscriptionFieldsService.scala b/app/uk/gov/hmrc/apisubscriptionfields/service/SubscriptionFieldsService.scala
@@ -22,8 +22,10 @@ import scala.concurrent.{ExecutionContext, Future}
 
 import cats.data.NonEmptyList
 import cats.data.{NonEmptyList => NEL}
+import cats.implicits._
 
 import uk.gov.hmrc.apiplatform.modules.common.domain.models._
+import uk.gov.hmrc.apiplatform.modules.common.services.EitherTHelper
 import uk.gov.hmrc.http.HeaderCarrier
 
 import uk.gov.hmrc.apisubscriptionfields.model.Types._
@@ -32,89 +34,116 @@ import uk.gov.hmrc.apisubscriptionfields.repository.SubscriptionFieldsRepository
 
 @Singleton
 class SubscriptionFieldsService @Inject() (
-    repository: SubscriptionFieldsRepository,
+    subscriptionFieldsRepository: SubscriptionFieldsRepository,
     apiFieldDefinitionsService: ApiFieldDefinitionsService,
     pushPullNotificationService: PushPullNotificationService
   )(implicit ec: ExecutionContext
   ) {
 
-  private def validate(fields: Fields, fieldDefinitions: NonEmptyList[FieldDefinition]): SubsFieldValidationResponse = {
-    SubscriptionFieldsService.validateAgainstValidationRules(fieldDefinitions, fields) ++ SubscriptionFieldsService.validateFieldNamesAreDefined(fieldDefinitions, fields) match {
-      case FieldErrorMap.empty => ValidSubsFieldValidationResponse
-      case errs: FieldErrorMap => InvalidSubsFieldValidationResponse(errorResponses = errs)
+  def upsert(clientId: ClientId, apiContext: ApiContext, apiVersionNbr: ApiVersionNbr, newFields: Fields)(implicit hc: HeaderCarrier): Future[SubsFieldsUpsertResponse] = {
+    def findPpnsField(fieldDefinitions: NEL[FieldDefinition]): Option[FieldDefinition] = fieldDefinitions.find(_.`type` == FieldDefinitionType.PPNS_FIELD)
+
+    def handleAnyPpnsSubscriptionRequired(
+        clientId: ClientId,
+        apiContext: ApiContext,
+        apiVersionNbr: ApiVersionNbr,
+        fieldDefinitions: NEL[FieldDefinition],
+        existingFields: Option[SubscriptionFields]
+      )(implicit
+        hc: HeaderCarrier
+      ): Future[Either[String, Unit]] = {
+
+      findPpnsField(fieldDefinitions) match {
+        case Some(fieldDefinition) =>
+          val fieldName = fieldDefinition.name
+          newFields.get(fieldName) match {
+            case Some(newFieldValue) =>
+              for {
+                boxId                  <- pushPullNotificationService.ensureBoxIsCreated(clientId, apiContext, apiVersionNbr, fieldName)
+                fieldValueHasNotChanged = existingFields.map(_.fields.get(fieldName).contains(newFieldValue)).getOrElse(false)
+                result                 <- if (fieldValueHasNotChanged) successful(Right(()))
+                                          else pushPullNotificationService.updateCallbackUrl(clientId, boxId, newFieldValue)
+              } yield result
+            case None                => successful(Right(()))
+          }
+        case None                  => successful(Right(()))
+      }
     }
-  }
 
-  private def upsertSubscriptionFields(clientId: ClientId, apiContext: ApiContext, apiVersionNbr: ApiVersionNbr, fields: Fields): Future[SuccessfulSubsFieldsUpsertResponse] = {
-    repository
-      .saveAtomic(clientId, apiContext, apiVersionNbr, fields)
-      .map(result => SuccessfulSubsFieldsUpsertResponse(result._1, result._2))
-  }
+    def validateFields(fields: Fields, fieldDefinitions: NonEmptyList[FieldDefinition]): Either[FieldErrorMap, Unit] = {
+      SubscriptionFieldsService.validateAgainstValidationRules(fieldDefinitions, fields) ++ SubscriptionFieldsService.validateFieldNamesAreDefined(fieldDefinitions, fields) match {
+        case FieldErrorMap.empty => Right(())
+        case errs: FieldErrorMap => Left(errs)
+      }
+    }
+
+    def translateValidateError(fieldErrorMessages: FieldErrorMap) = FailedValidationSubsFieldsUpsertResponse(fieldErrorMessages)
 
-  def handlePPNS(
-      clientId: ClientId,
-      apiContext: ApiContext,
-      apiVersionNbr: ApiVersionNbr,
-      fieldDefinitions: NEL[FieldDefinition],
-      fields: Fields
-    )(implicit
-      hc: HeaderCarrier
-    ): Future[SubsFieldsUpsertResponse] = {
-    val ppnsFieldDefinition: Option[FieldDefinition] = fieldDefinitions.find(_.`type` == FieldDefinitionType.PPNS_FIELD)
-
-    ppnsFieldDefinition match {
-      case Some(fieldDefinition) =>
-        val oFieldValue: Option[FieldValue] = fields.get(fieldDefinition.name)
-        pushPullNotificationService.subscribeToPPNS(clientId, apiContext, apiVersionNbr, oFieldValue, fieldDefinition).flatMap {
-          case PPNSCallBackUrlSuccessResponse       => upsertSubscriptionFields(clientId, apiContext, apiVersionNbr, fields)
-          case PPNSCallBackUrlFailedResponse(error) => Future.successful(FailedValidationSubsFieldsUpsertResponse(Map(fieldDefinition.name -> error)))
-        }
-      case None                  => upsertSubscriptionFields(clientId, apiContext, apiVersionNbr, fields)
+    def translatePpnsError(fieldDefinitions: NEL[FieldDefinition])(error: String) = {
+      val fieldName          = findPpnsField(fieldDefinitions).get.name
+      val fieldErrorMessages = Map(fieldName -> error)
+      FailedValidationSubsFieldsUpsertResponse(fieldErrorMessages)
     }
 
-  }
+    def upsertIfFieldsHaveChanged(anyExistingFields: Option[SubscriptionFields]) = {
+      def upsertSubscriptionFields(clientId: ClientId, apiContext: ApiContext, apiVersionNbr: ApiVersionNbr, fields: Fields): Future[SuccessfulSubsFieldsUpsertResponse] = {
+        subscriptionFieldsRepository
+          .saveAtomic(clientId, apiContext, apiVersionNbr, fields)
+          .map(result => SuccessfulSubsFieldsUpsertResponse(result._1, result._2))
+      }
+
+      anyExistingFields match {
+        case Some(existingFields) if (existingFields.fields == newFields) =>
+          successful(SuccessfulSubsFieldsUpsertResponse(existingFields, false))
+        case _                                                            =>
+          upsertSubscriptionFields(clientId, apiContext, apiVersionNbr, newFields)
+      }
+    }
 
-  def upsert(clientId: ClientId, apiContext: ApiContext, apiVersionNbr: ApiVersionNbr, fields: Fields)(implicit hc: HeaderCarrier): Future[SubsFieldsUpsertResponse] = {
-    val foFieldDefinitions: Future[Option[NonEmptyList[FieldDefinition]]] =
-      apiFieldDefinitionsService.get(apiContext, apiVersionNbr).map(_.map(_.fieldDefinitions))
-
-    get(clientId, apiContext, apiVersionNbr).flatMap(_ match {
-      case Some(sfields) if (sfields.fields == fields) => Future.successful(SuccessfulSubsFieldsUpsertResponse(sfields, false))
-      case _                                           =>
-        foFieldDefinitions.flatMap(_ match {
-          case None                   => successful(NotFoundSubsFieldsUpsertResponse)
-          case Some(fieldDefinitions) =>
-            validate(fields, fieldDefinitions) match {
-              case ValidSubsFieldValidationResponse                       => handlePPNS(clientId, apiContext, apiVersionNbr, fieldDefinitions, fields)
-              case InvalidSubsFieldValidationResponse(fieldErrorMessages) => successful(FailedValidationSubsFieldsUpsertResponse(fieldErrorMessages))
-            }
-        })
-    })
+    (for {
+      anyFieldDefinitions <- apiFieldDefinitionsService.get(apiContext, apiVersionNbr).map(_.map(_.fieldDefinitions))
+      anyExistingFields   <- subscriptionFieldsRepository.fetch(clientId, apiContext, apiVersionNbr)
+    } yield (anyFieldDefinitions, anyExistingFields))
+      .flatMap(_ match {
+        case (None, Some(existingFields)) if (existingFields.fields == newFields) => successful(SuccessfulSubsFieldsUpsertResponse(existingFields, false))
+        case (None, _)                                                            => successful(NotFoundSubsFieldsUpsertResponse)
+        case (Some(fieldDefinitions), anyExistingFields)                          =>
+          val E = EitherTHelper.make[FailedValidationSubsFieldsUpsertResponse]
+          (
+            for {
+              _        <- E.fromEither(validateFields(newFields, fieldDefinitions).leftMap(translateValidateError))
+              _        <- E.fromEitherF(handleAnyPpnsSubscriptionRequired(clientId, apiContext, apiVersionNbr, fieldDefinitions, anyExistingFields)
+                            .map(_.leftMap(translatePpnsError(fieldDefinitions))))
+              response <- E.liftF(upsertIfFieldsHaveChanged(anyExistingFields))
+            } yield response
+          )
+            .merge
+      })
   }
 
   def delete(clientId: ClientId, apiContext: ApiContext, apiVersionNbr: ApiVersionNbr): Future[Boolean] = {
-    repository.delete(clientId, apiContext, apiVersionNbr)
+    subscriptionFieldsRepository.delete(clientId, apiContext, apiVersionNbr)
   }
 
   def delete(clientId: ClientId): Future[Boolean] = {
-    repository.delete(clientId)
+    subscriptionFieldsRepository.delete(clientId)
   }
 
   def get(clientId: ClientId, apiContext: ApiContext, apiVersionNbr: ApiVersionNbr): Future[Option[SubscriptionFields]] = {
     for {
-      fetch <- repository.fetch(clientId, apiContext, apiVersionNbr)
+      fetch <- subscriptionFieldsRepository.fetch(clientId, apiContext, apiVersionNbr)
     } yield fetch
   }
 
   def getBySubscriptionFieldId(subscriptionFieldsId: SubscriptionFieldsId): Future[Option[SubscriptionFields]] = {
     for {
-      fetch <- repository.fetchByFieldsId(subscriptionFieldsId)
+      fetch <- subscriptionFieldsRepository.fetchByFieldsId(subscriptionFieldsId)
     } yield fetch
   }
 
   def getByClientId(clientId: ClientId): Future[Option[BulkSubscriptionFieldsResponse]] = {
     (for {
-      fields <- repository.fetchByClientId(clientId)
+      fields <- subscriptionFieldsRepository.fetchByClientId(clientId)
     } yield fields)
       .map {
         case Nil => None
@@ -124,7 +153,7 @@ class SubscriptionFieldsService @Inject() (
 
   def getAll: Future[BulkSubscriptionFieldsResponse] = {
     (for {
-      fields <- repository.fetchAll
+      fields <- subscriptionFieldsRepository.fetchAll
     } yield fields)
       .map(BulkSubscriptionFieldsResponse(_))
   }

diff --git a/conf/application.conf b/conf/application.conf
@@ -75,6 +75,8 @@ controllers {
 # You can disable evolutions if needed
 # evolutionplugin=disabled
 
+# Microservice specific config
+
 mongo-async-driver {
   akka {
     loglevel = WARNING

diff --git a/conf/logback.xml b/conf/logback.xml
@@ -45,7 +45,7 @@
 
     <logger name="uk.gov" level="INFO"/>
 
-    <logger name="connector" level="TRACE">
+    <logger name="connector" level="INFO">
         <appender-ref ref="STDOUT"/>
     </logger>
 

diff --git a/test/uk/gov/hmrc/apisubscriptionfields/FieldDefinitionTestData.scala b/test/uk/gov/hmrc/apisubscriptionfields/FieldDefinitionTestData.scala
@@ -80,7 +80,7 @@ trait FieldDefinitionTestData extends TestData {
     FieldDefinition("password", "password", "this is your password", FieldDefinitionType.SECURE_TOKEN, "password", Some(FakeValidationForPassword))
 
   final val FakeFieldDefinitionPPNSFields     =
-    FieldDefinition("callbackurl", "callbackurl", "please enter a callback url", FieldDefinitionType.PPNS_FIELD, "callbackurl", Some(FakeValidationForPPNS))
+    FieldDefinition(PPNSFieldFieldName, "Callback URL", "please enter a callback url", FieldDefinitionType.PPNS_FIELD, "callback", Some(FakeValidationForPPNS))
   final val FakeApiFieldDefinitionssWithRegex = NonEmptyList.fromListUnsafe(List(FakeFieldDefinitionAlphnumericField, FakeFieldDefinitionPassword))
 
   final val FakeApiFieldDefinitionsPPNSWithRegex =

diff --git a/test/uk/gov/hmrc/apisubscriptionfields/SubscriptionFieldsTestData.scala b/test/uk/gov/hmrc/apisubscriptionfields/SubscriptionFieldsTestData.scala
@@ -35,8 +35,10 @@ trait SubscriptionFieldsTestData extends FieldDefinitionTestData with Validation
   final val SubscriptionFieldsMatchRegexValidation: Fields    = Map(AlphanumericFieldName -> "ABC123ab", PasswordFieldName -> "Qw12@ert")
   final val SubscriptionFieldsNonMatchRegexValidation: Fields = Map(AlphanumericFieldName -> "ABC123a", PasswordFieldName -> "Qw12@er")
 
+  final val PPNSFieldFieldValue: FieldValue = "https://www.mycallbackurl.com"
+
   final val SubscriptionFieldsMatchRegexValidationPPNS: Fields      =
-    Map(AlphanumericFieldName -> "ABC123abc", PasswordFieldName -> "Qw12@erty", PPNSFieldFieldName -> "https://www.mycallbackurl.com")
+    Map(AlphanumericFieldName -> "ABC123abc", PasswordFieldName -> "Qw12@erty", PPNSFieldFieldName -> PPNSFieldFieldValue)
   final val SubscriptionFieldsDoNotMatchRegexValidationPPNS: Fields = Map(AlphanumericFieldName -> "ABC123abc", PasswordFieldName -> "Qw12@erty", PPNSFieldFieldName -> "foo")
   final val SubscriptionFieldsEmptyValueRegexValidationPPNS: Fields = Map(AlphanumericFieldName -> "ABC123abc", PasswordFieldName -> "Qw12@erty", PPNSFieldFieldName -> "")
   final val SubscriptionFieldsDoNotMatchRegexValidation: Fields     = Map(AlphanumericFieldName -> "ABC123abc=", PasswordFieldName -> "Qw12erty")

diff --git a/test/uk/gov/hmrc/apisubscriptionfields/TestData.scala b/test/uk/gov/hmrc/apisubscriptionfields/TestData.scala
@@ -22,6 +22,8 @@ import play.api.http.HeaderNames.{ACCEPT, CONTENT_TYPE}
 import play.api.http.MimeTypes
 import uk.gov.hmrc.apiplatform.modules.common.domain.models._
 
+import uk.gov.hmrc.apisubscriptionfields.model.BoxId
+
 trait TestData {
 
   type EmulatedFailure = UnsupportedOperationException
@@ -42,6 +44,7 @@ trait TestData {
 
   final val FakeClientId2 = ClientId(fakeRawClientId2)
 
+  final val FakeBoxId = BoxId(UUID.randomUUID())
 }
 
 object RequestHeaders {