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.
Drop the dead 'if coupon:' guard (get_doc would have thrown) and collapse the
duplicate increment branches into a single exhausted-check plus increment.
No behaviour change.
Use do_not_submit=1 for the service-item and reserve-warehouse validation
tests; they only exercise in-memory validation methods, so submitting the
Subcontracting Order is unnecessary.
The over-order check sums the same item across multiple order rows. Add a
test where one item is split into two Sales Order rows against the same
blanket order and together exceed its quantity.
Add tests for the previously-untested branches of validate_coupon_code
(not-yet-valid, expired, maximum-use exhausted) and update_coupon_code_count
(releasing a use on cancel, and rejecting use beyond the maximum). Both
functions are now fully covered.
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
calculate_service_costs paired the service_items and items child tables
by list index, which breaks if the tables are not index-aligned (e.g.
populate_items_table skips a service item with zero available qty),
assigning the wrong service cost or raising IndexError. Match by
purchase_order_item instead, and guard against division by zero qty.
Adds a regression test asserting service costs follow purchase_order_item
regardless of table ordering.
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%.
Covers previously untested Subcontracting Order paths:
- a Subcontracting Order requires a subcontracting Purchase Order
- service items must be non-stock items
- a supplied item's reserve warehouse must differ from the supplier warehouse
- the Subcontracting Receipt mapper applies BOM process-loss to the received qty
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.
The Patch Test starts the full bench (incl. workers) and then runs migrate.
Migrate enqueues orphan-link cleanup jobs (delete_dynamic_links) that the
workers pick up and process while migrate is altering tables, which
intermittently fails with MySQL 1412 'Table definition has changed, please
retry transaction'.
Start every bench process except the workers during migrate, so nothing
consumes the queue mid-migrate. Redis and the other services stay up; the
queued jobs just wait.
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.
