Add additional data to user for calculating required nutrients#37
Add additional data to user for calculating required nutrients#37LightJack05 merged 7 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds nutritional profiling fields to the user model to enable calculation of required nutrients. The changes include birthdate, sex, weight, and activity_level fields across the API and database layers.
Changes:
- Added four new user fields (birthdate, sex, weight, activity_level) to the database schema via migration
- Updated POST, GET, and PATCH endpoints to handle the new user fields
- Modified database queries to include the new fields in user creation, retrieval, and updates
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| db/migrations/0003_add-user-data.up.sql | Adds four new nullable columns to the users table |
| db/migrations/0003_add-user-data.down.sql | Provides rollback by dropping the four new columns |
| internal/database/user/model.go | Extends User struct with the four new fields using non-nullable types |
| internal/database/user/queries.go | Updates queries to handle new fields and adds individual update functions for each field |
| internal/handlers/userhandler/post.go | Adds new required fields to user registration with basic validation |
| internal/handlers/userhandler/patch.go | Adds optional update capability for new fields with transaction support |
| internal/handlers/userhandler/get.go | Includes new fields in user profile response |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Birthdate *time.Time `json:"birthdate,omitempty"` | ||
| Sex *string `json:"sex,omitempty"` | ||
| Weight *float64 `json:"weight,omitempty"` |
There was a problem hiding this comment.
The Weight field in the PATCH request lacks validation to ensure positive values, similar to the issue in the POST handler. When a user updates their weight via PATCH, they could provide zero or negative values, which would be invalid for nutrient calculations.
| Birthdate *time.Time `json:"birthdate,omitempty"` | ||
| Sex *string `json:"sex,omitempty"` | ||
| Weight *float64 `json:"weight,omitempty"` | ||
| ActivityLevel *float64 `json:"activityLevel,omitempty"` |
There was a problem hiding this comment.
The ActivityLevel field in the PATCH request lacks validation to ensure it's within a valid range, similar to the issue in the POST handler. When a user updates their activity level via PATCH, they could provide invalid values that would cause issues in nutrient calculations.
| type userPatchBody struct { | ||
| DisplayName *string `json:"displayName,omitempty"` | ||
|
|
||
| Birthdate *time.Time `json:"birthdate,omitempty"` |
There was a problem hiding this comment.
The Birthdate field in the PATCH request lacks validation to prevent future dates or unreasonably old dates, similar to the issue in the POST handler. When a user updates their birthdate via PATCH, they could provide invalid dates.
| DisplayName *string `json:"displayName,omitempty"` | ||
|
|
||
| Birthdate *time.Time `json:"birthdate,omitempty"` | ||
| Sex *string `json:"sex,omitempty"` | ||
| Weight *float64 `json:"weight,omitempty"` | ||
| ActivityLevel *float64 `json:"activityLevel,omitempty"` |
There was a problem hiding this comment.
The JSON field naming is inconsistent with other endpoints. The PATCH endpoint uses camelCase (displayName, activityLevel) while the POST and GET endpoints use snake_case (display_name, activity_level). For consistency, the PATCH endpoint should also use snake_case to match the existing convention.
|
|
||
| Birthdate time.Time `json:"birthdate" binding:"required"` | ||
| Sex string `json:"sex" binding:"required,len=1"` | ||
| Weight float64 `json:"weight" binding:"required"` |
There was a problem hiding this comment.
The Weight field lacks validation to ensure it's a positive value. Weight should not be zero or negative, but there's no validation constraint (e.g., binding:"gt=0") to enforce this. Invalid weight values could cause issues in downstream nutrient calculations.
| Weight float64 `json:"weight" binding:"required"` | |
| Weight float64 `json:"weight" binding:"required,gt=0"` |
| sqlStatement := `SELECT id, username, display_name, password_hash, birthdate, sex, weight, activity_level FROM users WHERE username=$1` | ||
| var user User | ||
| row := db.QueryRow(sqlStatement, username) | ||
| err := row.Scan(&user.ID, &user.Username, &user.DisplayName, &user.PasswordHash) | ||
| err := row.Scan(&user.ID, &user.Username, &user.DisplayName, &user.PasswordHash, &user.Birthdate, &user.Sex, &user.Weight, &user.ActivityLevel) |
There was a problem hiding this comment.
The database schema allows NULL values for the new columns (birthdate, sex, weight, activity_level), but the User model uses non-nullable types (time.Time, string, float64). When scanning a NULL value from the database into these fields, the scan operation will fail with an error. This will break GetUserByUsername for any existing users who don't have these fields populated. Either the database columns should be NOT NULL with default values, or the User model should use nullable types (e.g., *time.Time, *string, *float64) to handle existing users gracefully.
| sqlStatement := `INSERT INTO users (username, display_name, password_hash, birthdate, sex, weight, activity_level) VALUES ($1, $2, $3, $4, $5, $6, $7)` | ||
| _, err := db.Exec(sqlStatement, user.Username, user.DisplayName, user.PasswordHash, user.Birthdate, user.Sex, user.Weight, user.ActivityLevel) |
There was a problem hiding this comment.
The INSERT statement has a mismatch between column count and placeholder count. The statement declares 7 columns (username, display_name, password_hash, birthdate, sex, weight, activity_level) but only provides 3 placeholders ($1, $2, $3) in the VALUES clause. This should include placeholders $4, $5, $6, and $7 for the additional columns. The correct VALUES clause should be: VALUES ($1, $2, $3, $4, $5, $6, $7)
| Birthdate time.Time `json:"birthdate" binding:"required"` | ||
| Sex string `json:"sex" binding:"required,len=1"` | ||
| Weight float64 `json:"weight" binding:"required"` | ||
| ActivityLevel float64 `json:"activity_level" binding:"required"` |
There was a problem hiding this comment.
The ActivityLevel field lacks validation to ensure it's within a valid range. Activity level should likely be constrained to positive values or a specific range, but there's no validation constraint to enforce this. Invalid activity level values could cause issues in downstream nutrient calculations.
| ActivityLevel float64 `json:"activity_level" binding:"required"` | |
| ActivityLevel float64 `json:"activity_level" binding:"required,gt=0,lt=10"` |
| Password string `json:"password" binding:"required,min=8"` | ||
| DisplayName string `json:"display_name" binding:"required"` | ||
|
|
||
| Birthdate time.Time `json:"birthdate" binding:"required"` |
There was a problem hiding this comment.
The Birthdate field lacks validation to prevent future dates or unreasonably old dates. Users could provide a birthdate in the future or hundreds of years in the past, which would be invalid. Consider adding validation to ensure the birthdate is in the past and within a reasonable range (e.g., not more than 150 years ago).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
LightJack05
force-pushed
the
user-data
branch
from
c577194 to
ff84b8a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Birthdate *time.Time `json:"birthdate,omitempty"` | ||
| Sex *string `json:"sex,omitempty" binding:"omitempty,len=1"` | ||
| Weight *float64 `json:"weight,omitempty"` | ||
| ActivityLevel *float64 `json:"activityLevel,omitempty"` |
There was a problem hiding this comment.
Inconsistent JSON field naming convention detected. The PATCH endpoint uses camelCase (birthdate, activityLevel) while the POST and GET endpoints use snake_case (birthdate, activity_level) for the same fields. This creates an inconsistent API where clients must use different field names depending on the operation. For example, POST uses "activity_level" but PATCH uses "activityLevel". All user-related endpoints should use the same naming convention for consistency.
| ActivityLevel *float64 `json:"activityLevel,omitempty"` | |
| ActivityLevel *float64 `json:"activity_level,omitempty"` |
| ADD COLUMN birthdate DATE, | ||
| ADD COLUMN sex TEXT, | ||
| ADD COLUMN weight DOUBLE PRECISION, | ||
| ADD COLUMN activity_level DOUBLE PRECISION; |
There was a problem hiding this comment.
The database migration adds columns without NOT NULL constraints, but the POST handler requires all these fields (birthdate, sex, weight, activity_level) to be present. This mismatch creates a potential issue: existing users in the database will have NULL values for these fields, but the code expects these fields to always be populated. When retrieving existing users via GET, the code will return zero-values (empty time.Time, empty string, 0.0) instead of NULL, which is misleading. Consider either: (1) making these fields optional in the POST handler and allowing NULL values, or (2) adding a data migration to populate default values for existing users and adding NOT NULL constraints.
| ADD COLUMN birthdate DATE, | |
| ADD COLUMN sex TEXT, | |
| ADD COLUMN weight DOUBLE PRECISION, | |
| ADD COLUMN activity_level DOUBLE PRECISION; | |
| ADD COLUMN birthdate DATE NOT NULL DEFAULT CURRENT_DATE, | |
| ADD COLUMN sex TEXT NOT NULL DEFAULT '', | |
| ADD COLUMN weight DOUBLE PRECISION NOT NULL DEFAULT 0, | |
| ADD COLUMN activity_level DOUBLE PRECISION NOT NULL DEFAULT 0; |
| func UpdateBirthdate(db *sql.Tx, username string, birthdate time.Time) error { | ||
| // update birthdate in table users | ||
| sqlStatement := `UPDATE users SET birthdate=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, birthdate, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update birthdate in database: %v", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func UpdateSex(db *sql.Tx, username, sex string) error { | ||
| // update sex in table users | ||
| sqlStatement := `UPDATE users SET sex=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, sex, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update sex in database: %v", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func UpdateWeight(db *sql.Tx, username string, weight float64) error { | ||
| // update weight in table users | ||
| sqlStatement := `UPDATE users SET weight=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, weight, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update weight in database: %v", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func UpdateActivityLevel(db *sql.Tx, username string, activityLevel float64) error { | ||
| // update activity_level in table users | ||
| sqlStatement := `UPDATE users SET activity_level=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, activityLevel, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update activity level in database: %v", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing godoc comments for the new Update functions. The existing functions in this file (GetUserByUsername, CreateUser) have godoc comments explaining their purpose, but the new Update functions (UpdateBirthdate, UpdateSex, UpdateWeight, UpdateActivityLevel) only have inline comments. For consistency with the codebase (see queries.go:9, queries.go:26), add godoc comments that describe what each function does.
| func (u *UserController) updateUser(patchBody userPatchBody, username string) error { | ||
| tx, err := u.DB.Begin() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to begin transaction: %v", err) | ||
| } | ||
| defer tx.Rollback() | ||
| if patchBody.DisplayName != nil { | ||
| if err := u.updateDisplayName(username, *patchBody.DisplayName); err != nil { | ||
| if err := user.UpdateDisplayName(tx, username, *patchBody.DisplayName); err != nil { | ||
| return fmt.Errorf("failed to update display name: %v", err) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (u *UserController) updateDisplayName(username, displayName string) error { | ||
| err := user.UpdateDisplayName(u.DB, username, displayName) | ||
| if err != nil { | ||
| return fmt.Errorf("request failed: %v", err) | ||
| if patchBody.Birthdate != nil { | ||
| if err := user.UpdateBirthdate(tx, username, *patchBody.Birthdate); err != nil { | ||
| return fmt.Errorf("failed to update birthdate: %v", err) | ||
| } | ||
| } | ||
| if patchBody.Sex != nil { | ||
| if err := user.UpdateSex(tx, username, *patchBody.Sex); err != nil { | ||
| return fmt.Errorf("failed to update sex: %v", err) | ||
| } | ||
| } | ||
| if patchBody.Weight != nil { | ||
| if err := user.UpdateWeight(tx, username, *patchBody.Weight); err != nil { | ||
| return fmt.Errorf("failed to update weight: %v", err) | ||
| } | ||
| } | ||
| if patchBody.ActivityLevel != nil { | ||
| if err := user.UpdateActivityLevel(tx, username, *patchBody.ActivityLevel); err != nil { | ||
| return fmt.Errorf("failed to update activity level: %v", err) | ||
| } | ||
| } | ||
| if err := tx.Commit(); err != nil { | ||
| return fmt.Errorf("failed to commit transaction: %v", err) | ||
| } |
There was a problem hiding this comment.
Missing error logging in the updateUser function. For consistency with other handlers in the codebase (e.g., userhandler/post.go:65), database errors should be logged before being returned. This is particularly important for transaction errors which can be difficult to debug without proper logging. Consider adding logging for transaction begin, update, and commit failures.
| func UpdateBirthdate(db *sql.Tx, username string, birthdate time.Time) error { | ||
| // update birthdate in table users | ||
| sqlStatement := `UPDATE users SET birthdate=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, birthdate, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update birthdate in database: %v", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func UpdateSex(db *sql.Tx, username, sex string) error { | ||
| // update sex in table users | ||
| sqlStatement := `UPDATE users SET sex=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, sex, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update sex in database: %v", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func UpdateWeight(db *sql.Tx, username string, weight float64) error { | ||
| // update weight in table users | ||
| sqlStatement := `UPDATE users SET weight=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, weight, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update weight in database: %v", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func UpdateActivityLevel(db *sql.Tx, username string, activityLevel float64) error { | ||
| // update activity_level in table users | ||
| sqlStatement := `UPDATE users SET activity_level=$1 WHERE username=$2` | ||
| _, err := db.Exec(sqlStatement, activityLevel, username) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to update activity level in database: %v", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
High code duplication across the new Update functions (UpdateBirthdate, UpdateSex, UpdateWeight, UpdateActivityLevel). Each function follows the exact same pattern with only the column name and parameter differing. This makes maintenance more difficult as bug fixes or improvements must be replicated across all functions. Consider consolidating these into a single generic update function that accepts the column name and value, or use a query builder pattern to reduce duplication.
1 participant
No description provided.