Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughConsolidates CSV/JSON event export into a single parameterized /export/{export_format} endpoint; replaces admin username parameters with user_id across admin settings/users; wraps several list responses in container models; migrates select dataclasses to Pydantic BaseModel; standardizes response construction via model_validate. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Admin Events Route
participant Service as AdminEventsService
participant Repo as EventsRepository
Client->>API: GET /api/v1/admin/events/export/{export_format}?filters...
API->>Service: export_events(event_filter, limit, export_format)
Service->>Repo: fetch_events(event_filter, limit)
Repo-->>Service: events list
alt export_format == "csv"
Service->>Service: build CSV rows, generate bytes/stream
else export_format == "json"
Service->>Service: serialize events to JSON bytes/stream
end
Service-->>API: ExportResult (stream, filename, content-type)
API-->>Client: StreamingResponse (file download)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 39 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="backend/app/services/notification_service.py">
<violation number="1" location="backend/app/services/notification_service.py:546">
P2: Webhook URL validation is skipped when callers update only `webhook_url` (enabled is None), so invalid URLs can now be saved. Validate the URL whenever a webhook URL is provided, not only when `enabled` is true.</violation>
<violation number="2" location="backend/app/services/notification_service.py:551">
P2: Slack webhook validation is skipped when callers update only `slack_webhook` (enabled is None), allowing invalid URLs to be stored. Validate the Slack webhook whenever it is provided.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/openapi.json (1)
2080-2248:⚠️ Potential issue | 🟠 MajorDocument the export payload properly (current schema is empty).
The 200 response has an empty schema and only
application/json, which makes client generation and content negotiation ambiguous for a downloadable CSV/JSON file. Please describe the payload as a binary file and add the appropriate content types (e.g.,text/csvandapplication/jsonorapplication/octet-stream).💡 Suggested OpenAPI fix
"responses": { "200": { "description": "Successful Response", "content": { - "application/json": { - "schema": {} - } + "text/csv": { + "schema": { + "type": "string", + "format": "binary" + } + }, + "application/json": { + "schema": { + "type": "string", + "format": "binary" + } + } } },
🤖 Fix all issues with AI agents
In `@backend/app/services/notification_service.py`:
- Around line 546-555: The channel-specific URL validation incorrectly uses "if
... and update_data.enabled" which treats None as False and skips validation on
partial updates; update these checks in the notification_service (the block
using NotificationChannel, update_data, and NotificationValidationError) to run
when enabled is not explicitly False (e.g., change the condition to "if channel
== NotificationChannel.WEBHOOK and update_data.enabled is not False" and
similarly for "if channel == NotificationChannel.SLACK and update_data.enabled
is not False") so validation runs when enabled is True or omitted (None),
keeping the same error messages and checks for
webhook_url.startswith("https://") and
slack_webhook.startswith("https://hooks.slack.com/").
|
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
1 participant
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.
Summary by cubic
Standardized API responses with Pydantic model_validate/from_attributes and introduced typed list wrappers for scripts and subscriptions. Unified admin event export to /export/{format} and enforced strict notification URL validation; OpenAPI, SDK, and Editor are updated.
Refactors
Bug Fixes
Written for commit 6ebef43. Summary will update on new commits.
Summary by CodeRabbit
New Features
Changes
Validation