adding swagger support #2772
adding swagger support #2772
Conversation
|
Warning Rate limit exceeded@DraKen0009 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces comprehensive API documentation enhancements across multiple viewsets in the Care EMR system. The primary focus is on adding Changes
Possibly related PRs
Suggested Reviewers
Poem
Sequence DiagramsequenceDiagram
participant Developer
participant ViewSet
participant SwaggerGenerator
participant APISchema
Developer->>ViewSet: Apply @generate_swagger_schema_decorator
ViewSet->>SwaggerGenerator: Generate Schema
SwaggerGenerator->>APISchema: Create Detailed Documentation
APISchema-->>Developer: Enhanced API Specification
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: 0
🔭 Outside diff range comments (1)
care/emr/api/otp_viewsets/login.py (1)
Line range hint
65-77: Consider masking OTP validation logic in error messages.The error messages might reveal too much about the OTP validation process. It would be just lovely if we could make them more generic.
Update the error messages:
- raise ValidationError({"phone_number": "Max Retries has exceeded"}) + raise ValidationError({"detail": "Please try again later"})
🧹 Nitpick comments (22)
care/emr/api/viewsets/roles.py (1)
11-13: Those empty lines seem... interesting.I notice we're adding two empty lines before the schema generation call. While I'm sure there's a fascinating reason for this spacing choice, perhaps we could keep it consistent with the rest of the codebase?
- - RoleViewSet.generate_swagger_schema() +RoleViewSet.generate_swagger_schema()care/emr/api/otp_viewsets/patient.py (1)
25-27: Documentation would be lovely here.Since we're dealing with patient authentication and OTP functionality, it might be slightly helpful to document what this schema generation is actually doing. You know, for those who come after us.
Consider adding a docstring explaining the purpose and impact of the Swagger schema generation:
# Generate OpenAPI/Swagger documentation for the Patient OTP endpoints # This enhances API discoverability and helps maintain API documentation PatientOTPView.generate_swagger_schema()care/emr/api/viewsets/medication_request.py (1)
45-45: Another schema generation without documentation.I notice we're following the same pattern as other files, which is... consistent, I suppose. But since this viewset handles medication requests, which are just slightly important for patient care, perhaps we could add some documentation about what this schema generation accomplishes?
Consider adding a descriptive comment:
# Generate OpenAPI/Swagger documentation for medication request endpoints # This ensures API consumers can understand the expected request/response formats MedicationRequestViewSet.generate_swagger_schema()care/emr/api/viewsets/batch_request.py (1)
30-32: LGTM! Though you could make the schema even more helpful...The
@extend_schemadecorator is correctly implemented, but consider adding a description and examples to make the API documentation more user-friendly.@extend_schema( request=BatchRequest, + description="Process multiple API requests in a single transaction", + examples=[{ + "summary": "Example batch request", + "value": { + "requests": [{ + "url": "/api/v1/endpoint", + "method": "POST", + "body": {"key": "value"}, + "reference_id": "req-1" + }] + } + }] )care/emr/api/viewsets/scheduling/availability_exceptions.py (1)
74-76: I see you're quite generous with those newlines...While the
generate_swagger_schema()call is correct, the extra newlines are unnecessary and inconsistent with other files in the codebase.- - AvailabilityExceptionsViewSet.generate_swagger_schema() +care/emr/api/viewsets/observation.py (1)
59-61: The schema looks good, but it could be more descriptive...The
@extend_schemadecorator is correctly implemented, but adding response schema and description would make the API documentation more complete.@extend_schema( request=ObservationAnalyseRequest, + responses={200: {"type": "object", "properties": {"results": {"type": "array"}}}}, + description="Analyze observations based on provided codes", )care/emr/api/viewsets/allergy_intolerance.py (1)
86-87: Consider adding schema definitions for all operations.While you've added Swagger schema generation, the viewset is missing schema definitions for list and retrieve operations. It would be absolutely wonderful if you could document these as well.
Add the following decorators to the class:
@extend_schema_view( create=extend_schema(request=AllergyIntoleranceSpec), + list=extend_schema(responses=AllergyIntrolanceSpecRead), + retrieve=extend_schema(responses=AllergyIntrolanceSpecRead), + update=extend_schema(request=AllergyIntoleranceWriteSpec, responses=AllergyIntrolanceSpecRead), )care/emr/api/otp_viewsets/login.py (1)
49-51: Add response schemas to improve API documentation.The schema decorators only specify request bodies. Adding response schemas would make the documentation slightly more complete.
Update the decorators to include response schemas:
@extend_schema( request=OTPLoginRequestSpec, + responses={200: dict}, ) @extend_schema( request=OTPLoginSpec, + responses={200: dict}, )Also applies to: 83-85
care/emr/api/otp_viewsets/slot.py (2)
43-45: Add response schemas for all actions.The schema decorators are missing response definitions. It would be absolutely delightful if you could document the response structures.
Add response schemas to the decorators:
@extend_schema( request=SlotsForDayRequestSpec, + responses={200: dict}, ) @extend_schema( request=AppointmentBookingSpec, + responses={200: dict}, ) @extend_schema( request=CancelAppointmentSpec, + responses={200: dict}, )Also applies to: 53-55, 67-69
Line range hint
85-96: Add schema decorator for get_appointments action.The get_appointments action is missing its schema definition. I'm sure it was just an oversight.
Add the schema decorator:
+ @extend_schema( + responses={200: dict}, + ) @action(detail=False, methods=["GET"]) def get_appointments(self, request, *args, **kwargs):care/emr/api/viewsets/resource_request.py (1)
76-77: Add schema decorators for all viewset operations.Both viewsets are missing schema decorators for their operations. Adding them would make the API documentation just a tad more useful.
Add schema decorators to both viewsets:
+@extend_schema_view( + list=extend_schema(responses=ResourceRequestListSpec), + retrieve=extend_schema(responses=ResourceRequestRetrieveSpec), + create=extend_schema(request=ResourceRequestCreateSpec, responses=ResourceRequestRetrieveSpec), +) class ResourceRequestViewSet(EMRModelViewSet): +@extend_schema_view( + list=extend_schema(responses=ResourceRequestCommentListSpec), + retrieve=extend_schema(responses=ResourceRequestCommentRetrieveSpec), + create=extend_schema(request=ResourceRequestCommentCreateSpec, responses=ResourceRequestCommentRetrieveSpec), +) class ResourceRequestCommentViewSet(Also applies to: 104-105
care/emr/api/viewsets/condition.py (1)
86-86: Consider adding @extend_schema decorators to methods.While adding
generate_swagger_schema()is a good start, you might want to add@extend_schemadecorators to the viewset methods for more detailed API documentation... you know, like you did in the other files. Just saying.Also applies to: 127-128
care/emr/api/viewsets/file_upload.py (1)
124-125: Move ArchiveRequestSpec outside the viewset class.The
ArchiveRequestSpecclass should be moved outside the viewset for better code organization. It's generally not the best practice to nest model classes inside viewsets, but I suppose you already knew that.care/emr/api/viewsets/patient.py (1)
82-84: Consider moving request spec classes outside the viewset.The request spec classes (
SearchRequestSpec,SearchRetrieveRequestSpec,PatientUserCreateSpec,PatientUserDeleteSpec) should be moved outside the viewset class. It would make the code a tiny bit more organized, but it's your call.Also applies to: 106-108, 134-135, 151-151
care/emr/api/viewsets/facility.py (1)
Line range hint
115-132: Add @extend_schema decorators to cover_image methods.The
cover_imageandcover_image_deletemethods could use some@extend_schemadecorators to document their request/response types. I mean, if we're adding Swagger support, might as well do it thoroughly, right?care/emr/api/viewsets/scheduling/schedule.py (2)
132-132: Consider adding @extend_schema decorators to document endpoint parameters.While generating the Swagger schema is good, the endpoints could benefit from more detailed documentation using
@extend_schemadecorators, especially for methods likeperform_createandperform_updatethat handle complex operations.
199-199: LGTM, but consider documenting the available filters.The schema generation is correctly implemented, but it might be helpful to document the available filters for the viewset using OpenAPI parameters.
care/emr/api/viewsets/questionnaire.py (2)
139-142: Well-documented schema, but response could be more specific.The schema definition is good, but consider adding examples or a more detailed description of the expected response structure.
189-191: Nice schema definitions, but missing response specifications.While the request schemas are properly defined, you might want to add response schemas to
set_tagsandset_organizationsmethods... you know, for completeness.Also applies to: 210-212
care/emr/api/viewsets/base.py (1)
265-285: Excellent base implementation, but could be more flexible.The schema generation is well-implemented, but consider adding support for custom response codes and error responses. You know, because things don't always go as planned.
Consider adding error responses like this:
@classmethod def generate_swagger_schema(cls): return extend_schema_view( list=extend_schema( responses={ }, ), # ... rest of the methods )(cls)care/emr/api/viewsets/encounter.py (2)
178-181: Good schema definitions, but response schema could be more detailed.The schema definitions are properly implemented, but consider adding more detailed response schemas, especially for error cases.
Also applies to: 202-204
Line range hint
205-223: Using POST for removal operation is... interesting.While it works, using POST for a removal operation is somewhat unconventional. DELETE would be more RESTful here, even if it requires a request body.
Consider keeping it as DELETE and handling the request body, or if you must use POST, consider renaming to something like
remove_organizationsto make the intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
care/emr/api/otp_viewsets/login.py(4 hunks)care/emr/api/otp_viewsets/patient.py(1 hunks)care/emr/api/otp_viewsets/slot.py(4 hunks)care/emr/api/viewsets/allergy_intolerance.py(1 hunks)care/emr/api/viewsets/base.py(2 hunks)care/emr/api/viewsets/batch_request.py(2 hunks)care/emr/api/viewsets/condition.py(2 hunks)care/emr/api/viewsets/encounter.py(5 hunks)care/emr/api/viewsets/facility.py(4 hunks)care/emr/api/viewsets/facility_organization.py(2 hunks)care/emr/api/viewsets/file_upload.py(4 hunks)care/emr/api/viewsets/medication_administration.py(1 hunks)care/emr/api/viewsets/medication_request.py(1 hunks)care/emr/api/viewsets/medication_statement.py(1 hunks)care/emr/api/viewsets/notes.py(2 hunks)care/emr/api/viewsets/observation.py(3 hunks)care/emr/api/viewsets/organization.py(3 hunks)care/emr/api/viewsets/patient.py(6 hunks)care/emr/api/viewsets/questionnaire.py(6 hunks)care/emr/api/viewsets/questionnaire_response.py(1 hunks)care/emr/api/viewsets/resource_request.py(2 hunks)care/emr/api/viewsets/roles.py(1 hunks)care/emr/api/viewsets/scheduling/availability_exceptions.py(1 hunks)care/emr/api/viewsets/scheduling/booking.py(1 hunks)care/emr/api/viewsets/scheduling/schedule.py(2 hunks)care/emr/api/viewsets/user.py(1 hunks)care/emr/api/viewsets/valueset.py(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- care/emr/api/viewsets/valueset.py
- care/emr/api/viewsets/scheduling/booking.py
- care/emr/api/viewsets/questionnaire_response.py
- care/emr/api/viewsets/user.py
- care/emr/api/viewsets/organization.py
- care/emr/api/viewsets/notes.py
- care/emr/api/viewsets/facility_organization.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (10)
care/emr/api/viewsets/roles.py (1)
Line range hint
1-1: That TODO comment has been quite patient.The TODO comment suggests moving this to Security APIs. Since we're touching this file anyway, perhaps now would be an excellent time to address that?
Would you like me to help create a new issue to track the migration of this file to the Security APIs?
care/emr/api/viewsets/medication_statement.py (1)
43-43: Interesting placement choice.I see we're generating the schema before registering with the
InternalQuestionnaireRegistry. While I'm sure there's a good reason for this order, it might be worth documenting why, especially since we're dealing with medication statements which are, you know, somewhat important.Could you confirm if the order of these operations is significant? If not, consider grouping related operations with a comment:
# Generate API documentation and register viewset MedicationStatementViewSet.generate_swagger_schema() InternalQuestionnaireRegistry.register(MedicationStatementViewSet)care/emr/api/viewsets/medication_administration.py (1)
46-46: Looks good, schema generation is properly placed.The
generate_swagger_schema()call is correctly positioned before the registry registration.care/emr/api/viewsets/observation.py (1)
87-87: Schema generation looks good.The
generate_swagger_schema()call is correctly placed after the class definition.care/emr/api/viewsets/file_upload.py (1)
114-115: LGTM! Well-documented schema decorators.The
@extend_schemadecorators are properly implemented with appropriate request and response types.Also applies to: 126-129
care/emr/api/viewsets/patient.py (1)
85-87: LGTM! Comprehensive schema documentation.The
@extend_schemadecorators are well-implemented with proper request and response types for all endpoints.Also applies to: 109-111, 136-136, 152-152
care/emr/api/viewsets/facility.py (1)
134-134: LGTM! Consistent schema generation.The
generate_swagger_schema()calls are properly added to all viewsets.Also applies to: 151-151, 172-172, 192-192
care/emr/api/viewsets/questionnaire.py (1)
252-252: LGTM!The schema generation is properly implemented.
care/emr/api/viewsets/encounter.py (2)
294-296: LGTM!The email schema definition with validation is well implemented.
338-338: LGTM!The schema generation is properly implemented.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2772 +/- ##
===========================================
+ Coverage 64.90% 65.16% +0.25%
===========================================
Files 252 253 +1
Lines 12761 12855 +94
Branches 1121 1121
===========================================
+ Hits 8283 8377 +94
Misses 4369 4369
Partials 109 109 ☔ View full report in Codecov by Sentry. |
|
Can we make this as a decorator? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
care/utils/decorators/schema_decorator.py (3)
4-4: Consider adding a docstring for clarity.
A brief docstring explaining the purpose and usage ofgenerate_swagger_schema_decoratorwould help future maintainers quickly understand its role.
7-11: Specify common error response schemas.
While the decorator covers the success response (200), it might be beneficial to add 400 (Bad Request) or 403 (Forbidden) responses to document typical error scenarios.
12-18: Include support for partial updates or additional methods if needed.
Since “update” is specified, you might consider including “partial_update” for PATCH requests or a dictionary entry for “destroy.” This would provide a comprehensive schema for all relevant CRUD operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
care/emr/api/otp_viewsets/login.py(3 hunks)care/emr/api/otp_viewsets/patient.py(1 hunks)care/emr/api/otp_viewsets/slot.py(4 hunks)care/emr/api/viewsets/allergy_intolerance.py(1 hunks)care/emr/api/viewsets/condition.py(3 hunks)care/emr/api/viewsets/encounter.py(6 hunks)care/emr/api/viewsets/facility.py(5 hunks)care/emr/api/viewsets/facility_organization.py(3 hunks)care/emr/api/viewsets/file_upload.py(5 hunks)care/emr/api/viewsets/medication_administration.py(2 hunks)care/emr/api/viewsets/medication_request.py(1 hunks)care/emr/api/viewsets/medication_statement.py(1 hunks)care/emr/api/viewsets/notes.py(2 hunks)care/emr/api/viewsets/observation.py(4 hunks)care/emr/api/viewsets/organization.py(4 hunks)care/emr/api/viewsets/patient.py(6 hunks)care/emr/api/viewsets/questionnaire.py(6 hunks)care/emr/api/viewsets/questionnaire_response.py(2 hunks)care/emr/api/viewsets/resource_request.py(2 hunks)care/emr/api/viewsets/roles.py(1 hunks)care/emr/api/viewsets/scheduling/availability.py(2 hunks)care/emr/api/viewsets/scheduling/availability_exceptions.py(1 hunks)care/emr/api/viewsets/scheduling/booking.py(2 hunks)care/emr/api/viewsets/scheduling/schedule.py(2 hunks)care/emr/api/viewsets/user.py(2 hunks)care/emr/api/viewsets/valueset.py(2 hunks)care/utils/decorators/schema_decorator.py(1 hunks)config/settings/base.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- care/emr/api/viewsets/scheduling/availability.py
🚧 Files skipped from review as they are similar to previous changes (25)
- care/emr/api/viewsets/user.py
- care/emr/api/viewsets/medication_request.py
- care/emr/api/viewsets/valueset.py
- care/emr/api/otp_viewsets/patient.py
- care/emr/api/viewsets/roles.py
- care/emr/api/viewsets/scheduling/booking.py
- care/emr/api/viewsets/medication_administration.py
- care/emr/api/viewsets/scheduling/availability_exceptions.py
- care/emr/api/viewsets/medication_statement.py
- care/emr/api/viewsets/notes.py
- care/emr/api/viewsets/condition.py
- care/emr/api/viewsets/resource_request.py
- care/emr/api/viewsets/scheduling/schedule.py
- care/emr/api/viewsets/allergy_intolerance.py
- care/emr/api/viewsets/organization.py
- care/emr/api/viewsets/facility.py
- care/emr/api/viewsets/facility_organization.py
- care/emr/api/viewsets/file_upload.py
- care/emr/api/viewsets/observation.py
- care/emr/api/otp_viewsets/login.py
- care/emr/api/viewsets/questionnaire_response.py
- care/emr/api/viewsets/encounter.py
- care/emr/api/viewsets/questionnaire.py
- care/emr/api/otp_viewsets/slot.py
- care/emr/api/viewsets/patient.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test / test
🔇 Additional comments (2)
care/utils/decorators/schema_decorator.py (1)
24-24: Review the final return for consistency.
Callingextend_schema_view(**schema_dict)(cls)returns a decorated class. Nothing critical here—just ensure all named methods declared inschema_dictactually exist incls.config/settings/base.py (1)
390-390: Have you thought about the repercussions of suppressing all errors and warnings?
While it's tempting to silence warnings for cleaner logs, doing so might conceal important issues that need attention. It’s generally advisable to keep these enabled in non-production environments for improved visibility and troubleshooting.- "DISABLE_ERRORS_AND_WARNINGS": True, + "DISABLE_ERRORS_AND_WARNINGS": False,
| elif name in ["list", "retrieve"]: | ||
| schema_dict[name] = extend_schema( | ||
| responses={200: cls.pydantic_read_model or cls.pydantic_model} | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document query parameters for “list” and “retrieve.”
If the “list” or “retrieve” methods accept query/string parameters, consider extending the schema to detail them. This ensures thorough API documentation.
| "responses": {200: cls.pydantic_read_model or cls.pydantic_model}, | ||
| }, | ||
| "list": {"responses": {200: cls.pydantic_read_model or cls.pydantic_model}}, | ||
| "retrieve": {"responses": {200: cls.pydantic_read_model or cls.pydantic_model}}, |
There was a problem hiding this comment.
retrieve model gets first priority here
| "responses": {200: cls.pydantic_read_model or cls.pydantic_model}, | ||
| }, | ||
| "update": { | ||
| "request": cls.pydantic_retrieve_model |
There was a problem hiding this comment.
update model gets first priority here
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
Documentation
New Features
generate_swagger_schema_decoratorto automatically generate Swagger schemasChores