care icon indicating copy to clipboard operation
care copied to clipboard

fix:added field validator for onset datatime in allergy intolerance

Open nandkishorr opened this issue 5 months ago • 6 comments

Proposed Changes

  • Added a field validator for onset date time

Merge Checklist

  • [ ] Tests added/fixed
  • [ ] Update docs in /docs
  • [ ] Linting Complete
  • [ ] Any other necessary step

Only PR's with test cases included and passing lint and test pipelines will be reviewed

@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins

Summary by CodeRabbit

  • Bug Fixes
    • Improved date validation for allergy, condition, and medication records: onset/authored dates are normalized for timezone and cannot be set in the future. Users will see clear errors when entering invalid dates.
  • Tests
    • Updated test expectations to reflect a standardized error message: "Date cannot be in the future."

nandkishorr avatar Jul 07 '25 07:07 nandkishorr

📝 Walkthrough

Walkthrough

Added a shared datetime validator and wired it into allergy, condition, and medication-request specs to centralize timezone-awareness and future-date checks; tests updated to expect the new generic future-date error message.

Changes

Cohort / File(s) Change Summary
Common validator
care/emr/resources/common/validators.py
Added validate_datetime(datetime: datetime.datetime) to enforce timezone-awareness and prevent future datetimes.
Allergy onset validator
care/emr/resources/allergy_intolerance/spec.py
Added @field_validator("onset_datetime") validate_onset_datetime that delegates to validate_datetime.
Condition onset validator
care/emr/resources/condition/spec.py
Replaced manual timezone/future checks with a call to validate_datetime; removed unused imports and dropped the unused info parameter.
Medication authored_on validator
care/emr/resources/medication/request/spec.py
Added @field_validator("authored_on") validate_authored_on that delegates to validate_datetime.
Tests
care/emr/tests/test_diagnosis_api.py, care/emr/tests/test_symptom_api.py
Updated assertions to expect the more generic "Date cannot be in the future" error message.

Sequence Diagram(s)

sequenceDiagram
    participant User as User
    participant Spec as SpecClass
    participant Validator as CommonValidator

    Note over Spec,Validator: New centralized datetime validation flow
    User->>Spec: submit onset_datetime / authored_on
    Spec->>Validator: validate_datetime(datetime)
    Validator->>Validator: normalize timezone (make aware if needed)
    Validator->>Validator: check not in future (care_now())
    alt invalid (future)
        Validator-->>Spec: raise ValueError ("Date cannot be in the future")
    else valid
        Validator-->>Spec: return validated datetime
    end
    Spec-->>User: accept or reject input

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • ohcnetwork/care#2915 — touches the same Condition onset_datetime validator refactor; likely overlapping validation changes.
  • ohcnetwork/care#2745 — introduces/changes authored_on handling for MedicationRequest, related to the new authored_on validator.

Suggested reviewers

  • vigneshhari
  • praffq

Poem

A datetime once wandered, lax and loose,
Herded now through one exacting no-nonsense sluice.
Timezones made proper, futures firmly barred,
One validator stands — tidy, slightly hard. ⏰✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description includes the Proposed Changes and Merge Checklist sections as required, but it omits the mandatory Associated Issue section from the template, leaving reviewers without context for which issue this PR addresses. The Architecture changes section can be removed if unused, but the missing issue link is a critical gap. Without this information the template is incomplete and reviewers may struggle to verify the related issue. Please add an Associated Issue section with a link to the relevant issue and a brief explanation of how this PR resolves it so that reviewers have the necessary context.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly indicates that a field validator has been added for the onset datetime in the allergy intolerance context, which is a genuine aspect of the changeset, and it is not overly vague or unrelated. It succinctly describes a substantive change rather than using generic phrasing, so it meets the requirement for being related to the main change. While there are broader changes in other specs and tests, the title need not enumerate every detail.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch nandkishorr/fix/allergy-intolerence/onset-validation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Jul 07 '25 07:07 coderabbitai[bot]

Codecov Report

:x: Patch coverage is 50.00000% with 14 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 53.87%. Comparing base (fefde08) to head (7c61bfa). :warning: Report is 17 commits behind head on develop.

Files with missing lines Patch % Lines
care/emr/resources/common/validators.py 40.00% 6 Missing :warning:
care/emr/resources/allergy_intolerance/spec.py 57.14% 3 Missing :warning:
care/emr/resources/medication/request/spec.py 57.14% 3 Missing :warning:
care/emr/resources/condition/spec.py 50.00% 2 Missing :warning:
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3120   +/-   ##
========================================
  Coverage    53.87%   53.87%           
========================================
  Files          434      435    +1     
  Lines        19683    19703   +20     
  Branches      2129     2132    +3     
========================================
+ Hits         10604    10615   +11     
- Misses        9051     9060    +9     
  Partials        28       28           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Jul 07 '25 07:07 codecov[bot]

^ Resolve the comments

vigneshhari avatar Jul 07 '25 18:07 vigneshhari

tests failing , please check @nandkishorr

praffq avatar Jul 13 '25 16:07 praffq

This is not just validation, we are also adding timezone data to data without timezone, was that intended?

vigneshhari avatar Oct 06 '25 03:10 vigneshhari

This is not just validation, we are also adding timezone data to data without timezone, was that intended?

Its the same behavior as earlier, just refactored to be used in multiple places

We can reject dates without tz but that would make the frontend unhappy

sainak avatar Nov 19 '25 08:11 sainak