feat(spp_farmer_registry_demo): USE_CASES.md + demo data, eligibility/compliance/approval (#915)#244
Merged
Merged
Conversation
…ity/compliance/approval gaps Rewrites `spp_farmer_registry_demo/docs/USE_CASES.md` to mirror the canonical structure of `spp_mis_demo_v2/docs/USE_CASES.md` (8 farm stories with FM1-FM8 codes, 3 edge cases, 2 cooperatives, 9 demo scenarios, References section with constellation tables, program configuration, change requests, geographic distribution, overview metrics). Filipino-only locale tables — the farmer generator does not yet implement Togolese / Sri Lankan name pools. Along the way, fix or fill a handful of demo gaps surfaced while walking the doc against the running demo: - **Eligibility consistency.** Mirror the MIS fix from commit `aa6da89d`: a new `_configure_eligibility_manager` writes the program's `cel_expression` onto the eligibility manager's concrete record after wizard creation. Without this the manager has no CEL set, so seeded enrollments don't match what `Verify Eligibility` re-evaluates and registrants get demoted to Not Eligible. - **Compliance for Input Subsidy + Equipment Grant.** Add a `compliance_cel_expression` to both program defs and a sibling `_configure_compliance_manager` that wires it through. Mirrors how MIS handles Cash Transfer / Conditional Child Grant compliance. - **Approval definitions.** New `data/approval_definitions.xml` with Cycle / Entitlement / Entitlement-cycle-approver / In-kind defs, plus `_configure_program_approvals` that sets each program's cycle manager and entitlement manager `approval_definition_id` so cycles and entitlements actually enter the approval workflow. MIS demo ships the XML but never assigns the defs to the managers — that gap is for a separate ticket. - **CEL activity-count variables.** `crop_count`, `livestock_count`, `aquaculture_count`, `land_parcel_count`, `total_livestock_heads` in `spp_farmer_registry/data/cel_variables.xml` were declared with `source_type="aggregate"` against non-existent inverse names like `farm_aquaculture_activities` (the real o2m is `farm_aquaculture_act_ids`), and the framework's `aggregate_target` is a Selection limited to members/enrollments/entitlements anyway, so the lookup never resolved. Net effect: every CEL using these variables read 0 for every farm, so demo programs' CELs returned 0 matches even with seeded enrollments. Switched all five to `source_type="field"` reading from the existing stored Integer computes on `spp.farm`, and dropped `cache_strategy="ttl"` (which was routing them through `spp.data.value` cache and reading 0) in favour of `cache_strategy="none"` so the resolver inlines them as direct field reads. Flipped the file's `noupdate="1"` to `"0"` so the corrected definitions land on existing installs at upgrade.
…m assets, season state-machine, vocab coverage Round 2 on OP#915 — closes the gaps Celine flagged in her plan analysis without splitting into follow-up tickets. Demo data (`spp_farmer_registry_demo`): - Seed an irrigation network anchored on FM4 — Cotabato Irrigation Reservoir (5 000 m³ effective, silted) → Cotabato Main Canal Branch (300 m³). Source/destination M2M wired. Narratively explains FM4's 1 ha of idle/fallow land as the downstream consequence of degraded irrigation, not random non-cultivation. - Seed `spp.farm.asset` records: hand tractor on FM1, water pump on FM8. - Add a `manage_farm_asset` story CR on FM8 (water-pump expansion). The CR type is now uncommented in `spp_farmer_registry_cr/data/cr_types.xml`. - Extend `_create_active_season` to also seed a closed prior-year season, surfacing the `spp.farm.season` state machine (closed + active) at a glance. - Declare runtime deps explicitly: `spp_gis`, `spp_land_record`, `spp_irrigation`, `spp_farmer_registry_vocabularies` (all were used at runtime but never listed). Bump version 19.0.2.0.1 → 19.0.2.1.0. Docs (`USE_CASES.md`): - Add Scenario 10 (GIS + irrigation walk for FM4) anchored on the new reservoir → canal → idle-land narrative. - Add AGROVOC species-selection sub-step to Scenario 1 (FAO ICC `0116` on FM1, FAO ASFIS `TIL` on FM6) — the seed code already used these codes; the doc was just silent about it. - Add a farm-season state-machine sub-step to Scenario 1. - Update FM1, FM4, FM8 farm stories with assets / irrigation / parcel polygon details. Add a `manage_farm_asset` row to the Scenario 8 lifecycle table and the CR overview. - New References sections: irrigation infrastructure, farm assets, farm seasons. Bump CR count 9→10 and scenario count 9→10. Irrigation UI (`spp_irrigation`): - Surface irrigation assets on the farm (group) form via a new `irrigation_asset_ids` One2many on `res.partner` (inverse of the existing `farm_id`) and a new "Irrigation" notebook page. Without this, Scenario 10's "Switch to the Irrigation tab on FM4" instruction had no UI surface to point at — the records existed but were unreachable from the farm record. Bump 19.0.2.0.0 → 19.0.2.1.0. Refs OP#915.
- A2+A3 phone+bank on story personas via STORY_FARMS + _create_farm/_attach_bank_account - A4 land parcel GPS offsets to actual farmland (3–6 km from city centroids) - A5 Service Points seed (_create_service_points): Agri Co-op, Input Supply, Rural Bank - A6 entitlements: cycle flow already approves + distributes (verified during test run) - B7 demo users list + B8 locale disclaimer in USE_CASES.md - C9/C13 hide Relationships + Tags sections via inherited view - C11 move History tab right of Source Tracking - C12 add Location group with lat/lng on Profile tab - C14 hide Event Data smart button on both Group and Individual forms - C extra: hide Extension Services tab (empty for demo data) C10 (Extension Services section under Identity) — found instead as a top-level page on the farm form; hidden via inherited view. A1 (rename CR validator users) and D (deactivate household/family group types) shipped in earlier commits on this branch.
… via xml overrides (#915)
Round-3 follow-up — extend the xml override pattern already used for
role_line_ids to also rewrite name/login/email on the four upstream
spp_demo users:
demo_manager -> manager
demo_officer -> officer
demo_supervisor -> supervisor
demo_viewer -> viewer
xmlid references (env.ref("spp_demo.demo_manager") etc.) keep working
because we update fields via the same xmlid, not create new records.
Searched the tree for hardcoded login-string lookups of these users —
none found, all go through xmlid resolution. USE_CASES.md user table
updated to match.
…s, not bare res.partner.phone (#915) The previous round wrote phone numbers straight onto res.partner.phone which bypassed the spp.phone.number one2many (`phone_number_ids`) that the registrant form actually displays in its Phone Numbers tab. The legacy partner.phone char was getting populated but the registrant- form Phone Numbers list was empty — invisible to a tester looking at the proper UI surface. Add _attach_phone_number(partner, phone_no) which creates an spp.phone.number row tied to the partner; call it once per persona (head individual + farm group). The existing onchange in spp_registry.models.registrant.phone_number_ids_change still syncs the first non-disabled phone into partner.phone for legacy widgets, so nothing downstream regresses.
…eeded volume farms (#915)
Two QA-round follow-ups:
1. Two-way phone sync. Round-3 had switched phone-write from
res.partner.phone (char) to spp.phone.number (one2many) — but the
onchange that would normally sync the two only fires in form UI,
not from create(). Restore the partner.phone vals assignment so
legacy header widgets remain populated AND keep the spp.phone.number
row so the registrant Phone Numbers tab is populated. Documented in
the _attach_phone_number docstring.
2. Phone + bank on seeded volume farms. The previous round only wired
contact info on the 8 story personas; the ~600 seeded volume farms
created by SeededFarmGenerator had no phone, no bank. Adds:
- DEMO_BANKS rotation list (5 PH banks)
- _generate_phone() helper (+63 9XX XXX XXXX, 3 rng draws)
- phone + bank fields populated on every farm group + every individual
during Phase 1/3 vals build
- new Phase 4.5 _create_contact_records that batch-creates spp.phone.
number rows for all groups + individuals and res.partner.bank rows
for groups + head individuals (heads share the farm's bank)
RNG draws all added AFTER existing draws in each iteration so the
prior deterministic sequence (sizes, ages, names) is preserved.
…ocation to end of Profile tab (#915)
QA follow-up to round-3:
1. The previous Relationships hide only targeted the GROUP form's
group_relationships_section. The screenshot QA shared was from the
INDIVIDUAL form ('Family and social connections for this person'),
which uses relationships_section (no group_ prefix). Add the
individual-form hide. Also hide the preceding separator + helper div
in both forms so the visible 'RELATIONSHIPS' heading goes away too.
2. spp_registrant_gis adds a Location/coordinates group on the Profile
tab right after phone_section — middle of the tab. Move it to the
END of the Profile tab (after financial_section, which is invisible)
on both individual + group forms. Declare spp_registrant_gis as an
explicit dep so it's guaranteed loaded before our inherits.
3. Replace string-attribute xpath selectors (separator[@string='...']
and the Tags variant) with preceding-sibling::separator[1] anchored
on the named group. Odoo 19 view inheritance rejects string-based
selectors with 'View inheritance may not use attribute string as a
selector.'
4. Drop the round-3 plain Location group (latitude/longitude scalar
fields) on group_profile — redundant with the spp_registrant_gis
geo_point map widget, and lower quality.
…ort, tax_id, birth_cert) on demo personas + volume (#915) QA follow-up: demo registrants had empty Identity Documents tabs. Seed spp.registry.id rows so the Identity tab is populated with realistic test data: - Farm group: national_id + tax_id - Head individual: national_id + passport + birth_certificate (story personas); national_id + birth_certificate (volume head) - Non-head individuals in seeded volume: national_id only Values are derived deterministically from the partner name via zlib.crc32 — Python's built-in hash() is randomised per run with PYTHONHASHSEED=random, so we need a stable hash for reproducible demo data. Format mimics PH conventions: - national_id: 4-7-1 PhilSys-style block grouping - passport: P followed by 7 digits - tax_id: 9 digits - birth_certificate: BC- prefix + 7 digits All records have status='valid', verification_method='self_declared' so they show up green in the Identity tab without spoofing verification credentials. The (partner_id, id_type_id) unique constraint is respected — each partner gets at most one of each type.
…ndividuals (#915) QA follow-up: Story personas (8 farms) — STORY_FARMS dict now carries an explicit 'age' field per persona (28, 30, 32, 35, 38, 42, 50, 55 — keyed to their experience years). _create_farm builds a deterministic birthdate from age + zlib.crc32(farmer_name) so the birth month/day vary across personas but stay stable across reruns. gender was already wired via is_female flag; this commit fills in the missing birthdate. Seeded volume farms — the age rng.randint draw was already happening but was discarded with a 'Consume RNG state' comment. Capture it, draw month + day from rng (two extra draws per member after existing ones to preserve prior deterministic state for sizes/names/etc.), build a datetime.date(year - age, month, day) and set it on the individual vals. Every individual now has birthdate populated, which in turn surfaces the age field on the Demographics section.
…s, not openspp:gender (#915) The res.partner.gender_id Many2one is domain-locked to namespace_uri = 'urn:iso:std:iso:5218' with numeric codes (1=Male, 2=Female, 0=Unknown). Both demo generators were looking up 'urn:openspp:vocab:gender' with 'male'/'female' string codes, which silently returned False — every demo individual ended up with an empty Gender field. - farmer_demo_generator._create_farm: switch lookup to ISO 5218 with '1'/'2' codes. - seeded_farm_generator._get_gender_id: map 'male'/'female' to ISO 5218 numeric codes then look up in the right namespace. Plus housekeeping: drop unused PhoneNumber/PartnerBank/VocabCode locals the linter flagged, rename unused bp loop variable to _bp, and silence C901 complexity warning on _create_contact_records (it's a three-phase batch creation; splitting it would obscure the shared bank_id_by_name / id_type_ids resolutions).
…Types vocab (#915) QA round-3 asked us to 'add Service Points (research what would make sense to add here).' The previous pass seeded 3 minimal records with no service-type tags. Two gaps: 1. The Service Types vocabulary (urn:openspp:vocab:service-types) shipped by spp_service_points was empty — no codes defined, so the service-type tag field on each point couldn't be set. 2. The 3 record set didn't cover the realistic farmer touchpoint mix. This commit: - New data/service_types.xml seeds 6 codes in the Service Types vocab: crop_collection, input_supply, cash_disbursement, veterinary, extension_services, equipment_rental. The vocab is is_system=False so non-system writes are allowed. - _create_service_points expanded from 3 to 6 entries: * Agri Co-op Office (crop_collection + extension) * Input Supply Depot (input_supply) * Rural Bank Branch (cash_disbursement) * Provincial Veterinary Clinic (veterinary) * Agricultural Extension Office (extension_services) * Mechanization Equipment Rental Hub (equipment_rental) Each is tagged with one or more service types and anchored to a demo area (PH-NUE / PH-BUK / PH-MAG / PH-BTG / PH-LAG / PH-NUE).
…by farm type (#915)
QA follow-up: the 6 service points existed under the Service Points
menu but were never wired onto any farm group. The farm form's Service
Points tab was empty.
Add _FARM_TYPE_SERVICE_POINTS mapping (duplicated in both generators
since they live in separate files / classes — keeping them in sync is
trivial and avoids cross-imports):
crop -> Agri Co-op, Input Supply, Equipment Rental, Bank, Extension
livestock -> Veterinary, Input Supply, Bank, Extension
mixed -> all crop + livestock specialties
aquaculture -> Input Supply, Bank, Extension
Story farms: _create_story_farms now calls _resolve_farm_service_points
on each persona and writes service_point_ids on the farm group with
Command.set(...).
Seeded volume farms: _create_contact_records adds a final phase that
resolves all 6 service point IDs by name once, then walks every farm
group + its blueprint to write service_point_ids based on farm_type.
Falls back to {Rural Bank Branch + Agricultural Extension Office} for
unknown farm_type values.
…roups aren't identical (#915)
QA observation: 'all groups have the same amount of service points'.
The previous round linked every farm of a given type to the same fixed
list — so all crop farms had 5, all aquaculture farms had 3, etc. From
the user's POV scrolling through 600 farms, the Service Points tab
looked like a clone factory.
Restructure:
- _UNIVERSAL_SERVICE_POINTS — Rural Bank Branch + Agricultural Extension
Office — always linked on every farm (cash collection + advisory are
universal touchpoints).
- _FARM_TYPE_SPECIALISED_POINTS — the type-specific pool that varies
per farm:
crop -> {Agri Co-op, Input Supply, Equipment Rental}
livestock -> {Vet, Input Supply}
mixed -> {Agri Co-op, Input Supply, Vet, Equipment Rental}
aquaculture -> {Input Supply}
- Each farm picks a deterministic subset of size 1..N from its pool
using zlib.crc32(farm.name) so the same farm always shows the same
Service Points across reruns, but different farms of the same type
show different counts and combinations. Variance:
crop -> 1..3 specialised + 2 universal = 3..5 SPs total
livestock -> 1..2 specialised + 2 universal = 3..4 SPs total
mixed -> 1..4 specialised + 2 universal = 3..6 SPs total
aquaculture -> 1 specialised + 2 universal = 3 SPs total
Applied symmetrically in farmer_demo_generator (story farms) and
seeded_farm_generator (volume farms).
- Individual form: move History tab right of Source Tracking (the previous override only handled the group form's group_history / source_tracking_group page names; the individual form uses history / source_tracking). - Individual form: hide the Event Data smart button. spp_event_data adds two buttons with the same name to the same button_box, so a plain //button[@name='open_create_event_wizard'] xpath only catches the first occurrence. Replaced the two separate inherits with a single one on the base form that uses XPath contains() to distinguish the group vs individual button via their invisible expressions. - Demo wizard: surface the MapTiler API key prerequisite (spp_gis needs spp_gis.map_tiler_api_key in System Parameters to render tiles; the parameter ships with the placeholder YOUR_MAPTILER_API_KEY_HERE). - USE_CASES.md: re-verified all 10 scenario menu paths against the running instance via XML-RPC and corrected the wrong ones: * Vocabularies live under Settings, not Registry * Seasons live under Registry > Configuration, not Settings * Groups/Individuals under Registry > Browse All (Audit) * Programs is the top-level menu; sub-menu is also Programs * Cycles / Compliance Manager / Verify Eligibility are tabs and buttons on the program form, not menus * Scenario 10 reframed: spp_gis / spp_land_record / spp_irrigation don't ship top-level menus, so the scenario uses the form smart-button + developer-mode entry points instead.
…915) Verify Eligibility only re-evaluates registrants that already have a spp.program.membership row (filtered by states ['enrolled', 'not_eligible']). It will not scan the global registrant table for new matches, so the original wording — "create a new farm → click Verify Eligibility → it gets enrolled" — was logically broken. Added the missing step: from the new farm's form, click Enroll in Program first (action_enroll_in_program → enrollment wizard) to create the membership row, THEN run Verify Eligibility to evaluate the CEL and flip the state to enrolled. Added an explanatory note about why the two-step path is required.
…d against running instance (#915) Walked the program form, the registrant form, and the farm form in the running instance via XML-RPC + view-arch reads, and corrected every UI reference that didn't match reality: - Cycles is a smart button on the program form, not a tab. Scenarios 2, 5, 9 corrected. - New Cycle is a header-bar button on the program form (not inside a Cycles tab). Scenario 9 corrected. - Eligibility is a section inside the Configuration tab (Eligibility Method / Eligibility Manager separator), not its own tab. Scenarios 4, 5 corrected. - Compliance Method section lives in the Configuration tab on the cards layout; primary view exposes Compliance Manager as a separator/manager block. Scenario 3 corrected. - Land records are reached via the Land Parcels smart button or the Land Parcels notebook tab on the farm form — NOT a 'Land Records' smart button (which doesn't exist). Scenario 10 corrected. - Irrigation assets are on the Irrigation notebook tab on the farm form — NOT an 'Irrigation Assets' smart button. Scenario 10 corrected. - Change Requests has no per-partner smart button; use Change Requests → All Requests with a registrant search filter. Scenario 8 corrected. - Scenario 4 'Verify Eligibility on FM1' reframed: verify_eligibility takes no record arg. The actual flow is Enroll in Program → FM1 + Aquaculture Support → membership goes to not_eligible because the CEL evaluates aquaculture_count == 0 for FM1.
…R workflow, doc (#915) Addresses the May 20 QA findings (attachments 894-905) by fixing what the generator actually produces so it lines up with the doc: Story data - Enrollment dates were stamped with `today` because the stored compute `_compute_enrolled_date` re-fires after a write and reads the in-cache "to_compute" sentinel (False), so the SQL backdate looked unset. The compute now reads the persisted row before deciding, preserving demo generator/migration backdates. - Head ages were off by 1 when the deterministic birthday hadn't passed yet this year. Roll birth_year back one when the hashed month/day is still in the future. - Maria (FM1) and Juan (FM2) persona ages bumped to 42 and 45 to match the FM1/FM2 demographics tables. CR workflow - All approved/rejected/revision CRs were getting stuck at "under review" because admin lacks the CR Local/HQ Validator groups, and `action_reject` / `action_request_revision` are wizards that just open a dialog. New `_advance_cr_to_terminal` walks every approver tier with `with_user(validator).sudo()`, and routes reject/revision to `_do_reject` / `_do_request_revision` so the transition actually fires. - `manage_farm_asset` was missing an approval definition link, so CR10 stayed at draft. Re-enabled the local→HQ link in `approval_links.xml`. Doc - `USE_CASES.md` payment lists for FM2, FM5, FM7, FM8 now reflect the three-cycle reality the generator produces (3×₱200 / 3×₱275, no per-hectare/per-head bonus until the CEL-driven entitlement formula lands). - Input Subsidy + Livestock Support entitlement tables note that the formula is design intent and the demo cycles use the flat fallback. Verified by 4 clean resetdb+generator cycles; all 10 CRs end in the documented final state and every per-farm field in attachments 894-901 now matches.
…inistic under volume load (#915) Round-6 verification surfaced that with volume generation enabled, story farms ended up with **zero** entitlements/payments even though the cycles themselves looked OK. Root cause: three different queue-job dispatches race against the demo: 1. ``program_manager.new_cycle()`` calls ``cm.add_beneficiaries(...)``, which dispatches to ``_add_beneficiaries_async`` once enrollment crosses ``MIN_ROW_JOB_QUEUE``. Volume runs hit 200+ beneficiaries per program, so the cycle was created with ``is_locked = True`` and zero ``cycle_membership_ids``. Subsequent ``action_submit_for_approval`` raised ``Cycle is locked: Importing beneficiaries.``. 2. ``cycle.prepare_entitlement()`` also dispatches to ``_prepare_entitlements_async`` above the same threshold — would have produced no entitlements anyway because step 1 left no cycle members. 3. ``cycle.prepare_payment()`` dispatches to ``_prepare_payments_async`` once approved entitlements cross ``MAX_PAYMENTS_FOR_SYNC_PREPARE``, so even after entitlements existed payments were queued and never ran inside the demo's transaction. For deterministic demo data we now call the synchronous private hooks directly: - ``cycle_manager._add_beneficiaries(cycle, ids, "enrolled", do_count=True)`` when the cycle came back locked, then clear ``is_locked``. - ``cycle_manager._prepare_entitlements(cycle, do_count=True)`` regardless of beneficiary count. - ``payment_manager._prepare_payments(cycle, approved_entitlements)`` regardless of entitlement count. Also added a fallback that force-writes ``draft → to_approve`` on the cycle when ``action_submit_for_approval`` raises ``Cycle is locked``, so the rest of the flow can proceed even if a future call path still leaves the lock set. Verified with ``generate_volume=True`` (1170 memberships across 8 story farms + ~440 volume farms) — every story farm now has the expected entitlements in ``rdpd2ben`` state with the backdated approval dates, and CR states still resolve to the documented final states.
…ormulas, cycle approver, irrigation access (#915) - Create a compliance manager for programs that ship a compliance CEL. The base create wizard never makes one, so programs showed 'no compliance defined'. Enable compliance verification at wizard time so Input Subsidy and Equipment Grant carry their compliance rule. - Materialise the documented benefit formula on the cash entitlement manager: Input Subsidy = base + (farm_size_hectares * rate), Livestock = base + (total_livestock_heads * rate). Previously only a flat amount was disbursed. - Add a Program Manager demo user able to approve cycles + entitlements, and grant queue-job manager rights to the program-manager and cycle-approver roles — approving a cycle enqueues the entitlement validation job, which needs queue.job create rights. - Grant registry users read access to spp.irrigation.asset so farm records open without an Access Error. - Refresh USE_CASES.md (demo users, program config, Scenario 9). - Repair stale tests left by earlier rounds (GPS pick API, ISO 5218 gender codes, area-anchored land records, manage_farm_asset CR type) and add tests for the new compliance / formula / approver wiring.
…le access (#915) - Load Farmer Demo aborted on the second program: clearing the wizard's flat cash-entitlement line with (5, 0, 0) orphaned it (entitlement_id = NULL) instead of deleting it, and the deferred write violated the NOT-NULL constraint on the next program's flush, aborting the whole transaction. Delete the lines with unlink() instead. - Farm users hit 'not allowed to access Cycle (spp.cycle)' when opening an enrolled farm: the registrant form lists entitlements with their cycle_id column, but spp.cycle read was missing from the registry-officer ACL (entitlements, cycle memberships and payments already had it). Add the read grant for spp.cycle. - Add regression tests: full multi-program build (reproduces the orphaned line that single-program tests missed) and farm-user spp.cycle read.
The round-7 demo shipped a Program Manager user to approve cycles, but a Program Manager is not a Cycle Approver: cycle approval is gated on the Cycle Approver functional role (the group the Approve Cycle button is shown to), and group_programs_manager does not imply group_programs_cycle_approver. So the manager could neither see nor pass the cycle approval check. - Add a dedicated 'cycle_approver' demo user holding the Program Cycle Approver role (which also carries queue-job manager rights, so approving a cycle does not hit the entitlement-validation queue access error). - Route the demo's cycle approval definition through group_programs_cycle_approver so the approver can actually approve cycles. - Grant the Cycle Approver group to SPP Admin so Load Farmer Demo approves cycles cleanly instead of falling back to a forced state write. - Add regression tests covering the approval-group wiring and group membership.
…try-demo-use-cases # Conflicts: # spp_programs/data/user_roles.xml # spp_programs/security/ir.model.access.csv
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive enhancements to the OpenSPP Farmer Registry Demo, including GIS and irrigation network seeding, farm asset tracking, approval workflows for cycles and entitlements, and polished registrant form views. Feedback on these changes highlights a critical synchronization bug in 'program_membership.py' where raw SQL is used inside a compute method to fetch 'enrollment_date', which can bypass the cache and overwrite backdated values; using Odoo's ORM cache inspection is recommended instead. Additionally, the reviewer pointed out that using Python's non-deterministic 'hash()' function for bank account numbers in 'farmer_demo_generator.py' will cause inconsistencies across server restarts, and recommended using 'zlib.crc32'. Finally, the use of native 'datetime.date.today()' in both 'farmer_demo_generator.py' and 'seeded_farm_generator.py' should be replaced with 'fields.Date.today()' to respect timezone context and facilitate unit testing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+170
to
+187
| # The compute can re-fire after the field has already been persisted | ||
| # (re-write of `state`, ORM-level flushes, etc.). Reading `rec.enrollment_date` | ||
| # in-cache returns the "to_compute" sentinel (False) so we can't rely on it | ||
| # to detect a prior value — peek at the persisted row instead. Demo | ||
| # generators and migration scripts may have intentionally backdated this. | ||
| persisted = {} | ||
| existing_ids = [rec.id for rec in self if isinstance(rec.id, int)] | ||
| if existing_ids: | ||
| self.env.cr.execute( | ||
| "SELECT id, enrollment_date FROM spp_program_membership WHERE id IN %s", | ||
| (tuple(existing_ids),), | ||
| ) | ||
| persisted = dict(self.env.cr.fetchall()) | ||
|
|
||
| for rec in self: | ||
| if rec.state == "enrolled": | ||
| rec.enrollment_date = fields.Datetime.now() | ||
| prior = persisted.get(rec.id) | ||
| rec.enrollment_date = prior or fields.Datetime.now() |
There was a problem hiding this comment.
Executing raw SQL inside a compute method to fetch the persisted value of enrollment_date is an anti-pattern in Odoo and introduces a critical synchronization bug.
If enrollment_date is modified in the cache during the current transaction (e.g., backdated by a demo generator or migration script) but has not been flushed to the database yet, self.env.cr.execute will read the old value (or NULL) from the database. The compute method will then overwrite the newly assigned cache value with fields.Datetime.now(), discarding the intended backdated date.
Instead, use Odoo's ORM cache inspection (env.cache.contains) to check if a value is already present in the cache, and fall back to _origin to read the persisted database value cleanly without raw SQL.
Suggested change
| # The compute can re-fire after the field has already been persisted | |
| # (re-write of `state`, ORM-level flushes, etc.). Reading `rec.enrollment_date` | |
| # in-cache returns the "to_compute" sentinel (False) so we can't rely on it | |
| # to detect a prior value — peek at the persisted row instead. Demo | |
| # generators and migration scripts may have intentionally backdated this. | |
| persisted = {} | |
| existing_ids = [rec.id for rec in self if isinstance(rec.id, int)] | |
| if existing_ids: | |
| self.env.cr.execute( | |
| "SELECT id, enrollment_date FROM spp_program_membership WHERE id IN %s", | |
| (tuple(existing_ids),), | |
| ) | |
| persisted = dict(self.env.cr.fetchall()) | |
| for rec in self: | |
| if rec.state == "enrolled": | |
| rec.enrollment_date = fields.Datetime.now() | |
| prior = persisted.get(rec.id) | |
| rec.enrollment_date = prior or fields.Datetime.now() | |
| for rec in self: | |
| if rec.state == "enrolled": | |
| prior = False | |
| if rec.env.cache.contains(rec, rec._fields["enrollment_date"]): | |
| prior = rec.enrollment_date | |
| elif isinstance(rec.id, int): | |
| prior = rec._origin.enrollment_date | |
| rec.enrollment_date = prior or fields.Datetime.now() |
Comment on lines
+1066
to
+1067
| seed = f"{bank_name}|{salt}|{partner.name}" | ||
| digits = "".join(c for c in str(abs(hash(seed))) if c.isdigit())[:12].rjust(12, "0") |
There was a problem hiding this comment.
Using Python's built-in hash() function to generate bank account numbers is not deterministic across different Python processes or server restarts because of Python's default hash randomization. This means the generated demo bank account numbers will change every time the Odoo server is restarted, violating the design goal of deterministic demo data generation.
Use zlib.crc32 (which is stable and deterministic across runs) to generate the digits, similar to how other registry IDs are generated in this module.
import zlib
seed = f"{bank_name}|{salt}|{partner.name}"
digest = zlib.crc32(seed.encode("utf-8"))
digits = str(digest).rjust(12, "0")| import zlib | ||
|
|
||
| digest = zlib.crc32(farmer_name.encode("utf-8")) | ||
| today = datetime.date.today() |
There was a problem hiding this comment.
Using native datetime.date.today() is discouraged in Odoo because it does not respect the user's timezone context and cannot be easily mocked in unit tests. Use fields.Date.today() instead.
Suggested change
| today = datetime.date.today() | |
| today = fields.Date.today() |
| age = self.rng.randint(*member_spec["age_range"]) | ||
| birth_month = self.rng.randint(1, 12) | ||
| birth_day = self.rng.randint(1, 28) | ||
| today = datetime.date.today() |
There was a problem hiding this comment.
Using native datetime.date.today() is discouraged in Odoo because it does not respect the user's timezone context and cannot be easily mocked in unit tests. Use fields.Date.today() instead.
Suggested change
| today = datetime.date.today() | |
| today = fields.Date.today() |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #244 +/- ##
==========================================
+ Coverage 71.55% 71.83% +0.27%
==========================================
Files 977 996 +19
Lines 58265 59791 +1526
==========================================
+ Hits 41694 42952 +1258
- Misses 16571 16839 +268
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Pre-commit fixes for CI: regenerate the spp_farmer_registry_demo and spp_irrigation README/description from fragments, and apply prettier formatting to spp_irrigation/views/irrigation_view.xml.
gonzalesedwin1123
approved these changes
Jun 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why is this change needed?
OP#915 — rewrite
spp_farmer_registry_demo/docs/USE_CASES.mdin the MIS demo format and harden the farmer demo so it loads cleanly and exercises the full registry → enrollment → cycle → entitlement → payment flow. Iterated across multiple QA rounds.How was the change implemented?
spp.phone.number, bank accounts, service points linked by farm type, GPS anchored to verified farmland; deterministic cycle/payment generation under volume load.spp.cycleand irrigation assets.spp_demousers.New unit tests
spp_farmer_registry_demotest suite — story farms, season state-machine, demo program/cycle generation, multi-program build (reproduces the orphaned-line crash), cycle-approver wiring, and farm-userspp.cycleread.Unit tests executed by the author
spp_farmer_registry_demo: 0 failed, 0 errors of 128 tests (against the current19.0base after merge).How to test manually
See the QA guides on OP#915. In short:
./spp-demo start && ./spp-demo resetdb -y && ./spp-demo start, installspp_farmer_registry_demo, run Settings → Demo Data → Load Farmer Demo, then verify per the latest round guide (cycle approval ascycle_approver, entitlement formulas, compliance).Related links
OP#915