Adding `accounting_transfer_to` to match accounting cases.

[nitsujri]

Key Mentions

• The original thinking was Asset -> Income is how "money is made".

  • But most common situation (i.e. making money) should be the other way Income -> Asset.
  • The only time you do the original transfer is when giving income back (i.e. refunds) because you're "giving cash out".
  • Thusly, Asset -> Income should lower the balance of both accounts.

• The above also affects Payables -> Asset.

• The original specs said income -> asset and asset -> income resulted in +100, but that shouldn't be possible.

[Edit] This is a scary PR because results are different if you were doing particular transfers (asset -> income), so I invite lots of feedback.

[nitsujri]

Okay the specs pass now. Sorry about that.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.42 :tada:

Comparison is base (b9a4b65) 92.42% compared to head (4eacac6) 92.85%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #82 +/- ## ========================================== + Coverage 92.42% 92.85% +0.42% ========================================== Files 57 59 +2 Lines 3500 3708 +208 Branches 223 231 +8 ========================================== + Hits 3235 3443 +208 Misses 223 223 Partials 42 42 ``` | [Impacted Files](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock) | Coverage Δ | | |---|---|---| | [hordak/models/core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL21vZGVscy9jb3JlLnB5) | `98.22% <100.00%> (+0.10%)` | :arrow_up: | | [hordak/tests/models/test\_accounting\_tranfer\_to.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL21vZGVscy90ZXN0X2FjY291bnRpbmdfdHJhbmZlcl90by5weQ==) | `100.00% <100.00%> (ø)` | | | [hordak/tests/models/test\_core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?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%> (ø)` | | | [hordak/tests/test\_worked\_examples.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL3Rlc3Rfd29ya2VkX2V4YW1wbGVzLnB5) | `100.00% <100.00%> (ø)` | | | [hordak/tests/utilities/test\_currency.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL3V0aWxpdGllcy90ZXN0X2N1cnJlbmN5LnB5) | `99.58% <100.00%> (+<0.01%)` | :arrow_up: | | [hordak/tests/views/test\_transactions.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL3ZpZXdzL3Rlc3RfdHJhbnNhY3Rpb25zLnB5) | `92.67% <100.00%> (+0.15%)` | :arrow_up: | | [hordak/utilities/dreprecation.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/82?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3V0aWxpdGllcy9kcmVwcmVjYXRpb24ucHk=) | `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.

[PetrDlouhy]

@nitsujri Thank you very much for your contribution and making the PR pass all the tests.

I am maintaining this project, but I don't have deep understanding in accounting, so I would appreciate if somebody else will also review this patch.

I have looked at the code briefly and I have two questions:

1. Does the change of direction of transfer_to mean that the code is backward incompatible with current django-hordak implementations? If that is true, we would need to take much more conservative approach. I am not sure how to make this the easiest way, but I here are some steps as example:
   1. We release new version where the applications starts throwing DeprecationWarnings when transfer_to is used and new function like make_transfer_to (or something that indicates that the user is aware about the change of transfer direction) is introduced.
   2. We make the transfer_to function only a stub throwing some Deprecation error explaining the change.
   3. We remove the transfer_to function completely after some long time when we know, that all the users do know about the change. Maybe the step i. could be omitted if we introduce new major version of django-hordak and announce the change well enough.
2. Could you maybe expand your comments in the code a bit? It seems to me that you are removing longer explanatory comments and replacing them with things like Asset -> liability and not trading which doesn't explain it too much for me. Also Flipped direction for original seems to me that relates more to the current change than the code in general.

[nitsujri]

@PetrDlouhy thanks for the feedback and it is great.

Feedback Changes

• Restored transfer_to() and created accounting_transfer_to()
• Added deprecation warning to transfer_to()
• Comments that explain more + make more sense.

I agree on the conservative approach, let's not break userspace. I named it accounting_ as a forward thought towards a set of functions like accounting_is_credit and accounting_is_debit where this "namespace" adheres to the expected non-developer conventions.

