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

TS-1416: Change to EndOfTenureDate validation so it can be on the StartOfTenureDate #31

Merged
merged 6 commits into from
May 13, 2024

Conversation

martapederiva
Copy link
Contributor

Link to JIRA ticket

https://hackney.atlassian.net/browse/TS-1416

Describe this PR

What is the problem we're trying to solve

The BTA tool needs to be able to end a booking on the same day it starts. After convos with Housing and Finance (see ticket) we are now proceeding with such changes.

What changes have we introduced

Changes to validation + test
Also minor refactoring of surrounding tests as date setting was not super solid

Follow up actions after merging PR

Updating package version in consuming API + listener (for coherence)

@martapederiva martapederiva requested review from LBHMKeyworth, LBHSPreston and a team as code owners May 8, 2024 17:07
Copy link
Contributor

@Duslerke Duslerke left a comment

Choose a reason for hiding this comment

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

The PR changes in here are fine, but I think I've missed what's not in this PR.

This could be an issue:

When(tenure => tenure.EndOfTenureDate != null, () =>
{
RuleFor(x => x.StartOfTenureDate)
.LessThan(x => x.EndOfTenureDate)
.WithErrorCode(ErrorCodes.TenureEndDate);
});

Duslerke
Duslerke previously approved these changes May 9, 2024
Copy link
Contributor

@Duslerke Duslerke left a comment

Choose a reason for hiding this comment

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

Seems like it will be alright.

The effect of this is relaxed restriction on start/end date validation, which shouldn't break any API functionality contract-wise.

@Duslerke
Copy link
Contributor

Duslerke commented May 9, 2024

Of course, some tests on the Tenure API will need to be updated as well to match this new behaviour.
Tests like:
https://github.com/LBHackney-IT/tenure-api/blob/43446370f9307fc93159324daf0cea19a4b1e673/TenureInformationApi.Tests/V1/Controllers/TenureInformationControllerTests.cs#L311

Copy link
Contributor

@Duslerke Duslerke left a comment

Choose a reason for hiding this comment

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

The change in error code is fine. Just a string change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants