Code review comment for lp:~akretion-team/account-invoicing/70-add-invoice_fiscal_position_update
Revision history for this message
| Graeme Gellatly (gdgellatly) wrote | # |
review:
Approve
((code review, no test))
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
1b1ed1a
(Get the code!)
To the best of my knowledge a fiscal position is not dependent on a product and nor is an invoice line. It would be nice if this dependency wasn't there but recognise the difficulties in implementing consistently if an existing fiscal position has already remapped accounts and taxes before changing it. Also recognise that most invoices contain products.
So only comment would be to document the limitation or raise an error if none of the lines contain product_ids. Not sure how it would happen with a mix of both, but maybe it would pay to play it safe and raise an error if this case ever happens.
But otherwise LGTM.