[16.0][FIX] Pre-existing bugs found during the 19.0 migration scan by wasud · Pull Request #166 · nirun-life/nirun

wasud · 2026-10-19T05:37:48Z

Closes #135

Summary (TH)

แก้บั๊กที่มีอยู่แล้วบน 16.0 ซึ่งเจอระหว่างสแกนเพื่อ migrate ไป 19.0 (7 ข้อใน #135) และบั๊กอีก 2 ข้อที่ pilot ใน #141 เจอ (finding 4 และ 5) แยก commit [FIX] ทีละ module เพื่อให้ forward-port ไป 17/18/19 ได้

Changes

#	Module	Fix
1	`ni_patient`	`check_origin_date` raises `ValidationError` (was a plain string), and compares the transfer date with the start date in the user's timezone (was Date vs Datetime, a `TypeError`)
2	`ni_patient`	The "Until" search filter uses `period_end`
3	`survey_subject`	"Answer Survey" button: `('active', '=', False)`, not the string `'False'`
4	`ni_observation`	`_compute_observation` uses `rec.id`, so several latest rows can be read together
5	`ni_observation`	`ni.observation.category.parent_id`: `ondelete="set null"` (was `""`). The column already uses `ON DELETE SET NULL`, so the database does not change
6	`ni_questionnaire`	`compute_sudo=True` (the typo `sudo_compute` was ignored) on `response_count`
7	`partner_age`	Removed `tests/test_res_partner.py`: never imported, and a near copy of `test_res_partner_age.py`
F4 (#141)	`ni_patient`	"Error Entry" no longer archives the patient: `ni.encounter` has no `active` of its own and wrote `ni.patient.active` through `_inherits`. The state already marks the encounter inactive
F5 (#141)	`ni_period`	`period_start_date` has no default any more, so a record created from code gets the date of its `period_start`, not today
F5 data	`ni_period`	`migrations/16.0.0.1.1/end-migration.py` repairs the start dates the old default wrote: datetime-mode rows whose `period_start_date` differs from `period_start` and equals the creation day (± 1 day). Date-mode rows, SQL views and models with their own `period_start_date` are skipped
F4 data	`.tools`	`error_entry_archived_patients.sql`: read-only list of the patients Error Entry may have archived, for support to review and reactivate from the UI. The output holds patient identifiers and stays in support's own channel
–	`.tools`	`migration_sample_data_p1.py` no longer sets `period_start_date` by hand

Versions bumped (patch): ni_patient 16.0.0.4.1, ni_period 16.0.0.1.1, survey_subject 16.0.0.3.2, ni_observation 16.0.0.6.2, ni_questionnaire 16.0.0.5.1.

Decisions to review

[13.0][ADD] nirun_patient_group_survey: Add patient to group base on survey result #6 changes behaviour: response_count now counts every completed response of the patient/encounter, also those a user cannot read through record rules. This is what the code meant, and matches the other response computes on these models (compute_sudo=True). The list the stat button opens still applies the record rules, so a restricted user can see a count larger than the list.
F4: Dropping active from the write is the minimal fix. Giving ni.encounter its own active field would also stop the encounters of archived patients from being hidden by default, which is a larger change.
F4 data: no automatic reactivation. Support reviews the list from .tools/error_entry_archived_patients.sql and reactivates the confirmed patients from the UI, so the change is tracked in the chatter.
F5: The tests are in ni_period, on a fake model loaded with odoo_test_helper.FakeModelLoader: the create case, and the upgrade script on a fixed row, a date-mode row, a correct row and a model with its own field.
Not here: pilot finding 3 (portal templates use ni_practitioner's performer_id) is fixed with the ni_practitioner migration, as planned in [MIG 19.0][P1-6] Data migration pilot: OpenUpgrade 16→17→18→19 on Phase 1 modules #141.

How tested

Fresh 16.0 DB, ni_period, partner_age, ni_patient, survey_subject, ni_observation, ni_questionnaire: 0 failed, 0 errors of 101 tests.
Real upgrade: a sample database set back to ni_period 16.0.0.1.0 with corrupted rows; -u ni_period fixed 3 ni.encounter and 2 ni.encounter.participant rows and left the date-mode row alone.
The new tests fail without the fixes (Error Entry, start date, multi-row compute: 2 failed, 1 error).
pre-commit clean on the changed files.

Rollout

OCA keeps only the current version's migration scripts, so the F5 data fix exists on 16.0 only. Before the OpenUpgrade 16→17 step, production must:

have support review the F4 list and reactivate the confirmed patients from the UI;
run -u ni_period on 16.0 with this version, with a migration_snapshot.py snapshot before and after.

Both steps go into the P6 cutover runbook.

Forward-port

After merge, forward-port to 17.0/18.0/19.0 with oca-port (or replay into the open P1 stacks #153–#161 when they rebase). The 17/18/19 replays of the check_origin_date fix (row 1) are ready locally. #167 (the same fix on its own) was closed as a duplicate, so this PR stays the only 16.0 source for it.

piruin

Review

The fixes are correct. Superseded by the follow-up review: F5 needs a data-fix script and an upgrade test before merge.

Data left behind by the old bugs (decide before cutover)

F4, decision: no automatic reactivation; support reviews the list by hand. Since b25bb607 (2023-12-20), "Error Entry" archived the patient, and through _inherits that also hid all of the patient's encounters. This PR stops new cases, but existing databases still have patients archived this way.
- Patients archived through the archive wizard always have a state_reason_id (the wizard requires it), so the likely victims are archived patients with no state_reason_id and at least one encounter in entered-in-error.
- Please provide a read-only query (e.g. in .tools/) that lists them for support:
```
SELECT p.id, p.identifier
FROM ni_patient p
WHERE p.active IS NOT TRUE
  AND p.state_reason_id IS NULL
  AND EXISTS (
      SELECT 1 FROM ni_encounter e
      WHERE e.patient_id = p.id AND e.state = 'entered-in-error'
  )
```
- Support reactivates the confirmed ones from the UI, so the change is tracked in the chatter. The list holds patient identifiers: share it only through support's own channel, never in an issue or PR.
- Add the step to the P6 cutover runbook, before the OpenUpgrade run.
F5: records created from code have a stored period_start_date equal to the day they were created. A data-fix script with an upgrade test is required; see the follow-up review.

Minor

.tools/migration_sample_data_p1.py:191: the comment says the mixin's default wins over the compute, and the code sets period_start_date itself to work around that. After F5 the comment is wrong and the workaround isn't needed.
The F5 test should move into its own file next to the new upgrade test; see the follow-up review.
#6 (compute_sudo): no confirmation needed. response_count is deprecated, and its replacement will be migrated into 16.0.

Checked, OK

check_origin_date:
- It now raises ValidationError instead of a plain string.
- It handles an empty period_start.
- It compares the transfer date with the start date in the user's timezone (context_timestamp).
- The Bangkok test covers the case where the UTC date and the local date differ.
The "Until" filter now uses period_end.
The survey_subject button compares active with False, not the string 'False'.
_compute_observation reads each row's own record (rec.id), and the new test reads several rows at once.
ondelete="set null" matches the existing FK, and the compute_sudo typo is fixed.
The deleted partner_age test file was never imported, and test_res_partner_age.py has the same six tests.
Removing the period_start_date default doesn't change the form: period_start still defaults to now. ni_condition already set default=None itself.
There is one [FIX] commit per module, with patch versions bumped, so oca-port can carry them forward. CI is green.

piruin

Changes requested: F5 needs a data fix and an upgrade test

The ni_period fix stops new wrong values, but existing databases still store period_start_date = the day the record was created for every record created from code (imports, RPC, bulk wizards) since the default was added. Please ship the cleanup with the version bump.

Script: ni_period/migrations/16.0.0.1.1/end-migration.py

Use an end- script, not post-. ni_period has no dependencies, so at post- time none of the models that use the mixin are loaded yet.
Loop over the stored models that inherit the mixin: env.registry.descendants(["ni.period.mixin"], "_inherit").
- Skip _auto = False models (e.g. ni.condition.latest).
- Skip models that define their own period_start_date. ni.condition already had default=None, so the bug never affected it.
Fix only the rows the bug wrote:
- period_mode = 'datetime', period_start IS NOT NULL and period_start_date <> period_start::date;
- and period_start_date is the creation day. context_today used the creating user's timezone, so allow create_date::date ± 1 day.
Leave date-mode rows alone. There, _inverse_period_start_date does not touch period_start when the user edits the date, so a mismatch can be intended.
Set period_start_date = period_start::date, the same UTC date _compute_period_start_date produces, and log the number of rows fixed per model.

Tests

Put them in ni_period itself, using a test model loaded with odoo_test_helper.FakeModelLoader (it's already in test-requirements.txt). Fall back to ni_patient only if that doesn't work.
Move the create test (test_period_start_date_follows_period_start_on_create) out of test_ni_encounter_entered_in_error.py and into this file.
Upgrade test: corrupt rows with SQL, run the script, then check the result.
- The migrations folder name contains dots, so load the script with importlib.util.spec_from_file_location and call migrate(env.cr, "16.0.0.1.0").
- Cover four kinds of row:
  1. a datetime-mode row with period_start_date = the creation day is fixed;
  2. a date-mode row with a deliberate mismatch is left alone;
  3. a row that is already correct is left alone;
  4. a model that defines its own field is skipped.

Rollout

OCA keeps only the current version's scripts, so this exists on 16.0 only. Production must run -u ni_period on 16.0 with this version before the OpenUpgrade 16→17 step. Please add that to the P6 cutover runbook.
Take a migration_snapshot.py snapshot before and after that update, so the change in the data is recorded.

piruin · 2026-10-21T01:23:05Z

#167 (the same check_origin_date fix on its own) is closed as a duplicate, so this PR stays the only 16.0 source for it. Please keep row 1 and its forward-port in this PR's plan.

period_start_date is a stored computed field with a default of today. On create, a default counts as a given value, so the compute never runs: records created from code (import, RPC, bulk wizards) with only period_start got today as their start date, and an encounter could then fail the end-date check when it was closed. Drop the default. period_start already defaults to now, so a new record in the form still starts today. Existing databases still hold the wrong dates, so the version bump ships an end-migration script that repairs them. It runs at end- time because ni_period has no dependencies, and goes through every stored model on the mixin, skipping SQL views and models that declare their own period_start_date (ni.condition already had default=None). It only fixes datetime-mode rows whose start date differs from period_start and equals their creation day (+/- 1 day for the creating user's timezone); date-mode rows are left alone, since there a mismatch can be intended. Each row gets period_start::date, as the compute does, and the number of fixed rows is logged per model. The tests use a fake model on the mixin: the create case, and the script on a fixed row, a date-mode row, a correct row and a model with its own field. Production must run `-u ni_period` on 16.0 with this version before the OpenUpgrade 16 -> 17 step: OCA keeps only the current version's scripts. Refs: nirun-life#135

tests/test_res_partner.py was not imported in tests/__init__.py, so it never ran. It is a near copy of test_res_partner_age.py (the same cases, two assertions in a different order), so there is nothing to merge. Refs: nirun-life#135

The button's invisible condition compared active with the string 'False', which never matches a boolean, so the button also showed on archived surveys. Compare with False. Refs: nirun-life#135

…ondelete ni.observation.report._compute_observation browsed self.id inside the loop, so reading observation_id or the history of several latest rows at once (a list, an export) raised "Expected singleton". Use rec.id. The test reads two rows of the patient's latest observations together. ni.observation.category.parent_id had ondelete="", which is not a valid value. Set "set null", Odoo's default for a nullable Many2one and what the column already used, so the database does not change. Refs: nirun-life#135

response_count on ni.patient and ni.encounter passed sudo_compute=True, which is not a field parameter and was ignored. Use compute_sudo=True, as intended. The counts now include completed responses the user cannot read through record rules, the same as the other stored response computes of these models. Refs: nirun-life#135

…ntry check_origin_date raised the translated message itself, a plain string, so setting a transfer date without a transfer source failed with "exceptions must derive from BaseException" instead of a validation error. It also compared the transfer date (a Date) with the encounter start (a Datetime), which fails with a TypeError on every encounter that has both, and on encounters without a start. Raise ValidationError, and compare the transfer date with the start date in the user's timezone, skipping the check when the encounter has no start. The encounter search filter "Until" filtered on period_start; use period_end. action_entered_in_error() wrote active=False. ni.encounter has no active field of its own and inherits ni.patient.active through _inherits, so marking an encounter as an error entry archived the patient. The state alone already marks the encounter inactive (ENCOUNTER_INACTIVE_STATE). Tests cover the transfer date and Error Entry. Refs: nirun-life#135

…orkaround Add a read-only query that lists the patients "Error Entry" may have archived by mistake (archived, no state reason, an entered-in-error encounter). Support reviews the list and reactivates the confirmed patients from the UI; nothing is reactivated automatically. The output holds patient identifiers, so it stays in support's own channel. The pilot sample data no longer sets period_start_date by hand: since the ni_period fix the compute gives the date of period_start on create. Refs: nirun-life#135

wasud · 2026-10-21T01:48:54Z

Thanks for the review. Pushed (head 2e2b24c7); still one [FIX] commit per module.

F5, data fix and upgrade test (in [FIX] ni_period: compute the start date from period_start on create)

ni_period/migrations/16.0.0.1.1/end-migration.py (an end- script) loops over env.registry.descendants(["ni.period.mixin"], "_inherit"). It skips abstract models, _auto = False models (ni.condition.latest) and models that declare their own period_start_date (ni.condition).
It fixes only datetime-mode rows with period_start IS NOT NULL, period_start_date <> period_start::date and period_start_date between create_date::date - 1 and create_date::date + 1. It sets period_start_date = period_start::date and logs the rows fixed per model. Date-mode rows are left alone.
Tests in ni_period/tests/test_ni_period_start_date.py use a fake model loaded with FakeModelLoader:
- The create test moved here from test_ni_encounter_entered_in_error.py.
- The upgrade test corrupts rows with SQL, loads the script with importlib.util.spec_from_file_location, calls migrate(env.cr, "16.0.0.1.0") and checks the four cases plus the log line.
- A last test checks that a fresh install (version=None) does nothing.
Checked on a real install too: on a sample database set back to 16.0.0.1.0 with corrupted rows, -u ni_period fixed 3 ni.encounter and 2 ni.encounter.participant rows and left the date-mode row alone. Only ni.encounter, ni.encounter.location and ni.encounter.participant are picked up; ni.condition and ni.condition.latest are skipped.

F4, Error Entry victims (new commit [IMP] tools: list patients archived by Error Entry, drop start-date workaround)

.tools/error_entry_archived_patients.sql is your read-only query. Its header says support reactivates the confirmed patients from the UI and that the output stays in support's own channel.

Minor

.tools/migration_sample_data_p1.py: the wrong comment and the period_start_date workarounds are removed (same commit as F4). The sample data now gets the right start dates from the compute: 0 mismatches over the encounter, location and participant rows.

Rollout

Added to the P6-1 cutover runbook draft, before the OpenUpgrade run:
- the F4 review by support;
- -u ni_period on 16.0 with a migration_snapshot.py snapshot before and after.
[16.0][FIX] ni_patient: validate encounter transfer date without a TypeError #167: noted. Row 1 (check_origin_date) and its forward-port stay in this PR's plan.

Tests: 101 pass on a fresh 16.0 database (ni_period, partner_age, survey_subject, ni_observation, ni_questionnaire, ni_patient); pre-commit passes.

piruin

LGTM

piruin approved these changes Oct 21, 2026

View reviewed changes

piruin requested changes Oct 21, 2026

View reviewed changes

piruin mentioned this pull request Oct 21, 2026

[16.0][FIX] ni_patient: validate encounter transfer date without a TypeError #167

Closed

2 tasks done

wasud added 7 commits October 21, 2026 08:46

[FIX] partner_age: remove a test file that never ran

095d84b

tests/test_res_partner.py was not imported in tests/__init__.py, so it never ran. It is a near copy of test_res_partner_age.py (the same cases, two assertions in a different order), so there is nothing to merge. Refs: nirun-life#135

[FIX] survey_subject: show "Answer Survey" on active surveys only

d24c7cc

The button's invisible condition compared active with the string 'False', which never matches a boolean, so the button also showed on archived surveys. Compare with False. Refs: nirun-life#135

wasud force-pushed the 16.0-fix-p0-4 branch from f6b707d to 2e2b24c Compare October 21, 2026 01:48

wasud mentioned this pull request Oct 21, 2026

[17.0][MIG] ni_questionnaire: Migration to 17.0 #159

Merged

4 tasks done

piruin approved these changes Oct 21, 2026

View reviewed changes

piruin merged commit f2deb83 into nirun-life:16.0 Oct 21, 2026
4 checks passed

This was referenced Oct 29, 2026

[MIG 19.0][P1-8] Forward-port the 16.0 fixes merged after the Phase 1 stacks to 17.0/18.0/19.0 #181

Closed

[Epic] Migrate Nirun to Odoo 19.0 (OCA way) #131

Open

Navigation Menu

Repository navigation

[16.0][FIX] Pre-existing bugs found during the 19.0 migration scan - #166

[16.0][FIX] Pre-existing bugs found during the 19.0 migration scan#166
piruin merged 7 commits into
nirun-life:16.0from
wasud:16.0-fix-p0-4

wasud commented Oct 19, 2026 •

edited

Loading

Uh oh!

piruin left a comment •

edited

Loading

Uh oh!

piruin left a comment

Uh oh!

piruin commented Oct 21, 2026

Uh oh!

wasud commented Oct 21, 2026

Uh oh!

piruin left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

wasud commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary (TH)

Changes

Decisions to review

How tested

Rollout

Forward-port

Uh oh!

piruin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review

Uh oh!

piruin left a comment

Choose a reason for hiding this comment

Changes requested: F5 needs a data fix and an upgrade test

Uh oh!

piruin commented Oct 21, 2026

Uh oh!

wasud commented Oct 21, 2026

Uh oh!

piruin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wasud commented Oct 19, 2026 •

edited

Loading

piruin left a comment •

edited

Loading