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.

Sign up for GitHub

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

Add option to ignore financial transaction when calculating the balance #817

Merged
merged 1 commit into from
Feb 17, 2021
Merged

Add option to ignore financial transaction when calculating the balance #817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions app/models/financial_transaction_class.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
class FinancialTransactionClass < ApplicationRecord
has_many :financial_transaction_types, dependent: :destroy
has_many :supplier_category, dependent: :restrict_with_exception
has_many :financial_transactions, through: :financial_transaction_types
has_many :ordergroups, -> { distinct }, through: :financial_transactions

validates :name, presence: true
validates_uniqueness_of :name

after_save :update_balance_of_ordergroups

scope :sorted, -> { order(name: :asc) }

def self.has_multiple_classes
Expand All @@ -18,4 +22,10 @@ def display
I18n.t('activerecord.attributes.financial_transaction.amount')
end
end

private

def update_balance_of_ordergroups
ordergroups.each { |og| og.update_balance! }
end
Copy link
Member

Choose a reason for hiding this comment

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

This can be quite an expensive operation, I'm a little worried it may introduce problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm too, but we have no good way to do this otherwise, and it's a VERY unusual operation, so it shouldn't matter that much

Copy link
Member

Choose a reason for hiding this comment

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

It's indeed unusual, but I'd be reluctant to make people wary of changing the name, for example.
Actually, when/why exactly does the balance need to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we ignore some FinancialTransactionClass the account_balance can change

Copy link
Member

Choose a reason for hiding this comment

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

So that is: on create and destroy (when ignore_for_account_balance is false), and when ignore_for_account_balance changes, right?

What about not doing this when the name changes?
We can move it to a background job if it becomes too slow in practice for someone.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO getting the relevant data from the database to do a selective change is much more comlicated, than just doing it for all ordergroups. as already stated, this is something which nearly never happens and I don't see the benefit of adding complex logic for such a rare case

Copy link
Member Author

Choose a reason for hiding this comment

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

create would be not required, since there are no FinancialTransaction where we need to apply it.

end
8 changes: 8 additions & 0 deletions app/models/financial_transaction_type.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ class FinancialTransactionType < ApplicationRecord
belongs_to :financial_transaction_class
belongs_to :bank_account, optional: true
has_many :financial_transactions, dependent: :restrict_with_exception
has_many :ordergroups, -> { distinct }, through: :financial_transactions

validates :name, presence: true
validates_uniqueness_of :name
validates_uniqueness_of :name_short, allow_blank: true, allow_nil: true
validates_format_of :name_short, :with => /\A[A-Za-z]*\z/
validates :financial_transaction_class, presence: true

after_save :update_balance_of_ordergroups
before_destroy :restrict_deleting_last_financial_transaction_type

scope :with_name_short, -> { where.not(name_short: [nil, '']) }
Expand All @@ -27,4 +29,10 @@ def self.has_multiple_types
def restrict_deleting_last_financial_transaction_type
raise I18n.t('model.financial_transaction_type.no_delete_last') if FinancialTransactionType.count == 1
end

private

def update_balance_of_ordergroups
ordergroups.each { |og| og.update_balance! }
end
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this only really needs to happen when the financial_transaction_class changes - or perhaps I'm missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

end
6 changes: 5 additions & 1 deletion app/models/ordergroup.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ def update_stats!
end

def update_balance!
update_attribute :account_balance, financial_transactions.sum('amount')
new_account_balance = financial_transactions
.joins(financial_transaction_type: [:financial_transaction_class])
.where({ financial_transaction_classes: { ignore_for_account_balance: false} })
.sum(:amount)
update_attribute :account_balance, new_account_balance
end

def avg_jobs_per_euro
Expand Down
1 change: 1 addition & 0 deletions app/views/admin/financial_transaction_classes/_form.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
%h3= @financial_transaction_class.new_record? ? t('.title_new') : t('.title_edit')
.modal-body
= f.input :name
= f.input :ignore_for_account_balance
.modal-footer
= link_to t('ui.close'), '#', class: 'btn', data: {dismiss: 'modal'}
= f.submit class: 'btn btn-primary'
1 change: 1 addition & 0 deletions db/schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@

create_table "financial_transaction_classes", id: :integer, force: :cascade do |t|
t.string "name", null: false
t.boolean "ignore_for_account_balance", default: false, null: false
end

create_table "financial_transaction_types", id: :integer, force: :cascade do |t|
Expand Down
64 changes: 46 additions & 18 deletions spec/models/ordergroup_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,51 @@
let(:ftt3) { create :financial_transaction_type, financial_transaction_class: ftc2 }
let(:user) { create :user, groups:[create(:ordergroup)] }

it 'has correct FinancialTransactionClass sums' do
og = user.ordergroup
og.add_financial_transaction!(-1, '-1', user, ftt1)
og.add_financial_transaction!(2, '2', user, ftt1)
og.add_financial_transaction!(3, '3', user, ftt1)

og.add_financial_transaction!(-10, '-10', user, ftt2)
og.add_financial_transaction!(20, '20', user, ftt2)
og.add_financial_transaction!(30, '30', user, ftt2)

og.add_financial_transaction!(-100, '-100', user, ftt3)
og.add_financial_transaction!(200, '200', user, ftt3)
og.add_financial_transaction!(300, '300', user, ftt3)

result = Ordergroup.include_transaction_class_sum.where(id: og).first
expect(result["sum_of_class_#{ftc1.id}"]).to eq 4
expect(result["sum_of_class_#{ftc2.id}"]).to eq 440
end
context 'with financial transactions' do
before do
og = user.ordergroup
og.add_financial_transaction!(-1, '-1', user, ftt1)
og.add_financial_transaction!(2, '2', user, ftt1)
og.add_financial_transaction!(3, '3', user, ftt1)

og.add_financial_transaction!(-10, '-10', user, ftt2)
og.add_financial_transaction!(20, '20', user, ftt2)
og.add_financial_transaction!(30, '30', user, ftt2)

og.add_financial_transaction!(-100, '-100', user, ftt3)
og.add_financial_transaction!(200, '200', user, ftt3)
og.add_financial_transaction!(300, '300', user, ftt3)
end

it 'has correct account balance' do
og = user.ordergroup
expect(og.account_balance).to eq 444

ftc1.reload
ftc1.update_attributes!(ignore_for_account_balance: true)

og.reload
expect(og.account_balance).to eq 440

ftt2.reload
ftt2.update_attributes!(financial_transaction_class: ftc1)

og.reload
expect(og.account_balance).to eq 400
end

it 'has correct FinancialTransactionClass sums' do
og = user.ordergroup
result = Ordergroup.include_transaction_class_sum.where(id: og).first
expect(result["sum_of_class_#{ftc1.id}"]).to eq 4
expect(result["sum_of_class_#{ftc2.id}"]).to eq 440

ftt2.reload
ftt2.update_attributes!(financial_transaction_class: ftc1)

result = Ordergroup.include_transaction_class_sum.where(id: og).first
expect(result["sum_of_class_#{ftc1.id}"]).to eq 44
expect(result["sum_of_class_#{ftc2.id}"]).to eq 400
end
end
end