Migrate legacy send bill run functionality #771
Merged
Conversation
This file contains 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
Cruikshanks
added a commit
to DEFRA/water-abstraction-service
that referenced
this pull request
Feb 28, 2024
https://eaflood.atlassian.net/browse/WATER-4345 We have been working on replacing the legacy SROC annual billing engine with one in [water-abstraction-system](https://github.com/DEFRA/water-abstraction-system). That engine is based on the one we built for supplementary billing which is also in **water-abstraction-system**. In a previous change, we updated this project to [Stop marking bill runs as ready when they are not](#2442). The refresh job was marking the bill run as **ready** and _then_ kicking off the process to refresh WRLS with data from the Charging Module API. That process entails making a request for every bill in the bill run to the CHA and then updating our local invoice and transactions with the result. You can't **SEND** a bill run until this step is complete. When you view the bill run all the values will be £0.00 and the download CSV will be working. Yet the legacy code still marks the bill run as **READY**. Users familiar with the service may know they need to leave it awhile for the numbers to refresh. What they probably don't know is the CHA is getting hammered and the service is busy with this process. Ideally they should wait for this process to complete before starting a new bill run. But there is no way to now that. So, they next bill run will take longer to complete and you increase the risk of an error. Anyway, that's why we made the change. But when we did we had no way of knowing when you send a bill run this legacy service _calls the refresh invoices job again_!! When you confirm and send a bill run it tells the [charging-module-api (CHA)](https://github.com/DEFRA/sroc-charging-module-api) to send the bill run to SOP. The CHA does a range of things behind the scenes one of which is to generate a transaction reference for each invoice. That's the only new piece of information we need to obtain and you can get it from a single request to `GET`` the bill run summary from the CHA. So, the fact we are reusing the refresh invoices job again is quite simply 🦇💩! Anyway, we're going to stop this insanity once and for all in [Migrate legacy send bill run functionality](DEFRA/water-abstraction-system#771). But till then we need to get it working again in this project. We broke it by having the job set the bill run status to `ready` at the end of the process. Prior to our change when the refresh job was kicked off it would immediately set the status to `ready` or `sent` then get on with the process. Our change means a the bill run is getting marked as `sent`, then reset back to `ready` after the update is complete. This ~hack~ change ensures a sent bill run stays sent!
Cruikshanks
added a commit
to DEFRA/water-abstraction-service
that referenced
this pull request
Feb 29, 2024
https://eaflood.atlassian.net/browse/WATER-4345 We have been working on replacing the legacy SROC annual billing engine with one in [water-abstraction-system](https://github.com/DEFRA/water-abstraction-system). That engine is based on the one we built for supplementary billing, also in **water-abstraction-system**. In a previous change, we updated this project to [Stop marking bill runs as ready when they are not](#2442). The refresh job was marking the bill run as **ready** and _then_ kicking off the process to refresh WRLS with data from the Charging Module API. That process entails making a request for every bill in the bill run to the CHA and then updating our local invoice and transactions with the result. You can't **SEND** a bill run until this step is complete. When you view the bill run all the values will be £0.00 and the download CSV will be working. Yet the legacy code still marks the bill run as **READY**. Users familiar with the service may know they need to leave it a while for the numbers to refresh. What they probably don't know is the CHA is getting hammered and the service is busy with this process. Ideally, they should wait for this process to complete before starting a new bill run. But there is no way to know that. So, the next bill run will take longer to complete and you increase the risk of an error. Anyway, that's why we made the change. But when we did we had no way of knowing when you send a bill run this legacy service _calls the refresh invoices job again_!! When you confirm and send a bill run it tells the [charging-module-api (CHA)](https://github.com/DEFRA/sroc-charging-module-api) to send the bill run to SOP. The CHA does a range of things behind the scenes one of which is to generate a transaction reference for each invoice. That's the only new piece of information we need to obtain and you can get it from a single request to `GET`` the bill run summary from the CHA. So, the fact we are reusing the refresh invoices job again is quite simply 🦇💩! Anyway, we're going to stop this insanity once and for all in [Migrate legacy send bill run functionality](DEFRA/water-abstraction-system#771). But till then we need to get it working again in this project. We broke it by having the job set the bill run status to `ready` at the end of the process. Before our change when the refresh job was kicked off, it would immediately set the status to `ready` or `sent` then get on with the process. Our change means the bill run is getting marked as `sent`, then reset back to `ready` after the update is complete. This ~hack~ change ensures a sent bill run stays sent!
Cruikshanks
force-pushed
the
migrate-send-functionality
branch
from
4db9e1c to
623d033
Compare
https://eaflood.atlassian.net/browse/WATER-4390 We have been working on replacing the legacy SROC annual billing engine. It is slow and prone to error leading to the team having to devote resources every year to helping the billing & data team get the annual bill runs through. Like with our supplementary billing engine we focused on the first part of the process; creating the bill run record then generating the transactions to send to the [charging-module-api (CHA)](https://github.com/DEFRA/sroc-charging-module-api). With that done we then pass control back to the legacy code to refresh the data and mark the bill run as **READY**. But our testing demonstrated that even with this part of the process replaced annual bill runs can still prove to be problematic due to the shoddy way the previous team implemented them. For example, when we send the POST request to the legacy `/refresh` endpoint it kicks off a process to request the 'generated' billing information from the CHA. This entails sending a request for every bill in the bill run and then updating our local data with the result. You can't **SEND** a bill run until this step is complete. When you view the bill run all the values will be £0.00 and the download CSV will be working. Yet the legacy code still marks the bill run as **READY**. Users familiar with the service may know they need to leave it awhile for the numbers to refresh. What they probably don't know is the CHA is getting hammered and the service is busy with this process. Ideally they should wait for this process to complete before starting a new bill run. But there is no way to now that. So, they next bill run will take longer to complete and you increase the risk of an error. We felt this situation was ridiculous so opted to amend the [water-abstraction-service](https://github.com/DEFRA/water-abstraction-service) to solve this. (See [Stop marking bill runs as ready when they are not](DEFRA/water-abstraction-service#2442)). It worked but inadvertently broke the legacy 'send a bill run' process (the final step in the bill run journey). When you confirm and send a bill run it tells the CHA to send the bill run to SOP. The CHA does a range of things behind the scenes one of which is to generate a transaction reference for each invoice. Again we need that information on our side but only that. This is where things go 🦇💩! The legacy service does this by calling the refresh invoices job again!! So, we're going through the whole process of sending a request for every invoice, updating values we already have including dealing with updating the transactions all over again. _All of this_ for something we can get making ***one*** request to the CHA (a request the legacy process already does prior to going through all the invoices again). We didn't know the legacy code reused this job. So, when we amended it to set the bill run status to **READY** at the end of the process we broke sending because in that scenario we are setting the status back from **SENT** to **READY**. We've implemented a 'hack' to get round this. But enough is enough! With this change we take over the 'send bill run process' and resolve this properly.
Cruikshanks
force-pushed
the
migrate-send-functionality
branch
from
623d033 to
3df96e8
Compare
|
The SonarCloud issues are all to do with line length (and we're only a few lines over the 20 limit). The key thing is this is now a working replacement that is sooo much faster than the legacy version. However, a key change from the legacy is we don't have the final Bill run sent page. We've opted to not include it because it enforces the need for the 'spinner' page, something we are trying to quash with a burning passion! We understand this is the end of the 'journey' for a bill run but we think we can make the argument to drop it and just redirect the user back to 'Bill Runs' which is where every other journey now finishes. What'll help though is if we can get the bill runs page auto-refreshing. So, we are parking this change until we can get an opportunity to look into that. |
To make it easier to follow in the busy controllers we now pair the GET and POST requests under a single describe() block. We also use a reusable `_options()` function.
We've changed our mind on this plan. We've got more pressing priorities than migrating the bill runs page. We don't want to leave to improvement festering so we are opting to use the same solution we were required to follow in Migrate confirm remove licence from bill run page. We'll redirect to the legacy processing page (the spinner!) and allow it to do what it would normally do once it sees the status change to |
Cruikshanks
added a commit
that referenced
this pull request
Mar 21, 2024
https://eaflood.atlassian.net/browse/WATER-4390 We recently [Migrate legacy send bill run functionality](#771) to our system. The PR details the reasons why (😱🤬) but suffice to say we are now responsible for the final stages of a bill run. We've been putting the change through its paces and our ever vigilant QA team have spotted something. When an SROC supplementary bill run is sent the supplementary billing flags on the licences in the bill get cleared. But the same cannot be said for PRESROC supplementary bills. Clearly there is something we have overlooked that the legacy code was doing that we're not. This change corrects the issue and gets those flags cleared!
Cruikshanks
added a commit
that referenced
this pull request
Mar 21, 2024
https://eaflood.atlassian.net/browse/WATER-4390 We recently [Migrate legacy send bill run functionality](#771) to our system. The PR details the reasons why (😱🤬) but suffice to say we are now responsible for the final stages of a bill run. We've been putting the change through its paces and our ever-vigilant QA team has spotted something. Once a supplementary bill run is sent the supplementary billing flags on the licences in the bill are supposed to be cleared. But this isn't happening in our version. After double-checking we've found where the legacy code was doing this. We need to replicate this behaviour in our version!
Cruikshanks
added a commit
that referenced
this pull request
Mar 22, 2024
https://eaflood.atlassian.net/browse/WATER-4390 In [Migrate legacy send bill run functionality](#771) we replaced the mad-as-a-box-of-frogs legacy send functionality with our own! Only our supposed leader of tech and keeper of quality @Cruikshanks completely missed adding unit tests for the primary `SubmitSendService`! 🤦 This change adds the missing tests.
Cruikshanks
added a commit
that referenced
this pull request
Mar 22, 2024
https://eaflood.atlassian.net/browse/WATER-4390 We recently [Migrate legacy send bill run functionality](#771) to our system. The PR details the reasons why (😱🤬) but suffice to say we are now responsible for the final stages of a bill run. We then spotted that we'd overlooked dealing with unflagging licences that were flagged for supplementary billing. That got sorted in [Fix supplementary flags not clearing after 'send'](#849). But then we started discussing the differences between the legacy approach (which we had to recreate) and our approach. We realised there are scenarios we are not covering. 1. User makes a change that flags a licence for supplementary billing 2. User generates a supplementary bill run 3. User then makes another change (adds a new charge version) which is waiting to be approved 4. User sends the bill run Currently, we'd remove the flag when the licence needs to be included in the next one. Also, if in step 3 they added a new charge version and then approved it, we'd have no workflow record to hang off. We'd be removing the flag when again the licence needs to be in the next supplementary bill run. This applies to licences the engine determines are unbilled (these get unflagged during bill run creation) and those that were billed (dealt with when the bill run gets sent). So, this change adds some of that legacy logic to our ow services to cover these scenarios.
Cruikshanks
added a commit
that referenced
this pull request
Mar 22, 2024
https://eaflood.atlassian.net/browse/WATER-4390 We recently [Migrate legacy send bill run functionality](#771) to our system. The PR details the reasons why (😱🤬) but suffice to say we are now responsible for the final stages of a bill run. We then spotted that we'd overlooked dealing with unflagging licences that were flagged for supplementary billing. That got sorted in [Fix supplementary flags not clearing after 'send'](#849). But then we started discussing the differences between the legacy approach (which we had to recreate) and our approach. We realised there are scenarios we are not covering. 1. User makes a change that flags a licence for supplementary billing 2. User generates a supplementary bill run 3. User then makes another change (adds a new charge version) which is waiting to be approved 4. User sends the bill run Currently, we'd remove the flag when the licence needs to be included in the next one. Also, if in step 3 they added a new charge version and then approved it, we'd have no workflow record to hang off. We'd be removing the flag when again the licence needs to be in the next supplementary bill run. This applies to licences the engine determines are unbilled (these get unflagged during bill run creation) and those that were billed (dealt with when the bill run gets sent). So, this change adds some of that legacy logic to our services to cover these scenarios.
Cruikshanks
added a commit
that referenced
this pull request
Mar 24, 2024
https://eaflood.atlassian.net/browse/WATER-4390 In [Migrate legacy send bill run functionality](#771) we replaced the mad-as-a-box-of-frogs legacy send functionality with our own! Only our supposed leader of tech and keeper of quality @Cruikshanks completely missed adding unit tests for the primary `SubmitSendBillRunService`! 🤦 This change adds the missing tests. --- Note the change in the `SubmitSendService`. We have been working under the impression that we can return early from an async function by not awaiting the final function. Whilst in theory this does appear to work it's probably not how we should be doing it. This is because for this service (`SubmitCancelService` is the other place we are following this pattern) we are purposefully throwing an error assuming the `try/catch` in the controller will see it and log it. When it came to trying to write a unit test for it we found we just kept blowing up [Hapi/Lab](https://hapi.dev/module/lab/). So, we then tried it for real, forcing `ChargingModuleViewBillRunRequest` to throw an error. We confirmed the controller's try/catch doesn't see the error and we caused 'system' to crash. The only way to catch the error is to chain `.catch()` directly onto the call. We were only intending to log the error so what we're doing is now fine. But it is worth noting and we may need to revisit our foreground/background pattern.
Cruikshanks
added a commit
that referenced
this pull request
Apr 3, 2024
https://eaflood.atlassian.net/browse/WATER-4390 In a recent change we [Migrated legacy send bill run functionality](#771) to our service. We though all was well but then recently spotted an issue. When we are 'sending' a bill run (telling the CHA to generate the transaction file for SOP that results in the bills being sent to customers) we are calling our `SubmitSendBillRunService` which in turn is calling `UnflagBilledLicencesService`. Only it shouldn't be doing this _all_ the time. It should only be calling that service if the bill run is supplementary. This change fixes the issue.
Cruikshanks
added a commit
that referenced
this pull request
Apr 3, 2024
https://eaflood.atlassian.net/browse/WATER-4390 In a recent change we [Migrated the legacy send bill run functionality](#771) to our service. We thought all was well but then recently spotted an issue. When we are 'sending' a bill run (telling the CHA to generate the transaction file for SOP that results in the bills being sent to customers) we are calling our `SubmitSendBillRunService` which in turn is calling `UnflagBilledLicencesService`. Only it shouldn't be doing this _all_ the time. It should only be calling that service if the bill run is supplementary. This change fixes the issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
https://eaflood.atlassian.net/browse/WATER-4390
We have been working on replacing the legacy SROC annual billing engine. It is slow and prone to error leading to the team having to devote resources every year to helping the billing & data team get the annual bill runs through.
Like with our supplementary billing engine, we focused on the first part of the process; creating the bill run record and then generating the transactions to send to the charging-module-api (CHA). With that done we then pass control back to the legacy code to refresh the data and mark the bill run as READY.
But our testing demonstrated that even with this part of the process replaced annual bill runs can still prove to be problematic due to the shoddy way the previous team implemented them. For example, when we send the POST request to the legacy
/refreshendpoint it kicks off a process to request the 'generated' billing information from the CHA. This entails sending a request for every bill in the bill run and then updating our local data with the result.You can't SEND a bill run until this step is complete. When you view the bill run all the values will be £0.00 and the download CSV will be working. Yet the legacy code still marks the bill run as READY. Users familiar with the service may know they need to leave it a while for the numbers to refresh. What they probably don't know is the CHA is getting hammered and the service is busy with this process.
Ideally, they should wait for this process to complete before starting a new bill run. But there is no way to know that. So, the next bill run will take longer to complete and you increase the risk of an error.
We felt this situation was ridiculous so opted to amend the water-abstraction-service to solve this. (See Stop marking bill runs as ready when they are not).
It worked but inadvertently broke the legacy 'send a bill run' process (the final step in the bill run journey). When you confirm and send a bill run it tells the CHA to send the bill run to SOP. The CHA does a range of things behind the scenes one of which is to generate a transaction reference for each invoice. Again we need that information on our side but only that.
This is where things go 🦇💩! The legacy service does this by calling the refresh invoices job again!! So, we're going through the whole process of sending a request for every invoice, updating values we already have including dealing with updating the transactions all over again. All of this for something we can get making one request to the CHA (a request the legacy process already does before going through all the invoices again).
We didn't know the legacy code reused this job. So, when we amended it to set the bill run status to READY at the end of the process we broke sending because in that scenario we are setting the status back from SENT to READY.
We've implemented a 'hack' to get around this. But enough is enough! With this change, we take over the 'send bill run process' and resolve this properly.