In Ledger/Import, there is a setting to allow matching within X days. Should this have a bearing on attempting to match two transactions? i.e. to be able to select them for matching, or otherwise, and thence to import? I seem to be able to import transactions outside my set limit of 4 days. Reproducible: Always Steps to Reproduce: 1.Select two transactions which, apart from dates, should match. 2.Ensure dates are outside the limits set in the Settings. 3.Click on Match. Actual Results: The transactions will be matched, even though the dates indicate otherwise. Expected Results: Matching should not occur. From Dev list. On Sunday 15 March 2015 14:28:07 aga wrote: > In Ledger/Import, there is a setting to allow matching within X days. > Should this have a bearing on attempting to match two transactions? > i.e. to be able to select them for matching, or otherwise, and thence to > imnport? [Thomas] It is used to stop automatic matching in case the transaction dates of the stored transaction and the one imported is greater than the difference provided. The transaction is imported nevertheless. [Me] > I seem to be able to import transactions outside my set limit of 4 days. [Thomas] Yes, but it won't be matched against an existing transaction outside of a ± 4 day period of the imported transaction's posting date. Does that make sense? [Me] > Yes, that makes sense, and is what I expected. > What I should have mentioned, the important bit, is that it seems to be > ignored during matching. I've just successfully matched two > transactions with post dates over a month apart. [Thomas] Yes, that looks like one of those little six-legged creatures to me. Feel free to record it at b.k.o. so that we don't forget about it.
As I understand Thomas' comment, the number of days is used to determine whether an imported transaction should be matched to an existing transaction (within X± days) or imported as a new transaction (over X± days). I haven't looked at the code, but if this X is not used anywhere else, I would consider that the problem is just an insufficient label or explanation on how that number of days is used, not necessarily a bug. Would you like it to refuse to allow a manual match or just put up a notice/warning, it the manually selected transactions are too far apart? I would prefer either no action or a warning, since I often manually enter a check when I write it, but the import from the bank only happens when the check clears, which may be a long time later. (At some point I may enter a wish list so that match keeps the date from the manually entered transaction and not from the imported one, but that is a completely separate issue.)
(In reply to Jack from comment #1) > As I understand Thomas' comment, the number of days is used to determine > whether an imported transaction should be matched to an existing transaction > (within X± days) or imported as a new transaction (over X± days). I haven't > looked at the code, but if this X is not used anywhere else, I would > consider that the problem is just an insufficient label or explanation on > how that number of days is used, not necessarily a bug. Would you like it > to refuse to allow a manual match or just put up a notice/warning, it the > manually selected transactions are too far apart? I would prefer either no > action or a warning, since I often manually enter a check when I write it, > but the import from the bank only happens when the check clears, which may > be a long time later. (At some point I may enter a wish list so that match > keeps the date from the manually entered transaction and not from the > imported one, but that is a completely separate issue.) My reading of the purpose of this setting, I believe, tallies with Thomas's. If it indicates that matching shall occur only within that match-window, and if it does not do that, then that to me is definitely a bug. What I would expect to happen is that, in common with other parameters that are taken into account, an exception should occur to warn the user that the transactions do not match, and give the reason. The user then has the choice of how to proceed - edit a date, select the correct transactions, abandon the attempted matching, etc.
I think there (reasonably) is a difference of the effect of the setting on what the system will do iteself, and what it will allow the user to do. I think we all agree on the effect on automatic matching of imported transactions. I also think we agree that for a manually selected match, it should at minimum show an exception or warning if the dates are too far apart. I also think that leaves two differences. First, whether this is an explicit bug or enhancement request - but that is not really important. Second, should it actually allow the user to say "Yes, I really do want to match these two transactions" without forcing any change to the data. I suppose my enhancement request would be for it to allow the user to select either date as the one to keep on the merged transaction, but I certainly want to allow the merge, since most of the time I would make such a choice, I really do mean it - it's not an accident or wrong choice on my part.
(In reply to Jack from comment #3) > I think there (reasonably) is a difference of the effect of the setting on > what the system will do iteself, and what it will allow the user to do. I > think we all agree on the effect on automatic matching of imported > transactions. I also think we agree that for a manually selected match, it > should at minimum show an exception or warning if the dates are too far > apart. I also think that leaves two differences. First, whether this is an > explicit bug or enhancement request - but that is not really important. > Second, should it actually allow the user to say "Yes, I really do want to > match these two transactions" without forcing any change to the data. I > suppose my enhancement request would be for it to allow the user to select > either date as the one to keep on the merged transaction, but I certainly > want to allow the merge, since most of the time I would make such a choice, > I really do mean it - it's not an accident or wrong choice on my part. A suitably wide setting for the match-window should allow you to match such transactions, shouldn't it? That is more or less what happens now, with the setting being ignored. Whichever date is retained after matching could be adjusted as required. I'm not sure about this, but you could try experimenting. The retained date possibly depends on which transaction is selected first and which second. I think that may be what happens now.
(In reply to allan from comment #4) > (In reply to Jack from comment #3) > > I think there (reasonably) is a difference of the effect of the setting on > > what the system will do iteself, and what it will allow the user to do. I > > think we all agree on the effect on automatic matching of imported > > transactions. I also think we agree that for a manually selected match, it > > should at minimum show an exception or warning if the dates are too far > > apart. I also think that leaves two differences. First, whether this is an > > explicit bug or enhancement request - but that is not really important. > > > Second, should it actually allow the user to say "Yes, I really do want to > > match these two transactions" without forcing any change to the data. I > > suppose my enhancement request would be for it to allow the user to select > > either date as the one to keep on the merged transaction, but I certainly > > want to allow the merge, since most of the time I would make such a choice, > > I really do mean it - it's not an accident or wrong choice on my part. > > A suitably wide setting for the match-window should allow you to match > such transactions, shouldn't it? That is more or less what happens now, > with the setting being ignored. Whichever date is retained after > matching could be adjusted as required. > I'm not sure about this, but you could try experimenting. > The retained date possibly depends on which transaction is selected first and > which second. I think that may be what happens now. Whichever transaction is selected last is the one whose date is retained. (I'll verify this as my version has been patched.)
Fix applied as part of BUG:333949 M +6 -0 kmymoney/dialogs/transactionmatcher.cpp http://commits.kde.org/kmymoney/7e7a50e39170d03c9fe83ffcbe394d485cefaf52 diff --git a/kmymoney/dialogs/transactionmatcher.cpp b/kmymoney/dialogs/transactionmatcher.cpp index f211b5b..d0c7db8 100644 --- a/kmymoney/dialogs/transactionmatcher.cpp +++ b/kmymoney/dialogs/transactionmatcher.cpp @@ -21,6 +21,7 @@ #include <klocale.h> #include "mymoneyfile.h" +#include "kmymoneyglobalsettings.h" TransactionMatcher::TransactionMatcher(const MyMoneyAccount& acc) : m_account(acc) @@ -73,6 +74,11 @@ void TransactionMatcher::match(MyMoneyTransaction tm, MyMoneySplit sm, MyMoneyTr throw MYMONEYEXCEPTION(i18n("Splits for %1 have conflicting values (%2,%3)", m_account.name(), MyMoneyUtils::formatMoney(sm.shares(), m_account, sec), MyMoneyUtils::formatMoney(si.shares(), m_account, sec))); } + // check that dates are within user's setting + if (abs(tm.postDate().toJulianDay() - ti.postDate().toJulianDay()) > KMyMoneyGlobalSettings::matchInterval()) { + throw MYMONEYEXCEPTION(i18n("The transaction post-dates are not within the 'matchInterval' setting.")); + } +
I noticed this bug report because I observed the impact of this fix which was released just a few days ago. The new behavior seems incorrect to me. I agree completely with Jack: the purpose of the transactions match setting should be for automated/system matching, not to prevent manual matching. I want to keep that setting short (4 days seems reasonable) to prevent unintended matching when downloading transactions. However, when managing paper checks, it is quite common that a check isn't deposited or cleared for many days or weeks -- certainly beyond that setting window. In such cases, I should be able to manually match the downloaded transaction to the transaction that I entered weeks earlier. Allan wrote: "a suitably wide setting for the match-window should allow you to match such transactions, shouldn't it?" Absolutely not. What's suitable for manual matching is completely unsuitable for automatic matching and vice versa. It would be fine for the program to prompt for confirmation when attempting to manually match two transactions whose dates differ by a greater amount than the configured match setting, but a user should be able to manually match without needing to go back to the original transaction and change its date or needing to change the setting to a value so high as to become useless for protecting against improper automated matching.
I have not actually tested the new version, and I'm only now reading the patch in deatil, but if this actually prevents manually matching transactions more than X days apart, I think it is not a good change. Depending on the actual intent (of this bug and the patch) I would hope an improved description of the value would serve the purpose. I don't simply want to set the value to an arbitrarily large amount, since that would likely (although I have not actually tested) allow automatic matching of inappropriate transactions. For example, I would need to set it to as much as two months - which I suspect would inappropriately perform an automatic match on monthly bills.
OK. I've changed the patch to output a message if, when manual matching, the dates differ by more than the 'matchInterval'. BUT...I have it do this if either transaction is a manually entered one - not if both are imported. Does anyone want to test the patch before I commit?
I'm not sure I follow your comment "BUT...I have it do this if either transaction is a manually entered one - not if both are imported." I thought you could only manually match a manually entered transaction with an imported one. I think there is probably a wishlist entry about that - but I don't think we need to mix the two issues, as long as we remember about this date issue when the other eventually gets addressed. Also, given this is changing the exception you already committed to a popup warning, I'd say go ahead and commit.
Allan, thanks for fixing your patch. What you described sounds much better. I'm happy to review & test your new revised patch, but as Jack wrote, if this is just a change from throwing the exception to generating a popup warning, it's probably safe to commit.
(In reply to Jack from comment #10) > I'm not sure I follow your comment "BUT...I have it do this if either > transaction is a manually entered one - not if both are imported." I > thought you could only manually match a manually entered transaction with an > imported one. I think there is probably a wishlist entry about that - but I > don't think we need to mix the two issues, as long as we remember about this > date issue when the other eventually gets addressed. > Ah, no. See 7e7a50e39170d03c9fe83ffcbe394d485cefaf5 - BUG:333949 Remove restrictions on matching of any imported or manual transactions. I think it was this to which Glenn was referring. > Also, given this is changing the exception you already committed to a popup > warning, I'd say go ahead and commit. I'm going to hold off for a few days. I want to confirm how matching two imported transactions reacts to differing dates.
Git commit bdf3da7846b18b567f46e48c95c1f49c26b4f945 by Allan Anderson. Committed on 10/05/2015 at 11:54. Pushed by allananderson into branch 'master'. On manual matching, output message if dates are not within KMyMoneyGlobalSettings::matchInterval and allow acceptance. M +8 -1 kmymoney/dialogs/transactionmatcher.cpp http://commits.kde.org/kmymoney/bdf3da7846b18b567f46e48c95c1f49c26b4f945
Git commit c4b774fb7be7784aecf7bb591ca93c22b3d86e1d by Allan Anderson. Committed on 08/06/2015 at 21:56. Pushed by allananderson into branch '4.7'. On manual matching, output message if dates are not within KMyMoneyGlobalSettings::matchInterval and allow acceptance. (cherry picked from commit bdf3da7846b18b567f46e48c95c1f49c26b4f945) M +8 -1 kmymoney/dialogs/transactionmatcher.cpp http://commits.kde.org/kmymoney/c4b774fb7be7784aecf7bb591ca93c22b3d86e1d
Git commit e63be4c9ada64d318e8939501fda984e6b82fd07 by Allan Anderson. Committed on 11/06/2015 at 21:47. Pushed by allananderson into branch 'master'. (The date is ignored when matching) Correction to message text which referred to importing. M +1 -1 kmymoney/dialogs/transactionmatcher.cpp http://commits.kde.org/kmymoney/e63be4c9ada64d318e8939501fda984e6b82fd07