commission_rate was a free-text Data field on the Sales Person master and the
Sales Team child, storing percentages as strings. Convert both to Percent.
A pre_model_sync patch sanitizes the existing values first (empty / NULL /
non-numeric -> 0, others normalised via flt) so the Data -> Percent column
change casts cleanly under strict SQL mode, where Percent is a NOT NULL
decimal column. The patch is idempotent and avoids db-specific SQL so it works
on both MariaDB and Postgres.
The parent Commission section (Sales Partner commission) and the Sales Team
table (Sales Person contribution) drive separate logic in
SellingController.calculate_commission / calculate_contribution. Add
integration tests on Sales Order:
- sales partner commission: total_commission = eligible amount * rate / 100,
and the commission-rate 0..100 bound;
- sales-person allocated_amount tracks amount_eligible_for_commission
(grant_commission gated), not gross net_total, plus the incentive math;
- the allocated-percentage must-total-100 throw;
- rejection of a disabled sales person.
The method (cyclomatic complexity C/14) mixed packed-item separation, SRE
creation and packed-item reservation. Extract _extract_packed_item_details,
_packed_items_to_reserve and _reserve_packed_items (verbatim moves). Drops
C/14 -> A/3; no C-rank function remains in the module. No behaviour change
(stock-reservation, product-bundle and pick-list reservation suites green).
auto_close_opportunity fell back to 15 days in code when the CRM Setting was
blank (and its docstring still said 7). The field already defaults to 15, so
read the value straight from CRM Settings and add a patch to backfill 15 for
existing sites that left it blank, keeping the same auto-close schedule.
- assert cost-of-shipments against the PO base_amount instead of a
hardcoded total, so it holds when conversion_rate != 1
- guard the idempotency test's fixed scorecard name against leftovers
- clarify that the eval-statement zero/None substitution is a truthiness check
Add tests for get_lead_details and the Lead <-> Prospect lifecycle: editing a
lead syncs into its Prospect Lead row, and deleting the only lead of a
prospect removes the prospect. Lead controller coverage 65% -> 74%.
Add tests for get_item_details, auto_close_opportunity (a stale Replied
opportunity is closed, a recent one is not) and the Opportunity -> Prospect
opportunity sync. Opportunity controller coverage 62% -> 80%.
set_expired_status passed filters= and fieldname= kwargs that
frappe.db.set_value does not accept, so the daily scheduled task threw
TypeError on every run and quotations were never marked Expired. Pass
the filter dict as the positional docname argument, and scope it to
submitted documents so draft quotations aren't wrongly expired (matching
the selling Quotation behaviour).
Adds coverage for valid-till validation, the expiry task, and the
RFQ quote-status round-trip on submit/cancel.
declare_enquiry_lost had almost no coverage. Add tests that marking an
Opportunity as lost records the lost reasons, competitors and detailed
reason and sets status to Lost, and that it is blocked when an active
(submitted) Quotation exists.
Address Greptile review:
- customContext.files scope was **/*.py only, so Query Report SQL in .js/.sql/report .json
files didn't get the guide attached as context (the global instructions still applied).
Widen to .py/.js/.sql/report **/*.json.
- The guide's HAVING-alias rule said "with no GROUP BY"; PostgreSQL rejects a SELECT-alias in
HAVING regardless of GROUP BY. Reworded to match (repeat the expression, or move a
non-aggregate predicate to WHERE).
The PostgreSQL server-test job is label-gated, so until it is required the Greptile
PR-review bot is the always-on guard against cross-engine breaks. Extend
.greptile/config.json with `instructions` (and a `customContext` reference to a new
guide) so every review flags new/changed queries that would error on PostgreSQL or
silently diverge from MariaDB, under the prime rule that MariaDB output must not change.
- .github/POSTGRES_COMPATIBILITY.md — the catalogue the bot (and contributors) follow:
hard breaks (loose GROUP BY, MySQL-only funcs, UPDATE..JOIN, HAVING-on-alias,
DISTINCT+ORDER BY, single-quoted alias, varchar bitwise OR, capital identifiers,
set_value(Check,bool)), silent divergences (text case-sensitivity, name-lookup case,
empty-string↔NULL, NULL ordering, ORDER BY..LIMIT 1 tiebreakers, integer division,
distinct-drops-ORDER-BY-on-PG + casefold sorting, function-rewrite parity, UnixTimestamp
TZ), the GROUP BY row-count trap (Max()-wrap vs add-to-GROUP-BY; FD-by-source-table),
the InFailedSqlTransaction/savepoint rule, and the false positives NOT to flag
(.like→ILIKE, ifnull/backtick/LOCATE/REGEXP auto-translation, MariaDB-changing tiebreakers).
- Existing disabledLabels and frappe/frappe context are preserved.
The `skip_transfer` and transfer branches of `validate_manufacture` ran the
same per-item validation loop — look the row up or throw "not a part of",
check overconsumption, guard against duplicates, record — differing only in
the data source (SCIO Received Item vs Work Order Item), the available-qty
basis, the source-warehouse check (skip_transfer only) and the message text.
Split each branch into a small method that builds a normalised
`{item_code: {consumed_qty, available_qty}}` lookup, and share the loop via
`_validate_customer_provided_consumption`. Branch-specific throw messages are
passed as callbacks so the user-facing strings (and their translations) are
unchanged, and the order in which checks fire is preserved. Also drops the
unused `name` column from the skip_transfer query.
Adds a test for the non-skip-transfer manufacture flow (Material Transfer for
Manufacture -> Manufacture), which exercises the Work Order branch that the
existing suite — all of whose manufacture tests set skip_transfer=1 — never
covered. Full subcontracting-inward suite passes on MariaDB and PostgreSQL.
`get_fg_reference_names` built its LIKE filter with old-style
`"%%%s%%" % txt`. Use an f-string (`f"%{txt}%"`) for readability; the value
is still passed as a parameterised filter, so behaviour is unchanged.
Each new child row was given `idx=frappe.db.count(...) + 1`, issuing a count
query per inserted row across three insert loops (received items on receipt,
self-procured RM on manufacture, secondary items on manufacture). Compute the
starting index once before each loop and increment a local counter, producing
the same idx sequence with a single count query.
`update_inward_order_received_items_for_manufacture` unpacks
`zip(*item_code_wh.keys(), strict=True)`. When the manufacture entry has no
raw-material rows (all rows are finished/secondary/scrap), `item_code_wh` is
empty and the unpack raises `ValueError: not enough values to unpack`.
Return early when there are no such rows, mirroring the `if secondary_items:`
guard already present in `update_inward_order_secondary_items`.
In `validate_manufacture`, `customer_warehouse` is read only inside the
`skip_transfer` branch but was fetched unconditionally, wasting a lookup on
the non-skip-transfer path. Move it inside the branch that uses it.
`validate_material_transfer` ran the `Work Order Item` query and rebuilt
`wo_item_dict` inside the per-item loop, even though both depend only on
`self.work_order`. For an entry with N customer-provided rows that meant N
identical queries. Build the lookup once before the loop.
`validate_manufacture` already builds the analogous dict once up front, so
this also aligns the two methods.
In `update_inward_order_item`, the walrus assignment `scio_item_name :=` is
already part of the truthy `if` condition, so the nested `if scio_item_name:`
is always true. Remove it and dedent the body.
`validate_delivery_on_save` imported `pypika.terms.ValueWrapper` inside its
per-item loop, re-running the import on every iteration. Move it to the
module-level imports.
The "exceeds quantity available" throw in `validate_manufacture` passes a
third positional arg (`item.transfer_qty`), but the message only has `{0}`
and `{1}` placeholders, so `str.format` silently discards it. Remove the
dead argument; no behaviour change.
`validate_manufacture` builds its "Target Warehouse for Finished Good must
be same as Finished Good Warehouse ..." message with placeholders `{1}` and
`{2}`, but only passes two positional args (indices 0 and 1). `str.format`
raises `IndexError: Replacement index 2 out of range` instead of rendering
the message, so a user who sets the wrong FG target warehouse gets an opaque
traceback rather than the intended validation error.
Renumber the placeholders to `{0}` and `{1}` to match the args.
get_account_columns fetched the dynamic expense / unrealized-P&L account lists with
frappe.get_all(distinct=True, order_by=...). frappe silently drops ORDER BY for
distinct queries on postgres (db_query), so the generated account columns came back
in arbitrary order on Postgres while MariaDB kept them ordered — a cross-engine
parity gap (the sibling Sales Register had already moved to a python sort).
Sort the lists in python with key=str.casefold (dropping the ignored order_by) so the
column order is deterministic, case-insensitive (matching MariaDB's collation), and
identical on both engines. Add a regression test with two case-colliding expense
account names asserting the casefold column order on both engines.
get_account_columns sorts the dynamic income / unrealized-P&L account columns with
python sorted() (the original raw SQL used ORDER BY, which frappe drops for distinct
queries on postgres). Plain sorted() is case-sensitive (ASCII), so it reordered the
columns versus the pre-effort MariaDB output, whose ORDER BY ran under the
case-insensitive utf8mb4 collation.
Sort with key=str.casefold so the column order matches MariaDB's collation and is
identical on MariaDB and Postgres. Add a regression test with two case-colliding
account names ("aaa ..." / "ZZZ ...") that fails on case-sensitive sort and passes
after, on both engines.
get_customer_name's Postgres branch extracted the PURE TRAILING digits of the
name (regexp '^.*?(\d*)$'), while the MariaDB branch uses
CAST(SUBSTRING_INDEX(name, ' ', -1) AS UNSIGNED) — the LEADING digits of the last
whitespace token. For a scanned name like "<base> - 3a" MariaDB yields 3 but
Postgres yielded NULL→0, so the next de-duplicated number (and thus the generated
Customer name) diverged between engines.
Make the Postgres branch take the last whitespace token then its leading digits,
mirroring MariaDB exactly ("X - 3a"->3, "X - 1.5"->1, "X - Foo"->0). Add a
regression test with a "<base> - 3a" name asserting the next name is "<base> - 4"
on both engines (it produced "<base> - 1" on the old Postgres regex).
The Postgres-portability change added the Purchase Order Item PK (child.name) to
get_po_entries' GROUP BY. material_request_item is blank for PO lines not sourced
from a Material Request, so a multi-line PO previously collapsed to ONE row per
(PO, blank) on MariaDB but now produced one row PER LINE — changing the MariaDB
row count (and the add_total_row totals).
Group only by (PO, material_request_item) — the pre-effort key — and Max()-
aggregate the other selected columns so the query stays valid on Postgres while
restoring the prior one-row-per-group MariaDB output (per-column arbitrary→
deterministic, row count preserved). Add a regression test with a two-line PO
that fails on the multi-column GROUP BY (2 rows) and passes after (1 row), on
both MariaDB and Postgres.
