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

fix: long name variable #38972

Closed
wants to merge 5 commits into from
Closed

fix: long name variable #38972

wants to merge 5 commits into from

Conversation

RJPvT
Copy link
Contributor

@RJPvT RJPvT commented Dec 27, 2023

some banks no longer give check_numbers or meta data so the entire string is returned in 'name'. this gives an ExcededLength error.

btw, the truncated error gives an throw so no bank statement is created

frappe.exceptions.CharacterLengthExceededError: Bank Transaction ACC-BTN-2023-01109: 'Reference Number' (SEPA Overboeking IBAN: NL61RABO0111531012 BIC: RABONL2U Naam: NS Groep N.V. Omschrijving: FACTUUR2023-00427I NV-2023-00418 Kenmerk: 2000004896) will get truncated, as max characters allowed is 140

some banks no longer give check_numbers or meta data so the entire string is returned in 'name'. this gives an ExcededLength error
@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Dec 27, 2023
@@ -264,7 +264,7 @@ def new_bank_transaction(transaction):
"reference_number": (
transaction["check_number"]
or transaction["payment_meta"]["reference_number"]
or transaction["name"]
or transaction["name"][:20]
Copy link
Member

@ankush ankush Dec 29, 2023

Choose a reason for hiding this comment

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

Suggested change
or transaction["name"][:20]
or transaction["name"][:140]

Ideally you should strip to around ~135 characters and add ... to indicate it's stripped.

OR increase length

OR convert this field to small text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

convert so small text makes no sense; that would just lead to double information storage. 20 was chosen because of standard international banking check length. addition of [...] would be good though, want me to add that ?
btw. removing [name] is also an option. in v13 this field was also empty.

@RJPvT
Copy link
Contributor Author

RJPvT commented Jan 2, 2024

@ankush , please conform solution direction so this can be merged. Its breaking for our bank (ABN AMRO) now.

Copy link

stale bot commented Jan 17, 2024

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jan 17, 2024
@stale stale bot closed this Jan 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
inactive needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants