Fetch bank transactions#33
Conversation
…on into fetch-bank-transactions
Confidence Score: 3/5Not safe to merge — three P1 defects will cause runtime errors and broken UX. Two NameError bugs will crash at runtime when get_element() fails in except handlers; a missing completion event silently leaves the UI in a stale state; listener accumulation causes duplicate UI events on every form load. bank_integration/bank_integration/api/hdfc_bank_api.py and bank_integration/scripts/bank_reconciliation_tool.js
|
| Filename | Overview |
|---|---|
| bank_integration/bank_integration/api/hdfc_bank_api.py | Major refactor replacing HTML scraping with CSV download and date-range chunking; introduces two NameError bugs in except blocks and a missing completion event for empty results. |
| bank_integration/scripts/bank_reconciliation_tool.js | Adds date range UI, validation, and realtime listeners; listener accumulation in onload causes duplicate toast messages on repeat form visits. |
| bank_integration/bank_integration/api/transactions.py | Clean addition of optional from_date/to_date parameters to get_transactions; no issues found. |
| bank_integration/public/js/common.js | Minor UX improvement to show OTP verification as a toast alert for Bank Reconciliation Tool and Bank Account forms. |
| bank_integration/patches.txt | Routine patch version counter bump. |
Reviews (1): Last reviewed commit: "fix: made changes to support fetch and r..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe HDFC bank integration was refactored from HTML/enquiry parsing to a statement-download pipeline that chunks date ranges, automates portal downloads, parses CSV-like statements, and creates Bank Transaction records with deduplicated transaction_id hashes. The get_transactions API now accepts optional from_date and to_date. Client-side OTP feedback and the Bank Reconciliation Tool form were updated with new lifecycle handlers, validators, realtime listeners, and UI behaviors. The package patch version comment was adjusted. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
bank_integration/bank_integration/api/transactions.py (1)
14-24: Consider adding date validation for the new parameters.The
from_dateandto_dateparameters are passed through without validation. While downstream code may handle invalid values, validating at the API boundary provides clearer error messages and prevents unnecessary bank portal interactions.Optional validation example
`@frappe.whitelist`() def get_transactions(uid, from_account, from_date=None, to_date=None): + from frappe.utils import getdate + if from_date: + from_date = str(getdate(from_date)) + if to_date: + to_date = str(getdate(to_date)) + if from_date and to_date and from_date > to_date: + frappe.throw("From Date cannot be greater than To Date") bi = frappe.get_doc("Bank Integration Settings", from_account)bank_integration/scripts/bank_reconciliation_tool.js (1)
28-34: Settingbank_statement_to_dateon everyonloadmay overwrite user's selection.The
onloadhandler unconditionally setsbank_statement_to_dateto yesterday. This will reset any user-selected date when the form is refreshed or reloaded, which may be unexpected.Only set default if field is empty
frm.toggle_reqd(["bank_statement_to_date","bank_statement_from_date","bank_account"],1) let today = frappe.datetime.get_today(); let yesterday = frappe.datetime.add_days(today, -1); - frm.set_value("bank_statement_to_date",yesterday); + if (!frm.doc.bank_statement_to_date) { + frm.set_value("bank_statement_to_date", yesterday); + }bank_integration/bank_integration/api/hdfc_bank_api.py (3)
1033-1033: Unused parameterfrom_date.The
from_dateparameter is declared but never used. The actual dates are retrieved fromself.data.from_dateandself.data.to_date. This parameter should be removed to avoid confusion.Remove unused parameter
- def fetch_transactions(self, from_date=None): + def fetch_transactions(self):
1212-1258: Consider transaction safety for bulk creation.The method submits each
Bank Transactionindividually and commits once at the end. If an error occurs mid-way (e.g., validation failure on one transaction), some transactions will be in the database without being committed, while others won't be created at all.Consider wrapping in a try-except with
frappe.db.rollback()on failure, or usingfrappe.flags.in_import = Trueto batch operations.
1101-1138: Remove the unused_process_downloaded_statementmethod.The
_process_downloaded_statementmethod at lines 1101–1138 is defined but never called anywhere in the codebase. Remove it unless it's intended for future use, in which case document its purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b18d0670-2454-440e-a132-90259ee01a79
📒 Files selected for processing (5)
bank_integration/bank_integration/api/hdfc_bank_api.pybank_integration/bank_integration/api/transactions.pybank_integration/patches.txtbank_integration/public/js/common.jsbank_integration/scripts/bank_reconciliation_tool.js
| def _estimate_wait_time(self): | ||
| """Estimate wait time in minutes based on the date range.""" | ||
| from_date = datetime.strptime(self.data.from_date, "%Y-%m-%d") | ||
| to_date = datetime.strptime(self.data.to_date, "%Y-%m-%d") | ||
| days = (to_date - from_date).days | ||
|
|
||
| if days <= 90: | ||
| return 1 | ||
| elif days <= 365: | ||
| return 2 | ||
| else: | ||
| return 5 |
There was a problem hiding this comment.
Missing null check for date parameters.
_estimate_wait_time directly parses self.data.from_date and self.data.to_date without checking if they are None. If the API is called without dates, this will raise a TypeError.
Proposed fix to add defensive check
def _estimate_wait_time(self):
"""Estimate wait time in minutes based on the date range."""
+ if not self.data.from_date or not self.data.to_date:
+ return 2 # default wait time
from_date = datetime.strptime(self.data.from_date, "%Y-%m-%d")
to_date = datetime.strptime(self.data.to_date, "%Y-%m-%d")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _estimate_wait_time(self): | |
| """Estimate wait time in minutes based on the date range.""" | |
| from_date = datetime.strptime(self.data.from_date, "%Y-%m-%d") | |
| to_date = datetime.strptime(self.data.to_date, "%Y-%m-%d") | |
| days = (to_date - from_date).days | |
| if days <= 90: | |
| return 1 | |
| elif days <= 365: | |
| return 2 | |
| else: | |
| return 5 | |
| def _estimate_wait_time(self): | |
| """Estimate wait time in minutes based on the date range.""" | |
| if not self.data.from_date or not self.data.to_date: | |
| return 2 # default wait time | |
| from_date = datetime.strptime(self.data.from_date, "%Y-%m-%d") | |
| to_date = datetime.strptime(self.data.to_date, "%Y-%m-%d") | |
| days = (to_date - from_date).days | |
| if days <= 90: | |
| return 1 | |
| elif days <= 365: | |
| return 2 | |
| else: | |
| return 5 |
| def _get_date_chunks(self): | ||
| """Break self.data.from_date → self.data.to_date into ≤365-day chunks.""" | ||
| from_dt = datetime.strptime(self.data.from_date, "%Y-%m-%d") | ||
| to_dt = datetime.strptime(self.data.to_date, "%Y-%m-%d") | ||
| chunks = [] | ||
| current = from_dt | ||
| while current <= to_dt: | ||
| end = min(current + timedelta(days=364), to_dt) | ||
| chunks.append((current, end)) | ||
| current = end + timedelta(days=1) | ||
| return chunks |
There was a problem hiding this comment.
Same null-check concern as _estimate_wait_time.
This method also parses dates without checking for None. Consider adding defensive validation here or ensuring dates are validated before calling these methods.
| if not rows: | ||
| return [] | ||
|
|
||
| raw_headers = [h.strip().lower() for h in rows[1]] |
There was a problem hiding this comment.
Potential IndexError on malformed CSV files.
If the CSV has only one row (or is empty after parsing), accessing rows[1] on line 1152 will raise IndexError. The check on line 1149 only verifies the list is non-empty.
Proposed fix to add bounds check
if not rows:
return []
+ if len(rows) < 2:
+ return []
+
raw_headers = [h.strip().lower() for h in rows[1]]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not rows: | |
| return [] | |
| raw_headers = [h.strip().lower() for h in rows[1]] | |
| if not rows: | |
| return [] | |
| if len(rows) < 2: | |
| return [] | |
| raw_headers = [h.strip().lower() for h in rows[1]] |
| if(frm.doctype == 'Bank Reconciliation Tool' || frm.doctype == 'Bank Account'){ | ||
| frappe.show_alert({ | ||
| message: "Verifying OTP!!", | ||
| indicator: "green" | ||
| },5) | ||
| }else{ | ||
| frappe.msgprint("Verifying OTP!!") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how frm object is structured when passed to listenForOtp
rg -n -A5 'bi\.listenForOtp' --type=jsRepository: resilient-tech/bank_integration
Length of output: 3055
🏁 Script executed:
sed -n '1,60p' bank_integration/public/js/common.js | cat -nRepository: resilient-tech/bank_integration
Length of output: 2306
Use consistent doctype access pattern on line 37.
Line 33 correctly uses frm?.doc?.doctype || frm?.doctype to handle both form objects (where doctype is at frm.doc.doctype) and listview objects (where doctype is at frm.doctype). However, line 37's condition only checks frm.doctype, which is undefined for form objects, causing the condition to always evaluate to false and skip the intended frappe.show_alert behavior. This affects most calls to listenForOtp which pass form objects (from payment_entry.js, bank_reconciliation_tool.js, and bank_integration_settings.js).
Proposed fix to align with the existing pattern
+ const doctype = frm?.doc?.doctype || frm?.doctype;
- if(frm.doctype == 'Bank Reconciliation Tool' || frm.doctype == 'Bank Account'){
+ if(doctype == 'Bank Reconciliation Tool' || doctype == 'Bank Account'){📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if(frm.doctype == 'Bank Reconciliation Tool' || frm.doctype == 'Bank Account'){ | |
| frappe.show_alert({ | |
| message: "Verifying OTP!!", | |
| indicator: "green" | |
| },5) | |
| }else{ | |
| frappe.msgprint("Verifying OTP!!") | |
| } | |
| const doctype = frm?.doc?.doctype || frm?.doctype; | |
| if(doctype == 'Bank Reconciliation Tool' || doctype == 'Bank Account'){ | |
| frappe.show_alert({ | |
| message: "Verifying OTP!!", | |
| indicator: "green" | |
| },5) | |
| }else{ | |
| frappe.msgprint("Verifying OTP!!") | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
bank_integration/scripts/bank_reconciliation_tool.js (1)
30-32: Only defaultbank_statement_to_datewhen it's blank.Reopening the tool currently replaces any previously selected/saved end date with yesterday, which can silently shift the next sync window.
♻️ Proposed fix
let today = frappe.datetime.get_today(); let yesterday = frappe.datetime.add_days(today, -1); - frm.set_value("bank_statement_to_date",yesterday); + if (!frm.doc.bank_statement_to_date) { + frm.set_value("bank_statement_to_date", yesterday); + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3443d3fd-7a88-43c9-a32b-3350f727ab1c
📒 Files selected for processing (2)
bank_integration/bank_integration/api/hdfc_bank_api.pybank_integration/scripts/bank_reconciliation_tool.js
| def _click_statement_download(self, from_date_ui, to_date_ui): | ||
| """Find the statement row matching the date range in the recent downloads | ||
| table and click its download button. Returns True if found, False otherwise.""" |
There was a problem hiding this comment.
Don't re-request a statement that's already pending.
A matching recent-download row can exist without a download button while HDFC is still generating the file. Returning False here sends _fetch_chunk() down the fresh-download path, so each retry can submit another statement request for the same range.
🐛 Proposed fix
- """Find the statement row matching the date range in the recent downloads
- table and click its download button. Returns True if found, False otherwise."""
+ """Find the statement row matching the date range in the recent downloads
+ table and click its download button.
+ Returns True when a download starts, None when a matching row is still
+ pending, and False when no matching row exists."""
@@
- if not file_download_btn:
- return False
+ if not file_download_btn:
+ return None
@@
- if self._click_statement_download(from_date_ui, to_date_ui):
+ download_status = self._click_statement_download(from_date_ui, to_date_ui)
+ if download_status is True:
filename, content = self.wait_for_download()
if filename and content:
return self._parse_statement_file(content)
return []
+ elif download_status is None:
+ return NoneAlso applies to: 764-768, 1006-1041
| frappe.publish_realtime( | ||
| "show_alert", | ||
| { | ||
| "message": "Checking statement for {} to {}...".format( | ||
| chunk_from.strftime("%d %b %Y"), chunk_to.strftime("%d %b %Y") | ||
| ) | ||
| }, | ||
| user=frappe.session.user, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "show_alert publishers:"
rg -n -C2 'publish_realtime\("show_alert"' --type=py
echo
echo "show_alert listeners:"
rg -n -C2 'realtime\.on\("show_alert"' --type=jsRepository: resilient-tech/bank_integration
Length of output: 97
🏁 Script executed:
# First, verify the file exists and check the specific lines
file_path="bank_integration/bank_integration/api/hdfc_bank_api.py"
if [ -f "$file_path" ]; then
echo "=== File exists ==="
echo "Total lines: $(wc -l < "$file_path")"
echo ""
echo "=== Lines 1055-1070 (context around 1059-1067) ==="
sed -n '1055,1070p' "$file_path"
echo ""
echo "=== Lines 1088-1110 (context around 1092-1106) ==="
sed -n '1088,1110p' "$file_path"
echo ""
echo "=== Lines 1182-1195 (context around 1186-1190) ==="
sed -n '1182,1195p' "$file_path"
else
echo "File not found at $file_path"
# Try to find it
find . -name "hdfc_bank_api.py" -type f 2>/dev/null
fiRepository: resilient-tech/bank_integration
Length of output: 2231
🏁 Script executed:
# Search for all show_alert occurrences
echo "=== All 'show_alert' occurrences in Python files ==="
rg "show_alert" --type=py -n -B2 -A2
echo ""
echo "=== All 'show_alert' occurrences in JavaScript files ==="
rg "show_alert" --type=js -n -B2 -A2Repository: resilient-tech/bank_integration
Length of output: 2906
🏁 Script executed:
# Look for sync_transactions for comparison
echo "=== 'sync_transactions' context (mentioned in review as comparison) ==="
rg "sync_transactions" --type=py -n -B2 -A2 | head -50Repository: resilient-tech/bank_integration
Length of output: 644
🏁 Script executed:
# Search for uid usage in the context of these functions
echo "=== 'uid' usage in bank_reconciliation or hdfc_bank_api ==="
rg "uid" --type=py -n | grep -E "(bank_reconciliation|hdfc_bank)" | head -20Repository: resilient-tech/bank_integration
Length of output: 828
🏁 Script executed:
# Verify self.uid is available in the context of these methods
echo "=== Methods containing show_alert calls ==="
rg -B 20 'publish_realtime\(\s*"show_alert"' --type=py | grep -E "(def |self\.uid)" | head -20
echo ""
echo "=== Confirm sync_transactions uses uid ==="
sed -n '1076,1082p' bank_integration/bank_integration/api/hdfc_bank_api.pyRepository: resilient-tech/bank_integration
Length of output: 440
Add uid parameter to all show_alert events to prevent progress bleed across concurrent syncs.
The three show_alert publishers lack uid while sync_transactions (line 1080) includes it. The JavaScript listener in bank_reconciliation_tool.js (line 35) doesn't filter by uid, so progress alerts from unrelated syncs can overwrite active form messages.
Affected locations
- Line 1060: show_alert during chunk processing
- Line 1093: show_alert for pending statement
- Line 1187: show_alert for creating bank transactions
| txn["reference_number"] = str(txn.get("reference_number", "")).replace( | ||
| ".0", "" | ||
| ) |
There was a problem hiding this comment.
Only trim a terminal numeric .0 from reference_number.
replace(".0", "") removes every .0 substring, so a legitimate value like 12.01 becomes 121. That also changes the dedupe hash built in _create_bank_transactions().
🐛 Proposed fix
- txn["reference_number"] = str(txn.get("reference_number", "")).replace(
- ".0", ""
- )
+ reference_number = str(txn.get("reference_number", "")).strip()
+ txn["reference_number"] = (
+ reference_number[:-2]
+ if reference_number.endswith(".0") and reference_number[:-2].isdigit()
+ else reference_number
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| txn["reference_number"] = str(txn.get("reference_number", "")).replace( | |
| ".0", "" | |
| ) | |
| reference_number = str(txn.get("reference_number", "")).strip() | |
| txn["reference_number"] = ( | |
| reference_number[:-2] | |
| if reference_number.endswith(".0") and reference_number[:-2].isdigit() | |
| else reference_number | |
| ) |
| existing_ids = set( | ||
| item["transaction_id"] | ||
| for item in frappe.get_all( | ||
| "Bank Transaction", | ||
| filters={"bank_account": bank_account}, | ||
| fields="transaction_id", | ||
| ) | ||
| ) | ||
|
|
||
| # Remaining rows are data | ||
| for row in rows[1:]: | ||
| cells = row.find_all("td") | ||
| count = 0 | ||
| for txn in transactions: | ||
| hash_str = "{date}{narration}{debit}{credit}{closing_balance}{reference_number}".format( | ||
| **txn | ||
| ) | ||
| transaction_id = hashlib.sha224(hash_str.encode()).hexdigest() | ||
|
|
||
| # NOTE: Will not happen right? | ||
| if len(cells) != len(headers): | ||
| continue # Skip incomplete rows | ||
| if transaction_id in existing_ids: | ||
| continue | ||
|
|
||
| transaction = { | ||
| header: (cell.text.strip() or 0) for header, cell in zip(headers, cells) | ||
| } | ||
| bank_transaction = frappe.get_doc({"doctype": "Bank Transaction"}) | ||
| bank_transaction.update( | ||
| { | ||
| "transaction_id": transaction_id, | ||
| "date": txn["date"], | ||
| "description": txn["narration"], | ||
| "withdrawal": txn["debit"], | ||
| "deposit": txn["credit"], | ||
| "reference_number": txn["reference_number"], | ||
| "closing_balance": txn["closing_balance"], | ||
| "bank_account": bank_account, | ||
| "unallocated_amount": abs(txn["credit"] - txn["debit"]), | ||
| } | ||
| ) | ||
| bank_transaction.submit() | ||
| count += 1 | ||
|
|
||
| transactions.append(transaction) | ||
| existing_ids.add(transaction_id) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Schema / migrations related to transaction_id uniqueness:"
rg -n -C3 '"fieldname"\s*:\s*"transaction_id"|add_unique|CREATE UNIQUE|unique"\s*:\s*(1|true)' -g '*.json' -g '*.py'
echo
echo "Existing duplicate-insert handling:"
rg -n -C3 'DuplicateEntryError|transaction_id' --type=pyRepository: resilient-tech/bank_integration
Length of output: 6333
🏁 Script executed:
#!/bin/bash
# Find and check the Bank Transaction doctype definition
echo "=== Bank Transaction doctype definition ==="
find . -name "*bank_transaction*" -type f | grep -E '\.(json|py)$'
echo
echo "=== Checking for Bank Transaction JSON doctype ==="
if [ -f "bank_integration/bank_integration/doctype/bank_transaction/bank_transaction.json" ]; then
rg -n "transaction_id|unique" bank_integration/bank_integration/doctype/bank_transaction/bank_transaction.json
fi
echo
echo "=== Looking for any unique constraint on transaction_id in all JSON files ==="
rg -n '"fieldname"\s*:\s*"transaction_id"' -A 10 -g '*.json'
echo
echo "=== Checking for error handling around submit() in hdfc_bank_api.py ==="
sed -n '1200,1230p' bank_integration/bank_integration/api/hdfc_bank_api.pyRepository: resilient-tech/bank_integration
Length of output: 1478
🏁 Script executed:
#!/bin/bash
echo "=== Search for Bank Transaction doctype definition in Python ==="
rg -n "doctype.*Bank Transaction|\"Bank Transaction\"" --type=py | head -30
echo
echo "=== Check install.py for complete Bank Transaction configuration ==="
rg -n -B5 -A15 '"doctype": "Bank Transaction"' bank_integration/install.py
echo
echo "=== List all files related to bank_transaction ==="
find . -iname "*bank*transaction*" -type f
echo
echo "=== Check for frappe.get_doc doctype calls for Bank Transaction ==="
rg -n "get_doc.*Bank Transaction" --type=pyRepository: resilient-tech/bank_integration
Length of output: 2407
🏁 Script executed:
#!/bin/bash
echo "=== Check if Bank Transaction is a standard Frappe doctype ==="
rg -n "Bank Transaction" bank_integration/ --type=py | head -20
echo
echo "=== Search for any schema/migration/unique constraint files ==="
find . -name "*.py" -type f | xargs grep -l "unique\|constraint" | head -10
echo
echo "=== Check if there are any doctypes folder ==="
find . -type d -name "doctype"
echo
echo "=== Look for all JSON doctype definitions ==="
find . -name "*.json" -type f | xargs grep -l '"doctype"' | head -10Repository: resilient-tech/bank_integration
Length of output: 1495
🏁 Script executed:
#!/bin/bash
echo "=== List all items in doctype folder ==="
ls -la bank_integration/bank_integration/doctype/
echo
echo "=== Find all JSON files in doctype folder ==="
find bank_integration/bank_integration/doctype/ -name "*.json" -type f
echo
echo "=== Check install.py for complete property setter configuration ==="
cat bank_integration/install.py | head -60
echo
echo "=== Search for permission records in install.py ==="
sed -n '244,280p' bank_integration/install.pyRepository: resilient-tech/bank_integration
Length of output: 2663
🏁 Script executed:
#!/bin/bash
echo "=== Check complete custom_fields definition ==="
rg -n "custom_fields\s*=" -A 50 bank_integration/install.py | head -100
echo
echo "=== Check if Bank Transaction is standard Frappe doctype ==="
rg -n "Bank Transaction" --type=py | grep -E "create|define|doctype" | head -10
echo
echo "=== Check entire hdfc_bank_api.py function for error handling ==="
sed -n '1175,1235p' bank_integration/bank_integration/api/hdfc_bank_api.pyRepository: resilient-tech/bank_integration
Length of output: 4295
🏁 Script executed:
#!/bin/bash
echo "=== Check if Bank Transaction is defined anywhere in custom_fields ==="
rg -n "Bank Transaction" bank_integration/install.py | grep -v doctype_or_docfield
echo
echo "=== Search for any migrations or database setup ==="
find . -name "*.py" | xargs grep -l "migrate\|migration\|alter\|constraint" 2>/dev/null
echo
echo "=== Verify Bank Transaction is standard Frappe doctype by checking if it's imported ==="
rg -n "from frappe" bank_integration/bank_integration/api/hdfc_bank_api.py | head -5Repository: resilient-tech/bank_integration
Length of output: 510
🏁 Script executed:
#!/bin/bash
echo "=== Confirm Bank Transaction is from Frappe core (not defined locally) ==="
rg -n "class Bank Transaction" --type=py
echo
echo "=== Check entire install.py for Bank Transaction custom field definitions ==="
sed -n '244,260p' bank_integration/install.py
echo
echo "=== Verify submit() could raise DuplicateEntryError ==="
rg -n "DuplicateEntryError" bank_integration/install.py
echo
echo "=== Check if there's any unique constraint setup anywhere ==="
rg -n "unique.*=.*1|unique.*True" bank_integration/install.pyRepository: resilient-tech/bank_integration
Length of output: 705
Move duplicate prevention out of the in-memory snapshot.
existing_ids is only a point-in-time read. Two overlapping syncs can both miss the same hash and both submit the same Bank Transaction before either request updates this set. Add a unique constraint on transaction_id and wrap bank_transaction.submit() with explicit duplicate-insert error handling using frappe.DuplicateEntryError.
| frm.add_custom_button(__("Sync Transactions"), () => { | ||
| frappe.confirm(`Sync Transactions for <strong>${frm.doc.bank_account}</strong> ?`, () => { | ||
| frm.doc.uid = frappe.utils.get_random(7); | ||
| frappe.confirm(`Sync Transactions for <strong>${frm.doc.bank_account}</strong> using HDFC Netbanking Statement download?`, () => { | ||
| frm._uid = frappe.utils.get_random(7); | ||
| frappe.call({ | ||
| method: "bank_integration.bank_integration.api.transactions.get_transactions", | ||
| args: { uid: frm.doc.uid, from_account: frm.doc.bank_account }, | ||
| }); | ||
| frappe.msgprint("Syncing Transactions  <i class='fa fa-refresh fa-spin'></i>") | ||
| frappe.realtime.on("sync_transactions", function (data) { | ||
| if (data.uid != frm.doc.uid) return; | ||
| frappe.update_msgprint( | ||
| `Synced <strong>${data.count}</strong> Transaction${(data.count == 1) ? "" : "s"} from <strong>${data.after_date}</strong>.` | ||
| ) | ||
| if (data.count) { | ||
| frm.set_value("bank_statement_closing_balance", data.closing_balance); | ||
| } | ||
| frm.save(); | ||
| frm.trigger("make_reconciliation_tool"); | ||
| args: { uid: frm._uid, from_account: frm.doc.bank_account ,from_date: frm.doc.bank_statement_from_date, to_date: frm.doc.bank_statement_to_date}, | ||
| }); | ||
| show_msg("Syncing Transactions  <i class='fa fa-refresh fa-spin'></i>") |
There was a problem hiding this comment.
Block sync until the statement range is complete.
toggle_reqd only affects saves; this custom button can still call the API with empty dates. The new HDFC sync path parses both values immediately, so clicking Sync Transactions before both dates are filled will fail server-side.
🐛 Proposed fix
frm.add_custom_button(__("Sync Transactions"), () => {
+ if (
+ !frm.doc.bank_account ||
+ !frm.doc.bank_statement_from_date ||
+ !frm.doc.bank_statement_to_date
+ ) {
+ frappe.msgprint(
+ __("Please select Bank Account, From Date, and To Date before syncing transactions.")
+ );
+ return;
+ }
frappe.confirm(`Sync Transactions for <strong>${frm.doc.bank_account}</strong> using HDFC Netbanking Statement download?`, () => {
frm._uid = frappe.utils.get_random(7);1 participant
No description provided.