Merge pull request #2729 from ministryofjustice/feature/aps-1743-suit…

…ability-search-bug-fix

APS-1743 - Space search only returned a premise if it has 1 and only 1 room with the required characteristic

src/main/kotlin/uk/gov/justice/digital/hmpps/approvedpremisesapi/jpa/entity/cas1/Cas1SpaceSearchRepository.kt

@@ -13,7 +13,7 @@ private const val AP_TYPE_FILTER = """
 
 private const val PREMISES_CHARACTERISTICS_FILTER = """
   AND (
-    SELECT COUNT(*)
+    SELECT COUNT(distinct pc.characteristic_id)
     FROM premises_characteristics pc
     WHERE
       pc.premises_id = result.premises_id
@@ -23,10 +23,9 @@ private const val PREMISES_CHARACTERISTICS_FILTER = """
 
 private const val ROOM_CHARACTERISTICS_FILTER = """
   AND (
-    SELECT COUNT(*)
+    SELECT COUNT(distinct rc.characteristic_id)
     FROM room_characteristics rc
-    JOIN rooms r
-    ON rc.room_id = r.id
+    JOIN rooms r ON rc.room_id = r.id
     WHERE
       r.premises_id = result.premises_id
       AND rc.characteristic_id IN (:roomCharacteristics)

src/main/kotlin/uk/gov/justice/digital/hmpps/approvedpremisesapi/service/cas1/Cas1SpaceSearchService.kt

@@ -3,7 +3,6 @@ package uk.gov.justice.digital.hmpps.approvedpremisesapi.service.cas1
 import org.springframework.data.repository.findByIdOrNull
 import org.springframework.stereotype.Service
 import uk.gov.justice.digital.hmpps.approvedpremisesapi.api.model.Cas1SpaceSearchParameters
-import uk.gov.justice.digital.hmpps.approvedpremisesapi.api.model.Gender
 import uk.gov.justice.digital.hmpps.approvedpremisesapi.api.model.ServiceName
 import uk.gov.justice.digital.hmpps.approvedpremisesapi.jpa.entity.ApprovedPremiseApplicationRepository
 import uk.gov.justice.digital.hmpps.approvedpremisesapi.jpa.entity.CharacteristicEntity
@@ -54,33 +53,14 @@ class Cas1SpaceSearchService(
 
   private fun getRequiredCharacteristics(searchParameters: Cas1SpaceSearchParameters) = RequiredCharacteristics(
     searchParameters.requirements.apTypes?.map { it.asApprovedPremisesType() } ?: listOf(),
-    getGenderCharacteristics(searchParameters),
     getSpaceCharacteristics(searchParameters),
   )
 
-  private fun getGenderCharacteristics(searchParameters: Cas1SpaceSearchParameters): List<UUID> {
-    val characteristicNames = mutableListOf<String?>()
+  private fun getSpaceCharacteristics(searchParameters: Cas1SpaceSearchParameters): GroupedCharacteristics {
+    val propertyNames = searchParameters.requirements.spaceCharacteristics?.map { it.value } ?: listOf()
+    val characteristics = characteristicService.getCharacteristicsByPropertyNames(propertyNames, ServiceName.approvedPremises)
 
-    searchParameters.requirements.genders?.forEach {
-      characteristicNames += when (it) {
-        Gender.male -> null
-        Gender.female -> null
-      }
-    }
-
-    return getCharacteristicGroup(characteristicNames).premisesCharacteristics
-  }
-
-  private fun getSpaceCharacteristics(searchParameters: Cas1SpaceSearchParameters): CharacteristicGroup {
-    val characteristicNames = searchParameters.requirements.spaceCharacteristics?.map { it.value } ?: listOf()
-
-    return getCharacteristicGroup(characteristicNames)
-  }
-
-  private fun getCharacteristicGroup(characteristicNames: List<String?>): CharacteristicGroup {
-    val characteristics = characteristicService.getCharacteristicsByPropertyNames(characteristicNames.filterNotNull(), ServiceName.approvedPremises)
-
-    return CharacteristicGroup(
+    return GroupedCharacteristics(
       characteristics.filter { it.isPremisesCharacteristic() }.map { it.id },
       characteristics.filter { it.isRoomCharacteristic() }.map { it.id },
     )
@@ -121,11 +101,10 @@ class Cas1SpaceSearchService(
 
 data class RequiredCharacteristics(
   val apTypes: List<ApprovedPremisesType>,
-  val genders: List<UUID>,
-  val space: CharacteristicGroup,
+  val space: GroupedCharacteristics,
 )
 
-data class CharacteristicGroup(
+data class GroupedCharacteristics(
   val premisesCharacteristics: List<UUID>,
   val roomCharacteristics: List<UUID>,
 )

src/main/resources/application-local.yml

@@ -65,6 +65,8 @@ logging:
     # Uncomment the two entries below to see SQL binding
     #org.hibernate.orm.jdbc.bind: TRACE
     #org.hibernate.type.descriptor.sql.BasicBinder: TRACE
+    # Log jdbc template queries
+    org.springframework.jdbc.core.JdbcTemplate: debug
     # allows us to see the JWT token to simplify local API invocation
     uk.gov.justice.digital.hmpps.approvedpremisesapi.config.RequestResponseLoggingFilter: TRACE
     # allows us to see the request URL and method for upstream requests

src/test/kotlin/uk/gov/justice/digital/hmpps/approvedpremisesapi/integration/cas1/Cas1SpaceSearchTest.kt

@@ -20,6 +20,7 @@ import uk.gov.justice.digital.hmpps.approvedpremisesapi.integration.givens.given
 import uk.gov.justice.digital.hmpps.approvedpremisesapi.jpa.entity.ApprovedPremisesEntity
 import uk.gov.justice.digital.hmpps.approvedpremisesapi.jpa.entity.ApprovedPremisesGender
 import uk.gov.justice.digital.hmpps.approvedpremisesapi.transformer.cas1.Cas1SpaceSearchResultsTransformer
+import uk.gov.justice.digital.hmpps.approvedpremisesapi.util.randomInt
 import java.time.LocalDate
 
 class Cas1SpaceSearchTest : InitialiseDatabasePerClassTestBase() {
@@ -109,7 +110,7 @@ class Cas1SpaceSearchTest : InitialiseDatabasePerClassTestBase() {
 
   @ParameterizedTest
   @EnumSource
-  fun `Filtering APs by gender only returns APs matching associated gender in application`(gender: ApprovedPremisesGender) {
+  fun `Only returns APs matching associated gender in application`(gender: ApprovedPremisesGender) {
     postCodeDistrictFactory.produceAndPersist {
       withOutcode("SE1")
       withLatitude(-0.07)
@@ -359,7 +360,7 @@ class Cas1SpaceSearchTest : InitialiseDatabasePerClassTestBase() {
       }
 
       expectedPremises.forEach {
-        roomEntityFactory.produceAndPersist {
+        roomEntityFactory.produceAndPersistMultiple(amount = 5) {
           withPremises(it)
           withCharacteristicsList(listOf(characteristic.asCharacteristicEntity()))
         }
@@ -447,7 +448,7 @@ class Cas1SpaceSearchTest : InitialiseDatabasePerClassTestBase() {
       }
 
       expectedPremises.forEach {
-        roomEntityFactory.produceAndPersist {
+        roomEntityFactory.produceAndPersistMultiple(randomInt(1, 10)) {
           withPremises(it)
           withCharacteristicsList(Cas1SpaceCharacteristic.entries.slice(1..3).map { it.asCharacteristicEntity() })
         }

src/test/kotlin/uk/gov/justice/digital/hmpps/approvedpremisesapi/unit/service/cas1/Cas1SpaceSearchServiceTest.kt

@@ -186,11 +186,6 @@ class Cas1SpaceSearchServiceTest {
     verify(exactly = 1) {
       applicationRepository.findByIdOrNull(application.id)
 
-      characteristicService.getCharacteristicsByPropertyNames(
-        emptyList(),
-        ServiceName.approvedPremises,
-      )
-
       characteristicService.getCharacteristicsByPropertyNames(
         Cas1SpaceCharacteristic.entries.map { it.value },
         ServiceName.approvedPremises,
@@ -444,11 +439,6 @@ class Cas1SpaceSearchServiceTest {
     verify(exactly = 1) {
       applicationRepository.findByIdOrNull(application.id)
 
-      characteristicService.getCharacteristicsByPropertyNames(
-        emptyList(),
-        ServiceName.approvedPremises,
-      )
-
       spaceSearchRepository.findAllPremisesWithCharacteristicsByDistance(
         any(),
         any(),