(feat): add endpoints for admin and merchants dashboards#508
(feat): add endpoints for admin and merchants dashboards#508narsimhaReddyJuspay wants to merge 1 commit intojuspay:releasefrom
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 Use the checkbox below for a quick retry:
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:
WalkthroughSeparates merchant business entities from user login accounts: adds merchant_entities and user_accounts routers plus legacy users router, new Pydantic schemas, DB CRUD modules, migration 014 (merchants table + owner_id), RBAC updates (SHOP→USER), and handlers enforcing scoped permissions and audit logging. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin/Reseller
participant API as API Router
participant AuthZ as RBAC
participant Handler as Handler Layer
participant DB as Database
Admin->>API: POST /entity/merchant (MerchantEntityCreate)
API->>AuthZ: get_current_user_with_rbac
AuthZ-->>API: UserInfo (role, owner_id, scope)
API->>Handler: create_merchant_entity_handler(merchant_data, current_user)
Handler->>DB: check_merchant_id_exists(merchant_id)
DB-->>Handler: exists? (false)
Handler->>DB: INSERT INTO merchants (...)
DB-->>Handler: created row
Handler-->>API: MerchantEntityResponse (201)
API-->>Admin: 201 MerchantEntityResponse
sequenceDiagram
participant Admin as Admin
participant API as API Router
participant AuthZ as RBAC
participant Handler as Handler Layer
participant DB as Database
Admin->>API: POST /user (UserCreate)
API->>AuthZ: get_current_user_with_rbac
AuthZ-->>API: UserInfo (role, owner_id)
API->>Handler: create_user_handler(user_data, current_user)
Handler->>Handler: _check_create_access
Handler->>DB: create_user(username, hashed_password, role, owner_id, ...)
DB-->>Handler: User row
Handler-->>API: UserResponse (201)
API-->>Admin: 201 UserResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
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.
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
2 times, most recently
from
5e7c294 to
51ce9d8
Compare
|
@coderabbitai review once more |
|
✅ Actions performedReview triggered.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@app/api/routers/breeze_buddy/merchant_entities/handlers.py`:
- Around line 235-239: The view-access check is done after fetching the merchant
and thus leaks existence by returning 404 for missing and 403 for existing
resources; move the access check earlier: call
_check_merchant_view_access(current_user, merchant_id) before performing the DB
lookup that sets merchant (the variable named merchant) so unauthorized callers
hit the access check first, and then keep the existing raise
HTTPException(status_code=404, detail="Merchant entity not found") after the DB
read for users that passed the access check.
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 319-322: The DB call to user_queries.get_user_by_id(user_id) is
outside the try/except so DB errors aren't being caught or logged; move that
call (and the subsequent check that raises HTTPException("User not found"))
inside the existing try/except used by this handler, and in the except block log
the exception (using the same structured logger as other handlers) before
re-raising or returning the appropriate HTTPException to preserve consistent
error handling.
In `@app/database/queries/breeze_buddy/merchant_entities.py`:
- Around line 131-142: Both get_merchant_entity_by_merchant_id and
update_merchant_entity currently return None when get_db_connection() yields
nothing; after the async for loop in each function (the blocks using `async for
conn in get_db_connection():`), add a fallback that raises Exception("No
database connection available") to match the pattern used elsewhere in this
module and ensure a clear error is thrown when no DB connection is obtained.
- Around line 95-117: The create_merchant_entity function currently falls
through and returns None if get_db_connection() yields no connection; change it
to raise a specific error when no DB connection is acquired (consistent with
check_merchant_id_exists, get_all_merchant_entities,
get_merchant_entities_by_ids, and delete_merchant_entity). Inside
create_merchant_entity, after the async for get_db_connection() loop, detect the
case where the loop never yielded a connection and raise a RuntimeError (or a
shared custom DBConnectionError) with a clear message like "No DB connection
available for create_merchant_entity"; ensure existing exception handling
remains for other errors and that the raised error replaces the implicit None
return.
In `@app/database/queries/breeze_buddy/users.py`:
- Line 690: The delete_user function currently accepts caller_role but never
uses it, so either remove the parameter or enforce RBAC: when implementing the
admin-deletion guard inside delete_user (e.g., where you check the target user's
role), also verify the caller_role (the function parameter) is UserRole.ADMIN
before allowing deletion; if caller_role is not ADMIN return False/raise a
Permission error. Update the signature only if you choose removal, otherwise add
the check using the caller_role parameter (and keep references to delete_user
and UserRole to locate the change).
- Around line 200-216: get_merchant_by_id currently passes merchant_uuid
directly to the DB and can raise asyncpg DataError for invalid UUIDs; mirror
get_user_by_id by validating the UUID first with _is_valid_uuid and return None
if invalid. Update the get_merchant_by_id function to call
_is_valid_uuid(merchant_uuid) before running the query, short-circuit with a
None return when validation fails, and keep the existing try/except logging
behavior (logger.error and re-raise) unchanged.
In `@MERCHANT_USER_MANAGEMENT_GUIDE.md`:
- Around line 105-111: Documentation incorrectly states there is "NO delete
endpoint" for merchant entities; update the Soft Delete section to reflect that
a delete endpoint exists and describe its behavior: reference the existing
delete_merchant_entity_handler (in
app/api/routers/breeze_buddy/merchant_entities/handlers.py) and the legacy
DELETE /merchant/{id} route, clarify whether those endpoints perform a hard
delete or set is_active=false, and align the guidance (e.g., prefer PUT
is_active=false for deactivation if delete performs hard removal) so the text
accurately matches the implementation and expected RBAC/behavior.
🧹 Nitpick comments (8)
app/api/routers/breeze_buddy/user_accounts/handlers.py (2)
371-412: Self-deletion check at Line 387 is redundant for admins.Line 374 already blocks admin self-deletion. For non-admin users, Line 387 catches self-deletion. However, if a non-admin (e.g., reseller) reaches Line 387 with
user_id == current_user.id,_check_update_delete_accessat Line 391 would also need to handle this — but actually_check_update_delete_accessdoesn't block self-modification. The flow is fine but the ordering is slightly confusing.More importantly: the same DB-call-outside-try issue applies here —
get_user_by_idat Line 378 is outside thetryat Line 393. Consider wrapping Lines 378–391 inside thetryblock for consistency.
265-267:HTTPExceptionraised insidetryis swallowed by bareexcept Exception.In
get_all_users_handler, unlike the other handlers (e.g.,get_user_by_id_handlerat Lines 308-309), there is noexcept HTTPException: raiseclause before the broadexcept Exception. Ifuser_queries.get_all_usersraises anHTTPException(unlikely from query layer but possible in future), it would be masked as a 500.This is low-risk but inconsistent with the pattern in the other four handlers.
♻️ Add HTTPException passthrough for consistency
try: users, total = await user_queries.get_all_users( ... ) ... + 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/merchant_entities/handlers.py (1)
285-321:delete_merchant_entity_handlerreturns a plaindictinstead of a typed response model.Other delete handlers (e.g.,
delete_user_handler) returnDeleteResponse. Returning an untyped dict bypasses Pydantic serialization/validation and makes the OpenAPI schema less useful. Consider usingDeleteResponsefor consistency.♻️ Suggested change
+from app.schemas.breeze_buddy.users import DeleteResponse + async def delete_merchant_entity_handler( merchant_id: str, current_user: UserInfo -) -> dict: +) -> DeleteResponse: ... - return { - "success": True, - "message": f"Merchant entity '{merchant_id}' deleted", - "deleted_id": merchant_id, - } + return DeleteResponse( + success=True, + message=f"Merchant entity '{merchant_id}' deleted", + deleted_id=merchant_id, + )app/api/routers/breeze_buddy/users/__init__.py (1)
88-108: Path parameteridshadows Python builtin.The path parameter is named
idin multiple endpoints (Lines 90, 113, 138). While common in FastAPI examples, it shadows Python's built-inid(). Consider renaming tomerchant_idfor clarity and to match the domain language.♻️ Example rename
-@router.get("/merchant/{id}", response_model=MerchantResponse) +@router.get("/merchant/{merchant_id}", response_model=MerchantResponse) async def get_merchant( - id: str, + merchant_id: 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_id)app/schemas/breeze_buddy/users.py (1)
105-124: Consolidate duplicateUserCreate/UserUpdateschemas across modules.
app/schemas/breeze_buddy/auth.pydefinesUserCreate(lines 115–123) andUserUpdate(lines 127–134) with significantly different field constraints than their counterparts inusers.py:
usernameandpasswordlack validation constraints (nomin_length)shop_identifiersdefaults tolistinstead oflambda: ["*"]is_activeuses direct assignment vs Field()While the application correctly imports from
users.py, maintaining identically-named classes with different validation rules in sibling modules is unmaintainable and creates silent error risk. Remove the auth.py versions or rename one pair (e.g.,AuthUserCreate/AuthUserUpdate) for clarity.app/core/security/authorization.py (1)
135-145: Function nameis_shopis now misleading after the role rename toUserRole.USER.The function body correctly checks
UserRole.USER, but the function name and docstring still reference the old "shop" concept. Consider renaming tois_userfor consistency with the enum rename and improved clarity.♻️ 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 account. 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.USERapp/database/queries/breeze_buddy/users.py (2)
103-118:get_user_by_usernamemanually parses JSON instead of using_parse_json_field.Lines 109-118 inline the JSON parsing logic for
merchant_idsandshop_identifiers, but_build_user_responseand_build_merchant_responseuse the_parse_json_fieldhelper (which also handlesJSONDecodeError). This inconsistency means malformed JSON here would raise an unhandled exception instead of falling back gracefully.♻️ 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"], )
31-39:_parse_json_fieldis duplicated across query modules.This identical function exists in both this file and
app/database/queries/breeze_buddy/merchant_entities.py(lines 18-26). Consider extracting it into a shared utility (e.g.,app/database/queries/breeze_buddy/utils.py) to avoid drift.
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
2 times, most recently
from
a4b2a09 to
4071e5d
Compare
|
@coderabbitai review once more |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
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)
103-123:⚠️ Potential issue | 🟠 Major
get_user_by_usernameuses rawjson.loadsinstead of_parse_json_field— risksJSONDecodeError.Lines 109-118 parse
merchant_idsandshop_identifierswith barejson.loads, bypassing the_parse_json_fieldhelper (line 31) that was introduced specifically to handle malformed JSON gracefully. If a row contains corrupted JSON, this will crash the authentication flow.🐛 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/__init__.py`:
- Around line 33-57: The docstring and handler are inconsistent about
duplicate-merchant errors—update the duplicate-merchant response to use 409
Conflict to match create_user_handler: change the HTTPException raised in
create_merchant_entity_handler from status_code=400 to status_code=409 and
update the create_merchant_entity endpoint docstring (and any related tests or
assertions) to state "409: If merchant_id already exists"; keep the handler name
(create_merchant_entity_handler) and endpoint function (create_merchant_entity)
as references when making the changes.
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 245-267: The get_all_users_handler currently catches all
exceptions and converts them to a 500, which will swallow any HTTPException;
modify the error handling in get_all_users_handler so it first re-raises
HTTPException (add an "except HTTPException: raise" clause immediately before
the generic "except Exception as e"), then keep the logger.error and raise
HTTPException(status_code=500, detail="Internal server error") for other
exceptions; locate the handler around the call to user_queries.get_all_users and
the return of UserListResponse to add this clause.
In `@app/api/routers/breeze_buddy/users/handlers.py`:
- Around line 80-134: Move the DB pre-check calls into the existing try/except
in create_merchant_handler: include
check_username_exists(merchant_data.username) and
check_merchant_id_exists(merchant_data.merchant_id) inside the try block along
with create_merchant, so any DB errors they raise are caught; preserve the
existing except asyncpg.UniqueViolationError branch that inspects the exception
for "username" and the subsequent HTTPException mappings, and ensure you still
raise a 500 HTTPException if merchant is falsy and log via logger.info on
success.
In `@app/schemas/breeze_buddy/users.py`:
- Around line 105-124: Align the package exports so the constrained model in
users.py becomes the canonical UserCreate: update the re-exporting in
app/schemas/breeze_buddy/__init__.py and the top-level app/schemas (and
app/schemas/__init__.py or app/schemas.py if present) to import and expose
breeze_buddy.users.UserCreate as UserCreate (instead of
breeze_buddy.auth.UserCreate); remove or rename the unconstrained
auth.py::UserCreate (or export it under a distinct name like AuthUserCreate) to
avoid duplicate/conflicting symbols so all consumers importing
app.schemas.UserCreate get the constrained version used by handlers.
In `@MERCHANT_USER_MANAGEMENT_GUIDE.md`:
- Around line 366-384: Docs show truthy checks and defaulting for JSON fields
but the implementation uses explicit None checks; update the examples in
_parse_json_field and _build_merchant_entity_response to match the real code: in
_parse_json_field describe/illustrate returning value if value is not None else
[], and in _build_merchant_entity_response use an explicit "is not None" check
for shop_identifiers (and any owner_id handling) instead of a general truthiness
check so the docs reflect the actual behavior of the functions named
_parse_json_field and _build_merchant_entity_response.
- Around line 779-783: The test example incorrectly asserts that
UserRole.RESELLER is denied by _check_create_merchant_access; update the example
so it matches the RBAC matrix and implementation by either changing the role to
one that is actually denied (e.g., use UserRole.MERCHANT in
test_reseller_cannot_create_merchant) or change the assertion to expect success
for RESELLER (remove pytest.raises and assert no HTTPException), referencing the
test function test_reseller_cannot_create_merchant and the access-checking
function _check_create_merchant_access to locate the change.
🧹 Nitpick comments (9)
app/database/queries/breeze_buddy/merchant_entities.py (2)
19-27: Consider extracting_parse_json_fieldinto a shared utility.This helper is duplicated verbatim in
app/database/queries/breeze_buddy/users.py(lines 30-38). Extract it into a shared module (e.g.,app/database/queries/breeze_buddy/utils.py) to avoid drift between the two copies.
277-342:update_merchant_entitydoes not allow clearingdescriptiontoNone.All fields use
if field is not Noneguards, which means a caller can never explicitly setdescriptionback toNone/NULLonce it has a value. If clearing optional fields is a valid use case, consider a different sentinel or an explicit "clear" mechanism. If not needed now, this is fine to defer.app/api/routers/breeze_buddy/merchant_entities/handlers.py (2)
150-196: Reseller path fetches all scoped merchants into memory before filtering.For resellers,
get_merchant_entities_by_idsloads every merchant in their scope, then filtering, sorting, and pagination are applied in Python (lines 158-195). If a reseller has a large scope this is O(n) memory and CPU. Consider pushing filters into the DB query — e.g., adding filter parameters toget_merchant_entities_by_ids, or usingget_all_merchant_entitieswith a merchant_id-IN constraint.
285-327: Delete handler duplicates the role-check logic from_check_update_access.Lines 290-294 and 305-310 replicate the same admin/reseller + ownership check that
_check_update_accessalready implements. Reuse it to keep the authorization logic DRY and avoid divergence.Suggested refactor
async def delete_merchant_entity_handler( merchant_id: str, current_user: UserInfo ) -> dict: """Delete merchant entity (admin or reseller who owns it).""" - # Only admin and reseller can delete - if current_user.role not in [UserRole.ADMIN, UserRole.RESELLER]: - raise HTTPException( - status_code=403, - detail="Only admin or reseller can delete merchant entities", - ) - try: # Get the merchant first to check ownership 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") - # Resellers can only delete merchants they own - if current_user.role == UserRole.RESELLER: - if merchant.owner_id != current_user.id: - raise HTTPException( - status_code=403, - detail="You can only delete merchant entities you created", - ) + # Check update/delete access (admin can delete any, reseller only their own) + _check_update_access(current_user, merchant.owner_id) deleted = await merchant_queries.delete_merchant_entity(merchant_id)app/api/routers/breeze_buddy/users/__init__.py (1)
88-108: Path parameteridshadows the Python builtin.Using
idas a parameter name works but shadows the built-inid()function. Considermerchant_idoruser_idfor clarity — but this is a minor nit for a legacy endpoint.app/schemas/breeze_buddy/__init__.py (1)
54-66: Multiple import statements from the same module could be consolidated.Lines 54-60, 61, 62-65, and 66 all import from
app.schemas.breeze_buddy.users. The aliases (UserAccountCreate,UserAccountUpdate) require separate import statements, but the non-aliased imports (lines 54-60 and 62-65) could be merged into one. Minor readability nit.app/api/routers/breeze_buddy/user_accounts/handlers.py (1)
209-217: Add exception chaining withfromclauses for clearer tracebacks.Ruff B904 flags several re-raises in
exceptclauses withoutfrom. This makes it harder to trace the root cause during debugging. The pattern applies here and in the other handlers (lines 267, 312, 368, 410, 415).♻️ Proposed fix for this handler
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 eapp/api/routers/breeze_buddy/merchant_entities/__init__.py (1)
169-195: Handler return typedictvs routerresponse_model=DeleteResponse.The
delete_merchant_entity_handlerhas return type-> dictand returns a plain dict, while this route declaresresponse_model=DeleteResponse. FastAPI handles this at runtime (Pydantic validates the dict shape), but the type mismatch can confuse type checkers and developers. Consider updating the handler to return aDeleteResponseinstance for consistency — the user_accountsdelete_user_handleralready does this correctly.app/api/routers/breeze_buddy/users/handlers.py (1)
137-224: Missing error handling around DB calls in get/update/delete handlers.
create_merchant_handlerwraps the insert intry/exceptfor structured logging and graceful error conversion, butget_merchant_handler,update_merchant_handler, anddelete_merchant_handlerlet exceptions fromget_merchant_by_id,update_merchant, anddelete_merchantpropagate unhandled. While the query layer logs before re-raising, unhandled exceptions here bypass handler-level audit context (e.g., which admin triggered the failing operation).The user_accounts handlers consistently wrap all DB calls. Consider applying the same pattern here for consistency.
♻️ Example for get_merchant_handler
async def get_merchant_handler( current_user: UserInfo, merchant_uuid: str, ) -> MerchantResponse: """GET /merchant/{id} - Get merchant by UUID. Admin only.""" require_admin(current_user) + try: merchant = await get_merchant_by_id(merchant_uuid) if not merchant: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=f"Merchant with UUID '{merchant_uuid}' not found", ) return merchant + except HTTPException: + raise + except Exception as e: + logger.error(f"Error fetching merchant {merchant_uuid}: {e}") + raise HTTPException( + status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, + detail="Internal server error", + ) from e
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
4071e5d to
22e8a6b
Compare
|
@coderabbitai review once more |
|
✅ Actions performedReview triggered.
✅ Actions performedReview 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 138-166: The docstring for update_merchant_entity incorrectly
states "Admin-only" and lists a 403 for non-admins, but the actual RBAC (via
get_current_user_with_rbac and update_merchant_entity_handler) allows reseller
owners; either update the docstring/Raises section to reflect that reseller
owners and admins can perform updates, or enforce admin-only access by changing
the RBAC check used here and/or inside update_merchant_entity_handler (replace
or tighten get_current_user_with_rbac behavior to require admin role). Ensure
references to update_merchant_entity and update_merchant_entity_handler are
updated accordingly so docs match actual permission logic.
In `@app/api/routers/breeze_buddy/merchant_entities/handlers.py`:
- Around line 56-88: The RBAC functions _get_merchant_ids_filter and
_check_merchant_view_access currently allow any user with scoped merchant_ids to
list/view entities; restrict access to admin/reseller only by adding a role
guard: in both _get_merchant_ids_filter(current_user: UserInfo) and
_check_merchant_view_access(current_user: UserInfo, merchant_id: str) first
check if current_user.role is in {UserRole.ADMIN, UserRole.RESELLER} and only
then apply the existing wildcard/merchant_ids logic, otherwise return None for
the filter or raise HTTPException(status_code=403) in the checker;
alternatively, if merchant/user access is intended, update the router docs to
explicitly state that non-admin/reseller roles with scoped merchant_ids can view
entities.
In `@app/api/routers/breeze_buddy/user_accounts/__init__.py`:
- Around line 33-57: The route docstring for create_user_account is missing the
409 response for username conflicts; update the docstring for
create_user_account (above the function) to list 409 as a possible response when
the username already exists (to match create_user_handler's behavior), e.g., add
a "409: If username already exists" entry in the Raises/Returns section and
ensure the description aligns with create_user_handler's duplicate-user
behavior.
In `@app/api/routers/breeze_buddy/user_accounts/handlers.py`:
- Around line 185-191: Move the pre-check call to
user_queries.check_username_exists(user_data.username) into the existing
try/except block that wraps other DB operations in this handler so that any
exceptions are logged and mapped to an HTTPException consistently; specifically,
call check_username_exists inside the try, catch exceptions in the handler's
except block (or re-use its logging/error mapping) and only raise the 409
HTTPException when the call returns True—ensure you reference
user_queries.check_username_exists and user_data.username when relocating the
call.
In `@app/database/queries/breeze_buddy/merchant_entities.py`:
- Around line 178-186: Escape SQL wildcard characters in the user-supplied
filters before wrapping with % and add an ESCAPE clause: for merchant_id_filter
and name_filter, first replace backslashes with double backslashes, then replace
'%' with '\%' and '_' with '\_' and then do params.append(f"%{escaped}%"); keep
the parameter index logic using param_idx, and change the corresponding
where_conditions entries to include "ESCAPE '\\'" (e.g. f"merchant_id ILIKE
${param_idx} ESCAPE '\\'") so the backslash escapes are honored.
In `@MERCHANT_USER_MANAGEMENT_GUIDE.md`:
- Around line 189-209: The flow diagram incorrectly shows the request as "POST
/merchant ..." — update the diagram to use the actual endpoint "POST
/entity/merchant ..." so it matches the real handler name
create_merchant_entity_handler() and the database flow; ensure any other
mentions of "/merchant" in this diagram block are replaced with
"/entity/merchant" for consistency.
🧹 Nitpick comments (6)
app/schemas/breeze_buddy/users.py (1)
70-87:MerchantResponseis missingowner_idfield, unlikeUserResponse.
UserResponse(Line 149) includesowner_id, butMerchantResponsedoes not. Since both legacy merchants and user accounts now haveowner_idin the database (added by migration 014), the legacy merchant response model should also expose it for consistency and auditability.♻️ Suggested addition
class MerchantResponse(BaseModel): """Merchant response model.""" id: str = Field(..., description="Merchant's UUID") username: str email: Optional[str] = None role: UserRole = UserRole.MERCHANT merchant_id: str = Field( ..., description="Primary business identifier (e.g., 'redbus')" ) merchant_ids: List[str] = Field( default_factory=list, description="List of merchant identifiers for access control", ) shop_identifiers: List[str] = Field(default_factory=list) is_active: bool = True + owner_id: Optional[str] = None created_at: Optional[datetime] = None updated_at: Optional[datetime] = Noneapp/api/routers/breeze_buddy/users/__init__.py (1)
88-108: Path parameteridshadows the Python built-in.The parameter name
idinget_merchant,update_merchant, anddelete_merchantshadows Python's built-inid(). Consider renaming tomerchant_idfor clarity and to avoid the shadowing, which also aligns better with the new merchant entity endpoints that usemerchant_idas the path parameter name.♻️ Example for one endpoint (apply similarly to others)
-@router.get("/merchant/{id}", response_model=MerchantResponse) +@router.get("/merchant/{merchant_id}", response_model=MerchantResponse) async def get_merchant( - id: str, + merchant_id: str, current_user: UserInfo = Depends(get_current_user_with_rbac), ):app/core/security/authorization.py (1)
135-145: Function nameis_shopis semantically misaligned after the role rename.The
UserRole.SHOPwas renamed toUserRole.USER(as noted in the enum definition), but the functionis_shopwas not updated. It now checksUserRole.USERwhile maintaining the name and docstring referencing "shop-level user," creating confusion. Since this function is unused in the codebase, renaming tois_userwith an updated docstring is a safe improvement.♻️ 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 the user role, False otherwise """ return current_user.role == UserRole.USERapp/database/queries/breeze_buddy/merchant_entities.py (3)
19-27: Duplicate_parse_json_field— extract to a shared utility.This function is identical to
_parse_json_fieldinapp/database/queries/breeze_buddy/users.py(lines 30–38). Having two copies means bug fixes or behavior changes must be applied in both places.Consider extracting it into a shared helper module (e.g.,
app/database/queries/breeze_buddy/utils.py) and importing from bothusers.pyandmerchant_entities.py.
61-64: Consider using a custom exception class instead of bareException.Every "no database connection available" path raises
Exception(...), which makes it impossible for callers to distinguish a connection failure from other exceptions. A customDatabaseConnectionError(RuntimeError)would allow targeted handling and aligns with Ruff TRY002/TRY003 hints.♻️ Example
class DatabaseConnectionError(RuntimeError): """Raised when no database connection could be acquired.""" passThen replace each
raise Exception("No database connection available")withraise DatabaseConnectionError().Also applies to: 114-117, 139-142, 233-237, 271-274, 339-342, 364-367
277-342: No way to explicitly cleardescriptionback toNULL.Since the guard is
if description is not None, passingNonemeans "don't change" — but there's no mechanism for a caller to explicitly set description toNULLin the database. This is fine if the update schema usesOptional[str]with a default ofNoneto mean "omitted", but verify the API contract supports this. If clearing fields is needed in the future, consider a sentinel likeUNSETto distinguish "omitted" from "set to null."
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
22e8a6b to
85e6e02
Compare
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
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)
109-118:⚠️ Potential issue | 🟡 MinorInline JSON parsing lacks
JSONDecodeErrorhandling — use_parse_json_fieldinstead.
get_user_by_usernamemanually parsesmerchant_idsandshop_identifierswithjson.loadsbut without atry/exceptforJSONDecodeError. The_parse_json_fieldhelper (Line 31) already handles this safely. Use it here for consistency and resilience against malformed data.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/database/queries/breeze_buddy/users.py`:
- Around line 426-451: The create_user, get_user_by_id, and update_user
functions currently return None if the async for over get_db_connection()
doesn't run (no DB connection); add the same fallback after the async for loop
as used in get_all_users and delete_merchant by raising Exception("No database
connection available") so the error is surfaced instead of silently returning
None; locate the logic around the async for conn in get_db_connection() blocks
inside create_user, get_user_by_id, and update_user and append the raise when no
connection yields a result (retain existing logger.error/raise in the except
block and keep using _build_user_response, get_db_connection, and logger
symbols).
- Around line 493-497: The username_filter handling in the users.py block (and
the similar logic in merchant_entities.py) only escapes '%' and '_' but not the
backslash, so literal '\' can be misinterpreted in the ILIKE pattern; update the
escape sequence to first replace '\' with '\\' (escape backslashes) before
replacing '%' and '_' and ensure the params use the escaped value (e.g., in the
block that builds where_conditions and params for username_filter in users.py
and the analogous code in merchant_entities.py).
- Around line 264-284: The create_merchant function currently returns None if
get_db_connection() yields no connections; add the same explicit raise used in
other functions (e.g., get_all_merchants, delete_merchant) after the async for
loop to surface a "no database connection" error. Locate create_merchant and the
async for conn in get_db_connection() block, and after the loop add a raised
exception (with a clear message) so missing DB connections don't silently return
None; preserve the existing except block that logs exceptions from the query
execution.
- Around line 341-349: The async function update_merchant iterates `async for
conn in get_db_connection():` but if the generator yields no connections it
silently falls through; add an explicit raise after the loop (similar to
create_merchant) to surface a "no database connection" error. Locate
update_merchant and, after the try block's async-for (but before the except),
raise a descriptive exception (e.g., RuntimeError or a custom error) like "No
database connection available for update_merchant {merchant_uuid}" so failures
when get_db_connection yields nothing are not swallowed.
In `@MERCHANT_USER_MANAGEMENT_GUIDE.md`:
- Around line 556-566: The docs use the placeholder /user/{id} for the GET and
PUT endpoints whereas the router exposes /user/{user_id}; update the API
documentation to use the same placeholder /user/{user_id} (for GET
/user/{user_id} and PUT /user/{user_id}) so it matches the actual route names
and avoid confusion with DELETE /user/{user_id}; ensure any references to 'id'
in the user endpoints (GET, PUT) are replaced with 'user_id' throughout the
section.
🧹 Nitpick comments (2)
app/database/queries/breeze_buddy/users.py (2)
151-164:check_merchant_id_existsname collision withmerchant_entities.py.This function checks the
userstable (JSONB?operator onmerchant_ids), whilemerchant_entities.pyalso exports acheck_merchant_id_existsthat checks themerchantstable. Identical names with different semantics can cause subtle import bugs at call sites. Consider renaming one to clarify scope, e.g.,check_merchant_id_exists_in_users.
31-39:_parse_json_fieldis duplicated inmerchant_entities.py.Both files define an identical
_parse_json_fieldhelper. Consider extracting it to a shared utility module (e.g.,app/database/queries/breeze_buddy/utils.py) to avoid drift.
|
@coderabbitai please review |
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
85e6e02 to
7d830ed
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new “merchant entity vs login account” architecture for Breeze Buddy, introducing admin/reseller/merchant/user dashboard APIs with RBAC controls while keeping legacy merchant CRUD endpoints for compatibility.
Changes:
- Introduces new Merchant Entity CRUD APIs backed by a new
merchantstable (withmerchant_idas PK). - Adds RBAC-scoped User Account management APIs (CRUD + pagination/filtering/sorting) and tracks creator via
owner_id. - Renames role
SHOP→USERacross schemas/auth and migrates existing DB rows + constraint.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| app/schemas/breeze_buddy/users.py | Adds Pydantic schemas for legacy merchant CRUD and new user-account CRUD responses/requests. |
| app/schemas/breeze_buddy/merchant_entities.py | Adds Pydantic schemas for merchant business entities. |
| app/schemas/breeze_buddy/auth.py | Renames SHOP→USER and adds owner_id to user models. |
| app/schemas/breeze_buddy/init.py | Re-exports new schemas for Breeze Buddy package consumers. |
| app/schemas/init.py | Re-exports legacy merchant CRUD schemas at top-level schemas package. |
| app/database/queries/breeze_buddy/users.py | Expands query layer to support legacy merchant CRUD and RBAC user-account CRUD; adds owner_id support. |
| app/database/queries/breeze_buddy/merchant_entities.py | Implements query layer for merchants table CRUD + filtering/sorting. |
| app/database/migrations/014_create_merchants_table.sql | Creates merchants table, adds owner_id to users, migrates role values, and updates role CHECK constraint. |
| app/core/security/authorization.py | Updates is_shop role check to match renamed USER role. |
| app/api/routers/breeze_buddy/users/handlers.py | Adds admin-only legacy merchant CRUD handlers (active for dashboard compatibility). |
| app/api/routers/breeze_buddy/users/init.py | Adds legacy merchant CRUD routes (/merchants, /merchant*). |
| app/api/routers/breeze_buddy/user_accounts/handlers.py | Adds RBAC business logic for user account CRUD and listing. |
| app/api/routers/breeze_buddy/user_accounts/init.py | Adds user account routes (/user, /users, /user/{id}). |
| app/api/routers/breeze_buddy/merchant_entities/handlers.py | Adds RBAC business logic for merchant entity CRUD and listing. |
| app/api/routers/breeze_buddy/merchant_entities/init.py | Adds merchant entity routes under /entity/* to avoid legacy path conflicts. |
| app/api/routers/breeze_buddy/init.py | Registers new routers and removes old merchants router in favor of legacy users router + new entity/user-account routers. |
| MERCHANT_USER_MANAGEMENT_GUIDE.md | Adds a comprehensive guide covering architecture, RBAC, migration, deployment, and endpoint usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
2 times, most recently
from
4c1987d to
9a230b3
Compare
- added new merchant entity endpoints - added crud endpoints for user accounts
narsimhaReddyJuspay
force-pushed
the
add-user-crud-endpoints
branch
from
9a230b3 to
ddc250e
Compare
Summary by CodeRabbit
New Features
Documentation
Chores