feat: use single remark field with custom remark toggle#54131
feat: use single remark field with custom remark toggle#54131khushi8112 wants to merge 2 commits intofrappe:developfrom
Conversation
📝 WalkthroughWalkthroughAdds a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
erpnext/accounts/doctype/journal_entry/journal_entry.json (1)
204-212:⚠️ Potential issue | 🟠 Major
user_remarkfield is hidden from UI but still being set programmatically.The
user_remarkfield on Journal Entry Account rows is now hidden in the schema ("hidden": 1), buterpnext/accounts/doctype/payment_reconciliation/payment_reconciliation.py(lines 837, 852) continues to setuser_remarkon child rows during automatic reconciliation. Sincebuild_gl_map()injournal_entry.pyuses only the parentremarkfield (line 1140), the child rowuser_remarkdata is silently persisted but never used in GL Entry remarks or any downstream logic.Update
payment_reconciliation.pyto set the parentremarkfield instead of the childuser_remark, or formally deprecate and remove the child field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/journal_entry/journal_entry.json` around lines 204 - 212, The child field "user_remark" is hidden and no longer used by build_gl_map() in journal_entry.py, so update the reconciliation logic in payment_reconciliation.py to set the parent Journal Entry's "remark" field instead of writing into each child row's "user_remark"; locate the code blocks in payment_reconciliation.py where "user_remark" is assigned (the reconciliation/auto-reconcile routines) and change them to set the parent document's remark (e.g., frappe.get_doc(...).remark or the current journal entry object's remark) so GL entries use the populated remark, or alternatively remove/deprecate all assignments to "user_remark" if you choose to drop the field entirely.erpnext/accounts/doctype/journal_entry/journal_entry.py (1)
1559-1568:⚠️ Potential issue | 🟡 MinorAddress f-string SQL interpolation of
searchfieldparameter.While
@frappe.validate_and_sanitize_search_inputsdoes sanitizesearchfieldby rejecting non-alphanumeric characters (preventing common SQL injection payloads), the linter flag is valid. For defensive coding and to fully satisfy security linters, use a whitelist approach or parameterized column selection instead of f-string interpolation. This ensuressearchfieldis both valid and safe regardless of future changes to the sanitization logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/journal_entry/journal_entry.py` around lines 1559 - 1568, The SQL currently builds a query using f-string interpolation of the searchfield which triggers linter/security concerns; instead, strictly map the incoming searchfield to an allowed column name whitelist (e.g., a dict of allowed search keys → column names) and select the column name from that whitelist before building the query, then pass all user values (account, party, etc.) as parameters to frappe.db.sql; locate the code that constructs the SQL (the call to frappe.db.sql in journal_entry.py and the variable named searchfield) and replace the direct f-string interpolation with a lookup from the whitelist (raising an error for unknown keys) so only validated column names are inserted into the query while all other values remain parameterized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@erpnext/accounts/doctype/journal_entry/journal_entry.json`:
- Around line 204-212: The child field "user_remark" is hidden and no longer
used by build_gl_map() in journal_entry.py, so update the reconciliation logic
in payment_reconciliation.py to set the parent Journal Entry's "remark" field
instead of writing into each child row's "user_remark"; locate the code blocks
in payment_reconciliation.py where "user_remark" is assigned (the
reconciliation/auto-reconcile routines) and change them to set the parent
document's remark (e.g., frappe.get_doc(...).remark or the current journal entry
object's remark) so GL entries use the populated remark, or alternatively
remove/deprecate all assignments to "user_remark" if you choose to drop the
field entirely.
In `@erpnext/accounts/doctype/journal_entry/journal_entry.py`:
- Around line 1559-1568: The SQL currently builds a query using f-string
interpolation of the searchfield which triggers linter/security concerns;
instead, strictly map the incoming searchfield to an allowed column name
whitelist (e.g., a dict of allowed search keys → column names) and select the
column name from that whitelist before building the query, then pass all user
values (account, party, etc.) as parameters to frappe.db.sql; locate the code
that constructs the SQL (the call to frappe.db.sql in journal_entry.py and the
variable named searchfield) and replace the direct f-string interpolation with a
lookup from the whitelist (raising an error for unknown keys) so only validated
column names are inserted into the query while all other values remain
parameterized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c844e9d9-621f-4f14-a459-7e19bc86e534
📒 Files selected for processing (5)
erpnext/accounts/doctype/journal_entry/journal_entry.jserpnext/accounts/doctype/journal_entry/journal_entry.jsonerpnext/accounts/doctype/journal_entry/journal_entry.pyerpnext/accounts/doctype/journal_entry/journal_entry_list.jserpnext/accounts/doctype/journal_entry/test_journal_entry.py
|
@Mergifyio rebase |
|
Deprecation notice: This pull request comes from a fork and was rebased using |
✅ Branch has been successfully rebased |
khushi8112
force-pushed
the
journal-entry-custom-remark-toggle
branch
from
eb2ecc5 to
d6fcf4c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
erpnext/accounts/doctype/journal_entry/journal_entry.py (1)
1561-1576:⚠️ Potential issue | 🟡 MinorAddress the semgrep SQL injection warning.
The pipeline flags SQL injection risk due to the f-string interpolation of
{searchfield}. While thefrappe.db.has_column()check at line 1558 validates thatsearchfieldis a legitimate column name (mitigating the risk), the semgrep rule still flags the pattern.Consider using parameterized column selection or explicitly whitelisting allowed columns to satisfy the linter:
🔧 Proposed fix using explicit whitelist
`@frappe.whitelist`() `@frappe.validate_and_sanitize_search_inputs` def get_against_jv(doctype: str, txt: str, searchfield: str, start: int, page_len: int, filters: dict): - if not frappe.db.has_column("Journal Entry", searchfield): + allowed_search_fields = {"name", "posting_date", "remark", "cheque_no", "title"} + if searchfield not in allowed_search_fields: return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/journal_entry/journal_entry.py` around lines 1561 - 1576, The SQL query in the return frappe.db.sql(...) interpolates {searchfield} via an f-string which tripped a semgrep SQL injection warning despite the preceding frappe.db.has_column() check; replace the dynamic interpolation by enforcing an explicit whitelist of allowed column names (e.g., map/tuple of permitted search fields) and use the validated whitelist value in the query, or construct the column selection from that whitelist before calling frappe.db.sql; keep the existing frappe.db.has_column() as an extra guard but ensure the code uses the whitelisted column (variable searchfield) instead of raw string interpolation to satisfy the linter.erpnext/accounts/doctype/journal_entry/journal_entry.json (1)
203-212:⚠️ Potential issue | 🟡 MinorUpdate line number references for
user_remarkfield usages.The
user_remarkfield is indeed hidden but not removed, maintaining backward compatibility for programmatic access. However, external code still sets this field in multiple locations:
test_tax_withholding_category.pyat lines 3073 and 3134test_balance_sheet.pyat line 117Consider documenting the deprecation or updating these references in a follow-up PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/journal_entry/journal_entry.json` around lines 203 - 212, Tests still set the hidden field "user_remark" in test_tax_withholding_category.py and test_balance_sheet.py; update those test usages to reference the field by name (e.g., setting doc.user_remark or doc.set("user_remark", ...)) rather than line numbers, add a short inline comment in each test noting the field is hidden/deprecated, and if necessary wrap assignments in a compatibility block (e.g., if hasattr(doc, "user_remark") or try/except) so tests remain stable; plan a follow-up PR to document the deprecation of "user_remark" in journal_entry.json.
🧹 Nitpick comments (1)
erpnext/accounts/doctype/journal_entry/test_journal_entry.py (1)
595-601: Consider extending the test to verify GL entry remarks.The test correctly verifies that
custom_remarkprevents the remark from being overwritten on insert. However, it doesn't verify that the custom remark is correctly propagated to GL entries after submission.💡 Suggested enhancement for comprehensive coverage
def test_custom_remark(self): # When custom_remark is enabled, remark should not be auto-overwritten on save jv = make_journal_entry("_Test Cash - _TC", "_Test Bank - _TC", 100, save=False) jv.custom_remark = 1 jv.remark = "My custom remark text" jv.insert() self.assertEqual(jv.remark, "My custom remark text") + + # Verify remark persists after submit and appears in GL entries + jv.submit() + jv.reload() + self.assertEqual(jv.remark, "My custom remark text") + + gl_remarks = frappe.db.get_all( + "GL Entry", + filters={"voucher_no": jv.name, "is_cancelled": 0}, + pluck="remarks", + ) + for remark in gl_remarks: + self.assertIn("My custom remark text", remark)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@erpnext/accounts/doctype/journal_entry/test_journal_entry.py` around lines 595 - 601, Extend test_custom_remark to also assert that GL entries receive the custom remark: after creating the Journal Entry (test_custom_remark using make_journal_entry and setting jv.custom_remark and jv.remark), submit the jv (call jv.submit()), query the related GL Entry rows for that Journal Entry (filter by voucher_type "Journal Entry" and voucher_no == jv.name or use the existing helper that fetches GL entries), and assert each returned GL Entry's remark equals "My custom remark text"; then clean up (cancel/delete) the submitted jv to avoid side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@erpnext/accounts/doctype/journal_entry/journal_entry.json`:
- Around line 203-212: Tests still set the hidden field "user_remark" in
test_tax_withholding_category.py and test_balance_sheet.py; update those test
usages to reference the field by name (e.g., setting doc.user_remark or
doc.set("user_remark", ...)) rather than line numbers, add a short inline
comment in each test noting the field is hidden/deprecated, and if necessary
wrap assignments in a compatibility block (e.g., if hasattr(doc, "user_remark")
or try/except) so tests remain stable; plan a follow-up PR to document the
deprecation of "user_remark" in journal_entry.json.
In `@erpnext/accounts/doctype/journal_entry/journal_entry.py`:
- Around line 1561-1576: The SQL query in the return frappe.db.sql(...)
interpolates {searchfield} via an f-string which tripped a semgrep SQL injection
warning despite the preceding frappe.db.has_column() check; replace the dynamic
interpolation by enforcing an explicit whitelist of allowed column names (e.g.,
map/tuple of permitted search fields) and use the validated whitelist value in
the query, or construct the column selection from that whitelist before calling
frappe.db.sql; keep the existing frappe.db.has_column() as an extra guard but
ensure the code uses the whitelisted column (variable searchfield) instead of
raw string interpolation to satisfy the linter.
---
Nitpick comments:
In `@erpnext/accounts/doctype/journal_entry/test_journal_entry.py`:
- Around line 595-601: Extend test_custom_remark to also assert that GL entries
receive the custom remark: after creating the Journal Entry (test_custom_remark
using make_journal_entry and setting jv.custom_remark and jv.remark), submit the
jv (call jv.submit()), query the related GL Entry rows for that Journal Entry
(filter by voucher_type "Journal Entry" and voucher_no == jv.name or use the
existing helper that fetches GL entries), and assert each returned GL Entry's
remark equals "My custom remark text"; then clean up (cancel/delete) the
submitted jv to avoid side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5cfac26f-8151-4f66-b535-2ebe5b074002
📒 Files selected for processing (5)
erpnext/accounts/doctype/journal_entry/journal_entry.jserpnext/accounts/doctype/journal_entry/journal_entry.jsonerpnext/accounts/doctype/journal_entry/journal_entry.pyerpnext/accounts/doctype/journal_entry/journal_entry_list.jserpnext/accounts/doctype/journal_entry/test_journal_entry.py
🚧 Files skipped from review as they are similar to previous changes (1)
- erpnext/accounts/doctype/journal_entry/journal_entry_list.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #54131 +/- ##
========================================
Coverage 79.11% 79.12%
========================================
Files 1159 1159
Lines 125274 125280 +6
========================================
+ Hits 99116 99122 +6
Misses 26158 26158
🚀 New features to boost your workflow:
|
1 participant
No description provided.