(feat): add endpoints for admin and merchants dashboards #508
Conversation
WalkthroughReplaces the legacy merchants router with three new routers (merchant_entities, user_accounts, users), adds RBAC-aware handlers and DB queries for merchant entities and user accounts, introduces schemas and a migration for merchants/owner_id, renames role SHOP→USER, and makes JWT timezone handling timezone-aware. Changes
Sequence DiagramsequenceDiagram
participant Client as Client (Admin/Reseller/User)
participant Router as API Router
participant Auth as Auth/RBAC
participant Handler as Handler Layer
participant DB as Database
Client->>Router: HTTP request (e.g., POST /entity/merchant or POST /user)
Router->>Auth: get_current_user_with_rbac (validate token)
Auth-->>Router: current_user (role, merchant_ids)
Router->>Handler: invoke handler with current_user & params
Handler->>Handler: enforce RBAC (require_admin / _check_*)
Handler->>DB: existence checks / read queries
DB-->>Handler: rows / totals / existence flag
alt allowed
Handler->>DB: create/update/delete operation
DB-->>Handler: created/updated/removed row or success
Handler-->>Router: return response model
else forbidden or conflict
Handler-->>Router: raise HTTPException (403/409/404)
end
Router-->>Client: HTTP response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 6
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/users/handlers.py`:
- Around line 399-445: Add a guard in update_user_handler to prevent merchants
from updating accounts that are not SHOP: after obtaining target_user via
check_user_ownership(current_user, user_id), if current_user.role ==
UserRole.MERCHANT and target_user.role != UserRole.SHOP raise HTTPException 403
(e.g., "Merchants can only modify SHOP accounts"); keep the existing
merchant-specific checks (disallow changing role and restrict shop_identifiers)
but reference target_user where relevant so target_user is meaningfully used.
In `@app/database/queries/breeze_buddy/user_management.py`:
- Around line 77-98: Both check_username_exists and check_merchant_id_exists
currently catch all exceptions and return False, which treats DB failures as
“does not exist”; change them to log the error and re-raise the exception (or
raise a specific custom DB error) instead of returning False so callers can fail
safely. Specifically, in check_username_exists and check_merchant_id_exists (and
inside the async for get_db_connection() blocks) remove the swallowing return
False in the except clause, call logger.error(...) as now, then raise the caught
exception (or wrap it in a custom exception type) so callers receive the error
rather than a false existence result.
- Around line 243-272: The TOCTOU bug in delete_merchant_with_users can delete
dependent users before confirming the merchant exists; inside the same
transaction obtained via get_db_connection(), first SELECT the merchant row
using a FOR UPDATE lock (e.g., SELECT id FROM users WHERE role='merchant' AND
merchant_ids::jsonb ? $1 FOR UPDATE) to confirm existence and lock it, and only
if that returns a row proceed to run the DELETE for non-merchant users
(deleted_users_query) and then delete the merchant (delete_merchant_query); keep
the entire sequence inside the async with conn.transaction() block so the check
and deletes are atomic and roll back on failure.
In `@app/schemas/breeze_buddy/users.py`:
- Around line 11-19: The MerchantCreate model currently defaults
shop_identifiers to an empty list which gives new merchants no shop access;
confirm the intended behavior and either (A) change the default on the
shop_identifiers Field in MerchantCreate to represent “all shops” (e.g., default
to ["*"]) so new merchants inherit universal access, or (B) keep the empty list
but update any documentation and docstrings to explicitly state that merchants
start with no shop access unless shop_identifiers are provided; locate and
modify the Field declaration named shop_identifiers in the MerchantCreate class
accordingly and update related docs/comments or tests that assume the opposite
behavior.
In `@docs/USER_MANAGEMENT_API.md`:
- Around line 9-13: The fenced code blocks in USER_MANAGEMENT_API.md (the role
hierarchy block and the app/ file-structure block) are missing language
identifiers which triggers markdownlint MD040; update each triple-backtick fence
to include a language identifier such as "text" (e.g., ```text) for both the
block showing "Admin (Full Access) ├── Merchant..." and the block showing the
app/ directory tree so linting passes.
- Around line 57-61: The markdown tables (e.g., the parameter table containing
the `page` and `limit` rows) are missing blank lines before and after them which
triggers MD058; surround each table block with a blank line above and below to
satisfy markdownlint. Locate the tables that list parameters (those containing
`page`, `limit`, and similar parameter rows found throughout the doc) and insert
a single empty line immediately before the opening pipe row and a single empty
line immediately after the closing pipe row for each occurrence (including the
other listed table blocks).
🧹 Nitpick comments (1)
app/schemas/__init__.py (1)
61-71: ExportMerchantDeleteResponsealongside other user-management schemas.
DeleteResponseis re-exported, butMerchantDeleteResponseisn’t, even though the delete-merchant endpoint uses it. Re-exporting keeps the public schema surface consistent.♻️ Proposed update
from app.schemas.breeze_buddy.users import ( DeleteResponse, MerchantCreate, + MerchantDeleteResponse, MerchantListResponse, MerchantResponse, MerchantUpdate, UserCreateRequest, UserListResponse, UserResponse, UserUpdateRequest, ) @@ # User Management "DeleteResponse", + "MerchantDeleteResponse", "MerchantCreate", "MerchantListResponse", "MerchantResponse", "MerchantUpdate", "UserCreateRequest", "UserListResponse", "UserResponse", "UserUpdateRequest", ]Also applies to: 117-126
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
5 times, most recently
from
4931be9 to
85bf78c
Compare
|
@coderabbitai i have changed a lot of things please review agian |
|
✅ Actions performedFull review triggered. |
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
85bf78c to
7131021
Compare
|
@coderabbitai please review code |
|
✅ Actions performedReview triggered.
|
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
2 times, most recently
from
8d1ebf8 to
509dd68
Compare
|
@coderabbitai review again carefully |
|
Immediate Observations1. Naming Inconsistency # In app/api/routers/breeze_buddy/__init__.py
from app.api.routers.breeze_buddy.users import router as users_router
router.include_router(users_router, prefix="", tags=["merchants"])Consider renaming to 2. Security Implementation
3. Potential Areas for Review
I'll now trigger a comprehensive full review to analyze all aspects in detail. ✅ Actions performedFull review triggered. |
|
Key Changes to Review:
Areas of Focus:
I'll now trigger a comprehensive review of all these changes. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/database/queries/breeze_buddy/users.py`:
- Around line 105-127: The pre-insert uniqueness checks (check_username_exists,
check_merchant_id_exists) are racy; to handle conflicts you should catch unique
constraint violations around the actual insert: wrap the call to create_merchant
inside create_merchant_handler in a try/except that catches
asyncpg.exceptions.UniqueViolationError (or IntegrityError) and return an HTTP
409 CONFLICT response with a clear message; alternatively modify the insertion
in create_merchant to use an INSERT ... ON CONFLICT DO NOTHING/UPDATE and return
a handled result. Ensure you import asyncpg.exceptions.UniqueViolationError
where create_merchant_handler is defined and avoid letting the exception
propagate.
🧹 Nitpick comments (3)
app/schemas/breeze_buddy/users.py (1)
40-40: Consider usingEmailStrfor email validation.The
EmailStrtype provides built-in validation.♻️ Optional: Add email validation
from datetime import datetime from typing import List, Optional -from pydantic import BaseModel, Field +from pydantic import BaseModel, EmailStr, Field from app.schemas.breeze_buddy.auth import UserRoleThen update the field definitions:
- email: Optional[str] = Field(None, description="Email address of the merchant") + email: Optional[EmailStr] = Field(None, description="Email address of the merchant")Also applies to: 59-59
app/database/queries/breeze_buddy/users.py (1)
209-218: Minor: Use unpacking syntax for list concatenation.♻️ Cleaner list construction
# Ensure primary merchant_id is in the list elif merchant_id not in merchant_ids: - merchant_ids = [merchant_id] + merchant_ids + merchant_ids = [merchant_id, *merchant_ids]app/api/routers/breeze_buddy/__init__.py (1)
44-49: Consider renaming for clarity.The
users_routerhandles merchant endpoints and is tagged as"merchants". While functionally correct, this naming could cause confusion. Consider renaming tomerchant_users_routerormerchants_routerfor alignment.This is a minor naming inconsistency and doesn't affect functionality.
♻️ Optional: Rename for clarity
In
app/api/routers/breeze_buddy/users/__init__.py, the module could be renamed tomerchantsor the router aliased:-from app.api.routers.breeze_buddy.users import router as users_router +from app.api.routers.breeze_buddy.users import router as merchants_router ... # Users/Merchants (merchant management - RBAC enabled) -router.include_router(users_router, prefix="", tags=["merchants"]) +router.include_router(merchants_router, prefix="", tags=["merchants"])
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/users/handlers.py`:
- Around line 97-111: The call to create_merchant should be wrapped in a
try/except that catches asyncpg.exceptions.UniqueViolationError so concurrent
uniqueness constraint failures return a 409 instead of a 500; update the block
around create_merchant(...) in users/handlers.py to catch
asyncpg.exceptions.UniqueViolationError and raise
HTTPException(status_code=status.HTTP_409_CONFLICT, detail="Username or merchant
ID already exists"), and add the necessary import for
asyncpg.exceptions.UniqueViolationError if not present.
In `@app/database/queries/breeze_buddy/users.py`:
- Around line 112-114: The current except block in check_username_exists
swallows DB errors and returns False, which can let the create flow continue;
change the error handling in the check_username_exists function so that on
database exceptions you either (a) return True (treat as "exists"/deny create)
or (b) re-raise the exception to surface the failure to callers like
create_merchant; update the logger.error call to include context (username and
the caught exception) before returning True or raising, and ensure
create_merchant handles the propagated exception if you choose re-raise.
🧹 Nitpick comments (6)
app/schemas/breeze_buddy/users.py (2)
40-40: Consider adding email format validation.The
EmailStrtype for automatic email format validation.♻️ Proposed fix
+from pydantic import BaseModel, EmailStr, Field -from pydantic import BaseModel, Field - email: Optional[str] = Field(None, description="Email address of the merchant") + email: Optional[EmailStr] = Field(None, description="Email address of the merchant")
33-35: Consider adding length constraints tomerchant_id.The
usernamefield hasmin_length=3, max_length=50constraints, butmerchant_id(which is also a unique business identifier) has no such validation. This could allow empty strings or excessively long values.♻️ Proposed fix
merchant_id: str = Field( - ..., description="Primary business identifier (e.g., 'redbus')" + ..., min_length=1, max_length=100, description="Primary business identifier (e.g., 'redbus')" )app/database/queries/breeze_buddy/users.py (2)
212-214: Consider using spread syntax for list concatenation.Minor style improvement using Python's spread operator.
♻️ Proposed fix
# Ensure primary merchant_id is in the list elif merchant_id not in merchant_ids: - merchant_ids = [merchant_id] + merchant_ids + merchant_ids = [merchant_id, *merchant_ids]
29-43: Consider logging a warning whenmerchant_idsis empty.If
merchant_idsis empty (possibly due to data migration issues),merchant_idbecomes an empty string. Whilecreate_merchantprevents this for new records, adding a warning would help detect data integrity issues.♻️ Proposed defensive check
def _build_merchant_response(row) -> MerchantResponse: """Build MerchantResponse from database row.""" merchant_ids = _parse_json_field(row["merchant_ids"]) + if not merchant_ids: + logger.warning(f"Merchant {row['id']} has empty merchant_ids") return MerchantResponse( id=str(row["id"]),app/api/routers/breeze_buddy/__init__.py (2)
20-20: Naming inconsistency:users_routerserves merchant endpoints.The router is imported as
users_routerbut serves endpoints tagged as["merchants"]. This was flagged in PR comments. Consider renaming tomerchant_users_routeror adjusting the module name to better reflect its purpose, though this is a minor consistency concern.♻️ Option A: Rename import alias
-from app.api.routers.breeze_buddy.users import router as users_router +from app.api.routers.breeze_buddy.users import router as merchants_router -router.include_router(users_router, prefix="", tags=["merchants"]) +router.include_router(merchants_router, prefix="", tags=["merchants"])Also applies to: 48-49
44-46: Consider removing commented-out code.The commented-out
router.include_router(merchants_router, ...)line can be removed since the explanatory comment above it already documents the change. Keeping dead code (even commented) can cause confusion.♻️ Proposed cleanup
-# NOTE: Old merchants_router removed - conflicts with users_router which now handles -# merchant CRUD at /merchants, /merchant, /merchant/{id} endpoints -# router.include_router(merchants_router, prefix="", tags=["merchants"]) - +# Merchant CRUD now handled by users_router at /merchants, /merchant, /merchant/{id} endpoints # Users/Merchants (merchant management - RBAC enabled) router.include_router(users_router, prefix="", tags=["merchants"])
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
509dd68 to
6abacd1
Compare
|
@coderabbitai review once more |
|
✅ Actions performedFull review triggered. |
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
6 times, most recently
from
630ee89 to
9d311aa
Compare
|
@coderabbitai i have added a lot of new things review again carefully |
|
✅ Actions performedFull review triggered. |
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
Based on the substantial changes in this PR, I'll carefully review: Architecture & Design
Security Implementation
Database Layer
API Design
Code Quality
I'll provide detailed feedback on all aspects of the implementation. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/merchant_entities/__init__.py`:
- Around line 67-72: The Query parameters sort_by and sort_order use the
deprecated regex argument; update both Query calls to use pattern instead
(replace regex="^(merchant_id|name|created_at|updated_at)$" and
regex="^(asc|desc)$" with pattern="...") so Pydantic v2/FastAPI accepts them;
locate the Query definitions for sort_by and sort_order in
merchant_entities.__init__.py and change the keyword from regex to pattern
without altering the rest of the arguments.
In `@app/api/routers/breeze_buddy/merchant_entities/handlers.py`:
- Around line 78-84: The current TOCTOU check using
merchant_queries.check_merchant_id_exists before creating an entity is racy;
remove reliance on that for correctness and instead call
merchant_queries.create_merchant_entity directly inside a try/except that
catches the DB unique-constraint error (e.g.,
asyncpg.exceptions.UniqueViolationError or the ORM's IntegrityError) and
translate it into a 400/409 HTTPException with a clear "merchant_id already
exists" message; keep or optionally log the original exception for debugging.
Ensure the handlers.py code that calls merchant_queries.create_merchant_entity
(and currently references merchant_queries.check_merchant_id_exists) is updated
to handle the unique-constraint exception and no longer treats the pre-check as
the sole guard against duplicates.
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 111-119: The merchant-scope check in the handler incorrectly
denies access when target_user.merchant_ids is an empty list because any(...) is
vacuously false; update the logic in the handler that checks
current_user.merchant_ids and target_user.merchant_ids so that the membership
check only runs when target_user.merchant_ids is non-empty (e.g., if
target_user.merchant_ids and "*" not in current_user.merchant_ids: then perform
the any(...) check), otherwise treat an empty target_user.merchant_ids as
accessible to scoped users; modify both occurrences of this pattern in
handlers.py that reference current_user.merchant_ids and
target_user.merchant_ids.
In `@app/database/queries/breeze_buddy/merchant_entities.py`:
- Around line 267-274: get_merchant_entities_by_ids currently swallows all
exceptions and returns [] which is inconsistent with other DB helpers; change it
to log the error and re-raise the exception instead of returning [] so callers
can handle failures (keep the existing fallback return [] only for the case
where the async for loop finds no connection). Specifically update the except
block in get_merchant_entities_by_ids to call logger.error(...) with the
exception details and then raise (do not return), leaving the async for
get_db_connection() and the _build_merchant_entity_response usage unchanged.
In `@app/database/queries/breeze_buddy/users.py`:
- Around line 116-118: The current except block in get_user_by_username swallows
DB errors by logging and returning None, conflating "not found" with "error";
change it to log the exception and re-raise it (or remove the try/except so the
exception propagates) so callers can distinguish failures from missing users—use
the existing logger and the get_user_by_username symbol when making this change
(optionally wrap the original exception in a clearer custom DatabaseError before
raising if your codebase prefers typed errors).
- Around line 191-206: get_merchant_by_id currently swallows database exceptions
and returns None, which hides real errors; in the except block for
get_merchant_by_id replace the suppressing return None with re-raising the
caught exception (or raise a specific database error) after logging so callers
can distinguish real errors from “not found” responses—update the except block
to log the error via logger.error(f"...") and then raise the original exception
(e) rather than returning None.
🧹 Nitpick comments (9)
app/core/security/jwt.py (1)
98-106: Consider removing redundant expiration check.The
jwt.decode()call at line 90 already validates token expiration and raisesjwt.ExpiredSignatureError(caught at line 119). This manual check is redundant unlessverify_expis explicitly disabled in decode options.app/api/routers/breeze_buddy/__init__.py (1)
29-30: Naming inconsistency:users_routertagged asmerchants-legacy.The router is named
users_routerbut tagged["merchants-legacy"]. This is confusing—consumers expect a "users" router to handle user operations, not merchant CRUD.Consider renaming to
legacy_merchants_routerormerchant_crud_routerfor clarity:-from app.api.routers.breeze_buddy.users import router as users_router +from app.api.routers.breeze_buddy.users import router as legacy_merchants_router ... -router.include_router(users_router, prefix="", tags=["merchants-legacy"]) +router.include_router(legacy_merchants_router, prefix="", tags=["merchants-legacy"])Also applies to: 64-65
app/schemas/breeze_buddy/merchant_entities.py (1)
25-27: Consider adding item-level validation forshop_identifiers.The
shop_identifierslist accepts any strings, including empty strings or whitespace-only values. If shop identifiers should follow a specific format, consider adding item-level validation:♻️ Optional: Add item validation
from pydantic import field_validator class MerchantEntityCreate(BaseModel): # ... existing fields ... shop_identifiers: Optional[List[str]] = Field( None, description="Optional array of shop identifiers" ) `@field_validator`('shop_identifiers') `@classmethod` def validate_shop_identifiers(cls, v): if v is not None: for item in v: if not item or not item.strip(): raise ValueError('Shop identifiers cannot be empty strings') return vapp/core/security/authorization.py (1)
135-145: Function nameis_shopis inconsistent with checked role and appears to be unused.The function checks for
UserRole.USERbut is namedis_shop, breaking the naming pattern of other role-checking functions (is_admin,is_reseller,is_merchant). Additionally, this function is never called anywhere in the codebase and is not exported, suggesting it may be dead code. Consider either removing it or renaming it tois_userfor consistency.app/database/migrations/014_create_merchants_table.sql (1)
9-9: Redundant index onmerchant_id.The
UNIQUEconstraint onmerchant_id(Line 9) already creates an implicit index. The explicit indexidx_merchants_merchant_id(Line 20) is redundant and consumes extra storage/maintenance overhead.♻️ Suggested fix
-- Indexes for merchants table -CREATE INDEX IF NOT EXISTS idx_merchants_merchant_id ON merchants(merchant_id); CREATE INDEX IF NOT EXISTS idx_merchants_owner_id ON merchants(owner_id);Also applies to: 20-20
app/api/routers/breeze_buddy/merchant_entities/handlers.py (2)
34-41: Unused parametermerchant_entity.The
merchant_entityparameter is not used in_check_update_delete_access. Either remove it or use it for additional validation (e.g., owner-based checks).♻️ Suggested fix to remove unused parameter
-def _check_update_delete_access( - current_user: UserInfo, merchant_entity: MerchantEntityResponse -): +def _check_update_delete_access(current_user: UserInfo): """Check if user can update/delete merchant entity (admin only).""" if current_user.role != UserRole.ADMIN: raise HTTPException( status_code=403, detail="Only admins can modify merchant entities" )Update the call site at line 237:
- _check_update_delete_access(current_user, merchant) + _check_update_delete_access(current_user)
105-109: Chain exceptions for better debugging.Per Ruff B904, re-raised exceptions should use
from errorfrom Noneto preserve/clarify the exception chain.♻️ Suggested fix
except Exception as e: logger.error(f"Error creating merchant entity: {e}") - raise HTTPException(status_code=500, detail="Internal server error") + raise HTTPException(status_code=500, detail="Internal server error") from NoneThis pattern also applies to lines 193-195, 222-224, and 255-257.
app/api/routers/breeze_buddy/user_accounts/handlers.py (1)
210-212: Consider chaining exceptions for better debugging.Per Ruff B904, use
raise ... from Noneto clarify these are intentional exception transformations, not exception handling bugs.♻️ Example fix pattern
except Exception as e: logger.error(f"Error creating user: {e}") - raise HTTPException(status_code=500, detail="Internal server error") + raise HTTPException(status_code=500, detail="Internal server error") from NoneAlso applies to: 249-251, 288-290, 344-346, 380-382
app/api/routers/breeze_buddy/users/handlers.py (1)
108-119: Consider chaining exceptions for better debugging.The
raise ... from Noneorraise ... from epattern would clarify whether the original exception should be suppressed or preserved in logs.♻️ Optional improvement
except asyncpg.UniqueViolationError as e: # Handle race condition where concurrent request passed validation checks error_msg = str(e) if "username" in error_msg: raise HTTPException( status_code=status.HTTP_409_CONFLICT, detail="Username already exists", - ) + ) from None raise HTTPException( status_code=status.HTTP_409_CONFLICT, detail="Merchant already exists", - ) + ) from None
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
56543dc to
d644336
Compare
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
Review Focus AreasBased on the substantial architecture and implementation you've built, I'll carefully examine: Architecture & Separation of Concerns
RBAC Implementation
Database & Performance
Security & Data Integrity
Code Quality & Organization
I'll provide detailed feedback on all aspects of the implementation, including any potential issues, best practices alignment, and optimization opportunities. ٩(◕‿◕)۶ ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/schemas/breeze_buddy/auth.py (1)
19-25:⚠️ Potential issue | 🟡 MinorAdd backward compatibility handling for legacy
shoprole in JWT tokens.The old
SHOProle has been completely removed from theUserRoleenum. Any existing JWTs carrying"role": "shop"will fail at token validation (line 114 ofapp/api/security/breeze_buddy/rbac_token.py) whenUserRole(payload.get("role"))is called, as the enum constructor will raise aValueError. Implement either a temporary mapping layer that accepts legacy "shop" values and converts them to "user", or explicitly handle the parsing error with a migration path.
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/__init__.py`:
- Around line 54-66: The users_router and merchant_entities_router register
overlapping /merchant* routes causing FastAPI routing/OpenAPI conflicts; change
the include_router call for users_router (the one currently included with
prefix="") to use a legacy prefix (e.g. prefix="/legacy-merchants" or
"/merchants-legacy") and update its tag (currently "merchants-legacy") if
desired so routes like POST /merchant, GET /merchants, GET/PUT /merchant/{id}
from users_router no longer collide with merchant_entities_router; ensure only
merchant_entities_router keeps the empty prefix for the canonical merchant
endpoints.
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 6-12: The username creation handler that calls
user_queries.create_user should catch unique-constraint races explicitly: import
asyncpg (or asyncpg.UniqueViolationError) and wrap the insert call so that if
asyncpg.UniqueViolationError is raised you raise HTTPException(status_code=409,
detail="username already exists") (mirroring the merchant creation handler),
while keeping the existing generic exception logging/500 behavior for other
errors; ensure the handler references user_queries.create_user (and request
schema types like UserInfo/UserRole) so the unique-violation is handled only
around the actual DB insert.
In `@app/api/routers/breeze_buddy/users/handlers.py`:
- Around line 55-74: The list_merchants_handler currently returns
MerchantListResponse without computing total_pages; update
list_merchants_handler to compute total_pages (e.g., use math.ceil(total /
limit) or integer math equivalent) after calling get_all_merchants and include
total_pages in the returned MerchantListResponse payload so clients receive
correct pagination info; reference the function name list_merchants_handler, the
call get_all_merchants(page=page, limit=limit), and the response class
MerchantListResponse when adding this field.
In `@MERCHANT_USER_MANAGEMENT_GUIDE.md`:
- Around line 679-685: The migration guidance is inconsistent: replace the
suggested paths `/merchant-entities` and `/user-accounts` in the "Legacy
Endpoints" section with the actual endpoints used elsewhere (`/merchants` and
`/users`) or explicitly mark them as tag names if that was intended; update the
text referencing `/merchant-entities` and `/user-accounts` so it either reads
"migrate to `/merchants` and `/users`" or adds a clarifying note that
`/merchant-entities` and `/user-accounts` are tag labels rather than real routes
(ensure you update the two paths in the block under "Legacy Endpoints"
accordingly).
🧹 Nitpick comments (2)
app/api/routers/breeze_buddy/merchant_entities/handlers.py (1)
34-41: Drop the unused parameter to reduce confusion.
Themerchant_entityargument isn’t used; removing it simplifies the helper and call site.♻️ Suggested refactor
-def _check_update_delete_access( - current_user: UserInfo, merchant_entity: MerchantEntityResponse -): +def _check_update_delete_access(current_user: UserInfo): @@ - _check_update_delete_access(current_user, merchant) + _check_update_delete_access(current_user)Also applies to: 227-238
app/api/routers/breeze_buddy/users/handlers.py (1)
17-47: Reuse the centralizedrequire_adminhelper for consistency.
This avoids duplicated logic and keeps admin checks consistent with core logging/behavior.♻️ Suggested refactor
from app.core.logger import logger +from app.core.security.authorization import require_admin @@ -from app.schemas import UserInfo, UserRole +from app.schemas import UserInfo @@ -def require_admin(current_user: UserInfo) -> None: - """Raise 403 if user is not an admin.""" - if current_user.role != UserRole.ADMIN: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Admin access required", - )
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
d644336 to
3057746
Compare
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
Comprehensive Review ScopeBased on the substantial architecture and implementation in this PR, I'll carefully examine: Architecture & Separation of Concerns
RBAC & Security Implementation
Database Layer & Performance
API Design & Error Handling
Code Quality & Maintainability
Specific Areas of Focus
I'll provide detailed feedback on implementation quality, potential issues, security considerations, and opportunities for optimization. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/merchant_entities/handlers.py`:
- Around line 34-41: The helper _check_update_delete_access currently accepts a
merchant_entity but performs the admin check after the DB read; change it to
only accept current_user (remove the unused merchant_entity parameter), and
enforce the admin-role check immediately where it's called before any DB fetch
to ensure you raise 403 for non-admins rather than leaking existence via a 404;
update all call sites (including the other occurrence around the block
referenced at lines ~231-238) to call _check_update_delete_access(current_user)
prior to any database query.
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 185-190: The pre-check for existing usernames uses
HTTPException(status_code=400) which is inconsistent with the unique-constraint
path that returns 409; update the handler so that when
user_queries.check_username_exists(user_data.username) is True it raises
HTTPException with status_code=409 (Conflict) and keep the same detail message;
ensure the change is applied in the block where exists = await
user_queries.check_username_exists(...) and the raised exception replacing 400
with 409 for consistency with the constraint handling.
- Around line 143-158: When current_user.merchant_ids contains "*" we currently
return None (no filtering) which lets resellers/merchants see accounts they
shouldn't; update _get_rbac_merchant_filter or the handlers to enforce
role-based limits: in get_user_by_id_handler ensure that if current_user.role is
RESELLER or MERCHANT and "*" is present you explicitly deny access to target
users whose role is ADMIN (and for RESELLER users also deny access to other
RESELLER accounts unless allowed), and in get_all_users_handler apply the same
role filter when building the query (or enforce it in the DB query layer) so
wildcard does not bypass role restrictions; reference functions:
_get_rbac_merchant_filter, get_user_by_id_handler, get_all_users_handler.
In `@app/api/routers/breeze_buddy/users/handlers.py`:
- Around line 171-184: The current handler treats a None return from
update_merchant as a 500; change it to raise an HTTPException with
status_code=status.HTTP_404_NOT_FOUND and a clear detail (e.g., "Merchant not
found") when update_merchant(merchant_uuid=merchant_uuid, ...) returns None to
reflect a concurrent delete; keep the existing 500 handling for real failures
elsewhere and only convert the None path to a 404.
- Around line 111-122: The except block catching asyncpg.UniqueViolationError
should preserve the exception context by re-raising the HTTPException with an
explicit cause; update the re-raises in the except asyncpg.UniqueViolationError
handler so each raise HTTPException(... ) is written as raise HTTPException(...
) from e (or from None if you want to suppress the original traceback),
referring to the existing asyncpg.UniqueViolationError catch and the
HTTPException raises in this handler.
- Around line 206-212: The delete handler currently treats any False from
delete_merchant as a 500; update the handler to distinguish "not found" by
re-checking existence (e.g., call get_merchant_by_uuid or fetch_merchant_by_uuid
using merchant_uuid) after a failed delete and if the merchant is missing raise
HTTPException(status_code=404, detail="Merchant not found"), otherwise raise the
500 as before; reference delete_merchant for the delete call and
get_merchant_by_uuid (or the existing read/get function) for the re-check and
adjust the exception branches accordingly.
In `@app/database/queries/breeze_buddy/merchant_entities.py`:
- Around line 92-95: The current assignment converts an empty list to None by
using a truthiness check; update the logic around shop_ids_json (where
entity_id, now, shop_ids_json are set) to use an explicit None check on
shop_identifiers (i.e., if shop_identifiers is not None) so that an
intentionally empty list is preserved and serialized via json.dumps([]) instead
of being collapsed to None.
In `@MERCHANT_USER_MANAGEMENT_GUIDE.md`:
- Around line 32-39: The Merchant Entities endpoints in the table are missing
the mounted /entity prefix; update the endpoints for the "Merchant Entities"
section so `/merchant` => `/entity/merchant`, `/merchants` =>
`/entity/merchants`, and `/merchant/{id}` => `/entity/merchant/{id}` (also
search and replace the same incorrect paths in the other occurrences called out
around lines 396-446) to match the router mounting and avoid collision with
legacy merchant CRUD paths.
🧹 Nitpick comments (2)
app/core/security/jwt.py (1)
93-106: Timezone changes are correct; consider removing redundant manual expiration check.The switch to
datetime.now(timezone.utc)anddatetime.fromtimestamp(exp, tz=timezone.utc)is the right approach for timezone-aware comparison.However, this manual expiration check is redundant —
jwt.decode()on line 90 already validates theexpclaim by default and raisesjwt.ExpiredSignatureError(handled at line 119) if the token is expired. This manual check will never catch an expired token.♻️ Optional: Remove redundant expiration check
payload = jwt.decode(token, self.secret_key, algorithms=[self.algorithm]) - # Check if token has expired - exp = payload.get("exp") - if exp is None: - logger.warning("Token missing expiration claim") - raise CREDENTIALS_EXCEPTION - - if datetime.now(timezone.utc) > datetime.fromtimestamp( - exp, tz=timezone.utc - ): - logger.warning("Token has expired") - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Token has expired", - headers={"WWW-Authenticate": "Bearer"}, - ) - # Extract token dataapp/api/routers/breeze_buddy/users/handlers.py (1)
41-47: Reuse the sharedrequire_adminto keep audit logging consistent.There’s already a centralized
require_adminhelper with logging inapp/core/security/authorization.py. Duplicating the logic here risks drift and loses audit logs. Prefer importing the shared helper and drop the local definition.♻️ Suggested refactor
-from app.schemas import UserInfo, UserRole +from app.schemas import UserInfo +from app.core.security.authorization import require_admin @@ -def require_admin(current_user: UserInfo) -> None: - """Raise 403 if user is not an admin.""" - if current_user.role != UserRole.ADMIN: - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Admin access required", - )
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
3057746 to
a76fee8
Compare
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
I'll carefully examine all aspects of the implementation including:
I'll provide detailed feedback on the implementation. ✅ Actions performedFull review triggered. |
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)
app/schemas/breeze_buddy/auth.py (1)
19-26:⚠️ Potential issue | 🔴 CriticalUpdate hard-coded role value in rbac_token.py.
The enum rename is complete, but
app/api/security/breeze_buddy/rbac_token.pystill contains a hard-coded check forrole_str == "shop". Since the database migration converts oldrole='shop'values torole='user', this condition will never match after the migration. Update it toelif role_str == "user":to match the new enum value.
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 256-267: The post-query filter using UserRole.RESELLER and
UserRole.ADMIN applied to the in-memory users list and then resetting total =
len(users) breaks pagination; change the logic so exclusion happens in the
data-access layer instead: add an excluded_roles (or include_roles) parameter to
the get_all_users / corresponding query function and have the DB query and count
query exclude {UserRole.ADMIN, UserRole.RESELLER} when current_user.role is
UserRole.RESELLER or UserRole.MERCHANT, then use the DB-returned total/count
(not len(users)) to compute total_pages and return the paginated users from the
query.
🧹 Nitpick comments (5)
app/api/routers/breeze_buddy/user_accounts/handlers.py (2)
209-217: Preserve exception context when re-raising HTTPException.The
asyncpg.UniqueViolationErrorhandler and generic exception handler should chain exceptions for better traceability. This matches the pattern used in the merchant handlers.♻️ Suggested fix for exception chaining
except asyncpg.UniqueViolationError: raise HTTPException( status_code=409, detail=f"Username '{user_data.username}' already exists" - ) + ) from None except HTTPException: raise except Exception as e: logger.error(f"Error creating user: {e}") - raise HTTPException(status_code=500, detail="Internal server error") + raise HTTPException(status_code=500, detail="Internal server error") from e
401-408: Preserve exception context when converting ValueError to HTTPException.Line 403 should chain the exception for better debugging.
♻️ Suggested fix
except ValueError as e: # Admin protection from delete_user query - raise HTTPException(status_code=403, detail=str(e)) + raise HTTPException(status_code=403, detail=str(e)) from eapp/database/queries/breeze_buddy/users.py (2)
511-528: SQL construction is safe but could be more defensive.While
sort_byandsort_orderare validated against whitelists (lines 512-516), the f-string query construction still triggers static analysis warnings. The current validation is sufficient, but using a mapping for extra safety would eliminate the warnings entirely.♻️ Suggested defensive approach
# Validate sort fields - valid_sort_fields = {"username", "role", "created_at", "updated_at"} - if sort_by not in valid_sort_fields: - sort_by = "created_at" + SORT_FIELD_MAP = { + "username": "username", + "role": "role", + "created_at": "created_at", + "updated_at": "updated_at", + } + sort_by = SORT_FIELD_MAP.get(sort_by, "created_at") + if sort_order.lower() not in {"asc", "desc"}: sort_order = "desc" + sort_order = "ASC" if sort_order.lower() == "asc" else "DESC"This makes the mapping explicit and uses the mapped value directly.
33-47: Consider validating merchant_ids is non-empty in_build_merchant_response.Line 41 falls back to empty string when
merchant_idsis empty. While the learning indicates this shouldn't happen in practice, a defensive warning log would help catch data integrity issues.🛡️ Suggested defensive logging
def _build_merchant_response(row) -> MerchantResponse: """Build MerchantResponse from database row.""" merchant_ids = _parse_json_field(row["merchant_ids"]) + if not merchant_ids: + logger.warning(f"Merchant {row['id']} has empty merchant_ids - data integrity issue") return MerchantResponse( id=str(row["id"]),Based on learnings: merchant_ids are always provided during user account creation, so this is a defensive measure for unexpected edge cases.
app/api/routers/breeze_buddy/user_accounts/__init__.py (1)
72-77: Migrate from deprecatedregextopatternparameter.FastAPI deprecated the
regex=argument inQuery()in favor ofpattern=starting with version 0.100.0 (Pydantic v2 alignment). Replace both occurrences on lines 75 and 77 withpattern=to future-proof the code and avoid deprecation warnings.
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
a76fee8 to
4237232
Compare
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
Review ScopeI'll carefully examine all aspects of your implementation: Core Architecture
Security & RBAC
Database Layer
API Implementation
Code Quality
I'll provide detailed feedback on the implementation quality, potential issues, and any recommendations. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/users/handlers.py`:
- Around line 155-189: In update_merchant_handler, prevent removal of the
current primary merchant_id by checking the existing merchant (from
get_merchant_by_id) against incoming MerchantUpdate.merchant_ids: if
merchant_data.merchant_ids is provided and does not include the existing.primary
merchant_id (or existing.merchant_id), raise an HTTPException (400 Bad Request)
explaining the primary id is immutable; only proceed to call update_merchant
when the primary id remains present (or merchant_ids is None), referencing
update_merchant_handler, get_merchant_by_id, update_merchant, MerchantUpdate and
the existing.merchant_id field to locate where to add this guard.
🧹 Nitpick comments (2)
app/database/migrations/014_create_merchants_table.sql (1)
8-23: Avoid redundant index onmerchants.merchant_id.
TheUNIQUEconstraint already creates an index in PostgreSQL; the extraCREATE INDEXadds write cost and storage. Please verify on your PostgreSQL version and drop the explicit index if redundant.♻️ Suggested change
- CREATE INDEX IF NOT EXISTS idx_merchants_merchant_id ON merchants(merchant_id);app/api/routers/breeze_buddy/merchant_entities/handlers.py (1)
196-222: RBAC check order could leak entity existence.For resellers, the code fetches the merchant first (line 204), then checks RBAC (line 210-214). This means a reseller can distinguish between "entity doesn't exist" (404) and "entity exists but I don't have access" (403), potentially leaking information about merchant IDs outside their scope.
Consider checking RBAC conditions earlier or using a uniform 404 response for both cases when access is denied.
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
Review ScopeI'll carefully examine all aspects of your implementation: Core Architecture
Security & RBAC
Database Layer
API Implementation
Code Quality
I'll provide detailed feedback on implementation quality, potential issues, security considerations, and any recommendations for improvement. ✅ Actions performedReview triggered.
|
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
4237232 to
da76b82
Compare
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
Comprehensive Review ScopeI'll carefully examine all aspects of your implementation: Core Architecture
Security & RBAC Implementation
Database Layer & Performance
API Design & Error Handling
Code Quality & Maintainability
I'll provide detailed feedback on implementation quality, potential issues, security considerations, and any recommendations for improvement. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/merchant_entities/handlers.py`:
- Around line 152-158: The sort fallback uses a naive datetime (datetime.min)
causing timezone-aware vs naive comparisons in merchants.sort when sort_by is
"created_at" or "updated_at"; update the lambda fallbacks in the merchants.sort
calls (the key functions for created_at and updated_at) to use a timezone-aware
minimum (e.g., datetime.min.replace(tzinfo=timezone.utc)) so all compared values
are timezone-aware and comparisons won't raise TypeError.
In `@app/api/routers/breeze_buddy/user_accounts/__init__.py`:
- Around line 72-77: Replace the deprecated regex keyword with pattern in the
Query definitions for sort_by and sort_order: update the sort_by = Query(...)
and sort_order = Query(...) usages to pass
pattern="^(username|role|created_at|updated_at)$" and pattern="^(asc|desc)$"
respectively (keep the same strings and other args unchanged) so the Query calls
use the Pydantic v2 / FastAPI parameter name.
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 371-404: The handler delete_user_handler currently treats any
False return from user_queries.delete_user as "User not found", which masks DB
errors; fix by changing the query layer (user_queries.delete_user in users.py)
to re-raise database exceptions instead of returning False on errors so
delete_user_handler's existing "if not success: raise HTTPException(404...)"
only represents a real-not-found case, or alternatively update
delete_user_handler to first confirm existence (via get_user_by_id) and treat a
False from delete_user as a server error unless the query explicitly indicates
not-found; ensure references to user_queries.delete_user and delete_user_handler
are updated accordingly.
In `@app/database/queries/breeze_buddy/merchant_entities.py`:
- Around line 57-64: The fallback "return False" when no connection is available
in the async loop using get_db_connection() should be replaced with raising an
explicit exception to match users.py's behavior; locate the block that uses
async for conn in get_db_connection() (and the logger.error/except re-raise) and
change the no-connection path to raise Exception("No database connection
available") instead of returning False so callers don't silently treat missing
DB as "not found" (mirror check_username_exists behavior).
In `@MERCHANT_USER_MANAGEMENT_GUIDE.md`:
- Around line 648-668: The curl examples for merchant creation and verification
use the old endpoints; update the two URLs in the Test Endpoints section so they
include the entity prefix: replace POST /api/breeze_buddy/merchant with POST
/api/breeze_buddy/entity/merchant (the "Create merchant entity (admin only)"
example) and replace GET /api/breeze_buddy/merchants with GET
/api/breeze_buddy/entity/merchants (the "Verify creation" example) so they match
the new entity-based routes used elsewhere in the guide.
🧹 Nitpick comments (7)
app/schemas/breeze_buddy/merchant_entities.py (1)
28-28:Optional[bool]allowsNoneon creation, which likely isn't intended.
is_activedefaults toTruefor new entities, butOptional[bool]permits explicitly passingNone, which could bypass the default and storeNULL. Useboolinstead since creation should always resolve to a definite boolean.♻️ Suggested fix
- is_active: Optional[bool] = Field(True, description="Active status (default: true)") + is_active: bool = Field(True, description="Active status (default: true)")app/database/migrations/014_create_merchants_table.sql (1)
1-40: Consider wrapping the migration in an explicit transaction.This migration mixes DDL (
CREATE TABLE,ALTER TABLE,ADD CONSTRAINT) with DML (UPDATE). If any statement fails mid-way, the database could be left in a partially-migrated state. PostgreSQL supports transactional DDL, so wrapping the entire migration inBEGIN; ... COMMIT;ensures atomicity.♻️ Suggested wrapper
+BEGIN; + -- Migration 014: Create merchants table and add owner_id to users ... ALTER TABLE users ADD CONSTRAINT users_role_check CHECK (role IN ('admin', 'reseller', 'merchant', 'user')); + +COMMIT;app/schemas/breeze_buddy/__init__.py (1)
109-127: Two "merchant create" schemas exported from one package may confuse consumers.This
__init__.pyexports bothMerchantEntityCreate(business entity) andMerchantCreate(user account with merchant role) from the same namespace. The naming distinction is subtle and the "legacy" label onMerchantCreate(line 122) suggests it's on the deprecation path. Consider documenting the intended migration path or adding a deprecation warning so downstream consumers know which to use.app/core/security/authorization.py (1)
135-145:is_shopname is semantically inconsistent after the role rename.The function checks
UserRole.USERbut is still namedis_shopwith a docstring referencing "shop-level user." The enum was renamed fromSHOPtoUSER, but the function was not updated. Consider renaming tois_userand updating the docstring to match the new role semantics, bringing it in line with similar functions likeis_admin,is_reseller, andis_merchant.♻️ Suggested rename
-def is_shop(current_user: UserInfo) -> bool: +def is_user(current_user: UserInfo) -> bool: """ - Check if user is a shop-level user. + Check if user has the USER role. Args: current_user: Current authenticated user Returns: - True if user is shop, False otherwise + True if user has USER role, False otherwise """ return current_user.role == UserRole.USERapp/database/queries/breeze_buddy/users.py (1)
94-114:get_user_by_usernameuses inline JSON parsing instead of_parse_json_field.Lines 100–108 manually handle
isinstance(row["merchant_ids"], list)/json.loads(...)without theJSONDecodeErrorsafety net that_parse_json_fieldprovides. Consider reusing the helper for consistency and resilience against malformed data.app/api/routers/breeze_buddy/users/handlers.py (1)
65-67: Consider lazy log formatting instead of f-strings.Using f-strings in
logger.info(f"...")eagerly evaluates the string even if the log level is disabled. For hot paths,logger.info("...", username, ...)with%splaceholders is preferred. For admin-only endpoints this is a minor nit.Also applies to: 130-132, 187-187, 216-218
app/api/routers/breeze_buddy/users/__init__.py (1)
88-108: Path parameteridshadows the Python built-in.Using
idas a parameter name shadowsbuiltins.id. While it works fine in FastAPI, renaming tomerchant_idormerchant_uuid(matching the handler's parameter name) would be clearer and avoid the shadow. This applies toget_merchant,update_merchant, anddelete_merchant.Example rename
-@router.get("/merchant/{id}", response_model=MerchantResponse) +@router.get("/merchant/{merchant_uuid}", response_model=MerchantResponse) async def get_merchant( - id: str, + merchant_uuid: str, current_user: UserInfo = Depends(get_current_user_with_rbac), ): ... - return await get_merchant_handler(current_user, id) + return await get_merchant_handler(current_user, merchant_uuid)Apply similarly to
update_merchantanddelete_merchant.
- added new merchant entity endpoints - added crud endpoints for user accounts
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
da76b82 to
0379294
Compare
|
@coderabbitai fixed the previous comments once review the entire code carefully again |
|
I'll provide detailed feedback on the implementation. ✅ Actions performedFull review triggered. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/database/queries/breeze_buddy/users.py (1)
94-114:⚠️ Potential issue | 🟡 MinorInline JSON parsing bypasses
_parse_json_fieldsafety nets.Lines 100-108 manually parse
merchant_idsandshop_identifierswithout handlingNonevalues orJSONDecodeError. If either column isNULLin the DB,json.loads(None)raisesTypeError. The_parse_json_fieldhelper defined at line 22 handles both cases. Use it here for consistency and safety.🛡️ Proposed fix
return UserInDB( id=str(row["id"]), username=row["username"], password_hash=row["password_hash"], role=UserRole(row["role"]), email=row["email"], - merchant_ids=( - row["merchant_ids"] - if isinstance(row["merchant_ids"], list) - else json.loads(row["merchant_ids"]) - ), - shop_identifiers=( - row["shop_identifiers"] - if isinstance(row["shop_identifiers"], list) - else json.loads(row["shop_identifiers"]) - ), + merchant_ids=_parse_json_field(row["merchant_ids"]), + shop_identifiers=_parse_json_field(row["shop_identifiers"]), is_active=row["is_active"], owner_id=str(row["owner_id"]) if row.get("owner_id") else None, created_at=row["created_at"], updated_at=row["updated_at"], )
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/merchant_entities/handlers.py`:
- Around line 238-261: Move the call to get_merchant_entity_by_merchant_id into
the existing try/except so any non-HTTP exceptions are caught and logged;
specifically, wrap both merchant = await
merchant_queries.get_merchant_entity_by_merchant_id(merchant_id) and the
subsequent await merchant_queries.update_merchant_entity(...) inside the same
try block, keep the existing HTTPException re-raise, and ensure
logger.error(...) is used in the general except to log the error before raising
HTTPException(500) — this ensures both get_merchant_entity_by_merchant_id and
update_merchant_entity failures are handled consistently.
In `@app/api/routers/breeze_buddy/users/handlers.py`:
- Around line 192-224: The handler currently treats any False from
delete_merchant as a 404, which masks DB errors; update the data layer and
handler: change delete_merchant (in users.py) to raise on DB/execution errors
instead of returning False so callers can distinguish "not found" from real
errors, and in delete_merchant_handler wrap the await
delete_merchant(merchant_uuid) call in a try/except Exception to log the error
and raise an HTTPException(status_code=500) for unexpected DB failures while
preserving the existing 404 behavior when get_merchant_by_id(merchant_uuid)
returns None or when delete_merchant explicitly indicates a not-found outcome.
🧹 Nitpick comments (10)
app/core/security/authorization.py (1)
135-145: Function nameis_shopis now misleading after the SHOP→USER rename.The function checks
UserRole.USERbut is still calledis_shopwith a docstring referencing "shop-level user." Consider renaming tois_user(with an alias or deprecation wrapper foris_shop) to keep the API self-documenting. Otherwise callers will need tribal knowledge to understand the mapping.♻️ Suggested rename
-def is_shop(current_user: UserInfo) -> bool: +def is_user(current_user: UserInfo) -> bool: """ - Check if user is a shop-level user. + Check if user is a basic (user-level) user. Args: current_user: Current authenticated user Returns: - True if user is shop, False otherwise + True if user has the USER role, False otherwise """ return current_user.role == UserRole.USER + + +# Backward-compatible alias +is_shop = is_userapp/core/security/jwt.py (1)
92-106: Minor: manual expiry check is redundant with PyJWT's built-in check.
jwt.decode()(line 90) already raisesjwt.ExpiredSignatureErrorfor expired tokens (caught on line 119). The manual check on lines 93-106 is belt-and-suspenders. This is pre-existing behavior and not introduced by this PR, so no action needed now — just flagging for awareness.app/schemas/breeze_buddy/merchant_entities.py (1)
28-28:is_activeshould bebool, notOptional[bool], on create.During entity creation,
is_activeshould always resolve to a concrete boolean. TheOptionalwrapper allowsNoneto be passed explicitly, which is likely unintended for a create schema.🔧 Suggested fix
- is_active: Optional[bool] = Field(True, description="Active status (default: true)") + is_active: bool = Field(True, description="Active status (default: true)")app/schemas/breeze_buddy/users.py (1)
39-39: No email format validation onAll
Optional[str]with no format validation. Invalid email strings (e.g."not-an-email") will be accepted. Consider using Pydantic'sEmailStr(frompydantic[email]) for basic format enforcement.Also applies to: 58-58, 112-112, 131-131
app/database/migrations/014_create_merchants_table.sql (1)
7-16: Consider wrapping the migration in an explicit transaction.This migration performs DDL (CREATE TABLE, ALTER TABLE) and DML (UPDATE) across two tables. If the migration framework doesn't auto-wrap in a transaction, a partial failure could leave the database in an inconsistent state (e.g., roles updated but constraint not yet applied).
app/schemas/breeze_buddy/__init__.py (1)
54-66: DualUserCreateschemas may confuse consumers — consider documenting the migration path.
UserCreatefromauth.py(line 26) andUserAccountCreatealiased fromusers.py(line 61) both model user creation but with different fields. The__all__exports both. Downstream consumers importing from this package might pick the wrong one. A brief docstring note at the top of this file (or inline comments in__all__) clarifying which to use for new code would help.app/database/queries/breeze_buddy/merchant_entities.py (1)
95-117: Missing no-connection raise increate_merchant_entity,get_merchant_entity_by_merchant_id, andupdate_merchant_entity.These three functions silently return
Noneif theasync forloop completes without yielding, whilecheck_merchant_id_exists(line 61),get_all_merchant_entities(line 233), andget_merchant_entities_by_ids(line 271) all explicitlyraise Exception("No database connection available"). Align the pattern for consistency.🛡️ Proposed fix for create_merchant_entity (apply similar to get_by_merchant_id and update)
try: async for conn in get_db_connection(): row = await conn.fetchrow( query, merchant_id, name, description, shop_ids_json, is_active, owner_id, now, now, ) if row: logger.info(f"Created merchant entity: {merchant_id}") return _build_merchant_entity_response(row) return None + raise Exception("No database connection available") except Exception as e: logger.error(f"Error creating merchant entity {merchant_id}: {e}") raiseapp/api/routers/breeze_buddy/user_accounts/handlers.py (1)
245-267: Addexcept HTTPException: raisefor consistency with other handlers.Every other handler in this file (
create_user_handler,get_user_by_id_handler,update_user_handler,delete_user_handler) re-raisesHTTPExceptionbefore the catch-allexcept Exception. This handler is missing that guard. While the current try-block doesn't raiseHTTPException, adding it prevents accidental swallowing if the code evolves.♻️ Proposed fix
try: users, total = await user_queries.get_all_users( ... ) total_pages = (total + limit - 1) // limit if total > 0 else 0 return UserListResponse( users=users, total=total, page=page, limit=limit, total_pages=total_pages ) + except HTTPException: + raise except Exception as e: logger.error(f"Error fetching users: {e}") raise HTTPException(status_code=500, detail="Internal server error")app/api/routers/breeze_buddy/users/__init__.py (1)
88-108: Path parameteridshadows Python's built-inid().The path parameter is named
idin theget_merchant,update_merchant, anddelete_merchantendpoints. While this works at runtime, it shadows the built-in and reduces readability. Consider renaming tomerchant_uuidto match the handler parameter naming.♻️ Proposed fix (apply to all three endpoints)
-@router.get("/merchant/{id}", response_model=MerchantResponse) +@router.get("/merchant/{merchant_uuid}", response_model=MerchantResponse) async def get_merchant( - id: str, + merchant_uuid: str, current_user: UserInfo = Depends(get_current_user_with_rbac), ): ... - return await get_merchant_handler(current_user, id) + return await get_merchant_handler(current_user, merchant_uuid)app/api/routers/breeze_buddy/merchant_entities/handlers.py (1)
126-169: Reseller path fetches all scoped merchants into memory before filtering.For resellers with a large number of scoped merchants,
get_merchant_entities_by_idsloads all of them into memory, then applies filters, sorting, and pagination in Python (lines 132–169). This works but won't scale well.Consider extending
get_merchant_entities_by_ids(or adding a new query) to accept filter/sort/pagination parameters and push this work to the database, similar to the admin path.
| # Get existing merchant | ||
| merchant = await merchant_queries.get_merchant_entity_by_merchant_id(merchant_id) | ||
| if not merchant: | ||
| raise HTTPException(status_code=404, detail="Merchant entity not found") | ||
|
|
||
| try: | ||
| updated_merchant = await merchant_queries.update_merchant_entity( | ||
| merchant_id=merchant_id, | ||
| name=merchant_data.name, | ||
| description=merchant_data.description, | ||
| shop_identifiers=merchant_data.shop_identifiers, | ||
| is_active=merchant_data.is_active, | ||
| ) | ||
|
|
||
| if not updated_merchant: | ||
| raise HTTPException(status_code=404, detail="Merchant entity not found") | ||
|
|
||
| return updated_merchant | ||
|
|
||
| except HTTPException: | ||
| raise | ||
| except Exception as e: | ||
| logger.error(f"Error updating merchant entity {merchant_id}: {e}") | ||
| raise HTTPException(status_code=500, detail="Internal server error") |
There was a problem hiding this comment.
DB call on line 239 is outside the try/except block — exceptions won't be logged.
If get_merchant_entity_by_merchant_id throws a non-HTTP exception (e.g., connection error), it bypasses the try/except on line 243 and surfaces as an unhandled 500 without the structured logging on line 260. This is inconsistent with the other handlers that wrap all DB calls.
Suggested fix
async def update_merchant_entity_handler(
merchant_id: str, merchant_data: MerchantEntityUpdate, current_user: UserInfo
) -> MerchantEntityResponse:
"""Update merchant entity."""
# Check access first (before DB read)
_check_update_delete_access(current_user)
- # Get existing merchant
- merchant = await merchant_queries.get_merchant_entity_by_merchant_id(merchant_id)
- if not merchant:
- raise HTTPException(status_code=404, detail="Merchant entity not found")
-
try:
+ # Get existing merchant
+ merchant = await merchant_queries.get_merchant_entity_by_merchant_id(merchant_id)
+ if not merchant:
+ raise HTTPException(status_code=404, detail="Merchant entity not found")
+
updated_merchant = await merchant_queries.update_merchant_entity(
merchant_id=merchant_id,
name=merchant_data.name,
description=merchant_data.description,
shop_identifiers=merchant_data.shop_identifiers,
is_active=merchant_data.is_active,
)
if not updated_merchant:
raise HTTPException(status_code=404, detail="Merchant entity not found")
return updated_merchant
except HTTPException:
raise
except Exception as e:
logger.error(f"Error updating merchant entity {merchant_id}: {e}")
raise HTTPException(status_code=500, detail="Internal server error")🤖 Prompt for AI Agents
In `@app/api/routers/breeze_buddy/merchant_entities/handlers.py` around lines 238
- 261, Move the call to get_merchant_entity_by_merchant_id into the existing
try/except so any non-HTTP exceptions are caught and logged; specifically, wrap
both merchant = await
merchant_queries.get_merchant_entity_by_merchant_id(merchant_id) and the
subsequent await merchant_queries.update_merchant_entity(...) inside the same
try block, keep the existing HTTPException re-raise, and ensure
logger.error(...) is used in the general except to log the error before raising
HTTPException(500) — this ensures both get_merchant_entity_by_merchant_id and
update_merchant_entity failures are handled consistently.
| async def delete_merchant_handler( | ||
| current_user: UserInfo, | ||
| merchant_uuid: str, | ||
| ) -> DeleteResponse: | ||
| """DELETE /merchant/{id} - Delete merchant. Admin only.""" | ||
| require_admin(current_user) | ||
|
|
||
| # Check merchant exists | ||
| existing = await get_merchant_by_id(merchant_uuid) | ||
| if not existing: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=f"Merchant with UUID '{merchant_uuid}' not found", | ||
| ) | ||
|
|
||
| success = await delete_merchant(merchant_uuid) | ||
|
|
||
| if not success: | ||
| # Concurrent delete or merchant disappeared | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=f"Merchant with UUID '{merchant_uuid}' not found", | ||
| ) | ||
|
|
||
| logger.info( | ||
| f"Admin {current_user.username} deleted merchant UUID '{merchant_uuid}'" | ||
| ) | ||
|
|
||
| return DeleteResponse( | ||
| success=True, | ||
| message=f"Merchant '{existing.username}' deleted successfully", | ||
| deleted_id=merchant_uuid, | ||
| ) |
There was a problem hiding this comment.
Note: delete_merchant_handler will mask DB errors as 404.
The delete_merchant query function (in users.py) still returns False on exceptions instead of raising. This means a DB error during deletion will hit the if not success check (line 209) and return 404 "not found" instead of 500. This is a downstream effect of the issue flagged in users.py.
🤖 Prompt for AI Agents
In `@app/api/routers/breeze_buddy/users/handlers.py` around lines 192 - 224, The
handler currently treats any False from delete_merchant as a 404, which masks DB
errors; update the data layer and handler: change delete_merchant (in users.py)
to raise on DB/execution errors instead of returning False so callers can
distinguish "not found" from real errors, and in delete_merchant_handler wrap
the await delete_merchant(merchant_uuid) call in a try/except Exception to log
the error and raise an HTTPException(status_code=500) for unexpected DB failures
while preserving the existing 404 behavior when
get_merchant_by_id(merchant_uuid) returns None or when delete_merchant
explicitly indicates a not-found outcome.
1 participant
Summary by CodeRabbit