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

Determine if licence is 'billed' #101

Merged
merged 15 commits into from
Jan 31, 2023
Merged

Conversation

Jozzey
Copy link
Contributor

@Jozzey Jozzey commented Jan 27, 2023

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

We need to know whether a licence has already been ‘billed’ in the current period. If so we’ll have to ensure it’s credited as part of the bill run before we recalculate the debit.

For each licence that is a candidate for billing, we need to determine if it was included in a billing_batch for the current period.

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

We need to know whether a licence has already been ‘billed’ in the current period. If so we’ll have to ensure it’s credited as part of the bill run before we recalculate the debit.

For each licence that is a candidate for billing, we need to determine if it was included in a billing_batch for the current period.
@Jozzey Jozzey added the enhancement New feature or request label Jan 27, 2023
@Jozzey Jozzey self-assigned this Jan 27, 2023
@Jozzey Jozzey marked this pull request as ready for review January 30, 2023 12:53
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

I'm questioning the change in the default status used by the BillingBatchHelper. But if there is good reason for it I'm not going to argue!

But I have made some suggested changes to try and get the unit test descriptions 'flowing' again.

@@ -24,7 +24,7 @@ describe('Check Live Bill Run service', () => {
describe('when an sroc supplementary bill run exists for this region and financial year', () => {
describe('with a status considered to be "live"', () => {
beforeEach(async () => {
billRun = await BillingBatchHelper.add()
billRun = await BillingBatchHelper.add({ status: 'queued' })
Copy link
Member

Choose a reason for hiding this comment

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

Going to comment here but also applies to line 39 in the same file and the change to test/support/helpers/water/billing-batch.helper.js.

queued is the default state for a new BillingBatch record and that is why the helper uses that as its default. Is there a reason why we have switched the default to sent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I defaulted it to sent as there was only 2 tests that used that default value queued. As the majority of the tests use the status of sent I found the helper to be much more helpful if this was the default.

Copy link
Member

Choose a reason for hiding this comment

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

I think I'd still argue that the default matches with what would happen when the record is newly created and that we then apply the status we need in the tests.

It might be we have more tests that use 'sent' now. But that might change in the future. Plus, the status being sent is important in these tests whereas it might be meaningless in others. So, IMHO it's better to set the state where it matters. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You've talked me round 😁 I shall change the default back to queued

test/support/helpers/water/billing-batch.helper.js Outdated Show resolved Hide resolved
Jozzey and others added 8 commits January 30, 2023 16:52
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
@Jozzey Jozzey requested a review from Cruikshanks January 30, 2023 17:00
@Jozzey Jozzey merged commit 1a52bf3 into main Jan 31, 2023
@Jozzey Jozzey deleted the determine-if-licence-is-billed branch January 31, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants