added a dynamic valueset type #2945
Conversation
📝 WalkthroughWalkthroughThis pull request updates several specifications in the medical records module by replacing the generic Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Definitions (1)care/emr/resources/medication/administration/spec.py (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
care/emr/utils/valueset_coding_type.py (1)
1-20: Nice implementation of a parameterized type for value set bound coding.This is a well-designed implementation that uses the
__class_getitem__method to create a specialized type bound to a specific value set. This approach significantly improves type safety and validation compared to the previous approach of using separate field validators.A few minor suggestions that might make this even better:
from care.emr.registries.care_valueset.care_valueset import validate_valueset from care.emr.resources.common import Coding class ValueSetBoundCoding: + """ + A parametrized type that creates a Coding class bound to a specific value set. + + Usage: + field: ValueSetBoundCoding[SOME_VALUESET.slug] + """ @classmethod def __class_getitem__(cls, slug: str): class BoundCoding(Coding): @classmethod def __get_validators__(cls): yield cls.validate_input @classmethod def validate_input(cls, v): if isinstance(v, dict): v = Coding.model_validate(v) return validate_valueset("code", slug, v) return BoundCoding
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
care/emr/resources/allergy_intolerance/spec.py(2 hunks)care/emr/resources/condition/spec.py(3 hunks)care/emr/resources/medication/administration/spec.py(3 hunks)care/emr/resources/medication/request/spec.py(3 hunks)care/emr/resources/medication/statement/spec.py(2 hunks)care/emr/resources/observation/spec.py(2 hunks)care/emr/resources/questionnaire/spec.py(3 hunks)care/emr/utils/valueset_coding_type.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
care/emr/resources/medication/statement/spec.py (1)
care/emr/utils/valueset_coding_type.py (1) (1)
ValueSetBoundCoding(5-19)
care/emr/resources/observation/spec.py (1)
care/emr/utils/valueset_coding_type.py (1) (1)
ValueSetBoundCoding(5-19)
care/emr/resources/questionnaire/spec.py (1)
care/emr/utils/valueset_coding_type.py (1) (1)
ValueSetBoundCoding(5-19)
care/emr/resources/medication/administration/spec.py (1)
care/emr/utils/valueset_coding_type.py (1) (1)
ValueSetBoundCoding(5-19)
care/emr/resources/medication/request/spec.py (1)
care/emr/utils/valueset_coding_type.py (1) (1)
ValueSetBoundCoding(5-19)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test / Test
🔇 Additional comments (24)
care/emr/resources/medication/statement/spec.py (2)
12-12: Good addition of the import.The import statement is correctly placed and follows the existing import organization pattern.
40-40: Excellent refactoring of the medication field.Replacing the explicit
Codingtype and field validator withValueSetBoundCoding[CARE_MEDICATION_VALUESET.slug]is a cleaner approach. This change removes the need for a separate validation method while maintaining the same validation logic, which should make the code more maintainable.care/emr/resources/allergy_intolerance/spec.py (3)
4-4: Field validator import cleaner now.The
Fieldimport has been removed since it's no longer needed with theValueSetBoundCodingapproach. This is a nice cleanup.
12-12: Good addition of the import.The import statement is correctly placed among the other imports.
92-92: Excellent refactoring of the code field.Replacing the explicit
Codingtype with field validation usingValueSetBoundCoding[CARE_ALLERGY_CODE_VALUESET.slug]is much cleaner. This change improves type safety and makes the validation requirements clearer at the point of declaration rather than hidden in a separate validator method.care/emr/resources/observation/spec.py (2)
19-19: Good addition of the import.The import statement for
ValueSetBoundCodingis correctly placed with the other imports.
95-95: Nice consistent refactoring of body_site and method fields.Both fields have been updated to use
ValueSetBoundCodingwith their respective value set slugs, which is consistent with the changes in other files. This approach properly enforces type constraints at the field declaration level rather than through separate validators.I notice you've maintained the optional nature of these fields with
| None, which is good for backward compatibility.Also applies to: 97-97
care/emr/resources/condition/spec.py (5)
5-5: New import for validators looks properly integrated.
Thefield_validatorimport appears valid and is used consistently in existing validators. No conflicts observed.
14-16: Good step towards consistent validation.
ImportingValueSetBoundCodingis a solid move for standardized value set checks, aligning with the new approach across the codebase.
139-139: Same possible typo for code-based attribute.
As with line 85, ensure the references toCARE_CODITION_CODE_VALUESETare correct.
144-144: Ensure naming and references remain consistent.
This change repeats the same pattern and the same potential slug naming concern.
85-85:✅ Verification successful
Confirm correctness of value set slug naming.
The type annotation forcodereferencesCARE_CODITION_CODE_VALUESET.slug. The term “CODITION” might be a typo.Please run this script to scan for other references and confirm the slug usage is intentional:
🏁 Script executed:
#!/bin/bash # Searching for references to CARE_CODITION_CODE_VALUESET to verify consistency rg 'CARE_CODITION_CODE_VALUESET'Length of output: 612
Slug Naming Verification: Naming Is Consistent
After verifying the references across the repository, it turns out that the term
CARE_CODITION_CODE_VALUESETis used throughout (notably incare/emr/resources/condition/valueset.pyandcare/emr/resources/condition/spec.py). While it might initially look like a typo, the consistency here indicates that it's intentional. Please disregard the presumed typo unless there's a coordinated refactor plan.care/emr/resources/questionnaire/spec.py (3)
15-15: Import aligns with unified validation stance.
IntroducingValueSetBoundCodinghere is consistent with the project-wide approach of binding codes to value sets.
116-116: Refined attribute type for question code.
SwitchingcodetoValueSetBoundCoding[CARE_OBSERVATION_VALUSET.slug]elegantly streamlines validation of observation codes.
143-143: Consistent extension to ‘unit’ field.
BindingunittoCARE_UCUM_UNITS.slugensures more robust data integrity.care/emr/resources/medication/administration/spec.py (5)
10-10: Sensible addition of Quantity import.
ImportingQuantitymatches the usage in dosage logic. Implementation looks good.
18-18: Adoption of shared coding utility.
LeveragingValueSetBoundCodingis consistent with the PR’s shift away from custom field validators.
88-88: Refined attribute for status reason.
Transitioningstatus_reasontoValueSetBoundCoding[CARE_MEDICATION_VALUESET.slug]fits the pattern and should streamline validations.
91-91: Consistent usage of medication binding.
This update helps unify medication coding and eliminates manual validations.
68-70:✅ Verification successful
Unified approach for site, route, and method.
Switching these fields toValueSetBoundCodingsupports a centralized validation model.Could you confirm no leftover references to the older
Codingtype exist for these fields? Please run:
🏁 Script executed:
#!/bin/bash rg -A3 "site: Coding" rg -A3 "route: Coding" rg -A3 "method: Coding"Length of output: 786
Unified approach confirmed in medication administration spec
The updates in
care/emr/resources/medication/administration/spec.pylook good—site,route, andmethodare now usingValueSetBoundCodingas intended. The grep check revealed no lingering usages of the oldCodingtype in this file. (For reference, while there are still a couple ofCodingusages incare/emr/resources/questionnaire_response/spec.py, those seem to be unrelated to the current changes.)care/emr/resources/medication/request/spec.py (4)
24-24: Import approach looks fine.
No immediate concerns. This import ofValueSetBoundCodingappears consistent with the rest of the application’s architecture.
131-133: Improved type safety with ValueSetBoundCoding.
Switchingadditional_instructionfromCodingto a list ofValueSetBoundCodingreferences the correct slug and should eliminate the need for manual validator methods. Please confirm that any tests foradditional_instructionremain valid or have been updated to reflect the new type.
137-142: Consistent approach to slug-bound codings.
All these fields now rely on built-in validation fromValueSetBoundCoding, removing the need for separate validation code. This should simplify maintenance. Confirm you have test coverage for the new usage, especially around optional fields.
166-166: Direct ValueSetBoundCoding usage for medication.
Replacing the oldCodingfield and JSON schema extra withValueSetBoundCoding[CARE_MEDICATION_VALUESET.slug]is cleaner and more explicit. Verify that no external references or manual validations still expect the previousCodingtype.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2945 +/- ##
===========================================
+ Coverage 52.02% 52.11% +0.08%
===========================================
Files 239 240 +1
Lines 10900 10796 -104
Branches 1189 1173 -16
===========================================
- Hits 5671 5626 -45
+ Misses 5210 5151 -59
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/emr/resources/base.py (1)
13-13: Unused class variable may need cleanup.Since you've commented out the
as_questionnairemethod, the__questionnaire_cache__class variable on line 13 might no longer be needed. It would be nice if you could remove unused variables when you're making these kinds of changes, but I guess that's asking a bit much.- __questionnaire_cache__ = {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
care/emr/api/viewsets/base.py(1 hunks)care/emr/resources/base.py(2 hunks)care/emr/tests/test_allergy_intolerance_api.py(1 hunks)care/emr/tests/test_chronic_condition_api.py(1 hunks)care/emr/tests/test_diagnosis_api.py(1 hunks)care/emr/tests/test_medication_request.py(1 hunks)care/emr/tests/test_symptom_api.py(1 hunks)care/emr/utils/mfa.py(1 hunks)config/settings/test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- care/emr/api/viewsets/base.py
🔇 Additional comments (10)
care/emr/resources/base.py (2)
2-2: Import statement simplification noted.You've reduced the import statement from what was likely more extensive to just
Annotated, Unionfrom typing. Given that you've commented out theas_questionnairemethod that probably used the removed imports, this makes sense... I suppose.
90-139:❓ Verification inconclusive
Verify impact on frontend consumers.
The docstring of the commented-out method indicates it was created "so that the FE has an idea about bound valuesets and other metadata about the form." Have you verified that removing this method won't break frontend code that might be relying on it? I'm sure you've thought of this, but just double-checking.
🏁 Script executed:
#!/bin/bash # Search for any references to 'as_questionnaire' in the codebase echo "Searching for references to as_questionnaire method..." rg "as_questionnaire" --type pyLength of output: 461
Action: Verify Frontend Dependency on as_questionnaire
It appears that the removal (or commenting out) of
as_questionnaireincare/emr/resources/base.pymight be incomplete—there’s still a reference incare/emr/api/viewsets/base.py(even though it's currently within commented code). Since the original intent was to provide the frontend with metadata (like bound valuesets), please double-check that none of the frontend consumers rely on this method. If the function is no longer needed, consider cleaning up all related commented-out references to avoid future confusion. Otherwise, ensure an appropriate fallback or replacement is in place.care/emr/utils/mfa.py (2)
37-37:❓ Verification inconclusive
Rate limiting parameter removal may have security implications.
The removal of the explicit rate limit "10/5m" parameter from the IP-based rate limiting check could potentially change how aggressively this security mechanism throttles requests.
🏁 Script executed:
#!/bin/bash # Check how ratelimit function is implemented and where the rate limits are defined now. grep -r "def ratelimit" --include="*.py" . grep -r "mfa-login" --include="*.py" . cat config/ratelimit.pyLength of output: 2378
Action Required: Confirm Rate Limit Configuration in Settings
We've verified that the explicit "10/5m" parameter is no longer passed directly. Instead, the
ratelimitfunction now defaults to usingsettings.DJANGO_RATE_LIMITas its rate limit. This change affects both the IP-based and the user-based rate limiting incare/emr/utils/mfa.py.
- File:
care/emr/utils/mfa.py(around line 37)- Details: The call
if ratelimit(request, "mfa-login", ["ip"]):now relies on the default rate defined in settings rather than the previously hard-coded "10/5m".- Next Steps: Please double-check that
settings.DJANGO_RATE_LIMITis set to the correct value (i.e., "10/5m" if that was the intended value) to ensure the throttling behavior remains as expected.
43-43:❓ Verification inconclusive
User-based rate limit parameter removed.
Similar to the IP-based rate limiting, the explicit "3/5m" parameter has been removed from the user-based rate limiting check. Assuming this is now controlled via a central configuration.
🏁 Script executed:
#!/bin/bash # Check if there are any settings files that might define these rate limits grep -r "RATELIMIT_" --include="*.py" . grep -r "mfa-login" --include="*.settings.py" .Length of output: 87
Action Required: Verify Central Rate Limit Configuration
It appears that the explicit rate limit parameters for the user-based check have been removed—mirroring the changes made for IP-based rate limiting. However, the preliminary grep results didn’t locate any configuration references (e.g., no settings with "RATELIMIT_" or "mfa-login" in *.settings.py files). This could indicate one of two things: either the central configuration isn’t named as expected (and is defined elsewhere), or it might have been overlooked.
- The ratelimit call in
care/emr/utils/mfa.py(line 43) now uses only"mfa-login"and[user_id].- Compare this with the IP-based rate limiting that now also relies on central configuration.
- Kindly ensure that a corresponding central configuration exists and is correctly referenced for user-based rate limiting.
Please manually verify that the intended central configuration is in place and that the changes here are correctly backed by it.
config/settings/test.py (1)
104-104: LGTM - Test setting to disable rate limiting added.Adding the
DISABLE_RATELIMIT = Truesetting for testing environments is good practice. This will prevent rate limiting from interfering with test execution, which could otherwise lead to flaky tests.care/emr/tests/test_diagnosis_api.py (1)
43-43:✅ Verification successful
Import path updated for validate_valueset.
The import path has been updated to reflect the movement of
validate_valuesetfrom a resource-specific module to a more general utility module. This change appears to be part of a refactoring effort to improve code organization.
🏁 Script executed:
#!/bin/bash # Verify that the new module exists and contains the validate_valueset function if [ -f "care/emr/utils/valueset_coding_type.py" ]; then echo "File exists, checking for validate_valueset function:" grep -n "def validate_valueset" care/emr/utils/valueset_coding_type.py else echo "File does not exist: care/emr/utils/valueset_coding_type.py" fi # Check if this change is consistent across other test files grep -r "validate_valueset" --include="test_*.py" care/emr/tests/Length of output: 1745
Verified: Updated Import Path for validate_valueset
Our checks confirm that the function now resides in
care/emr/utils/valueset_coding_type.pyas expected and that all tests (includingtest_diagnosis_api.py) reference this updated path consistently. Nice work ensuring consistency across the test suite.care/emr/tests/test_chronic_condition_api.py (1)
42-42: Import path updated for consistency.The import path has been updated to match the new location of
validate_valueset. This ensures consistency across the test suite and reflects the module reorganization.care/emr/tests/test_symptom_api.py (1)
43-44: Import path update looks good for centralizing validation.The update to import
validate_valuesetfrom the utils module instead of from the condition-specific module indicates a positive refactoring effort to centralize validation logic. This move makes the codebase a bit more maintainable and adheres to the DRY principle.care/emr/tests/test_medication_request.py (1)
37-37: Consistent import path update maintained across test files.The same import path change has been implemented here, which shows consistency in the refactoring effort. Moving common validation logic to a central utility module improves maintainability and reduces duplication. I suppose I'll just have to trust that you've tested this thoroughly.
care/emr/tests/test_allergy_intolerance_api.py (1)
42-42: Import path modification continues the pattern of centralization.This is the third instance of moving the
validate_valuesetfunction to the utility module, which confirms a systematic effort to centralize validation logic. This type of refactoring improves code organization and maintainability by putting related functionality in a single location instead of scattered across different modules.
Proposed Changes
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
ValueSetBoundCoding.DISABLE_RATELIMITfor controlling rate limiting behavior during testing.validate_valuesetfunction across multiple test files to reflect changes in module organization.