Open
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
care/emr/api/viewsets/report/report_upload.py (1)
34-34:CeleryTaskErrorimport is unnecessary if the fix above is applied.If you replace the
CeleryTaskErrorusage on Line 176 withValidationError(as suggested), this import becomes unused and can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/api/viewsets/report/report_upload.py` at line 34, Remove the now-unused import CeleryTaskError from the top import list (the symbol CeleryTaskError) since the code path that used it was changed to raise ValidationError instead; update the imports to only include ValidationError (and any other remaining exceptions), ensuring there are no leftover unused imports referencing CeleryTaskError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Around line 156-158: The preview action currently omits the template.active
status validation present in the generate action (the check that fails when
template.status != "active"); either add the same status guard to the preview
handler (reject non-active templates the same way as generate) or, if previewing
drafts is intentional, add an explicit comment in the preview function noting
that skipping the template.status != "active" check is deliberate so designers
can preview non-active templates; reference the preview action and the generate
action's template.status check when making the change.
- Around line 178-182: Replace the ValueError raised when
ReportTypeRegistry.get(template.template_type) fails with a DRF ValidationError
so the API returns a 4xx client error; in the try/except around
ReportTypeRegistry.get (the block handling template.template_type) raise
rest_framework.exceptions.ValidationError (or the project's
serializers.ValidationError alias) with the same error_msg and update imports as
needed.
- Around line 156-200: In the preview action, fix three issues: (1) when
request_data.output_format is None, fallback to the template's default_format
like the generate action by resolving template before validating output_format
and using template.default_format if needed (look for GenerateReportRequest and
preview); (2) guard DataPointRegistry.get(template.context) for None before
accessing .context_key and raise a ValidationError with a clear message if the
context slug isn't registered (refer to DataPointRegistry.get and context_key
usage); (3) replace raising CeleryTaskError in the Template.DoesNotExist handler
with an appropriate DRF exception (ValidationError or Http404) and stop using a
blanket except Exception at the end — instead catch and re-raise known DRF
exceptions (e.g., ValidationError, PermissionDenied) and only catch unexpected
exceptions to log them and return a concise ValidationError, so real programming
errors aren't swallowed (refer to Template.DoesNotExist handling and the outer
try/except in preview).
---
Nitpick comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Line 34: Remove the now-unused import CeleryTaskError from the top import list
(the symbol CeleryTaskError) since the code path that used it was changed to
raise ValidationError instead; update the imports to only include
ValidationError (and any other remaining exceptions), ensuring there are no
leftover unused imports referencing CeleryTaskError.
Comment on lines
+156
to
+158
| @extend_schema( | ||
| request=GenerateReportRequest, responses={200: "Success"}, tags=["report"] | ||
| ) |
Contributor
There was a problem hiding this comment.
Missing template status check for preview.
The generate action validates template.status != "active" (Line 125-126) before proceeding. The preview action skips this check entirely. This could allow previewing inactive or draft templates. If that's the intended behavior (designers previewing drafts), that's fine — but it's worth being explicit about it with a comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@care/emr/api/viewsets/report/report_upload.py` around lines 156 - 158, The
preview action currently omits the template.active status validation present in
the generate action (the check that fails when template.status != "active");
either add the same status guard to the preview handler (reject non-active
templates the same way as generate) or, if previewing drafts is intentional, add
an explicit comment in the preview function noting that skipping the
template.status != "active" check is deliberate so designers can preview
non-active templates; reference the preview action and the generate action's
template.status check when making the change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3536 +/- ##
===========================================
- Coverage 75.45% 75.34% -0.11%
===========================================
Files 473 473
Lines 21977 22015 +38
Branches 2293 2296 +3
===========================================
+ Hits 16582 16587 +5
- Misses 4877 4909 +32
- Partials 518 519 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
care/emr/api/viewsets/report/report_upload.py (1)
180-182:except Exceptionis broader than necessary — should beexcept KeyError
ReportTypeRegistry.get()raisesKeyErrorwhen a type isn't registered. CatchingExceptionhere also inadvertently masks unrelated runtime errors (e.g., anAttributeErrorinsideget()itself). This is also moot when the outerexcept Exceptionswallows everything anyway (see below), but it's worth fixing regardless.♻️ Proposed fix
- except Exception as err: + except KeyError as err: error_msg = f"Report Type '{template.template_type}' not found in ReportTypeRegistry" raise ValidationError(error_msg) from err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/emr/api/viewsets/report/report_upload.py` around lines 180 - 182, The except block catching Exception around the call to ReportTypeRegistry.get() should be narrowed to except KeyError so only missing-report-type errors are handled; update the handler that currently builds error_msg using template.template_type and raises ValidationError to catch KeyError (from ReportTypeRegistry.get) and preserve other exceptions (i.e., do not swallow them), keeping the original error context when raising ValidationError from the caught KeyError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Line 161: Update the user-facing permission denial message in the raise
PermissionDenied(...) call to fix the typo: change "You dont have permission to
preview reports" to include the apostrophe ("don't"); locate the raise
PermissionDenied(...) in report_upload.py (the permission check/preview report
path) and replace the string accordingly.
---
Duplicate comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Around line 184-185: DataPointRegistry.get(template.context) can return None,
causing an AttributeError when accessing context_class.context_key; update the
code around the lookup (the call to DataPointRegistry.get, the context_class
variable and subsequent use of context_key) to guard against None by checking if
context_class is truthy and handling the unregistered slug explicitly (e.g.,
raise a clear error or return/skip with a descriptive message referencing
template.context) before accessing context_class.context_key so the outer
exception handler does not mask the real cause.
- Around line 165-166: The preview path incorrectly rejects requests where
request_data.output_format is None because
GeneratorRegistry.is_registered(request_data.output_format) returns False;
update the preview handling to mirror generate by falling back to
template.default_format when output_format is falsy (or perform the same
conditional used in GenerateReportRequest.validate_output_format: only call
GeneratorRegistry.is_registered when v is truthy). Locate the preview action and
the check using GeneratorRegistry.is_registered and change it to use
request_data.output_format or template.default_format (or guard the check with
"if v and ...") so None input uses template.default_format and avoids the
misleading ValidationError.
- Around line 168-200: The outer except Exception in the report preview flow is
catching and replacing specific DRF ValidationError messages raised earlier
(e.g., Template.DoesNotExist -> ValidationError in the Template lookup,
ReportTypeRegistry.get -> ValidationError) with a generic "Preview generation
failed"; change the exception handling in the viewset so that inside the
try/except around
GeneratorRegistry/Template/ReportTypeRegistry/DataPointRegistry/Renderer you
first re-raise known DRF/API exceptions (e.g., ValidationError, APIException)
unchanged, and only convert unexpected exceptions into a new ValidationError;
specifically adjust the except block that wraps GeneratorRegistry,
Template.objects.get, ReportTypeRegistry.get, DataPointRegistry.get,
Renderer(generator).render and generator.get_http_response to check for
isinstance(e, (ValidationError, APIException)) and re-raise it, otherwise raise
ValidationError("Preview generation failed") from e so that the precise messages
from Template/ReportType errors are preserved.
---
Nitpick comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Around line 180-182: The except block catching Exception around the call to
ReportTypeRegistry.get() should be narrowed to except KeyError so only
missing-report-type errors are handled; update the handler that currently builds
error_msg using template.template_type and raises ValidationError to catch
KeyError (from ReportTypeRegistry.get) and preserve other exceptions (i.e., do
not swallow them), keeping the original error context when raising
ValidationError from the caught KeyError.
Member
|
No AuthZ present |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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