refactor(user): update user services #25
Conversation
✅ Deploy Preview for splitwizer canceled.
|
WalkthroughThe changes standardize user-related API response schemas and error formats, update the transformation and update logic in the user service, and refactor tests to use synchronous FastAPI TestClient with updated data formats. Documentation is revised for naming consistency and clarification of date field serialization. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant UserService
participant Database
Client->>API: PATCH /users/me (update profile)
API->>UserService: update_user_profile(user_id, update_fields)
UserService->>Database: Update user with allowed fields (name, imageUrl, currency)
Database-->>UserService: Updated user document
UserService-->>API: Transformed user data (camelCase, ISO dates)
API-->>Client: 200 OK with updated user profile
alt No update fields
API-->>Client: 400 Bad Request {"detail": {"error": "...", "message": "..."}}
end
alt User not found
API-->>Client: 404 Not Found {"detail": {"error": "...", "message": "..."}}
end
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #25 +/- ##
==========================================
+ Coverage 71.33% 71.53% +0.19%
==========================================
Files 19 19
Lines 1591 1602 +11
Branches 156 158 +2
==========================================
+ Hits 1135 1146 +11
Misses 456 456
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
backend/app/user/routes.py (1)
30-34: Fix formatting issue and consider dependency injection pattern.There are two issues to address:
- Missing blank line before function definition (static analysis)
- Function call in argument defaults (static analysis)
Apply this diff to fix the formatting:
+ @router.delete("/me", response_model=DeleteUserResponse)For the dependency injection, the current pattern is acceptable in FastAPI, but if you want to follow the static analysis suggestion, you could refactor like this:
-async def delete_user_account(current_user: Dict[str, Any] = Depends(get_current_user)): +async def delete_user_account(current_user: Dict[str, Any] = Depends(get_current_user)):Actually, the B008 warning about
Dependsin defaults is a false positive for FastAPI - this is the standard FastAPI pattern and is intentional.backend/tests/user/test_user_service.py (1)
3-3: Remove unused import to clean up test file.The
get_databaseimport is flagged as unused by static analysis and should be removed.Apply this diff:
-from app.database import get_databasebackend/tests/user/test_user_routes.py (4)
6-6: Remove unused import.The
datetimeclass from the datetime module is imported but never used in the file.-from datetime import datetime, timedelta +from datetime import timedelta
27-29: Remove unused variable.The variable
iso_date3is defined but never used within the fixture scope.iso_date = "2023-01-01T00:00:00Z" iso_date2 = "2023-01-02T00:00:00Z" -iso_date3 = "2023-01-03T00:00:00Z"
93-99: Fix unused variable in test.The
iso_date3variable is defined locally but the mock uses a different variable name in line 99. This creates an inconsistency.iso_date = "2023-01-01T00:00:00Z" -iso_date3 = "2023-01-03T00:00:00Z" +iso_date_updated = "2023-01-03T00:00:00Z" update_payload = {"name": "Only Name Updated"} mocker.patch("app.user.service.user_service.update_user_profile", return_value={ "id": TEST_USER_ID, "name": "Only Name Updated", "email": TEST_USER_EMAIL, "imageUrl": None, "currency": "USD", - "createdAt": iso_date, "updatedAt": iso_date3 + "createdAt": iso_date, "updatedAt": iso_date_updated })
53-140: Address PEP 8 formatting issues.Multiple test functions are missing the required two blank lines before function definitions as per PEP 8 standards.
@pytest.fixture(scope="module") def auth_headers(): token = create_access_token( data={"sub": TEST_USER_EMAIL, "_id": TEST_USER_ID}, expires_delta=timedelta(minutes=15) ) return {"Authorization": f"Bearer {token}"} + @pytest.fixture(autouse=True, scope="function") async def setup_test_user(mocker): # ... rest of fixture yield + # --- Tests for GET /users/me --- + def test_get_current_user_profile_success(client: TestClient, auth_headers: dict, mocker):Apply similar spacing before all other test function definitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/app/user/routes.py(2 hunks)backend/app/user/schemas.py(1 hunks)backend/app/user/service.py(3 hunks)backend/tests/user/test_user_routes.py(2 hunks)backend/tests/user/test_user_service.py(5 hunks)docs/user-service.md(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/user/routes.py (3)
backend/app/user/service.py (2)
update_user_profile(47-62)delete_user(64-71)backend/app/user/schemas.py (1)
DeleteUserResponse(19-21)backend/app/auth/security.py (1)
get_current_user(102-119)
backend/tests/user/test_user_service.py (2)
backend/app/database.py (1)
get_database(31-37)backend/app/user/service.py (1)
transform_user_document(14-36)
🪛 Flake8 (7.2.0)
backend/app/user/routes.py
[error] 29-29: expected 2 blank lines, found 1
(E302)
backend/app/user/service.py
[error] 22-22: expected 1 blank line before a nested definition, found 0
(E306)
backend/app/user/schemas.py
[error] 5-5: expected 2 blank lines, found 1
(E302)
backend/tests/user/test_user_service.py
[error] 3-3: 'app.database.get_database' imported but unused
(F401)
[error] 82-82: expected 2 blank lines, found 1
(E302)
backend/tests/user/test_user_routes.py
[error] 6-6: 'datetime.datetime' imported but unused
(F401)
[error] 12-12: expected 2 blank lines, found 1
(E302)
[error] 29-29: local variable 'iso_date3' is assigned to but never used
(F841)
[error] 53-53: expected 2 blank lines, found 1
(E302)
[error] 65-65: expected 2 blank lines, found 1
(E302)
[error] 74-74: expected 2 blank lines, found 1
(E302)
[error] 91-91: expected 2 blank lines, found 1
(E302)
[error] 110-110: expected 2 blank lines, found 1
(E302)
[error] 116-116: expected 2 blank lines, found 1
(E302)
[error] 126-126: expected 2 blank lines, found 1
(E302)
[error] 134-134: expected 2 blank lines, found 1
(E302)
🪛 Ruff (0.11.9)
backend/app/user/routes.py
30-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/tests/user/test_user_service.py
3-3: app.database.get_database imported but unused
Remove unused import: app.database.get_database
(F401)
backend/tests/user/test_user_routes.py
6-6: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
29-29: Local variable iso_date3 is assigned to but never used
Remove assignment to unused variable iso_date3
(F841)
🪛 GitHub Check: codecov/patch
backend/app/user/service.py
[warning] 24-24: backend/app/user/service.py#L24
Added line #L24 was not covered by tests
[warning] 26-26: backend/app/user/service.py#L26
Added line #L26 was not covered by tests
🪛 Pylint (3.3.7)
backend/app/user/schemas.py
[refactor] 5-5: Too few public methods (0/2)
(R0903)
🔇 Additional comments (22)
backend/app/user/routes.py (3)
13-13: LGTM! Structured error response improves API consistency.The change from a plain string to a structured JSON object with
errorandmessagefields provides better error handling for API consumers.
23-23: LGTM! Consistent error response format.Good standardization of the validation error response format to match the other endpoints.
26-26: LGTM! Error response format aligns with API standards.The structured error format maintains consistency across all user endpoints.
backend/app/user/service.py (5)
29-29: LGTM! Field name standardization is consistent.The change from
_idtoidaligns with the API standardization goals and matches the schema updates.
32-32: LGTM! Backwards-compatible field name transition.The fallback logic
user.get("imageUrl") or user.get("avatar")provides good backwards compatibility during the transition period.
34-35: LGTM! Date field standardization with proper fallback.The conversion to camelCase and ISO 8601 strings is consistent, and the fallback to
created_atforupdatedAtwhen not present is appropriate.
53-55: Excellent security improvement with field filtering.The allowlist approach for update fields (
name,imageUrl,currency) prevents unauthorized field modifications and aligns with the schema updates.
71-71: Confirm delete logic change in user serviceI wasn’t able to find any other
delete_one()checks in the codebase to compare against, so please verify that switching fromresult.deleted_count == 1to
result.deleted_count > 0in
backend/app/user/service.py(line 71) was intentional. Althoughdelete_one()only ever returns 0 or 1, this looser check could mask unexpected behavior—let me know if this aligns with the intended delete semantics.docs/user-service.md (2)
39-39: LGTM! Field name standardization in documentation.The change from
image_urltoimageUrlaligns with the camelCase standardization implemented in the service layer.
41-47: Excellent clarification of date field handling.The inline comments and note clearly explain:
- API responses use ISO 8601 strings
- Backend frameworks may parse them as datetime objects
- Backend stores as datetime but serializes as strings
This documentation will help developers understand the data transformation process.
backend/app/user/schemas.py (4)
1-1: LGTM! EmailStr import aligns with schema requirements.Good addition of the EmailStr import to ensure proper email validation in the schemas.
6-6: LGTM! Schema simplification with service layer transformation.Removing the
_idalias makes sense since the service layer now handles the_idtoidtransformation intransform_user_document.
9-9: LGTM! Consistent field name standardization.Removing the
avataralias aligns with the service layer change to useimageUrldirectly.
17-17: LGTM! Currency field addition completes the update schema.The optional
currencyfield aligns with the allowlist filtering in the service layer'supdate_user_profilemethod.backend/tests/user/test_user_service.py (4)
30-35: Excellent improvement with fixed datetime constants.Using fixed datetime values instead of
datetime.now()makes tests deterministic and more reliable. The ISO string representations are properly formatted with 'Z' suffix for UTC.
41-41: LGTM! Test data aligns with schema changes.The field name updates (
avatar→imageUrl,_id→id, datetime objects → ISO strings) correctly reflect the service layer transformations.Also applies to: 48-48, 51-51, 53-55
71-78: LGTM! Comprehensive test coverage for minimal user data.The test properly validates the fallback behavior for optional fields and default values (
currency: "USD",imageUrl: None).
134-134: LGTM! Test assertions updated for new field names.The assertions correctly check for
idinstead of_idand validate the ISO string format forupdatedAt.Also applies to: 153-154
backend/tests/user/test_user_routes.py (4)
12-15: Good refactor to synchronous testing.The change from
httpx.AsyncClienttofastapi.testclient.TestClientsimplifies the test setup and aligns with the synchronous test approach used throughout the file.
30-38: Verify field name consistency with backend changes.The mock data correctly uses the new standardized field names (
id,imageUrl,createdAt,updatedAt) and ISO 8601 date format, which aligns with the PR objectives of standardizing API responses.
58-63: Good assertions for new field names and date format.The test correctly validates the new camelCase field names and ensures datetime fields are serialized as ISO 8601 strings ending with "Z".
70-70: Error response format verifiedThe test’s assertion exactly matches the routes’ implementation of error responses (
{"detail": {"error": "...", "message": "..."}}), so no changes are needed.
backend/app/user/service.py
Outdated
| # Ensure ISO 8601 string for dates | ||
| def iso(dt): | ||
| if not dt: | ||
| return None | ||
| if isinstance(dt, str): | ||
| return dt | ||
| return dt.isoformat().replace("+00:00", "Z") if dt.tzinfo else dt.isoformat() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ISO date conversion logic needs refinement and test coverage.
The iso function handles date-to-string conversion but has several concerns:
- Lines 24 and 26 are not covered by tests (static analysis)
- Missing blank line before nested function definition
- The timezone handling logic could be more robust
Apply this diff to fix formatting and improve the logic:
- # Ensure ISO 8601 string for dates
- def iso(dt):
+ # Ensure ISO 8601 string for dates
+
+ def iso(dt):
if not dt:
return None
if isinstance(dt, str):
return dt
- return dt.isoformat().replace("+00:00", "Z") if dt.tzinfo else dt.isoformat()
+ if hasattr(dt, 'isoformat'):
+ return dt.isoformat().replace("+00:00", "Z") if dt.tzinfo else dt.isoformat() + "Z"
+ return str(dt) # Fallback for unexpected typesThe test coverage issues should be addressed by adding tests for edge cases like string inputs and timezone-naive datetime objects.
🏁 Script executed:
#!/bin/bash
# Search for the iso function and its usages in the codebase
rg -n "def iso" -n backend/app
rg -n "iso(" -n backend/app
# Search for any tests covering iso or date conversion
rg -n "iso(" -n testsLength of output: 256
🏁 Script executed:
#!/bin/bash
# Inspect the transform_user_document implementation around the iso function
rg -n "def transform_user_document" -n backend/app/user/service.py
sed -n '1,200p' backend/app/user/service.py | sed -n '1,80p'
# Search for any tests invoking iso or transform_user_document
rg -nF "iso(" -n tests || echo "No iso usages in tests"
rg -nF "transform_user_document" -n tests || echo "No transform_user_document usages in tests"Length of output: 3076
Refine ISO date conversion logic and add comprehensive tests
Please update the nested iso helper in backend/app/user/service.py to improve readability, ensure consistent UTC output, and cover all branches with unit tests:
- Insert a blank line before the
def iso(dt):definition (PEP-8 spacing). - Replace the existing conversion logic with a UTC-normalizing approach that handles both naive and tz-aware datetimes, falling back to
str(dt)on error. - Add new tests under
tests/to cover:
•dt=None→ returnsNone
•dtasstr→ returns original string
• naivedatetime→ suffixed with “Z”
• awaredatetime(UTC and non-UTC) → converted to UTC “Z”
• unexpected types → converted viastr(dt)
Suggested diff:
--- a/backend/app/user/service.py
+++ b/backend/app/user/service.py
@@ -20,8 +20,11 @@ class UserService:
# Ensure ISO 8601 string for dates
- def iso(dt):
+
+ def iso(dt):
if not dt:
return None
if isinstance(dt, str):
return dt
- return dt.isoformat().replace("+00:00", "Z") if dt.tzinfo else dt.isoformat()
+ # Normalize to UTC and append 'Z'
+ try:
+ dt_utc = dt.astimezone(timezone.utc) if dt.tzinfo else dt.replace(tzinfo=timezone.utc)
+ return dt_utc.isoformat().replace("+00:00", "Z")
+ except AttributeError:
+ return str(dt)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-24: backend/app/user/service.py#L24
Added line #L24 was not covered by tests
[warning] 26-26: backend/app/user/service.py#L26
Added line #L26 was not covered by tests
🪛 Flake8 (7.2.0)
[error] 22-22: expected 1 blank line before a nested definition, found 0
(E306)
🤖 Prompt for AI Agents
In backend/app/user/service.py around lines 21 to 27, add a blank line before
the iso(dt) function definition to comply with PEP-8 spacing. Refactor the iso
function to normalize all datetime inputs to UTC, converting naive datetimes by
assuming UTC, converting aware datetimes to UTC, and formatting them with a "Z"
suffix. If conversion fails, return str(dt). Also, create comprehensive unit
tests in the tests/ directory covering all cases: None input returns None,
string input returns unchanged, naive datetime returns ISO format with "Z",
aware datetime (both UTC and non-UTC) converts to UTC with "Z", and unexpected
types convert to string.
There was a problem hiding this comment.
@vrajpatelll Thank you for addressing the ISO date conversion logic issues! I can see the iso function now includes proper spacing, robust UTC normalization, and comprehensive error handling as suggested. Great work!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/user/test_user_service.py (1)
105-141: Excellent comprehensive datetime transformation test coverage.These tests thoroughly verify the robust datetime handling for various edge cases including None values, string inputs, naive/aware datetimes, and timezone conversions.
Minor formatting fix needed:
-def test_transform_user_document_iso_unexpected_type(): - class Dummy: pass +def test_transform_user_document_iso_unexpected_type(): + class Dummy: + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/user/service.py(3 hunks)backend/tests/user/test_user_service.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/user/service.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/user/test_user_service.py (2)
backend/app/database.py (1)
get_database(31-37)backend/app/user/service.py (1)
transform_user_document(14-42)
🪛 Ruff (0.11.9)
backend/tests/user/test_user_service.py
3-3: app.database.get_database imported but unused
Remove unused import: app.database.get_database
(F401)
136-136: Multiple statements on one line (colon)
(E701)
🪛 Flake8 (7.2.0)
backend/tests/user/test_user_service.py
[error] 3-3: 'app.database.get_database' imported but unused
(F401)
[error] 82-82: expected 2 blank lines, found 1
(E302)
[error] 102-102: expected 2 blank lines, found 1
(E302)
[error] 105-105: expected 2 blank lines, found 1
(E302)
[error] 110-110: expected 2 blank lines, found 1
(E302)
[error] 115-115: expected 2 blank lines, found 1
(E302)
[error] 121-121: expected 2 blank lines, found 1
(E302)
[error] 128-128: expected 2 blank lines, found 1
(E302)
[error] 135-135: expected 2 blank lines, found 1
(E302)
[error] 136-136: multiple statements on one line (colon)
(E701)
🪛 Pylint (3.3.7)
backend/tests/user/test_user_service.py
[refactor] 136-136: Too few public methods (0/2)
(R0903)
🔇 Additional comments (4)
backend/tests/user/test_user_service.py (4)
3-35: LGTM! Deterministic datetime constants improve test reliability.The fixed datetime constants with corresponding ISO string representations are excellent for ensuring deterministic test behavior. The static analysis warning about unused
get_databaseimport is a false positive since it's used in the mock fixture at line 23.
37-55: Test data correctly reflects the schema standardization.The updates to use
imageUrlinstead ofavatar,idinstead of_id, and ISO string representations for datetime fields properly align with the API standardization objectives.
59-101: Existing tests properly updated for new schema.The updates maintain comprehensive test coverage while correctly adapting to the new field names and ISO date formatting requirements.
144-232: Service method tests properly updated for new schema.The tests correctly verify the service behavior with updated field names and deterministic datetime handling. The mock setup and assertions properly reflect the schema changes.
…factor(user)/update-user-service
Pull Request: User Service API Alignment & Improvements
Summary of changes:
idandimageUrlfields.createdAt,updatedAt) now usedatetimein backend models and are serialized as ISO 8601 strings in API responses.Result:
Backend, tests, and docs for user service are now fully aligned and consistent with the public API contract.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests