[Kmymoney-devel] Review Request 108489: added tests for transaction matching

[Cristian Oneț]

> On Feb. 5, 2013, 5:41 p.m., Cristian Oneț wrote:
> > kmymoney/converter/matchfindertest.cpp, lines 291-300
> > <http://git.reviewboard.kde.org/r/108489/diff/1/?file=108027#file108027line291>
> >
> >     I consider the current behavior correct considering that the bankId can only be saved once a transaction has been imported. The bankId can migrate in only one way from the outside world into KMyMoney. That's the source of the asymmetry.
> >     
> >     If you have in KMyMoney a transaction without a bankId you can match to it an imported transaction (you probably need to update the bankId) but it's wrong to match a KMyMoney transaction which has a bankId because it was already matched at a previous import.
> >     
> >     The bankId is the common key between KMyMoney and the outside world (a kind of external identifier).
> 
> Łukasz Maszczyński wrote:
>     Not sure I get you right. Every transaction with bankId set must be an imported transaction. But not every imported transaction must have a bankId. Is that what you mean?
>     
>     You've lost me in the 2nd paragraph :) I can't get the relation between what you wrote and the testcase, could you please explain it in a bit more details?
>

What I wanted to say is that a ledger transaction with a bankId should not match an imported transaction without a bankId no matter what. Based on this statement I consider the current test case correct so it should not be revised.

The reason I consider the above statement correct is the following:
A ledger transaction with a bankId can only be obtained trough an imported transaction with that bankId and it was matched. So I consider matching it again to an imported transaction without that bankId to be wrong since it must be a different transaction (it does not have the same bankId).


> On Feb. 5, 2013, 5:41 p.m., Cristian Oneț wrote:
> > kmymoney/converter/matchfindertest.cpp, lines 196-204
> > <http://git.reviewboard.kde.org/r/108489/diff/1/?file=108027#file108027line196>
> >
> >     I'm not sure about the desired behavior that you described. I think that the bankId should be the strongest criteria for matching transactions because it should really be a unique ID of the transaction. Could you point to a use case in which two unrelated transactions get the same bankId?
> 
> Łukasz Maszczyński wrote:
>     I also think that the bankId should be the strongest criteria, and I think it should not be the only one. It seems to be the case already - if two transactions have the same bankId, but:
>     - a different amount OR
>     - mismatching post date OR 
>     - refer different accounts 
>     they will not be considered duplicates. I think payee check should be added to the list, especially that it's rather a loose check - an empty payee in either transaction will be a match as well, just like in "regular" (non-duplicate) matching.

If the amount and the post date are checked you could also add the payee check although I'm not sure about the first two checks either. Scoping the bankId within an account seems OK though (I don't have a strong argument for this, it's just a feeling).


- Cristian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108489/#review26703
-----------------------------------------------------------


On Jan. 19, 2013, 5:08 p.m., Łukasz Maszczyński wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108489/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2013, 5:08 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Description
> -------
> 
> Added some tests to characterize application behavior regarding matching an imported transaction with existing/scheduled one.
> 
> I'd like to emphasize two important things in this review:
> 1. I've marked three tests with a comment "revise behavior", I believe they show unwanted behavior. Description of what I think should be expected is inside the test functions, so please comment those parts and let me know if my understanding is (in)correct.
> 2. I've removed a check, which I believe is superfluous - splitsReferenceSameAccount(), as transaction filter already takes care of filtering off the transactions which refer to a different account. I believe the 4 tests (*_accountMismatch_* and *_multipleAccounts_*) show that this check is not needed, however it was in the code for a long time, only rewritten during recent refactoring.
> 
> 
> Diffs
> -----
> 
>   kmymoney/converter/CMakeLists.txt 8433630af638df863dad55e10a3ead60f0b4c7f6 
>   kmymoney/converter/matchfindertest.h PRE-CREATION 
>   kmymoney/converter/matchfindertest.cpp PRE-CREATION 
>   kmymoney/converter/transactionmatchfinder.h 5f276a846de4cc72bd2700f2560a64b8ee260217 
>   kmymoney/converter/transactionmatchfinder.cpp 35e72cbc65dde762ed2e11651185afe2c4a14a3c 
> 
> Diff: http://git.reviewboard.kde.org/r/108489/diff/
> 
> 
> Testing
> -------
> 
> make test
> 
> 
> Thanks,
> 
> Łukasz Maszczyński
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20130408/ff78b809/attachment-0001.html>