mbulat / plutus

A Ruby on Rails Engine which provides a double entry accounting system for your application
Other
733 stars 232 forks source link

Code question on why not use :sum and some comments #23

Closed salex closed 10 years ago

salex commented 10 years ago

First let me thank you for Plutus and once again proving that I know a lot less about Ruby and Rails than I though I did - it's been a good learning experience. I'm also not an accountant but have had to add 'Accounting type features' to several projects over the years. I was always looking for some type of accounting engine, but never found one. Plutus is close to what I was looking for, it does most of the accounting rules stuff, you just have to implement it to meet your situation.

In digging into the code and trying to figure out how things worked, I was wondering about scalability, as one of the messages on your google groups asked. There are a few areas where you are looping through associations to get a sum rather than using :sum. I figured there was a reason, but sum seems to work, except it fails certain tests. For kicks I replaced :balance in module AccountsExtension with:

    def balance
        sum(:amount)
    end

It seems to work in console queries and in running rails in fixture_rails_root, but fails two of your tests: should require the debit and credit amounts to cancel (FAILED - 1) and should require the debit and credit amounts to cancel even with fractions (FAILED - 2) in transaction_spec.rb. I'm not up on rspec and factory, so I have not been able to figure out why only those tests fail. My reason for trying sum was that your validations do not allow :amount to be nil, so why test for it in a loop. Sum seems a little faster, but I only have a few accounts and transactions.

Some comments on other requests/enhancements.

I posted this in issues rather than the google group, since it was more or less related to code. Sorry if I put it in the wrong place. but I was curious on why :sum fails the tests.

yragnos commented 10 years ago

Hi Salex, You mentioned multiple customers with separation. I think some form of apartmentization is needed at the top level, to ensure that no leaks occur by accident to a customers data. I am starting to look at some Gems that provide this as a starting point (milia is one).

mbulat commented 10 years ago

@salex sum only works on previously persisted ActiveRecord objects, since it does the sum in the database, otherwise it will return 0. In our case, we need to be able to loop through all the credit/debits prior to persistence in order to validate the transaction prior to saving.

re. account_code: this one has been requested a bunch and is on my todo list. (See issue #10) PRs are much appreciated :)

re. multiple customers: this one also comes up a lot, but I'm not sure of a way that would work for every developer's specific case and without making plutus overly complex. Rails engines are actually quite flexible though, and I think the correct solution is to extend the engine in your application directly. You could actually extend the Plutus::Account model in your app and add something like a belongs_to :customer and even a validation that requires all accounts in the transaction to have the same customer. See the rails guide for some examples on extending engines: http://guides.rubyonrails.org/engines.html#improving-engine-functionality This might be something I eventually outline in the README, or again, feel free to submit a PR if you come up with something nice

salex commented 10 years ago

Thanks for the explanation, I figured there was a reason, I guess that's why I got the two errors.

I actually played around with extending the Plutus engine before your reply. I'm just scheming now but I think It can be done with just over riding a few methods in the main application, mainly the class methods and creating accounts and transactions (which you have to do in the main application anyhow). I would have to add a tenet_id to the tables, but they'd only be used by the override method. I think??!!

salex commented 10 years ago

Don't want to open another issue to update on my multiple accounts approach.

I've made some progress in adding acts_as_tenant to a test main application. I've run into a couple problems that I posted on StackOverflow.

The main problem, which I've run into several times is trying things with Plutus::Amount often error out with uninitialized constant Transaction.

Don't know if you have any ideas on why.

2.1.0 :002 > Plutus::Amount.acts_as_tenant(:tenant)
NameError: uninitialized constant Transaction
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.2/lib/active_support/inflector/methods.rb:226:in `const_get'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.2/lib/active_support/inflector/methods.rb:226:in `block in constantize'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.2/lib/active_support/inflector/methods.rb:224:in `each'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.2/lib/active_support/inflector/methods.rb:224:in `inject'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.2/lib/active_support/inflector/methods.rb:224:in `constantize'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/activesupport-4.0.2/lib/active_support/core_ext/string/inflections.rb:66:in `constantize'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/acts_as_tenant-0.3.4/lib/acts_as_tenant/model_extensions.rb:71:in `block in acts_as_tenant'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/acts_as_tenant-0.3.4/lib/acts_as_tenant/model_extensions.rb:69:in `each'
  from /Users/salex/.rvm/gems/ruby-2.1.0/gems/acts_as_tenant-0.3.4/lib/acts_as_tenant/model_extensions.rb:69:in `acts_as_tenant'
  from (irb):2