Bug 798958 - gncScrubLotLinks will infinite loop in some conditions #1718
Conversation
jralls
force-pushed
the
bug798958
branch
2 times, most recently
from
c5c2c24
to
a19b6f2
Compare
|
Is this scrubbing confirming the view that "a zero value/amount split shouldn't ever belong in a lot"? If so:
|
|
@christopherlam, A "lot" in accounting is a quantity of some commodity acquired on a particular date at a particular price. CDB-Man's investment accounting ignores lots in favor of an average cost model so this is irrelevant. OTOH lot accounting is concerned with applying the cost per-lot to the value of inventory or for calculating capital gains and losses on an investment so while there's no harm in zero-amount splits, they're generally irrelevant to the lot module's calculations. Obviously using this lots facility to link payments to invoices and bills has nothing to do with the accounting purpose. It does seem reasonable to me to exclude 0-amount splits from lots and also to add removing a split from its lot when its parent transaction is voided, but I'm not particularly motivated to make time to implement either of them right now. |
|
And the portfolio reports don't query lots at all, so, shouldn't be affected by this change. The only remaining concern is capgains.c which I've never understood. |
That's what "lot accounting is concerned with applying the cost per-lot to the value of inventory or for calculating capital gains and losses on an investment so while there's no harm in zero-amount splits, they're generally irrelevant to the lot module's calculations." was about: That's what capgains.c does for the first in first out strategy. It works, but it's really ugly. |
|
The need to remove empty splits from lot links arose from a poor initial assignment algorithm when I first implemented lot links. When shuffling open (business) lots around a lot it would fragment de lot amount over an increasing number of splits, eventually leading up to empty splits. The idea behind the scrubbing code was that these auto-generated splits could simply be removed. I haven't checked yet how a voided transaction comes into play here. This scrubbing was intended to clean up auto-generated splits. Perhaps I didn't consider all possible scenarios in which this code could be called. Or perhaps later modifications to gnucash resulted in this scrubbing code to be invoked in conditions it wasn't intended for. If users open an older book that was affected by the poor algorithm it could still have these empty splits. Should we find a better test to isolate these and remove them anyway? Like testing for empty and non-void? |
|
@gjanssens OK, but the lot-scrubbing code gets applied to plenty of other cases. We need something a bit more surgical to clean up just those empty splits. Your description of the problem suggests that empty splits might not be the only problem: A transaction might have several splits with small amounts in the same account that need to be consolidated into a single split. Is that right? Do I understand correctly that this use of lots is to calculate the remaining balance when there are multiple payments of a single bill/invoice? Will affected transactions always have the |
The specific code in this PR should only be called on business lots, not for stock related lots. BTW, reusing the lots code for business transaction tracking is probably an unfortunate design choice...
Yes. There is separate scrub code for that issue in xaccScrubMergeLotSubSplits |
This is the tricky bit. You suggested earlier there are more reasons to have 0-valued splits. Are you referring to capital gain calculations or stock split operations ? These should never happen on lots used in business transactions, or at least the code was written assuming so. The other case of a 0-value split would be one added explicitly by the user to model a 0-valued line on a payment receipt or something similar. The code never took that possibility into consideration and I don't know off-hand if we can distinguish such a split from an auto-generated one at all. |
They are used to track any payment of invoices or bills as well as outstanding payments to a vendor or customer that are not yet tied to any invoice or bill at all. All transactions related to any of the above will have the So both P-type and L-type transactions will be processed by the code in this PR. The I-type ones will be skipped. While writing this it occurs to me the current payment assignment algorithm may internally still generate intermediate 0-valued splits and depend on the scrubbing code to clean these up. I mean that the process of assigning payments and scrubbing old books share the code that actually evaluates existing lots and splits to recombine them (using a fifo strategy). Part of how that currently works may well generate temporary empty splits that are expected to be removed before the assignment process completes. I certainly hadn't considered users could void a payment transaction while implementing that. For those I do agree the cleaner way out is to remove them from the lot only. Oh and user entered empty splits as I mentioned earlier would not be an issue: while they could appear in a payment transaction these splits would never be in a business lot (unless the user explicitly added them to a lot in the lot viewer, but there is no good reason to do so). The scrubbing code at hand will only alter splits that are part of a business lot. |
But it doesn't get the 0 splits too? Can it?
It's called only on transactions with a split in an APAR account, so considering gncLot's limitations that's probably a reasonable assumption. So the minimum to fix the infinite loop we could check that the transaction isn't read-only before deleting the split, and if it is just remove the split from the lot list. Is that what you think we should do? BTW, is there a reason for testing the same condition twice or are they supposed to be different, maybe one of them should be amount? |
I don't think it currently does and I don't remember exactly why I did it like that back then. So I can't tell without revisiting the code whether it's safe to change that... (code reading on a cell phone is no fun)
Yes.
I'm pretty sure I intended to test both amount and value. Though I wonder if one of them would sufficient as well ? |
|
An additional thought regarding voided transactions. As I mentioned I didn't consider them when implementing credit notes and the revised split to lot assignment code. IIRC that code starts from am unbalanced invoice lot and then adds payment splits to it until it is balanced. If the last split would result in an overpayment, it would be split in a part to balance the invoice lot and a remainder split to store in an unassigned payment lot. So in essence an invoice lot can never have more payment than invoice value. That assumption may currently not hold if a user voids a payment (reducing the lot's payment value), adds another payment instead and finally decides to unvoid the original payment. It should be tested if this results in unexpected behaviour in the assignment logic, both for the invoice lot at hand and for later invoices and payments. |
There are two cases where one would be 0 and not the other, but I don't think either would apply to paying down invoices: If the price is 0 then so will be the value regardless of the amount, and we allow the amount to be 0 with a non-0 value for entering things like capital gains splits on commodities. I think for this purpose checking the value once is good enough.
Removing the split from the lot list with |
The conditions being containing a split from a voided transaction. Instead of trying to destroy zero-value splits (doomed to fail in that instance and generally rude otherwise) just remove them from the lot list.
|
@gjanssens Now deletes the split if it can, removes the split from the lot-list if it can't. I can merge if you're satisfied or wait till you have a big enough screen available to do a code review. |
|
Looks safe and good to me. Feel free to commit. For end users there will be a subtle change in behaviour to be aware of: if you accidentally void a payment transaction and unvoid it again, the payment is no longer linked to the invoice. Before this patch it would. I think the new behaviour is fine though, because that's also what happens if you unpost and re-post an invoice. So this is more consistent. Long-time users may be surprised by the change though... |
Only if you run check-and-repair or scrub lots in the lot viewer in between voiding and unvoiding. But consider that before this change if you voided an APAR split that was part of a lot and then did any of those things GnuCash would hang. @christopherlam is the first one to file a bug about this so I don't believe that many users will have gotten used to the lot-link being restored across one of those actions. |
|
Without studying the code again I can't say for sure. I believe parts of business scrub are shared with payment assignment code, but perhaps this part isn't. I'll revisit when I'm back. Meanwhile by all means merge this PR. |
The conditions being containing a split from a voided transaction. Instead of trying to destroy zero-value splits (doomed to fail in that instance and generally rude otherwise) just remove them from the lot list.