diff --git a/.github/POSTGRES_COMPATIBILITY.md b/.github/POSTGRES_COMPATIBILITY.md index e72529ce025..b4b7afbb5a3 100644 --- a/.github/POSTGRES_COMPATIBILITY.md +++ b/.github/POSTGRES_COMPATIBILITY.md @@ -60,10 +60,13 @@ Flag a changed query that uses any of these: check_field, True)`, `doc.db_set(field, False)`, or `frappe.qb.update(dt).set(check_field, True)` emit `SET col = true`, which PostgreSQL rejects on a `smallint`/`Check` column (`column is of type smallint but expression is of type boolean`). Pass `1`/`0`. -- **`.like()`/`.ilike()` (or raw `LIKE`) on a NON-text column** — `idx`, `docstatus`, a date, etc. - frappe maps `.like()` → `ILIKE`, and PostgreSQL has no `bigint ILIKE text` operator (`operator - does not exist: bigint ~~* unknown`). Cast the column to text first — **`Cast_(col, "varchar")`**, - not `Cast(col, "char")` (see below). MariaDB coerces the int implicitly, so the cast is a no-op there. +- **A direct `.like()`/`.ilike()` on a pypika field (or raw `LIKE`) on a NON-text column** — `idx`, + `docstatus`, a date, etc. frappe maps `.like()` → `ILIKE`, and PostgreSQL has no `bigint ILIKE text` + operator (`operator does not exist: bigint ~~* unknown`). Cast the column to text first — + **`Cast_(col, "varchar")`**, not `Cast(col, "char")` (see below). MariaDB coerces the int + implicitly, so the cast is a no-op there. A `["like", …]` filter passed to `get_all`/`get_list`/ + `qb.get_query`/`reportview` needs no cast: the framework casts non-text fields itself + (frappe/frappe#42449). - **`CAST(… AS CHAR)` / `Cast(x, "char")`** — on PostgreSQL bare `CHAR` is `character(1)`, so `CAST(12 AS CHAR)` → `'1'` (silently truncates multi-digit values); MariaDB gives the full string. Use `VARCHAR` / `Cast_(x, "varchar")`. @@ -192,8 +195,9 @@ pick a bound for a stated reason, and cover the varying-group case with a test. These are auto-handled by the framework and are **not** breaks: - **`.like()` / `["like", …]`** already renders as `ILIKE` on PostgreSQL — not a - case-sensitivity bug. *(Exception: `.like()` on a **non-text** column — `idx`, `docstatus` — - is a hard break, `bigint ILIKE`; see §1.)* + case-sensitivity bug. A `["like", …]` filter on a **non-text** field is also cast to text by + the framework. *(Exception: a direct `.like()` on a **non-text** pypika field — `idx`, + `docstatus` — is a hard break, `bigint ILIKE`; see §1.)* - **Raw `ifnull(...)`** inside `frappe.db.sql()` is rewritten to `coalesce(...)` on all engines. - **Backticks**, **`LOCATE`**, **`REGEXP`** / **`.regexp()`** in raw SQL are auto-translated on PostgreSQL (`REGEXP` → `~*`). **But `RLIKE` / `.rlike()` is NOT translated** — that one is a diff --git a/.github/helper/install.sh b/.github/helper/install.sh index 1abcd7683e7..ab8e664356e 100644 --- a/.github/helper/install.sh +++ b/.github/helper/install.sh @@ -4,6 +4,36 @@ set -e cd ~ || exit +# Authenticate git against github.com with the job token: anonymous git-over-HTTPS from the +# runners gets throttled to a 401, which kills whichever clone is in flight — the frappe fetch +# below, or payments under `bench get-app`. See the PR description. +# +# A credential helper rather than a url.insteadOf rewrite, because `git clone` PERSISTS a +# rewritten URL into the new repo's .git/config: an insteadOf would leave the token sitting in +# apps/payments/.git/config on the runner. A helper is consulted only when github.com actually +# challenges, and leaves the stored remote URL untouched. Passing it through GIT_CONFIG_* keeps +# the token out of ~/.gitconfig too, and child processes inherit it (bench shells out to git). +ci_github_token=${CI_GITHUB_TOKEN:-${GITHUB_TOKEN:-}} +if [ -n "$ci_github_token" ]; then + export CI_GITHUB_TOKEN="$ci_github_token" + export GIT_CONFIG_COUNT=3 + # Reset first: git runs EVERY configured helper and calls `store` on them after a successful + # auth, so a `credential.helper=store` inherited from the image's gitconfig would write the + # token to ~/.git-credentials. An empty value clears the list before ours is added. + export GIT_CONFIG_KEY_0="credential.helper" + export GIT_CONFIG_VALUE_0="" + export GIT_CONFIG_KEY_1="credential.https://github.com.username" + export GIT_CONFIG_VALUE_1="x-access-token" + export GIT_CONFIG_KEY_2="credential.https://github.com.helper" + # Single-quoted: $CI_GITHUB_TOKEN is expanded by the shell git runs the helper in, so the + # token is read from the environment at call time and never stored anywhere. Answering only + # `get` makes the helper inert for git's `store`/`erase` calls. + export GIT_CONFIG_VALUE_2='!f() { test "$1" = get && echo "password=$CI_GITHUB_TOKEN"; }; f' +fi + +# Whatever happens, never sit on a credential prompt: fail fast and legibly instead. +export GIT_TERMINAL_PROMPT=0 + githubbranch=${GITHUB_BASE_REF:-${GITHUB_REF##*/}} frappeuser=${FRAPPE_USER:-"frappe"} frappecommitish=${FRAPPE_BRANCH:-} @@ -188,7 +218,7 @@ restore_warm_bench() { # Phase 1 already fetched ~/frappe to the exact live develop SHA. Fetch that commit # straight from it (bench init names the remote 'upstream', not 'origin', and points # it at this local clone — so a plain `git fetch origin` does not work). - git fetch --no-tags "$HOME/frappe" HEAD || exit 1 + git fetch --no-tags --update-shallow "$HOME/frappe" HEAD || exit 1 git checkout --force FETCH_HEAD || exit 1 ); then echo "Fast-forward to ${frappe_sha} failed; falling back to full init" diff --git a/.github/workflows/patch.yml b/.github/workflows/patch.yml index e8eaa4f8ae4..cf760711501 100644 --- a/.github/workflows/patch.yml +++ b/.github/workflows/patch.yml @@ -121,6 +121,8 @@ jobs: env: DB: mariadb TYPE: server + # Anonymous git to github.com gets throttled to a 401; authenticate the clones. + CI_GITHUB_TOKEN: ${{ github.token }} - name: Run Patch Tests run: | diff --git a/.github/workflows/run-individual-tests.yml b/.github/workflows/run-individual-tests.yml index a70a2394757..319f62d230d 100644 --- a/.github/workflows/run-individual-tests.yml +++ b/.github/workflows/run-individual-tests.yml @@ -129,6 +129,8 @@ jobs: TYPE: server FRAPPE_USER: ${{ github.event.inputs.user }} FRAPPE_BRANCH: ${{ github.event.inputs.branch }} + # Anonymous git to github.com gets throttled to a 401; authenticate the clones. + CI_GITHUB_TOKEN: ${{ github.token }} - name: Run Tests run: | diff --git a/.github/workflows/server-tests-mariadb.yml b/.github/workflows/server-tests-mariadb.yml index c55c3f501f3..36ec44563b0 100644 --- a/.github/workflows/server-tests-mariadb.yml +++ b/.github/workflows/server-tests-mariadb.yml @@ -102,6 +102,8 @@ jobs: TYPE: server FRAPPE_USER: ${{ github.event.inputs.user }} FRAPPE_BRANCH: ${{ github.event.client_payload.sha || github.event.inputs.branch }} + # Anonymous git to github.com gets throttled to a 401; authenticate the clones. + CI_GITHUB_TOKEN: ${{ github.token }} DB_HOST: 127.0.0.1 DB_USER_HOST: '%' WKHTMLTOX_DEB: /tmp/wkhtmltox.deb diff --git a/.github/workflows/server-tests-postgres.yml b/.github/workflows/server-tests-postgres.yml index 44ab5dfab7a..52db9ca4f5c 100644 --- a/.github/workflows/server-tests-postgres.yml +++ b/.github/workflows/server-tests-postgres.yml @@ -103,6 +103,8 @@ jobs: DB: postgres TYPE: server FRAPPE_BRANCH: develop + # Anonymous git to github.com gets throttled to a 401; authenticate the clones. + CI_GITHUB_TOKEN: ${{ github.token }} BENCH_CACHE_DIR: /home/runner/bench-cache - name: Warm up test data diff --git a/.greptile/config.json b/.greptile/config.json index 1348468c611..cfbcb55be4f 100644 --- a/.greptile/config.json +++ b/.greptile/config.json @@ -7,7 +7,7 @@ "frappe/frappe" ] }, - "instructions": "ERPNext runs on both MariaDB and PostgreSQL from one codebase, but the PostgreSQL test job is label-gated and may not run on this PR, so review every new or changed database query (raw frappe.db.sql, frappe.qb, frappe.get_all/get_list/get_value, and report SQL) for cross-engine compatibility. PRIME RULE: MariaDB output must never change; PostgreSQL is bent to match MariaDB, never the reverse, so a change to the value, row count, or ordering MariaDB produced is a regression even if it looks more correct (the only accepted change is replacing an arbitrary/undefined result with a deterministic one, row count preserved, and it should be called out). Flag a changed query that (1) would ERROR on PostgreSQL: loose GROUP BY (selecting/ordering a column neither grouped nor aggregated -- including an aggregate like Sum()/Count() selected next to bare columns with NO .groupby() at all), MySQL-only functions (TIMESTAMP(date,time), TIMEDIFF, STR_TO_DATE, DATE_FORMAT, DATE_ADD/DATE_SUB, GROUP_CONCAT, PERIOD_DIFF, SQL IF()), .rlike()/RLIKE (frappe rewrites REGEXP->~* on PostgreSQL but does NOT translate RLIKE; use .regexp()), .like()/LIKE on a NON-text column such as idx/docstatus (bigint ILIKE has no operator; Cast_(col,'varchar') first), CAST AS CHAR / Cast(x,'char') (bare CHAR is character(1) on PostgreSQL and truncates multi-digit values; use 'varchar'), UPDATE..JOIN, HAVING on a SELECT alias, SELECT DISTINCT with an ORDER BY expr not in the select list, single-quoted column aliases, varchar bitwise OR, capital-cased identifiers used as fieldnames in get_value(dt,dn,'Status') or get_all(dt,fields=['Account']) (PostgreSQL matches the quoted identifier case-sensitively; use the stored lower-case name), a Python bool written to a Check/int column via set_value/db_set/qb.update().set() instead of 1/0, or IfNull/Coalesce of a typed column with a different-typed literal such as IfNull(date_col, 0) -> COALESCE(date, integer) (PostgreSQL: 'COALESCE types date and integer cannot be matched'; the common IfNull(date,0) != 0 / == 0 presence test should be date_col.isnotnull() / .isnull(), else coalesce to a same-type default), or division by a possibly-zero divisor (Sum(a)/Sum(b) or x/col where the data can drive the divisor to 0 -- MariaDB returns NULL for division by zero but PostgreSQL raises 'division by zero' and aborts the query, so wrap the divisor in NullIf(divisor, 0)); or (2) would SILENTLY DIVERGE across engines: case-sensitive ==/.isin()/Strpos on USER-ENTERED free-text columns such as Data/Small Text/Long Text but NOT on Link/Select/name columns where exact-case matching is intended (PostgreSQL is case-sensitive, use Lower() both sides), lowercasing a value used as a document-name lookup, empty-string vs NULL in Concat/Concat_ws (MariaDB CONCAT(x,NULL) is NULL but PostgreSQL CONCAT drops the NULL, so a label like Concat('MFG-', nullable_date) leaks a bare 'MFG-' on PostgreSQL -- guard with Case/Coalesce/NullIf), NULL ordering (PostgreSQL sorts NULLs last) in ORDER BY..LIMIT 1, integer division (int/int truncates on PostgreSQL; multiply by 100.0 or make a literal a float, e.g. col/1440 -> col/1440.0), get_all(distinct=True, order_by=...) (frappe DROPS the ORDER BY for distinct queries on PostgreSQL, so sort in Python with key=str.casefold), an engine-specific function rewrite that does not match MariaDB on edge cases, or UnixTimestamp(date)/date-to-epoch math that is timezone-dependent (a strict epoch <= now bound is flaky on PostgreSQL). Also flag CATCH-AND-CONTINUE inserts: on PostgreSQL a failed insert aborts the WHOLE transaction (InFailedSqlTransaction), so code that swallows a duplicate/unique error and keeps going in the same transaction must wrap the fallible insert in frappe.db.savepoint(name) + rollback(save_point=name), unless it re-throws with no DB call before the throw or the insert uses ignore_if_duplicate=True or autoname='hash'. When RECOVERING the poisoned txn, prefer a SCOPED savepoint over a full frappe.db.rollback(): a full rollback discards rows the handler already created before the failure -- which MariaDB keeps -- so it is a silent MariaDB regression. 'The bg job / whitelist entrypoint owns the txn' does NOT make a full rollback safe if it did multiple inserts in a loop first; a full rollback is safe only when it immediately re-throws/raises, has nothing successful before it (single op), or the batch is meant to be atomic (a partial result is invalid -> rollback + mark Failed is correct). Otherwise use a per-iteration/per-record savepoint, and keep the function's success/None return contract (don't return a value for a doc that was just rolled back). GROUP BY ROW-COUNT TRAP (most important): to make a loose GROUP BY PostgreSQL-valid, do NOT add a non-functionally-dependent column (the classic traps are the child/row primary key or an editable per-row field) to GROUP BY because that splits one row into N and changes the MariaDB row count; Max()/Min()-wrap it instead (row count preserved, value arbitrary to deterministic). Judge functional dependence by the SOURCE TABLE: a column from a master joined on the group key is FD and safe in GROUP BY, but a descriptive field on the transaction table (e.g. t1.supplier_name, t1.territory) is NOT FD and must be wrapped. The SAME row-count trap applies to SELECT DISTINCT: to satisfy PostgreSQL's ORDER-BY-expr-must-appear-in-the-select rule, do NOT blindly add the ordered column to the select -- if it is not single-valued per existing distinct row the DISTINCT key grows and MariaDB returns MORE rows (a regression); add it only if functionally dependent on the existing select columns, otherwise drop the SQL ORDER BY and sort in Python (key=str.casefold). Do NOT suggest changing a Max()-wrapped column to Sum() to make a number more correct, that changes MariaDB's value. SECOND-ORDER GROUP BY TRAPS (the Max()/Min() wrap itself can be the bug -- a wrap is only a no-op when the column is provably single-valued per group): (a) INCOHERENT PAIR: two semantically-coupled columns (a flag + a link like is_phantom_item + bom_no, a discriminator + its value) aggregated with INDEPENDENT Max()/Min() can pair values from DIFFERENT rows into a chimera row that never existed (MariaDB's loose pick was at least row-coherent) -- when a consumer uses the two values together (recursion into the link gated by the flag, dict keys, link+flag display) require grouping by the pair or a single representative-row subquery (Min(child.name) + join back). (b) COLLATION-DEPENDENT TEXT PICK: Max()/Min() over a TEXT column is a sort, and the engines sort text differently -- MariaDB's utf8mb4 collations fold case, PostgreSQL (as CI runs it) orders by byte value, so MAX('abc','ABD') is 'ABD' on MariaDB and 'abc' on PostgreSQL. Flag a Max()/Min() on a text column (description, item_name, warehouse, cost_center, remarks, uom, mode_of_payment, operation) that is NOT functionally dependent on the group key: it is a live MariaDB-vs-PostgreSQL divergence, not the arbitrary-pick preservation the wrap is usually justified as. Do NOT flag it when the column comes from a master joined ON the grouped key (then it is single-valued and collation is irrelevant). Fix by taking a representative row instead of sorting text. A local macOS PostgreSQL agrees with MariaDB here and gives a false all-clear -- trust CI. (b) NULL-SKIPPING: Max/Min ignore NULLs, so Max() over a mostly-NULL discriminator deterministically prefers the non-NULL value where MariaDB could return NULL -- flag when 'no value' is a meaningful state (fallback gates like 'if x:', dict keys, status decisions). (c) FABRICATED ARITHMETIC: Sum(x) * Max(y) -- or Python arithmetic combining a Sum'd and a Max'd column from the same grouped query -- where y can vary within the group invents a value no row ever had and Max biases it upward; require per-row Sum(x*y) when it feeds validation, budgets, valuation, or GL/stock values. (d) WRONG BOUND: when the aggregated value has a semantic, the bound must be chosen deliberately (Min(schedule_date) for a 'required by' date, Min(idx) for first-line ordering, a qty-weighted average for a rate); a blind Max can understate urgency or overstate a figure. Heuristic: if switching Max<->Min would change the answer, the column is NOT functionally dependent and wrapping either is the wrong fix -- group by it, restructure, or pick a bound for a stated reason with a test. REFACTOR / CONVERSION FAITHFULNESS: a commit labeled a 'refactor' or a raw-frappe.db.sql->frappe.qb/ORM conversion is meant to preserve behaviour but easily does not, and the change slips past the static checker and a one-engine green run -- diff the WHERE/predicate, the JOIN/ON conditions and the resulting ROW SET, not just the SELECT shape. A conversion that silently widens or narrows the filter (e.g. a 'posting_datetime > X' bound gaining an OR (posting_datetime == X AND creation > args.creation) branch under a sql->qb refactor) changes the rows touched on BOTH engines and is a regression hiding under a refactor label; call it out and require a test even if it is a deliberate bug-fix. DO NOT FLAG these false positives: .like()/['like'] on a TEXT column (already ILIKE on PostgreSQL -- but DO flag it on a non-text/integer column, see above), raw ifnull/backticks/LOCATE/REGEXP/.regexp() inside frappe.db.sql (auto-translated by the framework -- but RLIKE/.rlike() is NOT translated, see above), or an ORDER BY..LIMIT 1 tie where adding a tiebreaker would change MariaDB's current pick. Full catalog with examples and portable fixes is in .github/POSTGRES_COMPATIBILITY.md.", + "instructions": "ERPNext runs on both MariaDB and PostgreSQL from one codebase, but the PostgreSQL test job is label-gated and may not run on this PR, so review every new or changed database query (raw frappe.db.sql, frappe.qb, frappe.get_all/get_list/get_value, and report SQL) for cross-engine compatibility. PRIME RULE: MariaDB output must never change; PostgreSQL is bent to match MariaDB, never the reverse, so a change to the value, row count, or ordering MariaDB produced is a regression even if it looks more correct (the only accepted change is replacing an arbitrary/undefined result with a deterministic one, row count preserved, and it should be called out). Flag a changed query that (1) would ERROR on PostgreSQL: loose GROUP BY (selecting/ordering a column neither grouped nor aggregated -- including an aggregate like Sum()/Count() selected next to bare columns with NO .groupby() at all), MySQL-only functions (TIMESTAMP(date,time), TIMEDIFF, STR_TO_DATE, DATE_FORMAT, DATE_ADD/DATE_SUB, GROUP_CONCAT, PERIOD_DIFF, SQL IF()), .rlike()/RLIKE (frappe rewrites REGEXP->~* on PostgreSQL but does NOT translate RLIKE; use .regexp()), a direct .like()/LIKE on a NON-text column such as idx/docstatus (bigint ILIKE has no operator; Cast_(col,'varchar') first -- a ['like', ...] filter dict passed to get_all/get_list/qb.get_query is cast by the framework and needs nothing), CAST AS CHAR / Cast(x,'char') (bare CHAR is character(1) on PostgreSQL and truncates multi-digit values; use 'varchar'), UPDATE..JOIN, HAVING on a SELECT alias, SELECT DISTINCT with an ORDER BY expr not in the select list, single-quoted column aliases, varchar bitwise OR, capital-cased identifiers used as fieldnames in get_value(dt,dn,'Status') or get_all(dt,fields=['Account']) (PostgreSQL matches the quoted identifier case-sensitively; use the stored lower-case name), a Python bool written to a Check/int column via set_value/db_set/qb.update().set() instead of 1/0, or IfNull/Coalesce of a typed column with a different-typed literal such as IfNull(date_col, 0) -> COALESCE(date, integer) (PostgreSQL: 'COALESCE types date and integer cannot be matched'; the common IfNull(date,0) != 0 / == 0 presence test should be date_col.isnotnull() / .isnull(), else coalesce to a same-type default), or division by a possibly-zero divisor (Sum(a)/Sum(b) or x/col where the data can drive the divisor to 0 -- MariaDB returns NULL for division by zero but PostgreSQL raises 'division by zero' and aborts the query, so wrap the divisor in NullIf(divisor, 0)); or (2) would SILENTLY DIVERGE across engines: case-sensitive ==/.isin()/Strpos on USER-ENTERED free-text columns such as Data/Small Text/Long Text but NOT on Link/Select/name columns where exact-case matching is intended (PostgreSQL is case-sensitive, use Lower() both sides), lowercasing a value used as a document-name lookup, empty-string vs NULL in Concat/Concat_ws (MariaDB CONCAT(x,NULL) is NULL but PostgreSQL CONCAT drops the NULL, so a label like Concat('MFG-', nullable_date) leaks a bare 'MFG-' on PostgreSQL -- guard with Case/Coalesce/NullIf), NULL ordering (PostgreSQL sorts NULLs last) in ORDER BY..LIMIT 1, integer division (int/int truncates on PostgreSQL; multiply by 100.0 or make a literal a float, e.g. col/1440 -> col/1440.0), get_all(distinct=True, order_by=...) (frappe DROPS the ORDER BY for distinct queries on PostgreSQL, so sort in Python with key=str.casefold), an engine-specific function rewrite that does not match MariaDB on edge cases, or UnixTimestamp(date)/date-to-epoch math that is timezone-dependent (a strict epoch <= now bound is flaky on PostgreSQL). Also flag CATCH-AND-CONTINUE inserts: on PostgreSQL a failed insert aborts the WHOLE transaction (InFailedSqlTransaction), so code that swallows a duplicate/unique error and keeps going in the same transaction must wrap the fallible insert in frappe.db.savepoint(name) + rollback(save_point=name), unless it re-throws with no DB call before the throw or the insert uses ignore_if_duplicate=True or autoname='hash'. When RECOVERING the poisoned txn, prefer a SCOPED savepoint over a full frappe.db.rollback(): a full rollback discards rows the handler already created before the failure -- which MariaDB keeps -- so it is a silent MariaDB regression. 'The bg job / whitelist entrypoint owns the txn' does NOT make a full rollback safe if it did multiple inserts in a loop first; a full rollback is safe only when it immediately re-throws/raises, has nothing successful before it (single op), or the batch is meant to be atomic (a partial result is invalid -> rollback + mark Failed is correct). Otherwise use a per-iteration/per-record savepoint, and keep the function's success/None return contract (don't return a value for a doc that was just rolled back). GROUP BY ROW-COUNT TRAP (most important): to make a loose GROUP BY PostgreSQL-valid, do NOT add a non-functionally-dependent column (the classic traps are the child/row primary key or an editable per-row field) to GROUP BY because that splits one row into N and changes the MariaDB row count; Max()/Min()-wrap it instead (row count preserved, value arbitrary to deterministic). Judge functional dependence by the SOURCE TABLE: a column from a master joined on the group key is FD and safe in GROUP BY, but a descriptive field on the transaction table (e.g. t1.supplier_name, t1.territory) is NOT FD and must be wrapped. The SAME row-count trap applies to SELECT DISTINCT: to satisfy PostgreSQL's ORDER-BY-expr-must-appear-in-the-select rule, do NOT blindly add the ordered column to the select -- if it is not single-valued per existing distinct row the DISTINCT key grows and MariaDB returns MORE rows (a regression); add it only if functionally dependent on the existing select columns, otherwise drop the SQL ORDER BY and sort in Python (key=str.casefold). Do NOT suggest changing a Max()-wrapped column to Sum() to make a number more correct, that changes MariaDB's value. SECOND-ORDER GROUP BY TRAPS (the Max()/Min() wrap itself can be the bug -- a wrap is only a no-op when the column is provably single-valued per group): (a) INCOHERENT PAIR: two semantically-coupled columns (a flag + a link like is_phantom_item + bom_no, a discriminator + its value) aggregated with INDEPENDENT Max()/Min() can pair values from DIFFERENT rows into a chimera row that never existed (MariaDB's loose pick was at least row-coherent) -- when a consumer uses the two values together (recursion into the link gated by the flag, dict keys, link+flag display) require grouping by the pair or a single representative-row subquery (Min(child.name) + join back). (b) COLLATION-DEPENDENT TEXT PICK: Max()/Min() over a TEXT column is a sort, and the engines sort text differently -- MariaDB's utf8mb4 collations fold case, PostgreSQL (as CI runs it) orders by byte value, so MAX('abc','ABD') is 'ABD' on MariaDB and 'abc' on PostgreSQL. Flag a Max()/Min() on a text column (description, item_name, warehouse, cost_center, remarks, uom, mode_of_payment, operation) that is NOT functionally dependent on the group key: it is a live MariaDB-vs-PostgreSQL divergence, not the arbitrary-pick preservation the wrap is usually justified as. Do NOT flag it when the column comes from a master joined ON the grouped key (then it is single-valued and collation is irrelevant). Fix by taking a representative row instead of sorting text. A local macOS PostgreSQL agrees with MariaDB here and gives a false all-clear -- trust CI. (b) NULL-SKIPPING: Max/Min ignore NULLs, so Max() over a mostly-NULL discriminator deterministically prefers the non-NULL value where MariaDB could return NULL -- flag when 'no value' is a meaningful state (fallback gates like 'if x:', dict keys, status decisions). (c) FABRICATED ARITHMETIC: Sum(x) * Max(y) -- or Python arithmetic combining a Sum'd and a Max'd column from the same grouped query -- where y can vary within the group invents a value no row ever had and Max biases it upward; require per-row Sum(x*y) when it feeds validation, budgets, valuation, or GL/stock values. (d) WRONG BOUND: when the aggregated value has a semantic, the bound must be chosen deliberately (Min(schedule_date) for a 'required by' date, Min(idx) for first-line ordering, a qty-weighted average for a rate); a blind Max can understate urgency or overstate a figure. Heuristic: if switching Max<->Min would change the answer, the column is NOT functionally dependent and wrapping either is the wrong fix -- group by it, restructure, or pick a bound for a stated reason with a test. REFACTOR / CONVERSION FAITHFULNESS: a commit labeled a 'refactor' or a raw-frappe.db.sql->frappe.qb/ORM conversion is meant to preserve behaviour but easily does not, and the change slips past the static checker and a one-engine green run -- diff the WHERE/predicate, the JOIN/ON conditions and the resulting ROW SET, not just the SELECT shape. A conversion that silently widens or narrows the filter (e.g. a 'posting_datetime > X' bound gaining an OR (posting_datetime == X AND creation > args.creation) branch under a sql->qb refactor) changes the rows touched on BOTH engines and is a regression hiding under a refactor label; call it out and require a test even if it is a deliberate bug-fix. DO NOT FLAG these false positives: .like()/['like'] on a TEXT column (already ILIKE on PostgreSQL) or a ['like'] filter dict on a non-text field (the framework casts it -- but DO flag a direct .like() on a non-text/integer column, see above), raw ifnull/backticks/LOCATE/REGEXP/.regexp() inside frappe.db.sql (auto-translated by the framework -- but RLIKE/.rlike() is NOT translated, see above), or an ORDER BY..LIMIT 1 tie where adding a tiebreaker would change MariaDB's current pick. Full catalog with examples and portable fixes is in .github/POSTGRES_COMPATIBILITY.md.", "customContext": { "files": [ { diff --git a/banking/src/components/common/AccountsDropdown.tsx b/banking/src/components/common/AccountsDropdown.tsx index a98ace578c3..6bf23872fde 100644 --- a/banking/src/components/common/AccountsDropdown.tsx +++ b/banking/src/components/common/AccountsDropdown.tsx @@ -9,6 +9,7 @@ import Fuse from "fuse.js" import { ChevronDownIcon } from "lucide-react" import { useLayoutEffect, useMemo, useRef, useState } from "react" import { FormControl } from "../ui/form" +import useResetScrollOnSearch from "@/hooks/useResetScrollOnSearch" export interface AccountsDropdownProps { @@ -104,6 +105,10 @@ const AccountsDropdown = ({ root_type, report_type, account_type, value, onChang const buttonRef = useRef(null) + // Searching replaces the grouped list with a short result list, so pin the scroll back to + // the top - otherwise the auto-selected first result can be out of view. + const listRef = useResetScrollOnSearch(search) + const [width, setWidth] = useState(320) useLayoutEffect(() => { @@ -153,7 +158,7 @@ const AccountsDropdown = ({ root_type, report_type, account_type, value, onChang - + {_("No accounts found.")} {recommendedAccounts.length > 0 && ( diff --git a/banking/src/components/common/LinkFieldCombobox.tsx b/banking/src/components/common/LinkFieldCombobox.tsx index a41105b05d7..a486f6286c3 100644 --- a/banking/src/components/common/LinkFieldCombobox.tsx +++ b/banking/src/components/common/LinkFieldCombobox.tsx @@ -10,6 +10,7 @@ import { ChevronDownIcon, ExternalLink } from "lucide-react"; import { Button } from "../ui/button"; import { cn } from "@/lib/utils"; import { Command, CommandEmpty, CommandGroup, CommandInput, CommandItem, CommandList } from "../ui/command"; +import useResetScrollOnSearch from "@/hooks/useResetScrollOnSearch"; import _ from "@/lib/translate"; import ErrorBanner from "../ui/error-banner"; import MarkdownRenderer from "../ui/markdown"; @@ -149,6 +150,10 @@ const LinkFieldCombobox = ({ const buttonRef = useRef(null) + // Results change as the search runs, so pin the scroll back to the top to keep the + // auto-selected first result in view. + const listRef = useResetScrollOnSearch(searchInput) + const [width, setWidth] = useState(320) useLayoutEffect(() => { @@ -264,7 +269,7 @@ const LinkFieldCombobox = ({ {error && } - + {isLoading ? _("Loading...") : _("No results found.")} {items?.map((result) => ( @@ -272,7 +277,7 @@ const LinkFieldCombobox = ({ {result.label || result.value} - {result.description && + {result.description && } diff --git a/banking/src/components/features/BankReconciliation/BankBalance.tsx b/banking/src/components/features/BankReconciliation/BankBalance.tsx index 632f3d62e8d..a0b9b0e3160 100644 --- a/banking/src/components/features/BankReconciliation/BankBalance.tsx +++ b/banking/src/components/features/BankReconciliation/BankBalance.tsx @@ -6,13 +6,13 @@ import { Progress } from "@/components/ui/progress" import { useGetAccountClosingBalance, useGetAccountClosingBalanceAsPerStatement, useGetAccountOpeningBalance, useGetUnreconciledTransactions } from "./utils" import { flt, formatCurrency } from "@/lib/numbers" import { Skeleton } from "@/components/ui/skeleton" -import { StatContainer, StatLabel, StatValue } from "@/components/ui/stats" import { Edit, Info, Trash2 } from "lucide-react" import { H4, Paragraph } from "@/components/ui/typography" import { HoverCard, HoverCardContent, HoverCardTrigger } from "@/components/ui/hover-card" import { getCompanyCurrency } from "@/lib/company" import _ from "@/lib/translate" -import { Dialog, DialogClose, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle, DialogTrigger } from "@/components/ui/dialog" +import { cn } from "@/lib/utils" +import { Dialog, DialogClose, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle } from "@/components/ui/dialog" import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip" import { formatDate } from "@/lib/date" import { Form } from "@/components/ui/form" @@ -26,50 +26,109 @@ import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "@ import { toast } from "sonner" import ErrorBanner from "@/components/ui/error-banner" -const BankBalance = () => { +const useBankCurrency = () => { + const bankAccount = useAtomValue(selectedBankAccountAtom) + return bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? '') +} + +/** + * One line of the balance summary - label on the left, figure right-aligned. + * + * `items-baseline` keeps the figure on the label's FIRST line, so a row carrying a `subLabel` + * (the statement row's "As of " note) doesn't centre its value against both lines. + */ +const BalanceRow = ({ label, info, subLabel, emphasis, children }: { + label: React.ReactNode + info?: React.ReactNode + subLabel?: React.ReactNode + emphasis?: boolean + children: React.ReactNode +}) => ( +
+ + + {label} + {info} + + {subLabel} + +
{children}
+
+) + +/** + * Type styles for a figure. Shared so an interactive figure can put them on the + + {tooltip} + + } + subLabel={!isDateSame && data?.message.date + ? + {_("As of {0}", [formatDate(data?.message?.date ?? '', 'Do MMM YYYY')])} + + : undefined} + > + {/* Deliberately NOT a flex container: a flex box's baseline doesn't resolve to its + text, so the row's `items-baseline` couldn't line this up with the label. As a + plain inline button its baseline is the figure's own, like every other row. + "Set" gets the same treatment as a figure - it stands in for one. */} + {isLoading + ? + : + + {/* The figure styles live on the button itself - see + BALANCE_VALUE_CLASSES. `p-0` because preflight leaves the UA's + button padding in place. */} + + + {tooltip} + } + + + + setIsOpen(false)} + /> + + + + ) +} + +const DifferenceRow = () => { + const bankAccount = useAtomValue(selectedBankAccountAtom) + const currency = useBankCurrency() const { data, isLoading } = useGetAccountClosingBalance() @@ -102,16 +257,15 @@ const Difference = () => { const isError = difference !== 0 - return - {_("Difference")} - {isLoading ? : - {formatCurrency(difference, - bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? '')) - }} - + return + {isLoading + ? + : {formatCurrency(difference, currency)}} + } -const ReconcileProgress = () => { +/** Reconciliation progress through the selected date range: a count plus a slim bar. */ +const ReconciledRow = () => { const bankAccount = useAtomValue(selectedBankAccountAtom) @@ -132,75 +286,14 @@ const ReconcileProgress = () => { const progress = (totalCount ? reconciledCount / totalCount : 0) * 100 - return
-
- -
+ return
+ + {reconciledCount} / {totalCount ?? 0} + +
} -const ClosingBalanceAsPerStatement = () => { - - const bankAccount = useAtomValue(selectedBankAccountAtom) - const dates = useAtomValue(bankRecDateAtom) - const setValue = useSetAtom(bankRecClosingBalanceAtom(bankAccount?.name ?? '')) - - const { data, isLoading } = useGetAccountClosingBalanceAsPerStatement({ - onSuccess: (data) => { - if (data?.message && data?.message?.balance) { - setValue({ - value: data?.message?.balance, - stringValue: data?.message?.balance.toString() - }) - } - } - }) - - const isDateSame = data?.message?.date === dates.toDate - - const [isOpen, setIsOpen] = useState(false) - - - return - {_("Closing Balance as per statement")} -
- - - - -
- {isLoading ? : {formatCurrency(flt(data?.message?.balance, 2), bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? ''))}} - -
- - - {_("Click to set the closing balance as per statement")} - - - - - setIsOpen(false)} - /> - - - - - {!isDateSame && data?.message.date && {_("As of {0}", [formatDate(data?.message?.date ?? '', 'Do MMM YYYY')])}} -
- - -} - const ClosingBalanceForm = ({ defaultBalance, date, bankAccount, onClose }: { defaultBalance: number, date: string, bankAccount: SelectedBank | null, onClose: VoidFunction }) => { const { mutate } = useSWRConfig() @@ -302,7 +395,7 @@ const ClosingBalancesList = ({ bankAccount, date }: { bankAccount: SelectedBank return
-

{_("Balances as per bank statement before {0}", [formatDate(date, 'Do MMM YYYY')])}

+

{_("Balances as per bank statement before {0}", [formatDate(date, 'Do MMM YYYY')])}

@@ -331,4 +424,4 @@ const ClosingBalancesList = ({ bankAccount, date }: { bankAccount: SelectedBank } -export default BankBalance \ No newline at end of file +export default BankAccountBalancePanel diff --git a/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx b/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx index c26b9e9fb22..4c44507b2ba 100644 --- a/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx +++ b/banking/src/components/features/BankReconciliation/BankClearanceSummary.tsx @@ -205,9 +205,9 @@ const BankClearanceSummaryView = () => { const content = _("Below is a list of all accounting entries posted against the bank account {0} between {1} and {2}.", [`${bankAccount?.account}`, `${formattedFromDate}`, `${formattedToDate}`]) - return
+ return
-
+
@@ -220,8 +220,9 @@ const BankClearanceSummaryView = () => { data={data.message.result} columns={clearanceColumns} getRowId={(row) => `${row.payment_entry}-${row.posting_date}`} - maxHeight="calc(100vh - 200px)" - scrollAreaClassName="min-h-[calc(100vh-200px)]" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={_("No rows to display.")} /> ) : null} diff --git a/banking/src/components/features/BankReconciliation/BankPicker.tsx b/banking/src/components/features/BankReconciliation/BankPicker.tsx index 47b087bfa81..a5c65402ec2 100644 --- a/banking/src/components/features/BankReconciliation/BankPicker.tsx +++ b/banking/src/components/features/BankReconciliation/BankPicker.tsx @@ -74,7 +74,10 @@ const BankPicker = ({ className }: { className?: string }) => { } return (
4 ? 'pb-2' : '', className, )} style={{ @@ -108,12 +111,12 @@ const BankPickerItem = ({ bank }: { bank: SelectedBank }) => { role="button" title={`Select ${bank.account_name}`} onClick={onSelect} - className={cn('rounded-md border border-outline-gray-1 max-w-60 min-w-60 p-2 overflow-hidden cursor-pointer', + // `shrink-0`: this is a horizontally scrolling row, so cards keep their own width + // instead of being compressed to fit the container. + className={cn('w-60 shrink-0 rounded-md border border-outline-gray-1 p-2 overflow-hidden cursor-pointer transition-colors', isSelected ? 'border-outline-gray-5 bg-surface-gray-1' : 'hover:bg-surface-gray-1' )} > - -
diff --git a/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx b/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx index 84bd5278ccc..0cf530e1c6b 100644 --- a/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx +++ b/banking/src/components/features/BankReconciliation/BankRecDateFilter.tsx @@ -5,107 +5,179 @@ import { AVAILABLE_TIME_PERIODS, formatDate, getDatesForTimePeriod, TimePeriod } import { Button } from '@/components/ui/button' import { Popover, PopoverContent, PopoverTrigger } from '@/components/ui/popover' import { ChevronDownIcon, ChevronLeftIcon, ChevronRight } from 'lucide-react' -import { Command, CommandEmpty, CommandInput, CommandItem, CommandList } from '@/components/ui/command' +import { Command, CommandGroup, CommandInput, CommandItem, CommandList } from '@/components/ui/command' import { parse } from "chrono-node" import { Calendar } from '@/components/ui/calendar' import useFiscalYear from '@/hooks/useFiscalYear' import dayjs from 'dayjs' import _ from '@/lib/translate' import { useDirection } from '@/components/ui/direction' +import useResetScrollOnSearch from '@/hooks/useResetScrollOnSearch' + +const DATE_FORMAT = 'YYYY-MM-DD' + +/** Current fiscal year plus this many previous ones, for quarter/year options. */ +const PREVIOUS_FISCAL_YEARS = 2 + +type DateOption = { + /** Stable id - used as the cmdk value and the React key. */ + key: string + label: string + translatedLabel: string + fromDate: string + toDate: string + format: string + /** Extra terms to match against, beyond the labels and dates. */ + keywords?: string[] + /** Whether to show this option when the search box is empty. */ + isDefault?: boolean +} + +/** + * Fiscal years keep the same month/day boundaries year on year, so previous years can be + * derived by subtracting whole years instead of fetching them. Works for both Jan-Dec and + * Apr-Mar style fiscal years. + */ +const fiscalYearLabel = (start: dayjs.Dayjs, end: dayjs.Dayjs) => + start.year() === end.year() ? `${start.year()}` : `${start.year()}-${end.year()}` const BankRecDateFilter = () => { const [bankRecDate, setBankRecDate] = useAtom(bankRecDateAtom) - const { data: fiscalYear } = useFiscalYear() + const { fiscalYear } = useFiscalYear() - const timePeriodOptions = useMemo(() => { - const standardOptions = AVAILABLE_TIME_PERIODS.map((period) => { + const today = useMemo(() => dayjs().format(DATE_FORMAT), []) + + const allOptions = useMemo(() => { + const standardOptions: DateOption[] = AVAILABLE_TIME_PERIODS.map((period) => { const dates = getDatesForTimePeriod(period) return { + key: period, label: period, + translatedLabel: dates.translatedLabel ?? _(period), fromDate: dates.fromDate, toDate: dates.toDate, format: dates.format, - translatedLabel: dates.translatedLabel + isDefault: true, } }) - if (fiscalYear?.message) { - // For a fiscal year, we need to replace "Last Year", "This Year", and add options for quarters - const fiscalYearStart = fiscalYear.message.year_start_date - const fiscalYearEnd = fiscalYear.message.year_end_date - - const q1 = { - label: `Q1: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q1")}: ${fiscalYear.message.name}`, - fromDate: fiscalYearStart, - toDate: dayjs(fiscalYearStart).add(3, 'month').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - - const q2 = { - label: `Q2: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q2")}: ${fiscalYear.message.name}`, - fromDate: dayjs(fiscalYearStart).add(3, 'month').format('YYYY-MM-DD'), - toDate: dayjs(fiscalYearStart).add(6, 'month').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - - const q3 = { - label: `Q3: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q3")}: ${fiscalYear.message.name}`, - fromDate: dayjs(fiscalYearStart).add(6, 'month').format('YYYY-MM-DD'), - toDate: dayjs(fiscalYearStart).add(9, 'month').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - - const q4 = { - label: `Q4: ${fiscalYear.message.name}`, - translatedLabel: `${_("Q4")}: ${fiscalYear.message.name}`, - fromDate: dayjs(fiscalYearStart).add(9, 'month').format('YYYY-MM-DD'), - toDate: fiscalYearEnd, - format: 'MMM YYYY' - } - - const thisYear = { - label: `This Fiscal Year`, - translatedLabel: `${_("This Fiscal Year")}`, - fromDate: fiscalYearStart, - toDate: fiscalYearEnd, - format: 'MMM YYYY' - } - - const lastYear = { - label: `Last Fiscal Year`, - translatedLabel: `${_("Last Fiscal Year")}`, - fromDate: dayjs(fiscalYearStart).subtract(1, 'year').format('YYYY-MM-DD'), - toDate: dayjs(fiscalYearEnd).subtract(1, 'year').format('YYYY-MM-DD'), - format: 'MMM YYYY' - } - // Sort the options so that we get "This Month", "Last Month", quarters, fiscal year, then the rest of the standard options - - const topRankedItems = standardOptions.filter((option) => { - return option.label === "This Month" || option.label === "Last Month" - }) - - const bottomRankedItems = standardOptions.filter((option) => { - return option.label !== "This Month" && option.label !== "Last Month" - }) - - return [...topRankedItems, q1, q2, q3, q4, thisYear, lastYear, ...bottomRankedItems] + if (!fiscalYear) { + return standardOptions } - return standardOptions + const currentStart = dayjs(fiscalYear.year_start_date) + const currentEnd = dayjs(fiscalYear.year_end_date) + + const quarterOptions: DateOption[] = [] + const fiscalYearOptions: DateOption[] = [] + + // Static literals so the translation extractor can find them. + const quarterLabels = [_("Q1"), _("Q2"), _("Q3"), _("Q4")] + + for (let yearsAgo = 0; yearsAgo <= PREVIOUS_FISCAL_YEARS; yearsAgo++) { + const start = currentStart.subtract(yearsAgo, 'year') + const end = currentEnd.subtract(yearsAgo, 'year') + // Keep the real name for the current year; derive it for the earlier ones. + const yearLabel = yearsAgo === 0 ? fiscalYear.name : fiscalYearLabel(start, end) + + for (let quarter = 0; quarter < 4; quarter++) { + const quarterStart = start.add(quarter * 3, 'month') + // End the day before the next quarter starts, clamped to the fiscal year end + // so a short fiscal year can't spill over. + const nextQuarterStart = start.add((quarter + 1) * 3, 'month') + const quarterEnd = nextQuarterStart.subtract(1, 'day').isAfter(end) + ? end + : nextQuarterStart.subtract(1, 'day') + + if (quarterStart.isAfter(end)) continue + + quarterOptions.push({ + key: `Q${quarter + 1}-${yearLabel}`, + label: `Q${quarter + 1}: ${yearLabel}`, + translatedLabel: `${quarterLabels[quarter]}: ${yearLabel}`, + fromDate: quarterStart.format(DATE_FORMAT), + toDate: quarterEnd.format(DATE_FORMAT), + format: 'MMM YYYY', + keywords: ['quarter', `q${quarter + 1}`, yearLabel], + // Only the current fiscal year's quarters clutter the default list; + // older ones stay searchable. + isDefault: yearsAgo === 0, + }) + } + + const label = yearsAgo === 0 + ? 'This Fiscal Year' + : yearsAgo === 1 + ? 'Last Fiscal Year' + : `FY ${yearLabel}` + + fiscalYearOptions.push({ + key: `fiscal-year-${yearLabel}`, + label, + translatedLabel: yearsAgo <= 1 ? _(label) : `${_("FY")} ${yearLabel}`, + fromDate: start.format(DATE_FORMAT), + toDate: end.format(DATE_FORMAT), + format: 'MMM YYYY', + keywords: ['fiscal year', yearLabel], + isDefault: yearsAgo <= 1, + }) + } + + // "This Month"/"Last Month" first, then quarters and fiscal years, then the rest. + const topRanked = standardOptions.filter((o) => o.label === 'This Month' || o.label === 'Last Month') + const bottomRanked = standardOptions.filter((o) => o.label !== 'This Month' && o.label !== 'Last Month') + + return [...topRanked, ...quarterOptions, ...fiscalYearOptions, ...bottomRanked] }, [fiscalYear]) + // Reconciliation only looks backwards, so a period that hasn't started is never useful. + const selectableOptions = useMemo( + () => allOptions.filter((option) => option.fromDate <= today), + [allOptions, today], + ) + const [open, setOpen] = useState(false) const [value, setValue] = useState("") + // We filter ourselves (`shouldFilter={false}`) so that the parsed-date suggestion can be a + // real CommandItem alongside the predefined options, and keyboard navigation covers both. + const filteredOptions = useMemo(() => { + const query = value.trim().toLowerCase() + + if (!query) { + return selectableOptions.filter((option) => option.isDefault) + } + + const tokens = query.split(/\s+/) + + return selectableOptions.filter((option) => { + const haystack = [ + option.label, + option.translatedLabel, + ...(option.keywords ?? []), + option.fromDate, + option.toDate, + ].join(' ').toLowerCase() + + return tokens.every((token) => haystack.includes(token)) + }) + }, [selectableOptions, value]) + + const parsedOption = useMemo(() => parseDateRange(value), [value]) + + // Filtering shortens the list, so pin the scroll back to the top to keep the + // auto-selected first option in view. + const listRef = useResetScrollOnSearch(value) + + // Don't show a parsed suggestion that duplicates an option already in the list. + const showParsedOption = parsedOption + && !filteredOptions.some((o) => o.fromDate === parsedOption.fromDate && o.toDate === parsedOption.toDate) + const timePeriod: TimePeriod | string = useMemo(() => { if (bankRecDate.fromDate && bankRecDate.toDate) { - // Check if the from and to dates match any predefined time period - for (const period of timePeriodOptions) { + for (const period of allOptions) { if (period.fromDate === bankRecDate.fromDate && period.toDate === bankRecDate.toDate) { return period.label; } @@ -114,10 +186,11 @@ const BankRecDateFilter = () => { } else { return "Date Range"; } - }, [bankRecDate.fromDate, bankRecDate.toDate, timePeriodOptions]); + }, [bankRecDate.fromDate, bankRecDate.toDate, allOptions]); const handleTimePeriodChange = (fromDate: string, toDate: string) => { setBankRecDate({ fromDate, toDate }) + setValue("") setOpen(false) } @@ -130,7 +203,9 @@ const BankRecDateFilter = () => { const direction = useDirection() - + const RangeArrow = direction === 'ltr' + ? + : return
@@ -141,30 +216,57 @@ const BankRecDateFilter = () => { size='md' className='rounded-e-none border-e-0' role="combobox"> - {timePeriodOptions.find((period) => period.label === timePeriod)?.translatedLabel ?? _(timePeriod)} + {allOptions.find((period) => period.label === timePeriod)?.translatedLabel ?? _(timePeriod)} - + - - - - - - {timePeriodOptions.map((period) => ( - handleTimePeriodChange(period.fromDate, period.toDate)}> - - {period.translatedLabel ?? _(period.label)} - - - {formatDate(period.fromDate, period.format)} {direction === 'ltr' ? : } {formatDate(period.toDate, period.format)} - - - ))} + + + {showParsedOption && parsedOption && ( + + handleTimePeriodChange(parsedOption.fromDate, parsedOption.toDate)}> + {value} + + {parsedOption.fromDate === parsedOption.toDate + ? formatDate(parsedOption.fromDate, 'Do MMM YYYY') + : <>{formatDate(parsedOption.fromDate, 'Do MMM YY')} {RangeArrow} {formatDate(parsedOption.toDate, 'Do MMM YY')}} + + + + )} + + {filteredOptions.length > 0 && ( + + {filteredOptions.map((period) => ( + handleTimePeriodChange(period.fromDate, period.toDate)}> + + {period.translatedLabel} + + + {formatDate(period.fromDate, period.format)} {RangeArrow} {formatDate(period.toDate, period.format)} + + + ))} + + )} + + {!showParsedOption && filteredOptions.length === 0 && ( +
+ {_("No results found")} +
+ )} @@ -199,77 +301,97 @@ const BankRecDateFilter = () => { } const referentialKeywords = ["last", "this", "next", "previous"] -const EmptyState = ({ onSelect, value }: { onSelect: (fromDate: string, toDate: string) => void, value: string }) => { - const dates = useMemo(() => { - if (value) { - // Try parsing the value - const parsedDate = parse(value, undefined, { forwardDate: false }) +/** chrono exposes `knownValues` on ParsingComponents but doesn't type it publicly. */ +const knownValuesOf = (components: unknown): Record => + (components as { knownValues?: Record })?.knownValues ?? {} - if (parsedDate && parsedDate.length > 0) { - const startDate = parsedDate[0].start.date() - const endDate = parsedDate[0].end?.date() +/** + * How far back a parsed date must move to land in the past. Reconciliation only ever looks + * backwards, so an ambiguous input that chrono resolves into the future - "December" typed in + * September, or a bare weekday like "Friday" - is pulled to its most recent past occurrence. + * An explicitly stated year is respected; a range that is still future gets discarded later. + * + * This returns a shift rather than a date so that a range can be moved as a single unit - + * shifting its start and end independently would distort or invert it. + */ +const pastShift = (date: Date, knownValues: Record) => { + const today = dayjs() + let candidate = dayjs(date) - if (!endDate) { - const today = new Date() - // If today is greater than the start date, use today as the end date - if (startDate.getTime() > today.getTime()) { - return { fromDate: today, toDate: startDate } - } else { - // Check if the user only wants a specific month like "May 2025" - // If the "known values" just has month and year, then we need to get the first day of the month and the last day of the month - // @ts-expect-error - "Known Values" is available in the start "ParsingComponents" - if (parsedDate[0].start.knownValues?.month && !parsedDate[0].start.knownValues?.day) { - return { - fromDate: startDate, - toDate: dayjs(startDate).endOf('month').toDate() - } - // @ts-expect-error - "Known Values" is available in the start "ParsingComponents" - } else if (parsedDate[0].start.knownValues?.month && parsedDate[0].start.knownValues?.day && !referentialKeywords.some(keyword => value.toLowerCase().includes(keyword))) { - // If month and day is known, then we should not assume that the user wants to get everything until today - return { - fromDate: startDate, - toDate: startDate, - } - } - - return { - fromDate: startDate, - toDate: today - } - } - } else { - return { fromDate: startDate, toDate: endDate } - } - } - - } - }, [value]) - - const onClick = (fromDate: Date, toDate: Date) => { - onSelect(formatDate(fromDate, 'YYYY-MM-DD'), formatDate(toDate, 'YYYY-MM-DD')) + if (!candidate.isAfter(today, 'date') || knownValues.year !== undefined) { + return { amount: 0, unit: 'year' as const } } - const isEqual = dates?.fromDate && dates?.toDate && dayjs(dates.fromDate).isSame(dates.toDate, 'date') + // A bare weekday repeats weekly, everything else (month/day) repeats yearly. + const unit = knownValues.weekday !== undefined && knownValues.day === undefined + ? 'day' as const + : 'year' as const + const step = unit === 'day' ? 7 : 1 + let amount = 0 - return
- {dates ? -
onClick(dates.fromDate, dates.toDate)}> - - {value} - - {isEqual ? - {formatDate(dates.fromDate, 'Do MMM YYYY')} - : - - {formatDate(dates.fromDate, 'Do MMM YY')} {formatDate(dates.toDate, 'Do MMM YY')} - } -
: - - No results found - - } -
+ for (let i = 0; i < 200 && candidate.isAfter(today, 'date'); i++) { + candidate = candidate.subtract(step, unit) + amount += step + } + + return { amount, unit } } -export default BankRecDateFilter \ No newline at end of file +/** + * Parse free text into a past date range, or return undefined when it can't be parsed or + * resolves entirely into the future. + */ +const parseDateRange = (value: string): { fromDate: string, toDate: string } | undefined => { + if (!value.trim()) return undefined + + const parsedDate = parse(value, undefined, { forwardDate: false }) + + if (!parsedDate || parsedDate.length === 0) return undefined + + const result = parsedDate[0] + const startKnownValues = knownValuesOf(result.start) + + // Anchor the shift on the start and apply it to both ends, so an explicit range like + // "1st Sept to 30th Sept" keeps its shape instead of having only its end rolled back. + const shift = pastShift(result.start.date(), startKnownValues) + const startDate = dayjs(result.start.date()).subtract(shift.amount, shift.unit).toDate() + const endDate = result.end + ? dayjs(result.end.date()).subtract(shift.amount, shift.unit).toDate() + : undefined + + const today = new Date() + let range: { fromDate: Date, toDate: Date } + + if (endDate) { + const endKnownValues = knownValuesOf(result.end) + // chrono ends "Apr 2025 to Jun 2025" on the 1st of June, but the user means all of it. + const rangeEnd = endKnownValues.month && !endKnownValues.day + ? dayjs(endDate).endOf('month').toDate() + : endDate + range = { fromDate: startDate, toDate: rangeEnd } + } else if (startKnownValues.month && !startKnownValues.day) { + // The user only wants a specific month like "May 2025" - span the whole month + range = { fromDate: dayjs(startDate).startOf('month').toDate(), toDate: dayjs(startDate).endOf('month').toDate() } + } else if (startKnownValues.month && startKnownValues.day && !referentialKeywords.some(keyword => value.toLowerCase().includes(keyword))) { + // If month and day is known, then we should not assume that the user wants to get everything until today + range = { fromDate: startDate, toDate: startDate } + } else { + range = { fromDate: startDate, toDate: today } + } + + // A range that hasn't started yet is never useful for reconciliation. A range that merely + // ends in the future is kept as typed, the same way "This Month" spans the whole month. + if (dayjs(range.fromDate).isAfter(today, 'date')) return undefined + + if (dayjs(range.toDate).isBefore(range.fromDate, 'date')) { + range = { fromDate: range.toDate, toDate: range.fromDate } + } + + return { + fromDate: dayjs(range.fromDate).format(DATE_FORMAT), + toDate: dayjs(range.toDate).format(DATE_FORMAT), + } +} + +export default BankRecDateFilter diff --git a/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx b/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx index 0815bc8a65e..592acfff844 100644 --- a/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx +++ b/banking/src/components/features/BankReconciliation/BankReconciliationStatement.tsx @@ -191,9 +191,9 @@ const BankReconciliationStatementView = () => { const content = _("Below is a list of all entries posted against the bank account {0} which have not been cleared till {1}.", [`${bankAccount?.account}`, `${formatDate(dates.toDate)}`]) - return
+ return
-
+
@@ -201,16 +201,18 @@ const BankReconciliationStatementView = () => { {error && } - {data && } + {data &&
} {data && data.message.result.length > 0 && ( -
-

{_("Bank Reconciliation Statement")}

+
+

{_("Bank Reconciliation Statement")}

row.payment_entry} - maxHeight="min(70vh, 640px)" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={_("No entries with a payment document in this list.")} />
diff --git a/banking/src/components/features/BankReconciliation/BankTransactionList.tsx b/banking/src/components/features/BankReconciliation/BankTransactionList.tsx index 1513e567a4b..a09994bf3e5 100644 --- a/banking/src/components/features/BankReconciliation/BankTransactionList.tsx +++ b/banking/src/components/features/BankReconciliation/BankTransactionList.tsx @@ -245,9 +245,9 @@ const BankTransactionListView = () => { const content = _("Below is a list of all bank transactions imported in the system for the bank account {0} between {1} and {2}.", [`${bankAccount?.account_name}`, `${formattedFromDate}`, `${formattedToDate}`]) - return
+ return
-
+
@@ -278,8 +278,9 @@ const BankTransactionListView = () => { data={filteredResults} columns={transactionColumns} getRowId={(row) => row.name} - maxHeight="calc(100vh - 200px)" - scrollAreaClassName="min-h-[calc(100vh-200px)]" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={ diff --git a/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx b/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx index fac7e2dc533..2293b41c771 100644 --- a/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx +++ b/banking/src/components/features/BankReconciliation/IncorrectlyClearedEntries.tsx @@ -181,9 +181,9 @@ const IncorrectlyClearedEntriesView = () => { const entriesContent = _("Entries below have a posting date after {0} but the clearance date is before {1}.", [`${formattedToDate}`, `${formattedToDate}`]) - return
+ return
-
+

@@ -198,13 +198,15 @@ const IncorrectlyClearedEntriesView = () => { {error && } {data && data.message.result.length > 0 && ( -
-

{_("Incorrectly cleared entries as per the report.")}

+
+

{_("Incorrectly cleared entries as per the report.")}

`${row.payment_entry}-${row.posting_date}`} - maxHeight="min(70vh, 640px)" + className="min-h-0 flex-1" + maxHeight="none" + scrollAreaClassName="flex-1" emptyState={_("No rows to display.")} />
diff --git a/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx b/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx index 7549cf74150..dce64e033d7 100644 --- a/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx +++ b/banking/src/components/features/BankReconciliation/MatchAndReconcile.tsx @@ -37,7 +37,7 @@ import { Link } from "react-router" import { Alert, AlertDescription, AlertTitle } from "@/components/ui/alert" import { InputGroup, InputGroupAddon, InputGroupText } from "@/components/ui/input-group" -const MatchAndReconcile = ({ contentHeight }: { contentHeight: number }) => { +const MatchAndReconcile = () => { const selectedBank = useAtomValue(selectedBankAccountAtom) if (!selectedBank) { @@ -52,15 +52,15 @@ const MatchAndReconcile = ({ contentHeight }: { contentHeight: number }) => { } return <> -
-
-

{_("Unreconciled Transactions")}

- +
+
+

{_("Unreconciled Transactions")}

+
- -
-

{_("Match or Create")}

- + +
+

{_("Match or Create")}

+
@@ -69,16 +69,19 @@ const MatchAndReconcile = ({ contentHeight }: { contentHeight: number }) => { } -/** TanStack requires `estimateSize` for initial scroll range; `measureElement` on each row sets the real height. */ +/** + * TanStack requires `estimateSize` for initial scroll range; `measureElement` on each row sets + * the real height. The scroll container fills its flex parent rather than taking a pixel + * height - the virtualizer observes its own rect, so it stays correct across resizes and any + * layout change above it. + */ function VirtualizedListBody({ items, - height, getItemKey, children, estimateSize = 74, }: { items: T[] - height: number getItemKey: (item: T, index: number) => string | number children: (item: T, index: number) => React.ReactNode estimateSize?: number @@ -100,8 +103,7 @@ function VirtualizedListBody({ return (
({ ) } -const UnreconciledTransactions = ({ contentHeight }: { contentHeight: number }) => { +const UnreconciledTransactions = () => { const bankAccount = useAtomValue(selectedBankAccountAtom) const currency = bankAccount?.account_currency ?? getCompanyCurrency(bankAccount?.company ?? '') @@ -187,14 +189,13 @@ const UnreconciledTransactions = ({ contentHeight }: { contentHeight: number }) } const hasFilters = search !== '' || typeFilter !== 'All' || amountFilter.value !== 0 - const listHeight = contentHeight - 72 if (isLoading) { return } - return
-
+ return
+
@@ -278,7 +279,6 @@ const UnreconciledTransactions = ({ contentHeight }: { contentHeight: number }) transaction.name} > @@ -381,7 +381,7 @@ const UnreconciledTransactionItem = ({ transaction }: { transaction: Unreconcile } -const VouchersSection = ({ contentHeight }: { contentHeight: number }) => { +const VouchersSection = () => { const selectedBank = useAtomValue(selectedBankAccountAtom) const selectedTransactions = useAtomValue(bankRecSelectedTransactionAtom(selectedBank?.name || '')) @@ -402,8 +402,8 @@ const VouchersSection = ({ contentHeight }: { contentHeight: number }) => { return } - return
- + return
+
} @@ -535,11 +535,11 @@ const OptionsForMultipleTransactions = ({ transactions }: { transactions: Unreco } -const OptionsForSingleTransaction = ({ transaction, contentHeight }: { transaction: UnreconciledTransaction, contentHeight: number }) => { +const OptionsForSingleTransaction = ({ transaction }: { transaction: UnreconciledTransaction }) => { const { setTransferModalOpen, setRecordPaymentModalOpen, setRecordJournalEntryModalOpen } = useKeyboardShortcuts() - return
+ return
@@ -602,7 +602,7 @@ const OptionsForSingleTransaction = ({ transaction, contentHeight }: { transacti
{transaction.matched_transaction_rule && } - +
} @@ -774,12 +774,11 @@ const RuleAction = ({ transaction }: { transaction: UnreconciledTransaction }) = ) } -const VouchersForTransaction = ({ transaction, contentHeight }: { transaction: UnreconciledTransaction, contentHeight: number }) => { +const VouchersForTransaction = ({ transaction }: { transaction: UnreconciledTransaction }) => { const { data: vouchers, isLoading, error } = useGetVouchersForTransaction(transaction) const voucherList = vouchers?.message ?? [] - const listHeight = contentHeight - 120 if (error) { return @@ -801,8 +800,8 @@ const VouchersForTransaction = ({ transaction, contentHeight }: { transaction: U
} - return
-
+ return
+
or @@ -818,7 +817,6 @@ const VouchersForTransaction = ({ transaction, contentHeight }: { transaction: U } voucher.name} > diff --git a/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx b/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx index 53ffba910a5..210f2b87e95 100644 --- a/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx +++ b/banking/src/components/features/BankReconciliation/SelectedTransactionDetails.tsx @@ -59,8 +59,8 @@ const SelectedTransactionDetails = ({ transaction, showAccount = false, account
- {transaction.description} - {transaction.reference_number ? {_("Ref")}: {transaction.reference_number} : null} + {transaction.description} + {transaction.reference_number ? {_("Ref")}: {transaction.reference_number} : null} {showAccount && account ? {_("GL Account")}: {account} : null}
diff --git a/banking/src/components/features/BankReconciliation/TransferModalContent.tsx b/banking/src/components/features/BankReconciliation/TransferModalContent.tsx index d24cafebe40..eba905f604b 100644 --- a/banking/src/components/features/BankReconciliation/TransferModalContent.tsx +++ b/banking/src/components/features/BankReconciliation/TransferModalContent.tsx @@ -490,7 +490,7 @@ const RecommendedTransferAccount = ({ transaction, onAccountChange }: { transact {formatDate(data.message.date, 'Do MMM YYYY')}
- {data.message.description} + {data.message.description}
diff --git a/banking/src/components/features/BankReconciliation/logos.ts b/banking/src/components/features/BankReconciliation/logos.ts index a89c59ee6fc..cb193e37773 100644 --- a/banking/src/components/features/BankReconciliation/logos.ts +++ b/banking/src/components/features/BankReconciliation/logos.ts @@ -231,7 +231,7 @@ export const BANK_LOGOS: { keywords: string[], logo: string, locale?: string[], { keywords: ['Federal Bank'], logo: 'Federal_Bank.png', - logoDark: 'Federal_Bank-dark.png', + logoDark: 'Federal_Bank-Dark.png', locale: ['India'] }, { diff --git a/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx b/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx index b8ef25961f5..073645754d1 100644 --- a/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx +++ b/banking/src/components/features/BankStatementImporter/CSV/StatementDetails.tsx @@ -83,10 +83,13 @@ const StatementDetails = ({ data }: Props) => { } + // `progress` is a percentage (drives the bar); `current`/`total` are actual counts. const [progress, setProgress] = useState(0) + const [imported, setImported] = useState({ current: 0, total: 0 }) useFrappeEventListener("bank-rec-statement-import-progress", (event) => { setProgress(event.progress) + setImported({ current: event.current ?? 0, total: event.total ?? 0 }) }) const file_name = data.doc.file.split("/").pop() ?? "" @@ -112,7 +115,9 @@ const StatementDetails = ({ data }: Props) => { {data.doc.status === 'Completed' ? {_("Completed")} : + {loading ? _("Importing...") : data.final_transactions?.length === 1 + ? _("Import 1 transaction") + : _("Import {0} transactions", [data.final_transactions?.length?.toString() || "0"])} }
@@ -129,7 +134,9 @@ const StatementDetails = ({ data }: Props) => {
{progress > 0 &&
- {_("Importing {0} transactions", [progress.toString()])} + {imported.total === 1 + ? _("Importing 1 transaction") + : _("Importing {0} of {1} transactions", [imported.current.toString(), imported.total.toString()])}
} diff --git a/banking/src/components/ui/list-view.tsx b/banking/src/components/ui/list-view.tsx index ddd0c0e7020..2833bf2dff6 100644 --- a/banking/src/components/ui/list-view.tsx +++ b/banking/src/components/ui/list-view.tsx @@ -387,7 +387,7 @@ function ListViewInner({ )} role="columnheader" > -
+
{header.isPlaceholder ? null : flexRender(header.column.columnDef.header, header.getContext())} diff --git a/banking/src/hooks/useFiscalYear.ts b/banking/src/hooks/useFiscalYear.ts index 14a25060ea0..e64950a0b71 100644 --- a/banking/src/hooks/useFiscalYear.ts +++ b/banking/src/hooks/useFiscalYear.ts @@ -1,13 +1,58 @@ import { useFrappeGetCall } from "frappe-react-sdk" +import { useMemo } from "react" +import dayjs from "dayjs" +import { useCurrentCompany } from "./useCurrentCompany" -const useFiscalYear = () => { - - return useFrappeGetCall("erpnext.accounts.utils.get_fiscal_year", undefined, 'fiscal_year', { - revalidateOnFocus: false, - revalidateIfStale: false, - revalidateOnReconnect: false - }) - +export type FiscalYear = { + name: string + year_start_date: string + year_end_date: string } -export default useFiscalYear \ No newline at end of file +/** + * The fiscal year containing today, for the currently selected company. + * + * `company` matters in multi-company setups, where fiscal years can be restricted to + * specific companies. `date` matters because without it `get_fiscal_year` returns the newest + * fiscal year in the system (they're ordered by start date, descending) - which may be one + * created in advance for a year that hasn't started. + */ +const useFiscalYear = () => { + const company = useCurrentCompany() + + const { data, ...rest } = useFrappeGetCall<{ message: FiscalYear | [string, string, string] | false }>( + "erpnext.accounts.utils.get_fiscal_year", + { + date: dayjs().format("YYYY-MM-DD"), + company, + as_dict: 1, + // Return nothing instead of throwing/msgprinting when no fiscal year covers today. + raise_on_missing: 0, + verbose: 0, + }, + company ? `fiscal_year_${company}` : null, + { + revalidateOnFocus: false, + revalidateIfStale: false, + revalidateOnReconnect: false + } + ) + + // get_fiscal_year returns a dict with as_dict, a (name, start, end) tuple without it, and + // false when there's no match - normalise all three. + const fiscalYear = useMemo(() => { + const message = data?.message + if (!message) return undefined + + if (Array.isArray(message)) { + const [name, year_start_date, year_end_date] = message + return { name, year_start_date, year_end_date } + } + + return message + }, [data]) + + return { fiscalYear, ...rest } +} + +export default useFiscalYear diff --git a/banking/src/hooks/useResetScrollOnSearch.ts b/banking/src/hooks/useResetScrollOnSearch.ts new file mode 100644 index 00000000000..8c2c2bb1721 --- /dev/null +++ b/banking/src/hooks/useResetScrollOnSearch.ts @@ -0,0 +1,23 @@ +import { useLayoutEffect, useRef } from "react" + +/** + * Pins a scrollable list back to the top whenever the search term changes. + * + * Dropdowns that do their own filtering (`shouldFilter={false}`) swap a long list for a much + * shorter one while the scroll container keeps its previous offset - which can leave the + * auto-selected first item scrolled out of view. + * + * Returns a ref to attach to the scroll container (e.g. `CommandList`). + */ +const useResetScrollOnSearch = (search: string) => { + const listRef = useRef(null) + + // Layout effect so the reset lands before paint, avoiding a visible jump. + useLayoutEffect(() => { + listRef.current?.scrollTo({ top: 0 }) + }, [search]) + + return listRef +} + +export default useResetScrollOnSearch diff --git a/banking/src/index.css b/banking/src/index.css index 808a76c5efd..f2a02509507 100644 --- a/banking/src/index.css +++ b/banking/src/index.css @@ -1,5 +1,6 @@ @import "tailwindcss"; @import "tw-animate-css"; +@import "./styles/scroll-fade.css"; @font-face { font-family: InterVariable; diff --git a/banking/src/pages/BankReconciliation.tsx b/banking/src/pages/BankReconciliation.tsx index 235c304a4a5..8e5a743088b 100644 --- a/banking/src/pages/BankReconciliation.tsx +++ b/banking/src/pages/BankReconciliation.tsx @@ -1,4 +1,4 @@ -import BankBalance from "@/components/features/BankReconciliation/BankBalance" +import BankAccountBalancePanel from "@/components/features/BankReconciliation/BankBalance" import BankPicker from "@/components/features/BankReconciliation/BankPicker" import BankRecDateFilter from "@/components/features/BankReconciliation/BankRecDateFilter" import BankTransactionUnreconcileModal from "@/components/features/BankReconciliation/BankTransactionUnreconcileModal" @@ -9,10 +9,9 @@ import ActionLog from "@/components/features/ActionLog/ActionLog" import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs" import { TooltipProvider } from "@/components/ui/tooltip" import _ from "@/lib/translate" -import { lazy, Suspense, useLayoutEffect, useRef, useState } from "react" +import { lazy, Suspense } from "react" import { AlertTriangleIcon, CheckCircleIcon, HomeIcon, LandmarkIcon, ListIcon, Loader2Icon, ScrollTextIcon, ShuffleIcon } from "lucide-react" import { Breadcrumb, BreadcrumbItem, BreadcrumbList, BreadcrumbPage, BreadcrumbSeparator } from "@/components/ui/breadcrumb" -import { Badge } from "@/components/ui/badge" import { Empty, EmptyContent, EmptyDescription, EmptyHeader, EmptyMedia, EmptyTitle } from "@/components/ui/empty" import { Button } from "@/components/ui/button" import { useAtomValue } from "jotai" @@ -25,23 +24,13 @@ const IncorrectlyClearedEntries = lazy(() => import('@/components/features/BankR const BankReconciliation = () => { - const [headerHeight, setHeaderHeight] = useState(0) - - const ref = useRef(null) - - useLayoutEffect(() => { - if (ref.current) { - setHeaderHeight(ref.current.clientHeight) - } - }, []) - - const remainingHeightAfterTabs = window.innerHeight - headerHeight - 220 - return (
-
-
-
+ {/* The page owns the viewport height and the tabs/lists below fill what's left, so + the virtualizers size themselves from layout instead of a measured pixel value. */} +
+
+
@@ -54,7 +43,7 @@ const BankReconciliation = () => {
- {_("Banking")} {_("Beta")} + {_("Banking")}
@@ -71,10 +60,8 @@ const BankReconciliation = () => {
- -
- +
@@ -104,42 +91,53 @@ const BankReconciliation = () => { ) } -const BankRecTabs = ({ remainingHeightAfterTabs }: { remainingHeightAfterTabs: number }) => { +const BankRecWorkspace = () => { const selectedBankAccount = useAtomValue(selectedBankAccountAtom) - if (!selectedBankAccount) { - return null - } - - return - - {_("Match and Reconcile")} - {_("Bank Reconciliation Statement")} - {_("Bank Transactions")} - {_("Bank Clearance Summary")} - {_("Incorrectly Cleared Entries")} - - - - - - + return + {/* Picker + tab strip stack on the left, balance panel beside them - the tab strip + fills height the panel needs anyway, so it costs no row of its own. The picker + scrolls horizontally (`min-w-0` lets it shrink so its overflow-x engages) while + the panel stays put, so the figures never scroll away. */} + {/* No gap here: the panel's own `border-s ps-4` supplies the separation, and a gap + would leave dead space the picker's edge fade can't reach. */} +
+
+ + {selectedBankAccount && + {_("Match and Reconcile")} + {_("Reconciliation Statement")} + {_("Transactions")} + {_("Clearance Summary")} + {_("Incorrectly Cleared")} + }
- }> - - + {selectedBankAccount && } +
+ + {selectedBankAccount && <> + + - - - - - - - - - - + + +
+ }> + + + + + + + + + + + + + + } } diff --git a/banking/src/pages/BankStatementImporter.tsx b/banking/src/pages/BankStatementImporter.tsx index 8e6e5345bd7..8a21110e538 100644 --- a/banking/src/pages/BankStatementImporter.tsx +++ b/banking/src/pages/BankStatementImporter.tsx @@ -226,7 +226,7 @@ const StatementImportLog = () => { field: "creation", order: "desc" }, - limit: 10 + limit: 20 }, bankAccount ? undefined : null, { revalidateOnFocus: false }) diff --git a/banking/src/styles/scroll-fade.css b/banking/src/styles/scroll-fade.css new file mode 100644 index 00000000000..2b9a3fde62f --- /dev/null +++ b/banking/src/styles/scroll-fade.css @@ -0,0 +1,94 @@ +/* Scroll-edge fade mask for horizontal scroll containers (the bank picker strip). + Ported from Raven's `scroll-fade-x`; imported by index.css, since Tailwind processes + `@utility` in imported files the same as in the entry file. + + The scroll-timeline keyframes reveal each edge's fade only when there IS content to scroll + in that direction - no fade on the left edge when scrolled fully left, none on the right at + the end. `@property` makes the fade animate smoothly rather than jumping. + + Without scroll-timeline support (Firefox) there is deliberately NO fade at all: the fade + vars stay at their 0px initial value and the gradient stops collapse to the edges. A static + both-edges fallback was tried in Raven and removed - on a container with nothing to scroll + it dimmed the edges anyway, promising content that didn't exist. */ + +@property --scroll-fade-l { + /* length-percentage, NOT length: the fade size is min(12%, …) - a percentage. A + property rejects that value and reverts to initial-value (0px), zeroing the fade. */ + syntax: ""; + inherits: false; + initial-value: 0px; +} + +@property --scroll-fade-r { + syntax: ""; + inherits: false; + initial-value: 0px; +} + +@keyframes scroll-fade-reveal-l { + from { + --scroll-fade-l: 0px; + } + + to { + --scroll-fade-l: var(--_scroll-fade-size-l); + } +} + +@keyframes scroll-fade-reveal-r { + from { + --scroll-fade-r: var(--_scroll-fade-size-r); + } + + to { + --scroll-fade-r: 0px; + } +} + +@utility scroll-fade-x { + --_scroll-fade-size-l: var(--scroll-fade-l-size, + var(--scroll-fade-size, min(12%, calc(var(--spacing, 0.25rem) * 10)))); + --_scroll-fade-size-r: var(--scroll-fade-r-size, + var(--scroll-fade-size, min(12%, calc(var(--spacing, 0.25rem) * 10)))); + /* Eased (smoothstep) alpha ramp, sampled finely so it reads as a smooth curve, NOT fading + all the way to transparent: the edge floors at 0.25 (content dims, never vanishes), ramping + up to a full 1 for the body. The opaque end MUST be 1 or everything would be permanently + dimmed. Stops collapse to the edge when the size animates to 0, so the true first/last card + is never dimmed at rest. Tune the floor - higher (~0.4) = subtler, lower (~0.1) = stronger. */ + --scroll-fade-inline: linear-gradient(to right, + rgba(0, 0, 0, 0.25) 0, + rgba(0, 0, 0, 0.282) calc(var(--scroll-fade-l, 0px) * 0.125), + rgba(0, 0, 0, 0.367) calc(var(--scroll-fade-l, 0px) * 0.25), + rgba(0, 0, 0, 0.487) calc(var(--scroll-fade-l, 0px) * 0.375), + rgba(0, 0, 0, 0.625) calc(var(--scroll-fade-l, 0px) * 0.5), + rgba(0, 0, 0, 0.763) calc(var(--scroll-fade-l, 0px) * 0.625), + rgba(0, 0, 0, 0.883) calc(var(--scroll-fade-l, 0px) * 0.75), + rgba(0, 0, 0, 0.968) calc(var(--scroll-fade-l, 0px) * 0.875), + rgba(0, 0, 0, 1) var(--scroll-fade-l, 0px), + rgba(0, 0, 0, 1) calc(100% - var(--scroll-fade-r, 0px)), + rgba(0, 0, 0, 0.968) calc(100% - var(--scroll-fade-r, 0px) * 0.875), + rgba(0, 0, 0, 0.883) calc(100% - var(--scroll-fade-r, 0px) * 0.75), + rgba(0, 0, 0, 0.763) calc(100% - var(--scroll-fade-r, 0px) * 0.625), + rgba(0, 0, 0, 0.625) calc(100% - var(--scroll-fade-r, 0px) * 0.5), + rgba(0, 0, 0, 0.487) calc(100% - var(--scroll-fade-r, 0px) * 0.375), + rgba(0, 0, 0, 0.367) calc(100% - var(--scroll-fade-r, 0px) * 0.25), + rgba(0, 0, 0, 0.282) calc(100% - var(--scroll-fade-r, 0px) * 0.125), + rgba(0, 0, 0, 0.25) 100%); + -webkit-mask-image: var(--scroll-fade-mask, var(--scroll-fade-inline)); + mask-image: var(--scroll-fade-mask, var(--scroll-fade-inline)); + -webkit-mask-composite: source-in; + mask-composite: intersect; + -webkit-mask-repeat: no-repeat; + mask-repeat: no-repeat; + + @supports (animation-timeline: scroll()) { + animation: + scroll-fade-reveal-l 1ms ease-in-out, + scroll-fade-reveal-r 1ms ease-in-out; + animation-timeline: scroll(self x), scroll(self x); + animation-range: + 0 var(--scroll-fade-reveal, calc(var(--spacing, 0.25rem) * 24)), + calc(100% - var(--scroll-fade-reveal, calc(var(--spacing, 0.25rem) * 24))) 100%; + animation-fill-mode: both; + } +} diff --git a/banking/yarn.lock b/banking/yarn.lock index bee24613a43..32077f95aaa 100644 --- a/banking/yarn.lock +++ b/banking/yarn.lock @@ -1489,10 +1489,10 @@ balanced-match@^4.0.2: resolved "https://registry.yarnpkg.com/balanced-match/-/balanced-match-4.0.4.tgz#bfb10662feed8196a2c62e7c68e17720c274179a" integrity sha512-BLrgEcRTwX2o6gGxGOCNyMvGSp35YofuYzw9h1IMTRmKqttAZZVU67bdb9Pr2vUHA8+j3i2tJfjO6C6+4myGTA== -baseline-browser-mapping@^2.10.38: - version "2.10.40" - resolved "https://registry.yarnpkg.com/baseline-browser-mapping/-/baseline-browser-mapping-2.10.40.tgz#f372c8eb36ff4ad0b5e7ae467014abef124554ba" - integrity sha512-BSSLZ9/Cjjv7Gtj5B68ZzXcXUg8iOf3fme+FCuh8rC/Go+Kmh8cox7M3A8dolou16s64QjLPOSdngh7GxXvkSw== +baseline-browser-mapping@^2.11.12: + version "2.11.20" + resolved "https://registry.yarnpkg.com/baseline-browser-mapping/-/baseline-browser-mapping-2.11.20.tgz#26078c7a4b08299656ea7ddceaebec955dc44303" + integrity sha512-H0ulySigv6icDJ1F7SjtdCD6PrhTpdYCmP0CactWy1+ekh0AFd0o1Wn5T8b+hnTmdBx19u9yhL6wvCylXMY7zw== brace-expansion@^5.0.5: version "5.0.7" @@ -1509,15 +1509,15 @@ brace-expansion@^5.0.8: balanced-match "^4.0.2" browserslist@^4.24.0: - version "4.28.4" - resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-4.28.4.tgz#dd8b8167a32845ff5f8cd6ce13f5abba16cd04c9" - integrity sha512-MTc8i/x9jBQd1iMw2CFGS+rwMa07eYjLR0CCTLDACl9xhxy+nIs3KeML/biicXtk9JrZ6dnnTatmc7ErPXIxqw== + version "4.28.8" + resolved "https://registry.yarnpkg.com/browserslist/-/browserslist-4.28.8.tgz#a3c79ceb70028527e5da7dafc887f3200b5168c0" + integrity sha512-V2NpofLblG64mfOtSgDhOJESZEGogzDMBv/q+W6oc4LXWP/q75eOXoOaaOu1EOadB9U4Bwx/e0yzbvwKH8zalA== dependencies: - baseline-browser-mapping "^2.10.38" - caniuse-lite "^1.0.30001799" - electron-to-chromium "^1.5.376" - node-releases "^2.0.48" - update-browserslist-db "^1.2.3" + baseline-browser-mapping "^2.11.12" + caniuse-lite "^1.0.30001809" + electron-to-chromium "^1.5.402" + node-releases "^2.0.53" + update-browserslist-db "^1.3.0" call-bind-apply-helpers@^1.0.1, call-bind-apply-helpers@^1.0.2: version "1.0.2" @@ -1527,10 +1527,10 @@ call-bind-apply-helpers@^1.0.1, call-bind-apply-helpers@^1.0.2: es-errors "^1.3.0" function-bind "^1.1.2" -caniuse-lite@^1.0.30001799: - version "1.0.30001800" - resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001800.tgz#b896c773e1c39400809415162bb5320371291b36" - integrity sha512-MMHtuAz9Ys840zAY5F4k6fV5GaivZ9sPk+nz0mY+GYVzRBnYkN0mpqkSR92oWRQ19yQWo4HvBV/FnC16AJX8MA== +caniuse-lite@^1.0.30001809: + version "1.0.30001810" + resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30001810.tgz#4970b477dea3278374de9bc43aa8f5d39fc3cda2" + integrity sha512-TITQPUkaz+aVk5GL6NhOdwk1aEaNTSDPsGFWrTuhKGtjTF70jL/Oht2W4c6rXUe5fu7Ie19VIahAXHIIiWWNeg== ccount@^2.0.0: version "2.0.1" @@ -1697,10 +1697,10 @@ dunder-proto@^1.0.1: es-errors "^1.3.0" gopd "^1.2.0" -electron-to-chromium@^1.5.376: - version "1.5.383" - resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.5.383.tgz#5bd22306497d454103b289b0fef97260c56d0855" - integrity sha512-I2484/KkAvl8lm9VyjH2JnbOIV0d/UCqT7gbzs6l+o6Vmn9wgB66uVcKX+Vk6HrXtY6fbWTOEXuv8waDTuFNCw== +electron-to-chromium@^1.5.402: + version "1.5.420" + resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.5.420.tgz#fc66d26a722d6f227e2092acdf38dd55b198cb44" + integrity sha512-2yD6XreGusOfNV+dUcvipJEXc3n/n7fgr7996aszTG+YY5E4mqM4tOq/3uhP129cazL9YHbVWSpc79ePotWtPA== engine.io-client@~6.5.1: version "6.5.4" @@ -3012,10 +3012,10 @@ natural-compare@^1.4.0: resolved "https://registry.yarnpkg.com/natural-compare/-/natural-compare-1.4.0.tgz#4abebfeed7541f2c27acfb29bdbbd15c8d5ba4f7" integrity sha512-OWND8ei3VtNC9h7V60qff3SVobHr996CTwgxubgyQYEpg290h9J0buyECNNJexkFm5sOajh5G116RYA1c8ZMSw== -node-releases@^2.0.48: - version "2.0.50" - resolved "https://registry.yarnpkg.com/node-releases/-/node-releases-2.0.50.tgz#597197a852071ce42fc2550e58e223242bcba969" - integrity sha512-J6l92tKHX6w8Jy5nO1Vuc01NoIiRGi/d6qBKVxh+IQ8Cr3b6HbVNfKiF8ZpFKufTwpwxMmce2W3iQZ861ZRyTg== +node-releases@^2.0.53: + version "2.0.54" + resolved "https://registry.yarnpkg.com/node-releases/-/node-releases-2.0.54.tgz#09af17d5647aa9f221ec5cf2becb95b68a981afe" + integrity sha512-YHs7BmmcsdAI5Ozuf8JZo6PT0mv2GIWC9vMfvUC3dp65M8hn7Ux8CPL+2oBI7juNuj9d0ndhTcznq2ODBps9cQ== object-assign@^4.1.1: version "4.1.1" @@ -3589,10 +3589,10 @@ unist-util-visit@^5.0.0: unist-util-is "^6.0.0" unist-util-visit-parents "^6.0.0" -update-browserslist-db@^1.2.3: - version "1.2.3" - resolved "https://registry.yarnpkg.com/update-browserslist-db/-/update-browserslist-db-1.2.3.tgz#64d76db58713136acbeb4c49114366cc6cc2e80d" - integrity sha512-Js0m9cx+qOgDxo0eMiFGEueWztz+d4+M3rGlmKPT+T4IS/jP4ylw3Nwpu6cpTTP8R1MAC1kF4VbdLt3ARf209w== +update-browserslist-db@^1.3.0: + version "1.3.2" + resolved "https://registry.yarnpkg.com/update-browserslist-db/-/update-browserslist-db-1.3.2.tgz#9d99fbff56c50bb11ba5fd35cece5916da595836" + integrity sha512-UQ+MSxlhRm1bzjhU+DcuXfjFO1FzNtqhK5+9Yvlp90ItDLk5vT932A0rFu619nf7RVS+Y/VeaUW1jaRDqZ8VJw== dependencies: escalade "^3.2.0" picocolors "^1.1.1" diff --git a/erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json b/erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json index 01701ff7e3a..98812e17452 100644 --- a/erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json +++ b/erpnext/accounts/dashboard_chart/profit_and_loss/profit_and_loss.json @@ -9,7 +9,7 @@ "idx": 0, "is_public": 1, "is_standard": 1, - "modified": "2025-12-19 12:37:31.673782", + "modified": "2026-09-04 12:37:31.673782", "modified_by": "Administrator", "module": "Accounts", "name": "Profit and Loss", @@ -17,7 +17,6 @@ "owner": "Administrator", "report_name": "Profit and Loss Statement", "roles": [], - "show_values_over_chart": 1, "timeseries": 0, "type": "Line", "use_report_chart": 1, diff --git a/erpnext/accounts/doctype/account/account.json b/erpnext/accounts/doctype/account/account.json index e65d41fde19..5ed1b59a45c 100644 --- a/erpnext/accounts/doctype/account/account.json +++ b/erpnext/accounts/doctype/account/account.json @@ -122,6 +122,7 @@ "description": "Setting Account Type helps in selecting this Account in transactions.", "fieldname": "account_type", "fieldtype": "Select", + "in_preview": 1, "in_standard_filter": 1, "label": "Account Type", "oldfieldname": "account_type", @@ -203,7 +204,7 @@ "idx": 1, "is_tree": 1, "links": [], - "modified": "2026-08-21 23:11:37.851001", + "modified": "2026-09-03 12:59:42.190900", "modified_by": "Administrator", "module": "Accounts", "name": "Account", diff --git a/erpnext/accounts/doctype/account/account_tree.js b/erpnext/accounts/doctype/account/account_tree.js index 0248fab4602..9f06bdf4dbd 100644 --- a/erpnext/accounts/doctype/account/account_tree.js +++ b/erpnext/accounts/doctype/account/account_tree.js @@ -52,6 +52,42 @@ frappe.treeview_settings["Account"] = { ], root_label: "Accounts", get_tree_nodes: "erpnext.accounts.utils.get_children", + get_label: function (node) { + // clean display name — the account number renders as a badge (see + // onrender) instead of being glued into the name + return frappe.utils.escape_html(node.data.account_name || node.title || node.label); + }, + onrender: function (node) { + if (node.is_root || !node.data) return; + + const flags = []; + if (node.data.account_number) { + flags.push(frappe.ui.badge({ label: node.data.account_number })); + } + + const company = frappe.treeview_settings["Account"].treeview?.page?.fields_dict?.company?.get_value(); + const company_currency = company && erpnext.get_currency(company); + if ( + node.data.account_currency && + company_currency && + node.data.account_currency !== company_currency + ) { + flags.push(frappe.ui.badge({ label: node.data.account_currency, theme: "blue" })); + } + + if (node.data.freeze_account === "Yes") { + flags.push( + frappe.ui.badge({ + label: __("Frozen"), + icon: "lock", + title: __("Frozen - entries restricted"), + theme: "orange", + }) + ); + } + + erpnext.utils.render_tree_node_flags(node, flags); + }, on_node_render: function (node, deep) { const render_balances = () => { for (let account of cur_tree.account_balance_data) { @@ -232,7 +268,7 @@ frappe.treeview_settings["Account"] = { frappe.treeview_settings["Account"].treeview["tree"] = treeview.tree; if (treeview.can_create) { treeview.page.set_primary_action( - __("New"), + { label: __("Add Account"), short_label: __("Add") }, function () { let root_company = treeview.page.fields_dict.root_company.get_value(); if (root_company) { @@ -243,13 +279,14 @@ frappe.treeview_settings["Account"] = { treeview.new_node(); } }, - "add" + "plus" ); } }, toolbar: [ { label: __("Add Child"), + icon: "plus", condition: function (node) { return ( frappe.boot.user.can_create.indexOf("Account") !== -1 && @@ -272,6 +309,7 @@ frappe.treeview_settings["Account"] = { return !node.root && frappe.boot.user.can_read.indexOf("GL Entry") !== -1; }, label: __("View Ledger"), + icon: "book-open", click: function (node, btn) { frappe.route_options = { from_date: erpnext.utils.get_fiscal_year(frappe.datetime.get_today(), true)[1], @@ -286,6 +324,106 @@ frappe.treeview_settings["Account"] = { }, btnClass: "hidden-xs", }, + { + // same label and mechanism as the Account form's Actions button: + // NOT frappe's generic rename (Allow Rename stays off) — this is + // ERPNext's controlled update that rebuilds the derived + // "number - name - abbr" document name + label: __("Update Account Name / Number"), + icon: "text-cursor-input", + condition: function (node) { + return !node.is_root && frappe.model.can_write("Account"); + }, + click: function (node) { + const dialog = new frappe.ui.Dialog({ + title: __("Update Account Number / Name"), + fields: [ + { + fieldtype: "Data", + fieldname: "account_name", + label: __("Account Name"), + reqd: 1, + default: node.data.account_name, + }, + { + fieldtype: "Data", + fieldname: "account_number", + label: __("Account Number"), + default: node.data.account_number, + }, + ], + primary_action_label: __("Update"), + primary_action(values) { + dialog.hide(); + frappe.dom.freeze(__("Updating {0}", [node.label])); + frappe.call({ + method: "erpnext.accounts.doctype.account.account.update_account_number", + args: { + name: node.label, + account_name: values.account_name, + account_number: values.account_number, + }, + callback: function (r) { + if (r.exc) return; + const treeview = frappe.views.trees["Account"]; + node.parent_node && treeview.tree.load_children(node.parent_node); + }, + always: function () { + frappe.dom.unfreeze(); + }, + }); + }, + }); + dialog.show(); + }, + }, + { + label: __("Convert to Group"), + icon: "folder-tree", + condition: function (node) { + return !node.is_root && !node.expandable && frappe.model.can_write("Account"); + }, + click: function (node) { + erpnext.accounts.convert_tree_node("Account", node, "convert_ledger_to_group"); + }, + }, + { + label: __("Convert to Non-Group"), + icon: "file-text", + condition: function (node) { + // only on groups the user has opened and found empty — a + // group with children can't convert, so don't offer it + return ( + !node.is_root && + node.expandable && + node.loaded && + !node.$ul.children().length && + frappe.model.can_write("Account") + ); + }, + click: function (node) { + erpnext.accounts.convert_tree_node("Account", node, "convert_group_to_ledger"); + }, + }, ], extend_toolbar: true, }; + +frappe.provide("erpnext.accounts"); +// shared by the Account and Cost Center tree views (defined in both files, +// whichever loads first wins): run the doctype's whitelisted convert method, +// then re-render the branch so the node's group/leaf state updates +erpnext.accounts.convert_tree_node = + erpnext.accounts.convert_tree_node || + function (doctype, node, method) { + frappe.call({ + method: "run_doc_method", + args: { dt: doctype, dn: node.label, method: method }, + callback: function (r) { + if (r.exc) return; + const treeview = frappe.views.trees[doctype]; + node.parent_node && treeview.tree.load_children(node.parent_node); + frappe.show_alert({ message: __("{0} converted", [node.label]), indicator: "green" }); + }, + }); + }; diff --git a/erpnext/accounts/doctype/account/chart_of_accounts/chart_of_accounts.py b/erpnext/accounts/doctype/account/chart_of_accounts/chart_of_accounts.py index 89530b56e81..f11eb7855c2 100644 --- a/erpnext/accounts/doctype/account/chart_of_accounts/chart_of_accounts.py +++ b/erpnext/accounts/doctype/account/chart_of_accounts/chart_of_accounts.py @@ -102,6 +102,8 @@ def identify_is_group(child): def get_chart(chart_template: str | None, existing_company: str | None = None): chart = {} if existing_company: + frappe.has_permission("Company", doc=existing_company, throw=True) + return get_account_tree_from_existing_company(existing_company) elif chart_template == "Standard": diff --git a/erpnext/accounts/doctype/accounts_settings/accounts_settings.json b/erpnext/accounts/doctype/accounts_settings/accounts_settings.json index 62850ca6ff2..de712eff005 100644 --- a/erpnext/accounts/doctype/accounts_settings/accounts_settings.json +++ b/erpnext/accounts/doctype/accounts_settings/accounts_settings.json @@ -225,7 +225,8 @@ "description": "The percentage you are allowed to bill more against the amount ordered. For example, if the order value is $100 for an item and tolerance is set as 10%, then you are allowed to bill up to $110 ", "fieldname": "over_billing_allowance", "fieldtype": "Currency", - "label": "Over Billing Allowance (%)" + "label": "Over Billing Allowance (%)", + "non_negative": 1 }, { "default": "1", @@ -805,7 +806,7 @@ "index_web_pages_for_search": 1, "issingle": 1, "links": [], - "modified": "2026-08-14 15:26:49.070889", + "modified": "2026-09-04 10:08:30.115003", "modified_by": "Administrator", "module": "Accounts", "name": "Accounts Settings", diff --git a/erpnext/accounts/doctype/accounts_settings/accounts_settings.py b/erpnext/accounts/doctype/accounts_settings/accounts_settings.py index f742cb30efe..b98c0a355a4 100644 --- a/erpnext/accounts/doctype/accounts_settings/accounts_settings.py +++ b/erpnext/accounts/doctype/accounts_settings/accounts_settings.py @@ -222,6 +222,13 @@ class AccountsSettings(Document): set_allow_on_submit_for_dimension_fields(doctypes) +@frappe.whitelist(methods=["POST"]) +def get_posting_date_confirmation() -> int: + return cint( + frappe.db.get_single_value("Accounts Settings", "confirm_before_resetting_posting_date", cache=False) + ) + + def toggle_accounting_dimension_sections(hide): accounting_dimension_doctypes = frappe.get_hooks("accounting_dimension_doctypes") for doctype in accounting_dimension_doctypes: diff --git a/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.py b/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.py index d41e13ffe43..47ae22060e2 100644 --- a/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.py +++ b/erpnext/accounts/doctype/accounts_settings/test_accounts_settings.py @@ -1,9 +1,15 @@ import frappe +from erpnext.accounts.doctype.accounts_settings.accounts_settings import get_posting_date_confirmation from erpnext.tests.utils import ERPNextTestSuite class TestAccountsSettings(ERPNextTestSuite): + def test_posting_date_confirmation_uses_current_setting(self): + for enabled in (0, 1, 0): + frappe.db.set_single_value("Accounts Settings", "confirm_before_resetting_posting_date", enabled) + self.assertEqual(get_posting_date_confirmation(), enabled) + def test_stale_days(self): cur_settings = frappe.get_doc("Accounts Settings", "Accounts Settings") cur_settings.allow_stale = 0 diff --git a/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py b/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py index 38c81232252..34fe8447816 100644 --- a/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py +++ b/erpnext/accounts/doctype/bank_reconciliation_tool/bank_reconciliation_tool.py @@ -912,7 +912,7 @@ def search_for_transfer_transaction(transaction_id: str | int): days = frappe.db.get_single_value("Accounts Settings", "transfer_match_days") - if not days: + if days is None: days = 3 min_date = frappe.utils.add_days(date, -days) @@ -1336,9 +1336,11 @@ def get_pe_matching_query( ref_condition = pe.reference_no == transaction.reference_number ref_rank = frappe.qb.terms.Case().when(ref_condition, 1).else_(0) - amount_equality = pe.paid_amount == transaction.unallocated_amount + amount_field = pe.received_amount_after_tax if account_from_to == "paid_to" else pe.paid_amount_after_tax + + amount_equality = amount_field == transaction.unallocated_amount amount_rank = frappe.qb.terms.Case().when(amount_equality, 1).else_(0) - amount_condition = amount_equality if exact_match else pe.paid_amount > 0.0 + amount_condition = amount_equality if exact_match else amount_field > 0.0 party_condition = ( (pe.party_type == transaction.party_type) & (pe.party == transaction.party) & pe.party.isnotnull() @@ -1355,7 +1357,7 @@ def get_pe_matching_query( (ref_rank + amount_rank + party_rank + 1).as_("rank"), ConstantColumn("Payment Entry").as_("doctype"), pe.name, - pe.base_paid_amount_after_tax.as_("paid_amount"), + amount_field.as_("paid_amount"), pe.reference_no, pe.reference_date, pe.party, diff --git a/erpnext/accounts/doctype/bank_reconciliation_tool/test_bank_reconciliation_tool.py b/erpnext/accounts/doctype/bank_reconciliation_tool/test_bank_reconciliation_tool.py index 031f74f1a85..5bad7582dde 100644 --- a/erpnext/accounts/doctype/bank_reconciliation_tool/test_bank_reconciliation_tool.py +++ b/erpnext/accounts/doctype/bank_reconciliation_tool/test_bank_reconciliation_tool.py @@ -10,6 +10,7 @@ from erpnext.accounts.doctype.bank_reconciliation_tool.bank_reconciliation_tool auto_reconcile_vouchers, get_auto_reconcile_message, get_bank_transactions, + get_linked_payments, ) from erpnext.accounts.doctype.payment_entry.test_payment_entry import create_payment_entry from erpnext.accounts.test.accounts_mixin import AccountsTestMixin @@ -99,13 +100,14 @@ class TestBankReconciliationTool(ERPNextTestSuite, AccountsTestMixin): transactions = get_bank_transactions(self.bank_account, from_date, to_date) self.assertEqual(len(transactions), 0) - def make_bank_transaction(self, date, deposit=100): + def make_bank_transaction(self, date, deposit=100, withdrawal=0): return ( frappe.get_doc( { "doctype": "Bank Transaction", "date": date, "deposit": deposit, + "withdrawal": withdrawal, "bank_account": self.bank_account, "currency": "INR", } @@ -114,11 +116,73 @@ class TestBankReconciliationTool(ERPNextTestSuite, AccountsTestMixin): .submit() ) + def get_matching_payment_entries(self, bank_transaction, exact_match=False): + document_types = ["payment_entry", "exact_match"] if exact_match else ["payment_entry"] + vouchers = get_linked_payments( + bank_transaction, + document_types, + from_date=add_days(today(), -1), + to_date=today(), + ) + return [v for v in vouchers if v.get("doctype") == "Payment Entry"] + def test_get_bank_transactions_excludes_dates_after_to_date(self): self.make_bank_transaction(date=today()) names = [t.name for t in get_bank_transactions(self.bank_account, to_date=add_days(today(), -1))] self.assertEqual(names, []) + def test_deposit_matches_amount_received_in_bank_account(self): + # money leaves another bank account and lands here minus a charge, so the two sides differ + payment = frappe.get_doc( + { + "doctype": "Payment Entry", + "payment_type": "Internal Transfer", + "company": self.company, + "posting_date": today(), + "paid_from": "_Test Bank - _TC", + "paid_to": self.bank, + "paid_amount": 3537.64, + "received_amount": 3460.52, + "reference_no": "TRF-001", + "reference_date": today(), + } + ) + payment.set_missing_values() + payment.set_exchange_rate() + payment.set_amounts() + payment.deductions[-1].account = "_Test Exchange Gain/Loss - _TC" + payment.deductions[-1].cost_center = "_Test Cost Center - _TC" + payment = payment.save().submit() + + transaction = self.make_bank_transaction(date=today(), deposit=3460.52) + + # the received side is what reached this bank account, so that is what is shown + matches = self.get_matching_payment_entries(transaction.name) + self.assertEqual([m["name"] for m in matches], [payment.name]) + self.assertEqual(matches[0]["paid_amount"], 3460.52) + + # and what the exact match compares against + exact_matches = self.get_matching_payment_entries(transaction.name, exact_match=True) + self.assertEqual([m["name"] for m in exact_matches], [payment.name]) + + def test_withdrawal_matches_amount_paid_from_bank_account(self): + payment = create_payment_entry( + company=self.company, + payment_type="Pay", + party_type="Supplier", + party="_Test Supplier", + paid_from=self.bank, + paid_to="Creditors - _TC", + paid_amount=1250, + ) + payment = payment.save().submit() + + transaction = self.make_bank_transaction(date=today(), deposit=0, withdrawal=1250) + + exact_matches = self.get_matching_payment_entries(transaction.name, exact_match=True) + self.assertEqual([m["name"] for m in exact_matches], [payment.name]) + self.assertEqual(exact_matches[0]["paid_amount"], 1250) + def test_auto_reconcile_message_for_no_matches(self): message, indicator = get_auto_reconcile_message([], []) self.assertEqual(indicator, "blue") diff --git a/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py b/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py index 468bce0e1fd..ee9941ff3eb 100644 --- a/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py +++ b/erpnext/accounts/doctype/bank_statement_import_log/bank_statement_import_log.py @@ -375,8 +375,7 @@ class BankStatementImportLog(Document): table["column_mapping"] = guess_column_mapping_by_content(table["rows"]) final_transactions, table["date_format"], table["amount_format"] = build_table_transactions(table) - # Tables with no detectable transactions (ads, summaries, headers) start excluded. - table["included"] = bool(final_transactions) + table["included"] = should_include_table(table, final_transactions) self.pdf_tables = json.dumps(tables) return tables @@ -542,6 +541,8 @@ class BankStatementImportLog(Document): "bank-rec-statement-import-progress", { "progress": round(progress / total_transactions * 100), + "current": progress, + "total": total_transactions, }, doctype="Bank Statement Import Log", docname=self.name, @@ -551,6 +552,7 @@ class BankStatementImportLog(Document): "bank-rec-statement-import-progress", { "progress": 100, + "current": total_transactions, "total": total_transactions, }, doctype="Bank Statement Import Log", @@ -821,6 +823,15 @@ def compute_final_transactions(transaction_rows: list, date_format: str, amount_ """Pure version of the final-transaction builder (date normalized, amount split).""" final_transactions = [] + # Which marker does this statement actually write? A statement that only ever says "Cr" + # is marking the credits as its exceptions, so an unmarked row is a withdrawal; one that + # only ever says "Dr" means the opposite. With both markers present an unmarked row is + # genuinely undetermined, so it stays a withdrawal. + unmarked_is_deposit = False + if amount_format == 'Amount column has "CR"/"DR" values': + markers = {get_amount_cr_dr_marker(row.get("amount")) for row in transaction_rows} + unmarked_is_deposit = markers - {None} == {"dr"} + def parse_amount(transaction_row: dict): if amount_format == "Separate columns for withdrawal and deposit": return get_float_amount(transaction_row.get("withdrawal")), get_float_amount( @@ -829,44 +840,43 @@ def compute_final_transactions(transaction_rows: list, date_format: str, amount_ if amount_format == 'Amount column has "CR"/"DR" values': amount = transaction_row.get("amount") + marker = get_amount_cr_dr_marker(amount) + # The marker carries the direction, so the amount's own sign is ignored. + signed_amount = get_float_amount(amount) or 0 - # If the amount column has CR/DR in it - we should remove any signs (negative or positive) from the amount - float_amount = abs(get_float_amount(amount) or 0) - if "cr" in amount.lower(): - return 0, float_amount - else: - return float_amount, 0 + if marker: + return (0, abs(signed_amount)) if marker == "cr" else (abs(signed_amount), 0) + # An unmarked row takes the opposite direction to the marker this statement + # uses. A negative amount reverses that again (a refund). + is_deposit = unmarked_is_deposit + if signed_amount < 0: + is_deposit = not is_deposit + + return (0, abs(signed_amount)) if is_deposit else (abs(signed_amount), 0) + + # `or 0` below: get_float_amount returns None for an unparseable cell, and a blank + # transaction-type cell comes through as None. Both used to raise. if amount_format == "Amount column has positive/negative values": - amount = get_float_amount(transaction_row.get("amount", "0")) + amount = get_float_amount(transaction_row.get("amount", "0")) or 0 if amount > 0: return 0, abs(amount) else: return abs(amount), 0 + transaction_type = str(transaction_row.get("debit_credit") or "").strip().lower() + amount = abs(get_float_amount(transaction_row.get("amount", "0")) or 0) + if amount_format == 'Transaction type column has "CR"/"DR" values': - transaction_type = transaction_row.get("debit_credit") - amount = get_float_amount(transaction_row.get("amount", "0")) - if "cr" in transaction_type.lower(): - return 0, abs(amount) - else: - return abs(amount), 0 + # "credit" contains "cr". "debit" does not contain "dr", so it correctly falls + # through to the withdrawal side. + return (0, amount) if "cr" in transaction_type else (amount, 0) if amount_format == 'Transaction type column has "C"/"D" values': - transaction_type = transaction_row.get("debit_credit") - amount = get_float_amount(transaction_row.get("amount", "0")) - if transaction_type.lower().strip() == "c": - return 0, abs(amount) - else: - return abs(amount), 0 + return (0, amount) if transaction_type == "c" else (amount, 0) if amount_format == 'Transaction type column has "Deposit"/"Withdrawal" values': - transaction_type = transaction_row.get("debit_credit") - amount = get_float_amount(transaction_row.get("amount", "0")) - if "deposit" in transaction_type.lower(): - return 0, abs(amount) - else: - return abs(amount), 0 + return (0, amount) if "deposit" in transaction_type else (amount, 0) return 0, 0 @@ -910,6 +920,26 @@ def build_table_transactions(table: dict): return final_transactions, date_format, amount_format +def should_include_table(table: dict, final_transactions: list) -> bool: + """ + Whether a freshly extracted PDF table should START as included - only the default state + of the checkbox, which the user can change afterwards. + + It must have yielded transactions, and it must have a Description column mapped. A + transaction table always carries a narration; the summary boxes printed around it - + payment due, credit limit, reward points - are dates and figures only. Otherwise the + HDFC credit-card "Payment Due Date / Total Dues / Minimum Amount Due" box parses as one + transaction and imports a phantom row. + + A description is NOT needed to import (it is not mandatory on Bank Transaction), so a + bank that omits narration still works - its table just starts unticked. + """ + if not final_transactions: + return False + + return any(column.get("maps_to") == "Description" for column in table.get("column_mapping", [])) + + def _clean_cell(cell) -> str: """Normalize a pdfplumber cell: None -> '', collapse wrapped newlines, strip.""" if cell is None: @@ -1055,6 +1085,43 @@ def get_float_amount(amount): return amount +# A "CR"/"DR" marker on the amount itself, at either end: "2,378.00Cr", "Cr 100", +# "INR 50.90 Cr.", "DR 1,234.50". +# `(?![a-zA-Z])` rather than `\b` on the leading form: there is no word boundary between +# the "r" of "Cr100" and the digit, but there IS one inside "CREDIT" and "DRAFT". +AMOUNT_CR_DR_PATTERN = re.compile(r"^\s*(cr|dr)(?![a-zA-Z])\.?|(?:^|[\s\d.)])(cr|dr)\b\.?\s*$", re.IGNORECASE) + + +def get_amount_cr_dr_marker(amount) -> str | None: + """ + Return "cr" or "dr" if the amount cell carries a direction marker of its own, else None. + + What is left after removing the marker has to look like an amount - it must hold a digit + and at most a short currency token - so that text which merely starts or ends with the + letters is not read as a marker. That guard is what separates "Cr 100" from a + description that bled into the amount column, like "Dr Smith Clinic 500". + """ + if not isinstance(amount, str): + return None + + match = AMOUNT_CR_DR_PATTERN.search(amount) + if not match: + return None + + # Only the marker itself is removed - the surrounding character the pattern needed to + # anchor on (a digit, say) stays part of the remainder. + group = 1 if match.group(1) else 2 + start, end = match.span(group) + remainder = amount[:start] + amount[end:] + + if not any(char.isdigit() for char in remainder): + return None + if sum(char.isalpha() for char in remainder) > 3: + return None + + return match.group(group).lower() + + def get_file_properties(transactions: list): """ From the transaction rows, try to figure out the following: @@ -1075,6 +1142,8 @@ def get_file_properties(transactions: list): 'Transaction type column has "C"/"D" values': 0, } + amount_column_has_cr_dr = False + for transaction in transactions: date_format = transaction.get("date_format") @@ -1092,33 +1161,40 @@ def get_file_properties(transactions: list): if not amount: continue - if isinstance(amount, str) and ("cr" in amount.lower() or "dr" in amount.lower()): + debit_credit = str(transaction.get("debit_credit") or "").strip().lower() + + # One vote per row, most specific signal first. Order matters: "withdrawal" contains + # "dr", so it must be matched before the loose cr/dr check or a Deposit/Withdrawal + # column reads as CR/DR. "debit" needs listing because, unlike "credit", it does not + # contain "dr". The final else means every row votes, even an unrecognised type. + if get_amount_cr_dr_marker(amount): + amount_column_has_cr_dr = True amount_format_frequency['Amount column has "CR"/"DR" values'] += 1 - - # Check if there's a debit_credit column containing "cr"/"dr" - if transaction.get("debit_credit", None): - if ( - "cr" in transaction.get("debit_credit", "").lower() - or "dr" in transaction.get("debit_credit", "").lower() - ): - amount_format_frequency['Transaction type column has "CR"/"DR" values'] += 1 - elif ( - "deposit" in transaction.get("debit_credit", "").lower() - or "withdrawal" in transaction.get("debit_credit", "").lower() - ): - amount_format_frequency['Transaction type column has "Deposit"/"Withdrawal" values'] += 1 - elif (transaction.get("debit_credit", "").lower().strip() == "c") or ( - transaction.get("debit_credit", "").lower().strip() == "d" - ): - amount_format_frequency['Transaction type column has "C"/"D" values'] += 1 - - # Else assume that the amount is expressed as positive/negative value + elif "deposit" in debit_credit or "withdrawal" in debit_credit: + amount_format_frequency['Transaction type column has "Deposit"/"Withdrawal" values'] += 1 + elif debit_credit in ("c", "d"): + amount_format_frequency['Transaction type column has "C"/"D" values'] += 1 + elif any(token in debit_credit for token in ("cr", "dr", "debit")): + amount_format_frequency['Transaction type column has "CR"/"DR" values'] += 1 else: + # Nothing said which direction this is, so assume the amount carries the sign. amount_format_frequency["Amount column has positive/negative values"] += 1 most_common_date_format = max(date_format_frequency, key=date_format_frequency.get) most_common_amount_format = max(amount_format_frequency, key=amount_format_frequency.get) + # With no votes at all (no rows, or every amount blank) max() would return whichever key + # happens to be first in the dict. Say what we mean instead. + if not amount_format_frequency[most_common_amount_format]: + most_common_amount_format = "Amount column has positive/negative values" + + # A CR/DR amount column is proved by a single marker, not by a majority: both formats + # describe the same column, and an unmarked row is only the default direction, not + # evidence against the notation. Statements mark just the exceptions - one HDFC + # credit-card page has 18 rows and a single "50.90Cr". + if amount_column_has_cr_dr and most_common_amount_format == "Amount column has positive/negative values": + most_common_amount_format = 'Amount column has "CR"/"DR" values' + return most_common_date_format, most_common_amount_format diff --git a/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py b/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py index 5d2c02ec305..6caca5441f2 100644 --- a/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py +++ b/erpnext/accounts/doctype/bank_statement_import_log/test_bank_statement_import_log.py @@ -11,12 +11,14 @@ from erpnext.accounts.doctype.bank_statement_import_log.bank_statement_import_lo detect_column_mapping, detect_header_row, extract_pdf_tables, + get_amount_cr_dr_marker, get_float_amount, get_statement_details, guess_column_mapping_by_content, reextract_pdf_table, set_header_index, set_pdf_table_header, + should_include_table, update_column_mapping, update_pdf_tables, ) @@ -124,6 +126,184 @@ class TestBankStatementImportLog(ERPNextTestSuite, AccountsTestMixin): self.assertIsNone(get_float_amount("ABCD")) self.assertIsNone(get_float_amount("****")) + # ------------------------------------------------------------------ # + # Amount format detection + # ------------------------------------------------------------------ # + + def test_amount_cr_dr_marker(self): + """The marker is read at either end of the cell, but only next to the amount.""" + for amount in ("2,378.00Cr", "50.90 CR", "INR 50.90 Cr.", "1000cr", "5cr", "(100) Cr"): + self.assertEqual(get_amount_cr_dr_marker(amount), "cr", amount) + + for amount in ("2,378.00Dr", "50.90 DR", "1000dr", "-100 Dr"): + self.assertEqual(get_amount_cr_dr_marker(amount), "dr", amount) + + # Some banks put the marker in front of the digits instead. + for amount in ("Cr 100", "Cr100", "CR INR 100", "cr 0.00"): + self.assertEqual(get_amount_cr_dr_marker(amount), "cr", amount) + + for amount in ("Dr 100", "Dr100", "Dr. 1,234.50"): + self.assertEqual(get_amount_cr_dr_marker(amount), "dr", amount) + + for amount in ("100.00", "-2,000.00", "INR 25,236.00", "", None, 100.0): + self.assertIsNone(get_amount_cr_dr_marker(amount), amount) + + # Text that merely starts or ends with the letters must not be read as a marker, or + # a description that bled into the amount column would reclassify the statement. + for amount in ( + "CREDIT CARD PAYMENT 500", + "DRAFT 100", + "Dr Smith Clinic 500", + "DR AMBEDKAR ROAD BRANCH 500", + "500 CRC", + "Cheque Dr", + "Cr", + ): + self.assertIsNone(get_amount_cr_dr_marker(amount), amount) + + def test_sparsely_marked_cr_dr_amount_column(self): + """One marker is enough to prove a CR/DR amount column - it is not a majority vote. + + A real HDFC credit-card page carries 18 rows and a single "50.90Cr": the unmarked + rows are ordinary purchases, and only the exceptions are marked. A frequency vote + therefore picked "positive/negative" 17-1 and imported that lone credit as a debit. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Transaction Description", "Amount (in Rs.)"], + ["21/07/2026", "ITC MAURYA NEW DELHI", "2,495.00"], + ["22/07/2026", "ZOMATO LIMITED Gurugram", "1,288.68"], + ["23/07/2026", "SWIGGY Bangalore", "532.00"], + ["26/07/2026", "SWIGGY Bangalore", "1,043.00"], + ["27/07/2026", "PETRO SURCHARGE WAIVER", "50.90Cr"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + # Only "Cr" appears, so it is the marked exception and unmarked rows are debits. + self.assertEqual(doc.total_credits, 50.90) + self.assertEqual(doc.total_credit_transactions, 1) + self.assertEqual(doc.total_debits, 5358.68) + self.assertEqual(doc.total_debit_transactions, 4) + + def test_dr_only_statement_treats_unmarked_rows_as_deposits(self): + """The mirror image of a Cr-only statement: only withdrawals are marked. + + The unmarked default cannot be hardcoded to the debit, because which side gets + marked varies by bank. It is derived from the markers the statement actually uses - + here only "Dr" appears, so "Dr" is the exception and everything unmarked is a + deposit. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Amount"], + ["01/04/2026", "ATM WITHDRAWAL", "2,000.00Dr"], + ["03/04/2026", "SALARY", "20,000.00"], + ["05/04/2026", "INTEREST", "150.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + self.assertEqual(doc.total_debits, 2000.0) + self.assertEqual(doc.total_debit_transactions, 1) + self.assertEqual(doc.total_credits, 20150.0) + self.assertEqual(doc.total_credit_transactions, 2) + + def test_leading_cr_dr_markers(self): + """Some banks print the marker in front of the amount.""" + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Amount"], + ["01/04/2026", "ATM WITHDRAWAL", "Dr 2,000.00"], + ["03/04/2026", "SALARY", "Cr 20,000.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + self.assertEqual(doc.total_debits, 2000.0) + self.assertEqual(doc.total_credits, 20000.0) + + def test_partially_marked_cr_dr_amount_column(self): + """A CR/DR amount column stays CR/DR even when some rows carry no marker. + + Every unmarked row used to also vote for "positive/negative", so an ordinary + statement with a few unmarked rows was detected as positive/negative and a + "2000.00Dr" was then imported as a deposit. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Amount", "Balance"], + ["01/04/2026", "OPENING FEE", "100.00", "9,900.00"], + ["03/04/2026", "SALARY", "20000.00Cr", "29,900.00"], + ["05/04/2026", "ATM WDL", "2000.00Dr", "27,900.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Amount column has "CR"/"DR" values') + # Both markers appear, so an unmarked row is undetermined and stays a debit. + self.assertEqual(doc.total_debits, 2100.0) + self.assertEqual(doc.total_debit_transactions, 2) + self.assertEqual(doc.total_credits, 20000.0) + self.assertEqual(doc.total_credit_transactions, 1) + + def test_deposit_withdrawal_type_column(self): + """The word Withdrawal contains "dr", so a loose CR/DR check claims this column first. + + It then reads "Deposit" (which has no "cr" in it) as a withdrawal, flipping the + direction of every credit in the statement. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Transaction Type", "Amount"], + ["01/04/2026", "ATM WDL", "Withdrawal", "2,000.00"], + ["03/04/2026", "SALARY", "Deposit", "20,000.00"], + ["05/04/2026", "ATM WDL", "Withdrawal", "500.00"], + ] + ) + + self.assertEqual( + doc.detected_amount_format, 'Transaction type column has "Deposit"/"Withdrawal" values' + ) + self.assertEqual(doc.total_debits, 2500.0) + self.assertEqual(doc.total_debit_transactions, 2) + self.assertEqual(doc.total_credits, 20000.0) + self.assertEqual(doc.total_credit_transactions, 1) + + def test_unrecognised_type_column_falls_back_to_signed_amount(self): + """An unrecognised transaction type must not stop the amount being read. + + No tally was incremented for these rows, so max() returned the first key - + "Separate columns for withdrawal and deposit" - and, with no such columns in the + file, every amount came through as None. + """ + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Transaction Type", "Amount"], + ["01/04/2026", "ATM WDL", "NEFT", "-2,000.00"], + ["03/04/2026", "SALARY", "IMPS", "20,000.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, "Amount column has positive/negative values") + self.assertEqual(doc.total_debits, 2000.0) + self.assertEqual(doc.total_credits, 20000.0) + + def test_blank_transaction_type_cell(self): + """A blank type cell used to raise - `None.lower()` - instead of parsing the row.""" + doc = self._create_bank_statement_import_log( + [ + ["Date", "Narration", "Transaction Type", "Amount"], + ["01/04/2026", "ATM WDL", "Dr", "2,000.00"], + ["03/04/2026", "SALARY", "Cr", "20,000.00"], + ["05/04/2026", "UNKNOWN", None, "500.00"], + ] + ) + + self.assertEqual(doc.detected_amount_format, 'Transaction type column has "CR"/"DR" values') + # The unmarked row has no direction of its own, so it counts as a withdrawal. + self.assertEqual(doc.total_debits, 2500.0) + self.assertEqual(doc.total_credits, 20000.0) + # ------------------------------------------------------------------ # # PDF statement import # ------------------------------------------------------------------ # @@ -159,7 +339,8 @@ class TestBankStatementImportLog(ERPNextTestSuite, AccountsTestMixin): else: table["header_index"] = None table["column_mapping"] = guess_column_mapping_by_content(table["rows"]) - table["included"] = True + final_transactions, _df, _af = build_table_transactions(table) + table["included"] = should_include_table(table, final_transactions) return table def test_pdf_multi_page_kept_separate_and_unioned(self): @@ -197,6 +378,74 @@ class TestBankStatementImportLog(ERPNextTestSuite, AccountsTestMixin): final, _df, _af = build_table_transactions(ad_table) self.assertEqual(final, []) + def test_pdf_summary_box_not_auto_included(self): + """A summary box that happens to parse as one transaction must not start included. + + The "Payment Due Date / Total Dues / Minimum Amount Due" block on an HDFC + credit-card statement has a date column and a figures column, so it yields a single + transaction - the due date and the minimum amount - and used to import as a phantom + row. What it does not have, and a real transaction table always does, is a narration. + """ + summary_box = { + "header_index": 1, + "rows": [ + ["Statement Date:17/08/2025", "Card No: 4341 55XX XXXX 2754", ""], + ["Payment Due Date", "Total Dues", "Minimum Amount Due"], + ["06/09/2025", "73,200.00", "3,660.00"], + ["Credit Limit", "Available Credit Limit", "Available Cash Limit"], + ["", "32,800", ""], + ], + "column_mapping": [ + {"index": 0, "header_text": "Payment Due Date", "variable": "a", "maps_to": "Date"}, + {"index": 1, "header_text": "Total Dues", "variable": "b", "maps_to": "Do not import"}, + {"index": 2, "header_text": "Minimum Amount Due", "variable": "c", "maps_to": "Amount"}, + ], + } + + final, _df, _af = build_table_transactions(summary_box) + # It really does parse as a transaction - that is why the previous check missed it. + self.assertEqual(len(final), 1) + self.assertFalse(should_include_table(summary_box, final)) + + # The transaction table beside it, which does carry a narration, still starts included. + transactions = self._auto_map( + { + "rows": [ + ["Date", "Transaction Description", "Amount (in Rs.)"], + ["21/07/2025", "ITC MAURYA NEW DELHI", "2,495.00"], + ["27/07/2025", "PETRO SURCHARGE WAIVER", "50.90Cr"], + ] + } + ) + self.assertTrue(transactions["included"]) + + def test_pdf_table_without_description_still_importable(self): + """No narration column means "starts unticked", NOT "cannot be imported". + + `description` is not mandatory on Bank Transaction, so a bank that omits narration + must still import once the user ticks the table. + """ + table = { + "header_index": 0, + "rows": [ + ["Date", "Amount", "Balance"], + ["01/04/2025", "500.00", "9,500.00"], + ["03/04/2025", "20000.00", "29,500.00"], + ], + "column_mapping": [ + {"index": 0, "header_text": "Date", "variable": "a", "maps_to": "Date"}, + {"index": 1, "header_text": "Amount", "variable": "b", "maps_to": "Amount"}, + {"index": 2, "header_text": "Balance", "variable": "c", "maps_to": "Balance"}, + ], + } + + final, _df, _af = build_table_transactions(table) + self.assertFalse(should_include_table(table, final)) + + # The transactions themselves are intact and importable. + self.assertEqual(len(final), 2) + self.assertEqual([t["date"] for t in final], ["2025-04-01", "2025-04-03"]) + def test_headerless_content_mapping(self): """Without a header row, columns are guessed from their contents.""" rows = [ diff --git a/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.js b/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.js index b7200883124..9dac8500166 100644 --- a/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.js +++ b/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.js @@ -16,6 +16,8 @@ frappe.ui.form.on("Chart of Accounts Importer", { () => generate_tree_preview(frm), () => create_import_button(frm), () => frm.set_df_property("chart_preview", "hidden", 0), + // the preview is the point of this page — open it right away + () => frm.fields_dict.chart_preview.collapse(false), ]); } @@ -128,7 +130,6 @@ var create_import_button = function (frm) { freeze_message: __("Creating Accounts..."), callback: function (r) { if (!r.exc) { - clearInterval(frm.page["interval"]); frm.page.set_indicator(__("Import Successful"), "blue"); create_reset_button(frm); } @@ -142,42 +143,95 @@ var create_reset_button = function (frm) { frm.page .set_primary_action(__("Reset"), function () { frm.page.clear_primary_action(); - delete frm.page["show_import_button"]; frm.reload_doc(); }) .addClass("btn btn-primary"); }; -var validate_coa = function (frm) { - if (frm.doc.import_file) { - let parent = __("All Accounts"); - return frappe.call({ - method: "erpnext.accounts.doctype.chart_of_accounts_importer.chart_of_accounts_importer.get_coa", - args: { - file_name: frm.doc.import_file, - parent: parent, - doctype: "Chart of Accounts Importer", - file_type: frm.doc.file_type, - for_validate: 1, - }, - callback: function (r) { - if (r.message["show_import_button"]) { - frm.page["show_import_button"] = Boolean(r.message["show_import_button"]); - } - }, - }); - } -}; - var generate_tree_preview = function (frm) { let parent = __("All Accounts"); - $(frm.fields_dict["chart_tree"].wrapper).empty(); // empty wrapper to load new data + const wrapper = $(frm.fields_dict["chart_tree"].wrapper).empty(); // empty wrapper to load new data + + // search + expand/collapse-all lean on frappe.ui.Tree helpers added with + // row mode; when running against an older frappe that predates them, skip + // this toolbar so the preview still renders (just without the extras) + const has_row_helpers = + typeof frappe.ui.Tree.prototype.get_expansion_state === "function" && + typeof frappe.ui.Tree.prototype.filter_nodes === "function"; + + let tree; + let deep_loaded = false; + let search_text = ""; + let update_buttons = () => {}; + + if (has_row_helpers) { + // same toolbar anatomy as the tree view: search on the left, + // expand/collapse-all on the right (three-state: fully collapsed -> + // Expand All, fully expanded -> Collapse All, partially expanded -> both) + const $toolbar = $('
').appendTo(wrapper); + + const search_control = frappe.ui.form.make_control({ + df: { fieldtype: "Data", fieldname: "preview_search", placeholder: __("Search") }, + parent: $toolbar, + only_input: true, + }); + search_control.refresh(); + $(search_control.wrapper).addClass("m-0").css("width", "220px"); + search_control.$input.addClass("input-xs"); + search_control.$input.on( + "input", + frappe.utils.debounce(() => { + search_text = search_control.$input.val(); + const run = () => { + // a newer keystroke superseded this one while the deep load ran + if (search_text !== search_control.$input.val()) return; + tree.filter_nodes(search_text); + }; + if (!search_text || deep_loaded) { + run(); + return; + } + tree.load_children(tree.root_node, true).then(() => { + deep_loaded = true; + run(); + }); + }, 300) + ); + + const $actions = $('
').appendTo($toolbar); + update_buttons = () => { + const state = tree.get_expansion_state(); + $expand_all.prop("disabled", !(state === "collapsed" || state === "partial")); + $collapse_all.prop("disabled", !(state === "expanded" || state === "partial")); + }; + // tooltip on a wrapper: a disabled es-button has pointer-events:none, + // so hover falls through to the wrapper and the tooltip still shows + const make_action = (icon, label, onclick) => { + const $btn = $( + frappe.ui.button({ icon, disabled: true, onclick, attrs: { "aria-label": label } }) + ); + const $wrapper = $('').append($btn).appendTo($actions); + frappe.ui.tooltip($wrapper, { text: label }); + return $btn; + }; + var $expand_all = make_action("chevrons-up-down", __("Expand All"), () => { + tree.load_children(tree.root_node, true).then(() => { + deep_loaded = true; + }); + }); + var $collapse_all = make_action("chevrons-down-up", __("Collapse All"), () => { + tree.load_children(tree.root_node, false); + }); + } // generate tree structure based on the csv data - return new frappe.ui.Tree({ - parent: $(frm.fields_dict["chart_tree"].wrapper), + tree = new frappe.ui.Tree({ + parent: wrapper, label: parent, expandable: true, + // read-only preview: row-mode visuals without actions or hover cards + // (ignored by an older frappe, which renders the legacy tree) + row_style: true, method: "erpnext.accounts.doctype.chart_of_accounts_importer.chart_of_accounts_importer.get_coa", args: { file_name: frm.doc.import_file, @@ -185,8 +239,9 @@ var generate_tree_preview = function (frm) { doctype: "Chart of Accounts Importer", file_type: frm.doc.file_type, }, - onclick: function (node) { - parent = node.value; - }, + on_node_render: () => update_buttons(), + // expanded flips right after this callback — check on the next tick + on_click: () => setTimeout(update_buttons, 0), }); + return tree; }; diff --git a/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.py b/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.py index 5402c4d65c8..40def49b32e 100644 --- a/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.py +++ b/erpnext/accounts/doctype/chart_of_accounts_importer/chart_of_accounts_importer.py @@ -8,6 +8,7 @@ from functools import reduce import frappe from frappe import _ +from frappe.core.doctype.file.utils import find_file_by_url from frappe.desk.form.linked_with import get_linked_fields from frappe.model.document import Document from frappe.utils import cint, cstr @@ -58,6 +59,8 @@ def validate_columns(data): @frappe.whitelist() def validate_company(company: str): + frappe.has_permission("Chart of Accounts Importer", throw=True) + parent_company, allow_account_creation_against_child_company = frappe.get_cached_value( "Company", company, ["parent_company", "allow_account_creation_against_child_company"] ) @@ -110,7 +113,10 @@ def import_coa(file_name: str, company: str): def get_file(file_name): - file_doc = frappe.get_doc("File", {"file_url": file_name}) + file_doc = find_file_by_url(file_name) + if not file_doc: + raise frappe.PermissionError + parts = file_doc.get_extension() extension = parts[1] extension = extension.lstrip(".") @@ -179,6 +185,8 @@ def get_coa( ): """called by tree view (to fetch node's children)""" + frappe.has_permission("Chart of Accounts Importer", throw=True) + file_doc, extension = get_file(file_name) parent = None if parent == _("All Accounts") else parent @@ -326,6 +334,8 @@ def build_response_as_excel(writer): @frappe.whitelist() def download_template(file_type: str, template_type: str, company: str): + frappe.has_permission("Chart of Accounts Importer", throw=True) + writer = get_template(template_type, company) if file_type == "CSV": @@ -378,7 +388,6 @@ def get_sample_template(writer, company): return writer -@frappe.whitelist() def validate_accounts(file_doc: Document, extension: str): if extension == "csv": accounts = generate_data_from_csv(file_doc, as_dict=True) diff --git a/erpnext/accounts/doctype/cost_center/cost_center_tree.js b/erpnext/accounts/doctype/cost_center/cost_center_tree.js index 3edeb8efb0b..c36a2922846 100644 --- a/erpnext/accounts/doctype/cost_center/cost_center_tree.js +++ b/erpnext/accounts/doctype/cost_center/cost_center_tree.js @@ -12,6 +12,19 @@ frappe.treeview_settings["Cost Center"] = { ], root_label: "Cost Centers", get_tree_nodes: "erpnext.accounts.utils.get_children", + get_label: function (node) { + // clean display name — the number renders as a badge (see onrender) + return frappe.utils.escape_html(node.data.cost_center_name || node.title || node.label); + }, + onrender: function (node) { + if (node.is_root || !node.data) return; + + const flags = []; + if (node.data.cost_center_number) { + flags.push(frappe.ui.badge({ label: node.data.cost_center_number })); + } + erpnext.utils.render_tree_node_flags(node, flags); + }, add_tree_node: "erpnext.accounts.utils.add_cc", menu_items: [ { @@ -42,6 +55,37 @@ frappe.treeview_settings["Cost Center"] = { }, ], ignore_fields: ["parent_cost_center"], + toolbar: [ + { + label: __("Convert to Group"), + icon: "folder-tree", + condition: function (node) { + return !node.is_root && !node.expandable && frappe.model.can_write("Cost Center"); + }, + click: function (node) { + erpnext.accounts.convert_tree_node("Cost Center", node, "convert_ledger_to_group"); + }, + }, + { + label: __("Convert to Non-Group"), + icon: "file-text", + condition: function (node) { + // only on groups the user has opened and found empty — a + // group with children can't convert, so don't offer it + return ( + !node.is_root && + node.expandable && + node.loaded && + !node.$ul.children().length && + frappe.model.can_write("Cost Center") + ); + }, + click: function (node) { + erpnext.accounts.convert_tree_node("Cost Center", node, "convert_group_to_ledger"); + }, + }, + ], + extend_toolbar: true, onload: function (treeview) { function get_company() { return treeview.page.fields_dict.company.get_value(); @@ -82,3 +126,22 @@ frappe.treeview_settings["Cost Center"] = { ); }, }; + +frappe.provide("erpnext.accounts"); +// shared by the Account and Cost Center tree views (defined in both files, +// whichever loads first wins): run the doctype's whitelisted convert method, +// then re-render the branch so the node's group/leaf state updates +erpnext.accounts.convert_tree_node = + erpnext.accounts.convert_tree_node || + function (doctype, node, method) { + frappe.call({ + method: "run_doc_method", + args: { dt: doctype, dn: node.label, method: method }, + callback: function (r) { + if (r.exc) return; + const treeview = frappe.views.trees[doctype]; + node.parent_node && treeview.tree.load_children(node.parent_node); + frappe.show_alert({ message: __("{0} converted", [node.label]), indicator: "green" }); + }, + }); + }; diff --git a/erpnext/accounts/doctype/dunning/dunning.py b/erpnext/accounts/doctype/dunning/dunning.py index c1186a01354..3d271775293 100644 --- a/erpnext/accounts/doctype/dunning/dunning.py +++ b/erpnext/accounts/doctype/dunning/dunning.py @@ -17,7 +17,8 @@ import json import frappe from frappe import _ from frappe.contacts.doctype.address.address import get_address_display -from frappe.utils import getdate +from frappe.query_builder.functions import Sum +from frappe.utils import flt, getdate from erpnext.controllers.accounts_controller import AccountsController @@ -147,6 +148,31 @@ class Dunning(AccountsController): ) row.dunning_level = len(past_dunnings) + 1 + def get_unpaid_base_dunning_amount(self): + """Interest and dunning fee that is still to be collected, in company currency.""" + if not self.base_dunning_amount: + return 0.0 + + return flt( + flt(self.base_dunning_amount) - get_paid_dunning_amount(self.name), + self.precision("base_dunning_amount"), + ) + + def get_unpaid_dunning_amount(self): + """Interest and dunning fee that is still to be collected, in the dunning currency.""" + return flt( + self.get_unpaid_base_dunning_amount() / (flt(self.conversion_rate) or 1), + self.precision("dunning_amount"), + ) + + def get_unpaid_overdue_payments(self): + """Overdue payments with their outstanding as of now, not as of dunning creation.""" + return [ + (row, outstanding) + for row in self.overdue_payments + if (outstanding := get_current_outstanding(row)) > 0 + ] + def on_cancel(self): super().on_cancel() self.ignore_linked_doctypes = [ @@ -161,6 +187,7 @@ class Dunning(AccountsController): "Unreconcile Payment Entries", "Payment Ledger Entry", "Serial and Batch Bundle", + "Payment Entry", ] @frappe.whitelist() @@ -259,11 +286,73 @@ def update_linked_dunnings(doc, previous_outstanding_amount): if has_outstanding: break - new_status = "Resolved" if not has_outstanding else "Unresolved" + set_dunning_status(dunning, has_outstanding, respect_manual_resolution=True) - if dunning.status != new_status: - dunning.status = new_status - dunning.save() + +def update_dunnings_linked_to_payment(payment_entry): + """Refresh dunnings whose interest and fee are settled by this payment.""" + dunnings = {row.dunning for row in payment_entry.get("deductions") if row.dunning} + + for name in dunnings: + dunning = frappe.get_doc("Dunning", name) + if dunning.docstatus != 1: + continue + + set_dunning_status(dunning, bool(dunning.get_unpaid_overdue_payments())) + + +def set_dunning_status(dunning, has_outstanding_payments: bool, respect_manual_resolution: bool = False): + """A dunning is only resolved once the invoiced sum *and* its interest and fee are paid.""" + has_unpaid_dunning_amount = dunning.get_unpaid_dunning_amount() > 0 + new_status = "Unresolved" if has_outstanding_payments or has_unpaid_dunning_amount else "Resolved" + + # resolving by hand waives the interest, only an invoice that is owed again reopens it + if respect_manual_resolution and dunning.status == "Resolved" and not has_outstanding_payments: + return + + if dunning.status != new_status: + dunning.db_set("status", new_status, notify=True) + + +def get_paid_dunning_amount(dunning: str) -> float: + """Interest and fee collected for this dunning, in company currency.""" + deduction = frappe.qb.DocType("Payment Entry Deduction") + payment_entry = frappe.qb.DocType("Payment Entry") + + paid = ( + frappe.qb.from_(deduction) + .join(payment_entry) + .on(payment_entry.name == deduction.parent) + .select(Sum(deduction.amount)) + .where((deduction.dunning == dunning) & (payment_entry.docstatus == 1)) + ).run() + + # the dunning amount is booked as a negative deduction, against the income account + return -flt(paid[0][0]) if paid else 0.0 + + +def get_current_outstanding(overdue_payment) -> float: + """Outstanding of an overdue payment as of now, in the invoice's transaction currency.""" + invoice = frappe.db.get_value( + "Sales Invoice", + overdue_payment.sales_invoice, + ["outstanding_amount", "currency", "party_account_currency"], + as_dict=True, + ) + schedule_outstanding = ( + flt(frappe.db.get_value("Payment Schedule", overdue_payment.payment_schedule, "outstanding")) + if overdue_payment.payment_schedule + else flt(overdue_payment.outstanding) + ) + + if flt(invoice.outstanding_amount) <= 0 or schedule_outstanding <= 0: + return 0.0 + + outstanding = min(schedule_outstanding, flt(overdue_payment.outstanding)) + if invoice.currency == invoice.party_account_currency: + outstanding = min(outstanding, flt(invoice.outstanding_amount)) + + return outstanding def get_linked_dunnings_as_per_state(sales_invoice, state): diff --git a/erpnext/accounts/doctype/dunning/test_dunning.py b/erpnext/accounts/doctype/dunning/test_dunning.py index cb2559e75f4..5988e055f3a 100644 --- a/erpnext/accounts/doctype/dunning/test_dunning.py +++ b/erpnext/accounts/doctype/dunning/test_dunning.py @@ -55,6 +55,125 @@ class TestDunning(ERPNextTestSuite): dunning.reload() self.assertEqual(dunning.status, "Resolved") + def test_dunning_not_resolved_by_payment_of_invoiced_sum_only(self): + """ + Regression for #58220: paying the invoice without the interest and fee must not + resolve the dunning, the interest is still owed and has to stay claimable. + """ + dunning = create_dunning(overdue_days=15, dunning_type_name="Second Notice - _TC") + dunning.submit() + sales_invoice = dunning.overdue_payments[0].sales_invoice + + pe = get_payment_entry("Sales Invoice", sales_invoice) + pe.reference_no, pe.reference_date = "4", nowdate() + pe.insert() + pe.submit() + + self.assertEqual(frappe.get_value("Sales Invoice", sales_invoice, "outstanding_amount"), 0) + + dunning.reload() + self.assertEqual(dunning.status, "Unresolved") + self.assertEqual(round(dunning.get_unpaid_dunning_amount(), 2), 10.41) + + # the interest and fee can still be collected on their own + pe = get_payment_entry("Dunning", dunning.name) + pe.reference_no, pe.reference_date = "5", nowdate() + self.assertEqual(pe.references, []) + self.assertEqual(round(pe.paid_amount, 2), 10.41) + pe.insert() + pe.submit() + + dunning.reload() + self.assertEqual(dunning.status, "Resolved") + self.assertEqual(dunning.get_unpaid_dunning_amount(), 0) + + # cancelling the interest payment makes the dunning claimable again + pe.cancel() + dunning.reload() + self.assertEqual(dunning.status, "Unresolved") + self.assertEqual(round(dunning.get_unpaid_dunning_amount(), 2), 10.41) + + def test_dunning_can_be_cancelled_after_its_interest_was_paid(self): + """ + The payment collecting the interest links back to the dunning, which must not stand in + the way of cancelling it. + """ + dunning = create_dunning(overdue_days=15, dunning_type_name="Second Notice - _TC") + dunning.submit() + + pe = get_payment_entry("Dunning", dunning.name) + pe.reference_no, pe.reference_date = "6", nowdate() + pe.insert() + pe.submit() + + dunning.reload() + self.assertEqual(dunning.status, "Resolved") + + dunning.cancel() + self.assertEqual(dunning.docstatus, 2) + + def test_waived_interest_keeps_a_manually_resolved_dunning_resolved(self): + """ + Resolving a dunning by hand waives its interest, so a later payment of the invoice + must not reopen it. + """ + dunning = create_dunning(overdue_days=15, dunning_type_name="Second Notice - _TC") + dunning.submit() + sales_invoice = dunning.overdue_payments[0].sales_invoice + + # what the "Resolve" button does + dunning.reload() + dunning.status = "Resolved" + dunning.save() + + pe = get_payment_entry("Sales Invoice", sales_invoice) + pe.reference_no, pe.reference_date = "7", nowdate() + pe.insert() + pe.submit() + + dunning.reload() + self.assertEqual(dunning.status, "Resolved") + self.assertEqual(round(dunning.get_unpaid_dunning_amount(), 2), 10.41) + + @ERPNextTestSuite.change_settings( + "Accounts Settings", {"allow_multi_currency_invoices_against_single_party_account": 1} + ) + def test_unpaid_dunning_amount_is_tracked_in_company_currency(self): + """ + The interest and fee are collected as a Payment Entry deduction, a company currency + field, so what is left to collect has to be measured in the same currency. + """ + si = create_sales_invoice( + posting_date=add_days(today(), -15), + currency="USD", + conversion_rate=50, + rate=100, + debit_to="Debtors - _TC", + ) + + dunning = create_dunning_from_sales_invoice(si.name) + dunning_type = frappe.get_doc("Dunning Type", "Second Notice - _TC") + dunning.dunning_type = dunning_type.name + dunning.rate_of_interest = dunning_type.rate_of_interest + dunning.dunning_fee = dunning_type.dunning_fee + dunning.income_account = dunning_type.income_account + dunning.cost_center = dunning_type.cost_center + dunning.save() + + self.assertEqual(dunning.currency, "USD") + self.assertEqual(dunning.conversion_rate, 50) + self.assertEqual(round(dunning.dunning_amount, 2), 10.41) + self.assertEqual(round(dunning.base_dunning_amount, 2), 520.55) + + # nothing collected yet, in either currency + self.assertEqual(round(dunning.get_unpaid_base_dunning_amount(), 2), 520.55) + self.assertEqual(round(dunning.get_unpaid_dunning_amount(), 2), 10.41) + + # the deduction booking the interest is in company currency + dunning.submit() + pe = get_payment_entry("Dunning", dunning.name) + self.assertEqual(round(pe.deductions[0].amount, 2), -520.55) + def test_fetch_overdue_payments(self): """ Create SI with overdue payment. Check if overdue payment is fetched in Dunning. diff --git a/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py b/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py index 0a4da97d400..a9759a73630 100644 --- a/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py +++ b/erpnext/accounts/doctype/financial_report_template/financial_report_engine.py @@ -32,6 +32,7 @@ from erpnext.accounts.doctype.financial_report_template.financial_report_validat AccountFilterValidator, CalculationFormulaValidator, DependencyValidator, + get_valid_api_method, ) from erpnext.accounts.report.financial_statements import ( get_columns, @@ -490,7 +491,10 @@ class DataCollector: if company: query = query.where(account.company == company) - if conditions := filter_parser.build_conditions(account_rows, account): + # filters are optional: no filter means all (enabled, non-group) accounts of the company. + # invalid filters can't reach here — build_conditions raises on them (raise_on_invalid). + conditions = filter_parser.build_conditions(account_rows, account, raise_on_invalid=True) + if conditions is not None: query = query.where(conditions) return query.run(pluck=True) @@ -802,17 +806,20 @@ class FilterExpressionParser: def __init__(self): self.validator = AccountFilterValidator() - def build_conditions(self, report_rows, table): + def build_conditions(self, report_rows, table, raise_on_invalid=False): conditions = [] for row in report_rows or []: - condition = self.build_condition(row, table) + condition = self.build_condition(row, table, raise_on_invalid=raise_on_invalid) if condition is not None: conditions.append(condition) + if not conditions: + return None + # ensure brackets in or condition return reduce(lambda a, b: (a) | (b), conditions) - def build_condition(self, report_row, table): + def build_condition(self, report_row, table, raise_on_invalid=False): """ Build SQL condition directly from filter formula. @@ -842,9 +849,11 @@ class FilterExpressionParser: if not filter_formula: return None - errors = self.validator.validate(report_row) + errors = self.validator.validate_filter(report_row) if not errors.is_valid: error_messages = [str(issue) for issue in errors.issues] + if raise_on_invalid: + frappe.throw("

".join(error_messages), title=_("Invalid Filter")) frappe.log_error(f"Filter validation errors found:\n{'

'.join(error_messages)}") return None @@ -1041,7 +1050,11 @@ class FormulaFieldUpdater: @frappe.whitelist() def get_filtered_accounts(company: str, account_rows: str | list): + if not company: + frappe.throw(_("Company is required"), title=_("Missing Company")) + frappe.has_permission("Financial Report Template", ptype="read", throw=True) + frappe.has_permission("Company", doc=company, throw=True) account_rows = [frappe._dict(row) for row in frappe.parse_json(account_rows)] @@ -1182,10 +1195,12 @@ class RowProcessor: def _process_api_row(self, row) -> RowData: api_path = row.calculation_formula - # TODO + + method = get_valid_api_method(api_path) try: - values = frappe.call(api_path, filters=self.context.filters, periods=self.period_list, row=row) + # nosemgrep: frappe-semgrep-rules.rules.security.frappe-codeinjection-eval + values = frappe.call(method, filters=self.context.filters, periods=self.period_list, row=row) if row.reverse_sign: values = [-1 * v for v in values] diff --git a/erpnext/accounts/doctype/financial_report_template/financial_report_template.js b/erpnext/accounts/doctype/financial_report_template/financial_report_template.js index fe04d11b2c4..12099700cf3 100644 --- a/erpnext/accounts/doctype/financial_report_template/financial_report_template.js +++ b/erpnext/accounts/doctype/financial_report_template/financial_report_template.js @@ -236,6 +236,8 @@ async function refresh_tree_view(dialog, account_rows) { parent: wrapper, label: company, root_value: company, + // read-only preview: row-mode visuals without actions + row_style: true, method: "erpnext.accounts.doctype.financial_report_template.financial_report_engine.get_children_accounts", args: { doctype: "Account", company: company, filtered_accounts: filtered_accounts, missed: missed }, toolbar: [], @@ -370,7 +372,7 @@ function update_formula_description(frm, data_source) { description_html = `
Custom API Setup
-

Path to your custom method that returns financial data.

+

Path to your custom whitelisted method that returns financial data. It must permit GET requests.

Format:
    @@ -380,7 +382,8 @@ function update_formula_description(frm, data_source) {
    Method Signature:
    - 
    def get_custom_data(filters, periods, row): 
      # filters: dict — report filters (company, period, etc.) 
      # periods: list[dict] — period definitions 
      # row: dict — the current report row 

      return [1000.0, 1200.0, 1150.0]  # one value per period
    + + 
    @frappe.whitelist(methods=["GET"])
def get_custom_data(filters, periods, row):
    # filters: dict — report filters (company, period, etc.)
    # periods: list[dict] — period definitions
    # row: dict — the current report row

    return [1000.0, 1200.0, 1150.0]  # one value per period
    Return Format:
    diff --git a/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py b/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py index 7abc6fb98b9..3f3000f33cf 100644 --- a/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py +++ b/erpnext/accounts/doctype/financial_report_template/financial_report_validation.py @@ -8,10 +8,25 @@ from dataclasses import dataclass, field from typing import Any import frappe -from frappe import _ +from frappe import _, is_whitelisted from frappe.database.operator_map import OPERATOR_MAP +def get_valid_api_method(api_path: str): + """Resolve `api_path`, ensuring it is whitelisted and permits GET (i.e. read-only).""" + method = frappe.get_attr(api_path) + is_whitelisted(method) + + if "GET" not in frappe.allowed_http_methods_for_whitelisted_func.get(method, ()): + frappe.throw( + _("Method {0} must permit GET requests").format(frappe.bold(api_path)), + frappe.PermissionError, + title=_("Method Not Allowed"), + ) + + return method + + def get_formula_field_label(data_source: str) -> str: # Must mirror the `labels` map in financial_report_template.js (update_formula_label), labels = { @@ -175,8 +190,10 @@ class TemplateStructureValidator(Validator): if not row.calculation_formula: result.add_error( ValidationIssue( - message=_("{0} is required for {1}").format( - get_formula_field_label(row.data_source), row.data_source + message=_("{0} is required when {1} is {2}").format( + get_formula_field_label(row.data_source), + row.meta.get_translated_label("data_source"), + _(row.data_source), ), row_idx=row.idx, ) @@ -204,7 +221,14 @@ class DependencyValidator(Validator): for row in self.template.rows: if row.reference_code and row.data_source == "Calculated Amount" and row.calculation_formula: - deps = extract_reference_codes_from_formula(row.calculation_formula, list(available_codes)) + # skip self-reference, `CalculationFormulaValidator` already reports it + deps = [ + code + for code in extract_reference_codes_from_formula( + row.calculation_formula, list(available_codes) + ) + if code != row.reference_code + ] if deps: graph[row.reference_code] = deps @@ -266,7 +290,9 @@ class DependencyValidator(Validator): row_idx = self._get_row_idx(ref_code) result.add_error( ValidationIssue( - message=_("Line References undefined in Formula: {0}").format(", ".join(undefined)), + message=_("Line references undefined in {0}: {1}").format( + get_formula_field_label("Calculated Amount"), ", ".join(undefined) + ), row_idx=row_idx, ) ) @@ -293,17 +319,6 @@ class CalculationFormulaValidator(Validator): if row.data_source != "Calculated Amount": return result - if not row.calculation_formula: - result.add_error( - ValidationIssue( - message=_("{0} is required for Calculated Amount").format( - get_formula_field_label(row.data_source) - ), - row_idx=row.idx, - ) - ) - return result - formula = self._preprocess_formula(row.calculation_formula) row.calculation_formula = formula @@ -328,16 +343,6 @@ class CalculationFormulaValidator(Validator): ) ) - # Check undefined references - undefined = set(refs) - set(available_codes) - if undefined: - result.add_error( - ValidationIssue( - message=_("Formula references undefined codes: {0}").format(", ".join(undefined)), - row_idx=row.idx, - ) - ) - # Try to evaluate with dummy values eval_error = self._test_formula_evaluation(formula, available_codes) if eval_error: @@ -395,21 +400,19 @@ class AccountFilterValidator(Validator): self.account_fields = account_fields or set(self.account_meta._valid_columns) def validate(self, row) -> ValidationResult: - result = ValidationResult() - + # dispatch-path guard: only account-data rows are validated here if row.data_source != "Account Data": - return result + return ValidationResult() - if not row.calculation_formula: - result.add_error( - ValidationIssue( - message=_("{0} is required for Account Data").format( - get_formula_field_label(row.data_source) - ), - row_idx=row.idx, - ) - ) - return result + return self.validate_filter(row) + + def validate_filter(self, row) -> ValidationResult: + """Validate calculation_formula as an Account filter, regardless of data_source. + + The caller has already decided this row is an account filter, so unlike + `validate()` this does not opt out based on `data_source`. + """ + result = ValidationResult() try: filter_config = json.loads(row.calculation_formula) @@ -422,7 +425,9 @@ class AccountFilterValidator(Validator): if error: result.add_error( ValidationIssue( - message=_("{0}: {1}").format(get_formula_field_label(row.data_source), error), + message=_("[{0}] {1}", context="Financial Report Template").format( + get_formula_field_label("Account Data"), error + ), row_idx=row.idx, ) ) @@ -430,8 +435,9 @@ class AccountFilterValidator(Validator): except json.JSONDecodeError as e: result.add_error( ValidationIssue( - message=_("{0}: Invalid JSON format: {1}").format( - get_formula_field_label(row.data_source), str(e) + message=_("[{0}] {1}", context="Financial Report Template").format( + get_formula_field_label("Account Data"), + _("Invalid JSON format: {0}").format(str(e)), ), row_idx=row.idx, ) @@ -455,12 +461,9 @@ class AccountFilterValidator(Validator): if not isinstance(field, str) or not isinstance(operator, str): return _("Field and operator must be strings") - display = ( - field if advanced_filtering else self.account_meta.get_translated_label(field) - ) or field - if field not in account_fields: - return _("Field '{0}' is not a valid Account field").format(display) + # escape: `field` is caller-supplied and this message renders as HTML + return _("Field '{0}' is not a valid Account field").format(frappe.utils.escape_html(field)) if operator.casefold() not in OPERATOR_MAP: return _("Invalid operator '{0}'").format(operator) @@ -531,29 +534,24 @@ class FormulaValidator(Validator): ) return result - # Method exists? try: - module_path, method_name = api_path.rsplit(".", 1) - module = frappe.get_module(module_path) - - if not hasattr(module, method_name): - result.add_error( - ValidationIssue( - message=_( - "{0}: Method '{1}' not found in module '{2}' (might be environment-specific)" - ).format(get_formula_field_label(row.data_source), method_name, module_path), - row_idx=row.idx, - ) - ) + get_valid_api_method(api_path) except Exception as e: - result.add_error( - ValidationIssue( - message=_("Could not validate {0}: {1}").format( - get_formula_field_label(row.data_source), str(e) - ), - row_idx=row.idx, + if isinstance(e, frappe.PermissionError | frappe.ValidationError): + # frappe.throw inside get_valid_api_method logs a message that would pop up in UI + frappe.clear_last_message() + + if isinstance(e, frappe.PermissionError): + message = _("[{0}] {1}", context="Financial Report Template").format( + get_formula_field_label(row.data_source), + _("Method '{0}' must be whitelisted and permit GET requests").format(api_path), ) - ) + else: + message = _("Could not validate {0}: {1}").format( + get_formula_field_label(row.data_source), str(e) + ) + + result.add_error(ValidationIssue(message=message, row_idx=row.idx)) return result diff --git a/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py b/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py index e3ca33a747e..7b23398a472 100644 --- a/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py +++ b/erpnext/accounts/doctype/financial_report_template/test_financial_report_template.py @@ -2,7 +2,13 @@ # For license information, please see license.txt import frappe +from frappe.tests.utils import whitelist_for_tests +from erpnext.accounts.doctype.financial_report_template.financial_report_validation import ( + AccountFilterValidator, + FormulaValidator, + get_valid_api_method, +) from erpnext.tests.utils import ERPNextTestSuite @@ -72,3 +78,173 @@ class FinancialReportTemplateTestCase(ERPNextTestSuite): {"doctype": "Financial Report Template", "template_name": template_name, "rows": rows_data} ) return template + + +def not_whitelisted_method(**kwargs): + return [42.0] + + +@whitelist_for_tests(methods=["POST"]) +def whitelisted_post_only_method(**kwargs): + return [42.0] + + +@whitelist_for_tests(methods=["GET"]) +def whitelisted_get_method(**kwargs): + return [42.0] + + +class TestCustomAPIValidation(FinancialReportTemplateTestCase): + """Custom API rows must point to whitelisted methods that permit GET""" + + TEST_MODULE = "erpnext.accounts.doctype.financial_report_template.test_financial_report_template" + NOT_WHITELISTED = f"{TEST_MODULE}.not_whitelisted_method" + WHITELISTED_POST_ONLY = f"{TEST_MODULE}.whitelisted_post_only_method" + WHITELISTED_GET = f"{TEST_MODULE}.whitelisted_get_method" + + def create_api_template(self, api_path): + template = self.create_test_template_with_rows( + [ + { + "reference_code": "API001", + "display_name": "API Row", + "data_source": "Custom API", + "calculation_formula": api_path, + } + ] + ) + template.report_type = "Profit and Loss Statement" + return template + + def test_get_valid_api_method(self): + self.assertRaises(frappe.PermissionError, get_valid_api_method, self.NOT_WHITELISTED) + self.assertRaises(frappe.PermissionError, get_valid_api_method, self.WHITELISTED_POST_ONLY) + self.assertEqual(get_valid_api_method(self.WHITELISTED_GET), frappe.get_attr(self.WHITELISTED_GET)) + + def test_save_rejects_invalid_api_methods(self): + for api_path in (self.NOT_WHITELISTED, self.WHITELISTED_POST_ONLY): + template = self.create_api_template(api_path) + self.assertRaises(frappe.ValidationError, template.insert) + + def test_save_allows_get_whitelisted_method(self): + template = self.create_api_template(self.WHITELISTED_GET) + template.insert() + template.delete() + + def test_engine_rejects_invalid_api_methods(self): + from erpnext.accounts.doctype.financial_report_template.financial_report_engine import ( + ReportContext, + RowProcessor, + ) + + for api_path in (self.NOT_WHITELISTED, self.WHITELISTED_POST_ONLY): + template = self.create_api_template(api_path) + context = ReportContext(template=template, filters={}, period_list=[{"key": "p1"}]) + processor = RowProcessor(context) + self.assertRaises(frappe.PermissionError, processor._process_api_row, template.rows[0]) + + def test_engine_calls_valid_api_method(self): + from erpnext.accounts.doctype.financial_report_template.financial_report_engine import ( + ReportContext, + RowProcessor, + ) + + template = self.create_api_template(self.WHITELISTED_GET) + context = ReportContext(template=template, filters={}, period_list=[{"key": "p1"}]) + processor = RowProcessor(context) + row_data = processor._process_api_row(template.rows[0]) + self.assertEqual(row_data.values, [42.0]) + + def test_validation_keeps_message_log_clean(self): + validator = FormulaValidator(frappe._dict(rows=[])) + message_count = len(frappe.local.message_log) + + # last path raises AppNotInstalledError, which also logs a message via frappe.throw + for api_path in (self.NOT_WHITELISTED, self.WHITELISTED_POST_ONLY, "missing_app.api.method"): + row = frappe._dict(data_source="Custom API", calculation_formula=api_path, idx=1) + result = validator.validate(row) + self.assertFalse(result.is_valid) + self.assertEqual(len(frappe.local.message_log), message_count) + + +class TestAccountFilter(FinancialReportTemplateTestCase): + """Filter fields must be validated on the account-filter parser path.""" + + @staticmethod + def _row(formula, **extra): + return frappe._dict(calculation_formula=formula, idx=1, **extra) + + def test_validate_filter_enforces_allow_list_without_data_source(self): + # the parser path has no `data_source`; the field allow-list must still apply + validator = AccountFilterValidator() + self.assertFalse(validator.validate_filter(self._row('["bad_field", "=", "x"]')).is_valid) + self.assertTrue(validator.validate_filter(self._row('["root_type", "=", "Income"]')).is_valid) + + def test_validate_gate_still_opts_out_for_non_account_data(self): + # validate() is the dispatch gate: it must not validate non "Account Data" rows + validator = AccountFilterValidator() + row = self._row('["bad_field", "=", "x"]', data_source="Custom API") + self.assertTrue(validator.validate(row).is_valid) + + def test_error_message_labels_and_escapes_field(self): + validator = AccountFilterValidator() + result = validator.validate_filter(self._row('["