Repository navigation
[19.0][MIG] ni_questionnaire: Migration to 19.0 - #161
Conversation
1cc2e68 to
3e24376
Compare
3e24376 to
dee257f
Compare
3e52c6d to
679411e
Compare
piruin
left a comment
piruin
left a comment
There was a problem hiding this comment.
Review of ni_questionnaire only (the patient/observation/survey commits belong to the stacked PRs). The findings marked "all three" on #159 also apply here; repeated below for completeness.
Medium — subject wizard now shows the patient/encounter pickers that 16.0 hid (wizard/survey_subject_views.xml)
On 16.0, subject_ni_patient / subject_ni_encounter had invisible="context.get('default_subject_ni_…')" next to attrs. A truthy context value overrode the attrs domain (16.0 core ir_ui_view.py, context-dependent modifiers). The 17.0 conversion kept only type not in [...], so opening the wizard from an encounter or observation ("Start Survey") now shows editable patient/encounter fields. Suggested:
invisible="context.get('default_subject_ni_patient') or type not in ['ni.patient', 'ni.encounter']"(and the same with default_subject_ni_encounter for subject_ni_encounter). This is the only element in the module that had both.
Low — the latest-response view and compute break ties differently (report/ni_patient_survey_latest.py)
The survey_latest views order by create_date DESC with no tie-breaker, while ni.encounter._compute_response sorts by (create_date, id). If two done responses share a create_date (e.g. created in one transaction), response_latest_id can point to a row the view's DISTINCT ON dropped. Adding , id DESC to the view's ORDER BY aligns them (and matches the index, which already includes id).
Nit — the error branch of _answer_group_observation raises AttributeError (models/survey_user_input.py)
code is group.observation_code_id, already an ni.observation.type, which has no observation_type_id. So code.observation_type_id.name / .value_type crash instead of raising the ValidationError. Mostly unreachable thanks to the field domain, but these lines were just rewritten; use code.name / code.value_type.
Note (non-blocking) — creating the answer with user=env.user counts attempts against the clinician's partner, not the patient's. On a survey with limited attempts, a clinician runs out across patients. The survey_subject wizard behaves the same way.
Low — 17.0 commit 0843dfac is missing from this stack
#159's last commit ([FIX] ni_questionnaire: recompute the encounters' latest response on upgrade: the 17.0 post-migration script and the README "Upgrading from 16.0" section) is not in the replayed 17.0 history here. Upgrades still work (16→17→18→19 runs the script from 17.0 code), but the README here doesn't mention the step. Decide whether this branch should carry it.
Checked and fine: self.env._, group_ids, env.cr, the named placeholder in the deceased-patient warning. The encounter _create_answer access issue from #159 does not apply on 19.0 (officers may create responses on unrestricted surveys).
de6e944 to
56baea5
Compare
|
Thanks for the review. Pushed (head
Tests: 102 pass on a fresh 19.0 database; pre-commit passes. |
…response smart button
… write issue) Took 24 seconds
Clinical instruments such as MMSE are not scored out of a fixed total: a respondent with no schooling is not given three of the items, so the test is scored out of 23 instead of 30 and the normal/abnormal cutoff moves with education level, at 14, 17 or 22 points. Core already shrinks the maximum, since _mark_done() drops the inactive conditional questions from predefined_question_ids. What was missing is everything downstream, so survey.grade gains two things: a points basis, because a fixed points cutoff stops being a fixed percentage once the total moves, and triggering_answer_ids, so one survey can hold several sets of ranges keyed on the answer that decided which items were asked. Those ranges have to overlap numerically, so the unique and overlap rules move from _sql_constraints to Python keyed on _scope_key() - a Many2many cannot take part in a SQL unique index. Observations are built in _mark_done() after super(). The old write() hook ran before core trimmed the denominator and so recorded the pre-trim percentage. survey.grade.interpretation_id lets the observation follow the grade rather than ni.observation.reference.range, which scopes only by age and gender and therefore cannot read a total whose maximum varies. grading_basis, observation_answer_type and observation_score_type are deliberately not required: a NOT NULL column on survey.survey or survey.question breaks the tests of every module upstream of these two, which run before either is in the registry. Also corrects the value-mapped answer branch, which dispatched on truthiness: a numerical answer of 0 was taken for "no answer given" and datetime had no branch at all, both of which blocked survey completion. The tests are modelled on MMSE-Thai 2002 itself so the shrinking maximum and the three cutoffs are checked against a real instrument. Stock Odoo 16 gates a conditional question on a single answer, so the skipped items hang off a binary "Attended school?" question while the cutoff bands hang off a separate three-level "Education level" question. Grade UI follows: triggering answers read "Question: Answer" since many surveys reuse "Yes"/"No" across questions; a scoped grade is flagged in the tree with the fa-code-fork icon the question tree already uses, with the answers themselves on a "Conditional" tab of the grade form; and the patient scoping (gender, age, interpretation) is reachable from the grade form too, gender drawn as a mars/venus icon in the tree. Both modules are committed together because the change is not separable - ni_questionnaire overrides the very constraint and grade lookup that survey_grading reworks, so neither half passes its own tests alone. Took 3 minutes Took 8 minutes
Core gates the entire score block of the completion page on scoring_success_min, but a graded questionnaire has no meaningful passing percentage - a points-based instrument such as ADL leaves it at 0. The badge sat inside that block and so never reached the page. Widening the gate to `scoring_success_min or grade_ids` is preferred over moving the badge out of the block, because survey_subject rewrites the same column through a positional t[1]/div xpath: inserting any element ahead of that target breaks it with "cannot be located in parent view". It also brings back survey_subject's own score line, which shared the gate. Passed/failed stays tied to scoring_success_min. Without one, scoring_success is 0 >= 0 and every respondent would be congratulated, and since a bare t-else always fires, the failure branch has to become a t-elif on the same condition. Both verdict specs address direct children of the scoring block rather than matching t-else anywhere in the template. survey_subject carries a t-else of its own and the apply order between the two modules follows install order, so an unscoped match rewrites the wrong node - silently, leaving a broken score line and an ungated verdict. The guard for that combination lives in ni_questionnaire, the only module that depends on both. Took 49 minutes
grade_for() narrows the bands by the patient's gender and age, which is invisible to whoever reads the result: two patients with the same score can hold different grades with nothing on screen to explain it. Extend grade_conditions() so the grading reference says which traits applied. Only traits the survey actually grades differently by are named - a gender when any band carries one, an age when the bands fall into more than one window. Printing them unconditionally would put an age and a gender on every result page that had no bearing on the grade. Took 48 minutes
…upgrade 16.0 computed the encounter's latest response from a SQL view before the response reached the database, so the stored response_latest_id, its grade and date are empty or stale in 16.0 databases. 17.0 computes them from the responses, but only when a response changes. Recompute them once in the 17.0 post-migration script, so that migrated encounters show their latest grade and the "Print Questionnaire" button. Found by the Phase 1 data migration pilot (nirun-life#141).
56baea5 to
dbc4712
Compare
Decisions to review
security/res_groups.xmlonly addssurvey.group_survey_usertoni_patient.group_viewer's implied groups, which works unchanged with the 19 privileges (P1-2 [MIG 19.0][P1-2] ni_patient #137).security/ir_rules.xmlare global and unchanged.group_ids(migrator).ni_questionnaire.What changed
[IMP]pre-commit (no changes) and[MIG] ni_questionnaire: Migration to 19.0on top of the 18.0 PR._cr/_context→env.cr/env.context, menugroups_id→group_ids, version.self.env._(...)everywhere, with a named placeholder for the deceased-patient warning._mark_done(),_create_answer(),action_start_survey(answer=)/action_print_survey(answer=),_description_selection.answer_score/answer_is_correctare computed but still stored, so the monthly report still reads them as columns.th.poregenerated, 18.0 translations merged.Why
Closes #140 (P1-5, epic #131).
Migration notes
env.cr/env.context,group_ids.self.env._, lint.How tested
pre-commitclean onni_questionnaire.Documentation
README.md(19.0)Notes for reviewer
Rebased on the updated [19.0][MIG] ni_observation: Migration to 19.0 #158 (new
[FIX] ni_observationcommit from the P1-6 pilot, [MIG 19.0][P1-6] Data migration pilot: OpenUpgrade 16→17→18→19 on Phase 1 modules #141). No change in this module.Stacked on [19.0][MIG] ni_observation: Migration to 19.0 #158 (P1-4 19.0) and [19.0][MIG] survey_subject, survey_grading: Migration to 19.0 #152 (P1-3 19.0), and on [18.0][MIG] ni_questionnaire: Migration to 18.0 #160 (18.0). Merge those first.