Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Auto set Party in Bank Transaction #34675

Merged
merged 25 commits into from
Jun 20, 2023

Conversation

marination
Copy link
Collaborator

@marination marination commented Mar 30, 2023

Resolves part of #34477
Ref: alyf-de/banking#17
Docs: https://docs.erpnext.com/docs/v14/user/manual/en/accounts/bank-reconciliation

Issue

  • Bank Transaction holds almost no information about the Counter Party (the party we are paying/receiving from) as per the Bank Statement.
  • This counter party information is not likely to exactly match the Party in the system
  • Due to which the fields Party Type and Party are almost always blank. This makes identification hard in the Bank Reconciliation Tool
  • Bank Transactions are usually bulk imported, hence setting these values manually are painful

Enhancement

Note: This matching is done fairly conservatively. It's better to not have a match than to have multiple wrong matches.

This PR adds an optional automatic Party matcher.

  1. Enable it in Accounts Settings. Fuzzy matching can be optionally enabled
    Screenshot 2023-05-18 at 1 31 57 PM

  2. Register primary/most used bank details like Account Number & IBAN against Customer, Supplier & Employee via Bank Account or Employee DocType (for Employees). This is optional and will make the match more accurate if set by users.

  3. Added fields to hold Counter party information in Bank Transaction.(The population of these values will depend on integrations that import transactions or the users that upload the bank statement )
    Screenshot 2023-04-11 at 12 27 31 AM

  4. If Account details are absent in the Bank Transaction, fuzzy matching is optionally done using the Party Name (Bank Statement) or Description. These are matched against Customer/Supplier/Employee names.

  5. The matching is done and values are set on Submit if auto matching is enabled

  6. Users can correct the Party & Party Type, if incorrect, after submission. This will only impact the edited transaction.
    Screenshot 2023-04-11 at 1 18 19 AM

Working

Expand to view the working

Automatch Flow

  • If more than one match was found against a description/party name, with the same score, we skip matching and do not set a party in the bank transaction. We do not want to automate close calls, the user must decide these.

TODO:

  • Provision for user to correct mapping
  • Provision for users to create a mapping (they can manually set the party in the transaction & also create a Bank Party Mapper)
  • Test
  • Docs
  • Description
  • Use old bank fields to get party against bank data. Don't introduce new ways of storing bank data.
  • Optional fuzzy matching + If multiple similar matches, skip automatching

For the reviewer: Hit me up for test data

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Mar 30, 2023
- Created Bank Party Mapper
- Created class to auto match by account/iban or party name/description(fuzzy)
- Automatch and set in transaction or create mapper
- `rapidfuzz` introduced
…tion

- Description is volatile and will keep changing
- It will lead to multiple Bank Party Mapper docs for the same party that will never be referenced again
- Parts of the descripton keep changing which is why it will never match a mapper record
- If matched by desc, dont create mapper record.
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #34675 (8ab8230) into develop (b26d70b) will increase coverage by 0.04%.
The diff coverage is 91.58%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #34675      +/-   ##
===========================================
+ Coverage    64.12%   64.17%   +0.04%     
===========================================
  Files          810      811       +1     
  Lines        61899    62006     +107     
===========================================
+ Hits         39695    39793      +98     
- Misses       22204    22213       +9     
Impacted Files Coverage Δ
...ounts/doctype/bank_transaction/bank_transaction.py 68.75% <90.00%> (+1.28%) ⬆️
...ounts/doctype/bank_transaction/auto_match_party.py 91.75% <91.75%> (ø)

- On updating bank trans.n party after submit, the corresponding mapper doc will be updated too
- The mapper doc in turn will update all linked bank transactions that do not have this updated value
- Added Bank Party Mapper hidden link in Bank Transaction
- Rename field in BPM to `Party Name` as it does not hold description data
- If a BT matches with a BPM record, link that record in the BT
- misc: Clearer naming
- Checkbox in Accounts settings "Enable Automatic Party Matching"
- Check before invoking automatching methods
- misc: Remove TODO comments
@marination marination linked an issue Apr 4, 2023 that may be closed by this pull request
- A BT could have both account and iban, and a Supplier could have only IBAN set
- In this case, matching by either (only account) gives no match
- Match by Account OR IBAN, use `or_filters`
- If matched, set both account no. and IBAN in Bank Party Mapper

