feat(spp_drims): Donations review — creation, receipt, inspection and follow-up - #414
feat(spp_drims): Donations review — creation, receipt, inspection and follow-up#414emjay0921 wants to merge 12 commits into
Conversation
Adds a Draft start stage to the donation lifecycle (Draft -> Announced -> Received -> Inspected -> Stocked) with a "Mark Announced" action, and reshapes the form around it: - Cancel renamed to "Cancel Donation". - Donor limited to DRIMS organisations whose role is Donor; donations cannot be recorded against a closed incident (domain + constraint). - At least one item is required to save; the line Description field is removed and Pledged must be entered (no default, must be > 0). - Line columns appear progressively: Received (manual, mandatory) and Variance from Announced, Condition and Action from Inspected; Expiry Date only when product_expiry is installed. - Received quantities are entered manually instead of being auto-copied from Pledged. - Once inspected, items with a non-accept disposition are listed in a separate "Items Not Accepted for Stock" table.
Donation items can only be added or removed while the donation is in draft. The line table is read-only from the received state onward so the "Add a line" row no longer appears once a donation has been received, and donation-line create/unlink are guarded at the model level. The inspection wizard's split rows opt out of the guard so splitting an inspected donation still works.
A product added through "Add a line" on a donation was created non-storable, so it never tracked in inventory and did not appear in Stock on Hand. The donation-line product field now defaults inline product creation to a storable Good (type consu, Track Inventory on).
Items excluded from stock at stocking time (Return / Dispose / Quarantine) are now tracked to resolution instead of only being mentioned in a toast. - Donation lines gain a disposal status (Pending -> Resolved) with resolved date, user and notes; stocking seeds the excluded lines to Pending. - A 'Mark Resolved' action records who/when and posts an audit note to the donation's chatter for accountability. - A dedicated 'Non-Accepted Items' list + menu under Monitoring lists the excluded items (default Pending), filterable and groupable by action; the donation form's 'Items Not Accepted for Stock' table shows the disposal status and a resolve button.
…op empty rows (#1058)
…ons-review # Conflicts: # spp_drims/__manifest__.py # spp_drims/security/ir.model.access.csv
A new donation offered no way to add items. The stylesheet that hides Odoo's blank filler rows on the donation tables excluded .o_field_x2many_list_row_add from the rows it hid, on the assumption that class sits on the <tr>. In Odoo 19 it is on the <td> and the row itself is <tr class="d-print-none">, so the exclusion never matched and the rule hid the "Add a line" row along with the fillers. This was worse than being unable to add items. OP#1076 also requires at least one item before a donation can be saved, so with the add row hidden there was no way to satisfy that rule: a donation could not be created at all. The two rules closed the door on each other. Match the fillers by having no class instead, which is what actually distinguishes them - data rows carry o_data_row and the add row carries d-print-none. If Odoo ever gives fillers a class the rule stops applying and the blank rows come back, which is a cosmetic regression rather than a form nobody can enter data into. Also switches the donation line's hidden quantity field from invisible to column_invisible. Inside a list the former blanks the cells but still renders the column, which is why an empty "Quantity" column sat between Pledged and Unit.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #414 +/- ##
==========================================
+ Coverage 72.66% 72.77% +0.11%
==========================================
Files 329 330 +1
Lines 24298 24412 +114
==========================================
+ Hits 17655 17766 +111
- Misses 6643 6646 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The SL demo generator could no longer produce donations. It assumed a donation is created in "announced" and that Mark Received copies the pledged quantity, both of which OP#1076 changed: donations now start in draft, and the received quantity is entered by hand and required before a donation can be marked received. Walk from draft with an explicit announce step, and record demo donations as arriving in full so the receive step has quantities to work with. Caught by CI, not locally: spp_drims_sl_demo is two levels down the dependency chain from spp_drims and a spp_drims-only run cannot see it.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Thanks — of the current DRIMS PRs this is the strongest one. The line guards are server-side with a documented, deliberate escape hatch for the inspection wizard's split rows; the closed-incident rule is an @api.constrains, so it fires on create and on re-pointing incident_id (exactly the write-hole I flagged on #416's create-only guard); the tr:not([class]) CSS incident is thoroughly documented with a stated cosmetic failure mode and pinned by a regression test; and the column_invisible-vs-invisible arch test is a nice systemic guard. The _receive_donation test helper and demo-generator updates are consistent throughout.
Two blockers, one cross-PR problem, and two smaller items.
1. Blocker — no version bump; without an upgrade, new donations get NO state
spp_drims stays at 19.0.3.0.0 and spp_drims_sl_demo at 19.0.2.0.0 — both identical to 19.0, both present in the 2026.08 release tag. This one is worse than the usual "views don't load": the new draft state is a data record (code_drims_donation_draft in vocabulary_codes.xml), and _get_default_state now searches for code = 'draft'. Deploy the new code without -u spp_drims and that search returns an empty recordset — every new donation is created with no state at all, so no lifecycle buttons match and the donation flow is dead. The Python takes effect on deploy; the data it depends on does not.
Ask: version bumps on both modules so deployments actually upgrade and load the vocabulary code, views, CSS asset and access rows.
2. Blocker — description removed from spp.drims.donation.line, undescribed and unmigrated
The field is dropped from the model and every view. That is (a) a schema change to a released module with no migrations/ entry, (b) silent hiding of user-entered data — the column is orphaned and any existing descriptions become invisible, and (c) absent from the PR description, so it lands unreviewed as far as the PR body is concerned.
Ask: either restore the field, or state the removal in the PR body (with QA's confirmation) and ship it deliberately alongside the version bump/migration story from #1.
3. Major — collision with #416 (both open, both touch the donation flow)
- Semantic conflict that will break
19.0: #416's tests create donations with no lines (test_1164_donation_allowed_warehouses_respects_filter,test_1164_allowed_warehouses_fallback_when_incident_has_none) — this PR's new_check_has_linesconstraint forbids exactly that. Whichever merges second turns the base branch red, with no textual conflict to warn anyone. - Duplicate closed-incident guard: #416 adds a
_drims_ensure_opencall indonation.create(UserError); this PR adds_check_incident_not_closed(ValidationError, strictly broader). Suggest keeping this PR's constraint and dropping the donation-create guard from #416. - Both PRs also edit
donation.py::createanddonation_views.xml, so expect textual conflicts too.
Cheapest reconciliation: #416 adds a line to its two donation fixtures and drops its donation-create guard in favour of this constraint. Worth deciding the merge order now, since both are yours.
4. Medium — required="parent.state == 'announced'" on Received blocks legitimate zeroes
For float fields the web client treats 0.0 as "not set", so on an announced donation a line whose item simply never arrived (received = 0, variance = the full shortfall) cannot be saved through inline editing — while the server rule is only "at least one line > 0" and the wizard happily writes 0. The same entry has different rules depending on which surface you use. Suggest dropping required (the action_mark_received guard already enforces the business rule) or documenting the stricter-UI intent.
5. Minor
- No non-negative guard on
quantity_received(inline or wizard): a negative value passes theany(> 0)check when another line is positive, then flows into the receipt-picking moves and fails later with an obscure stock error. Mirror the new_check_quantity_pledgedwith a>= 0constraint on received. test_1108_inline_product_defaults_to_storablebuilds the context by hand, so it exercises Odoo's defaulting machinery rather than the fix — it would still pass if the view lost itscontext="{'default_type': 'consu', 'default_is_storable': True}". An arch assertion onproduct_id's context (in the style of this PR's other arch tests) would pin the actual change.
…s, negative guard Version bumps on spp_drims and spp_drims_sl_demo. Without one, a deployment loads the new Python without upgrading the module, and the new draft state is a data record: _get_default_state searches for code 'draft', finds nothing, and every new donation is created with no state at all, so no lifecycle button matches. The changelog entries record that, and state the donation line's Description removal explicitly — it was replaced by the product and quantity columns during the rework, and the database column is left in place, so existing values are retained but no longer reachable through the ORM. Received is no longer required by the items list. The web client reads 0.0 on a float as "not set", so the attribute refused to save a line for an item that was pledged and never arrived — received 0, variance the full shortfall — while the wizard wrote exactly that and the server rule only asks for one line above zero. The same entry was legal in one surface and impossible in the other; the rule that matters stays in action_mark_received. A negative received quantity is now refused. It slipped past "at least one line above zero" whenever another line was positive, then reached the receipt picking and failed there as an obscure stock error, a long way from the field that caused it. test_1108 asserted through Odoo's defaulting machinery rather than the fix: it passed whether or not the view still carried the product context that OP#1108 added. It now reads that context off the items list — located by its Pledged column, since the form holds other lists whose product_id carries no such context.
|
Thanks — all five addressed. Pushed as 1. Version bump — done. 2. 3. #416 collision — half fixed here, half deferred, deliberately.
Merge order preference either way? 4. 5. Minor — both done.
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approved — every item from the review is resolved, and the resolutions carry their own documentation.
Verified on b47149a1:
- Bumps with the right semantics.
spp_drims→19.0.4.0.0, with the Breaking label earned by the Description removal, and the changelog recording the exact no-upgrade failure mode (the draft state is a data record, so un-upgraded deployments would create donations with no state).spp_drims_sl_demo→19.0.2.1.0. - The Description removal is now a deliberate, disclosed decision — stated in the PR body as Breaking, QA's pass noted, the no-migration reasoning explicit (Odoo leaves the column; values retained, ORM-unreachable). Keeping the column rather than dropping it is the conservative right call; no explicit drop needed from my side.
- The Received
requiredis gone, with the view comment documenting the float-0.0 trap and where the real rule lives (action_mark_received), and an arch test pinning thatrequiredstays absent — so the two surfaces now agree, and stay agreed. - Negative received refused by a constraint whose docstring preserves the zero-is-meaningful case, tested in both directions.
test_1108now pins the actual fix: it reads the context off the items list itself, located by its Pledged column — it fails if the view loses the storable-product defaults, which the old test did not.- The #416 collision is resolved from both sides (its fixtures gained lines on that branch; the duplicate-guard consolidation is tracked in OP#1176 with this PR's constraint as the canonical shape).
One merge-time note, not a change request: at 19.0.4.0.0, if this lands first — your proposal, which also front-loads the constraint OP#1176's cleanup depends on — then #416 (19.0.3.1.0) and #433 (19.0.3.0.4) both become version decreases and renumber above 4.0.0 at their merge-time rebases, with #416's migration directory moving to its new number. Edwin has all the ordering inputs.
Merge stays with Edwin per the usual flow.
2 participants
Why is this change needed?
OP#1076 (DRIMS - Full review - Donations) and all four of its children have passed QA — this raises the PR for that work, per the flow of opening PRs once QA signs off.
What is in here
Donation creation and lifecycle UX reworked — the form leads with what the donor pledged, columns appear as the donation progresses (Pledged in draft, Received and Variance once announced, Condition and Action after inspection), and a donation now requires at least one item before it can be saved.
Line entry is bounded by state (OP#1055) — items can be added while the donation is draft or announced and not after, so nobody can append product-less rows to an already-received donation.
Inline-created products are storable (OP#1108) — a product quick-created from the donation line now defaults to a storable Good, so it actually tracks in inventory instead of silently never appearing in Stock on Hand.
Received quantities are entered through a wizard (OP#1163) — "Mark Received" opens a form for what actually arrived, rather than assuming the pledged quantity.
Breaking — the donation line's
descriptionfield is removed. It was replaced by the product and quantity columns in the rework, and QA passed the flow without it. Odoo does not drop the column when a field goes, so values entered before this change are retained in the database but are no longer reachable through the ORM and appear in no view. Nomigrations/entry, because nothing needs migrating; say so in review if the column should be dropped explicitly instead.Version bumps —
spp_drimsto19.0.4.0.0andspp_drims_sl_demoto19.0.2.1.0. These matter more than usual here: the newdraftstate is a data record (code_drims_donation_draft), and_get_default_statesearches for it by code. A deployment that loads this Python without upgrading the module finds nothing, and every new donation is created with no state at all.Non-accepted items have a follow-up workflow (OP#1058) — items returned, disposed of or quarantined at inspection are tracked to resolution in their own table, instead of disappearing from the donation.
New unit tests
Covering the lifecycle guards, the receive wizard, the follow-up states on non-accepted items, and the form structure.
spp_drimsruns 285 tests.Unit tests executed by the author
On the merged branch:
./spp lint --all-filesclean.How to test manually
Related links
https://openspp.openproject.com/work_packages/1076
Reviewer notes
Two rules in here close a door on each other if either is wrong, which is worth knowing while reviewing. A donation cannot be saved without at least one item, and the items table is state-bounded. During QA a CSS rule intended to hide Odoo's blank filler rows also hid the "Add a line" control — so there was no way to add an item and no way to save without one, and a donation could not be created at all. Fixed in
4751c7ee; the filler rows are now matched by having no class, which is what actually distinguishes them from the add row.The branch was brought up to date by merging
19.0, not rebasing, since it was already published.