Conversation
| @@ -0,0 +1,41 @@ | |||
| name: Deploy API Docs to GitHub Pages | |||
There was a problem hiding this comment.
| @@ -0,0 +1,22 @@ | |||
| version: "3.9" | |||
There was a problem hiding this comment.
can ignore - this is for local testing
| @@ -0,0 +1,41 @@ | |||
| { | |||
There was a problem hiding this comment.
can ignore - for local testing
| @@ -0,0 +1,29 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
can ignore - for local testing
| return datetime.now(timezone.utc).strftime('%Y-%m-%dT%H:%M:%S.%f')[:-3] + 'Z' | ||
|
|
||
|
|
||
| class UserType(str, Enum): |
There was a problem hiding this comment.
UserTypes
Really only need Neighbor and Volunteer right now, but in the future ORGANIZER or HOST might have some special privileges
There was a problem hiding this comment.
Mobile App would have to change check from isVolunteer to user.userType == 'Volunteer'
| email: Optional[str] = None | ||
| phoneNumber: Optional[str] = None | ||
| zipcode: Optional[str] = None | ||
| fcmToken: Optional[str] = None #TODO: Consider making this a set to allow for multiple devices to receive notifications |
There was a problem hiding this comment.
when signing out we need to set this to None
we should also consider allowing for multiple devices to receive notifications
There was a problem hiding this comment.
I think fcmToken should be a list of tokens to allow for multiple devices
| } | ||
|
|
||
|
|
||
| def error_response(status_code: int, code: ErrorCode, message: str, field: Optional[str] = None, |
There was a problem hiding this comment.
API errors are returned in this format:
Status Code: 400-500
Headers: X-Request-Id - to query Cloudwatch
{
"error": {
"code": "INTERNAL_ERROR_CODE",
"message": "MESSAGE"
}
}
example:
{
"error": {
"code": "FORBIDDEN",
"message": "User can only update their own user data"
}
}
| Authorizers: | ||
| FirebaseAuthorizer: | ||
| JwtConfiguration: | ||
| issuer: !Sub "https://securetoken.google.com/${FirebaseProjectId}" |
There was a problem hiding this comment.
Api Gateway Validates JWT tokens
User Service API Endpoints1. Create UserPOST
Request Body: {
"userId": "firebase-uid-here",
"username": "mariamekaba",
"email": "mariame@example.com",
"phoneNumber": "+1234567890",
"zipcode": "12345",
"fcmToken": "fcm-token-here",
"settings": {
"pushNotificationEnabled": true,
"emailNotificationEnabled": true,
"geofenceEnabled": false
},
"userType": "Neighbor"
}Success Response (201 Created): {
"user": {
"userId": "firebase-uid-here",
...,
"createdAt": "2024-03-20T14:22:00.000Z",
"lastUpdated": "2024-03-20T14:22:00.000Z",
"lastLoginAt": "2024-03-20T14:22:00.000Z"
}
}2. Get UserGET
{
"user": {
"userId": "firebase-uid-here",
...
}
}3. Update UserPATCH
Request Body (example - update email and settings): {
"email": "newemail@example.com",
"settings": {
"pushNotificationEnabled": false
}
}Success Response (200 OK): {
"user": {
"userId": "firebase-uid-here",
"email": "newemail@example.com",
"settings": {
"pushNotificationEnabled": false,
"emailNotificationEnabled": true,
"geofenceEnabled": false
},
"lastUpdated": "2024-03-20T15:30:00.000Z",
...,
}
}4. Delete UserDELETE
Success Response (204 No Content): 5. Check Username AvailabilityGET
Success Response (200 OK) - Available: {
"available": true
}Success Response (200 OK) - Taken: {
"available": false
}API errors are returned in this format:Status Code: 400-500 example: API Base URLs
AuthenticationAll endpoints (except username check) require Firebase JWT in the Documentation📖 Interactive API Docs: https://fridgefinder.github.io/CFM_User/ |
| - '*' | ||
|
|
||
| # DynamoDB Table for Users | ||
| UsersTable: |
There was a problem hiding this comment.
Hash/PrimaryKey - userId
Indexed - username
| parameter_overrides = [ | ||
| "Environment=dev", | ||
| "DeploymentTarget=aws", | ||
| "FirebaseProjectId=YOUR_STAGING_FIREBASE_PROJECT_ID", |
There was a problem hiding this comment.
TOOD: create firebase project on fridgefinder google account for: Dev and Prod
| # Fields that can be updated (excluding userType - handled above) | ||
| allowed_fields = { | ||
| 'email', 'phoneNumber', 'username', 'points', | ||
| 'zipcode', 'fcmToken', 'lastLoginAt', 'settings' |
There was a problem hiding this comment.
lastLoginAt - I think there should be an api that updates this instead of the frontend sending the lastLoginAt field
| GEOFENCE_ENABLED = "geofenceEnabled" | ||
|
|
||
|
|
||
| class User(BaseModel): |
There was a problem hiding this comment.
Reviewer: User Model, confirm fields and structure
mansoorsiddiqui
left a comment
There was a problem hiding this comment.
Solid work overall, looks clean. Most of my comments are around a few edge cases and making sure we're aligned on the cross-service contract with the notification PR.
Cross-PR concerns:
- EventBridge contract gap: The notification PR subscribes to
"User Deleted"events from"user-service", but this PR never publishes that event. Notification cleanup won't happen on account deletion. - Error code naming: This PR uses
USER_NOT_FOUND,USER_ALREADY_EXISTSwhile the notification PR usesNOT_FOUND,ALREADY_EXISTS. The Flutter app will need to parse errors from both — should we align on a convention? - Error response format: Both PRs use
{"error": {"code": "...", "message": "..."}}which is good, but the OpenAPI spec here documents it differently. Let's make sure the specs match the implementations.
| Uses Pydantic's model_dump with enum values as strings | ||
| Excludes None values to save space | ||
| """ | ||
| return self.model_dump(mode='json', exclude_none=True) |
There was a problem hiding this comment.
Quick sanity check — to_dict() uses exclude_none=True, and then update_user does a full put_item which replaces the entire DynamoDB item. So if a user clears a field (sets it to None), the old value in DynamoDB would get dropped because put_item replaces everything. Is that the intended behavior? Just want to make sure we're aligned on that.
There was a problem hiding this comment.
Yes that was the intended behavior
Not sure why the API client would set any fields from not-None to None, but if it is set to None: it is recommended to not have the field at all in the db
There was a problem hiding this comment.
If there are fields we don't want to go from VALUE -> NONE, we could block that as it might be a mistake? But left it open for now
| push: | ||
| branches: | ||
| - main | ||
| - U1_JP #TODO: remove this branch after testing |
There was a problem hiding this comment.
U1_JP branch is still in the trigger list with a #TODO: remove this branch after testing. Don't forget to pull this out before merging to main.
There was a problem hiding this comment.
yes will remove before merging in
|
User Model and APIs
API docs: https://fridgefinder.github.io/CFM_User/