Add option to ignore financial transaction when calculating the balance #817
Conversation
|
|
||
| def update_balance_of_ordergroups | ||
| ordergroups.each { |og| og.update_balance! } | ||
| end |
There was a problem hiding this comment.
This can be quite an expensive operation, I'm a little worried it may introduce problems.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
if we ignore some FinancialTransactionClass the account_balance can change
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
create would be not required, since there are no FinancialTransaction where we need to apply it.
spec/models/ordergroup_spec.rb
Outdated
| 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 | ||
| expect(og.account_balance).to eq 400 |
There was a problem hiding this comment.
Can we make this separate tests, please? So that we can more easily see which parts break when modifying code.
|
|
||
| def update_balance_of_ordergroups | ||
| ordergroups.each { |og| og.update_balance! } | ||
| end |
There was a problem hiding this comment.
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?
|
|
||
| def update_balance_of_ordergroups | ||
| ordergroups.each { |og| og.update_balance! } | ||
| end |
There was a problem hiding this comment.
I suppose this only really needs to happen when the financial_transaction_class changes - or perhaps I'm missing something?
No description provided.