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

[Łukasz Maszczyński]

> On Feb. 5, 2013, 5:41 p.m., Cristian Oneț wrote:
> > It's really nice that you made an effort to create test cases for this part of the application that did not have any before. See my comments bellow regarding the current behavior.

Thanks for your insights, and sorry for keeping you waiting - it's been busy time becoming a father :)


> On Feb. 5, 2013, 5:41 p.m., Cristian Oneț wrote:
> > kmymoney/converter/matchfindertest.cpp, lines 206-214
> > <http://git.reviewboard.kde.org/r/108489/diff/1/?file=108027#file108027line206>
> >
> >     Consider the following case:
> >     The user import's the statement and matches the transactions. Now if the user imports the same statements again wouldn't the behavior you desire create the same transactions again?

You have a point, this one needs to stay as is.


> 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).

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?


> 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?

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.


- Łukasz


-----------------------------------------------------------
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/20130302/dbefcec0/attachment-0001.html>