[Kmymoney-devel] Review Request: Refactoring of matching a transaction-under-import

Łukasz Maszczyński lukasz at maszczynski.net
Wed Dec 5 20:29:29 UTC 2012



> On Nov. 29, 2012, 6:52 p.m., Cristian Oneț wrote:
> > There still seem to be some problems with the patch here are the steps to reproduce the (using the same ofx file that I've sent you):
> > 1. Before importing the file prepare a transaction so that one of the imported transactions are matched
> > 2. Import the ofx file
> > 3. Observe the match - don't accept it just save the kmymoney file (I don't know if this and the next step is important)
> > 4. Close and reopen the file
> > 5. Import the ofx file again
> > 
> > Expected result:
> > The matched transaction is detected as a duplicate and not imported.
> > 
> > Actual result:
> > The matched transaction is imported again.
> > 
> > Note that I performed steps 1-3 with the previous buggy version of the patch so this could be a no-issue if only this patch is used. Anyway I'll send you my testfile by mail.

That was an issue, indeed. I hastily simplified the method TransactionMatchFinder::findMatchingSplit() which finds a match for a split-under-import, so that both duplicate matching and regular matching works on a similar set of conditions, while the original version didn't really work this way. 
I think there are some inconsistencies in handling duplicates vs. regular matches that need to be addressed - I will do that in a separate review after this one is finished.

Anyway I fixed this case and also some other(s) in rev. 6.


- Łukasz


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


On Dec. 5, 2012, 8:28 p.m., Łukasz Maszczyński wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107137/
> -----------------------------------------------------------
> 
> (Updated Dec. 5, 2012, 8:28 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Description
> -------
> 
> 1. please note that dependency on Boost is no longer optional (see changes in CMakeLists.txt)
> 
> 2. Architectural changes
>    Until now, the method MyMoneyStatementReader::processTransactionEntry() handled the whole process of importing a transaction - that is: handling the securities, matching and creating payees, and - at the very end of the method - adding the transaction to the ledger.
>    The last step (adding transaction to ledger) is the main target of this refactoring. Its algorithm was as follows:
>    1. find a matching transaction (either existing or scheduled) - using TransactionMatcher::findMatch()
>    2. If an "existing transaction match" is found - handle it (in the old code it's the block starting with a comment "// it matched a simple transaction. that's the easy case")
>    3. Else if a "scheduled transaction match" is found - handle it ("// a match has been found in a pending schedule"...)
> 
>    Code "mapping" is as follows:
>    - step 2 (above) is extracted to handleMatchingOfExistingTransaction()
>    - step 3 (above) is extracted to handleMatchingOfScheduledTransaction()
>    - TransactionMatcher::findMatch() is extracted to TransactionMatchFinder::findMatch() (note: there are two pure-virtual functions that are implemented in ExistingTransactionMatchFinder, ScheduledTransactionMatchFinder classes)
>    - TransactionMatcher::checkTransaction() is extracted to TransactionMatchFinder::findMatchingSplit()
> 
> 3. Memory management changes
>    Raw pointers are no longer used, as these are typically error-prone. Pointers were replaced either with object instances, or boost::optional is used if applicable (e.g. see members of TransactionMatchFinder class).
> 
> 4. dynamic_casts removed (were used on pointers returned by TransactionMatcher::findMatch(), no longer needed - see TransactionMatchFinder::getMatchedTransaction() and getMatchedSchedule() )
> 
> 5. variable/method names - I did my best to keep those meaningful: e.g. "importedTransaction" instead of "t")
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 93af070 
>   kmymoney/converter/mymoneystatementreader.h 758ff00 
>   kmymoney/converter/mymoneystatementreader.cpp 42c4841 
>   kmymoney/dialogs/CMakeLists.txt 9a8d782 
>   kmymoney/dialogs/existingtransactionmatchfinder.h PRE-CREATION 
>   kmymoney/dialogs/existingtransactionmatchfinder.cpp PRE-CREATION 
>   kmymoney/dialogs/scheduledtransactionmatchfinder.h PRE-CREATION 
>   kmymoney/dialogs/scheduledtransactionmatchfinder.cpp PRE-CREATION 
>   kmymoney/dialogs/transactionmatcher.h d09a4cd 
>   kmymoney/dialogs/transactionmatcher.cpp c380877 
>   kmymoney/dialogs/transactionmatchfinder.h PRE-CREATION 
>   kmymoney/dialogs/transactionmatchfinder.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/107137/diff/
> 
> 
> Testing
> -------
> 
> make test
> 
> 
> Thanks,
> 
> Łukasz Maszczyński
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20121205/2b154277/attachment.html>


More information about the KMyMoney-devel mailing list