PDJB-268: Add joint landlord invitation expiry scheduled task and notification email#1392
PDJB-268: Add joint landlord invitation expiry scheduled task and notification email#1392Bill-Haigh wants to merge 13 commits into
Conversation
| override fun run(args: ApplicationArguments?) { | ||
| println("Executing expire joint landlord invitations scheduled task") | ||
|
|
||
| jointLandlordInvitationExpiryService.expirePendingInvitations() |
There was a problem hiding this comment.
do we want any additional logging here?
There was a problem hiding this comment.
Yeah might be helpful for debugging purposes, maybe something like "{n} invitations deleted"? I don't think we need lots (although we've not had to do much debugging of scheduled tasks yet so may end up wanting more later!)
There was a problem hiding this comment.
yeah, I think if I was debugging this task it'd be useful to get some reference to which landlords it expired. we can put the invitation ID in the logs as is it non-PII
- Switch JointLandlordInvitationExpiryService impls to @PrsdbTaskService so they load in task-runner mode - Add the two flag-on/flag-off beans to PrsdbTaskApplicationTests expected-bean set - Add ExpireJointLandlordInvitationsTaskApplicationRunnerTests covering the runner method - Derive expiry days from JOINT_LANDLORD_INVITATION_LIFETIME_IN_HOURS and pass via a Notify placeholder instead of hardcoding 28 days - Reword expiry email subject to reflect the inviting landlord is the recipient
this is how we treat it in code / in wording
much more suitable to test the underlying logic than that a single method call makes another method call
since we can't automatically determine the task name
0321eb3 to
6f8c759
Compare
| override fun run(args: ApplicationArguments?) { | ||
| println("Executing expire joint landlord invitations scheduled task") | ||
|
|
||
| jointLandlordInvitationExpiryService.expirePendingInvitations() |
There was a problem hiding this comment.
Yeah might be helpful for debugging purposes, maybe something like "{n} invitations deleted"? I don't think we need lots (although we've not had to do much debugging of scheduled tasks yet so may end up wanting more later!)
| private set | ||
|
|
||
| @Column(nullable = false) | ||
| var expired: Boolean = false |
There was a problem hiding this comment.
I don't think this should be added to the database, I think we should calculate it from the createdDate when we need to check it.
I'm looking at adding
fun getInvitationHasExpired(invitation: JointLandlordInvitation): Boolean {
val dateTimeHelper = DateTimeHelper()
val expiresOnDate =
DateTimeHelper
.getDateInUK(invitation.createdDate.toKotlinInstant())
.plus(DatePeriod(days = JOINT_LANDLORD_INVITATION_LIFETIME_IN_DAYS))
return dateTimeHelper.getCurrentDateInUK() > expiresOnDate
to JointLandlordInvitationService in my current ticket, maybe something like that would work here?
There was a problem hiding this comment.
mm I see, the reason why I was thinking to do it this way is that it means that the 'your invite is expired' email and the expiration happening occur at the same time. though thinking again the email is more of a formality so agree let's 'expire' immediately and send the email later
There was a problem hiding this comment.
I'm thinking of having this method as a prop on JointLandlordInvitation itself as it's specific to the invitation model itself
There was a problem hiding this comment.
Adding it to JointLandlordService might be more consistent with what we do elsewhere (it's what I'm likely to do here #1409 if you don't beat me to it)
| val expiredInvitations = invitationRepository.findAllByExpiredFalseAndCreatedDateBefore(cutoff) | ||
|
|
||
| expiredInvitations.forEach { invitation -> | ||
| try { | ||
| sendExpiryEmailsForInvitation(invitation) | ||
| invitation.markAsExpired() | ||
| invitationRepository.save(invitation) | ||
| } catch (ex: PersistentEmailSendException) { | ||
| printFailureMessage(ex, invitation) | ||
| } catch (ex: TransientEmailSentException) { | ||
| printFailureMessage(ex, invitation) | ||
| } |
There was a problem hiding this comment.
I don't think we need to mark invitations as expired in the db, we should generally just check if they were created before the cutoff.
Although... I'm guessing we want to only send them one email to tell them that the invitation has expired? We did something similar with the incomplete properties reminder task - we added a new ReminderEmailSent table to record in the db which incomplete properties we had sent a reminder email about, and checked to make sure we didn't send another.
We might not need this to be as complicated though, maybe add an invitationExpiredEmailSent field to the invitation entity?
There was a problem hiding this comment.
I'll rework the flag to work more like this
This reverts commit 65e8c77.
makes it more clear that this flag is only there to stop expiry emails being sent again
JasminConterioSW
left a comment
There was a problem hiding this comment.
I've not looked at everything, just a few comments as I'm off tomorrow!
| private set | ||
|
|
||
| @Column(nullable = false) | ||
| var invitationExpiredEmailSent: Boolean = false |
There was a problem hiding this comment.
It might be useful to have this as a date that the email was sent (that's what we save in ReminderEmailSent) in case this is useful for tracking (it could be null if nothing has been sent).
But I don't think anyone has asked for this so it's probably fine!
| } | ||
| } | ||
|
|
||
| // TODO PDJB-260: include accepted joint landlords once that data model exists. |
| private set | ||
|
|
||
| @Column(nullable = false) | ||
| var expired: Boolean = false |
There was a problem hiding this comment.
Adding it to JointLandlordService might be more consistent with what we do elsewhere (it's what I'm likely to do here #1409 if you don't beat me to it)
| private val expiryEmailNotificationService: EmailNotificationService<JointLandlordInvitationExpiryEmail>, | ||
| private val absoluteUrlProvider: AbsoluteUrlProvider, | ||
| ) : JointLandlordInvitationExpiryService { | ||
| override fun expirePendingInvitations(): List<Long> { |
There was a problem hiding this comment.
This one will probably want to be renamed - expiring isn't something we do to an invitation, it just happens when they get old.
It's more like sendExpiredInvitationEmails (or you could include something saying that we record that too)
Ticket number
PDJB-268
Goal of change
Expire pending joint landlord invitations after 28 days and notify the inviting landlord by email.
Description of main change(s)
JOINT_LANDLORD_INVITATION_LIFETIME_IN_HOURSrather than hardcodedAbsoluteUrlProviderto build the absolute URL to the landlord-facing property details page (linked from the expiry email)JOINT_LANDLORDSfeature flag (no-op when the flag is off)@PrsdbTaskServiceand extendsPrsdbTaskApplicationTestsso the beans are exercised in task-runner modeAnything you'd like to highlight to the reviewer?
Checklist
/src/main/resources/emails/emailTemplates.json