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

Update licence query to just return SROC #43

Merged
merged 4 commits into from
Dec 8, 2022
Merged

Conversation

Cruikshanks
Copy link
Member

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

The original acceptance criteria for what licences to consider was anything flagged as 'include in supplementary billing'.

There are reasons why those that are void, deferred, revoked etc still need to be included. But our current scope is to generate SROC supplementary bill runs. So, on review, it makes sense that we only look at licences that have at least 1 SROC charge version.

This change updates our query to filter out anything that isn't SROC.

@Cruikshanks Cruikshanks added the enhancement New feature or request label Dec 7, 2022
@Cruikshanks Cruikshanks self-assigned this Dec 7, 2022
@Cruikshanks Cruikshanks added the do not merge Used for spikes and experiments label Dec 7, 2022
@Cruikshanks
Copy link
Member Author

Turns out our current support for working with schemas does not support relations. We have implemented this solution with success. But rather than pollute this PR we'll make those changes in a new PR and then rebase. Our changes should then build.

Cruikshanks added a commit that referenced this pull request Dec 7, 2022
https://eaflood.atlassian.net/browse/WATER-3829

Our current solution for working with schemas is to specify the schema name in the `tableName()` getter respone.

That has been working fine when just working in isolation. But our [first attempt](#43) to generate a query that uses relations is erroring.

```text
Licence.relationMappings.charge-versions: join: either `from` or `to` must point to the owner model table and the other one to the related table. It might be that specified table 'water.charge_versions` is not correct
```

Some digging later we found [this issue](Vincit/objection.js#85) which highlights our solution won't work

> Setting the schema to the tableName doesn't work. knex doesn't interpret the dot as a schema separator, but as a part of the table name.

So, this change implements the solution documented in that issue. We extend Objection's `QueryBuilder` with one that defaults all queries to use `withSchema()` taking the schema to use from a new model property `defaultSchema`.
Cruikshanks added a commit that referenced this pull request Dec 8, 2022
https://eaflood.atlassian.net/browse/WATER-3829

Our [first attempt](#43) to generate a query that uses relations is erroring.

```text
Licence.relationMappings.charge-versions: join: either `from` or `to` must point to the owner model table and the other one to the related table. It might be that specified table 'water.charge_versions` is not correct
```

Initially, we thought it was the way we're working with schemas as highlighted in [this issue](Vincit/objection.js#85). But actually, it was just a typo.

So, this change fixes that. It also corrects some other details of the relationships and adds some tests just to ensure they are working as expected.
https://eaflood.atlassian.net/browse/WATER-3787

The original acceptance criteria for what licences to consider was anything flagged as 'include in supplementary billing'.

There are reasons why those that are void, deferred, revoked etc still need to be included. But our current scope is to generate SROC supplementary bill runs. So, on review it makes sense that we only look at licences that have at least 1 SROC charge version.

This change updates our query to filter out anything that isn't SROC.
Get our failing tests implemented so we have an expectation to work to.
We aso needed to tweak the helper to allow us to support this scenario.
@Cruikshanks Cruikshanks removed the do not merge Used for spikes and experiments label Dec 8, 2022
@Cruikshanks Cruikshanks marked this pull request as ready for review December 8, 2022 08:51
@Cruikshanks Cruikshanks merged commit 27b7c89 into main Dec 8, 2022
@Cruikshanks Cruikshanks deleted the amend-licence-query branch December 8, 2022 23:04
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