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

adamcharnock · 2024-07-01T06:22:03Z

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

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: Applying hordak.0035_account_currencies_json... raises psycopg2.errors #111)

…count.credit & Account.debit

adamcharnock · 2024-07-01T09:58:32Z

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?

…0 adding two new currencies. This then causes migrations to be generated for currency fields with new default values.

adamcharnock · 2024-07-01T20:54:08Z

All tests now passing 🎉🎉🎉

nitsujri

Love everything about this. I won't be able to test this until August due to travels and deadlines, but in spirit, I 100% approve and happy to see it go out ahead of our team's direct testing.

hordak/models/core.py

nitsujri · 2024-07-02T20:50:43Z

hordak/tests/test_worked_examples.py

@@ -155,7 +155,7 @@ def test_utility_bill(self):

        # We receive the actual bill (we are moving a negative amount of money,
        # as this is an outgoing)
-        self.gas_payable.transfer_to(self.cash, Money(-300, "EUR"))
+        self.cash.transfer_to(self.gas_payable, Money(300, "EUR"))


This is incredible! I'm insanely excited to see this change right here.

I'm sure you already know this, but just stating it explicitly to be on the same page:

This is a breaking change for anyone doing RHS -> LHS, vice versa - using the current .transfer_to. Saw that accounting_transfer_to "effectively" replaces it which I think is 100% correct, just it's a significant change and warrants an "Upgrade to 2.0" document.

I'm very pleased to hear it! And yes, absolutely understood re upgrading. I've put these notes in the changelog. How does that seem to you?

codecov-commenter · 2024-07-15T17:41:47Z

⚠️ 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 ⚠️
hordak/templates/hordak/transactions/leg_list.html	0.00%	4 Missing ⚠️
...nagement/commands/create_benchmark_transactions.py	0.00%	2 Missing ⚠️
hordak/forms/transactions.py	85.71%	0 Missing and 1 partial ⚠️
...emplates/hordak/transactions/transaction_list.html	0.00%	1 Missing ⚠️

❗ 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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGES.txt

adamcharnock · 2024-07-18T09:36:34Z

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 · 2024-07-18T10:27:17Z

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 · 2024-07-18T13:21:50Z

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.

nitsujri

Was able to test it on our codebase. Overall looks great after internal modifications.

Recommended changes:

Remove views
Leg.credit and debit max_digits needs to use HORDAK_MAX_DIGITS.
get_queryset needs to swap signs based on account type.

hordak/migrations/0044_legview.py

hordak/admin.py

…ved balance(raw=...).

…n 1.17.0)

…tabase function get_balance()

…notations

…(in case multiple legs in the same transaction point to the same account)

adamcharnock · 2024-07-28T13:59:27Z

@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 · 2024-07-28T14:10:59Z

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.

nitsujri

debit column still holds negative value post migration - blocking issue.

nitsujri · 2024-07-30T03:34:10Z

hordak/migrations/0051_amount_to_debit_credit_data.py

+                    hordak_leg
+                SET
+                    credit = CASE WHEN amount > 0 THEN amount END,
+                    debit = CASE WHEN amount < 0 THEN amount END,


Tried migrating our staging system in preps for production. Found the debit column was holding a negative value. Same with the views.

I believe it should be flipped?

debit = CASE WHEN amount < 0 THEN -amount END,

Thank you for catching this! Fixed.

adamcharnock · 2024-08-29T10:23:06Z

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

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

Adding migrations and early work on code changes

f446ee6

adamcharnock marked this pull request as draft July 1, 2024 06:22

adamcharnock added 2 commits July 1, 2024 11:44

Code changes to support PR: Experiment in moving Account.amount to Ac…

2970b15

…count.credit & Account.debit

Test changes to support PR: Experiment in moving Account.amount to Ac…

2698f43

…count.credit & Account.debit

adamcharnock mentioned this pull request Jul 1, 2024

Change of architecture :) #44

Closed

adamcharnock added 5 commits July 1, 2024 15:38

Now supporting legacy amount argument to Leg, with deprecation warning

ca60528

Tests now pass for mysql

ee63c76

Setting mysql error code correctly

341bcfb

Adding tests and errors for invalid Leg credit/debit values

3cb4419

Merge branch 'master' into experiment/amount-to-debit-credit

a365d6a

adamcharnock marked this pull request as ready for review July 1, 2024 18:37

adamcharnock changed the title ~~Experiment in moving Account.amount to Account.credit & Account.debit~~ Moving Account.amount to Account.credit & Account.debit Jul 1, 2024

adamcharnock added 7 commits July 1, 2024 20:59

Now storing creation migrations for debugging

800b6e1

Now storing creation migrations for debugging

61d812d

