Review Request 126875: Combined the questions from TransactionMatcher::match and MyMoneyStatementReader::askUserToEnterScheduleForMatching
Allan Anderson
agander93 at gmail.com
Wed Feb 24 18:11:27 UTC 2016
> On Jan. 24, 2016, 9: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.
>
> Christian David wrote:
> 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.
Yes, of course. I'm pretty sure, though, that I'm not the only user who welcomed the removal of restrictions on matching. I'm unaware of any means to warn of regressions.
Is there a fundamental difference betwwen a message box question, and an exception requiring a response? There are a few still there.
- Allan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126875/#review91536
-----------------------------------------------------------
On Jan. 24, 2016, 10: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, 10: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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20160224/65c14267/attachment-0001.html>
More information about the KMyMoney-devel
mailing list