Fix display of credit transactions in bill views #522
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
https://eaflood.atlassian.net/browse/WATER-4131 In testing we've spotted that the display of credit values in certain screens has gone awry! In most cases we'll display `£150.25 credit` which is correct. But there are times, mainly in tables, where we need to show the signed version, for example `-£150.25`. A change to clean up our money formatters in `BasePresenter` broke this so now negative values in the licence summaries table and in the transaction tables total row are always being displayed as positive. This change updates the formatter to allow callers to specify whether a sign should be returned. It also fixes all the JSDoc comments we forgot to update when we made the change. Doh!
Ensure it asks for the signed version of the total for the licence. Update the tests to cover this.
Again, also ensure we have this covered in future in the tests.
In local testing the browser kept wrapping the total after the sign, for example
```
-
£150.25
```
So, we have updated the CSS for the cell class we apply to avoid this happening.
Specifically in the transactions screen.
We took the need to check for both the batch type being supplementary and the source being NALD from the legacy code. It's why even in `production` you'll see some bills display without the credits column. But in testing nothing seems to have blown up when we looked at bill licences connected to `nald` supplementary bill runs. So, we're removing the check from our logic.
Cruikshanks
added a commit
that referenced
this pull request
Nov 14, 2023
https://eaflood.atlassian.net/browse/WATER-4132 An issue was found with how credits were displayed that we sorted; [Fix display of credit transactions in bill views](#522). Or we thought we did. 😩 The bill information has a total which needs to be `£100.00 credit`. The transactions table beneath also has a total but it needs to display `-£100.00`. To build the single licence bill view we combine the output from `BillPresenter` with `ViewBillLicencePresenter`. It just so happens both return a `total:` property. 🤦 This means one is overriding the other leading to an issue on the single licence bill page. This change fixes it.
Cruikshanks
added a commit
that referenced
this pull request
Nov 14, 2023
https://eaflood.atlassian.net/browse/WATER-4132 An issue was found with how credits were displayed that we sorted; [Fix display of credit transactions in bill views](#522). Or we thought we did. 😩 The bill information has a total which needs to be `£100.00 credit`. The transactions table beneath also has a total but it needs to display `-£100.00`. To build the single licence bill view we combine the output from `BillPresenter` with `ViewBillLicencePresenter`. It just so happens that both return a `total:` property. 🤦 This means one is overriding the other leading to an issue on the single licence bill page. This change fixes it.
Cruikshanks
added a commit
that referenced
this pull request
Nov 14, 2023
https://eaflood.atlassian.net/browse/WATER-4131 When working on [Fix display of credit transactions in bill views](#522) after adding the missing `-` to credit values we found the UI was wrapping it _after_ the sign, for example ``` - £126.74 ``` We knew we could fix this by updating the CSS for the class to include `white-space: nowrap;`. The issue was where do we add it. We wasted a bunch of time trying to find the class `govuk-table__cell--total` in the design system code. Finally, we tracked it now to a custom class we'd added. This change is about not making that mistake again and trying to set a precedent. From now on, if we have to add out own custom class we should ***not*** prefix it with `govuk` so we can distinguish between our own code and that provided by GDS.
Cruikshanks
added a commit
that referenced
this pull request
Nov 15, 2023
https://eaflood.atlassian.net/browse/WATER-4131 When working on [Fix display of credit transactions in bill views](#522) after adding the missing `-` to credit values we found the UI was wrapping it _after_ the sign, for example ``` - £126.74 ``` We knew we could fix this by updating the CSS for the class to include `white-space: nowrap;`. The issue was where do we add it. We wasted a bunch of time trying to find the class `govuk-table__cell--total` in the design system code. Finally, we tracked it now to a custom class we'd added. This change is about not making that mistake again and trying to set a precedent. From now on, if we have to add a custom class we should ***not*** prefix it with `govuk` so we can distinguish between our code and that provided by GDS.
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-4131
In testing, we've spotted that the display of credit values on certain pages has gone awry!
The first issue is how credit values are displayed.
In most cases, we'll display
£150.25 creditwhich is correct. But there are times, mainly in tables, where we need to show the signed version, for example-£150.25. A change to clean up our money formatters inBasePresenterbroke this. So now negative values in the licence summaries table and the transaction table's total row are always displayed as positive.In this change, we update the formatter to allow callers to specify whether a sign should be returned. It also fixes all the JSDoc comments we forgot to update when we made the original change. Doh!
The second issue is about when we show credit values. During the initial build of the screens, we encountered some errors locally. We realised that bills sourced from NALD were missing data. When we checked the legacy code we found it only displayed credits when the bill run type was
supplementaryand its source waswrls. When we did the same the errors went away.During QA it's been spotted, rightly, that this means the views can appear inconsistent sometimes showing the credits column and sometimes not. Without knowing why we're doing it, it just looks 'broken'.
So, we dug deeper and realised that the key difference between bills from NALD and those created in WRLS is that the
invoiceValueandcreditNoteValuecolumns are empty for NALD ones. All we get is thenetAmount. Knowing that we can change how we determine the credit and debit total values and not cause an error. So, the second change is to always show the credits column for supplementary bills but to add a little 'dev magic' to handle how we calculate the totals!