If it helps I'm happy to hop on a call or share spreadsheets explaining the accounting concepts. While I'm quite new to this space, I've run this all by our in-house accountant.

Thoughts?

[PetrDlouhy]

@nitsujri Thank you very much for this great work.

I have noticed that this PR contains migrations update to fix tests due to external change in django-currencies. I have updated master with this fix and also I included tests in recent Django/Python versions. Can you please rebase to current master?

I also see changes of .gitignore and .pre-commit-config.yaml that seems unrelated to this PR. Can you please send them in separate PRs with some comment if they are relevant? I will merge them first. I would also see the test_core.py sign correction to be fixed separately.

[nitsujri]

@PetrDlouhy thanks, latest changes:

• Merged master
• Stripped out .gitignore and .precommit... moving to separate PR
• Stripped out test_core.py spec fixes, moving to separate PR.

[EDIT] I reread the comment and realized you probably meant test_transactions.py sign flip.

• Removing test_transactions.py spec changes.

But if I'm mistaken let me know.

[PetrDlouhy]

I meant the .replace("\xa0", ""). It seems like these are fixes for the local testing (those tests do fail on my local computer too). These appreciated improvements but they are not essential for this PR and it's tests to pass (if I am not mistaken).

EDIT: I looked at my local tests and I see that with Czech locale this fix would not be valid. My tests do output following:

AssertionError: '0 Account 1 [0,00\xa0€, 0,00\xa0£]' != '0 Account 1 [€\xa00.00, £\xa00.00]'
- 0 Account 1 [0,00 €, 0,00 £]
+ 0 Account 1 [€ 0.00, £ 0.00]

So the correct fix for this issue should involve somehow unifying the locale (or some other settings). Please include it in separate PR if you find suitable solution.

[PetrDlouhy]

@nitsujri Also now I noticed we would need at least some info in documentation regarding this change. Please add at least a note to the CHANGES.txt file about the deprecation in the new version (I will fix the date when I will release it).

[nitsujri]

@PetrDlouhy Done!

• Updated CHANGES.txt
• Added deprecation notes to .transfer_to()
• Removed local spec fixes - will bring in another PR.

[PetrDlouhy]

@nitsujri I have added some fixes to the master branch. Could you please rebase once more?

[nitsujri]

@PetrDlouhy mmmm i don't see anything new? latest i see is: https://github.com/adamcharnock/django-hordak/commit/b9a4b65961bb23ff3b0a56926d09f621ce094f97

[PetrDlouhy]

@nitsujri Yes, that matches, but it is weird, because I see This branch cannot be rebased due to conflicts here and the merge button is disabled.

[PetrDlouhy]

Oh, I can see what is happening. I have the option "Rebase and merge" as default and it is not able to do rebase with those merges with master branch. Would you be able to make an actual rebase to the master branch and force-push it here? Also I would prefer if the history would stay clean (without reversions of changes).

[nitsujri]

Can we do squash and merge? It's similar in that it goes down to one commit but it doesn't have to go through the rebasing dance.

I do agree there's a bunch of commits in here that we can simply leave out.

[PetrDlouhy]

@nitsujri OK, you are right. Squash and merge would be the easiest solution now.

I see some changes in hordak/tests/views/test_transactions.py that I commited to the master branch and then found a better solution. Could you please remove those?

Otherwise I am satisfied with the PR. Thank you for great work. I would be happy to merge it. I will only wait for a few days if somebody else wouldn't like to take a second look at this. @adamcharnock @rajeshr188 @lie-nielsen?

[nitsujri]

Done, removed the last bit.

[PetrDlouhy]

@nitsujri You will have to fix this after merging the other PRs.

[nitsujri]

@PetrDlouhy done!

[PetrDlouhy]

OK, nobody have taken interest to make additional review of this, so I am merging the code.