API-4259: return errors from PPNS service and do not save subscriptio…

…n fields when (#85)

* saving to share

* API-4529: Fixed SubscriberFieldsService, started addressing associated tests

* API-4529: added more tests

* API-4529: Update tests for SubscriptionFieldsService

* API-4529: Updated service tests

* API-4529: Cleaned up commented out code

* API-4529: Added clientId to call to add callback in PPNS

Co-authored-by: Chris Rowe <chris@zeroalpha.technology>

app/uk/gov/hmrc/apisubscriptionfields/connector/JsonFormatters.scala

@@ -29,6 +29,8 @@ trait JsonFormatters {
   implicit val subscriberRequestJF = Json.format[SubscriberRequest]
   implicit val updateSubscriberRequestJF = Json.format[UpdateSubscriberRequest]
   implicit val updateSubscriberResponseJF = Json.format[UpdateSubscriberResponse]
+  implicit val updateCallBackUrlRequestJF = Json.format[UpdateCallBackUrlRequest]
+  implicit val updateCallBackUrlResponseJF = Json.format[UpdateCallBackUrlResponse]
 }
 
 object JsonFormatters extends JsonFormatters

app/uk/gov/hmrc/apisubscriptionfields/connector/Model.scala

@@ -27,3 +27,7 @@ private[connector] case class SubscriberRequest(callBackUrl: String, subscriberT
 private[connector] case class UpdateSubscriberRequest(subscriber: SubscriberRequest)
 
 private[connector] case class UpdateSubscriberResponse(boxId: BoxId)
+
+private[connector] case class UpdateCallBackUrlRequest(clientId: ClientId, callbackUrl: String)
+
+private[connector] case class UpdateCallBackUrlResponse(successful: Boolean, errorMessage: Option[String])

app/uk/gov/hmrc/apisubscriptionfields/connector/PushPullNotificationServiceConnector.scala

@@ -18,13 +18,15 @@ package uk.gov.hmrc.apisubscriptionfields.connector
 
 import javax.inject.{Inject, Singleton}
 import uk.gov.hmrc.apisubscriptionfields.config.ApplicationConfig
-import uk.gov.hmrc.apisubscriptionfields.model.{BoxId, ClientId}
+import uk.gov.hmrc.apisubscriptionfields.model.{BoxId, ClientId, PPNSCallBackUrlValidationResponse, PPNSCallBackUrlSuccessResponse, PPNSCallBackUrlFailedResponse}
 import uk.gov.hmrc.http.HeaderCarrier
 import uk.gov.hmrc.play.bootstrap.http.HttpClient
 import uk.gov.hmrc.play.http.metrics._
 
 import scala.concurrent.{ExecutionContext, Future}
 import scala.util.control.NonFatal
+import uk.gov.hmrc.apisubscriptionfields.model.PPNSCallBackUrlSuccessResponse
+import uk.gov.hmrc.apisubscriptionfields.model.PPNSCallBackUrlFailedResponse
 
 @Singleton
 class PushPullNotificationServiceConnector @Inject()(http: HttpClient, appConfig: ApplicationConfig, val apiMetrics: ApiMetrics)
@@ -54,4 +56,16 @@ class PushPullNotificationServiceConnector @Inject()(http: HttpClient, appConfig
       case NonFatal(e) => throw new RuntimeException(s"Unexpected response from $externalServiceUri: ${e.getMessage}")
     }
   }
+
+  def updateCallBackUrl(clientId: ClientId, boxId: BoxId, callbackUrl: String)(implicit hc: HeaderCarrier): Future[PPNSCallBackUrlValidationResponse] = {
+    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)
+    ).recover {
+      case NonFatal(e) => throw new RuntimeException(s"Unexpected response from $externalServiceUri: ${e.getMessage}")
+    }
+  }
 }

app/uk/gov/hmrc/apisubscriptionfields/model/Responses.scala

