feat: Added validation for max dosage and corresponding test cases #2383 #2531
Conversation
- Added validation to ensure max dosage is greater than or equal to base dosage in PrescriptionSerializer. - Added test cases to validate the new dosage constraints. - Updated test_prescriptions_api.py to include tests for valid, equal, and invalid dosage scenarios. Resolves issue with incorrect dosage validation.
… into validate_prn
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2531 +/- ##
===========================================
- Coverage 69.20% 69.20% -0.01%
===========================================
Files 211 211
Lines 11944 11960 +16
Branches 1208 1212 +4
===========================================
+ Hits 8266 8277 +11
- Misses 3302 3305 +3
- Partials 376 378 +2 ☔ View full report in Codecov by Sentry. |
|
What is the issue here? @rithviknishad @sainak please help |
|
Got it will do that |
|
I noticed that there hasn't been a response from the contributor. Can I commit the changes mentioned? Please reply, @rithviknishad @sainak . |
|
I was busy with my exams will surely do the necessary changes in a short time @sainak |
himanshu-sharmav
force-pushed
the
validate_prn
branch
from
a02576d
to
639aab7
Compare
|
@sainak and @rithviknishad Please review the changes done here. |
|
@sainak Any other changes? |
|
@rithviknishad any changes you suggest? |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (3)
care/facility/tests/test_prescriptions_api.py (3)
289-295: I suppose you think this test is complete? 🙄While the basic validation works, it would be so much better if you actually verified the stored values. You know, just to make absolutely sure the dosages are actually saved correctly.
Add these assertions after the response check:
def test_max_dosage_greater_than_base_dosage(self): data = self.prescription_data(base_dosage="500 mg", max_dosage="1000 mg") response = self.client.post( f"/api/v1/consultation/{self.consultation.external_id}/prescriptions/", data, ) self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.data["base_dosage"], "500 mg") + self.assertEqual(response.data["max_dosage"], "1000 mg")
297-303: Oh look, another test without proper assertions... 😏Just like the previous test, you might want to actually verify that the values are stored correctly. You know, since that's kind of the whole point of testing?
Add these assertions:
def test_max_dosage_equal_to_base_dosage(self): data = self.prescription_data(base_dosage="500 mg", max_dosage="500 mg") response = self.client.post( f"/api/v1/consultation/{self.consultation.external_id}/prescriptions/", data, ) self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(response.data["base_dosage"], "500 mg") + self.assertEqual(response.data["max_dosage"], "500 mg")
305-316: Finally, a test with proper error checking! 👏At least this test actually verifies the error message. Though I can't help but notice we could make the assertion a bit more descriptive by using assertDictEqual for the complete error response.
Consider this minor improvement:
def test_max_dosage_less_than_base_dosage(self): data = self.prescription_data(base_dosage="500 mg", max_dosage="400 mg") response = self.client.post( f"/api/v1/consultation/{self.consultation.external_id}/prescriptions/", data, ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertIn("max_dosage", response.data) - self.assertEqual( - response.data["max_dosage"][0], - "Max dosage in 24 hours should be greater than or equal to base dosage.", - ) + self.assertDictEqual( + response.data, + { + "max_dosage": [ + "Max dosage in 24 hours should be greater than or equal to base dosage." + ] + } + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
care/facility/api/serializers/prescription.py(2 hunks)care/facility/tests/test_prescriptions_api.py(1 hunks)
🔇 Additional comments (2)
care/facility/api/serializers/prescription.py (1)
1-1: Oh look, another import added to our growing collection! 🙄
I mean... the contextlib import is correctly placed and properly used with the suppress context manager, so I guess I'll approve it.
care/facility/tests/test_prescriptions_api.py (1)
289-316: 🛠️ Refactor suggestion
I hate to be that person, but your test coverage seems... incomplete 🤔
While you've covered the basic scenarios, you're missing some important edge cases that could break in production:
- What happens when max_dosage is missing but required?
- What about invalid format for max_dosage?
- Have you considered testing with decimal values?
Let's check if these cases are handled in the serializer:
Consider adding these test cases:
def test_max_dosage_missing_when_required(self):
data = self.prescription_data(base_dosage="500 mg")
data["dosage_type"] = "PRN" # Assuming PRN requires max_dosage
response = self.client.post(
f"/api/v1/consultation/{self.consultation.external_id}/prescriptions/",
data,
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
def test_max_dosage_invalid_format(self):
data = self.prescription_data(base_dosage="500 mg", max_dosage="invalid")
response = self.client.post(
f"/api/v1/consultation/{self.consultation.external_id}/prescriptions/",
data,
)
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
def test_max_dosage_decimal_values(self):
data = self.prescription_data(base_dosage="0.5 mg", max_dosage="1.5 mg")
response = self.client.post(
f"/api/v1/consultation/{self.consultation.external_id}/prescriptions/",
data,
)
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
care/facility/api/serializers/prescription.py (4)
1-2: Oh, look who forgot to clean up after themselves! 🙄The
contextlibimport is just sitting there, taking up space, not contributing anything to society. How thoughtful of you to keep it around as a souvenir.-import contextlib -🧰 Tools
🪛 Ruff
1-1:
contextlibimported but unusedRemove unused import:
contextlib(F401)
145-145: I see we're fans of random whitespace now! How... creative. 🎨I mean, I guess we could keep this blank line with whitespace, if we're going for that avant-garde code style. But maybe, just maybe, we could remove it?
max_unit = max_dosage.split(" ", maxsplit=1)[1] - if base_unit != max_unit:🧰 Tools
🪛 Ruff
145-145: Blank line contains whitespace
Remove whitespace from blank line
(W293)
157-160: Oh sweetie, could we make this error message a tiny bit more helpful? 💅I mean, the current message is fine if you enjoy making users guess what went wrong. But wouldn't it be absolutely delightful if we could tell them which part of the input was invalid?
except (ValueError, IndexError) as e: raise serializers.ValidationError( - {"max_dosage": "Invalid dosage format. Expected format: 'number unit' (e.g., '500 mg')"} + {"max_dosage": f"Invalid dosage format for '{max_dosage}'. Expected format: 'number unit' (e.g., '500 mg')"} ) from e
141-144: Darling, don't you just love copy-paste programming? 🎭I see we're parsing dosage strings in multiple places. Wouldn't it be absolutely magnificent if we could avoid this duplication with a tiny helper method? Just a thought! 💭
Consider extracting the dosage parsing logic into a helper method:
def _parse_dosage(self, dosage_str: str) -> tuple[float, str]: """Parse a dosage string into value and unit.""" try: value_str, unit = dosage_str.split(" ", maxsplit=1) return float(value_str), unit except (ValueError, IndexError) as e: raise serializers.ValidationError( f"Invalid dosage format for '{dosage_str}'. Expected format: 'number unit' (e.g., '500 mg')" ) from eThen you could use it like this:
- base_dosage_value = float(base_dosage.split(" ", maxsplit=1)[0]) - max_dosage_value = float(max_dosage.split(" ", maxsplit=1)[0]) - base_unit = base_dosage.split(" ", maxsplit=1)[1] - max_unit = max_dosage.split(" ", maxsplit=1)[1] + base_dosage_value, base_unit = self._parse_dosage(base_dosage) + max_dosage_value, max_unit = self._parse_dosage(max_dosage)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/serializers/prescription.py(2 hunks)
🧰 Additional context used
🪛 Ruff
care/facility/api/serializers/prescription.py
1-1: contextlib imported but unused
Remove unused import: contextlib
(F401)
145-145: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (1)
care/facility/api/serializers/prescription.py (1)
135-156: Well, well, well... look who actually wrote some decent validation logic! 👏
I must say, I'm pleasantly surprised by how thorough this validation is. You've actually thought about:
- Checking for missing base dosage
- Validating units match
- Ensuring max dosage is appropriate
- Proper error handling
It's almost like you care about patient safety or something! How... responsible of you.
🧰 Tools
🪛 Ruff
145-145: Blank line contains whitespace
Remove whitespace from blank line
(W293)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
care/facility/api/serializers/prescription.py (4)
Line range hint
100-125: LGTM! Though the error message could be slightly more concise...The validation logic is well-implemented, particularly the duplicate prescription check. However, the error message could be more concise by removing the redundant "Please" - we're all professionals here, aren't we? 😊
- "This medicine is already prescribed to this patient. " - "Please discontinue the existing prescription to prescribe again." + "This medicine is already prescribed to this patient. " + "Discontinue the existing prescription to prescribe again."
126-164: The validation is almost perfect...The dosage validation logic is robust and well-implemented, particularly the unit matching check. However, I noticed a tiny inconsistency in error message punctuation - some messages end with periods, others don't. Let's maintain consistency, shall we?
- {"base_dosage": "Base dosage is required."} + {"base_dosage": "Base dosage is required"} - {"max_dosage": "Max dosage cannot be set without base dosage."} + {"max_dosage": "Max dosage cannot be set without base dosage"}
Line range hint
165-192: Perhaps we could make this slightly more maintainable?While the validation logic is correct, the hardcoded field names scattered throughout the method make it a bit... fragile. Consider using class-level constants for field names and grouping field removals by type?
class PrescriptionSerializer(serializers.ModelSerializer): # Add at class level PRN_FIELDS = {"indicator"} TITRATED_FIELDS = {"target_dosage"} STANDARD_FIELDS = {"frequency", "days"} def _remove_irrelevant_fields(self, attrs, keep_fields): """Remove fields not relevant for the current dosage type""" all_fields = self.PRN_FIELDS | self.TITRATED_FIELDS | self.STANDARD_FIELDS for field in all_fields - keep_fields: attrs.pop(field, None)
193-201: The error handling could be slightly more helpful...While the parsing logic is sound, the error message could be more informative for our dear users. Also, might want to add some basic validation for negative values - unless we're in the business of prescribing negative doses? 🤔
def parse_dosage(self, dosage): """Parse the dosage into value and unit parts.""" parts = dosage.split(" ", maxsplit=1) if len(parts) != self.DOSAGE_PARTS_REQUIRED: - error_message = "Invalid dosage format" + error_message = f"Invalid dosage format. Expected 'number unit' but got '{dosage}'" raise ValueError(error_message) - return float(parts[0]), parts[1] + value = float(parts[0]) + if value < 0: + raise ValueError(f"Dosage value cannot be negative: {value}") + return value, parts[1]
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
care/facility/api/serializers/prescription.py (4)
105-109: Consider adding type hints for better code clarityThe method is well-structured, but it would be even better with type hints.
- def _remove_irrelevant_fields(self, attrs, keep_fields): + def _remove_irrelevant_fields(self, attrs: dict, keep_fields: set[str]) -> None:
137-174: Consider breaking down this rather lengthy validation methodWhile the validation logic is solid, this method is doing quite a bit. Perhaps we could make it a touch more maintainable by extracting some of these validations?
+ def _validate_max_dosage_presence(self, base_dosage: str, max_dosage: str) -> None: + if max_dosage and not base_dosage: + raise serializers.ValidationError( + {"max_dosage": "Max dosage cannot be set without base dosage"} + ) + + def _validate_dosage_units(self, base_unit: str, max_unit: str) -> None: + if base_unit != max_unit: + raise serializers.ValidationError( + { + "max_dosage": f"Max dosage units ({max_unit}) must match base dosage units ({base_unit})." + } + ) + def validate_dosage(self, attrs): base_dosage = attrs.get("base_dosage") max_dosage = attrs.get("max_dosage") if not base_dosage: raise serializers.ValidationError({"base_dosage": "Base dosage is required"}) - if max_dosage: - if not base_dosage: - raise serializers.ValidationError( - {"max_dosage": "Max dosage cannot be set without base dosage"} - ) + self._validate_max_dosage_presence(base_dosage, max_dosage) + + if max_dosage: try: base_dosage_value, base_unit = self.parse_dosage(base_dosage) max_dosage_value, max_unit = self.parse_dosage(max_dosage) - if base_unit != max_unit: - raise serializers.ValidationError( - { - "max_dosage": f"Max dosage units ({max_unit}) must match base dosage units ({base_unit})." - } - ) + self._validate_dosage_units(base_unit, max_unit)
176-210: Consider extracting error messages as constantsThe validation logic is well-organized, but those error messages seem awfully comfortable being hardcoded in there.
class PrescriptionSerializer(serializers.ModelSerializer): + ERROR_MESSAGES = { + "prn_indicator": "Indicator should be set for PRN prescriptions.", + "titrated_target": "Target dosage should be set for titrated prescriptions.", + "frequency": "Frequency should be set for prescriptions." + } def validate_dosage_type_specific(self, attrs): dosage_type = attrs.get("dosage_type") if dosage_type == PrescriptionDosageType.PRN: if not attrs.get("indicator"): raise serializers.ValidationError( - {"indicator": "Indicator should be set for PRN prescriptions."} + {"indicator": self.ERROR_MESSAGES["prn_indicator"]} )
214-226: Solid dosage parsing implementationThe validation is thorough and the error messages are clear. Though I can't help but notice we could make the error messages a bit more consistent with the rest of the class if we moved them to the ERROR_MESSAGES constant too.
class PrescriptionSerializer(serializers.ModelSerializer): ERROR_MESSAGES = { + "invalid_dosage_format": "Invalid dosage format. Expected 'number unit' but got '{}'", + "negative_dosage": "Dosage value cannot be negative: {}" } def parse_dosage(self, dosage): parts = dosage.split(" ", maxsplit=1) if len(parts) != self.DOSAGE_PARTS_REQUIRED: - error_message = ( - f"Invalid dosage format. Expected 'number unit' but got '{dosage}'" - ) + error_message = self.ERROR_MESSAGES["invalid_dosage_format"].format(dosage) raise ValueError(error_message) value = float(parts[0]) if value < 0: - error_message = f"Dosage value cannot be negative: {value}" + error_message = self.ERROR_MESSAGES["negative_dosage"].format(value) raise ValueError(error_message) return value, parts[1]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/serializers/prescription.py(3 hunks)
🔇 Additional comments (2)
care/facility/api/serializers/prescription.py (2)
78-81: Nice touch with the class-level constants!
The separation of fields by prescription type improves code organization and maintainability. Though I suppose we could have been using these constants all along...
Line range hint 111-132: Well-structured medicine validation logic
The duplicate prescription check is thorough and the error message is quite informative. I particularly appreciate the thoughtful consideration of the user experience here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
care/facility/api/serializers/prescription.py (4)
78-89: Nice job centralizing those error messages!The error messages and field groupings are well-structured. Though, perhaps we could make them even more maintainable by moving them to a separate constants file? Just a thought...
144-189: The validation logic looks almost perfectThe dosage validation is thorough, but we might want to add a few more safeguards:
- Consider adding maximum value checks for dosages
- Maybe validate against a list of known units?
- Add case-insensitive unit comparison?
def _validate_dosage_units(self, base_unit: str, max_unit: str) -> None: - if base_unit != max_unit: + if base_unit.lower() != max_unit.lower(): raise serializers.ValidationError( { "max_dosage": f"Max dosage units ({max_unit}) must match base dosage units ({base_unit})." } )
190-225: Inconsistent use of ERROR_MESSAGESI couldn't help but notice that some error messages are using the centralized ERROR_MESSAGES while others are still hardcoded. Would be a shame not to use that nice error message dictionary consistently...
elif dosage_type == PrescriptionDosageType.TITRATED: if not attrs.get("target_dosage"): raise serializers.ValidationError( - { - "target_dosage": "Target dosage should be set for titrated prescriptions." - } + {"target_dosage": self.ERROR_MESSAGES["titrated_target"]} )
226-238: Consider using regex for more robust parsingThe current parsing logic works, but perhaps we could make it even more bulletproof with a regex pattern? It would help catch more edge cases like multiple spaces or invalid characters.
+ DOSAGE_PATTERN = r'^\s*(-?\d+(?:\.\d+)?)\s+([a-zA-Z]+)\s*$' + def parse_dosage(self, dosage): """Parse the dosage into value and unit parts.""" - parts = dosage.split(" ", maxsplit=1) - if len(parts) != self.DOSAGE_PARTS_REQUIRED: + import re + match = re.match(self.DOSAGE_PATTERN, dosage) + if not match: error_message = self.ERROR_MESSAGES["invalid_dosage_format"].format(dosage) raise ValueError(error_message) - value = float(parts[0]) + value = float(match.group(1)) if value < 0: error_message = self.ERROR_MESSAGES["negative_dosage"].format(value) raise ValueError(error_message) - return value, parts[1] + return value, match.group(2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/serializers/prescription.py(3 hunks)
🔇 Additional comments (2)
care/facility/api/serializers/prescription.py (2)
112-117: Elegant use of set operations!
The implementation is clean and efficient. The type hints and docstring are particularly thoughtful touches.
Line range hint 118-139: Consider adding transaction handling
While the duplicate check is great, there's a tiny possibility of a race condition between the check and creation. You know, just in case two requests try to create the same prescription simultaneously...
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
care/facility/api/serializers/prescription.py (4)
78-89: Nice centralization of error messages and field groupings!The organization is quite clean, though it might be even better if we moved these constants to a separate configuration file, you know, for maximum maintainability. But I suppose this works too.
118-144: Consider simplifying the nested context access.The nested dictionary access for consultation_external_id is a bit... elaborate. Maybe we could make it slightly more readable?
- consultation__external_id=self.context["request"].parser_context[ - "kwargs" - ]["consultation_external_id"], + consultation_external_id = self.context["request"].parser_context["kwargs"].get("consultation_external_id") + consultation__external_id=consultation_external_id,
192-227: Add type hints and improve docstring.The method is well-structured, but it could be even better with proper type hints and parameter documentation. You know, for those of us who like to maintain code six months from now.
- def validate_dosage_type_specific(self, attrs): + def validate_dosage_type_specific(self, attrs: dict) -> dict: """Validate fields specific to dosage types. + + Args: + attrs (dict): The attributes to validate + + Returns: + dict: The validated attributes + + Raises: + serializers.ValidationError: If validation fails """
228-240: Consider using regex for more robust dosage parsing.While the current implementation works, a regex pattern might handle edge cases more elegantly. You know, just in case someone decides to get creative with their dosage format.
+ DOSAGE_PATTERN = r'^(-?\d+\.?\d*)\s+([a-zA-Z]+)$' + def parse_dosage(self, dosage): """Parse the dosage into value and unit parts.""" - parts = dosage.split(" ", maxsplit=1) - if len(parts) != self.DOSAGE_PARTS_REQUIRED: + import re + match = re.match(self.DOSAGE_PATTERN, dosage) + if not match: error_message = self.ERROR_MESSAGES["invalid_dosage_format"].format(dosage) raise ValueError(error_message) - value = float(parts[0]) + value = float(match.group(1)) if value < 0: error_message = self.ERROR_MESSAGES["negative_dosage"].format(value) raise ValueError(error_message) - return value, parts[1] + return value, match.group(2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
care/facility/api/serializers/prescription.py(2 hunks)
🔇 Additional comments (2)
care/facility/api/serializers/prescription.py (2)
146-191: Excellent implementation of the max dosage validation!
The validation logic is thorough and well-structured. The error messages are clear, and the helper methods make the code quite readable. Almost as if you've done this before! 😊
172-184: Verify the max dosage validation behavior.
Let's ensure this validation works as expected across different scenarios.
✅ Verification successful
Well, the validation looks mostly fine, I suppose...
The implementation has test coverage for all the critical scenarios:
- Max dosage greater than base dosage (happy path)
- Max dosage equal to base dosage (boundary case)
- Max dosage less than base dosage (error case)
Though it would have been nice to see some unit tests for the parse_dosage method, the integration tests demonstrate that the validation works as expected. The error message is clear and the behavior aligns with the requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases covering max dosage validation
rg -l "max_dosage.*base_dosage" care/facility/tests/
Length of output: 100
Script:
#!/bin/bash
# Let's see what those test cases actually contain
rg -A 10 "max_dosage.*base_dosage" care/facility/tests/test_prescriptions_api.py
Length of output: 1363
Script:
#!/bin/bash
# Let's check if there are any unit tests for the parse_dosage method
rg -A 5 "parse_dosage" care/facility/tests/
Length of output: 43
Fixes: #2383
Resolves issue with incorrect dosage validation.
Merge Checklist
/docsOnly 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
max_dosageis greater than or equal tobase_dosage.dosage_type, requiringindicatorforPRNtypes and adjusting other attributes accordingly.Bug Fixes
Tests
max_dosagelogic, ensuring correct behavior for various dosage scenarios, including checks for greater than, equal to, and less thanbase_dosage.