Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDIT-826 Model agency establishment as embeddedid #1602

Merged
merged 3 commits into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion build.gradle.kts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
plugins {
id("uk.gov.justice.hmpps.gradle-spring-boot") version "5.1.4-beta-4"
// id("uk.gov.justice.hmpps.gradle-spring-boot") version "5.1.4-beta-4"
id("uk.gov.justice.hmpps.gradle-spring-boot") version "5.2.0-beta-2"
kotlin("plugin.spring") version "1.8.21"
kotlin("plugin.jpa") version "1.8.21"
kotlin("plugin.lombok") version "1.8.21"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package uk.gov.justice.hmpps.prison.repository.jpa.model

import jakarta.persistence.DiscriminatorValue
import jakarta.persistence.Entity
import lombok.NoArgsConstructor

@Entity
@DiscriminatorValue(AgencyEstablishmentType.ESTABLISHMENT_TYPE)
@NoArgsConstructor
class AgencyEstablishmentType(code: String?, description: String?) :
ReferenceCode(ESTABLISHMENT_TYPE, code, description) {
Comment on lines +10 to +11
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Model this as a reference code instead of loading manually.


companion object {
const val ESTABLISHMENT_TYPE = "ESTAB_TYPE"
fun pk(code: String?): Pk {
return Pk(ESTABLISHMENT_TYPE, code)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public class AgencyLocation extends AuditableEntity {
@Column(name = "LONG_DESCRIPTION")
private String longDescription;

@OneToMany(mappedBy = "agencyLocId", cascade = CascadeType.ALL)
@OneToMany(mappedBy = "agencyLocation", cascade = CascadeType.ALL, fetch = FetchType.LAZY, orphanRemoval = true)
@Default
private List<AgencyLocationEstablishment> establishmentTypes = new ArrayList<>();

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package uk.gov.justice.hmpps.prison.repository.jpa.model

import jakarta.persistence.Embeddable
import jakarta.persistence.EmbeddedId
import jakarta.persistence.Entity
import jakarta.persistence.FetchType
import jakarta.persistence.JoinColumn
import jakarta.persistence.ManyToOne
import jakarta.persistence.Table
import org.hibernate.Hibernate
import org.hibernate.annotations.JoinColumnOrFormula
import org.hibernate.annotations.JoinColumnsOrFormulas
import org.hibernate.annotations.JoinFormula
import java.io.Serializable

@Embeddable
data class AgencyLocationEstablishmentId(
@JoinColumn(name = "AGY_LOC_ID", nullable = false)
val agencyLocationId: String,

@JoinColumn(name = "ESTABLISHMENT_TYPE", nullable = false)
val establishmentTypeCode: String,
) : Serializable

@Entity
@Table(name = "AGY_LOC_ESTABLISHMENTS")
data class AgencyLocationEstablishment(
@EmbeddedId
val id: AgencyLocationEstablishmentId,
Comment on lines +29 to +30
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to EmbeddedId as the IdClass wasn't mapping correctly on a OneToMany annotation in the parent class AgencyLocation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually to be more accurate...

We could have just continued to use @Id on the columns as long as we modelled AgencyLocation in the child as a Hibernate managed object instead of just the id. This would then fulfil the oppopsite side of the bidirectional relationship and Hibernate would be happy. (and note we could have removed the @IdClass as that's only needed for repositories but we don't have one for this class).

BUT as I changed the establishmentType to be a reference code this means that we had to use a JoinFormula and you can't have both @Id and a join formula on the same column (Hibernate throws in that case). Hence we need @EmbeddedId.


@ManyToOne(optional = false, fetch = FetchType.LAZY)
@JoinColumn(name = "AGY_LOC_ID", updatable = false, insertable = false)
val agencyLocation: AgencyLocation,

@ManyToOne
@JoinColumnsOrFormulas(
value = [
JoinColumnOrFormula(
formula = JoinFormula(
value = "'" + AgencyEstablishmentType.ESTABLISHMENT_TYPE + "'",
referencedColumnName = "domain",
),
), JoinColumnOrFormula(column = JoinColumn(name = "ESTAB_TYPE", referencedColumnName = "code", nullable = true, updatable = false, insertable = false)),
],
)
val establishmentType: AgencyEstablishmentType,
) {

override fun equals(other: Any?): Boolean {
if (this === other) return true
if (other == null || Hibernate.getClass(this) != Hibernate.getClass(other)) return false
other as AgencyLocationEstablishment
return id.agencyLocationId == other.id.agencyLocationId &&
id.establishmentTypeCode == other.id.establishmentTypeCode
}

override fun hashCode(): Int {
return this.javaClass.hashCode()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ public class OffenderSubAccount {
@Column(name = "TRUST_ACCOUNT_CODE", nullable = false, insertable = false, updatable = false)
private Long accountCode;

@Id
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not actually included in the id and causes an error in latest Hibernate verison.

@Column(name = "BALANCE", nullable = false)
private BigDecimal balance;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ public class OffenderTrustAccount {
@Column(name = "OFFENDER_ID", nullable = false, insertable = false, updatable = false)
private Long offenderId;

@Id
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not actually included in the id and causes an error in latest Hibernate verison.

@Column(name = "ACCOUNT_CLOSED_FLAG", nullable = false)
private String accountClosedFlag;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,15 @@

import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.CrudRepository;
import uk.gov.justice.hmpps.prison.api.model.Assessment;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AssessmentEntry;
import uk.gov.justice.hmpps.prison.repository.jpa.model.OffenderAssessment;

import java.util.List;
import java.util.Optional;

public interface AssessmentRepository extends CrudRepository<AssessmentEntry, Long> {
@Query("SELECT question FROM AssessmentEntry question " +
"INNER JOIN question.parentAssessment questionSet INNER JOIN questionSet.parentAssessment type " +
"WHERE type.assessmentId = :assessmentId AND type.parentAssessment is null " +
"AND type.cellSharingAlertFlag = 'Y' AND questionSet.assessmentCode != 'COMPLETE' " +
"AND type.cellSharingAlertFlag = 'Y' AND questionSet.assessmentCode <> 'COMPLETE' " +
Copy link
Contributor Author

@mikehalmamoj mikehalmamoj May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Work around bug in latest Spring data which removed != alias allowed by Hibernate - fixed in version 3.2 which hasn't been released yet.

"ORDER BY question.listSeq ASC")
List<AssessmentEntry> findCsraQuestionsByAssessmentTypeIdOrderedByListSeq(Long assessmentId);
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,6 @@
@AllArgsConstructor
public class AgencyService {

private static final String ESTABLISHMENT_TYPE_DOMAIN = "ESTAB_TYPE";

private static final Comparator<Location> LOCATION_DESCRIPTION_COMPARATOR = Comparator.comparing(
Location::getDescription,
new AlphaNumericComparator());
Expand Down Expand Up @@ -376,16 +374,17 @@ public OffenderCell getCellAttributes(@NotNull final Long locationId) {

public AgencyEstablishmentTypes getEstablishmentTypes(final String agencyId) {
final var agency = agencyLocationRepository.findById(agencyId).orElseThrow(EntityNotFoundException.withId(agencyId));

return AgencyEstablishmentTypes.builder().agencyId(agencyId).establishmentTypes(agency.getEstablishmentTypes()
.stream()
.map(et -> {
final var establishment = referenceDomainService.getReferenceCodeByDomainAndCode(ESTABLISHMENT_TYPE_DOMAIN, et.getEstablishmentType(), false).orElseThrow(EntityNotFoundException.withMessage("Establishment type %s for agency %s not found.", et.getEstablishmentType(), agencyId));

return AgencyEstablishmentType.builder().code(establishment.getCode()).description(establishment.getDescription()).build();
})
.collect(toList()))
.build();
return AgencyEstablishmentTypes.builder().agencyId(agencyId).establishmentTypes(
agency.getEstablishmentTypes()
.stream()
.map(establishment -> {
return AgencyEstablishmentType.builder()
.code(establishment.getEstablishmentType().getCode())
.description(establishment.getEstablishmentType().getDescription())
.build();
})
.collect(toList()))
.build();
}

private OffenderCell transform(final AgencyInternalLocation cell, final boolean treatZeroOperationalCapacityAsNull) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,10 @@
import org.springframework.security.test.context.support.WithMockUser;
import org.springframework.test.context.ActiveProfiles;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyLocation;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyLocationEstablishment;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyLocationType;
import uk.gov.justice.hmpps.prison.security.AuthenticationFacade;
import uk.gov.justice.hmpps.prison.web.config.AuditorAwareImpl;

import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;
import static org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase.Replace.NONE;

Expand Down Expand Up @@ -86,9 +83,6 @@ public void createAnAgency() {
.description("A Test Agency")
.active(true)
.type(AgencyLocationType.PRISON_TYPE)
.establishmentTypes(List.of(AgencyLocationEstablishment.builder()
Copy link
Contributor Author

@mikehalmamoj mikehalmamoj May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't test by creating an AgencyLocationEstablishment first because it now has a child relationship with AgencyLocation. But the real createAgency endpoint doesn't allow establishment types to be created at the same time anyway so just removed from the test.

.agencyLocId("AgencyRepositoryTestTEST")
.establishmentType("IF").build()))
.build();

final var createdAgency = repository.save(newAgency);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,18 @@
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import uk.gov.justice.hmpps.prison.api.model.AgencyEstablishmentType;
import uk.gov.justice.hmpps.prison.api.model.AgencyEstablishmentTypes;
import uk.gov.justice.hmpps.prison.api.model.OffenderCell;
import uk.gov.justice.hmpps.prison.api.model.OffenderCellAttribute;
import uk.gov.justice.hmpps.prison.api.model.PrisonContactDetail;
import uk.gov.justice.hmpps.prison.api.model.ReferenceCode;
import uk.gov.justice.hmpps.prison.api.model.Telephone;
import uk.gov.justice.hmpps.prison.repository.AgencyRepository;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AddressType;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyEstablishmentType;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyInternalLocation;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyInternalLocationProfile;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyLocation;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyLocationEstablishment;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyLocationEstablishmentId;
import uk.gov.justice.hmpps.prison.repository.jpa.model.AgencyLocationType;
import uk.gov.justice.hmpps.prison.repository.jpa.model.City;
import uk.gov.justice.hmpps.prison.repository.jpa.model.Country;
Expand Down Expand Up @@ -219,20 +218,22 @@ public void shouldReturnCellWithAttributes_notFoundLivingUnit() {

@Test
public void shouldReturnAllEstablishmentTypesForMoorland() {
final var agencyLocation = AgencyLocation.builder()
.id("MDI")
.description("Moorland")
.type(AgencyLocationType.PRISON_TYPE)
.build();
final var establishmentType = new AgencyEstablishmentType("IM", "Closed Young Offender Institute (Male)");
when(agencyLocationRepository.findById("MDI")).thenReturn(Optional.of(AgencyLocation.builder()
.id("MDI")
.establishmentTypes(List.of(AgencyLocationEstablishment
.builder()
.agencyLocId("MDI")
.establishmentType("IM")
.build()))
.establishmentTypes(List.of(new AgencyLocationEstablishment(new AgencyLocationEstablishmentId(agencyLocation.getId(), establishmentType.getCode()), agencyLocation, establishmentType)))
.build()));

when(referenceDomainService.getReferenceCodeByDomainAndCode("ESTAB_TYPE", "IM", false)).thenReturn(Optional.of(ReferenceCode.builder().code("IM").description("Closed Young Offender Institute (Male)").build()));

final var establishmentTypes = service.getEstablishmentTypes("MDI");

assertThat(establishmentTypes).isEqualTo(AgencyEstablishmentTypes.builder().agencyId("MDI").establishmentTypes(List.of(AgencyEstablishmentType.builder().code("IM").description("Closed Young Offender Institute (Male)").build())).build());
assertThat(establishmentTypes.getAgencyId()).isEqualTo("MDI");
assertThat(establishmentTypes.getEstablishmentTypes()).extracting("code").containsExactly("IM");
assertThat(establishmentTypes.getEstablishmentTypes()).extracting("description").containsExactly("Closed Young Offender Institute (Male)");
}

@Test
Expand All @@ -242,22 +243,6 @@ public void shouldFailForEstablishmentTypesWhenAgencyNotFound() {
assertThatThrownBy(() -> service.getEstablishmentTypes("unknown")).isInstanceOf(EntityNotFoundException.class).hasMessage("Resource with id [unknown] not found.");
}

@Test
public void shouldFailForEstablishmentTypesWhenAgencyEstablishmentNotFound() {
Comment on lines -245 to -246
Copy link
Contributor Author

@mikehalmamoj mikehalmamoj May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no agency location establishments with an invalid establishment type code (DOMAIN=ESTAB_TYPE) in any Nomis env. So as we are now modelling the establishment type as a reference code this test is redundant.

when(agencyLocationRepository.findById("MDI")).thenReturn(Optional.of(AgencyLocation.builder()
.id("MDI")
.establishmentTypes(List.of(AgencyLocationEstablishment
.builder()
.agencyLocId("MDI")
.establishmentType("IM")
.build()))
.build()));

when(referenceDomainService.getReferenceCodeByDomainAndCode("ESTAB_TYPE", "IM", false)).thenReturn(Optional.empty());

assertThatThrownBy(() -> service.getEstablishmentTypes("MDI")).isInstanceOf(EntityNotFoundException.class).hasMessage("Establishment type IM for agency MDI not found.");
}

private List<PrisonContactDetail> buildPrisonContactDetailsList() {
return ImmutableList.of(
PrisonContactDetail.builder().agencyId("ABC")
Expand Down