@@ -33,6 +33,10 @@ 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
+
 object ErrorCode extends Enumeration {
   type ErrorCode = Value
 

app/uk/gov/hmrc/apisubscriptionfields/service/PushPullNotificationService.scala

@@ -21,8 +21,6 @@ import javax.inject.{Inject, Singleton}
 import uk.gov.hmrc.apisubscriptionfields.model.FieldDefinition
 import scala.concurrent.Future
 import uk.gov.hmrc.apisubscriptionfields.model._
-import uk.gov.hmrc.apisubscriptionfields.model.Types._
-import cats.data.{NonEmptyList => NEL}
 import scala.concurrent.ExecutionContext
 import uk.gov.hmrc.http.HeaderCarrier
 
@@ -33,27 +31,16 @@ class PushPullNotificationService @Inject()(ppnsConnector: PushPullNotificationS
     s"${apiContext.value}${separator}${apiVersion.value}${separator}${fieldDefinition.name.value}"
   }
 
-  private def subscribeToPPNS(clientId: ClientId,
+  def subscribeToPPNS(clientId: ClientId,
                               apiContext: ApiContext,
                               apiVersion: ApiVersion,
-                              fieldDefinition: FieldDefinition,
-                              oFieldValue: Option[FieldValue])
-                             (implicit hc: HeaderCarrier) = {
+                              callBackUrl: String,
+                              fieldDefinition: FieldDefinition)
+                             (implicit hc: HeaderCarrier): Future[PPNSCallBackUrlValidationResponse] = {
     for {
       boxId <- ppnsConnector.ensureBoxIsCreated(makeBoxName(apiContext, apiVersion, fieldDefinition), clientId)
-      _ <- oFieldValue.fold(Future.successful(()))(fieldValue => ppnsConnector.subscribe(boxId, fieldValue))
-    } yield ()
+      result <- ppnsConnector.updateCallBackUrl(clientId, boxId, callBackUrl)
+    } yield result
   }
 
-  def subscribeToPPNS(clientId: ClientId, apiContext: ApiContext, apiVersion: ApiVersion, fieldDefinitions: NEL[FieldDefinition], fields: Fields)
-                     (implicit hc: HeaderCarrier): Future[Unit] = {
-    val subscriptionResponses : List[Future[Unit]] =
-      fieldDefinitions
-      .filter(_.`type` == FieldDefinitionType.PPNS_FIELD )
-      .map { fieldDefn =>
-        subscribeToPPNS(clientId, apiContext, apiVersion, fieldDefn, fields.get(fieldDefn.name).filterNot(_.isEmpty))
-      }
-
-    Future.sequence(subscriptionResponses).map(_ => ())
-  }
 }

app/uk/gov/hmrc/apisubscriptionfields/service/SubscriptionFieldsService.scala

@@ -28,6 +28,7 @@ import scala.concurrent.Future.successful
 import cats.data.NonEmptyList
 import uk.gov.hmrc.apisubscriptionfields.repository.SubscriptionFieldsRepository
 import uk.gov.hmrc.http.HeaderCarrier
+import cats.data.{NonEmptyList => NEL}
 
 @Singleton
 class UUIDCreator {
@@ -49,24 +50,48 @@ class SubscriptionFieldsService @Inject() (
     }
   }
 
-  private def upsert(clientId: ClientId, apiContext: ApiContext, apiVersion: ApiVersion, fields: Fields, fieldDefinitions: NonEmptyList[FieldDefinition])(implicit hc: HeaderCarrier): Future[SuccessfulSubsFieldsUpsertResponse] = {
+  private def upsertSubscriptionFields(clientId: ClientId, apiContext: ApiContext, apiVersion: ApiVersion, fields: Fields)
+                                      (implicit hc: HeaderCarrier): Future[SuccessfulSubsFieldsUpsertResponse] = {
     val subscriptionFieldsId = SubscriptionFieldsId(uuidCreator.uuid())
     val subscriptionFields = SubscriptionFields(clientId, apiContext, apiVersion, subscriptionFieldsId, fields)
 
-    for {
-      result  <- repository.saveAtomic(subscriptionFields)
-      _       <- pushPullNotificationService.subscribeToPPNS(clientId, apiContext, apiVersion, fieldDefinitions, fields)
-    } yield SuccessfulSubsFieldsUpsertResponse(result._1, result._2)
+    repository.saveAtomic(subscriptionFields)
+      .map(result => SuccessfulSubsFieldsUpsertResponse(result._1, result._2))
+  }
+
+  def handlePPNS(clientId: ClientId,
+                 apiContext: ApiContext,
+                 apiVersion: ApiVersion,
+                 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 callBackUrl: Option[FieldValue] = fields.get(fieldDefinition.name)
+        val callBackResponse: Future[PPNSCallBackUrlValidationResponse] = callBackUrl match {
+            case Some(fieldValue) => pushPullNotificationService.subscribeToPPNS(clientId, apiContext, apiVersion, fieldValue, fieldDefinition)
+            case None => Future.successful(PPNSCallBackUrlSuccessResponse)
+          }
+        callBackResponse.flatMap {
+          case PPNSCallBackUrlSuccessResponse =>  upsertSubscriptionFields(clientId, apiContext, apiVersion, fields)
+          case PPNSCallBackUrlFailedResponse(error) => Future.successful(FailedValidationSubsFieldsUpsertResponse(Map(fieldDefinition.name -> error)))
+        }
+      case None =>  upsertSubscriptionFields(clientId, apiContext, apiVersion, fields)
+    }
+
   }
 
