search

adamcharnock / django-hordak

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

Trigger documentation #36

Closed coler-j closed 5 years ago

coler-j commented 5 years ago

Adds documentation for the use of database triggers and constraints. Please review for accuracy of my understanding of each trigger, and for the design choice to use triggers over Django signals.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 95.483% when pulling 82b5df568d30bdcd4c6e254fd1434b662ede0851 on coler-j:Trigger-Documentation into fba41012f67995710c788bdca08bbf949f28235e on adamcharnock:master.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 95.483% when pulling 82b5df568d30bdcd4c6e254fd1434b662ede0851 on coler-j:Trigger-Documentation into fba41012f67995710c788bdca08bbf949f28235e on adamcharnock:master.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 95.483% when pulling 82b5df568d30bdcd4c6e254fd1434b662ede0851 on coler-j:Trigger-Documentation into fba41012f67995710c788bdca08bbf949f28235e on adamcharnock:master.

adamcharnock commented 5 years ago

This is excellent @coler-j, thank you very much. I need to review this properly, but it is looking good from what I have seen so far. My hope is to get to this in the next couple of days. If you have not heard back by Wednesday please feel free to ping me.

coler-j commented 5 years ago

@adamcharnock thanks. As a note, you should create a task to use (eventually) Django 2.2s constraints https://docs.djangoproject.com/en/dev/ref/models/constraints/ to make some of this more transparent.

adamcharnock commented 5 years ago

This looks great @coler-j, thank you very much, I've merged the PR.

I'm somewhat tempted to link directly to the migration files rather than reproduce the code within the docs. However, I don't have a strong view either way and I'm happy to keep it as is as long as the docs manage to stay in sync with reality down the road.

Thank you for pointing out Django 2.2's constraints, I wasn't aware of that. I suspect that isn't something Hordak will do for some time because of a) as you point out, backwards compatibility, and b) Hordak would still need custom (postgres-specific) migrations for the stored procedures. I will keep an eye on it though.

Thank you again for the docs update. I'll happily review any other PRs should you be inclined, and feel free to contact me first if you want to run anything by me ahead of putting in the time.

coler-j commented 5 years ago

Yeah, I migrated a custom fork of Hordak to 2.2 and used the new constraints, but they only work for the simple constraints and not the procedures. So it simplifies a bit, but not a full solution.