Added validation for sync charge item costs #3350
Conversation
📝 WalkthroughWalkthroughSingle-file change adding validation inside the discount handling loop of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (.cursorrules)
Files:
🪛 Ruff (0.14.5)care/emr/resources/charge_item/sync_charge_item_costs.py43-43: Avoid specifying long messages outside the exception class (TRY003) 45-47: Avoid specifying long messages outside the exception class (TRY003) 49-49: Unused Remove unused (RUF100) 51-51: Avoid specifying long messages outside the exception class (TRY003) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
care/emr/resources/charge_item/sync_charge_item_costs.py (2)
11-14: Consider defining error messages as constants.The static analysis tool suggests avoiding inline exception messages. While not critical, defining these messages as module-level constants or custom exception classes would improve maintainability and consistency, especially if these messages need to be referenced elsewhere.
Example refactor:
# At module level ERROR_AMOUNT_NEGATIVE = "Amount cannot be negative" ERROR_AMOUNT_EXCEEDS_BASE = "Amount cannot be more than base amount" ERROR_FACTOR_OUT_OF_RANGE = "Factor must be between 1 and 100" def calculate_amount(component, quantity, base): if component.amount: if component.amount < 0: raise ValidationError(ERROR_AMOUNT_NEGATIVE) if component.amount > base: raise ValidationError(ERROR_AMOUNT_EXCEEDS_BASE) # ... rest of the logicAlso applies to: 18-20
26-59: Consider adding validation for quantity and base amount.While the new validations for surcharge/discount/tax components are helpful, there's no validation for
quantity(line 31) or the basecomponent.amount(line 37). Negative or zero quantities could lead to unexpected behavior in the calculations.Would you like me to suggest validation logic for these parameters as well?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/resources/charge_item/sync_charge_item_costs.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: Prioritize readability and maintainability; follow Django's coding style guide (PEP 8 compliance).
Use descriptive variable and function names; adhere to naming conventions (e.g., lowercase with underscores for functions and variables).
Files:
care/emr/resources/charge_item/sync_charge_item_costs.py
🪛 Ruff (0.14.3)
care/emr/resources/charge_item/sync_charge_item_costs.py
12-12: Avoid specifying long messages outside the exception class
(TRY003)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
20-20: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test / Test
| if component.amount < 0: | ||
| raise ValidationError("Amount cannot be negative") | ||
| if component.amount > base: | ||
| raise ValidationError("Amount cannot be more than base amount") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the amount validation logic.
The validation at line 13 compares component.amount (unit amount, before multiplication) against base (which is the total base amount after quantity multiplication from line 39). This means you're comparing a unit price to a total price, which seems inconsistent.
For example, if base unit = 100, quantity = 10 (so base = 1000), and component.amount = 150, the validation would fail even though the intent might be to ensure the computed total component amount (150 × 10 = 1500) doesn't exceed some threshold. Please clarify whether you intended to compare unit amounts or validate the final computed amount instead.
Run the following to check how this validation behaves with actual test cases:
🏁 Script executed:
#!/bin/bash
# Description: Search for test cases or usage examples of calculate_amount to understand expected behavior
rg -n -C5 'calculate_amount|sync_charge_item_costs' --type=py -g '!care/emr/resources/charge_item/sync_charge_item_costs.py'Length of output: 5548
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full file to understand the context of base and component.amount values
cat care/emr/resources/charge_item/sync_charge_item_costs.py
Length of output: 2719
---
🏁 Script executed:
```shell
#!/bin/bash
# Check the test file for any test cases that exercise calculate_amount logic
rg -n -A 20 'test_sync_charge_item_costs_called' care/emr/tests/test_charge_item_api.py
Length of output: 783
Fix the unit vs. total comparison in amount validation at lines 11–14.
The validation compares component.amount (a unit amount, not yet multiplied by quantity at line 15) against base (which is already the total amount after multiplication at line 39). When calculate_amount is called for surcharge/discount/tax components (lines 43, 48, 52), this means you're comparing apples to oranges.
For example: if the base unit is 100 with quantity 10 (total base = 1,000) and a surcharge unit is 500, the validation passes (500 ≤ 1,000), but the computed surcharge becomes 500 × 10 = 5,000, which far exceeds the base. Either validate the final computed amount (unit × quantity) or adjust the comparison logic to be unit-to-unit.
🧰 Tools
🪛 Ruff (0.14.3)
12-12: Avoid specifying long messages outside the exception class
(TRY003)
14-14: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In care/emr/resources/charge_item/sync_charge_item_costs.py around lines 11–14,
the validation incorrectly compares component.amount (a per-unit value) to base
(a total already multiplied by quantity). Fix by validating the same units:
either compute the component total (component.amount * quantity) and compare
that to base, or compute the base per-unit (base / quantity) and compare
component.amount to that per-unit base; update the error messages accordingly
and ensure quantity is available/used in the check.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3350 +/- ##
===========================================
- Coverage 73.88% 73.85% -0.03%
===========================================
Files 435 435
Lines 19760 19766 +6
Branches 2143 2146 +3
===========================================
Hits 14599 14599
- Misses 4715 4721 +6
Partials 446 446 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if component.factor: | ||
| component.amount = base * component.factor / 100 | ||
| max_factor = 100 | ||
| if component.factor < 1 or component.factor > max_factor: |
There was a problem hiding this comment.
Factor can be any number, surcharges can be more than 100% and surcharges can be negative as well, remove this validation
…dkishorr/fix/chargeitem_validation
Proposed Changes
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
✏️ Tip: You can customize this high-level summary in your review settings.