-  def upsert(clientId: ClientId, apiContext: ApiContext, apiVersion: ApiVersion, fields: Fields)(implicit hc: HeaderCarrier): Future[SubsFieldsUpsertResponse] = {
-    val foFieldDefinitions: Future[Option[NonEmptyList[FieldDefinition]]] = apiFieldDefinitionsService.get(apiContext, apiVersion).map(_.map(_.fieldDefinitions))
+  def upsert(clientId: ClientId, apiContext: ApiContext, apiVersion: ApiVersion, fields: Fields)
+            (implicit hc: HeaderCarrier): Future[SubsFieldsUpsertResponse] = {
+    val foFieldDefinitions: Future[Option[NonEmptyList[FieldDefinition]]] =
+      apiFieldDefinitionsService.get(apiContext, apiVersion).map(_.map(_.fieldDefinitions))
 
     foFieldDefinitions.flatMap( _ match {
       case None                   => successful(NotFoundSubsFieldsUpsertResponse)
       case Some(fieldDefinitions) => {
         validate(fields, fieldDefinitions) match {
-          case ValidSubsFieldValidationResponse                       => upsert(clientId, apiContext, apiVersion, fields, fieldDefinitions)
+          case ValidSubsFieldValidationResponse                       => handlePPNS(clientId, apiContext, apiVersion, fieldDefinitions, fields)
           case InvalidSubsFieldValidationResponse(fieldErrorMessages) => successful(FailedValidationSubsFieldsUpsertResponse(fieldErrorMessages))
         }
       }

test/uk/gov/hmrc/apisubscriptionfields/FieldDefinitionTestData.scala

@@ -34,6 +34,7 @@ trait FieldDefinitionTestData extends TestData {
 
   final val AlphanumericFieldName: FieldName = "alphanumericField"
   final val PasswordFieldName: FieldName = "password"
+  final val PPNSFieldFieldName: FieldName = "callbackurl"
 
   final val FakeValidationRule: RegexValidationRule = RegexValidationRule(".*")
 
@@ -52,9 +53,14 @@ trait FieldDefinitionTestData extends TestData {
 
   final val AlphaNumericRegexRule: RegexValidationRule = RegexValidationRule("^[a-zA-Z0-9]+$")
   final val PasswordRegexRule: RegexValidationRule = RegexValidationRule("^(?=.{8,})(?=.*[a-z])(?=.*[A-Z])(?=.*[@#$%^&+=]).*$")
+  final val CallBackUrlRegexRule: RegexValidationRule = RegexValidationRule("^https.*")
   final val FakeValidationForAlphanumeric: ValidationGroup = ValidationGroup("Needs to be alpha numeric", NonEmptyList.one(AlphaNumericRegexRule))
   final val FakeValidationForPassword: ValidationGroup =
     ValidationGroup("Needs to be at least 8 chars with at least one lowercase, uppercase and special char", NonEmptyList.one(PasswordRegexRule))
+  final val FakeValidationForPPNS: ValidationGroup =
+    ValidationGroup("CallBackUrl Validation", NonEmptyList.one(CallBackUrlRegexRule))
+
+
   final val FakeFieldDefinitionAlphnumericField = FieldDefinition(
     "alphanumericField",
     "an alphanumeric filed",
@@ -65,10 +71,17 @@ trait FieldDefinitionTestData extends TestData {
   )
   final val FakeFieldDefinitionPassword =
     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))
   final val FakeApiFieldDefinitionssWithRegex = NonEmptyList.fromListUnsafe(List(FakeFieldDefinitionAlphnumericField, FakeFieldDefinitionPassword))
+  final val FakeApiFieldDefinitionsPPNSWithRegex = NonEmptyList.fromListUnsafe(List(FakeFieldDefinitionAlphnumericField, FakeFieldDefinitionPassword, FakeFieldDefinitionPPNSFields))
+
   final val FakeApiFieldDefinitionsWithRegex = ApiFieldDefinitions(FakeContext, FakeVersion, FakeApiFieldDefinitionssWithRegex)
   final val FakeApiFieldDefinitionsResponseWithRegex: ApiFieldDefinitions = ApiFieldDefinitions(FakeContext, FakeVersion, FakeApiFieldDefinitionssWithRegex)
 
+  final val FakeApiFieldDefinitionsResponsePPNSWithRegex: ApiFieldDefinitions = ApiFieldDefinitions(FakeContext, FakeVersion, FakeApiFieldDefinitionsPPNSWithRegex)
+
 
   final val jsonInvalidRegexFieldsDefinitionRequest =
     """{

test/uk/gov/hmrc/apisubscriptionfields/SubscriptionFieldsTestData.scala

@@ -31,6 +31,9 @@ trait SubscriptionFieldsTestData extends FieldDefinitionTestData with Validation
   final val EmptyResponse: Future[Option[SubscriptionFields]] = Future.successful(None)
   final val FakeSubscriptionFields: Map[FieldName, String] = Map(fieldN(1) -> "X", fieldN(2) -> "Y")
   final val SubscriptionFieldsMatchRegexValidation: Fields = Map(AlphanumericFieldName -> "ABC123abc", PasswordFieldName -> "Qw12@erty")
+  final val SubscriptionFieldsMatchRegexValidationPPNS: Fields = Map(AlphanumericFieldName -> "ABC123abc", PasswordFieldName -> "Qw12@erty", PPNSFieldFieldName -> "https://www.mycallbackurl.com")
+  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")
 
   final val FakeApiSubscription = SubscriptionFields(FakeClientId, FakeContext, FakeVersion, FakeFieldsId, FakeSubscriptionFields)

test/uk/gov/hmrc/apisubscriptionfields/connector/PushPullNotificationServiceConnectorSpec.scala

@@ -32,8 +32,7 @@ import org.scalatest.BeforeAndAfterAll
 import play.api.http.HeaderNames.{CONTENT_TYPE, USER_AGENT}
 import uk.gov.hmrc.apisubscriptionfields.AsyncHmrcSpec
 import uk.gov.hmrc.http.HeaderCarrier
-import uk.gov.hmrc.apisubscriptionfields.model.{BoxId, SubscriptionFieldsId}
-import uk.gov.hmrc.apisubscriptionfields.model.ClientId
+import uk.gov.hmrc.apisubscriptionfields.model._
 
 class PushPullNotificationServiceConnectorSpec
     extends AsyncHmrcSpec
@@ -76,57 +75,97 @@ class PushPullNotificationServiceConnectorSpec
     val boxId = BoxId(ju.UUID.randomUUID())
 
     val connector = app.injector.instanceOf[PushPullNotificationServiceConnector]
+
+    def primeStub(path: String, requestBody: String, responseBody: String){
+        wireMockServer.stubFor(
+        put(path).withRequestBody(equalTo(requestBody))
+        .willReturn(aResponse()
+          .withHeader(CONTENT_TYPE, "application/json")
+          .withBody(responseBody)
+          .withStatus(OK)))
+    }
+
+    def verifyMock(path: String){
+        wireMockServer.verify(
+        putRequestedFor(urlPathEqualTo(path))
+        .withHeader(CONTENT_TYPE, equalTo("application/json"))
+        .withHeader(USER_AGENT, equalTo("api-subscription-fields"))
+      )
+    }
+          implicit val hc: HeaderCarrier = HeaderCarrier()
   }
 
   "PPNS Connector" should {
     "send proper request to post box" in new Setup {
       val requestBody = Json.stringify(Json.toJson(CreateBoxRequest(boxName, clientId)))
-      val response: CreateBoxResponse = CreateBoxResponse(boxId)
+      val responseBody = Json.stringify(Json.toJson(CreateBoxResponse(boxId)))
 
       val path = "/box"
-      wireMockServer.stubFor(
-        put(path).withRequestBody(equalTo(requestBody))
-        .willReturn(aResponse()
-          .withHeader(CONTENT_TYPE, "application/json")
-          .withBody(Json.stringify(Json.toJson(response)))
-          .withStatus(OK)))
-
-      implicit val hc: HeaderCarrier = HeaderCarrier()
+      primeStub(path, requestBody, responseBody)
 
       val ret = await(connector.ensureBoxIsCreated(boxName, clientId))
       ret shouldBe (boxId)
 
-      wireMockServer.verify(
-        putRequestedFor(urlPathEqualTo(path))
-        .withHeader(CONTENT_TYPE, equalTo("application/json"))
-        .withHeader(USER_AGENT, equalTo("api-subscription-fields"))
-      )
+     verifyMock(path)
     }
 
     "send proper request to subscribe" in new Setup {
       val callbackUrl = "my-callback"
-      val updateRequest: UpdateSubscriberRequest = UpdateSubscriberRequest(SubscriberRequest(callbackUrl, "API_PUSH_SUBSCRIBER"))
-      val requestBody = Json.stringify(Json.toJson(updateRequest))
-      val response: UpdateSubscriberResponse = UpdateSubscriberResponse(boxId)
+      val requestBody = Json.stringify(Json.toJson(UpdateSubscriberRequest(SubscriberRequest(callbackUrl, "API_PUSH_SUBSCRIBER"))))
+      val responseBody = Json.stringify(Json.toJson(UpdateSubscriberResponse(boxId)))
 
       val path = s"/box/${boxId.value}/subscriber"
-      wireMockServer.stubFor(
-        put(path).withRequestBody(equalTo(requestBody))
-        .willReturn(aResponse()
-          .withHeader(CONTENT_TYPE, "application/json")
-          .withBody(Json.stringify(Json.toJson(response)))
-          .withStatus(OK)))
-
-      implicit val hc: HeaderCarrier = HeaderCarrier()
+     primeStub(path, requestBody, responseBody)
 
       val ret = await(connector.subscribe(boxId, callbackUrl))
       ret shouldBe ()
 
-      wireMockServer.verify(
-        putRequestedFor(urlPathEqualTo(path))
-        .withHeader(CONTENT_TYPE, equalTo("application/json"))
-        .withHeader(USER_AGENT, equalTo("api-subscription-fields"))
-      )
+      verifyMock(path)
+    }
+
+
+
+     "send proper request to update callback and map response on success" in new Setup {
+      val callbackUrl = "my-callback"
+      val requestBody = Json.stringify(Json.toJson(UpdateCallBackUrlRequest(clientId, callbackUrl)))
+      val responseBody = Json.stringify(Json.toJson(UpdateCallBackUrlResponse(true, None)))
+
+      val path = s"/box/${boxId.value}/callback"
+     primeStub(path, requestBody, responseBody)
+
+      val ret: PPNSCallBackUrlValidationResponse = await(connector.updateCallBackUrl(clientId, boxId, callbackUrl))
+      ret shouldBe PPNSCallBackUrlSuccessResponse
+
+    verifyMock(path)
+    }
+
+
+     "send proper request to update callback and map response on failure" in new Setup {
+      val callbackUrl = "my-callback"
+      val requestBody = Json.stringify(Json.toJson( UpdateCallBackUrlRequest(clientId, callbackUrl)))
+      val responseBody = Json.stringify(Json.toJson(UpdateCallBackUrlResponse(false, Some("some error"))))
+
+      val path = s"/box/${boxId.value}/callback"
+      primeStub(path, requestBody, responseBody)
+
+      val ret: PPNSCallBackUrlValidationResponse = await(connector.updateCallBackUrl(clientId, boxId, callbackUrl))
+      ret shouldBe PPNSCallBackUrlFailedResponse("some error")
+
+     verifyMock(path)
+    }
+
+    "send proper request to update callback and map response on failure with Unknown Error" in new Setup {
+      val callbackUrl = "my-callback"
+      val requestBody = Json.stringify(Json.toJson(UpdateCallBackUrlRequest(clientId, callbackUrl)))
+      val responseBody = Json.stringify(Json.toJson(UpdateCallBackUrlResponse(false, None)))
+
+      val path = s"/box/${boxId.value}/callback"
+      primeStub(path, requestBody, responseBody)
+
+      val ret: PPNSCallBackUrlValidationResponse = await(connector.updateCallBackUrl(clientId, boxId, callbackUrl))
+      ret shouldBe PPNSCallBackUrlFailedResponse("Unknown Error")
+
+     verifyMock(path)
     }
   }
 }