<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107137/">http://git.reviewboard.kde.org/r/107137/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 27th, 2012, 8:51 p.m., <b>Cristian Oneț</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Today I have managed to make a functional test. It seems that there is a bug somewhere since repeatedly importing the same ofx file adds the same transactions over and over again although the import dialog states that the duplicates where found.
If you need the sample file let me know and I'll mail it to you. The current version correctly imports the transactions only once.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Confirmed - that is a change of behaviour compared to the original version. I fixed it in diff revision 4.</pre>
<br />
<p>- Łukasz</p>
<br />
<p>On November 28th, 2012, 8:19 p.m., Łukasz Maszczyński wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KMymoney.</div>
<div>By Łukasz Maszczyński.</div>
<p style="color: grey;"><i>Updated Nov. 28, 2012, 8:19 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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")
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">make test</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(93af070)</span></li>
<li>kmymoney/converter/mymoneystatementreader.h <span style="color: grey">(758ff00)</span></li>
<li>kmymoney/converter/mymoneystatementreader.cpp <span style="color: grey">(42c4841)</span></li>
<li>kmymoney/dialogs/CMakeLists.txt <span style="color: grey">(9a8d782)</span></li>
<li>kmymoney/dialogs/existingtransactionmatchfinder.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kmymoney/dialogs/existingtransactionmatchfinder.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kmymoney/dialogs/scheduledtransactionmatchfinder.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kmymoney/dialogs/scheduledtransactionmatchfinder.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>kmymoney/dialogs/transactionmatcher.h <span style="color: grey">(d09a4cd)</span></li>
<li>kmymoney/dialogs/transactionmatcher.cpp <span style="color: grey">(c380877)</span></li>
<li>kmymoney/dialogs/transactionmatchfinder.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>kmymoney/dialogs/transactionmatchfinder.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107137/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>