Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a POST 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.32% 75.26% -0.07%
===========================================
Files 473 473
Lines 22066 22112 +46
Branches 2305 2310 +5
===========================================
+ Hits 16622 16642 +20
- Misses 4926 4952 +26
Partials 518 518 ☔ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
care/emr/api/viewsets/report/report_upload.py (3)
155-161: Missing template status/active check for preview.The
generateaction validatestemplate.status != "active"(lines 124–125). Thepreviewaction skips this entirely. If previewing inactive/draft templates is intentional, a comment documenting that would be helpful for the next person reading this code.🤖 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 155 - 161, The preview action (preview method) is missing the same Template status check that generate enforces (generate checks template.status != "active"); either add the same guard in preview to raise 404 or permission error when Template.status is not "active" (using the Template instance fetched via get_object_or_404) or, if previewing inactive templates is intentional, add a clear inline comment above preview explaining that difference and why inactive/draft templates are allowed; reference the preview method and the generate method's status check to keep behavior consistent or documented.
205-206: Blanketexcept Exceptionstill swallows all errors indiscriminately.
ValidationErrorandPermissionDeniedraised inside thetryblock (e.g., line 189) get caught here and re-raised as a genericValidationError("Preview generation failed"), losing the original meaningful message. Programming bugs (TypeError,AttributeError, etc.) are also silently wrapped. At minimum, re-raise DRF exceptions and log the unexpected ones.Proposed fix
- except Exception as e: - raise ValidationError("Preview generation failed") from e + except (ValidationError, PermissionDenied): + raise + except Exception as e: + logger.exception("Preview generation failed") + raise ValidationError("Preview generation failed") from e🤖 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 205 - 206, The current blanket except catches DRF exceptions and hides their messages by always raising ValidationError("Preview generation failed"); update the except block that now reads "except Exception as e: raise ValidationError('Preview generation failed') from e" to instead detect and re-raise DRF exceptions (e.g., ValidationError, PermissionDenied) using isinstance checks and plain raise, and for all other exceptions call logger.exception("Preview generation failed") (use logger = logging.getLogger(__name__) if not present) before raising a new ValidationError("Preview generation failed") from e so unexpected errors are logged but DRF errors keep their original messages.
168-171:⚠️ Potential issue | 🟠 Major
ReportTypeRegistry.get()raisesKeyError, never returnsNone— this guard is dead code and the real error is unhandled.Per the
ReportTypeRegistry.get()implementation (seecare/emr/reports/report_type_registry.py, lines 52–56), it raisesKeyErrorwhen the key is not found — it does not returnNone. So theif report_type_config is Nonecheck on line 169 will never trigger. Meanwhile, these lines sit outside thetryblock (line 178), so theKeyErrorwill propagate as an unhandled 500 error rather than a clean 400.Proposed fix
- report_type_config = ReportTypeRegistry.get(template.template_type) - if report_type_config is None: - error_msg = f"Report Type '{template.template_type}' not found in ReportTypeRegistry" - raise ValidationError(error_msg) + try: + report_type_config = ReportTypeRegistry.get(template.template_type) + except KeyError: + raise ValidationError( + f"Report type '{template.template_type}' is not registered" + )🤖 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 168 - 171, The code assumes ReportTypeRegistry.get(...) can return None but it actually raises KeyError; replace the dead None-check by catching KeyError around the call: wrap ReportTypeRegistry.get(template.template_type) in a try/except KeyError, and in the except raise a ValidationError with the same error_msg (e.g., "Report Type '...' not found in ReportTypeRegistry"); remove the unnecessary if report_type_config is None branch so the KeyError is converted to a 400 ValidationError instead of a 500.
🧹 Nitpick comments (1)
care/emr/api/viewsets/report/report_upload.py (1)
199-203: Synchronous rendering on the request thread — consider timeout or size guardrails.Unlike
generate, which delegates to a Celery task,previewperforms the full render cycle synchronously. For complex templates or large datasets, this could block the request thread for an extended period. A rendering timeout or a content-size limit might be worth considering to prevent slow clients from tying up workers.🤖 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 199 - 203, The preview path performs synchronous rendering via Renderer(generator).render(...) and then returns generator.get_http_response(...), which can block the request thread; update this code to enforce render guardrails by (a) pre-checking input size (e.g., template.template_data length and any large collections in validated_options/context) and rejecting or truncating requests above a configured MAX_PREVIEW_PAYLOAD, and (b) executing the render call with a timeout (e.g., run Renderer(...).render(...) in a short-lived worker/future or delegated Celery/worker task and raise a TimeoutError if it exceeds MAX_PREVIEW_SECONDS), then return an appropriate error response instead of blocking; keep references to Renderer and generator.get_http_response so the change targets this synchronous preview flow.
🤖 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 163-166: The preview action calls
AuthorizationController.call("can_preview_report_from_template", request.user,
template.facility) without guarding for template.facility being None, which can
pass a None facility into check_permission_in_facility_organization; change the
preview action to mirror the generate action by only performing the permission
check when template.facility is truthy (i.e., wrap the
AuthorizationController.call and subsequent PermissionDenied raise in an if
template.facility and not AuthorizationController.call(...) block), keeping the
same error message (PermissionDenied("You do not have permission to preview
reports")).
In `@care/security/authorization/template.py`:
- Around line 56-64: The can_preview_report_from_template method is using the
overly-broad TemplatePermissions.can_generate_report_from_template.name; replace
that permission with the dedicated, more-restrictive
TemplatePermissions.can_preview_template.name in the call to
check_permission_in_facility_organization inside
can_preview_report_from_template (or if broad access is intended, rename the
method to reflect generation rights and remove duplication) so the permission
check matches other preview logic (see TemplatePermissions.can_preview_template
and can_preview_report_from_template).
---
Duplicate comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Around line 155-161: The preview action (preview method) is missing the same
Template status check that generate enforces (generate checks template.status !=
"active"); either add the same guard in preview to raise 404 or permission error
when Template.status is not "active" (using the Template instance fetched via
get_object_or_404) or, if previewing inactive templates is intentional, add a
clear inline comment above preview explaining that difference and why
inactive/draft templates are allowed; reference the preview method and the
generate method's status check to keep behavior consistent or documented.
- Around line 205-206: The current blanket except catches DRF exceptions and
hides their messages by always raising ValidationError("Preview generation
failed"); update the except block that now reads "except Exception as e: raise
ValidationError('Preview generation failed') from e" to instead detect and
re-raise DRF exceptions (e.g., ValidationError, PermissionDenied) using
isinstance checks and plain raise, and for all other exceptions call
logger.exception("Preview generation failed") (use logger =
logging.getLogger(__name__) if not present) before raising a new
ValidationError("Preview generation failed") from e so unexpected errors are
logged but DRF errors keep their original messages.
- Around line 168-171: The code assumes ReportTypeRegistry.get(...) can return
None but it actually raises KeyError; replace the dead None-check by catching
KeyError around the call: wrap ReportTypeRegistry.get(template.template_type) in
a try/except KeyError, and in the except raise a ValidationError with the same
error_msg (e.g., "Report Type '...' not found in ReportTypeRegistry"); remove
the unnecessary if report_type_config is None branch so the KeyError is
converted to a 400 ValidationError instead of a 500.
---
Nitpick comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Around line 199-203: The preview path performs synchronous rendering via
Renderer(generator).render(...) and then returns
generator.get_http_response(...), which can block the request thread; update
this code to enforce render guardrails by (a) pre-checking input size (e.g.,
template.template_data length and any large collections in
validated_options/context) and rejecting or truncating requests above a
configured MAX_PREVIEW_PAYLOAD, and (b) executing the render call with a timeout
(e.g., run Renderer(...).render(...) in a short-lived worker/future or delegated
Celery/worker task and raise a TimeoutError if it exceeds MAX_PREVIEW_SECONDS),
then return an appropriate error response instead of blocking; keep references
to Renderer and generator.get_http_response so the change targets this
synchronous preview flow.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
care/emr/api/viewsets/report/report_upload.pycare/emr/reports/authorizers/encounter.pycare/security/authorization/encounter.pycare/security/authorization/template.py
…ohcnetwork/care into nandkishorr/report_live_preview
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
care/emr/api/viewsets/report/report_upload.py (2)
155-161: No template status check — still unaddressed from prior review.The
generateaction (line 124) validatestemplate.status != "active", butpreviewdoesn't check status at all. If previewing non-active templates is intentional (e.g., for draft design work), a brief comment explaining this would save future readers from wondering whether it was simply… overlooked.🤖 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 155 - 161, The preview action fetches a Template without checking its status while the generate action explicitly rejects non-active templates (see generate's template.status != "active" check); either add the same status validation to preview (raise 404 or PermissionDenied for non-active templates) or, if previewing drafts is intended, add a concise explanatory comment in the preview method documenting that preview intentionally allows non-active templates and why—update the preview function around the get_object_or_404(Template, external_id=...) call to enforce the chosen behavior and reference Template, preview, and the existing generate status check for consistency.
205-206: Blanketexcept Exceptionstill swallows specificValidationErrormessages raised within thetryblock.The
ValidationErroron line 189 (missing context) and anypydantic.ValidationErroron line 182 (invalid options) are caught here and replaced with the wonderfully descriptive"Preview generation failed". At minimum, re-raiseValidationErrorandPermissionDeniedbefore catching the rest, so callers get actionable error messages instead of having to guess what went wrong.🔧 Proposed fix — let known exceptions through
- except Exception as e: - raise ValidationError("Preview generation failed") from e + except (ValidationError, PermissionDenied): + raise + except Exception as e: + logger.exception("Preview generation failed") + raise ValidationError("Preview generation failed") from e🤖 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 205 - 206, The current broad except block swallows specific ValidationError and PermissionDenied (and pydantic.ValidationError) raised earlier; update the exception handling in the preview generation try/except so known exceptions are re-raised unchanged (e.g., explicitly detect and re-raise ValidationError, PermissionDenied, and pydantic.ValidationError) and only convert other unexpected exceptions into the generic ValidationError("Preview generation failed") from e; locate the except block that currently reads "except Exception as e:" in report_upload.py and replace it with logic that re-raises those specific exception types first, then handles all other exceptions by raising the generic preview-failed ValidationError.
🧹 Nitpick comments (1)
care/emr/api/viewsets/report/report_upload.py (1)
199-203: Synchronous rendering on the request thread — consider timeout or size guard.Unlike
generate, which dispatches to Celery,previewrenders the full report synchronously. For complex templates or large datasets, this could block the request thread for a noticeable duration. It might be worth adding a timeout or limiting preview to lighter-weight formats (e.g., HTML only) to avoid tying up workers. Just something to keep in mind as templates grow in complexity.🤖 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 199 - 203, Synchronous preview rendering blocks the request thread; update the preview path (the code calling Renderer(generator).render and generator.get_http_response) to enforce a guard: either restrict preview to lightweight formats (e.g., only allow 'html' in validated_options and return 400 for others) or run the render in a bounded worker with a timeout (e.g., submit Renderer(generator).render(template.template_data, context, validated_options) to a ThreadPoolExecutor/ProcessPoolExecutor or task queue and raise/handle TimeoutError if it exceeds X seconds), and enforce a max output size check before calling generator.get_http_response so oversized previews are rejected with a clear error/HTTP status.
🤖 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 168-171: Replace the dead None-check around
ReportTypeRegistry.get(template.template_type) with a try/except KeyError: call
ReportTypeRegistry.get inside a try block, catch KeyError and raise a
ValidationError with the current error_msg (e.g., "Report Type
'{template.template_type}' not found in ReportTypeRegistry"); remove the
unreachable if report_type_config is None branch so the code uses the registry's
actual KeyError behavior and surfaces a clear ValidationError instead of
allowing the later broad except Exception to mask the failure.
---
Duplicate comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Around line 155-161: The preview action fetches a Template without checking
its status while the generate action explicitly rejects non-active templates
(see generate's template.status != "active" check); either add the same status
validation to preview (raise 404 or PermissionDenied for non-active templates)
or, if previewing drafts is intended, add a concise explanatory comment in the
preview method documenting that preview intentionally allows non-active
templates and why—update the preview function around the
get_object_or_404(Template, external_id=...) call to enforce the chosen behavior
and reference Template, preview, and the existing generate status check for
consistency.
- Around line 205-206: The current broad except block swallows specific
ValidationError and PermissionDenied (and pydantic.ValidationError) raised
earlier; update the exception handling in the preview generation try/except so
known exceptions are re-raised unchanged (e.g., explicitly detect and re-raise
ValidationError, PermissionDenied, and pydantic.ValidationError) and only
convert other unexpected exceptions into the generic ValidationError("Preview
generation failed") from e; locate the except block that currently reads "except
Exception as e:" in report_upload.py and replace it with logic that re-raises
those specific exception types first, then handles all other exceptions by
raising the generic preview-failed ValidationError.
---
Nitpick comments:
In `@care/emr/api/viewsets/report/report_upload.py`:
- Around line 199-203: Synchronous preview rendering blocks the request thread;
update the preview path (the code calling Renderer(generator).render and
generator.get_http_response) to enforce a guard: either restrict preview to
lightweight formats (e.g., only allow 'html' in validated_options and return 400
for others) or run the render in a bounded worker with a timeout (e.g., submit
Renderer(generator).render(template.template_data, context, validated_options)
to a ThreadPoolExecutor/ProcessPoolExecutor or task queue and raise/handle
TimeoutError if it exceeds X seconds), and enforce a max output size check
before calling generator.get_http_response so oversized previews are rejected
with a clear error/HTTP status.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/security/permissions/template.py`:
- Around line 76-90: can_preview_report_from_template duplicates
can_generate_report_from_template by granting the same 8 roles, so either remove
the redundant permission or change its role set to actually restrict previewing;
specifically update can_preview_report_from_template to mirror the intended
policy (e.g., make it admin-only like can_preview_template by limiting roles to
FACILITY_ADMIN_ROLE and ADMINISTRATOR or another smaller admin role list) OR
document in a comment that preview and generate are intentionally identical and
delete the separate permission to avoid confusion; reference
can_generate_report_from_template and can_preview_template when applying the
change.
This reverts commit 56b8379.
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
New Features
Security / Permissions