Details
- Reviewers
vanmeeuwen - Group Reviewers
Restricted Project - Commits
- rKd14052d8c047: VAT modes
Diff Detail
- Repository
- rK kolab
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Two other points:
- Instead of storing the vat with the payment I would store a foreign key to the vat entry with the payment (that said, I don't have a case where the current implementation doesn't work).
- Not a blocker, but we'll likely require some UI changes as well for the payment process.
| src/app/Documents/Receipt.php | ||
|---|---|---|
| 134 | Not related, but is this expected in here?. | |
| 157 | Should the rate be hardcoded here? | |
| 158 | Use ceil here as well. I would create a a function in Payment to calculate the amount (and potentially not store base_amount at all, unless necessary). | |
| 181 | use ceil | |
| src/app/Http/Controllers/API/V4/Admin/WalletsController.php | ||
| 87 | I would just call the methods instead of assembling a string. | |
| src/app/Http/Controllers/API/V4/PaymentsController.php | ||
| 511 | I think we should be using ceil instead of round, so we always cover at least the tax, | |
| src/app/Payment.php | ||
| 12 | How about amount_after_taxes, or taxed_amount, or wallet_amount? | |
| src/app/Providers/Payment/Coinbase.php | ||
| 355 | I would prefer passing the $payment to credit, and then extract the correct amount there. | |
| src/app/Providers/PaymentProvider.php | ||
| 211–212 | Rather call the actual method instead of assembling a string that is then invoked as a method, IMO. | |
| src/config/app.php | ||
| 250 | I would prefer a string "taxed_at_source", "not_taxed" (or something like that). | |
As for the need for base_amount... We could get rid of it if we never support change of app.vat.mode on a working system. We could also store the mode instead. Also, having a calculated value that can be "directly" used for balance-related operations is handy. E.g. for Income chart we don't have to calculate the value in a sql query.
| src/app/Documents/Receipt.php | ||
|---|---|---|
| 134 | Fake data is here so we can produce a test output of a receipt, using template:render command | |
| src/app/Providers/Payment/Coinbase.php | ||
| 355 | Accepting \App\Payment in these Wallet methods might be a good idea. | |
Instead of storing the vat with the payment I would store a foreign key to the vat entry with the payment (that said, I don't have a case where the current implementation doesn't work).
One thing that would currently be difficult is to show in the UI which tax rate was applied beyond the percentage. I think we should preserve the link to the db entry so we can e.g. show in some UI that the the tax rate from switzerland, active in 2023 was used for this payment. If we only have the percentage we loose the ability to do this (cleanly).
I think it should be fine to change the mode. We store the applied tax with the existing payment, and never change that, so changing the mode should only apply to new payments. That having it calculated for SQL reasons is handy makes sense.
- Payments: tax_rate -> tax_rate_id, base_amount -> credit_amount
- Fix some code errors