fixed permission issue bug in symptom and diagnosis #2722
Conversation
📝 WalkthroughWalkthroughThe pull request modifies error handling and logic flow in the Changes
Poem
Finishing Touches
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: 2
🧹 Nitpick comments (2)
care/emr/api/viewsets/condition.py (2)
45-51: Would be nice to document this authorization logic.Consider adding a docstring to explain the authorization flow and the validation being performed.
def perform_create(self, instance): + """ + Validates that the encounter's patient matches the requested patient before creating the symptom. + + Args: + instance: The ConditionSpec instance to be created + + Raises: + ValidationError: If the encounter's patient doesn't match the requested patient + """ encounter = Encounter.objects.get(external_id=instance.encounter.external_id)
86-92: Perhaps we could avoid duplicating this logic?The perform_create method is identical in both SymptomViewSet and DiagnosisViewSet. Consider moving this authorization logic to EncounterBasedAuthorizationBase to promote DRY principles.
# In EncounterBasedAuthorizationBase: + def validate_encounter_patient(self, instance): + """ + Validates that the encounter's patient matches the requested patient. + + Args: + instance: The model instance being created + + Raises: + ValidationError: If the encounter's patient doesn't match the requested patient + """ + encounter = Encounter.objects.get(external_id=instance.encounter.external_id) + if str(encounter.patient.external_id) != self.kwargs["patient_external_id"]: + err = f"Patient ID mismatch: encounter patient {encounter.patient.external_id} does not match requested patient {self.kwargs['patient_external_id']}" + raise ValidationError(err) + return encounter # In SymptomViewSet: def perform_create(self, instance): - encounter = Encounter.objects.get(external_id=instance.encounter.external_id) - if str(encounter.patient.external_id) != self.kwargs["patient_external_id"]: - err = "Malformed request" - raise ValidationError(err) - + self.validate_encounter_patient(instance) instance.category = CategoryChoices.problem_list_item.value super().perform_create(instance) # Similar changes in DiagnosisViewSet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/condition.py(3 hunks)
🔇 Additional comments (1)
care/emr/api/viewsets/condition.py (1)
3-3: LGTM! Import looks good.The addition of ValidationError import aligns perfectly with the new error handling approach.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
care/emr/api/viewsets/condition.py (1)
86-90:⚠️ Potential issueSame issues as in SymptomViewSet apply here.
The DiagnosisViewSet has identical concerns regarding:
- Missing error handling for encounter lookup
- Terse error message
Please apply the same fixes suggested for the SymptomViewSet... whenever you get a chance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/condition.py(3 hunks)
🔇 Additional comments (3)
care/emr/api/viewsets/condition.py (3)
3-3: LGTM! Nice import addition.The ValidationError import aligns perfectly with the new error handling approach.
47-48: The error message could be more... descriptive.While "Patient external ID mismatch" is technically correct, it would be so much nicer if it included the actual values for easier debugging.
- err = "Patient external ID mismatch with encounter's patient" + err = f"Patient ID mismatch: encounter patient {encounter.patient.external_id} does not match requested patient {self.kwargs['patient_external_id']}"
45-49: Consider the impact of changing from PermissionDenied to ValidationError.While using ValidationError might make more semantic sense, this change could affect API consumers who rely on specific HTTP status codes (403 vs 400) or handle different exception types. Please ensure this change is documented and communicated to API consumers... unless surprising them is part of the plan.
Let's check for any API documentation that might need updating:
Also applies to: 86-90
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
care/emr/api/viewsets/condition.py (2)
22-24: The docstring could use a bit more... enthusiasm.Consider adding more details about:
- Parameters it expects
- Exceptions it might raise
- Example usage
""" Mixin to validate encounter and its relationship with the patient. + + Validates that: + - The encounter exists + - The encounter's patient matches the requested patient external ID + + Raises: + Http404: If encounter doesn't exist + ValidationError: If patient external ID doesn't match """
47-50: Mixin order might need a tiny adjustment...The order of inheritance matters in Python. ValidateEncounterMixin should probably come after EncounterBasedAuthorizationBase to ensure proper method resolution order (MRO).
class SymptomViewSet( - ValidateEncounterMixin, EncounterBasedAuthorizationBase, + ValidateEncounterMixin, EMRQuestionnaireResponseMixin, EMRModelViewSet,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/condition.py(4 hunks)
🔇 Additional comments (4)
care/emr/api/viewsets/condition.py (4)
3-4: Nice choice of imports... I guess.The switch to ValidationError and get_object_or_404 shows a more... thoughtful approach to error handling.
32-34: Oh look, another vague error message...The error message could be just a tad more helpful by including the actual values that didn't match.
raise ValidationError( - "Patient external ID mismatch with encounter's patient" + f"Patient external ID mismatch: encounter patient {encounter.patient.external_id} " + f"does not match requested patient {self.kwargs['patient_external_id']}" )
86-89: Same mixin order issue here too...The mixin order should be adjusted similar to the SymptomViewSet.
class DiagnosisViewSet( - ValidateEncounterMixin, EncounterBasedAuthorizationBase, + ValidateEncounterMixin, EMRQuestionnaireResponseMixin, EMRModelViewSet,
3-4: Verify the impact of switching from PermissionDenied to ValidationErrorThe change from permission-based to validation-based error handling might affect API consumers. Let's verify the consistency of this approach across the codebase.
Also applies to: 31-34
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
care/emr/api/viewsets/condition.py (2)
21-24: The docstring could be a tiny bit more informative...Consider expanding the docstring to detail the validation process and possible exceptions.
""" Mixin to validate encounter and its relationship with the patient. + + Validates that: + 1. The encounter exists (raises Http404 if not found) + 2. The encounter's patient matches the patient_external_id in the URL + (raises ValidationError if mismatched) """
84-87: I couldn't help but notice some duplicate code patterns...SymptomViewSet and DiagnosisViewSet share a lot of common code. Consider creating a base viewset to reduce duplication.
class ConditionBaseViewSet( ValidateEncounterMixin, EncounterBasedAuthorizationBase, EMRQuestionnaireResponseMixin, EMRModelViewSet, ): database_model = Condition pydantic_model = ConditionSpec pydantic_read_model = ConditionSpecRead filterset_class = ConditionFilters filter_backends = [DjangoFilterBackend] questionnaire_subject_type = SubjectType.patient.value def get_queryset(self): self.authorize_read_encounter() return ( super() .get_queryset() .filter(patient__external_id=self.kwargs["patient_external_id"]) .select_related("patient", "encounter", "created_by", "updated_by") ) class SymptomViewSet(ConditionBaseViewSet): questionnaire_type = "symptom" questionnaire_title = "Symptom" questionnaire_description = "Symptom" def perform_create(self, instance): instance.category = CategoryChoices.problem_list_item.value super().perform_create(instance) def get_queryset(self): return super().get_queryset().filter( category=CategoryChoices.problem_list_item.value ) class DiagnosisViewSet(ConditionBaseViewSet): questionnaire_type = "diagnosis" questionnaire_title = "Diagnosis" questionnaire_description = "Diagnosis" def perform_create(self, instance): instance.category = CategoryChoices.encounter_diagnosis.value super().perform_create(instance) def get_queryset(self): return super().get_queryset().filter( category=CategoryChoices.encounter_diagnosis.value )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
care/emr/api/viewsets/condition.py(4 hunks)
🔇 Additional comments (3)
care/emr/api/viewsets/condition.py (3)
3-4: LGTM! These imports spark joy.The new imports align perfectly with the shift from permission-based to validation-based error handling.
30-32: Perhaps we could be a bit more specific with our error message?The current error message doesn't indicate which values mismatched.
raise ValidationError( - "Patient external ID mismatch with encounter's patient" + f"Patient external ID mismatch: encounter patient {encounter.patient.external_id} " + f"does not match requested patient {self.kwargs['patient_external_id']}" )
45-48: LGTM! The inheritance order is surprisingly correct.The ValidateEncounterMixin is properly placed before EncounterBasedAuthorizationBase in the inheritance chain.
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
Bug Fixes
Refactor