Fix for #397

[jaapjansma]

Keep probability of already found contacts and do not overwrite them … with the bank account reference matching probability

See #397

[bjendres]

Wouldn't it make sense to add a configuration options for this slight change of behaviour?

I'm a little worried that there might be rare cases where this would change the results in some automated processes and things would go wrong without people noticing right away. Or are you 100% that this is not the case?

[jaapjansma]

I can understand your worry. But I believe the original behavior is undesired. At Velt it turned out that things which first went automatically now are stopped for being processed automatically.

As fas I understand we have to chose between the following two behaviours:

1. Match on bank account and make the probability setting of this leading and overriding the contact_id or external_id from the transaction data. Meaning that if other matchers or analysers are setting a contact_id this will be set to the probablity of setting of the bank account when the contact also has a bank account.
2. Match on bank account and only make the probability setting take into account , but if contact_id or external_id are set in the transaction data give those a presence of probability of 1.0

For me option 1 makes senses but does generate unexpected results I believe which are difficult to detect by the user.

With option 2 you can still create a situation like option 1 with a penalty setting, that when contact_id is present in the transaction data then give it a penalty.

Off-topic: I am against creating a setting for this. Because I believe that create a setting for this is because you feel insecure about this change. And your doubt might be for a good reason but lets discuss those in the topic. I think you have a couple of options. The first one is whether you want to act as the owner (and deciding boss) on this extension. Then you can make a call on this PR. The other chose you have is whether you want this extension to be community driven and then you have to let go the control (you can still give a review and your opinion which is very valuable).

[bjendres]

I see your point, but for me it's paramount that existing automated processes won't change through an update. A lot of money passes through this, and an unnoticed error might be very painful for the user/organisation.

@jaapjansma can you think of any way to ensure this?

[jaapjansma]

Yes I understand that. And my point is that in the scenario where you have set the bank account reference matching probability to 0.9 (or anything below 1). It means it already stopped the automatic processing. That behavior will not be changed.

Unless: an organization has an analyzer or marcher which lookup the contact based on other data. In that case the automation is broken right now, but will start again after this fix. Your question is whether we can keep this broken. Not sure if I understand that correctly.

Do you have a scenario in mind at which my fix breaks?

[bjendres]

Do you have a scenario in mind at which my fix breaks?

I think as maintainers our job is to keep the behaviour as consistent as possible because of the reasons outlined above. An additional setting would ensure that, and me failing to come up with scenario that breaks doesn't.

So I still think a setting is the safest and easiest way. Should you not have the time to implement this, I can do it.

Anyone else care to weigh in on this?

[jaapjansma]

Ok. Time is not the issue for me but I do not believe in a solution for a setting. I will leave the PR here and will continue to keep a fork of banking for Velt at https://lab.civicrm.org/partners/civicoop/velt/org.project60.banking

[bjendres]

Ok. Time is not the issue for me but I do not believe in a solution for a setting. I will leave the PR here and will continue to keep a fork of banking for Velt at https://lab.civicrm.org/partners/civicoop/velt/org.project60.banking

That's fine. I'll add the setting on this repository and then merge this PR anyway, so we have it in master.