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

Deleted agreements shouldn't show in the setup tab #1220

Merged
merged 13 commits into from
Aug 2, 2024

Conversation

rvsiyad
Copy link
Contributor

@rvsiyad rvsiyad commented Jul 31, 2024

https://eaflood.atlassian.net/browse/WATER-4604

Currently, when an agreement is deleted within the licence set up tab, the agreement still remains. This is because the agreement is being 'soft deleted' rather than being removed from the database completely by adding the current time and date to the 'deleted_at' column in the licence agreements table. The logic for pulling licence agreements is missing instructions to only show licence agreements where the 'deleted_at' is null.

This PR is focused on not showing licence agreements in the licence set up tab where the licence has the 'deleted_at' column populated.

https://eaflood.atlassian.net/browse/WATER-4604

Currently, when an agreement is deleted within the licence set up tab, the agreement still remains.
This is because the agreement is being 'soft deleted' rather than being removed from the database
completely by adding the current time and date to the 'deleted_at' column in the licence agreements table.
The logic for pulling licence agreements is missing instructions to only show licence agreements where the 'deleted_at' is null.

This PR is focused on not showing licence agreements in the licence set up tab where the licence has the 'deleted_at' column populated.
@rvsiyad rvsiyad added the bug Something isn't working label Jul 31, 2024
@rvsiyad rvsiyad self-assigned this Jul 31, 2024
@rvsiyad rvsiyad marked this pull request as ready for review August 1, 2024 09:33
@rvsiyad rvsiyad requested a review from Jozzey August 1, 2024 10:47
@rvsiyad rvsiyad requested a review from Jozzey August 1, 2024 11:26
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

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

Spotted another issue you have with the tests on this. We are no longer adding await DatabaseSupport.clean() to any of our unit tests unless we really need to so this needs to be removed from your tests. When you do that your tests are going to break. This is because the financialAgreement id is hardcoded. There is no need to hardcode any of the FinancialAgreementHelper data, it will generate random data for this which is fine for these tests.

You also have the licenceRef hardcoded. You should let theLicenceAgreementHelper generate this and have it declared as a let rather than a const and then update the licenceRef variable when you generate the record with the helper.

Let me know if you want me to go through this with you.

@rvsiyad
Copy link
Contributor Author

rvsiyad commented Aug 1, 2024

Spotted another issue you have with the tests on this. We are no longer adding await DatabaseSupport.clean() to any of our unit tests unless we really need to so this needs to be removed from your tests. When you do that your tests are going to break. This is because the financialAgreement id is hardcoded. There is no need to hardcode any of the FinancialAgreementHelper data, it will generate random data for this which is fine for these tests.

You also have the licenceRef hardcoded. You should let theLicenceAgreementHelper generate this and have it declared as a let rather than a const and then update the licenceRef variable when you generate the record with the helper.

Let me know if you want me to go through this with you.

Think i've got it working fine now, let me know if i should change the names for the variables to have a Data prefix for clarity e.g. let financialAgreementData

@rvsiyad rvsiyad requested a review from Jozzey August 1, 2024 12:37
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

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

Sorry, Spotted another couple of things while on a call. That should be it this time though!

Up to you if you want to rename the variables to things like let financialAgreementData. I'm fine with them as they are.

test/services/licences/fetch-agreements.service.test.js Outdated Show resolved Hide resolved
test/services/licences/fetch-agreements.service.test.js Outdated Show resolved Hide resolved
@rvsiyad rvsiyad requested a review from Jozzey August 1, 2024 16:34
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

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

@rvsiyad rvsiyad merged commit 9a9e239 into main Aug 2, 2024
6 checks passed
@rvsiyad rvsiyad deleted the do-not-show-agreements-that-have-been-deleted branch August 2, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants