Org: Round invoice to 0.05 before grand total#2570
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
The ==7.16 pin was a local install workaround and doesn't belong in the ticket.
Running darker over these files made black re-wrap every decorator it touched, turning a three-word label rename into a 179 line diff. The only actual change is 'Value Added Tax' -> 'Prices'.
The rounding call had been wrapped in a stray parenthesized block with a trailing conditional. An early return for the missing resource reads better.
apply_price_rounding lives in onegov.org.utils, so testing it from tests.onegov.pay made the lower layer import the higher one. round_to_five_rappen stays with onegov.pay.
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Overall this seems fine. However you forgot to apply rounding to form and directory entry submissions.
I think the design could also use some refactoring, so we're less likely to forget to apply rounding. The most robust idea I have is introducing a new helper type InvoiceMeta that is aware of the potential existence of a rounding base and manual invoice items on an existing invoice and return that new helper type in a lot of places instead of list[InvoiceItemMeta].
That way we can standardize a lot of the logic and bake it into this helper, instead of having to remember to call a utility function in half a dozen different places.
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks good, I just have a few more follow-up suggestions.
| new_item_metas = self.invoice_items(request) | ||
| if not new_item_metas: | ||
| invoice_meta = self.invoice_items(request, rounding_base) | ||
| if not invoice_meta.items: |
There was a problem hiding this comment.
I just realized this part is a little controversial, since we delete the invoice as soon as there are no automatic invoice items, regardless of whether are any manual items.
On the other hand, it's not possible to add manual invoice items when there is nothing to invoice, so maybe this is actually good for consistency. Otherwise we would have to consider always allowing manually adding invoice items to a ticket. Which could be useful, but for now I think we'll just leave it like this, until someone has an actual use-case..
Co-authored-by: David Salvisberg <dave@daverball.com>
|
I think I have everything now |
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks good, there's one minor footgun left with InvoiceMeta.total_vat we probably should address, but it won't matter until we allow setting a VAT rate on manual invoice items.
2 participants
Please fill in the commit message below and work through the checklist. You can delete parts that are not needed, e.g. the optional description, the link to a ticket or irrelevant options of the checklist.
Commit message
Org: Round invoice to 0.05 before grand total
TYPE: Feature
LINK: OGC-2957
Checklist