<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/126875/">https://git.reviewboard.kde.org/r/126875/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 24th, 2016, 9:02 p.m. UTC, <b>Christian David</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/126875/diff/1/?file=439594#file439594line1458" style="color: black; font-weight: bold; text-decoration: underline;">kmymoney/converter/mymoneystatementreader.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1458</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">questionMsg</span> <span class="o">+=</span> <span class="n">i18np</span><span class="p">(</span><span class="s">"The transaction dates are one day apart.<br/>"</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Translated strings should not be combined, as it confuses the translators. It is better to create two long strings using i18np() although this duplicates the text (which is usually bad).</p></pre>
</blockquote>
<p>On January 24th, 2016, 9:06 p.m. UTC, <b>Artur Puzio</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But the i18np() is for plurals. What we can do is change the previous string only slitly to add the variable that will contain <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">"The transaction dates are one day apart.<br/>"</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">""</code>.</p></pre>
</blockquote>
<p>On January 24th, 2016, 9:23 p.m. UTC, <b>Christian David</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You could use something like</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">i18n("<span style="color: #008000; font-weight: bold"><html></span>KMyMoney has found a scheduled transaction which matches an imported transaction.<span style="color: #008000; font-weight: bold"><br/></span>"
"Schedule name: <span style="color: #008000; font-weight: bold"><b></span>%1<span style="color: #008000; font-weight: bold"></b><br/></span>"
"Transaction: <span style="color: #008000; font-weight: bold"><i></span>%2 %3<span style="color: #008000; font-weight: bold"></i><br/></span>"
"Do you want KMyMoney to enter this schedule now so that the transaction can be matched?<span style="color: #008000; font-weight: bold"></html></span>",
scheduleName, splitValue, payeeName);
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and
i18np("<html>KMyMoney has found a scheduled transaction which matches an imported transaction.<br/>"
"Schedule name: <b>%2</b><br/>"
"Transaction: <i>%3 %4</i><br/>"
"The transaction dates are one day apart.<br/>"
"Do you want KMyMoney to enter this schedule now so that the transaction can be matched?</html>",
"<html>KMyMoney has found a scheduled transaction which matches an imported transaction.<br/>"
"Schedule name: <b>%2</b><br/>"
"Transaction: <i>%3 %4</i><br/>"
"The transaction dates are %1 days apart.<br/>"
"Do you want KMyMoney to enter this schedule now so that the transaction can be matched?</html>",
gap ,scheduleName, splitValue, payeeName);</p></pre>
</blockquote>
<p>On January 24th, 2016, 9:44 p.m. UTC, <b>Allan Anderson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think this code was transferred from kmymoney/dialogs/transactionmatcher.cpp lines 79-88, and now, here, I think it deals with matching a schedule. It's a while since I was in this area, so this could be totally wrong, but I think that in the original position, it also handled matched non-scheduled and non-imported transactions. Have you tested this capability? If so, great and I'm wrong.</p></pre>
</blockquote>
<p>On January 24th, 2016, 10:07 p.m. UTC, <b>Artur Puzio</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Allan, you are correct, I will check if the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">match</code> function is invoked elsewhere.</p></pre>
</blockquote>
<p>On January 24th, 2016, 10:22 p.m. UTC, <b>Christian David</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Actually, I think you are right Allan. But which non-imported transactions are matched? Do you mean user entered transactions in the register?</p></pre>
</blockquote>
<p>On January 25th, 2016, 12:38 a.m. UTC, <b>Allan Anderson</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">See https://bugs.kde.org/show_bug.cgi?id=333949.
This added the ability to match two imported transactions, and also two manually entered ones. Originally, only a combination of one of each could be matched. No scheduled transactions were changed, although I did add allowing the user to accept a match outside the default date scope.</p></pre>
</blockquote>
<p>On February 3rd, 2016, 8:07 p.m. UTC, <b>Christian David</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I rechecked this. According to KDevelop’s code parser <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TransactionMatcher::match(...)</code> is used three times, namely in:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">MyMoneyStatementReader::handleMatchingOfExistingTransaction(...)
MyMoneyStatementReader::handleMatchingOfScheduledTransaction(...)
KMyMoneyApp::transactionMatch()
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The second one is using <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MyMoneyStatementReader::askUserToEnterScheduleForMatching</code> where the question was moved to. So the question which is left: do we need the time check in the other two functions (I do not know when they are used, yet).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, the question must be moved out of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TransactionMatcher::match()</code>. Otherwise the user is asked multiple times if he wants to match a single transaction – a usability sin.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw: <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TransactionMatcher::match()</code> should not contain any user interaction by design to keep our codebase readable. It is a backend function.</p></pre>
</blockquote>
<p>On February 24th, 2016, 9:17 a.m. UTC, <b>Christian David</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I did some research how the other functions use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TransactionMatcher::match()</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KMyMoneyApp::transactionMatch()</code> is only called if the user manually starts a match using a button. So the check is reasonable but not necessary – the user probably knows what he is doing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MyMoneyStatementReader::handleMatchingOfExistingTransaction()</code> is a bit more tricky. It is call by <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MyMoneyStatementReader::processTransaction()</code> which is used by <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MyMoneyStatementReader::import()</code> which is finaly called by <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KMyMoneyApp::slotStatementImport()</code>. So I assume the call to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">handleMatchingOfExistingTransaction()</code> is in the end caused by an import of a file with many entries. Therefore the question is a really good idea. However in it's current way (before this review request) absolutly useless. The user has no information which transaction he is asked about – remember this is a mass import. So removing this question is actually mandatory in my opinion.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So I recommend to ship this request.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KMyMoneyApp::transactionMatch() is only called if the user manually starts a match using a button. So the check is reasonable but not necessary > the user probably knows what he is doing.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While the user will know what he thinks he is doing, he could have made a mistake. For instance, when selecting the two transations to be matched, he could inadvertantly click on the wrong line after duplicating, resulting in one date being outside the configured matching scope. His resulting transaction could contain an error.</p></pre>
<br />
<p>- Allan</p>
<br />
<p>On January 24th, 2016, 10:07 p.m. UTC, Artur Puzio wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KMymoney and Christian David.</div>
<div>By Artur Puzio.</div>
<p style="color: grey;"><i>Updated Jan. 24, 2016, 10:07 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kmymoney
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Moved the question from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TransactionMatcher::match</code> to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MyMoneyStatementReader::askUserToEnterScheduleForMatching</code>.
Added <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MyMoneyTransaction & importedTransaction</code> parmeter to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">MyMoneyStatementReader::askUserToEnterScheduleForMatching</code>, as <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">importedTransation</code> is required to calculate if the question from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TransactionMatcher::match</code> should be shown.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Automated tests still pass, but they don't check the subject of work.
Screenshot: <a href="https://imgur.com/3Veh70N" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">link</a></p></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>kmymoney/converter/mymoneystatementreader.h <span style="color: grey">(80da202)</span></li>
<li>kmymoney/converter/mymoneystatementreader.cpp <span style="color: grey">(1634bbb)</span></li>
<li>kmymoney/dialogs/transactionmatcher.cpp <span style="color: grey">(7d97404)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126875/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>