Moving `Account.amount` to `Account.credit` & `Account.debit`

[adamcharnock]

trafficstars

I've had a go at this this morning, and you can see the the core changes here.

Most of the lines changed are tests.

Notes:

• It is a straightforward but far-reaching change
• I was hoping this would drop out a bunch of complexity, but it did not
• my_leg.amount is still available as a property on the Django model. It is now always a positive value.
• Can still use the amount argument when creating a Leg, a deprication warning will be shown.
• I've added amount_legacy (I.e. debits are negative) to the LegView, so it is still available at my_leg.view.amount_legacy
• I'm not against merging this for Hordak 2.0 or 3.0
• The more time I spend with this branch the more it does feel like a much better way to structure things. I'm rather in favour of merging this.

Remaining Tasks

• [x] Test against MySQL
• [x] Trigger to ensure that only credit or debit is set
  • [x] Test
• [x] Trigger to ensure that credit/debit is > 0
  • [x] Test

Follow-up work

I'll do this next in another PR

• Ensure 'balance()' functionality hasn't changed (is the sign of the return value still the same?)
• Implement 'accounting_balance()' if necessary
• Test data migration (related: #111)

[adamcharnock]

I would love the input of both @nitsujri and @PetrDlouhy on this.

@nitsujri - I know this isn't precisely what you asked for, but what are your thoughts?

@PetrDlouhy - How comfortable would be feel about deploying this kind of change (once stable) into your production environment?

[adamcharnock]

All tests now passing 🎉🎉🎉

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 84.57944% with 33 lines in your changes missing coverage. Please review.

Project coverage is 89.52%. Comparing base (9f11999) to head (ab11c27). Report is 7 commits behind head on master.

Files	Patch %	Lines
hordak/tests/models/test_core.py	73.95%	25 Missing :warning:
hordak/templates/hordak/transactions/leg_list.html	0.00%	4 Missing :warning:
...nagement/commands/create_benchmark_transactions.py	0.00%	2 Missing :warning:
hordak/forms/transactions.py	85.71%	0 Missing and 1 partial :warning:
...emplates/hordak/transactions/transaction_list.html	0.00%	1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
- Coverage   90.62%   89.52%   -1.10%     
==========================================
  Files          69       69              
  Lines        4414     4506      +92     
  Branches      294      324      +30     
==========================================
+ Hits         4000     4034      +34     
- Misses        363      415      +52     
- Partials       51       57       +6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[adamcharnock]

Hi @PetrDlouhy – I'm inclined to merge this sometime soon. If you wanted to have a dramatic "I object!" moment then please do so soon (other reasoned input also very welcome)

[nitsujri]

Would it be possible to remove the views? I believe the views are now no longer needed?

Specifically migrations against any column that is part of a view becomes a pain.

[PetrDlouhy]

Ať first glance this seems reasonable. I don't have any direct objection. I would have to find some time to test this with my project to tell more.

[adamcharnock]

@nitsujri

Leg.credit and debit max_digits needs to use HORDAK_MAX_DIGITS.

Done

get_queryset needs to swap signs based on account type.

Done

Remove views

Done

I'm a bit loathed to removed them as I think they may be useful, but I'm also willing to be found wrong on this. I've therefore added this warning to the docs for each view: Hordak's database views are still experimental and may change or be removed in a future version.

In the mean-time, my hope is that the new way of structuring sql-heavy migrations will make this problem less onerous.

[adamcharnock]

I've also made some additional changes, in particular moving many balance calculations into annotations. I think this should help performance a lot. Once this settles we can see if something like #76 (running totals) is still required.

I'll merge this soon unless anyone cries out. Let me know if you find any time for some testing @PetrDlouhy. I could always do an alpha-release if that helps at all.

[adamcharnock]

A pre-release is now out as 2.0.0a1 🎉🎉🎉🎉🎉

A huge thank you to @nitsujri and @PetrDlouhy for working through this with me.