Now storing creation migrations for debugging

41415ac

Now storing creation migrations for debugging

5e9a71b

Now storing creation migrations for debugging

b2c6575

Requiring djmoney>3 due to migration creation problem with djmoney 3.…

62cfa2a

…0 adding two new currencies. This then causes migrations to be generated for currency fields with new default values.

Formatting fix

bec5fdb

adamcharnock requested a review from PetrDlouhy July 1, 2024 20:53

adamcharnock changed the title ~~Moving Account.amount to Account.credit & Account.debit~~ Moving Account.amount to Account.credit & Account.debit Jul 2, 2024

nitsujri reviewed Jul 2, 2024

View reviewed changes

adamcharnock added 6 commits July 3, 2024 13:53

Fixing typo in deprecation warning

4ffc107

Expanding out the changelog

cc407b7

Making model choices available using the old syntax

a4e74f4

Fixing SQL error message

c0f03d5

Fixing postgres reverse migration issue

ef658e6

Migrations now correctly set number of decimal places for decimal fields

ab11c27

nitsujri reviewed Jul 18, 2024

View reviewed changes

CHANGES.txt Outdated Show resolved Hide resolved

Fixing error in changelog, thanks to @nitsujri for catching it

be40944

Including migration sql files in build

26f28e8

nitsujri reviewed Jul 23, 2024

View reviewed changes

hordak/migrations/0044_legview.py Show resolved Hide resolved

hordak/admin.py Outdated Show resolved Hide resolved

adamcharnock added 14 commits July 28, 2024 08:29

GET_BALANCE() now sets sign correctly (postgres)

c7e68e6

GET_BALANCE() now sets sign correctly mysql & postgres)

133c401

Updating orm-based balance calculations to calculate correct sign

e8c0088

Fixes to balance calculations given updates to sum_to_balance(). Remo…

6024b46

…ved balance(raw=...).

Renaming balance() to get_balance() (as per deprecation in 1.17.0)

41c07ac

Renaming all uses of balance() to get_balance() (as per deprecation i…

eeee60e

…n 1.17.0)

Updating admin to use the balance property now it is available

00d3cd2

Moving account_balance_after() and account_balance_before() to use da…

05b5ab7

…tabase function get_balance()

Moving account_balance_after() and account_balance_before() to use an…

c0ef2cc

…notations

get_balance() db function now uses leg_id rather than transcation ID …

ef8cf08

…(in case multiple legs in the same transaction point to the same account)

Simplifying tests as it was a bit flaky between postgres and mysql

e75321d

Docs improvements

9e025e3

Docs improvements

f419477

Adding warning in docs re views

4bf9c55

Work on changelog

06e1337

nitsujri suggested changes Jul 30, 2024

View reviewed changes

Fixing error in credit/debit data migration, #133

ea90bd0

adamcharnock merged commit da9a3c7 into master Aug 29, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adamcharnock commented Jul 1, 2024 •

edited

Loading

adamcharnock commented Jul 1, 2024

adamcharnock commented Jul 1, 2024

nitsujri left a comment

nitsujri Jul 2, 2024

adamcharnock Jul 3, 2024

codecov-commenter commented Jul 15, 2024

adamcharnock commented Jul 18, 2024

nitsujri commented Jul 18, 2024

PetrDlouhy commented Jul 18, 2024

nitsujri left a comment

adamcharnock commented Jul 28, 2024

adamcharnock commented Jul 28, 2024

nitsujri left a comment

nitsujri Jul 30, 2024

adamcharnock Aug 29, 2024

adamcharnock commented Aug 29, 2024

Moving Account.amount to Account.credit & Account.debit #133

Moving Account.amount to Account.credit & Account.debit #133

Conversation

adamcharnock commented Jul 1, 2024 • edited Loading

Notes:

Remaining Tasks

Follow-up work

adamcharnock commented Jul 1, 2024

adamcharnock commented Jul 1, 2024

nitsujri left a comment

Choose a reason for hiding this comment

nitsujri Jul 2, 2024

Choose a reason for hiding this comment

adamcharnock Jul 3, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jul 15, 2024

Codecov Report

adamcharnock commented Jul 18, 2024

nitsujri commented Jul 18, 2024

PetrDlouhy commented Jul 18, 2024

nitsujri left a comment

Choose a reason for hiding this comment

adamcharnock commented Jul 28, 2024

adamcharnock commented Jul 28, 2024

nitsujri left a comment

Choose a reason for hiding this comment

nitsujri Jul 30, 2024

Choose a reason for hiding this comment

adamcharnock Aug 29, 2024

Choose a reason for hiding this comment

adamcharnock commented Aug 29, 2024

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

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

adamcharnock commented Jul 1, 2024 •

edited

Loading