- Explain AutoMatchParty
- Add type hints to return values
- Use `set_value` to set values in BT after matching since its an after submit event
@marination marination added the dont squash Don't squash and merge label Apr 5, 2023
@marination marination added needs-docs and removed needs-tests This PR needs automated unit-tests. labels Apr 10, 2023
@marination marination marked this pull request as ready for review April 10, 2023 20:00
@marination marination linked an issue Apr 10, 2023 that may be closed by this pull request
pyproject.toml Show resolved Hide resolved
@ankush ankush removed the request for review from rohitwaghchaure April 11, 2023 07:33
…ches

- Fuzzy matching can be enabled optionally in the settings
- If a query gets multiple matches with the same score, do not set a party as it is an extremely close call
- misc: Add 'cancelled' status to Bank transaction
- Test for skipping matching with extremely close matches
@marination marination marked this pull request as ready for review May 18, 2023 07:36
@marination
Copy link
Collaborator Author

@deepeshgarg007 Docs and description has been updated. The latest changes resolve the following ambiguities:

Why add bank details to Customer and Supplier instead of fetching linked Bank Accounts?

Bank details are searched for in Bank Account now. In the case of Employee, the Bank Account is searched and then the Employee doctype as it has bank account fields in it historically.
No new ways to store bank details

@vorasmit's concern: "For a client, we did the recent implementation, they have over 20k customers. Many are similar and need suffixes as to from which area they belong."

I did brainstorm this quite a bit and discussed this with @barredterra. Smit suggested rules but that would cause users to create way too many rules to match their plethora of parties. Could not really come up with anything else that wasn't bad UX.

So fuzzy matching is optional. In the docs, users are encouraged to test the feature out first. The matching might be a wee bit problematic where extremely similar names exist (e.g. Adithya Medical& Surgical vs Adithya Medical & Surgical vs Adithya Medical And Surgical) but will work great for data where there is not so much similarity.

I also increased the match score cutoff to 80.

Additionally fuzzy matching does not set any value if multiple matches with the same score were obtained for a query.
E.g. if the query in the Bank Transaction is "A.K. Medical" and two parties exist such as "A.K. Medical Hall" and "A.K. Medical Store", both will be accurate matches. In this case we do not make the system decide between close calls and leave it blank for the user to set it themselves or not set it at all.

@marination marination marked this pull request as draft June 6, 2023 13:20
- Matching by Acc No/IBAN can easily happen with Bank Accounts. It's not a tedious query
- Historical lookups for  Party Name/Desc match are very tricky. The user could have manually set a match and we would not know. Also this leaves the Bank Party Mapper only useful for Party Name/Desc lookups, which feels excessive.
- We want to reduce the number of places the same data is stored and reduce confusion
- The Party Name/Desc will optionally happen fuzzily, or not at all
- There will be no Mapper lookups
@barredterra barredterra marked this pull request as ready for review June 15, 2023 15:06
Copy link
Collaborator

@barredterra barredterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested matching by party name, description and IBAN. LGTM.

Todo: Update docs and PR description (to not confuse future readers).

@marination
Copy link
Collaborator Author

@barredterra docs and PR description updated ✅

@barredterra barredterra added the backport version-14-hotfix backport to version 14 label Jun 19, 2023
@deepeshgarg007 deepeshgarg007 merged commit 32c35b8 into develop Jun 20, 2023
13 checks passed
@barredterra barredterra deleted the bank-trans-party-automatch branch June 20, 2023 08:42
deepeshgarg007 pushed a commit that referenced this pull request Jun 24, 2023
* feat: Party auto-matcher from Bank Transaction data

- Created Bank Party Mapper
- Created class to auto match by account/iban or party name/description(fuzzy)
- Automatch and set in transaction or create mapper
- `rapidfuzz` introduced

* chore: Single query with or filter to search Party Mapper by name/desc

* feat: Store Party bank details in party records (Customer/Supplier/Employee/Shareholder)

* fix: Don't set description as key in Mapper doc if matched by description

- Description is volatile and will keep changing
- It will lead to multiple Bank Party Mapper docs for the same party that will never be referenced again
- Parts of the descripton keep changing which is why it will never match a mapper record
- If matched by desc, dont create mapper record.

* feat: Manually Update/Correct Party in Bank Transaction

- On updating bank trans.n party after submit, the corresponding mapper doc will be updated too
- The mapper doc in turn will update all linked bank transactions that do not have this updated value
- Added Bank Party Mapper hidden link in Bank Transaction
- Rename field in BPM to `Party Name` as it does not hold description data
- If a BT matches with a BPM record, link that record in the BT

* chore: Perform automatch on submit

- misc: Clearer naming

* chore: Make auto matching party configurable

