Shopify Order Cancellation lead management #368
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe PR introduces call abortion functionality to the BreezeBuddy API by adding a new POST Changes
Sequence DiagramsequenceDiagram
actor Client
participant Router as POST /call/abort
participant Handler as handle_call_abort
participant QueryGen as abort_call_by_lead_id_query
participant Database as DB
Client->>Router: CallAbortRequest(lead_id)
activate Router
Router->>Handler: handle_call_abort(lead_id)
activate Handler
Handler->>QueryGen: abort_call_by_lead_id_query(lead_id)
activate QueryGen
QueryGen-->>Handler: (UPDATE query, params)
deactivate QueryGen
Handler->>Database: Execute parameterized UPDATE
activate Database
Database-->>Handler: Result row or None
deactivate Database
alt Row found
Handler->>Handler: Decode LeadCallTracker<br/>Log success with customer_name, order_id
Handler-->>Router: LeadCallTracker
Router-->>Client: 200 {lead_id, customer_name, order_id}
else No pending call
Handler-->>Router: None
Router-->>Client: 404 Not Found
else Error
Handler->>Handler: Log exception
Handler-->>Router: None
Router-->>Client: 500 Server Error
end
deactivate Handler
deactivate Router
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
404-426: Consider more specific exception handling.While catching broad
Exceptionwith logging is acceptable, consider catching more specific exceptions (e.g., database-related exceptions) to distinguish between expected error conditions and unexpected bugs. However, this pattern is consistent with other functions in this file.app/schemas.py (1)
146-149: Consider adding field validation.The
CallAbortRequestmodel is functional, but could benefit from validation onlead_idto ensure it's non-empty and follows expected format patterns.Example enhancement:
from pydantic import Field class CallAbortRequest(BaseModel): """Request model for aborting a call by lead ID""" lead_id: str = Field(..., min_length=1, description="The unique identifier of the lead call to abort")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/api/routers/breeze_buddy.py(4 hunks)app/database/accessor/__init__.py(2 hunks)app/database/accessor/breeze_buddy/lead_call_tracker.py(1 hunks)app/database/queries/breeze_buddy/lead_call_tracker.py(2 hunks)app/schemas.py(2 hunks)scripts/create_tables.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/database/accessor/breeze_buddy/lead_call_tracker.py (5)
app/schemas.py (1)
LeadCallTracker(47-66)app/database/queries/breeze_buddy/lead_call_tracker.py (1)
abort_call_by_lead_id_query(354-381)app/database/queries/__init__.py (1)
run_parameterized_query(14-30)app/database/accessor/breeze_buddy/call_execution_config.py (1)
get_row_count(24-28)app/database/decoder/breeze_buddy/lead_call_tracker.py (1)
decode_lead_call_tracker(19-46)
app/database/queries/breeze_buddy/lead_call_tracker.py (1)
app/schemas.py (2)
LeadCallStatus(30-34)LeadCallOutcome(37-44)
app/database/accessor/__init__.py (1)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
handle_call_abort(397-426)
app/api/routers/breeze_buddy.py (2)
app/database/accessor/breeze_buddy/lead_call_tracker.py (1)
handle_call_abort(397-426)app/schemas.py (1)
CallAbortRequest(146-149)
🪛 Ruff (0.14.5)
app/database/accessor/breeze_buddy/lead_call_tracker.py
424-424: Do not catch blind exception: Exception
(BLE001)
425-425: Use explicit conversion flag
Replace with conversion flag
(RUF010)
app/database/queries/breeze_buddy/lead_call_tracker.py
359-372: Possible SQL injection vector through string-based query construction
(S608)
app/api/routers/breeze_buddy.py
305-305: Do not catch blind exception: Exception
(BLE001)
307-307: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
307-307: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (6)
scripts/create_tables.py (1)
36-36: LGTM! Database constraint updated correctly.The addition of 'ABORT' to the outcome CHECK constraint properly extends the schema to support the new abort functionality.
app/database/accessor/__init__.py (1)
20-20: LGTM! Public API surface extended appropriately.The new
handle_call_abortfunction is properly exported following the existing module patterns.Also applies to: 61-61
app/database/queries/breeze_buddy/lead_call_tracker.py (2)
343-343: LGTM! Analytics properly extended for ABORT outcome.The addition of
aborted_callscounter maintains consistency with other outcome types in the analytics query.
354-381: LGTM! Query correctly implements abort logic.The query properly:
- Uses parameterized values to prevent SQL injection
- Restricts abort to pending calls (BACKLOG/RETRY status, no existing outcome)
- Updates all relevant fields (status, outcome, timestamps, metadata)
- Returns the updated row for confirmation
app/schemas.py (1)
44-44: LGTM! Enum value added correctly.The ABORT outcome is properly added to the LeadCallOutcome enum, maintaining consistency with the database schema changes.
app/api/routers/breeze_buddy.py (1)
620-622: LGTM! Analytics correctly track aborted calls.Both call-based and lead-based analytics properly account for the new ABORT outcome, maintaining consistency with how other outcomes are tracked.
Also applies to: 652-654
app/api/routers/breeze_buddy.py
Outdated
| @router.post("/call/abort") | ||
| async def abort_call( | ||
| abort_request: CallAbortRequest, | ||
| ): | ||
| """ | ||
| Abort a call by lead ID. | ||
| Sets status to FINISHED and outcome to CANCEL. | ||
| """ | ||
| try: | ||
| aborted_lead = await handle_call_abort(abort_request.lead_id) | ||
|
|
||
| if aborted_lead: | ||
| # Extract customer info for response | ||
| customer_name = aborted_lead.payload.get("customer_name") if aborted_lead.payload else None | ||
| order_id = aborted_lead.payload.get("order_id") if aborted_lead.payload else None | ||
|
|
||
| return { | ||
| "status": "success", | ||
| "message": f"Call aborted successfully for lead {abort_request.lead_id}", | ||
| "lead_id": abort_request.lead_id, | ||
| "customer_name": customer_name, | ||
| "order_id": order_id, | ||
| "updated_status": "FINISHED", | ||
| "updated_outcome": "ABORT" | ||
| } | ||
| else: | ||
| return { | ||
| "status": "not_found", | ||
| "message": f"No pending call found for lead {abort_request.lead_id}", | ||
| "lead_id": abort_request.lead_id | ||
| } | ||
|
|
||
| except Exception as e: | ||
| logger.error(f"Error aborting call for lead {abort_request.lead_id}: {e}") | ||
| raise HTTPException(status_code=500, detail=f"Failed to abort call: {str(e)}") | ||
|
|
There was a problem hiding this comment.
Critical: Missing authentication on abort endpoint.
Unlike all other POST endpoints in this router (e.g., trigger_order_confirmation at line 310, add_outbound_number at line 182, add_call_execution_config at line 438), the /call/abort endpoint lacks authentication via Depends(get_current_user). This allows unauthenticated users to abort any call by providing a lead_id, which is a significant security vulnerability.
Additionally, the docstring at line 279 incorrectly states "outcome to CANCEL" when it should say "outcome to ABORT".
Apply this diff to add authentication and fix the docstring:
@router.post("/call/abort")
async def abort_call(
abort_request: CallAbortRequest,
+ current_user: TokenData = Depends(get_current_user),
):
"""
Abort a call by lead ID.
- Sets status to FINISHED and outcome to CANCEL.
+ Sets status to FINISHED and outcome to ABORT.
+ Requires JWT authentication.
"""
try:
+ logger.info(
+ f"Authenticated user {current_user.user_id} aborting call for lead {abort_request.lead_id}"
+ )
aborted_lead = await handle_call_abort(abort_request.lead_id)🧰 Tools
🪛 Ruff (0.14.5)
305-305: Do not catch blind exception: Exception
(BLE001)
307-307: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
307-307: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In app/api/routers/breeze_buddy.py around lines 273 to 308, the POST /call/abort
endpoint is missing authentication and has an incorrect docstring outcome;
update the function signature to require authentication by adding the dependency
(e.g., user: User = Depends(get_current_user)) consistent with other POST
endpoints in this router, ensure any required imports (Depends,
get_current_user, User type) are present at the top of the file, and change the
docstring line to state that the endpoint sets the outcome to "ABORT" instead of
"CANCEL".
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| async def handle_call_abort(lead_id: str) -> Optional[LeadCallTracker]: | ||
| """ | ||
| Abort a call by lead ID. | ||
| Sets status to FINISHED and outcome to CANCEL. | ||
| """ |
There was a problem hiding this comment.
Fix incorrect docstring.
The docstring states "outcome to CANCEL" but the implementation sets outcome to ABORT. This documentation inconsistency could mislead future maintainers.
Apply this diff to correct the docstring:
async def handle_call_abort(lead_id: str) -> Optional[LeadCallTracker]:
"""
Abort a call by lead ID.
- Sets status to FINISHED and outcome to CANCEL.
+ Sets status to FINISHED and outcome to ABORT.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def handle_call_abort(lead_id: str) -> Optional[LeadCallTracker]: | |
| """ | |
| Abort a call by lead ID. | |
| Sets status to FINISHED and outcome to CANCEL. | |
| """ | |
| async def handle_call_abort(lead_id: str) -> Optional[LeadCallTracker]: | |
| """ | |
| Abort a call by lead ID. | |
| Sets status to FINISHED and outcome to ABORT. | |
| """ |
🤖 Prompt for AI Agents
In app/database/accessor/breeze_buddy/lead_call_tracker.py around lines 397 to
401, the docstring for handle_call_abort incorrectly states that the outcome is
set to CANCEL while the implementation sets outcome to ABORT; update the
docstring to state that the outcome is set to ABORT (or otherwise match the
actual return value), keeping the rest of the docstring intact and consistent
with the function behavior.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
app/api/routers/breeze_buddy.py
Outdated
|
|
||
| @router.post("/call/abort") | ||
| async def abort_call( | ||
| abort_request: CallAbortRequest, |
yugesh-ganipudi
force-pushed
the
BZ-46487-shopify-order-cancellation-lead-management
branch
from
a59dd89 to
043db78
Compare
| Abort a call by lead ID. | ||
| Sets status to FINISHED and outcome to ABORT. | ||
| """ | ||
| logger.info(f"🚀 Call abort started for lead {lead_id}") |
| logger.info(f"🚀 Call abort started for lead {lead_id}") | ||
|
|
||
| try: | ||
| from app.database.queries.breeze_buddy.lead_call_tracker import abort_call_by_lead_id_query |
| "status" = $1, | ||
| "outcome" = $2, | ||
| "updated_at" = NOW(), | ||
| "call_end_time" = NOW(), |
There was a problem hiding this comment.
why are we updating the call_end_time
app/api/routers/breeze_buddy.py
Outdated
| raise HTTPException(status_code=400, detail=str(e)) | ||
|
|
||
|
|
||
| @router.post("/call/abort") |
There was a problem hiding this comment.
instead of post can we use delete endpoint with id in params?
yugesh-ganipudi
force-pushed
the
BZ-46487-shopify-order-cancellation-lead-management
branch
2 times, most recently
from
5ac31c2 to
194897e
Compare
app/schemas.py
Outdated
| scopes: list[str] = Field(default_factory=list) | ||
|
|
||
|
|
||
| class CallAbortRequest(BaseModel): |
There was a problem hiding this comment.
where is this being used?
yugesh-ganipudi
force-pushed
the
BZ-46487-shopify-order-cancellation-lead-management
branch
2 times, most recently
from
eb6c88f to
8841cd0
Compare
…o FINISHED and outcome to ABORT whenever gets triggered - It requires lead_id in request
yugesh-ganipudi
force-pushed
the
BZ-46487-shopify-order-cancellation-lead-management
branch
from
8841cd0 to
3fda196
Compare
2 participants
Summary by CodeRabbit