> On Jan. 24, 2016, 10:02 p.m., Christian David wrote: > > kmymoney/converter/mymoneystatementreader.cpp, line 1458 > > <https://git.reviewboard.kde.org/r/126875/diff/1/?file=439594#file439594line1458> > > > > 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). > > Artur Puzio wrote: > But the i18np() is for plurals. What we can do is change the previous > string only slitly to add the variable that will contain `"The transaction > dates are one day apart.<br/>"` or `""`. > > Christian David wrote: > You could use something like > > i18n("<html>KMyMoney has found a scheduled transaction which matches > an imported transaction.<br/>" > "Schedule name: <b>%1</b><br/>" > "Transaction: <i>%2 %3</i><br/>" > "Do you want KMyMoney to enter this schedule > now so that the transaction can be matched?</html>", > scheduleName, splitValue, payeeName); > > 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); > > Allan Anderson wrote: > 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. > > Artur Puzio wrote: > Allan, you are correct, I will check if the `match` function is invoked > elsewhere. > > Christian David wrote: > Actually, I think you are right Allan. But which non-imported > transactions are matched? Do you mean user entered transactions in the > register? > > Allan Anderson wrote: > 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. > > Christian David wrote: > I rechecked this. According to KDevelop’s code parser > ```TransactionMatcher::match(...)``` is used three times, namely in: > > MyMoneyStatementReader::handleMatchingOfExistingTransaction(...) > MyMoneyStatementReader::handleMatchingOfScheduledTransaction(...) > KMyMoneyApp::transactionMatch() > > The second one is using > ```MyMoneyStatementReader::askUserToEnterScheduleForMatching``` 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). > > However, the question must be moved out of > ```TransactionMatcher::match()```. Otherwise the user is asked multiple times > if he wants to match a single transaction – a usability sin. > > Btw: ```TransactionMatcher::match()``` should not contain any user > interaction by design to keep our codebase readable. It is a backend function. > > Christian David wrote: > I did some research how the other functions use > ```TransactionMatcher::match()```. > > ```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. > > ```MyMoneyStatementReader::handleMatchingOfExistingTransaction()``` is a > bit more tricky. It is call by > ```MyMoneyStatementReader::processTransaction()``` which is used by > ```MyMoneyStatementReader::import()``` which is finaly called by > ```KMyMoneyApp::slotStatementImport()```. So I assume the call to > ```handleMatchingOfExistingTransaction()``` 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. > > So I recommend to ship this request. > > Allan Anderson wrote: > > 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. > > 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.
I was afraid you will say this and of course you are right. Are you okay if I publish this review request anyway under the condition that I will try to add the question for the manual merge again in the next couple of months (so it will get into KMyMoney 5)? I want highlight that ```TransactionMatcher::match()``` is the wrong place for this message box in any case. - Christian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/126875/#review91536 ----------------------------------------------------------- On Jan. 24, 2016, 11:07 p.m., Artur Puzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/126875/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2016, 11:07 p.m.) > > > Review request for KMymoney and Christian David. > > > Repository: kmymoney > > > Description > ------- > > Moved the question from `TransactionMatcher::match` to > `MyMoneyStatementReader::askUserToEnterScheduleForMatching`. > Added `MyMoneyTransaction & importedTransaction` parmeter to > `MyMoneyStatementReader::askUserToEnterScheduleForMatching`, as > `importedTransation` is required to calculate if the question from > `TransactionMatcher::match` should be shown. > > > Diffs > ----- > > kmymoney/converter/mymoneystatementreader.h 80da202 > kmymoney/converter/mymoneystatementreader.cpp 1634bbb > kmymoney/dialogs/transactionmatcher.cpp 7d97404 > > Diff: https://git.reviewboard.kde.org/r/126875/diff/ > > > Testing > ------- > > Automated tests still pass, but they don't check the subject of work. > Screenshot: [link](https://imgur.com/3Veh70N) > > > Thanks, > > Artur Puzio > >
