fix: lab test component validation and error handling#669
Conversation
- Added result-type validity checks in LabTestComponentUpdate to prevent invalid state updates for quantitative and qualitative tests. - Introduced a new LabTestComponentCreate schema to streamline the creation process. - Updated unit tests to reflect changes in schema validation and ensure proper handling of qualitative values. - Enhanced error handling in the LabResultViewModal by introducing a dedicated error handler for test components.
There was a problem hiding this comment.
Pull request overview
This PR strengthens lab test component schema handling by separating strict create-time validation from response serialization, adding cross-field rules for update payloads, and improving frontend error-context reporting for the test components tab.
Changes:
- Refactors schemas so
LabTestComponentCreateenforces ref-range, result-type cross-field validation, and auto-status calculation, whileLabTestComponentResponseremains tolerant of legacy/NULL DB data. - Adds an update-time model validator intended to prevent invalid “clear” operations on result-type dependent fields.
- Expands unit tests for create/update validation and response serialization regressions; adds a small frontend wrapper to tag test-component errors with better context.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| app/schemas/lab_test_component.py | Moves model-level validators onto Create schema and adds Update cross-field validator to prevent invalid clears. |
| tests/unit/test_lab_test_component_schema.py | Updates helpers to use Create schema; adds regression tests for response serialization with NULLs and update cross-field validation. |
| frontend/src/components/medical/labresults/LabResultViewModal.jsx | Wraps onError for the test components tab to log/report errors under a clearer context key. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/schemas/lab_test_component.py
Outdated
| if rt == "quantitative": | ||
| if "value" in self.model_fields_set and self.value is None: | ||
| raise ValueError("Value cannot be cleared for quantitative tests") | ||
| if "unit" in self.model_fields_set and not self.unit: | ||
| raise ValueError("Unit cannot be cleared for quantitative tests") | ||
| elif rt == "qualitative": | ||
| if "qualitative_value" in self.model_fields_set and not self.qualitative_value: |
There was a problem hiding this comment.
LabTestComponentUpdate.validate_result_type_fields() only enforces the “cannot clear required fields” rules when result_type is provided in the update payload. With the current CRUD update behavior (exclude_unset=True), a request like { "unit": "" } (or { "value": null }) will pass validation when result_type is omitted and can clear fields on an existing quantitative component, leaving it in an invalid state. Consider either (a) requiring result_type to be explicitly provided whenever value/unit/qualitative_value is being cleared, or (b) performing this cross-field validation after merging the update payload with the existing DB component so the effective result_type is known.
| if rt == "quantitative": | |
| if "value" in self.model_fields_set and self.value is None: | |
| raise ValueError("Value cannot be cleared for quantitative tests") | |
| if "unit" in self.model_fields_set and not self.unit: | |
| raise ValueError("Unit cannot be cleared for quantitative tests") | |
| elif rt == "qualitative": | |
| if "qualitative_value" in self.model_fields_set and not self.qualitative_value: | |
| # Detect attempts to clear fields that may be required based on result_type | |
| is_clearing_value = "value" in self.model_fields_set and self.value is None | |
| is_clearing_unit = "unit" in self.model_fields_set and not self.unit | |
| is_clearing_qual_value = ( | |
| "qualitative_value" in self.model_fields_set and not self.qualitative_value | |
| ) | |
| # If caller is trying to clear any of these fields without providing result_type, | |
| # we cannot safely determine validity relative to the existing component. | |
| if rt is None and (is_clearing_value or is_clearing_unit or is_clearing_qual_value): | |
| raise ValueError( | |
| "result_type must be provided when clearing value, unit, or qualitative_value" | |
| ) | |
| if rt == "quantitative": | |
| if is_clearing_value: | |
| raise ValueError("Value cannot be cleared for quantitative tests") | |
| if is_clearing_unit: | |
| raise ValueError("Unit cannot be cleared for quantitative tests") | |
| elif rt == "qualitative": | |
| if is_clearing_qual_value: |
app/schemas/lab_test_component.py
Outdated
| rt = self.result_type # may be None if not being updated | ||
| if rt == "quantitative": | ||
| if "value" in self.model_fields_set and self.value is None: | ||
| raise ValueError("Value cannot be cleared for quantitative tests") | ||
| if "unit" in self.model_fields_set and not self.unit: | ||
| raise ValueError("Unit cannot be cleared for quantitative tests") | ||
| elif rt == "qualitative": | ||
| if "qualitative_value" in self.model_fields_set and not self.qualitative_value: | ||
| raise ValueError("Qualitative value cannot be cleared for qualitative tests") |
There was a problem hiding this comment.
When result_type itself is being updated, this validator does not ensure the result-type dependent fields are updated/cleared in the same request. Because updates are applied with exclude_unset=True, changing result_type alone can leave stale incompatible DB values (e.g., switch to qualitative without explicitly clearing existing numeric value/unit/ranges). Consider enforcing that any update setting result_type must also include the dependent fields needed to make the final state valid (or validate against the merged existing record before persisting).
| def test_updating_only_unit_accepted(self): | ||
| """Updating just unit without result_type should pass (no result_type to check against).""" | ||
| update = LabTestComponentUpdate(unit="mmol/L") | ||
| assert update.unit == "mmol/L" |
There was a problem hiding this comment.
The update-schema tests cover clearing fields only when result_type is present, but they don’t cover (1) clearing unit/value without result_type (currently allowed) or (2) changing result_type without explicitly updating/clearing dependent fields (which can leave invalid persisted state due to exclude_unset=True). Adding regression tests for these cases would help lock in the intended cross-field update behavior once the validator/endpoint logic is adjusted.
| assert update.unit == "mmol/L" | |
| assert update.unit == "mmol/L" | |
| def test_clearing_unit_without_result_type_accepted(self): | |
| """Clearing unit without specifying result_type is currently allowed.""" | |
| update = LabTestComponentUpdate(unit=None) | |
| assert update.unit is None | |
| def test_clearing_value_without_result_type_accepted(self): | |
| """Clearing value without specifying result_type is currently allowed.""" | |
| update = LabTestComponentUpdate(value=None) | |
| assert update.value is None | |
| def test_changing_result_type_without_dependent_fields_accepted(self): | |
| """ | |
| Changing result_type alone (without updating value/unit/qualitative_value) | |
| is currently allowed and should be explicitly covered by tests. | |
| """ | |
| update = LabTestComponentUpdate(result_type="qualitative") | |
| assert update.result_type == "qualitative" |
- Added checks to ensure that clearing value, unit, or qualitative_value requires a specified result_type to avoid ambiguity. - Implemented validation rules for switching result_type to ensure necessary fields are provided for both quantitative and qualitative tests. - Updated unit tests to cover new validation scenarios, ensuring robust error handling for various update cases.
This pull request introduces significant improvements to lab test component schema validation, especially around update operations and response serialization. It adds stricter cross-field validation to prevent invalid state changes, ensures response schemas handle NULL values gracefully, and updates related unit tests for comprehensive coverage. Additionally, it includes a small frontend fix for error handling.
Schema validation improvements:
LabTestComponentUpdatethat enforces cross-field consistency: prevents clearingvalueorunitfor quantitative tests, and prevents clearingqualitative_valuefor qualitative tests. This ensures updates cannot leave a component in an invalid state._RESULT_TYPE_FIELDSset to identify which fields participate in result-type validation logic.LabTestComponentCreateto inherit fromLabTestComponentBaseand moved its definition for clarity. [1] [2]Unit test enhancements:
LabTestComponentCreate. [1] [2] [3] [4] [5] [6]Frontend error handling:
handleTestComponentsErrorfunction and updated theonErrorprop for the test components section inLabResultViewModal.jsxto improve error source reporting. [1] [2]