Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase number of decimal places for account entry amounts #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

omarkhan
Copy link
Contributor

Needed for currencies with high denominations, like Indonesian Rupiah. This pull request also adds a missing migration to make account names unique, and makes urls.py compatible with django 1.9.

@g--
Copy link
Contributor

g-- commented Feb 5, 2016

Yes. Yes, this. 8 decimal places isn't enough.

I've been thinking of solving the problem more generally: use an integer field and store the number of places after the decimal field in a currency table.

In the mean time, your patch works!

@@ -247,7 +247,7 @@ def natural_key(self):

account = models.ForeignKey(Account, db_column='accid', related_name='entries')

amount = models.DecimalField(max_digits=8, decimal_places=2,
amount = models.DecimalField(max_digits=1000, decimal_places=2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about setting it to 16 instead of 1000? This would make it small enough to fit in a 64bit integer.
(64 bits is enough for a signed 18 digit number, minus 2 decimal places is 16.)

Is that enough?

Copy link
Contributor Author

@omarkhan omarkhan Feb 5, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to do the honours?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@g-- done, sorry for the delay

@omarkhan
Copy link
Contributor Author

omarkhan commented Apr 6, 2016

I have rebased this on top of master, but I can't seem to run the tests anymore as the settings file has been removed. How do you run the tests now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants