Fix/362 birthdate not future - #397
Conversation
| with self.assertRaises(ValidationError): | ||
| self.individual_a.write({"birthdate": future}) | ||
|
|
||
| def test_future_birthdate_rejected_on_create(self): |
There was a problem hiding this comment.
This test passes on the base branch without the new constraint, so it does not actually cover the create path.
registration_date is fields.Date(default=lambda self: fields.Date.today()) (spp_registry/models/registrant.py:47), and Odoo applies defaults before validating: _create runs records._validate_fields(name for data in data_list for name in data["stored"]), and data["stored"] includes defaulted fields. So _check_registration_date (registrant.py:127-136) fires on this create and raises "Registration date must be later than the birth date." because registration_date (today) < birthdate (tomorrow).
assertRaises(ValidationError) therefore succeeds for the wrong reason — remove _check_birthdate_not_future and the test still passes. Assert on the message, e.g. assertRaisesRegex(ValidationError, "Date of birth cannot be in the future"), or pass an explicit past registration_date so only the new constraint can fire.
There was a problem hiding this comment.
Correct, and thanks for tracing it. One thing though: the second remedy you suggested doesn't work. Passing an explicit past registration_date can't isolate the new constraint, because _check_registration_date refuses any registration_date earlier than the birthdate and once the birthdate is in the future that's true of every valid date, so there's no past date that escapes it. What does isolate it is passing registration_date as empty. The field isn't required, so that constraint short-circuits on if record.registration_date: and _check_birthdate_not_future becomes the only one that can fire. The test now does that and asserts on the message via assertRaisesRegex, so it fails if the constraint is removed. I put the reasoning in the class docstring so the empty registration_date doesn't get tidied away later as redundant.
| } | ||
| ) | ||
|
|
||
| def test_future_birthdate_rejected_on_import(self): |
There was a problem hiding this comment.
Same vacuity as the create test: load() applies the registration_date default (today), so the pre-existing _check_registration_date raises "Registration date must be later than the birth date." for a tomorrow birthdate. result["messages"] is truthy and result["ids"] is falsy on the base branch too, so this test does not prove the import path is covered by the new constraint.
Assert the message text instead, e.g. self.assertIn("Date of birth cannot be in the future", str(result["messages"])), or include a past registration_date column in the load.
There was a problem hiding this comment.
Same fix as the create test: the load() call now includes a registration_date column with an empty value, and asserts "Date of birth cannot be in the future" appears in result["messages"] rather than just checking that messages exist. test (spp_registry) passes with it.
| @@ -1,3 +1,7 @@ | |||
| ### 19.0.2.1.5 | |||
There was a problem hiding this comment.
Version regression: the module is already at 19.0.2.2.2 on the base branch (19.0.2.2.3 on 19.0 now), so a 19.0.2.1.5 heading inserted above 19.0.2.2.2 both breaks the descending order of this file and names a version older than what ships.
Also, spp_registry/__manifest__.py is not bumped at all, so this behaviour change ships with no version increment and Odoo will not run a module upgrade for it — sites that only upgrade on a version change will keep the old code.
Two follow-ups: bump the manifest and use the next version above the current head (e.g. 19.0.2.2.4), and regenerate spp_registry/README.rst — its Changelog section is generated from this file and currently has no entry for this change.
There was a problem hiding this comment.
All three fixed. Entry is now 19.0.2.2.4, above 19.0.2.2.3, so the descending order holds. spp_registry/manifest.py bumped to match. README.rst and static/description/index.html regenerated for this module and for spp_change_request_v2. I also added an upgrade note, because the constraint only validates on write: registrants already holding a future birthdate stay as they are until something writes that field, and that write then fails, possibly on an unrelated edit. The note carries a query to find them. Happy to add a migration that fixes the data instead if you'd rather it not be left to operators.
| } | ||
| } | ||
|
|
||
| @api.constrains("birthdate") |
There was a problem hiding this comment.
The constraint closes the partner-side hole, but spp_change_request_v2 stores a proposed birthdate on its own detail models with no matching guard, so the failure now lands at the worst moment.
spp_change_request_v2/details/create_group.py:369 (birthdate = fields.Date(...)), details/edit_individual.py:54 and wizards/create_group_member_wizard.py:179 all accept a future date; details/create_group.py:426-434 _compute_age even clamps the result to 0, so the UI shows nothing wrong. The value only reaches res.partner at apply time (strategies/add_member.py:35,50 and strategies/create_group.py:321), which is called unguarded from change_request.py:1521 (strategy.apply(sudo_self)).
Concrete scenario: a CR with a future DOB is submitted, reviewed and approved; on the final approve the res.partner.create raises ValidationError, the whole approval transaction rolls back, and the approver sees "Date of birth cannot be in the future." with no indication which CR field caused it. Before this PR the CR applied (badly, but successfully). Worth mirroring the check on the CR detail/wizard models so it is caught at data entry.
There was a problem hiding this comment.
Good catch and that failure mode was worse than the bug being fixed, I closed with a shared spp.cr.birthdate.mixin in spp_change_request_v2, inherited by the four models that store a proposed birthdate: spp.cr.detail.add_member, spp.cr.detail.edit_individual, spp.cr.detail.create_group.member_new and the create-group member wizard. It uses the same context_today comparison as the registry constraint, so the two guards can't disagree and let a value through one to be refused by the other at apply time. spp_change_request_v2 bumped to 19.0.3.1.15. I left _compute_age's max(..., 0) clamp alone, since it can no longer mask a future date.
| fires on every write path and keeps the non-stored ``age`` | ||
| compute from ever rendering a negative string. | ||
| """ | ||
| today = fields.Date.today() |
There was a problem hiding this comment.
fields.Date.today() is the server date (UTC in a normal deployment), not the users date. fields.Date.context_today(record) is the timezone-aware form Odoo uses for "not in the future" checks.
Concrete scenario: a registrar in Pacific/Auckland (UTC+13 during DST) at 10:00 local on 2 January is at 21:00 UTC on 1 January. Recording a newborn born that morning, birthdate = 2026-01-02 compares > the server today of 2026-01-01 and the write is refused with "Date of birth cannot be in the future." — for a date that is unambiguously in the past for that user. Any deployment east of UTC hits this for part of each day.
(The pre-existing _birthdate_onchange has the same flaw, but there it silently resets a field; here it is a hard failure on every write path including imports.)
There was a problem hiding this comment.
Fixed now 👍 - fields.Date.context_today(record), moved inside the loop since it needs the record to resolve the timezone. The tests derive their dates from context_today too, so they can't go flaky on a runner whose local date differs from UTC. Left the pre-existing onchange alone, as you noted it's a silent reset rather than a hard failure.
| today = fields.Date.today() | ||
| for record in self: | ||
| if record.birthdate and record.birthdate > today: | ||
| raise ValidationError(_("Date of birth cannot be in the future.")) |
There was a problem hiding this comment.
The message names neither the record nor the offending value, and _validate_fields does not decorate constraint errors with record info (odoo/models.py:1358-1367 just calls check(self)).
Concrete scenario: a 5,000-row CSV import of registrants, one of which has a typod birthdate. The whole batch aborts with the bare string "Date of birth cannot be in the future." and the operator has no way to find the bad row. Including the display name and the value — e.g. _("Date of birth cannot be in the future: %(name)s has %(date)s", name=record.display_name, date=record.birthdate)` — makes this actionable.
Separately, this new translatable string is not in spp_registry/i18n/spp_registry.pot (nor es.po/fr.po), which do carry the sibling onchange message ("You cant select a date of birth greater than today", pot line 1483) — the catalogs need regenerating.
There was a problem hiding this comment.
Message now reads "Date of birth cannot be in the future: %(name)s has %(date)s." with record.display_name and record.birthdate, so a bad row in a large import is findable. Added to spp_registry.pot, es.po and fr.po, plus the equivalent string for the change request mixin in that module's catalogs. A bit unrelated, but noticed while in there: the sibling onchange entry at spp_registry.pot:1483 is already stale on 19.0 it's missing the ". The field has been reset." the source string has. Left it alone rather than mix it into this PR.
| with self.assertRaises(ValidationError): | ||
| self.IDType.create({"name": False}) | ||
|
|
||
| @tagged("post_install", "-at_install") |
There was a problem hiding this comment.
Only one blank line before the class decorator; PEP 8 / ruff-format want two. .pre-commit-config.yaml:131-136 runs ruff with --exit-non-zero-on-fix plus ruff-format, so the pre-commit CI job will fail on this hunk until the extra blank line is added.
| @tagged("post_install", "-at_install") | |
| @tagged("post_install", "-at_install") |
There was a problem hiding this comment.
Fixed, two blank lines. ruff and ruff format both pass on the branch now.
…ot-future # Conflicts: # spp_registry/readme/HISTORY.md
Compare against fields.Date.context_today rather than the server's UTC date, so a registrar east of UTC is not refused a birth recorded earlier that local day, and name the record and the offending value in the error so a bad row in a bulk import can be found. De-vacuum the create and load tests. registration_date defaults to today and _check_registration_date refuses any registration_date earlier than the birthdate, which is true of every valid date once the birthdate is in the future — so both tests passed on the base branch without the new constraint existing. They now pass registration_date explicitly as empty and assert on the message. Test dates are based on context_today to match the constraint. Mirror the guard on the change request models. A proposed birthdate was stored unguarded on the Add Member, Edit Individual and Create Group detail models and the create-group member wizard, and was only refused when a strategy wrote it to res.partner on the final approval, rolling back the whole approval with an error the approver could not trace to a field. A shared spp.cr.birthdate.mixin applies the same rule at data entry. Bump spp_registry to 19.0.2.2.4 and spp_change_request_v2 to 19.0.3.1.15, add the changelog entries under the correct versions with an upgrade note about registrants already holding a future birthdate, and add the new strings to both modules' catalogs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #397 +/- ##
==========================================
- Coverage 76.92% 73.76% -3.17%
==========================================
Files 735 596 -139
Lines 47935 40131 -7804
==========================================
- Hits 36874 29602 -7272
+ Misses 11061 10529 -532 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Output of the oca-gen-addon-readme pre-commit hook for the new spp_registry 19.0.2.2.4 and spp_change_request_v2 19.0.3.1.15 entries.
Exercises spp.cr.birthdate.mixin on the edit_individual and add_member details and on the create_group new-member sub-model, including the today boundary that must pass.
|
Since the review: merged 19.0 (conflict cleared), and added tests for the new spp.cr.birthdate.mixin in spp_change_request_v2/tests/test_birthdate_mixin.py, closing the coverage gap Codecov flagged. CI: the earlier test (spp_api_v2_simulation) red was not a test failure that suite reported 0 failed, 0 error(s) of 178 tests and the job died in Upload coverage on a tokenless Codecov connection error from a fork. test-summary followed it. If it recurs it needs a re-run from someone with write access. Two open questions, both product calls: the second carried over from the description: Existing data. The constraint validates on write, so registrants already holding a future birthdate stay as they are until something writes that field, and that write then fails. Documented as an upgrade note with a query to find them, happy to write a migration instead if you'd rather the data were fixed than reported. Unrelated: .pre-commit-config.yaml sets manual: true on oca-gen-addon-readme and oca-gen-external-dependencies (lines 70, 72), but manual isn't a valid hook key pre-commit expects stages: [manual], warns, and ignores it. Both hooks run in the default stage, which is why any PR touching a readme/*.md fragment fails pre-commit until the regenerated files are committed. Happy to open a separate issue or PR, whichever you prefer. |
2 participants
Why is this change needed?
res.partner.birthdate accepts future dates through every path except the web form. The only guard, _birthdate_onchange, runs solely in the form UI, so ORM create/write, CSV/Excel import, and API writes (XML-RPC, API v2, DCI) all persist a future birthdate. The non-stored age compute then renders that as a negative string (e.g. "-3") in views, exports, and anything reading age. Fixes #362.
How was the change implemented?
Added a stored-field @api.constrains("birthdate") (_check_birthdate_not_future) in spp_registry/models/individual.py. Because birthdate is stored and writeable, the constraint fires on every write path and can't be bypassed. _birthdate_onchange is intentionally kept as the friendlier silent-reset UX in the form; the constraint is the server-side backstop.
New unit tests
Added TestBirthdateNotFutureConstraint in spp_registry/tests/test_constraints.py, covering the paths that bypass onchange — create, write, and load() (import) — plus boundary cases: today must pass, an ordinary past date must pass, and an approximate DOB (birthdate_not_exact) still can't be in the future.
Unit tests executed by the author
Ran via CI on this PR (no local environment available).
How to test manually
In an Odoo shell: env["res.partner"].create({"name": "Test", "is_registrant": True, "birthdate": date.today() + timedelta(days=1)}) should raise ValidationError. Setting birthdate to today or any past date should succeed.
Related links
Closes #362.
A couple of things I'd value your steer on, @gonzalesedwin1123:
Approximate birthdate I've assumed an approximate DOB (birthdate_not_exact) still can't be in the future, and there's a test asserting that. Flagging it as the product decision you noted in the issue in case you'd rather it be exempt.
Existing bad data, the constraint only validates on write, so any records already holding a future birthdate stay invalid until next touched, and would then block otherwise-unrelated writes. Happy to pair this with a data-quality check or a migration note, whichever you'd prefer. Also: the module README regeneration needs the repo toolchain, which I couldn't run locally. Should a maintainer run it, or does CI handle that step?