-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
@@ -36,7 +36,6 @@ public class OffenderSubAccount { | |||
@Column(name = "TRUST_ACCOUNT_CODE", nullable = false, insertable = false, updatable = false) | |||
private Long accountCode; | |||
|
|||
@Id |
There was a problem hiding this comment.
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.
@@ -31,7 +31,6 @@ public class OffenderTrustAccount { | |||
@Column(name = "OFFENDER_ID", nullable = false, insertable = false, updatable = false) | |||
private Long offenderId; | |||
|
|||
@Id |
There was a problem hiding this comment.
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.
|
||
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' " + |
There was a problem hiding this comment.
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.
class AgencyEstablishmentType(code: String?, description: String?) : | ||
ReferenceCode(ESTABLISHMENT_TYPE, code, description) { |
There was a problem hiding this comment.
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.
@EmbeddedId | ||
val id: AgencyLocationEstablishmentId, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
.
@Test | ||
public void shouldFailForEstablishmentTypesWhenAgencyEstablishmentNotFound() { |
There was a problem hiding this comment.
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.
@@ -86,9 +83,6 @@ public void createAnAgency() { | |||
.description("A Test Agency") | |||
.active(true) | |||
.type(AgencyLocationType.PRISON_TYPE) | |||
.establishmentTypes(List.of(AgencyLocationEstablishment.builder() |
There was a problem hiding this comment.
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.
@@ -231,7 +230,7 @@ public void verifyCanViewSensitiveBookingInfo() { | |||
|
|||
@Test | |||
public void verifyCanViewSensitiveBookingInfo_systemUser() { | |||
when(authenticationFacade.isOverrideRole(any())).thenReturn(true); | |||
when(authenticationFacade.isOverrideRole(any(String[].class))).thenReturn(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mocks for vararg parameters changed in Mockito 5 so that you have to either match the correct number of args or instead use an array type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.