care icon indicating copy to clipboard operation
care copied to clipboard

adding validation in evaluator

Open praffq opened this issue 6 months ago • 1 comments

Proposed Changes

Fixes https://github.com/ohcnetwork/roadmap/issues/137

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

  • New Features

    • Added operation-specific validation for evaluation rules, covering age (equality and range), gender (equality), and encounter tags.
    • Provides clearer, user-friendly error messages for invalid inputs (e.g., non-numeric ages, invalid ranges, empty gender, malformed IDs).
    • Extensible validation framework enables consistent checks across metrics.
  • Bug Fixes

    • Prevents misconfiguration-related errors by blocking invalid rule values before evaluation.
    • Improves stability and predictability when setting up evaluation criteria.

praffq avatar Sep 26 '25 08:09 praffq

📝 Walkthrough

Walkthrough

Adds operation-specific validation to evaluation metrics via a class-level validators mapping in the base class. Subclasses wire appropriate validators for age, gender, and encounter tag metrics. New validator functions are introduced to enforce type/format constraints, and the base validation flow optionally invokes them.

Changes

Cohort / File(s) Change Summary
Base validation hook
care/utils/evaluators/evaluation_metric/base.py
Adds class-level validators = {}; validate_rule now optionally calls an operation-specific validator if present after operation verification.
Metric-specific validator wiring
care/utils/evaluators/evaluation_metric/encounter_tag.py, care/utils/evaluators/evaluation_metric/patient_age.py, care/utils/evaluators/evaluation_metric/patient_gender.py
Imports validator functions and defines validators dicts in each metric class mapping AllowedOperations.*.value to corresponding validator. No method signature changes.
Validator implementations
care/utils/evaluators/evaluation_metric/validators.py
Adds validate_patient_age_equality, validate_patient_age_in_range, validate_patient_gender_equality, validate_encounter_tag_has_tag with type/format checks (including UUID4).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Metric as MetricSubclass
  participant Base as EvaluationMetricBase
  participant V as validators.py

  Caller->>Metric: evaluate(rule)
  Metric->>Base: validate_rule(rule)
  Base->>Base: verify operation in AllowedOperations
  alt validator mapped
    Base->>V: call validator(value)
    V-->>Base: ok or raise ValueError
  else no validator
    Note over Base: Skip operation-specific validation
  end
  Base-->>Metric: validation result
  Metric-->>Caller: proceed or error

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

A guard at the gate, quiet and keen,
Validators whisper, “keep inputs clean.”
Age, gender, tags line up in queue—
UUIDs checked, and ranges too.
The base nods once, “you may proceed,”
Rules now behave, with far less need to plead.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description remains the untouched template with placeholder text and provides no actual details on the validation additions, related issue tracking, architecture notes, or completed merge checklist items, so it leaves reviewers guessing at the actual implementation. Please fill in each section with a concise summary of the validation features added, link and explain the associated issue, remove or update the architecture changes section as needed, and mark off completed tests, documentation, and linting steps in the merge checklist.
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 pull request title “adding validation in evaluator” succinctly captures the primary change of introducing validation logic into the evaluator components, so it accurately reflects the core update.
✨ Finishing touches
  • [ ] 📝 Generate Docstrings
🧪 Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch prafful/adding-validation-in-evaluator

[!TIP]

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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

coderabbitai[bot] avatar Sep 26 '25 08:09 coderabbitai[bot]