PDJB-300: Add invitation lists to property page#1402
Conversation
a222ca6 to
cb0fc58
Compare
| model.addAttribute("complianceDetails", propertyComplianceDetails) | ||
| model.addAttribute("complianceInfoTabId", COMPLIANCE_INFO_FRAGMENT) | ||
| model.addAttribute("isLandlordView", false) | ||
| model.addAttribute("isJointLandlordsEnabled", false) |
There was a problem hiding this comment.
this may change - right now assuming that LCs won't be able to view pending/expired invites though design to input on what should happen here
| th:text="#{propertyDetails.landlordDetails.invitations.pendingInvitations.sentOn(${invitation.sentDate})}"> | ||
| Sent on 18 October 2025 | ||
| </p> | ||
| <ul class="govuk-summary-list__actions-list"> |
There was a problem hiding this comment.
I'm suspicious of this - this is the simplest class that would give us the divider between actions but it's not a great use of GDS components. fine to keep as is or should we add our own copy of the relevant CSS?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, this feels unsafe to me. It might work now, but who knows what it'll be like when govuk-frontend updates another version or two. Owning our own copy has its own risks (we'll have to come and check this again for compatibility when we upgrade), but that feels a bit more explicit, at least.
9d349fa to
3f74d30
Compare
3f74d30 to
856ec34
Compare
| invitingLandlord = MockLandlordData.createLandlord() | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
the diff is weird here since the existing tests are now wrapped in a class
| val pendingInvitations = | ||
| jointLandlordInvitationService | ||
| .getPendingInvitations(propertyOwnership) | ||
| .map { InvitationViewModelBuilder.buildPendingViewModel(it) } | ||
| val expiredInvitations = | ||
| jointLandlordInvitationService | ||
| .getExpiredInvitations(propertyOwnership) | ||
| .map { InvitationViewModelBuilder.buildExpiredViewModel(it) } |
There was a problem hiding this comment.
This is going to mean two identical queries to the database, and a tiny race condition between them. I think it might be better for the service to have a single getPendingAndExpiredInvitations() that ultimately does a single queries and then partitions in memory.
It's a tiny issue, but also hopefully not hard to fix?
There was a problem hiding this comment.
good point, have changed this to partition
| invitations: | ||
| pendingInvitations: | ||
| heading: 'Pending invitations ({0})' | ||
| expiresIn: 'Expires in {0} days ({1})' |
There was a problem hiding this comment.
Do we need to worry about "1 days" and "0 days"?
There was a problem hiding this comment.
suspect we do, have added alternate copy for this
I don't think any of these other strings need a singular equivalent
| th:text="#{propertyDetails.landlordDetails.invitations.pendingInvitations.sentOn(${invitation.sentDate})}"> | ||
| Sent on 18 October 2025 | ||
| </p> | ||
| <ul class="govuk-summary-list__actions-list"> |
There was a problem hiding this comment.
Yeah, this feels unsafe to me. It might work now, but who knows what it'll be like when govuk-frontend updates another version or two. Owning our own copy has its own risks (we'll have to come and check this again for compatibility when we upgrade), but that feels a bit more explicit, at least.
| val expiryDate: Instant | ||
| get() = createdDate.plus(JOINT_LANDLORD_INVITATION_LIFETIME_IN_DAYS.toLong(), ChronoUnit.DAYS) | ||
|
|
||
| val daysUntilExpiry: Long | ||
| get() = ChronoUnit.DAYS.between(Instant.now(), expiryDate).coerceAtLeast(0) | ||
|
|
||
| val isExpired: Boolean | ||
| get() { | ||
| val dateTimeHelper = DateTimeHelper() | ||
|
|
||
| val expiresOnDate = | ||
| DateTimeHelper | ||
| .getDateInUK(createdDate.toKotlinInstant()) | ||
| .plus(DatePeriod(days = JOINT_LANDLORD_INVITATION_LIFETIME_IN_DAYS)) | ||
|
|
||
| return dateTimeHelper.getCurrentDateInUK() > expiresOnDate | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm just slightly nervous that we have some non-trivial date arithmetic here that all ought to agree. Might be worth some tests for this to check that they all behave as expected, and consistently with each other? Especially at common problem times, like near midnight or near a DST change.
There was a problem hiding this comment.
see #1401 which I'll rebase in when it's merged, there will be some stringent tests for this
this means we only query the database once avoiding a potential race condition
Ticket number
PDJB-300
Goal of change
adds lists of pending & expired invitations to the property details page
Description of main change(s)
adds logic to the property tab to show the invitations
adds database model to get invitations of a certain type
adds some useful methods to invitation entity
adds tests
Anything you'd like to highlight to the reviewer?
see comments
Checklist
Delete any that are not applicable, and add explanation below for any that are applicable but haven't been done
and any related functionality)
mention that here