-
Notifications
You must be signed in to change notification settings - Fork 1
PDJB-302: Remove (hide) expired invites #1408
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
Open
samyou-softwire
wants to merge
3
commits into
feat/PDJB-300-property-record-landlord-invite-details
Choose a base branch
from
feat/PDJB-302-remove-expired-jl-invites
base: feat/PDJB-300-property-record-landlord-invite-details
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 7 additions & 0 deletions
7
...n/kotlin/uk/gov/communities/prsdb/webapp/constants/enums/JointLandlordInvitationStatus.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package uk.gov.communities.prsdb.webapp.constants.enums | ||
|
|
||
| enum class JointLandlordInvitationStatus { | ||
| PENDING, | ||
| EXPIRED, | ||
| HIDDEN, | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 change: 1 addition & 0 deletions
1
src/main/resources/db/migrations/V1_29_0__add_is_hidden_to_joint_landlord_invitation.sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ALTER TABLE joint_landlord_invitation ADD COLUMN is_hidden BOOLEAN NOT NULL DEFAULT FALSE; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
16 changes: 8 additions & 8 deletions
16
...st/kotlin/uk/gov/communities/prsdb/webapp/database/entity/JointLandlordInvitationTests.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,35 +1,35 @@ | ||
| package uk.gov.communities.prsdb.webapp.database.entity | ||
|
|
||
| import org.junit.jupiter.api.Assertions.assertFalse | ||
| import org.junit.jupiter.api.Assertions.assertTrue | ||
| import org.junit.jupiter.api.Test | ||
| import uk.gov.communities.prsdb.webapp.constants.JOINT_LANDLORD_INVITATION_LIFETIME_IN_DAYS | ||
| import uk.gov.communities.prsdb.webapp.constants.enums.JointLandlordInvitationStatus | ||
| import uk.gov.communities.prsdb.webapp.testHelpers.mockObjects.MockJointLandlordData | ||
| import java.time.Instant | ||
| import java.time.temporal.ChronoUnit | ||
| import kotlin.test.assertEquals | ||
|
|
||
| class JointLandlordInvitationTests { | ||
| @Test | ||
| fun `isExpired returns false when the current day is earlier than the expiry date`() { | ||
| fun `status returns PENDING when the current day is earlier than the expiry date`() { | ||
| val createdDate = Instant.now().minus((JOINT_LANDLORD_INVITATION_LIFETIME_IN_DAYS - 1).toLong(), ChronoUnit.DAYS) | ||
| val invitation = MockJointLandlordData.createJointLandlordInvitation(createdDate = createdDate) | ||
|
|
||
| assertFalse(invitation.isExpired) | ||
| assertEquals(invitation.status, JointLandlordInvitationStatus.PENDING) | ||
| } | ||
|
|
||
| @Test | ||
| fun `isExpired returns false when the current day equals the expiry date`() { | ||
| fun `status returns PENDING when the current day equals the expiry date`() { | ||
| val createdDate = Instant.now().minus(JOINT_LANDLORD_INVITATION_LIFETIME_IN_DAYS.toLong(), ChronoUnit.DAYS) | ||
| val invitation = MockJointLandlordData.createJointLandlordInvitation(createdDate = createdDate) | ||
|
|
||
| assertFalse(invitation.isExpired) | ||
| assertEquals(invitation.status, JointLandlordInvitationStatus.PENDING) | ||
| } | ||
|
|
||
| @Test | ||
| fun `isExpired returns true when the current day is later than the expiry date`() { | ||
| fun `status returns EXPIRED when the current day is later than the expiry date`() { | ||
| val createdDate = Instant.now().minus((JOINT_LANDLORD_INVITATION_LIFETIME_IN_DAYS + 1).toLong(), ChronoUnit.DAYS) | ||
| val invitation = MockJointLandlordData.createJointLandlordInvitation(createdDate = createdDate) | ||
|
|
||
| assertTrue(invitation.isExpired) | ||
| assertEquals(invitation.status, JointLandlordInvitationStatus.EXPIRED) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
copilot wanted this a POST but I didn't really see the point of wrapping the link in a form to make that work
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.
Sorry, but I agree with Copilot here - although it is really an issue with the design (we've explained that state changes = button, and that GDS really doesn't like buttons styled as links...). As it stands though if a user clicks on that link twice in quick succession (before the first response has arrived) they'll get a 404 page, because browsers don't protect against accidental repeated GET requests. The security side of things is less important because hiding an already expired invite is low stakes - but just having the code smell of GETs that make state changes as a precedent risks others copying it in higher stakes cases where we want the protection of a CSRF token.
My suggestion would be that we merge as-is but we a) create a ticket to add a confirmation page, b) put a bit TODO & health warning here about it being a GET when it should be a POST, just in case that change never materialises, c) ask the designers for a confirmation page (and maybe suggest this should be a 'delete' or 'cancel' rather than a 'hide' action to make clear that it's destructive).
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.
Tbf have spotted this won't trigger a 404, we'll just set hidden = true again even though it was already true. The wider point still stands though