Skip to content

Conversation

@ili-ad
Copy link

@ili-ad ili-ad commented Jan 3, 2026

Summary

Fix two PostgreSQL-specific failures encountered during bench --site iliad.local migrate on ERPNext v15.

Reproduction

  • DB: PostgreSQL
  • Run: bench --site iliad.local migrate

Fix 1: update_pick_list_fields fails on Postgres (UPDATE ... JOIN)

  • Patch: erpnext.patches.v15_0.update_pick_list_fields
  • Error: psycopg2.errors.SyntaxError: syntax error at or near "JOIN"
  • Cause: MariaDB-style UPDATE <table> JOIN <table> is not supported on Postgres
  • Change: use frappe.db.multisql:
    • MariaDB: keep existing UPDATE ... JOIN
    • Postgres: equivalent UPDATE ... SET ... FROM ... WHERE ...

Fix 2: allow_on_submit_dimensions_for_repostable_doctypes fails on Postgres (DISTINCT + ordering)

  • Patch: erpnext.patches.v15_0.allow_on_submit_dimensions_for_repostable_doctypes
  • Call chain: get_allowed_types_from_settings(child_doc=True) in
    erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py
  • Error: psycopg2.errors.InvalidColumnReference: for SELECT DISTINCT, ORDER BY expressions must appear in select list
  • Cause: fields=["distinct(document_type)"] generates a SELECT DISTINCT ... query which conflicts with ordering rules on Postgres
  • Change: replace distinct(document_type) with pluck="document_type" and de-duplicate results in Python while preserving order

Notes

  • Scope is limited to migration code paths
  • MariaDB behavior unchanged
  • Target branch: version-15-hotfix

Summary by CodeRabbit

  • Refactor
    • Improved retrieval and deduplication of document types eligible for reposting, preserving original order when expanding child document types.
    • Rewrote the delivery note pick-list synchronization to use a more direct, database-specific update approach for reliable propagation to line items.

✏️ Tip: You can customize this high-level summary in your review settings.

@ili-ad ili-ad requested a review from ruthra-kumar as a code owner January 3, 2026 14:47
@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Jan 3, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 3, 2026

Walkthrough

This change refactors two database-related operations. In repost_accounting_ledger, retrieval of allowed document types was changed from a DISTINCT(document_type) query to pluck-based fetching with application-level deduplication and preservation of order, and results are returned as single-element lists. In the v15 patch, delivery note pick_list propagation was rewritten from ORM-based updates to dialect-specific SQL: an UPDATE...JOIN for MariaDB and UPDATE...FROM for PostgreSQL.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing PostgreSQL-specific failures in v15 migrations, which is the core focus of both file changes in the PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


🧹 Recent nitpick comments
erpnext/patches/v15_0/update_pick_list_fields.py (1)

2-2: Remove unused IfNull import.

The IfNull import is no longer used after refactoring to raw SQL with COALESCE. This is now dead code.

Suggested fix
 import frappe
-from frappe.query_builder.functions import IfNull

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 36228f6 and 626cab7.

📒 Files selected for processing (2)
  • erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py
  • erpnext/patches/v15_0/update_pick_list_fields.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • erpnext/accounts/doctype/repost_accounting_ledger/repost_accounting_ledger.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: vorasmit
Repo: frappe/erpnext PR: 51099
File: erpnext/patches/v16_0/migrate_tax_withholding_data.py:904-910
Timestamp: 2025-12-23T11:28:49.128Z
Learning: In the old Tax Withholding system (before the Tax Withholding Entry refactor), only one TCS account per Sales Invoice was supported. When reviewing migration code in erpnext/patches/v16_0/migrate_tax_withholding_data.py, the query selecting TCS accounts from GL Entries doesn't need explicit aggregation because historical data will only have one TCS account per invoice.
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.

