Enhance participant tracking with accountID and mainProfile attributes#177
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughStudies can opt into account tracking: participants store a hashed account ID and a main-profile flag. The study service hashes and stores account info on enter when enabled, management API toggles tracking per study, participant API supplies account context, and a migration job backfills existing participants. ChangesAccount Tracking and Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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)
pkg/study/study-service.go (1)
69-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist account tracking info for existing participants too.
AccountInfois only set in theisNewParticipantbranch, while already-active participants return early. That leaves tracked studies with stale/missingaccountInfofor existing participants.Suggested direction
- if isNewParticipant { - // save particicpant id profile lookup - if err = studyDBService.AddConfidentialIDMapEntry(instanceID, confidentialID, profileID, studyKey); err != nil { - slog.Error("Error saving participant ID profile lookup", slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("error", err.Error())) - } - if study.Configs.TrackAccount && accountID != "" { - // reuse same hashing mechanism to pseudonymize the account ID - hashedAccountID, hashErr := studyUtils.ProfileIDtoParticipantID(accountID, globalSecret, study.SecretKey, study.Configs.IdMappingMethod) - if hashErr != nil { - slog.Error("Error hashing account ID", slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("error", hashErr.Error())) - } else { - pState.AccountInfo = &studyTypes.AccountInfo{ - HashedAccountID: hashedAccountID, - IsMainProfile: isMainProfile, - } - } - } - } + if isNewParticipant { + if err = studyDBService.AddConfidentialIDMapEntry(instanceID, confidentialID, profileID, studyKey); err != nil { + slog.Error("Error saving participant ID profile lookup", slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("error", err.Error())) + } + } + + if study.Configs.TrackAccount && accountID != "" { + hashedAccountID, hashErr := studyUtils.ProfileIDtoParticipantID(accountID, globalSecret, study.SecretKey, study.Configs.IdMappingMethod) + if hashErr != nil { + slog.Error("Error hashing account ID", slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("error", hashErr.Error())) + } else { + pState.AccountInfo = &studyTypes.AccountInfo{ + HashedAccountID: hashedAccountID, + IsMainProfile: isMainProfile, + } + } + }Also apply the same update path before the early return for already-active participants.
Also applies to: 90-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/study/study-service.go` around lines 69 - 72, The early return when pState.StudyStatus == studyTypes.PARTICIPANT_STUDY_STATUS_ACTIVE prevents updating AccountInfo for existing participants; before returning AssignedSurveys, apply the same persistence/update path used in the isNewParticipant branch to write the participant's AccountInfo (and any tracking fields) to storage so tracked studies stay current. Locate the branch that sets AccountInfo in the isNewParticipant flow and replicate/invoke that update (or extract it into a helper) for the already-active case (also ensure the same fix is applied to the analogous block around the referenced 90-101 region) so both new and existing participants persist account tracking consistently.
🧹 Nitpick comments (2)
jobs/user-management/migrate-account-info.go (2)
84-91: 💤 Low valueConsider adding migration count for observability.
Other job functions in this codebase log the count of processed items. Adding a counter for successfully migrated participants would help operators verify the migration completed as expected.
Suggested approach
+ migratedCount := 0 + err = participantUserDBService.FindAndExecuteOnUsers( context.Background(), instanceID, bson.M{}, nil, false, func(user umTypes.User, args ...interface{}) error { // ... existing code ... _, err = studyDBService.SaveParticipantState(instanceID, study.Key, pState) if err != nil { slog.Error("Error saving participant state", ...) continue } + migratedCount++ slog.Debug("Migrated account info for participant", ...) } } return nil }, ) // ... error handling ... - slog.Info("Finished migrating account info for participants", slog.String("instanceID", instanceID)) + slog.Info("Finished migrating account info for participants", slog.String("instanceID", instanceID), slog.Int("migratedCount", migratedCount))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobs/user-management/migrate-account-info.go` around lines 84 - 91, Add a success counter to this migration so operators can observe how many participants were migrated: introduce a local integer (e.g., migratedCount) in the scope that runs the loop, increment it immediately after the successful SaveParticipantState call that returns no error (the block using studyDBService.SaveParticipantState, pState, participantID), and update the logging to include the count (either log migratedCount periodically or once at the end of the job along with the existing slog.Debug message). Ensure the counter is thread-safe only if this loop becomes concurrent; otherwise a simple int increment is sufficient, and reference migratedCount in the final slog log to show total migrated participants.
61-65: ⚡ Quick winUse idiomatic error handling to distinguish "not found" from other database errors.
The code silently continues on any error from
GetParticipantByID, including database connectivity issues, timeouts, or context cancellation. Only when the participant is not found in the study should the code skip silently. Other errors should be logged.Instead of string matching on error messages, use
errors.Is(err, mongo.ErrNoDocuments)to properly detect the "not found" case, which is the idiomatic MongoDB/Go approach:pState, err := studyDBService.GetParticipantByID(instanceID, study.Key, participantID) if err != nil { if !errors.Is(err, mongo.ErrNoDocuments) { // Log actual errors for visibility slog.Debug("Failed to fetch participant", slog.String("instanceID", instanceID), slog.String("studyKey", study.Key), slog.String("participantID", participantID), slog.String("error", err.Error())) } continue }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobs/user-management/migrate-account-info.go` around lines 61 - 65, The call to studyDBService.GetParticipantByID currently treats any error as "not found" and skips silently; change the error handling to detect mongo.ErrNoDocuments using errors.Is(err, mongo.ErrNoDocuments) and only silently continue for that case, while logging other errors (use slog.Debug or appropriate logger and include instanceID, study.Key, participantID and err.Error()); ensure you import "errors" and reference mongo.ErrNoDocuments when implementing the check around GetParticipantByID and keep the pState usage unchanged when err == nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/db/study/studyInfos.go`:
- Around line 193-199: Capture the UpdateOne result (res, err :=
collection.UpdateOne(ctx, filter, update)), check err as before, then if
res.MatchedCount == 0 return a clear not-found error (either a package-level
sentinel like ErrStudyNotFound or fmt.Errorf("study not found")) instead of
returning nil; reference collection.UpdateOne, filter, update and the result's
MatchedCount to locate and implement the check.
In `@services/management-api/apihandlers/study-management.go`:
- Around line 1419-1433: The request currently uses
StudyTrackAccountUpdateReq.TrackAccount as a bool so an omitted field silently
becomes false; update StudyTrackAccountUpdateReq to use *bool for TrackAccount
(or add a `binding:"required"` tag) and in updateStudyTrackAccount validate that
req.TrackAccount is not nil after ShouldBindJSON, returning HTTP 400 with an
appropriate error if nil, then use *req.TrackAccount for the actual
enable/disable logic; reference the StudyTrackAccountUpdateReq type and the
updateStudyTrackAccount handler when making these changes.
---
Outside diff comments:
In `@pkg/study/study-service.go`:
- Around line 69-72: The early return when pState.StudyStatus ==
studyTypes.PARTICIPANT_STUDY_STATUS_ACTIVE prevents updating AccountInfo for
existing participants; before returning AssignedSurveys, apply the same
persistence/update path used in the isNewParticipant branch to write the
participant's AccountInfo (and any tracking fields) to storage so tracked
studies stay current. Locate the branch that sets AccountInfo in the
isNewParticipant flow and replicate/invoke that update (or extract it into a
helper) for the already-active case (also ensure the same fix is applied to the
analogous block around the referenced 90-101 region) so both new and existing
participants persist account tracking consistently.
---
Nitpick comments:
In `@jobs/user-management/migrate-account-info.go`:
- Around line 84-91: Add a success counter to this migration so operators can
observe how many participants were migrated: introduce a local integer (e.g.,
migratedCount) in the scope that runs the loop, increment it immediately after
the successful SaveParticipantState call that returns no error (the block using
studyDBService.SaveParticipantState, pState, participantID), and update the
logging to include the count (either log migratedCount periodically or once at
the end of the job along with the existing slog.Debug message). Ensure the
counter is thread-safe only if this loop becomes concurrent; otherwise a simple
int increment is sufficient, and reference migratedCount in the final slog log
to show total migrated participants.
- Around line 61-65: The call to studyDBService.GetParticipantByID currently
treats any error as "not found" and skips silently; change the error handling to
detect mongo.ErrNoDocuments using errors.Is(err, mongo.ErrNoDocuments) and only
silently continue for that case, while logging other errors (use slog.Debug or
appropriate logger and include instanceID, study.Key, participantID and
err.Error()); ensure you import "errors" and reference mongo.ErrNoDocuments when
implementing the check around GetParticipantByID and keep the pState usage
unchanged when err == nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d991988-da3d-4ecd-8f6a-2628e627d655
📒 Files selected for processing (9)
jobs/user-management/init.gojobs/user-management/main.gojobs/user-management/migrate-account-info.gopkg/db/study/studyInfos.gopkg/study/study-service.gopkg/study/types/participant.gopkg/study/types/study.goservices/management-api/apihandlers/study-management.goservices/participant-api/apihandlers/study-service.go
| type StudyTrackAccountUpdateReq struct { | ||
| TrackAccount bool `json:"trackAccount"` | ||
| } | ||
|
|
||
| func (h *HttpEndpoints) updateStudyTrackAccount(c *gin.Context) { | ||
| token := c.MustGet("validatedToken").(*jwthandling.ManagementUserClaims) | ||
|
|
||
| studyKey := c.Param("studyKey") | ||
|
|
||
| var req StudyTrackAccountUpdateReq | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| slog.Error("failed to bind request", slog.String("error", err.Error())) | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
Require explicit trackAccount in the request body.
With bool, omitted JSON field defaults to false and still passes binding, so {} can silently disable tracking.
Suggested fix
type StudyTrackAccountUpdateReq struct {
- TrackAccount bool `json:"trackAccount"`
+ TrackAccount *bool `json:"trackAccount" binding:"required"`
}
@@
var req StudyTrackAccountUpdateReq
if err := c.ShouldBindJSON(&req); err != nil {
slog.Error("failed to bind request", slog.String("error", err.Error()))
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid request"})
return
}
@@
- err := h.studyDBConn.UpdateStudyTrackAccount(token.InstanceID, studyKey, req.TrackAccount)
+ err := h.studyDBConn.UpdateStudyTrackAccount(token.InstanceID, studyKey, *req.TrackAccount)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/management-api/apihandlers/study-management.go` around lines 1419 -
1433, The request currently uses StudyTrackAccountUpdateReq.TrackAccount as a
bool so an omitted field silently becomes false; update
StudyTrackAccountUpdateReq to use *bool for TrackAccount (or add a
`binding:"required"` tag) and in updateStudyTrackAccount validate that
req.TrackAccount is not nil after ShouldBindJSON, returning HTTP 400 with an
appropriate error if nil, then use *req.TrackAccount for the actual
enable/disable logic; reference the StudyTrackAccountUpdateReq type and the
updateStudyTrackAccount handler when making these changes.
Greptile SummaryThis PR adds opt-in account tracking to studies, storing a pseudonymized
Confidence Score: 4/5Safe to merge with awareness of the open HashedAccountID exposure in getParticipantState (flagged in a prior review round and not yet addressed) and the minor IsMainProfile inconsistency between the migration and production paths for edge-case user accounts. The core tracking logic, permission gating, and migration job are well-structured. The getParticipantState endpoint still returns HashedAccountID to participant clients (noted in a previous review round), and the migration uses GetMainAndOtherProfiles to derive IsMainProfile while production reads profile.MainProfile directly — these two paths diverge for accounts with no designated main profile. Neither is a data-loss risk, but the leaked field is a privacy concern for the pseudonymization goal of this feature. services/participant-api/apihandlers/study-service.go (getParticipantState returns HashedAccountID to the client) and jobs/user-management/migrate-account-info.go (IsMainProfile derivation differs from production).
|
| Filename | Overview |
|---|---|
| jobs/user-management/migrate-account-info.go | New migration job backfills HashedAccountID/IsMainProfile for existing participants in tracking-enabled studies; correctly guards against empty accountID; minor IsMainProfile inconsistency vs. production code for accounts with no designated main profile. |
| pkg/study/study-service.go | OnEnterStudy extended to accept accountID/isMainProfile and populates HashedAccountID+IsMainProfile for new participants in tracking-enabled studies; re-entering participants (isNewParticipant=false) are intentionally skipped (handled by migration). |
| pkg/study/types/participant.go | Adds HashedAccountID and IsMainProfile pointer fields to Participant struct; both fields use omitempty but are included in JSON responses served to participant clients. |
| pkg/study/types/study.go | Adds TrackAccount bool to StudyConfigs; DEFAULT_ID_MAPPING_METHOD corrected from sha-256 to sha256 (the recognized method string), creating a hashing algorithm difference between legacy and new studies. |
| services/management-api/apihandlers/study-management.go | Adds PUT /track-account endpoint with correct permission checks; handler reads studyKey from URL params and delegates directly to DB layer. |
| services/participant-api/apihandlers/study-service.go | enterStudy refactored to fetch user once and call FindProfile directly, forwarding accountID and isMainProfile to OnEnterStudy; getParticipantState does not strip HashedAccountID before returning to the client. |
| pkg/user-management/utils/profiles.go | Adds empty-slice guard to prevent index-out-of-bounds panic when a user has no profiles and no main profile is designated. |
Sequence Diagram
sequenceDiagram
participant Client
participant ParticipantAPI
participant UserDB
participant StudyService
participant StudyDB
Client->>ParticipantAPI: PUT /studies/:studyKey/enter
ParticipantAPI->>UserDB: GetUser(instanceID, userID)
UserDB-->>ParticipantAPI: user with AccountID and Profiles
ParticipantAPI->>ParticipantAPI: FindProfile to get isMainProfile
ParticipantAPI->>StudyService: OnEnterStudy with accountID and isMainProfile
StudyService->>StudyDB: GetStudy and check TrackAccount flag
StudyService->>StudyDB: GetParticipantByID and check isNewParticipant
alt isNewParticipant and TrackAccount and accountID not empty
StudyService->>StudyService: Hash accountID with study secret
StudyService->>StudyService: Set HashedAccountID and IsMainProfile on pState
end
StudyService->>StudyDB: SaveParticipantState
StudyService-->>ParticipantAPI: assignedSurveys
ParticipantAPI-->>Client: 200 assignedSurveys
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
jobs/user-management/migrate-account-info.go:54-58
`IsMainProfile` computed differently in migration vs. production: `enterStudy` sets `isMainProfile = profile.MainProfile` directly from the stored flag, while this migration calls `GetMainAndOtherProfiles`, which falls back to promoting the first non-main profile as main when no profile has `MainProfile == true`. For accounts in that inconsistent state the two code paths produce different `IsMainProfile` values. Using `profile.MainProfile` directly (and removing the `GetMainAndOtherProfiles` call) keeps the migration consistent with what production writes.
Reviews (7): Last reviewed commit: "Move account check before fetching profi..." | Re-trigger Greptile
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/study/study-service.go (1)
85-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackfill missing account metadata on re-entry, not just first insert.
Because this branch only runs for
isNewParticipant, participants that already exist whentrackAccountis turned on will keepHashedAccountID/IsMainProfileunset even after they enter again. The new per-study toggle makes that state reachable, so this should opportunistically fill missing fields instead of relying solely on a separate migration.💡 Proposed fix
if isNewParticipant { // save particicpant id profile lookup if err = studyDBService.AddConfidentialIDMapEntry(instanceID, confidentialID, profileID, studyKey); err != nil { slog.Error("Error saving participant ID profile lookup", slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("error", err.Error())) } - if study.Configs.TrackAccount && accountID != "" { - // reuse same hashing mechanism to pseudonymize the account ID - hashedAccountID, hashErr := studyUtils.ProfileIDtoParticipantID(accountID, globalSecret, study.SecretKey, study.Configs.IdMappingMethod) - if hashErr != nil { - slog.Error("Error hashing account ID", slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("error", hashErr.Error())) - } else { - pState.HashedAccountID = &hashedAccountID - pState.IsMainProfile = &isMainProfile - } - } } + + if study.Configs.TrackAccount && accountID != "" && (pState.HashedAccountID == nil || pState.IsMainProfile == nil) { + // reuse same hashing mechanism to pseudonymize the account ID + hashedAccountID, hashErr := studyUtils.ProfileIDtoParticipantID(accountID, globalSecret, study.SecretKey, study.Configs.IdMappingMethod) + if hashErr != nil { + slog.Error("Error hashing account ID", slog.String("instanceID", instanceID), slog.String("studyKey", studyKey), slog.String("error", hashErr.Error())) + } else { + pState.HashedAccountID = &hashedAccountID + pState.IsMainProfile = &isMainProfile + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/study/study-service.go` around lines 85 - 99, The code only fills HashedAccountID/IsMainProfile when isNewParticipant is true, so participants who re-enter after TrackAccount toggles keep those fields nil; change the logic where study.Configs.TrackAccount && accountID != "" to run not only for isNewParticipant but also when the stored state is missing (e.g., pState.HashedAccountID == nil || pState.IsMainProfile == nil), call studyUtils.ProfileIDtoParticipantID(accountID, globalSecret, study.SecretKey, study.Configs.IdMappingMethod) and on success set pState.HashedAccountID and pState.IsMainProfile, then persist the updated participant state using the existing DB updater (use the same persistence path you use elsewhere in this file, e.g., the function that saves participant state) so missing account metadata is backfilled on re-entry.
🧹 Nitpick comments (1)
jobs/user-management/migrate-account-info.go (1)
49-50: ⚡ Quick winMove the empty-
accountIDcheck before the profile/study fan-out.
accountIDis user-level, but this guard runs after computing participant IDs and loading participant state for every profile/study pair. That creates avoidable DB traffic for users who cannot be migrated anyway.Suggested fix
func(user umTypes.User, args ...interface{}) error { accountID := user.Account.AccountID + if accountID == "" { + return nil + } mainProfileID, _ := umUtils.GetMainAndOtherProfiles(user) @@ - if accountID == "" { - continue - } - // Reuse same hashing mechanism to pseudonymize the account IDAlso applies to: 74-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jobs/user-management/migrate-account-info.go` around lines 49 - 50, The code is doing expensive per-profile/study work before checking if the user has an accountID; move the empty-accountID guard up so we return early and skip computing profiles and loading participant state for unmigratable users: check accountID (user.Account.AccountID) is non-empty before calling umUtils.GetMainAndOtherProfiles and before any participant/state fan-out (the logic around GetMainAndOtherProfiles and subsequent participant/state loading), and apply the same early-return check for the similar block referenced at the later occurrence (the code around lines with the second accountID guard).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/study/study-service.go`:
- Around line 85-99: The code only fills HashedAccountID/IsMainProfile when
isNewParticipant is true, so participants who re-enter after TrackAccount
toggles keep those fields nil; change the logic where study.Configs.TrackAccount
&& accountID != "" to run not only for isNewParticipant but also when the stored
state is missing (e.g., pState.HashedAccountID == nil || pState.IsMainProfile ==
nil), call studyUtils.ProfileIDtoParticipantID(accountID, globalSecret,
study.SecretKey, study.Configs.IdMappingMethod) and on success set
pState.HashedAccountID and pState.IsMainProfile, then persist the updated
participant state using the existing DB updater (use the same persistence path
you use elsewhere in this file, e.g., the function that saves participant state)
so missing account metadata is backfilled on re-entry.
---
Nitpick comments:
In `@jobs/user-management/migrate-account-info.go`:
- Around line 49-50: The code is doing expensive per-profile/study work before
checking if the user has an accountID; move the empty-accountID guard up so we
return early and skip computing profiles and loading participant state for
unmigratable users: check accountID (user.Account.AccountID) is non-empty before
calling umUtils.GetMainAndOtherProfiles and before any participant/state fan-out
(the logic around GetMainAndOtherProfiles and subsequent participant/state
loading), and apply the same early-return check for the similar block referenced
at the later occurrence (the code around lines with the second accountID guard).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b2832ce-8db1-4517-b812-04123b3a612f
📒 Files selected for processing (3)
jobs/user-management/migrate-account-info.gopkg/study/study-service.gopkg/study/types/participant.go
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
This PR introduces a new feature to track account information for study participants. Enables studies to optionally track pseudonymized account IDs and main profile status for participants, with a migration job and management endpoints to control this feature.
Summary by CodeRabbit
New Features
Chores