HIPP-1697: Remove deleteAllClients method (#213)

* HIPP-1697: Remove deleteAllClients method * HIPP-1697: Address PR commnets
hmrc · Nov 28, 2024 · c98ffeb · c98ffeb
1 parent a4e7b9e
commit c98ffeb
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 407 deletions.
diff --git a/app/uk/gov/hmrc/apihubapplications/connectors/IdmsConnector.scala b/app/uk/gov/hmrc/apihubapplications/connectors/IdmsConnector.scala
@@ -39,6 +39,4 @@ trait IdmsConnector {
 
   def fetchClientScopes(environmentName: EnvironmentName, clientId: String)(implicit hc: HeaderCarrier): Future[Either[IdmsException, Seq[ClientScope]]]
 
-  def deleteAllClients(application: Application)(implicit hc: HeaderCarrier): Future[Either[IdmsException, Unit]]
-
 }
diff --git a/app/uk/gov/hmrc/apihubapplications/connectors/IdmsConnectorImpl.scala b/app/uk/gov/hmrc/apihubapplications/connectors/IdmsConnectorImpl.scala
@@ -113,20 +113,6 @@ class IdmsConnectorImpl @Inject()(
       }
   }
 
-  override def deleteAllClients(application: Application)(implicit hc: HeaderCarrier): Future[Either[IdmsException, Unit]] = {
-    val allFutures = Seq(Primary, Secondary).flatMap(environmentName => {
-      application.getCredentials(environmentName).map(credential => deleteClient(environmentName, credential.clientId))
-    })
-
-    Future.sequence(allFutures)
-      .map(ignoreClientNotFound)
-      .map(useFirstException)
-      .map {
-        case Right(_) => Right(())
-        case Left(e) => Left(e)
-      }
-  }
-
   private def baseUrlForEnvironment(environmentName: EnvironmentName): String = {
     val baseUrl = servicesConfig.baseUrl(s"idms-$environmentName")
     val path = servicesConfig.getConfString(s"idms-$environmentName.path", "")

diff --git a/app/uk/gov/hmrc/apihubapplications/connectors/IdmsConnectorParityImpl.scala b/app/uk/gov/hmrc/apihubapplications/connectors/IdmsConnectorParityImpl.scala
@@ -219,24 +219,6 @@ class IdmsConnectorParityImpl @Inject()(
       }
   }
 
-  override def deleteAllClients(application: Application)(implicit hc: HeaderCarrier): Future[Either[IdmsException, Unit]] = {
-    Future.sequence(
-        hipEnvironments.environments.flatMap(
-          hipEnvironment =>
-            application.getCredentials(hipEnvironment.environmentName).map(
-              credential =>
-                deleteClient(hipEnvironment.environmentName, credential.clientId)
-            )
-        )
-      )
-      .map(ignoreClientNotFound)
-      .map(useFirstException)
-      .map {
-        case Right(_) => Right(())
-        case Left(e) => Left(e)
-      }
-  }
-
   private def headersForEnvironment(hipEnvironment: HipEnvironment): Seq[(String, String)] = {
     Seq(
       Some(("Authorization", authorizationForEnvironment(hipEnvironment))),

diff --git a/app/uk/gov/hmrc/apihubapplications/services/ApplicationsLifecycleService.scala b/app/uk/gov/hmrc/apihubapplications/services/ApplicationsLifecycleService.scala
@@ -22,9 +22,10 @@ import uk.gov.hmrc.apihubapplications.config.HipEnvironments
 import uk.gov.hmrc.apihubapplications.connectors.{EmailConnector, IdmsConnector}
 import uk.gov.hmrc.apihubapplications.models.application.{Application, Deleted, NewApplication, Primary, Secondary, TeamMember}
 import uk.gov.hmrc.apihubapplications.models.application.ApplicationLenses.*
-import uk.gov.hmrc.apihubapplications.models.exception.{ApplicationsException, ExceptionRaising}
+import uk.gov.hmrc.apihubapplications.models.exception.{ApplicationsException, ExceptionRaising, IdmsException}
 import uk.gov.hmrc.apihubapplications.repositories.ApplicationsRepository
 import uk.gov.hmrc.apihubapplications.services.helpers.ApplicationEnrichers
+import uk.gov.hmrc.apihubapplications.services.helpers.Helpers.{ignoreClientNotFound, useFirstException}
 import uk.gov.hmrc.http.HeaderCarrier
 
 import java.time.{Clock, LocalDateTime}
@@ -82,7 +83,7 @@ class ApplicationsLifecycleServiceImpl @ Inject()(
   override def delete(applicationId: String, currentUser: String)(implicit hc: HeaderCarrier): Future[Either[ApplicationsException, Unit]] = {
     searchService.findById(applicationId).flatMap {
       case Right(application) =>
-        idmsConnector.deleteAllClients(application) flatMap {
+        deleteClients(application, hipEnvironments).flatMap {
           case Right(_) =>
             accessRequestsService.cancelAccessRequests(applicationId) flatMap {
               case Right(_) =>
@@ -108,6 +109,17 @@ class ApplicationsLifecycleServiceImpl @ Inject()(
     }
   }
 
+  private def deleteClients(application: Application, hipEnvironments: HipEnvironments)(implicit hc: HeaderCarrier): Future[Either[IdmsException, Unit]] =
+    Future.sequence(hipEnvironments.environments.flatMap(hipEnvironment =>
+      application.getCredentials(hipEnvironment.environmentName)
+        .map(credential => idmsConnector.deleteClient(hipEnvironment.environmentName, credential.clientId))
+    )).map(ignoreClientNotFound)
+    .map(useFirstException)
+    .map {
+      case Right(_) => Right(())
+      case Left(e) => Left(e)
+    }
+
   private def softDelete(application: Application, currentUser: String): Future[Either[ApplicationsException, Unit]] = {
     val softDeletedApplication = application.copy(deleted = Some(Deleted(LocalDateTime.now(clock), currentUser)))
     repository.update(softDeletedApplication)

diff --git a/app/uk/gov/hmrc/apihubapplications/services/ApplicationsService.scala b/app/uk/gov/hmrc/apihubapplications/services/ApplicationsService.scala
@@ -21,15 +21,16 @@ import uk.gov.hmrc.apihubapplications.models.application.*
 import uk.gov.hmrc.apihubapplications.models.exception.*
 import uk.gov.hmrc.apihubapplications.models.requests.AddApiRequest
 import uk.gov.hmrc.http.HeaderCarrier
-import uk.gov.hmrc.apihubapplications.config.HipEnvironment
+import uk.gov.hmrc.apihubapplications.config.{HipEnvironment, HipEnvironments}
+
 import scala.concurrent.Future
 
 @Singleton
 class ApplicationsService @Inject()(
   apiService: ApplicationsApiService,
   credentialsService: ApplicationsCredentialsService,
   lifecycleService: ApplicationsLifecycleService,
-  searchService: ApplicationsSearchService
+  searchService: ApplicationsSearchService,
 ) extends ApplicationsApiService
   with ApplicationsCredentialsService
   with ApplicationsLifecycleService

diff --git a/it/test/uk/gov/hmrc/apihubapplications/IdmsConnectorParityImplSpec.scala b/it/test/uk/gov/hmrc/apihubapplications/IdmsConnectorParityImplSpec.scala
@@ -589,201 +589,6 @@ class IdmsConnectorParityImplSpec
       }
     }
   }
-
-  "IdmsConnector.deleteAllClients" - {
-    "must place the correct requests to IDMS and succeed" in {
-      val clientId1 = "test-client-id-1"
-      val clientId2 = "test-client-id-2"
-
-      stubFor(
-        delete(urlEqualTo(s"/$Primary/identity/clients/$clientId1"))
-          .withHeader("Authorization", equalTo(authorizationHeaderFor(Primary)))
-          .withHeader("x-api-key", apiKeyHeaderPatternFor(Primary))
-          .withHeader("X-Correlation-Id", equalTo(correlationId))
-          .willReturn(aResponse()))
-
-      stubFor(
-        delete(urlEqualTo(s"/$Secondary/identity/clients/$clientId2"))
-          .withHeader("Authorization", equalTo(authorizationHeaderFor(Secondary)))
-          .withHeader("x-api-key", apiKeyHeaderPatternFor(Secondary))
-          .willReturn(aResponse()))
-
-      val id = "test-id"
-      val application = buildApplication(clientId1, clientId2, id)
-
-      buildConnector(this).deleteAllClients(application)(HeaderCarrier(requestId = requestId)) map {
-        actual =>
-          verify(deleteRequestedFor(urlEqualTo(s"/$Primary/identity/clients/$clientId1")))
-          verify(deleteRequestedFor(urlEqualTo(s"/$Secondary/identity/clients/$clientId2")))
-          actual mustBe Right(())
-      }
-    }
-
-    "must ignore IdmsException with an IdmsIssue of ClientNotFound when IDMS returns 404 Not Found" in {
-      val clientId1 = "test-client-id-1"
-      val clientId2 = "test-client-id-2"
-
-      val context = Seq(
-        "environmentName" -> Secondary,
-        "clientId" -> clientId2,
-        "X-Correlation-Id" -> correlationId
-      )
-
-      stubFor(
-        delete(urlEqualTo(s"/$Primary/identity/clients/$clientId1"))
-          .willReturn(
-            aResponse()
-              .withStatus(NOT_FOUND)
-          )
-      )
-
-      stubFor(
-        delete(urlEqualTo(s"/$Secondary/identity/clients/$clientId2"))
-          .willReturn(
-            aResponse()
-              .withStatus(INTERNAL_SERVER_ERROR)
-          )
-      )
-
-      val id = "test-id"
-      val application = buildApplication(clientId1, clientId2, id)
-
-      buildConnector(this).deleteAllClients(application)(HeaderCarrier(requestId = requestId)) map {
-        actual =>
-          verify(deleteRequestedFor(urlEqualTo(s"/$Primary/identity/clients/$clientId1")))
-          verify(deleteRequestedFor(urlEqualTo(s"/$Secondary/identity/clients/$clientId2")))
-          actual mustBe Left(IdmsException.unexpectedResponse(500, context))
-      }
-    }
-
-    "must return IdmsException for any non-success response apart from 404" in {
-      val clientId1 = "test-client-id-1"
-      val clientId2 = "test-client-id-2"
-
-      val context = Seq(
-        "environmentName" -> Primary,
-        "clientId" -> clientId1,
-        "X-Correlation-Id" -> correlationId
-      )
-
-      forAll(nonSuccessResponses) { (status: Int) =>
-        stubFor(
-          delete(urlEqualTo(s"/$Primary/identity/clients/$clientId1"))
-            .willReturn(
-              aResponse()
-                .withStatus(status)
-            )
-        )
-
-        stubFor(
-          delete(urlEqualTo(s"/$Secondary/identity/clients/$clientId2"))
-            .willReturn(
-              aResponse()
-                .withStatus(status)
-            )
-        )
-
-        val id = "test-id"
-        val application = buildApplication(clientId1, clientId2, id)
-
-        buildConnector(this).deleteAllClients(application)(HeaderCarrier(requestId = requestId)) map {
-          actual =>
-            actual mustBe Left(IdmsException.unexpectedResponse(status, context))
-        }
-      }
-    }
-
-    "must return IdmsException for any errors" in {
-      val clientId1 = "test-client-id-1"
-      val clientId2 = "test-client-id-2"
-
-      stubFor(
-        delete(urlEqualTo(s"/$Primary/identity/clients/$clientId1"))
-          .willReturn(
-            aResponse()
-              .withFault(Fault.CONNECTION_RESET_BY_PEER)
-          )
-      )
-
-      stubFor(
-        delete(urlEqualTo(s"/$Secondary/identity/clients/$clientId2"))
-          .willReturn(
-            aResponse()
-              .withFault(Fault.CONNECTION_RESET_BY_PEER)
-          )
-      )
-
-      val id = "test-id"
-      val application = buildApplication(clientId1, clientId2, id)
-
-      buildConnector(this).deleteAllClients(application)(HeaderCarrier(requestId = requestId)) map {
-        actual =>
-          actual.left.value mustBe a[IdmsException]
-          actual.left.value.issue mustBe CallError
-      }
-    }
-
-    "must return InvalidCredential for 401 errors" in {
-      val clientId1 = "test-client-id-1"
-      val clientId2 = "test-client-id-2"
-
-      stubFor(
-        delete(urlEqualTo(s"/$Primary/identity/clients/$clientId1"))
-          .willReturn(
-            aResponse()
-              .withStatus(401)
-          )
-      )
-
-      stubFor(
-        delete(urlEqualTo(s"/$Secondary/identity/clients/$clientId2"))
-          .willReturn(
-            aResponse()
-              .withStatus(401)
-          )
-      )
-
-      val id = "test-id"
-      val application = buildApplication(clientId1, clientId2, id)
-
-      buildConnector(this).deleteAllClients(application)(HeaderCarrier(requestId = requestId)) map {
-        actual =>
-          actual.left.value mustBe a[IdmsException]
-          actual.left.value.issue mustBe InvalidCredential
-      }
-    }
-
-    "must return InvalidCredential for 403 errors" in {
-      val clientId1 = "test-client-id-1"
-      val clientId2 = "test-client-id-2"
-
-      stubFor(
-        delete(urlEqualTo(s"/$Primary/identity/clients/$clientId1"))
-          .willReturn(
-            aResponse()
-              .withStatus(403)
-          )
-      )
-
-      stubFor(
-        delete(urlEqualTo(s"/$Secondary/identity/clients/$clientId2"))
-          .willReturn(
-            aResponse()
-              .withStatus(403)
-          )
-      )
-
-      val id = "test-id"
-      val application = buildApplication(clientId1, clientId2, id)
-
-      buildConnector(this).deleteAllClients(application)(HeaderCarrier(requestId = requestId)) map {
-        actual =>
-          actual.left.value mustBe a[IdmsException]
-          actual.left.value.issue mustBe InvalidCredential
-      }
-    }
-  }
-
 }
 
 object IdmsConnectorParityImplSpec extends HttpClientV2Support with TableDrivenPropertyChecks {
Original file line number	Diff line number	Diff line change
Expand Up		@@ -39,6 +39,4 @@ trait IdmsConnector {

		def fetchClientScopes(environmentName: EnvironmentName, clientId: String)(implicit hc: HeaderCarrier): Future[Either[IdmsException, Seq[ClientScope]]]

		def deleteAllClients(application: Application)(implicit hc: HeaderCarrier): Future[Either[IdmsException, Unit]]

		}