Applied to files:

  • erpnext/patches/v15_0/update_pick_list_fields.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: Python Unit Tests (3)
  • GitHub Check: Python Unit Tests (1)
  • GitHub Check: Python Unit Tests (4)
  • GitHub Check: Python Unit Tests (2)
  • GitHub Check: Patch Test
  • GitHub Check: Summary
🔇 Additional comments (2)
erpnext/patches/v15_0/update_pick_list_fields.py (2)

10-28: LGTM!

The dialect-specific SQL is correctly structured for both databases:

  • MariaDB uses the UPDATE ... JOIN syntax with backtick-quoted identifiers.
  • PostgreSQL uses the UPDATE ... FROM syntax with double-quoted identifiers and the join condition in the WHERE clause.

The COALESCE filter ensures consistent behavior across both dialects.


31-40: LGTM!

This single-table update using the query builder works correctly on both MariaDB and PostgreSQL. No changes needed here.

✏️ Tip: You can disable this entire section by setting review_details to false in your review 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ruthra-kumar ruthra-kumar self-assigned this Jan 6, 2026
@ruthra-kumar ruthra-kumar added the postgres issue/pr related to postgres label Jan 16, 2026
@ruthra-kumar ruthra-kumar force-pushed the fix/postgres-migrate-patches-hotfix branch from b816699 to 626cab7 Compare January 16, 2026 10:04
@ruthra-kumar ruthra-kumar merged commit eef26fe into frappe:version-15-hotfix Jan 16, 2026
10 checks passed
frappe-pr-bot pushed a commit that referenced this pull request Jan 20, 2026
# [15.95.0](v15.94.3...v15.95.0) (2026-01-20)

### Bug Fixes

* **accounts_controller:** make return message translatable ([8f6095d](8f6095d))
* **accounts:** add missing accounting dimensions in advance taxes and charges ([1d5f406](1d5f406))
* add other charges in total ([3ef4fa5](3ef4fa5))
* allow disassemble stock entry without work order (backport [#51761](#51761)) ([#51835](#51835)) ([be20698](be20698))
* calculate net profit amount from root node accounts ([e9573b0](e9573b0))
* common_party_path ([#51826](#51826)) ([6225217](6225217))
* docs_path ([b3df300](b3df300))
* **manufacturing:** consider process loss qty while validating the work order ([4418fb4](4418fb4))
* **pos:** reapply set warehouse during cart update ([75b4a0a](75b4a0a))
* **postgres:** compute current month sales without DATE_FORMAT ([fbf4305](fbf4305))
* **postgres:** fix v15 migration failures on Postgres ([#51481](#51481)) ([eef26fe](eef26fe))
* prevent UOM from updating incorrectly while scanning barcode ([d196956](d196956))
* **process statement of accounts:** allow renaming ([8b2778b](8b2778b))
* **process statement of accounts:** naming of reports ([054468a](054468a))
* RFQ does not fetch html response ([90e8090](90e8090))
* **sales analytics:** add curve filter ([c2995f6](c2995f6))
* Show non-SLE vouchers with GL entries in Stock vs Account Value Comparison report ([6219d7d](6219d7d))
* **stock entry:** calculate transferred quantity using transfer_qty (backport [#51656](#51656)) ([#51675](#51675)) ([1da781f](1da781f))
* **stock:** resolve quantity issue when adding items via barcode scan ([c508ef5](c508ef5))
* **transaction.js:** use flt instead of cint for plc_conversion_rate ([f618bf2](f618bf2))
* valuation rate for non batchwise valuation ([3008c7a](3008c7a))

### Features

* add new 2025 Charts of Accounts for France ([6af6fe8](6af6fe8))
* **process statement of accounts:** added more frequency options for auto email ([546ab05](546ab05))
* remove old French chart of accounts with code as nex 2025 is provided ([e568ab2](e568ab2))

### Performance Improvements

* prevent duplicate reposting for the same item ([eff9595](eff9595))
@frappe-pr-bot
Copy link
Collaborator

🎉 This PR is included in version 15.95.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-tests This PR needs automated unit-tests. postgres issue/pr related to postgres released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants