search

adamcharnock / django-hordak

Double entry accounting in Django
http://django-hordak.readthedocs.io
MIT License
254 stars 58 forks source link

JSONField replacement for ArrayField #90

Closed nitsujri closed 1 year ago

nitsujri commented 1 year ago

JSONField can work as a repayment for ArrayField.

Key Differences

[EDIT] Turning this into a real PR.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 94.93% and project coverage change: +0.02 :tada:

Comparison is base (8c5453a) 92.92% compared to head (92468bd) 92.95%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #90 +/- ## ========================================== + Coverage 92.92% 92.95% +0.02% ========================================== Files 59 59 Lines 3747 3817 +70 Branches 236 245 +9 ========================================== + Hits 3482 3548 +66 - Misses 223 224 +1 - Partials 42 45 +3 ``` | [Impacted Files](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/90?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock) | Coverage Δ | | |---|---|---| | [hordak/tests/views/test\_accounts.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/90?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL3ZpZXdzL3Rlc3RfYWNjb3VudHMucHk=) | `91.50% <80.00%> (-0.57%)` | :arrow_down: | | [hordak/defaults.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/90?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL2RlZmF1bHRzLnB5) | `92.85% <85.71%> (-7.15%)` | :arrow_down: | | [hordak/forms/accounts.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/90?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL2Zvcm1zL2FjY291bnRzLnB5) | `94.11% <90.47%> (-2.55%)` | :arrow_down: | | [hordak/models/core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/90?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL21vZGVscy9jb3JlLnB5) | `98.23% <100.00%> (-0.01%)` | :arrow_down: | | [hordak/tests/forms/test\_accounts.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/90?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL2Zvcm1zL3Rlc3RfYWNjb3VudHMucHk=) | `100.00% <100.00%> (ø)` | | | [hordak/tests/models/test\_core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/90?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL21vZGVscy90ZXN0X2NvcmUucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

nitsujri commented 1 year ago

@PetrDlouhy this is ready for review.

Let me know if there's anything else you'd like to see.

PetrDlouhy commented 1 year ago

I tried to run tests on my project with this code and I have got quite a few errors. I am trying to debug them, but I am a bit lost now.

One of the errors was:

Traceback (most recent call last):
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
           ^^^^^^^^^^^^^^^^^^^^^^^^