- Checkbox in Accounts settings "Enable Automatic Party Matching"
- Check before invoking automatching methods
- misc: Remove TODO comments

* fix: Match by both Account No and IBAN & other cleanups

- A BT could have both account and iban, and a Supplier could have only IBAN set
- In this case, matching by either (only account) gives no match
- Match by Account OR IBAN, use `or_filters`
- If matched, set both account no. and IBAN in Bank Party Mapper

- Explain AutoMatchParty
- Add type hints to return values
- Use `set_value` to set values in BT after matching since its an after submit event

* test: Match by Account No, IBAN, Party Name, Desc and match correction

* fix: Remove bank details fields from Shareholder

* fix: Use existing bank fields to match by bank account no/IBAN

- Remove newly added fields in Party doctypes to store bank details
- Use Bank Account's fields to match against account no/iban
- For employee, if Bank Account does not exist, find in Employee doctype against account no/iban

* fix: Tests

* feat: Optional Fuzzy Matching & Skip Matches for multiple similar matches

- Fuzzy matching can be enabled optionally in the settings
- If a query gets multiple matches with the same score, do not set a party as it is an extremely close call
- misc: Add 'cancelled' status to Bank transaction
- Test for skipping matching with extremely close matches

* chore: Remove Bank Party Mapper implementation

- Matching by Acc No/IBAN can easily happen with Bank Accounts. It's not a tedious query
- Historical lookups for  Party Name/Desc match are very tricky. The user could have manually set a match and we would not know. Also this leaves the Bank Party Mapper only useful for Party Name/Desc lookups, which feels excessive.
- We want to reduce the number of places the same data is stored and reduce confusion
- The Party Name/Desc will optionally happen fuzzily, or not at all
- There will be no Mapper lookups

* chore: Remove instances of `bank_party_mapper` and use `new_doc`
frappe-pr-bot pushed a commit that referenced this pull request Jun 28, 2023
# [14.28.0](v14.27.14...v14.28.0) (2023-06-28)

### Bug Fixes

* asset capitalization ([#35832](#35832)) ([fb823b5](fb823b5))
* asset movement ([#35918](#35918)) ([e16c148](e16c148))
* delivery trip driver is only required on submit (backport [#35876](#35876)) ([#35893](#35893)) ([fc051d1](fc051d1))
* don't allow to make reposting entry for closing stock balance period ([e68b088](e68b088))
* filter parent warehouses not showing (backport [#35897](#35897)) ([#35899](#35899)) ([87ba196](87ba196))
* incorrect cost center error in bank reconciliation ([cacb0f6](cacb0f6))
* issue of asset value_after_depreciation field getting updated twice if workflow is enabled in Journal Entry (backport [#35821](#35821)) ([#35827](#35827)) ([20f2bef](20f2bef))
* make credit note and debit note exclusive ([#35781](#35781)) ([fafb46e](fafb46e))
* multiple Work Orders agaist same production plan ([8ddfc34](8ddfc34))
* no permission for accounts settings on payment reconciliation ([200ddbf](200ddbf))
* Payment Term must be mandatory if `Allocate Payment based on ..` is checked ([#35798](#35798)) ([3dd3935](3dd3935))
* POS Closing Entry load all invoices with one request on save ([#35819](#35819)) ([8ecca2a](8ecca2a))
* reconcile invoice against credit note. ([#35604](#35604)) ([5c388a1](5c388a1))
* Remove special treatment for P&L Accounts ([#35602](#35602)) ([b023448](b023448))
* Set Asset cost center default as PR or PI Item Cost Center while auto creating ([#35844](#35844)) ([4a7d75b](4a7d75b))
* show non-depreciable assets in fixed asset register ([#35858](#35858)) ([42d0944](42d0944))
* TDS amount calculation post LDC breach ([851b887](851b887))
* use correct fieldname for purchase receipt column in item_wise_purchase_register report ([#35828](#35828)) ([8f13b48](8f13b48))
* **ux:** PO Get Items From Open Material Requests (backport [#35894](#35894)) ([#35895](#35895)) ([2ef2057](2ef2057))

### Features

* Auto set Party in Bank Transaction ([#34675](#34675)) ([d53b197](d53b197))
* Provision to send Accounts Receivable Reports using Process SOA ([#35789](#35789)) ([21d560c](21d560c)), closes [#35707](#35707)

### Performance Improvements

* improve item wise register reports ([#35908](#35908)) ([33ee011](33ee011))
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 dont squash Don't squash and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Bank Reconciliation Automatically link Party Type and Party
6 participants