search

system76 / beansbooks

A revolutionary cloud accounting platform designed for small and medium businesses.
129 stars 36 forks source link

Allow up to 3 decimals in quantity for Expenses, Orders, Sales and Invo… #239

Closed Elyrith closed 8 years ago

Elyrith commented 9 years ago

This is a first attempt and I'm very new. I tried to make sure I got all the changes from #120, but there are a few things I can't test:

  1. The database modifications in application/classes/beans/setup/update/v/1/4/1.php (because I just made the change myself without applying it as a patch), and...
  2. The validation code in application/classes/beans/customer.php (because I don't totally understand when this is supposed to be used since the Javascript manages the decimal places.)
  3. I don't even completely understand what these do, but I figured out they needed to be changed and what to change:
    • /application/classes/beans/vendor/expense/create.php and update.php
    • /application/classes/beans/vendor/purchase/create.php and update.php
    • /application/classes/beans/customer/sale/create.php and update.php

As for my testing, this seems to work for Customers > Sales Orders, Customers > Invoices, Vendors > Expenses, Vendors > Purchase Orders.

This is an attempt to complete issue #120.

funnylookinhat commented 9 years ago

Hi @Elyrith -

Thanks for sending this in! I'm going to do quite a bit of testing before merging; any change that can affect the numbers in the journal needs as much scrutiny as we can muster. Give me the weekend and I'll get back to you though.

Thanks again!

Elyrith commented 9 years ago

I already found an error, but I think I fixed it: The previous length of form_lines.quantity was "10, 0", so since we're adding 3 decimals it should now be "13, 3".

(I should mention that I'm a very amateur accountant, so don't skimp on the scrutiny. Assume nothing.)

cdp1337 commented 8 years ago

This is similar to some work I've already done, check/diff my repo if you feel so inclined. https://github.com/cdp1337/beansbooks/tree/eval-master

funnylookinhat commented 8 years ago

Hey @Elyrith -

This PR looks pretty good! I've made a few comments on it for some minor changes, but once I have those and the SQL file mentioned below, we should be able to merge.

One thing that is missing from the PR is changing this line in the installation SQL: https://github.com/system76/beansbooks/blob/master/install_files/database_structure.sql#L118

We treat that SQL as the canonical source for the database ( i.e. as it should be with each released version ), and then we use the update scripts to tick everything up to that point should someone update their code. Go ahead and change the line in that to be a decimal(13,3) like you have in your update script.

One other thing I want to point out that you did correctly so that anyone watching can learn. You created the update script for the next patch release version without bumping the actual stored version in beans.php. This is correct for a few reasons: 1) It allows us to pull and test the PR by individually running the update. 2) We can merge more changes into that update script if we decide more features should hit this release. 3) If we decide the next release is actually a minor release ( or even a major release ), then it's easier for us to update the develop branch with that info and test accordingly.

So thank you!

Elyrith commented 8 years ago

All changes made as per your details.

funnylookinhat commented 8 years ago

Awesome - thanks! I'll test again just to be safe and then merge if we're good.

funnylookinhat commented 8 years ago

This looks good! Thanks for the pull request! We've got a few more minor fixes to move into the develop branch, but after that we'll stage this as a minor version release ( v1.5 ) - as the quantities is a significant feature improvement - so thank you!

Elyrith commented 8 years ago

I'm glad I can help! Thank YOU for this amazing piece of software!