psycopg2.errors.RaiseException: Destination account does not support currency USD
CONTEXT:  PL/pgSQL function check_leg_and_account_currency_match() line 13 at RAISE

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/test/testcases.py", line 418, in _setup_and_call
    self._post_teardown()
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/test/testcases.py", line 1267, in _post_teardown
    self._fixture_teardown()
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/test/testcases.py", line 1488, in _fixture_teardown
    connections[db_name].check_constraints()
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/db/backends/postgresql/base.py", line 314, in check_constraints
    cursor.execute("SET CONSTRAINTS ALL IMMEDIATE")
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/cachalot/monkey_patch.py", line 137, in inner
    return original(cursor, sql, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/sentry_sdk/integrations/django/__init__.py", line 582, in execute
    return real_execute(self, sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django_read_only/__init__.py", line 79, in blocker
    return execute(sql, params, many, context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 84, in _execute
    with self.db.wrap_database_errors:
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/home/nginx/.cache/pypoetry/virtualenvs/blenderkit-server-6gE1vKXj-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
           ^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.InternalError: Destination account does not support currency USD
CONTEXT:  PL/pgSQL function check_leg_and_account_currency_match() line 13 at RAISE

Which was caused by fixtures used in tests. I had to change

            "currencies": "[\"USD\"]",

to

            "currencies": ["USD"],

in the fixtures JSON. I am not sure if we can treat this somehow, but the error is very confusing and not pointing to the culprit of the problem.

When I fixed that error, I am getting this:

  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
           ^^^^^^^^^^^^^^^^^^^^^^^^
psycopg2.errors.RaiseException: Sum of transaction amounts in each currency must be 0. Currency USD has non-zero total 0.01, transaction id 413 from account "Blender foundation donations (BFA) ["USD"]", to account "User account for author@test.com (<NULL>) ["USD"]".
CONTEXT:  PL/pgSQL function check_leg() line 26 at RAISE

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/test/testcases.py", line 418, in _setup_and_call
    self._post_teardown()
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/test/testcases.py", line 1267, in _post_teardown
    self._fixture_teardown()
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/test/testcases.py", line 1488, in _fixture_teardown
    connections[db_name].check_constraints()
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/db/backends/postgresql/base.py", line 314, in check_constraints
    cursor.execute("SET CONSTRAINTS ALL IMMEDIATE")
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/cachalot/monkey_patch.py", line 137, in inner
    return original(cursor, sql, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/sentry_sdk/integrations/django/__init__.py", line 582, in execute
    return real_execute(self, sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
    return executor(sql, params, many, context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 84, in _execute
    with self.db.wrap_database_errors:
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/db/utils.py", line 91, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/home/petr/.cache/pypoetry/virtualenvs/blenderkit-server-eqrfB9dY-py3.11/lib/python3.11/site-packages/django/db/backends/utils.py", line 87, in _execute
    return self.cursor.execute(sql)
           ^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.InternalError: Sum of transaction amounts in each currency must be 0. Currency USD has non-zero total 0.01.
CONTEXT:  PL/pgSQL function check_leg() line 26 at RAISE

And the transaction amounts in the tests are mismatching. I am still trying to debug this, but didn't find what is causing this so far. I am creating 4 legged transactions with bulk_create, so that might contribute to the failed DB checks.

PetrDlouhy commented 1 year ago

Maybe we could add a little bit more information about the account to the check_leg_and_account_currency_match() when we are already making the migration. I think it would be nice if the user would see which account has the unsupported currency and maybe even supported currencies on that account. I tried similar thing for check_leg() in #93.

nitsujri commented 1 year ago

And the transaction amounts in the tests are mismatching. I am still trying to debug this, but didn't find what is causing this so far. I am creating 4 legged transactions with bulk_create, so that might contribute to the failed DB checks.

Hrm this PR should not have affected anything in the Leg or check_leg(), so I'm surprised there's an issue that this PR raises.

But, the fact that it's off by a penny suggests a rounding error. In the bulk_create, are you performing division/multiplication to create the Legs values? Are the values actually floats? Are there more decimal places than 2? like it's 2.9999999 and is a float?

If there's a spec that you can pass me happy to take a look.

Separately I added more info to the DB function https://github.com/adamcharnock/django-hordak/pull/90/commits/10ee645f7502a245f2ba8c7159e87688ada435ed. Let me know if you'd like to see more though.

PetrDlouhy commented 1 year ago

@nitsujri I am using django-hordak models with Decimal type for amounts. I have only set different defaults settings:

HORDAK_DECIMAL_PLACES = 8
HORDAK_MAX_DIGITS = 20

The difference is much bigger than could be caused by rounding errors:

AssertionError: Money('-15.00', 'USD') != Money('-14.85', 'USD')

I am still trying to find what is causing this.

PetrDlouhy commented 1 year ago

I have found, that at least part of the errors are not caused by this patch, but because the DECIMAL_PLACES settings was broken on the main branch. It didn't affect my code until now, because I kept my own set of migrations, which I deleted in order to test this.

I have fixed this in 6d13838572a2cd316cc7e5a289de2385e18d60e9 and added some tests in 6d13838572a2cd316cc7e5a289de2385e18d60e9. Please rebase to the current origin/master. I will try if this fixes all the failing tests on my project.

nitsujri commented 1 year ago

@PetrDlouhy ah makes sense. I've included master's updates.

PetrDlouhy commented 1 year ago

@nitsujri Everything seems very nice now, thank you. Did you look into the fixtures issue? Are we able to somehow show the user some pretty error message? Or at least mention it in the CHANGELOG?

And with the forms - do you think it will be no issue for django-hordak users? It could broke at least their testing routines, right? Is it possible to at least display some message if the value is in the old format?

nitsujri commented 1 year ago

Did you look into the fixtures issue? Are we able to somehow show the user some pretty error message? Or at least mention it in the CHANGELOG?

For the fixture issue, there's no good way around it (without creating a ton of hacks). It has to be a valid json.dumps-able-json object. I mentioned it in the changelog here, but happy to change it to make it more obvious.

And with the forms - do you think it will be no issue for django-hordak users? It could broke at least their testing routines, right? Is it possible to at least display some message if the value is in the old format?

Good idea! I added a much more verbose message when trying to use the old format with specs. The JSONField auto clears non-valid json so had to store it in a tmp variable.

Thanks for this - it was super good because it revealed the specs were setup incorrectly yet still passing.

PetrDlouhy commented 1 year ago

@nitsujri Thank you very much.

If it is not possible to catch the fixtures, I would explicitly mention the fixture case in the CHANGELOG with the solution I mentioned above. It took me some time to figure out the exact change, so lets make it easier for other users.

Other than that everything seems good. You would only need to rebase to current master, because I merged #95

nitsujri commented 1 year ago

Wow that took a bit for me to understand what was going on to get master to merge w/ specs passing.

Something for a different PR or discussion - I don't fully understand is what is HORDAK_INTERNAL_CURRENCY / INTERNAL_CURRENCY. It seems to be for Leg and managing fx, but how is that materially different than DEFAULT_CURRENCY? It's odd to have this internal currency that isn't in any documentation be the default for fx when the end developer is adding their own DEFAULT_CURRENCY. Leg doesn't matter as much because it always gets defined at creation via Money object anyway.

PetrDlouhy commented 1 year ago

@nitsujri Thank you very much. I think it is very good now. I am merging it.