----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108489/#review26703 -----------------------------------------------------------
Ship it! 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. kmymoney/converter/matchfindertest.cpp <http://git.reviewboard.kde.org/r/108489/#comment20276> 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? kmymoney/converter/matchfindertest.cpp <http://git.reviewboard.kde.org/r/108489/#comment20277> 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? kmymoney/converter/matchfindertest.cpp <http://git.reviewboard.kde.org/r/108489/#comment20278> 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). - Cristian Oneț 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 > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
