Study engine/send message now#137
Conversation
… sender in StudyEngine
…led breaks for clarity
…ionality in StudyEngine
…for email and phone types
…and enhance message sending configuration
WalkthroughAdds a pluggable StudyMessageSender to the study engine, extends Init to accept and register it, implements a concrete sender and SendInstantStudyEmail, adds SEND_MESSAGE_NOW action and payload assembly, adjusts expression loop control, swaps email-template lookup arg order with structured logging, and introduces typed contact-info and GetEmail. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Service as Service (management/participant)
participant Sender as StudyMessageSender
participant Study as study.Init
participant Engine as StudyEngine
Service->>Sender: NewStudyMessageSender(deps, config) %% participant-api only
Service->>Study: Init(db, globalSecret, externalServices, Sender/nil)
Study->>Engine: RegisterStudyMessageSender(Sender)
note right of Engine: Engine can invoke SEND_MESSAGE_NOW action
sequenceDiagram
autonumber
participant Engine as StudyEngine
participant Sender as StudyMessageSender
participant DBs as DBs (study/participant/messaging/globalinfos)
participant Email as Email Templating
rect rgba(230,250,230,0.6)
note over Engine,Sender: Action: SEND_MESSAGE_NOW
Engine->>Engine: Build extra payload (flags, linkingCodes, eventData)
Engine->>Sender: SendInstantStudyEmail(instanceID, studyKey, confidentialPID, messageType, payload, opts)
end
Sender->>DBs: Resolve profile ID, load user/profile
alt create login token
Sender->>DBs: Persist temporary login token
end
Sender->>Email: Render & send email template with payload
Email-->>Sender: send result
Sender-->>Engine: success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (6)
pkg/study/studyengine/expressions.go (1)
400-406: Guard the type assertion to prevent a panic on non-boolean results.
expResult.(bool)will panic if the inner expression does not resolve to a boolean (misconfigured rules, user error, or future changes). Prefer a safe assertion and return a typed error.Apply this diff:
- val := expResult.(bool) + val, ok := expResult.(bool) + if !ok { + return false, fmt.Errorf("checkConditionForOldResponses: expected bool result, got %T", expResult) + }pkg/user-management/types/user.go (4)
84-95: Guard against type/address mismatches when confirming contact infoThe condition checks the requested type t but doesn’t ensure the matched ContactInfo actually has the same Type. If data is inconsistent, we might confirm the wrong record. Add a ci.Type == t guard.
Apply this diff:
func (u *User) ConfirmContactInfo(t string, addr string) error { for i, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } + if t == CONTACT_INFO_TYPE_EMAIL && ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } else if t == CONTACT_INFO_TYPE_PHONE && ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } } return errors.New("contact not found") }
97-107: Also guard ci.Type in SetContactInfoVerificationSentSame mismatch risk as above; set the sent-at timestamp only when type and address both match the same record.
Apply this diff:
func (u *User) SetContactInfoVerificationSent(t string, addr string) { for i, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + if t == CONTACT_INFO_TYPE_EMAIL && ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() return - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + } else if t == CONTACT_INFO_TYPE_PHONE && ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() return } } }
109-118: FindContactInfoByTypeAndAddr should also verify ci.Type matches tCurrently it matches on requested type and address, but does not ensure the ContactInfo’s Type matches. Add the ci.Type check to avoid false positives.
Apply this diff:
func (u User) FindContactInfoByTypeAndAddr(t string, addr string) (ContactInfo, bool) { for _, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + if t == CONTACT_INFO_TYPE_EMAIL && ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { return ci, true - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + } else if t == CONTACT_INFO_TYPE_PHONE && ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { return ci, true } } return ContactInfo{}, false }
129-143: Bug: RemoveContactInfo doesn’t clean up contact preferences on successful removalThe docstring says it should remove references from contact preferences too, but on success it returns without that cleanup. This can leave dangling IDs in ContactPreferences.SendNewsletterTo.
Apply this diff:
if ci.ID.Hex() == id { if u.Account.Type == ACCOUNT_TYPE_EMAIL && ci.Email == u.Account.AccountID { return errors.New("cannot remove main address") } - u.ContactInfos = append(u.ContactInfos[:i], u.ContactInfos[i+1:]...) - return nil + u.ContactInfos = append(u.ContactInfos[:i], u.ContactInfos[i+1:]...) + u.RemoveContactInfoFromContactPreferences(id) + return nil }Optionally, consider making RemoveContactInfoFromContactPreferences return whether it changed anything and log if a dangling reference was found when the contact itself didn’t exist.
pkg/study/study-service.go (1)
1106-1114: Bug: GetParticipantByID called with profileID instead of participantID.This will fail for most studies because participantID != profileID. Use the computed participantID from above.
- pState, err := studyDBService.GetParticipantByID(instanceID, studyKey, profileID) + pState, err := studyDBService.GetParticipantByID(instanceID, studyKey, participantID)
🧹 Nitpick comments (16)
pkg/study/studyengine/expressions.go (3)
420-425: Count branch: early-exit is correct; minor idiomatic tweak.
- Logic to return as soon as
counter >= checkForCountis set is correct and improves performance.- Nit:
counter++is a bit more idiomatic thancounter += 1in Go.- counter += 1 + counter++
309-328: Validate non-positive count threshold upfront to avoid surprising results.If
checkForis "count" andcheckForCount <= 0, the expression is trivially satisfied. Currently it can only return true after encountering at least onetrueresponse, which may be surprising for0. Consider short-circuiting.switch arg1Val := arg1.(type) { case string: checkFor = arg1Val case float64: checkFor = "count" checkForCount = int(arg1Val) default: return val, fmt.Errorf("type unknown %T", arg1Val) } + if checkFor == "count" && checkForCount <= 0 { + return true, nil + }
391-429: Drop the loop label by using early returns to simplify control flow.Labels are correct but can be avoided here, reducing cognitive load and indentation. This keeps behavior identical while making intent clearer.
-responsesLoop: for _, resp := range responses { oldEvalContext := EvalContext{ ParticipantState: ctx.ParticipantState, Event: StudyEvent{ Response: resp, }, } expResult, err := ExpressionEval(*condition, oldEvalContext) if err != nil { return false, err } - val := expResult.(bool) + val, ok := expResult.(bool) + if !ok { + return false, fmt.Errorf("checkConditionForOldResponses: expected bool result, got %T", expResult) + } switch checkFor { case "all": if val { result = true } else { - result = false - break responsesLoop + return false, nil } case "any": if val { - result = true - break responsesLoop + return true, nil } case "count": if val { - counter += 1 + counter++ if counter >= checkForCount { - result = true - break responsesLoop + return true, nil } } } } return result, nilpkg/user-management/types/user.go (1)
12-13: Introduce a dedicatedContactInfoTypealias and centralize “email”/“phone” constantsDefining a typed alias and moving these values into one file makes the intent clearer and prevents future drift. Here’s what to do:
• Create
pkg/user-management/types/contact_info_types.gocontaining:package types // ContactInfoType distinguishes how a user can be contacted. type ContactInfoType string const ( ContactInfoTypeEmail ContactInfoType = "email" ContactInfoTypePhone ContactInfoType = "phone" )• In
pkg/user-management/types/user.go, replaceconst CONTACT_INFO_TYPE_EMAIL = "email" const CONTACT_INFO_TYPE_PHONE = "phone"with
// deprecated: use types.ContactInfoTypeEmail const CONTACT_INFO_TYPE_EMAIL = string(ContactInfoTypeEmail) // deprecated: use types.ContactInfoTypePhone const CONTACT_INFO_TYPE_PHONE = string(ContactInfoTypePhone)• Update the signature of methods like
FindContactInfoByTypeAndAddrto acceptContactInfoTypeinstead ofstring, and change all calls fromuser.FindContactInfoByTypeAndAddr("email", addr)to
user.FindContactInfoByTypeAndAddr(types.ContactInfoTypeEmail, addr)• Migrate the raw literals you found to use the new constants; key locations include (but aren’t limited to):
– services/participant-api/apihandlers/utils.go (lines 40, 71)
– services/participant-api/apihandlers/password-reset.go
– services/participant-api/apihandlers/user-management.go (lines 299, 330, 340)
– services/participant-api/apihandlers/authentication.go (e.g. lines 713–715, 867–872)
– jobs/user-management/main.go (lines 133, 223)
– pkg/user-management/types/otp.go (EmailOTP constant)
– pkg/study/exporter/survey-definition/compToResponseDef.go (cases “email”/“phone”)
– jobs/messaging/scheduled-messages.go (lines 108–110, 159–163, 228–230, 273–275)
– services/management-api/apihandlers/user-management.go (lines 253, 257)
– services/management-api/apihandlers/management-auth.go (logging on sign-up/sign-in)You can batch-migrate with your IDE or
rg+sed, and then remove the deprecated untyped constants once the codebase no longer references them.pkg/messaging/email-sending/utils.go (1)
34-38: Prefer structured error fields (slog.Any) and include language for better triage.Logging err.Error() as a string loses stack details when aggregated. Also, lang is part of the fetch context and helpful when diagnosing missing translations.
Apply:
- slog.Error("failed to fetch email template", - slog.String("instanceID", instanceID), - slog.String("messageType", messageType), - slog.String("studyKey", studyKey), - slog.String("error", err.Error())) + slog.Error("failed to fetch email template", + slog.String("instanceID", instanceID), + slog.String("studyKey", studyKey), + slog.String("messageType", messageType), + slog.String("lang", lang), + slog.Any("err", err))pkg/study/studyengine/types.go (4)
24-25: Field addition is fine; document that nil is a valid/expected state.Given jobs pass nil intentionally, a short comment on messageSender being optional will help future maintainers.
type StudyEngine struct { studyDBService StudyDBService externalServices []ExternalService - messageSender StudyMessageSender + // messageSender may be nil (e.g., in jobs that do not send messages). + messageSender StudyMessageSender }
38-46: Avoid using the global inside a method; use the receiver and add a nil receiver guard.Current code unnecessarily couples to the global singleton. Using the receiver improves testability and clarity.
-func (se *StudyEngine) RegisterStudyMessageSender(sender StudyMessageSender) { - if CurrentStudyEngine != nil { - CurrentStudyEngine.messageSender = sender - } else { - slog.Error("StudyEngine not initialized, cannot register message sender") - } -} +func (se *StudyEngine) RegisterStudyMessageSender(sender StudyMessageSender) { + if se == nil { + slog.Error("StudyEngine not initialized, cannot register message sender") + return + } + se.messageSender = sender +}
93-97: Clarify time unit for ExpiresAt to avoid ambiguity.Document that the value is Unix seconds to prevent misinterpretation (ms vs s).
type SendOptions struct { - ExpiresAt int64 // if message could not sent until this time, it will be discarded + ExpiresAt int64 // Unix seconds. If sending hasn't succeeded by this timestamp, discard the message. LanguageOverride string }
101-110: Consider accepting context.Context for cancellation/deadlines.If sending can involve network IO or IO-bound operations, a context allows cooperative cancellation and better deadline propagation. If changing the interface now is too invasive, consider a follow-up.
pkg/study/study-service.go (1)
28-43: Init wiring is correct; consider logging when no sender is provided.Registering a nil sender is acceptable. Emitting a debug/info log when sender is nil can help catch misconfigurations during deployment.
studyengine.InitStudyEngine(studyDB, externalServices) -studyengine.CurrentStudyEngine.RegisterStudyMessageSender(studyMessageSender) +studyengine.CurrentStudyEngine.RegisterStudyMessageSender(studyMessageSender) +if studyMessageSender == nil { + slog.Debug("No StudyMessageSender configured; SEND_MESSAGE_NOW actions will be no-ops") +}pkg/study/studyengine/actions.go (3)
63-65: New SEND_MESSAGE_NOW action: add to docs/playbooks and expression builders.Looks good. Please ensure authoring tools, docs, and any validation that enumerates action names include "SEND_MESSAGE_NOW" to avoid runtime “action name not known” errors when composing expressions.
653-674: Avoid duplicate ‘studyKey’ in payload (sender adds it again).getExtraPayload injects "studyKey", and the sender builds a payload with "studyKey" too. This duplication is harmless but redundant and can mask accidental divergence. Suggest removing it here and letting the sender be the single source of truth.
Apply this diff:
-func getExtraPayload(pState studyTypes.Participant, event StudyEvent) map[string]string { - payload := map[string]string{ - "studyKey": event.StudyKey, - } +func getExtraPayload(pState studyTypes.Participant, event StudyEvent) map[string]string { + payload := map[string]string{}
676-734: Harden argument parsing and preconditions; improve error clarity.
- Don’t reuse ok1 for language parsing; use a separate variable for readability.
- Fail fast if ParticipantIDForConfidentialResponses is empty; currently the sender fails later with a generic “profileID lookup failed”.
Apply this diff:
- languageOverride := "" + languageOverride := "" if len(action.Data) > 1 { arg2, err := EvalContext.ExpressionArgResolver(action.Data[1]) if err != nil { return newState, err } - languageOverride, ok1 = arg2.(string) - if !ok1 { + langOK := false + languageOverride, langOK = arg2.(string) + if !langOK { slog.Debug("could not parse language override") } } + // Precondition: we need a confidential participant ID to resolve user/profile + if event.ParticipantIDForConfidentialResponses == "" { + slog.Debug("sendMessageNow: missing participantID for confidential responses") + return newState, errors.New("sendMessageNow requires participantIDForConfidentialResponses") + } + extraPayload := getExtraPayload(newState.PState, event)I can add unit tests for:
- missing sender
- missing args
- language override parsing
- payload composition (flags/linkingCodes/eventData)
- ExpiresAt ≈ Now()+24h (using the overridable Now()).
services/management-api/init.go (1)
166-171: Option: pass nil until Management API gains messaging configuration.Safer default to avoid partial configuration.
Apply this diff:
- study.Init( - studyDBService, - conf.StudyConfigs.GlobalSecret, - conf.StudyConfigs.ExternalServices, - sender, - ) + study.Init( + studyDBService, + conf.StudyConfigs.GlobalSecret, + conf.StudyConfigs.ExternalServices, + nil, // no messaging sender in management-api until messaging is initialized here + )pkg/study/studyengine/sender/sender.go (2)
118-129: Default ExpiresAt if not provided.Make the sender robust to callers forgetting to set ExpiresAt by applying a sensible default (e.g., 24h).
Apply this diff:
- err = emailsending.SendInstantEmailByTemplate( + expiresAt := opts.ExpiresAt + if expiresAt == 0 { + expiresAt = time.Now().Add(24 * time.Hour).Unix() + } + err = emailsending.SendInstantEmailByTemplate( instanceID, to, messageType, studyKey, lang, payload, false, // useLowPrio - opts.ExpiresAt, + expiresAt, )
136-151: Guard zero TTL for login tokens.If LoginTokenTTL was not configured, default to a safe value to avoid creating already-expired tokens.
Apply this diff:
- tempTokenInfos := umTypes.TempToken{ + ttl := s.loginTokenTTL + if ttl == 0 { + ttl = 24 * time.Hour + } + tempTokenInfos := umTypes.TempToken{ UserID: user.ID.Hex(), InstanceID: instanceID, Purpose: umTypes.TOKEN_PURPOSE_SURVEY_LOGIN, Info: map[string]string{"studyKey": studyKey}, - Expiration: umUtils.GetExpirationTime(s.loginTokenTTL), + Expiration: umUtils.GetExpirationTime(ttl), }I can add focused tests with fakes for studyDB/participantUserDB/globalInfosDB to exercise:
- missing/invalid confidentialPID
- missing profiles
- language override and payload merge
- zero TTL and zero ExpiresAt defaults.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
jobs/messaging/init.go(1 hunks)jobs/study-timer/init.go(1 hunks)jobs/user-management/init.go(1 hunks)pkg/messaging/email-sending/utils.go(2 hunks)pkg/study/study-service.go(2 hunks)pkg/study/studyengine/actions.go(2 hunks)pkg/study/studyengine/expressions.go(2 hunks)pkg/study/studyengine/sender/sender.go(1 hunks)pkg/study/studyengine/types.go(4 hunks)pkg/user-management/types/user.go(9 hunks)services/management-api/init.go(2 hunks)services/participant-api/init.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/study/study-service.go (2)
pkg/study/studyengine/sender/sender.go (1)
StudyMessageSender(20-27)pkg/study/studyengine/types.go (2)
StudyMessageSender(101-110)CurrentStudyEngine(28-28)
pkg/study/studyengine/sender/sender.go (6)
pkg/study/studyengine/types.go (1)
SendOptions(94-97)pkg/user-management/types/account.go (1)
Account(3-15)pkg/messaging/email-sending/send.go (1)
SendInstantEmailByTemplate(61-110)pkg/user-management/types/user.go (1)
User(15-23)pkg/user-management/types/temptoken.go (2)
TempToken(19-27)TOKEN_PURPOSE_SURVEY_LOGIN(13-13)pkg/user-management/utils/temp-tokens.go (1)
GetExpirationTime(33-35)
pkg/study/studyengine/actions.go (4)
pkg/study/types/participant.go (1)
Participant(14-26)pkg/study/studyengine/types.go (5)
StudyEvent(76-85)ActionData(57-60)CurrentStudyEngine(28-28)EvalContext(88-91)SendOptions(94-97)pkg/study/types/expression.go (1)
Expression(3-10)pkg/study/studyengine/helpers.go (1)
Now(13-13)
services/management-api/init.go (2)
pkg/study/studyengine/sender/sender.go (2)
NewStudyMessageSender(34-49)MessageSenderConfig(29-32)pkg/study/study-service.go (1)
Init(28-43)
pkg/user-management/types/user.go (2)
pkg/user-management/types/contact-info.go (1)
ContactInfo(5-12)pkg/user-management/types/account.go (1)
Account(3-15)
services/participant-api/init.go (3)
pkg/study/studyengine/sender/sender.go (2)
NewStudyMessageSender(34-49)MessageSenderConfig(29-32)pkg/messaging/types/messaging-config.go (1)
MessagingConfigs(12-22)pkg/study/study-service.go (1)
Init(28-43)
🔇 Additional comments (17)
pkg/study/studyengine/expressions.go (2)
391-398: Good fix: labeled break exits the for-range loop (not just the switch).Using
responsesLoop:andbreak responsesLoopcorrectly implements early-exit from the outer loop, which the previous plainbreakwould not do inside aswitch. This is both a correctness and performance improvement when the condition is met early.
406-418: Ensure and Document Empty-Set Semantics forcheckConditionForOldResponses
- Confirm that returning
falsefor the"all"case when there are zero responses is your intended behavior (many definitions treat “all of an empty set” astrue).- Add explicit unit tests covering an empty-response scenario:
t.Run("checkType all – empty responses", …)expectingtrueorfalseper your spect.Run("checkType any – empty responses", …)expectingfalse- Update the function’s GoDoc (or the README) to clearly state the empty-set behavior for
"all"and"any"pkg/user-management/types/user.go (4)
25-37: LGTM: AddNewEmail now uses CONTACT_INFO_TYPE_EMAILReplacing the inline literal with the constant improves consistency. No functional issues spotted.
48-63: LGTM: SetPhoneNumber filtering and creation use CONTACT_INFO_TYPE_PHONEThe migration away from magic strings is correct; the behavior of replacing existing phone entries is preserved.
65-73: LGTM: ConfirmPhoneNumber uses CONTACT_INFO_TYPE_PHONEThe change preserves behavior while aligning with constants.
75-82: LGTM: GetPhoneNumber uses CONTACT_INFO_TYPE_PHONEBehavior maintained; naming and returns remain consistent with the rest of the API.
pkg/messaging/email-sending/utils.go (1)
31-32: Argument order swap verified across all call sites
I’ve checked every invocation ofGetStudyEmailTemplateByMessageTypein the repo—including
- services/management-api/apihandlers/messaging-service.go
- pkg/messaging/email-sending/utils.go
- pkg/db/messaging/email-templates.go
- jobs/messaging/participant-messages.go
- jobs/messaging/researcher-notifications.go
All calls now correctly use
(instanceID, studyKey, messageType)per the updated DB API. No further changes are needed.jobs/user-management/init.go (1)
248-255: Nil sender safety confirmed for study service initialization.All calls to SendInstantStudyEmail in pkg/study/studyengine are properly guarded by a nil‐check on CurrentStudyEngine.messageSender (see actions.go around line 679), so passing
nilininitStudyServiceis safe. No unguarded messaging paths are reachable from this job, and no further changes are required.pkg/study/study-service.go (1)
28-43: All study.Init call sites are using the 4-argument signatureI’ve reviewed every invocation in:
- services/management-api/init.go
- services/participant-api/init.go
- jobs/user-management/init.go
- jobs/study-timer/init.go
- jobs/messaging/init.go
and confirmed they all pass four parameters (including the global secret and a message sender or nil). There are no remaining 3-arg calls.
jobs/messaging/init.go (1)
208-215: Nil sender usage is safe—no further action neededI ran a search for any direct calls to
SEND_MESSAGE_NOW,SendInstantStudyEmail, or use of.messageSenderwithin thejobs/messagingcode and found no matches (only the handlers inpkg/study/studyengine, which guard against a nil sender). Moreover, instudyengine/actions.go, the"SEND_MESSAGE_NOW"case checks forCurrentStudyEngine.messageSender == niland returns early before any call toSendInstantStudyEmail, ensuring a nil sender no-ops gracefully.No code changes are required here. Great work!
jobs/study-timer/init.go (2)
135-141: Init signature update wired correctly (passing nil sender).The updated call to study.Init matches the new 4-arg signature, and passing nil for the StudyMessageSender is appropriate here since this job does not send messages.
135-141: Confirm this job never triggers SEND_MESSAGE_NOW actions.If this process evaluates a study rule with SEND_MESSAGE_NOW, the action will return an error ("message sender for study engine not registered"). If that’s acceptable, keep nil; otherwise consider wiring a real sender or guarding against this action in job configs.
services/management-api/init.go (1)
154-165: Management API builds a sender but never initializes the email-sending module.This process doesn’t call emailsending.InitMessageSendingVariables, so SendInstantStudyEmail will fail here with “connection to smtp bridge not initialized.” If Management API isn’t meant to send immediate study emails, prefer passing nil instead of constructing a sender. If it should, wire the messaging config and call emailsending.InitMessageSendingVariables.
services/participant-api/init.go (3)
17-18: Import alias for sender is appropriate.The import and aliasing are clear and consistent with usage.
170-174: Good: message-sending config is initialized before creating the sender.Ordering guarantees the email-sending module is ready before the sender is registered with the study engine.
269-286: Sender wiring and TTL/constants look correct.Constructing the StudyMessageSender with a 24h login token TTL and passing messaging constants from config is sound.
pkg/study/studyengine/sender/sender.go (1)
3-7: No changes needed: Go 1.23.0 in go.mod already supports maps.CopyThe
go.modfile declaresgo 1.23.0, which is ≥ 1.21 and thus includes themapspackage and itsCopyfunction—no update required.
…ure accurate participant state retrieval
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/user-management/types/user.go (4)
48-63: SetPhoneNumber drops existing phone ContactInfos but leaves stale IDs in ContactPreferencesOld phone ContactInfo IDs remain referenced (e.g., SendNewsletterTo), causing dangling references and potential “contact not found” at send time. Remove those references when you purge old phone entries.
Apply this diff:
func (u *User) SetPhoneNumber(phone string) { - var newContactInfos []ContactInfo + var newContactInfos []ContactInfo + var removedPhoneIDs []string for _, ci := range u.ContactInfos { - if ci.Type == CONTACT_INFO_TYPE_PHONE { - continue - } + if ci.Type == CONTACT_INFO_TYPE_PHONE { + removedPhoneIDs = append(removedPhoneIDs, ci.ID.Hex()) + continue + } newContactInfos = append(newContactInfos, ci) } contactInfo := ContactInfo{ ID: primitive.NewObjectID(), Type: CONTACT_INFO_TYPE_PHONE, ConfirmedAt: 0, Phone: phone, } - u.ContactInfos = append(newContactInfos, contactInfo) + // Clean up dangling references in preferences + for _, id := range removedPhoneIDs { + u.RemoveContactInfoFromContactPreferences(id) + } + u.ContactInfos = append(newContactInfos, contactInfo) }
84-95: ConfirmContactInfo should also check ci.Type; current code can match by field aloneGuard against accidental matches by validating both the requested type and the stored ContactInfo.Type.
Apply this diff:
func (u *User) ConfirmContactInfo(t string, addr string) error { for i, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } + switch t { + case CONTACT_INFO_TYPE_EMAIL: + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } + case CONTACT_INFO_TYPE_PHONE: + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } + } } return errors.New("contact not found") }
109-118: FindContactInfoByTypeAndAddr ignores ci.Type; function name implies it should validate typeUse ci.Type in the match to prevent false positives and align behavior with the name.
Apply this diff:
func (u User) FindContactInfoByTypeAndAddr(t string, addr string) (ContactInfo, bool) { for _, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - return ci, true - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - return ci, true - } + switch t { + case CONTACT_INFO_TYPE_EMAIL: + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + return ci, true + } + case CONTACT_INFO_TYPE_PHONE: + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + return ci, true + } + } } return ContactInfo{}, false }
144-153: Docstring vs implementation mismatch: remove all references, not just the firstThe comment says “delete all references,” but the current code removes one occurrence then returns. Filter the slice to drop all matches.
Apply this diff:
func (u *User) RemoveContactInfoFromContactPreferences(id string) { // remove address from contact preferences - for i, addrRef := range u.ContactPreferences.SendNewsletterTo { - if addrRef == id { - u.ContactPreferences.SendNewsletterTo = append(u.ContactPreferences.SendNewsletterTo[:i], u.ContactPreferences.SendNewsletterTo[i+1:]...) - return - } - } + filtered := u.ContactPreferences.SendNewsletterTo[:0] + for _, addrRef := range u.ContactPreferences.SendNewsletterTo { + if addrRef != id { + filtered = append(filtered, addrRef) + } + } + u.ContactPreferences.SendNewsletterTo = filtered }
♻️ Duplicate comments (1)
pkg/user-management/types/user.go (1)
39-46: GetEmail may pick an unconfirmed or non-primary address; prioritize main login email, then confirmed emailsThis mirrors an existing review. Given the PR introduces messaging, picking the wrong address is high-risk. Implement priority:
- If Account.Type == ACCOUNT_TYPE_EMAIL, return the contact whose Email == Account.AccountID.
- Else return first confirmed email.
- Else fall back to first email.
Apply this diff:
func (u *User) GetEmail() (ContactInfo, error) { - for _, ci := range u.ContactInfos { - if ci.Type == CONTACT_INFO_TYPE_EMAIL { - return ci, nil - } - } - return ContactInfo{}, errors.New("email not found") + // 1) Prefer the main account email when account type is email + if u.Account.Type == ACCOUNT_TYPE_EMAIL && u.Account.AccountID != "" { + if ci, ok := u.FindContactInfoByTypeAndAddr(CONTACT_INFO_TYPE_EMAIL, u.Account.AccountID); ok { + return ci, nil + } + } + // 2) Fallback to the first confirmed email + for _, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.ConfirmedAt > 0 { + return ci, nil + } + } + // 3) Fallback to the first email + for _, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_EMAIL { + return ci, nil + } + } + return ContactInfo{}, errors.New("email not found") }Run this to validate and catch other literal usages that should switch to constants:
#!/bin/bash # Find call sites of GetEmail to assess assumptions rg -n --type go -C2 '\bGetEmail\s*\(' # Find any remaining hard-coded type-string comparisons rg -n --type go -P '\bType\s*==\s*"email"|\bType\s*==\s*"phone"' # Broad scan for raw "email"/"phone" tokens in code (review results manually) rg -n --type go -P '\b"email"\b|\b"phone"\b' -g '!**/vendor/**'
🧹 Nitpick comments (4)
pkg/user-management/types/user.go (4)
12-14: Good move: centralize contact-info type strings behind constants; minor Go naming nitReplacing string literals with constants reduces bugs and improves consistency. Nit: Go style typically prefers PascalCase for exported identifiers (e.g., ContactInfoTypeEmail), but this file already uses ACCOUNT_TYPE_EMAIL, so keeping it consistent is reasonable.
65-73: ConfirmPhoneNumber confirms the first phone entry; consider confirming by exact numberGiven SetPhoneNumber enforces a single phone, this is likely fine. If multiple phones can exist from legacy data, consider passing the target number to avoid confirming the wrong entry.
// Optional alternative signature for precision: func (u *User) ConfirmPhoneNumber(phone string) error { /* match by type + number */ }
75-82: Prefer confirmed phone number when availableMirror the email selection logic to reduce the chance of using an unverified number for outbound messaging.
Apply this diff:
func (u *User) GetPhoneNumber() (ContactInfo, error) { - for _, ci := range u.ContactInfos { - if ci.Type == CONTACT_INFO_TYPE_PHONE { - return ci, nil - } - } - return ContactInfo{}, errors.New("phone number not found") + // Prefer confirmed phone + for _, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.ConfirmedAt > 0 { + return ci, nil + } + } + // Fallback to any phone + for _, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_PHONE { + return ci, nil + } + } + return ContactInfo{}, errors.New("phone number not found") }
97-107: SetContactInfoVerificationSent: also check ci.Type to avoid touching the wrong recordUse a switch and validate both type and field to be consistent with ConfirmContactInfo.
Apply this diff:
func (u *User) SetContactInfoVerificationSent(t string, addr string) { for i, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() - return - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() - return - } + switch t { + case CONTACT_INFO_TYPE_EMAIL: + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() + return + } + case CONTACT_INFO_TYPE_PHONE: + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() + return + } + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/study/study-service.go(3 hunks)pkg/user-management/types/user.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/study/study-service.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/user-management/types/user.go (2)
pkg/user-management/types/contact-info.go (1)
ContactInfo(5-12)pkg/user-management/types/account.go (1)
Account(3-15)
🔇 Additional comments (3)
pkg/user-management/types/user.go (3)
25-37: AddNewEmail: Looks goodUses the new CONTACT_INFO_TYPE_EMAIL and timestamps correctly. No concerns.
129-142: RemoveContactInfo: Correctly blocks removal of main login email and cleans preferences before deletionThe guard against removing the primary email and the in-loop removal is sound. With the proposed fix to RemoveContactInfoFromContactPreferences (below) to remove all references, this path looks good.
155-163: ReplaceContactInfoInContactPreferences: LGTMIterates and replaces all occurrences; consistent with expected behavior.
…ure accurate contact info handling
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/user-management/types/user.go (1)
61-76: Bug: SetPhoneNumber replaces the ContactInfo object and may leave dangling references in ContactPreferencesBy filtering out all phone-type entries and creating a new ContactInfo (new ID), any existing references in ContactPreferences.SendNewsletterTo will point to a non-existent ID after this call.
Fix by updating in place when a phone entry exists (preserves ID and any references). Only append when none exists:
func (u *User) SetPhoneNumber(phone string) { - var newContactInfos []ContactInfo - for _, ci := range u.ContactInfos { - if ci.Type == CONTACT_INFO_TYPE_PHONE { - continue - } - newContactInfos = append(newContactInfos, ci) - } - contactInfo := ContactInfo{ - ID: primitive.NewObjectID(), - Type: CONTACT_INFO_TYPE_PHONE, - ConfirmedAt: 0, - Phone: phone, - } - u.ContactInfos = append(newContactInfos, contactInfo) + for i, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_PHONE { + // No-op if same number; otherwise update in place and reset confirmation. + if ci.Phone == phone { + return + } + u.ContactInfos[i].Phone = phone + u.ContactInfos[i].ConfirmedAt = 0 + return + } + } + // No existing phone on record; append new ContactInfo. + contactInfo := ContactInfo{ + ID: primitive.NewObjectID(), + Type: CONTACT_INFO_TYPE_PHONE, + ConfirmedAt: 0, + Phone: phone, + } + u.ContactInfos = append(u.ContactInfos, contactInfo) }If you intentionally want a new ID per phone change, then also clean up preferences:
- u.ContactInfos = append(newContactInfos, contactInfo) + // Remove any dangling references to old phone IDs + for _, old := range u.ContactInfos { + if old.Type == CONTACT_INFO_TYPE_PHONE { + u.RemoveContactInfoFromContactPreferences(old.ID.Hex()) + } + } + u.ContactInfos = append(newContactInfos, contactInfo)Please confirm which behavior you want; I can adjust tests accordingly.
🧹 Nitpick comments (6)
pkg/user-management/types/user.go (6)
39-59: GetEmail selection logic is robust and resolves prior riskPrefers the main account email when account type is email, otherwise falls back to confirmed email, then any email. This addresses the previously noted risk of selecting an unconfirmed/non-primary address.
Suggested tests to add:
- Account.Type == email and AccountID present in ContactInfos → returns that ContactInfo.
- Account.Type == email but AccountID absent → returns first confirmed email.
- No confirmed emails → returns first email.
- No emails → returns error.
Do you want me to open a test PR with these cases?
41-44: Consider case-insensitive matching for the main emailIf emails aren’t normalized, exact string compare can miss the main address. Use strings.EqualFold for email comparisons.
Apply this change in FindContactInfoByTypeAndAddr (concrete diff provided below in a related comment). Also add:
+ import "strings"to the imports.
88-95: Prefer returning a confirmed phone number when availableFor parity with GetEmail, return a confirmed phone first, then fall back.
func (u *User) GetPhoneNumber() (ContactInfo, error) { - for _, ci := range u.ContactInfos { - if ci.Type == CONTACT_INFO_TYPE_PHONE { - return ci, nil - } - } + // Prefer confirmed phone + for _, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.ConfirmedAt > 0 { + return ci, nil + } + } + // Fall back to any phone + for _, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_PHONE { + return ci, nil + } + } return ContactInfo{}, errors.New("phone number not found") }
99-105: Reduce duplication with a switch on type in ConfirmContactInfoFunctionality is fine; a small refactor improves readability.
func (u *User) ConfirmContactInfo(t string, addr string) error { - for i, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } - } + for i, ci := range u.ContactInfos { + switch t { + case CONTACT_INFO_TYPE_EMAIL: + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } + case CONTACT_INFO_TYPE_PHONE: + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } + } + } return errors.New("contact not found") }
112-118: Likewise: switch on type in SetContactInfoVerificationSentKeeps branches symmetric and easier to extend.
func (u *User) SetContactInfoVerificationSent(t string, addr string) { - for i, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() - return - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() - return - } - } + for i, ci := range u.ContactInfos { + switch t { + case CONTACT_INFO_TYPE_EMAIL: + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() + return + } + case CONTACT_INFO_TYPE_PHONE: + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + u.ContactInfos[i].ConfirmationLinkSentAt = time.Now().Unix() + return + } + } + } }
124-127: Harden FindContactInfoByTypeAndAddr and support case-insensitive email compare
- Early-continue on mismatched types to reduce duplication.
- Use strings.EqualFold for email address comparison to avoid case pitfalls.
-func (u User) FindContactInfoByTypeAndAddr(t string, addr string) (ContactInfo, bool) { - for _, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - return ci, true - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - return ci, true - } - } - return ContactInfo{}, false -} +func (u User) FindContactInfoByTypeAndAddr(t string, addr string) (ContactInfo, bool) { + for _, ci := range u.ContactInfos { + if ci.Type != t { + continue + } + switch t { + case CONTACT_INFO_TYPE_EMAIL: + if strings.EqualFold(ci.Email, addr) { + return ci, true + } + case CONTACT_INFO_TYPE_PHONE: + if ci.Phone == addr { + return ci, true + } + } + } + return ContactInfo{}, false +}Add import:
import ( "errors" "time" + "strings" "go.mongodb.org/mongo-driver/bson/primitive" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/user-management/types/user.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/user-management/types/user.go (2)
pkg/user-management/types/contact-info.go (1)
ContactInfo(5-12)pkg/user-management/types/account.go (1)
Account(3-15)
🔇 Additional comments (4)
pkg/user-management/types/user.go (4)
12-14: Good move: introduced contact-info type constants to eliminate magic stringsThis aligns with existing ACCOUNT_TYPE_EMAIL and improves consistency across the package.
29-31: Switched AddNewEmail to use CONTACT_INFO_TYPE_EMAILRemoves a magic string and keeps type checks uniform.
78-86: ConfirmPhoneNumber: constant usage is correctReads cleanly and sets confirmation timestamp on the phone entry.
146-149: Correctly prevent removal of main email and clean up preferences only on successUsing ACCOUNT_TYPE_EMAIL for the guard is consistent, and moving RemoveContactInfoFromContactPreferences into the success path avoids accidental dangling changes when the contact isn’t found.
…eNow function and improve error logging
…onses function to improve clarity and performance
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/participant-api/apihandlers/authentication.go (2)
713-717: Fix compile error: pass ContactInfoType instead of string to typed APIsFindContactInfoByTypeAndAddr and SetContactInfoVerificationSent now accept ContactInfoType. Passing "email" will not compile.
Apply this diff:
- ci, found := user.FindContactInfoByTypeAndAddr("email", req.Email) + ci, found := user.FindContactInfoByTypeAndAddr(userTypes.CONTACT_INFO_TYPE_EMAIL, req.Email) if !found { slog.Warn("email not found", slog.String("email", req.Email)) c.JSON(http.StatusBadRequest, gin.H{"error": "email not found"}) return } ... - user.SetContactInfoVerificationSent("email", req.Email) + user.SetContactInfoVerificationSent(userTypes.CONTACT_INFO_TYPE_EMAIL, req.Email)Also applies to: 728-734
1-1: Update remaining string-typed ContactInfo call sitesI ran the sweep for string-typed call sites and uncovered the following usages that still need to be updated to the new typed APIs:
services/participant-api/apihandlers/authentication.go:713
ci, found := user.FindContactInfoByTypeAndAddr("email", req.Email)services/participant-api/apihandlers/user-management.go:299
oldCI, oldFound := user.FindContactInfoByTypeAndAddr("email", user.Account.AccountID)services/participant-api/apihandlers/user-management.go:330
ci, found := user.FindContactInfoByTypeAndAddr("email", req.Email)services/participant-api/apihandlers/user-management.go:340
newCI, newFound := user.FindContactInfoByTypeAndAddr("email", req.Email)services/participant-api/apihandlers/authentication.go:728
user.SetContactInfoVerificationSent("email", req.Email)No matches were found for
ConfirmContactInfo("…").Please refactor these call sites to use the strongly-typed ContactInfo API (e.g. passing the
ContactType.Emailconstant or equivalent) instead of raw string literals. This will ensure consistency and catch any mismatches at compile time.
♻️ Duplicate comments (2)
pkg/study/studyengine/sender/sender.go (1)
86-98: Thanks for addressing the prior panic on user.Profiles[0].The length guard before indexing resolves the panic highlighted in a previous review.
pkg/user-management/types/user.go (1)
43-63: GetEmail selection logic now matches previous recommendation — LGTMPrefers the account’s main email, then any confirmed email, then any email; returns an error otherwise. This aligns with the prior review guidance.
🧹 Nitpick comments (16)
pkg/study/studyengine/actions.go (2)
653-674: Constrain and sanitize dynamic payload to prevent accidental PII/size blowups.You’re flattening participant flags, linking codes, and arbitrary event payload into an email-template payload. Two recommendations:
- Skip empty keys and clamp overly long values to avoid accidentally dumping large objects into outgoing emails.
- Keep the current namespacing; it’s good for avoiding collisions.
Apply a minimal guard:
func getExtraPayload(pState studyTypes.Participant, event StudyEvent) map[string]string { payload := map[string]string{ "studyKey": event.StudyKey, } // include participant flags into payload: for k, v := range pState.Flags { - payload["flags."+k] = v + if strings.TrimSpace(k) == "" { + continue + } + vv := v + const maxValueLen = 2048 + if len(vv) > maxValueLen { + vv = vv[:maxValueLen] + } + payload["flags."+k] = vv } // include linking codes into payload for k, v := range pState.LinkingCodes { - payload["linkingCodes."+k] = v + if strings.TrimSpace(k) == "" { + continue + } + vv := v + const maxValueLen = 2048 + if len(vv) > maxValueLen { + vv = vv[:maxValueLen] + } + payload["linkingCodes."+k] = vv } // include event payload into message (template) payload for k, v := range event.Payload { - payload["eventData."+k] = fmt.Sprintf("%v", v) + if strings.TrimSpace(k) == "" { + continue + } + vv := fmt.Sprintf("%v", v) + const maxValueLen = 2048 + if len(vv) > maxValueLen { + vv = vv[:maxValueLen] + } + payload["eventData."+k] = vv } return payload }If some values genuinely need to exceed 2 KB, adjust maxValueLen accordingly.
676-739: Make language override truly optional and validate message type.Today, failure to resolve the optional language argument aborts the action. Also, trim/validate messageType to avoid sending with an empty template key.
func sendMessageNow(action studyTypes.Expression, oldState ActionData, event StudyEvent) (newState ActionData, err error) { newState = oldState @@ arg1, err := EvalContext.ExpressionArgResolver(action.Data[0]) if err != nil { return newState, err } - messageType, ok1 := arg1.(string) - if !ok1 { + messageType, ok1 := arg1.(string) + messageType = strings.TrimSpace(messageType) + if !ok1 || messageType == "" { return newState, errors.New("could not parse arguments") } @@ - if len(action.Data) > 1 { - arg2, err := EvalContext.ExpressionArgResolver(action.Data[1]) - if err != nil { - return newState, err - } - languageOverride, ok1 = arg2.(string) - if !ok1 { - slog.Debug("could not parse language override") - } - } + if len(action.Data) > 1 { + if arg2, err := EvalContext.ExpressionArgResolver(action.Data[1]); err == nil { + if v, ok := arg2.(string); ok { + languageOverride = v + } else { + slog.Debug("SEND_MESSAGE_NOW: language override not a string; skipping") + } + } else { + slog.Debug("SEND_MESSAGE_NOW: failed to resolve language override; skipping", slog.String("error", err.Error())) + } + }Consider adding unit tests for:
- missing participantIDForConfidentialResponses
- empty/whitespace messageType
- language override present/invalid/resolve-error
pkg/study/studyengine/sender/sender.go (7)
114-115: Prevent extraPayload from overriding reserved keys (profileAlias/profileId/loginToken).Protect invariants while still allowing rich template data.
- maps.Copy(payload, extraPayload) + for k, v := range extraPayload { + switch k { + case "profileAlias", "profileId", "loginToken": + continue + } + payload[k] = v + }If some reserved key overrides are desired, we can allowlist them explicitly per messageType.
117-121: Set a safe default language before applying override.Avoids sending with an empty lang when PreferredLanguage is unset.
- lang := user.Account.PreferredLanguage - if opts.LanguageOverride != "" { - lang = opts.LanguageOverride - } + lang := user.Account.PreferredLanguage + if lang == "" { + lang = "en" + } + if opts.LanguageOverride != "" { + lang = opts.LanguageOverride + }
60-63: Initialization check: clarify intent and coverage.You currently check studyDB/participantUserDB/messagingDB but not globalInfosDB (used by token creation). With the guard above, keeping globalInfosDB optional is fine. Consider removing messagingDB from this check since it’s not referenced here (the email-sending package uses its own wiring).
145-160: Nit: rename getTemploginToken → getTempLoginToken.Improves readability and follows camel casing.
- func (s *StudyMessageSender) getTemploginToken(instanceID string, user umTypes.User, studyKey string) (string, error) { + func (s *StudyMessageSender) getTempLoginToken(instanceID string, user umTypes.User, studyKey string) (string, error) { @@ - loginToken, err := s.getTemploginToken(instanceID, user, studyKey) + loginToken, err := s.getTempLoginToken(instanceID, user, studyKey)Also applies to: 107-107
29-32: Config defaulting: treat zero LoginTokenTTL as “disabled” or set a sensible default.Zero TTL means tokens expire immediately. Either document that 0 disables token issuance (and gate on >0 as suggested) or default it in NewStudyMessageSender to a safe value (e.g., 15m).
Would you like me to push a small defaulting change in NewStudyMessageSender?
51-59: Minor: method doc and dependency minimality.Good docstring. Since this method doesn’t directly use messagingDB, consider dropping it from the struct to reduce coupling—or keep it if you expect to use it shortly.
78-85: Email presence handling is fine; consider soft-bounce path.GetEmail error path is logged and returned. If desired, we can add a study-specific “no-email” report or metric to aid monitoring.
pkg/user-management/types/contact-info.go (1)
5-12: Optional: enforce “one-of” invariant and trim payload noiseTo keep documents tidy and invariants clear:
- Only one of Email or Phone should be set per ContactInfo; consider validating this at constructors/mutators.
- Consider adding ,omitempty to Email and Phone to avoid storing empty strings.
Apply this diff:
type ContactInfo struct { ID primitive.ObjectID `bson:"_id,omitempty" json:"id"` Type ContactInfoType `bson:"type" json:"type"` ConfirmedAt int64 `bson:"confirmedAt" json:"confirmedAt"` ConfirmationLinkSentAt int64 `bson:"confirmationLinkSentAt" json:"confirmationLinkSentAt"` - Email string `bson:"email" json:"email"` - Phone string `bson:"phone" json:"phone"` + Email string `bson:"email,omitempty" json:"email,omitempty"` + Phone string `bson:"phone,omitempty" json:"phone,omitempty"` }If helpful, I can wire a small validator that runs where ContactInfo instances are created/updated.
services/participant-api/apihandlers/authentication.go (2)
823-835: Validate token “type” and return 400 for unknown/absent contact; avoid 500sCasting cType to ContactInfoType is fine, but if the token carries an unsupported value, ConfirmContactInfo just returns “contact not found,” which you map to 500. Treat this as client error (400) and short-circuit on unknown types.
Apply this diff:
cType, ok1 := tokenInfos.Info["type"] email, ok2 := tokenInfos.Info["email"] if !ok1 || !ok2 { slog.Error("missing type or email in token infos", slog.String("error", "missing type or email in token infos"), slog.String("instanceID", tokenInfos.InstanceID), slog.String("userID", tokenInfos.UserID)) c.JSON(http.StatusBadRequest, gin.H{"error": "missing type or email in token infos"}) return } - if err := user.ConfirmContactInfo(userTypes.ContactInfoType(cType), email); err != nil { - slog.Error("failed to confirm contact info", slog.String("error", err.Error()), slog.String("instanceID", tokenInfos.InstanceID), slog.String("userID", tokenInfos.UserID)) - c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to confirm contact info"}) - return - } + typed := userTypes.ContactInfoType(cType) + if typed != userTypes.CONTACT_INFO_TYPE_EMAIL && typed != userTypes.CONTACT_INFO_TYPE_PHONE { + slog.Warn("unsupported contact info type from token", slog.String("type", cType)) + c.JSON(http.StatusBadRequest, gin.H{"error": "unsupported contact info type"}) + return + } + if _, ok := user.FindContactInfoByTypeAndAddr(typed, email); !ok { + slog.Warn("contact not found for token", slog.String("type", cType), slog.String("addr", email)) + c.JSON(http.StatusBadRequest, gin.H{"error": "contact not found"}) + return + } + if err := user.ConfirmContactInfo(typed, email); err != nil { + slog.Error("failed to confirm contact info", slog.String("error", err.Error()), slog.String("instanceID", tokenInfos.InstanceID), slog.String("userID", tokenInfos.UserID)) + c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to confirm contact info"}) + return + }This preserves the 500 path for real persistence failures but maps bad token content to 400.
494-497: Nit: avoid string literal drift for “email” keyFor lastOTP, prefer reusing the canonical value to prevent typos and make future renames safer.
Apply this diff:
- lastOTP := map[string]int64{ - "email": time.Now().Unix(), - } + lastOTP := map[string]int64{ + string(userTypes.CONTACT_INFO_TYPE_EMAIL): time.Now().Unix(), + }pkg/user-management/types/user.go (4)
12-17: Typed ContactInfoType + constants look good; consider centralized validationThe enum-like constants eliminate magic strings. Add a tiny helper for validation to keep checks consistent.
Apply this diff:
type ContactInfoType string const ( CONTACT_INFO_TYPE_EMAIL ContactInfoType = "email" CONTACT_INFO_TYPE_PHONE ContactInfoType = "phone" ) + +func (t ContactInfoType) IsValid() bool { + return t == CONTACT_INFO_TYPE_EMAIL || t == CONTACT_INFO_TYPE_PHONE +}
33-41: Prevent duplicate email entries on AddNewEmailIf called multiple times with the same addr, this appends duplicates. Guard against that to keep ContactInfos clean.
Apply this diff:
func (u *User) AddNewEmail(addr string, confirmed bool) { + // De-duplicate on address + for _, ci := range u.ContactInfos { + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + if confirmed && ci.ConfirmedAt == 0 { + // upgrade existing to confirmed + // Note: use ReplaceUser to persist after caller mutates u + // Caller should persist u after this call. + } + return + } + } contactInfo := ContactInfo{ ID: primitive.NewObjectID(), Type: CONTACT_INFO_TYPE_EMAIL, ConfirmedAt: 0, Email: addr, }If you prefer, I can wire the “upgrade to confirmed” inline by switching to index-based update.
101-135: Minor: reduce duplication across typed contact-info lookupsConfirmContactInfo, SetContactInfoVerificationSent, and FindContactInfoByTypeAndAddr repeat the same t/type/email-or-phone checks. A small switch-based matcher would simplify and de-risk future additions (e.g., more ContactInfoType values).
Example refactor for one function:
func (u *User) ConfirmContactInfo(t ContactInfoType, addr string) error { - for i, ci := range u.ContactInfos { - if t == CONTACT_INFO_TYPE_EMAIL && ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } else if t == CONTACT_INFO_TYPE_PHONE && ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { - u.ContactInfos[i].ConfirmedAt = time.Now().Unix() - return nil - } - } + for i, ci := range u.ContactInfos { + switch t { + case CONTACT_INFO_TYPE_EMAIL: + if ci.Type == CONTACT_INFO_TYPE_EMAIL && ci.Email == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } + case CONTACT_INFO_TYPE_PHONE: + if ci.Type == CONTACT_INFO_TYPE_PHONE && ci.Phone == addr { + u.ContactInfos[i].ConfirmedAt = time.Now().Unix() + return nil + } + } + } return errors.New("contact not found") }If you like this pattern, I can apply it consistently to SetContactInfoVerificationSent and FindContactInfoByTypeAndAddr.
126-135: Pointer vs value receiver consistencyFindContactInfoByTypeAndAddr now uses a value receiver while most other methods on User use pointers. It works (slices aren’t deep-copied), but keeping receiver style consistent improves readability.
Apply this diff:
-func (u User) FindContactInfoByTypeAndAddr(t ContactInfoType, addr string) (ContactInfo, bool) { +func (u *User) FindContactInfoByTypeAndAddr(t ContactInfoType, addr string) (ContactInfo, bool) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pkg/study/studyengine/actions.go(2 hunks)pkg/study/studyengine/expressions.go(2 hunks)pkg/study/studyengine/sender/sender.go(1 hunks)pkg/user-management/types/contact-info.go(1 hunks)pkg/user-management/types/user.go(6 hunks)services/management-api/init.go(1 hunks)services/participant-api/apihandlers/authentication.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- services/management-api/init.go
- pkg/study/studyengine/expressions.go
🧰 Additional context used
🧬 Code graph analysis (5)
pkg/user-management/types/contact-info.go (1)
pkg/user-management/types/user.go (1)
ContactInfoType(12-12)
services/participant-api/apihandlers/authentication.go (1)
pkg/user-management/types/user.go (1)
ContactInfoType(12-12)
pkg/study/studyengine/actions.go (3)
pkg/study/types/participant.go (1)
Participant(14-26)pkg/study/studyengine/types.go (5)
StudyEvent(76-85)ActionData(57-60)CurrentStudyEngine(28-28)EvalContext(88-91)SendOptions(94-97)pkg/study/studyengine/helpers.go (1)
Now(13-13)
pkg/user-management/types/user.go (2)
pkg/user-management/types/contact-info.go (1)
ContactInfo(5-12)pkg/user-management/types/account.go (1)
Account(3-15)
pkg/study/studyengine/sender/sender.go (6)
pkg/study/studyengine/types.go (1)
SendOptions(94-97)pkg/user-management/types/account.go (1)
Account(3-15)pkg/messaging/email-sending/send.go (1)
SendInstantEmailByTemplate(61-110)pkg/user-management/types/user.go (1)
User(19-27)pkg/user-management/types/temptoken.go (2)
TempToken(19-27)TOKEN_PURPOSE_SURVEY_LOGIN(13-13)pkg/user-management/utils/temp-tokens.go (1)
GetExpirationTime(33-35)
🔇 Additional comments (5)
pkg/study/studyengine/actions.go (1)
63-64: Wiring looks correct for SEND_MESSAGE_NOW.Dispatching to sendMessageNow is consistent with the existing action switch.
pkg/study/studyengine/sender/sender.go (1)
3-17: No action needed: Go version supports maps.CopyThe project’s go.mod specifies Go 1.23.0, which is above the 1.21 minimum required for maps.Copy, so no compatibility changes are necessary.
pkg/user-management/types/contact-info.go (1)
7-7: Good move to strong typing for ContactInfo.TypeSwitching from string to ContactInfoType improves type-safety and prevents accidental misuse while keeping BSON/JSON wire format unchanged (underlying type is string). No data migration should be required.
pkg/user-management/types/user.go (2)
150-154: Good: prevent removal of the main login emailBlocks deleting the primary address when the account type is email; also updates contact preferences only upon successful removal. Solid guardrail.
146-170: RemoveContactInfoFromContactPreferences covers all current ID-based fieldsVerified that
ContactPreferencesonly contains one slice of contact IDs—SendNewsletterTo—and two boolean flags (SubscribedToNewsletter,SubscribedToWeekly). The existingRemoveContactInfoFromContactPreferencesmethod already removes any matching ID fromSendNewsletterTo, so no additional cleanup is needed at this time.• pkg/user-management/types/contact-preferences.go:
ContactPreferencesdefines only
–SendNewsletterTo []string(cleaned byRemoveContactInfoFromContactPreferences)
–SubscribedToNewsletter,SubscribedToWeekly(booleans, no ID references)
• pkg/user-management/types/user.go:RemoveContactInfoFromContactPreferenceshandlesSendNewsletterToNo changes required—this comment is resolved. Please remember to update this method when new slice-based preferences (e.g., SMS or push ID lists) are added.
…fo type in authentication and user management handlers
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
services/participant-api/apihandlers/user-management.go (2)
333-344: Avoid a redundant lookup and reuse the result variableYou can reuse the first lookup result and only re-query when you actually add a new email. This removes one extra
FindContactInfoByTypeAndAddrcall on the “found” path and simplifies variable usage later.Apply:
- ci, found := user.FindContactInfoByTypeAndAddr( - userTypes.CONTACT_INFO_TYPE_EMAIL, - req.Email, - ) -if found { + newCI, newFound := user.FindContactInfoByTypeAndAddr( + userTypes.CONTACT_INFO_TYPE_EMAIL, + req.Email, + ) +if newFound { // new email already confirmed - if ci.ConfirmedAt > 0 { - user.Account.AccountConfirmedAt = ci.ConfirmedAt + if newCI.ConfirmedAt > 0 { + user.Account.AccountConfirmedAt = newCI.ConfirmedAt } } else { user.AddNewEmail(req.Email, false) + // reload to get the ID of the newly added contact + newCI, newFound = user.FindContactInfoByTypeAndAddr( + userTypes.CONTACT_INFO_TYPE_EMAIL, + req.Email, + ) }Note: This keeps
newCI/newFoundin scope for subsequent use (e.g., inReplaceContactInfoInContactPreferences), and removes an unnecessary extra lookup on the “found” branch.
346-349: Remove now-redundant second lookup (handled in the earlier block)If you adopt the previous refactor, this second
FindContactInfoByTypeAndAddrblock becomes unnecessary; you’ll already havenewCI/newFoundpopulated.Apply:
- newCI, newFound := user.FindContactInfoByTypeAndAddr( - userTypes.CONTACT_INFO_TYPE_EMAIL, - req.Email, - )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
services/participant-api/apihandlers/authentication.go(3 hunks)services/participant-api/apihandlers/user-management.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/participant-api/apihandlers/authentication.go
🧰 Additional context used
🧬 Code graph analysis (1)
services/participant-api/apihandlers/user-management.go (2)
pkg/user-management/types/user.go (1)
CONTACT_INFO_TYPE_EMAIL(15-15)pkg/user-management/types/account.go (1)
Account(3-15)
🔇 Additional comments (1)
services/participant-api/apihandlers/user-management.go (1)
299-302: Migration verified: no raw "email" usages remainI ran the ripgrep searches to look for any calls to
FindContactInfoByTypeAndAddrpassing a string literal"email". All instances now use theCONTACT_INFO_TYPE_EMAILconstant:
- services/participant-api/apihandlers/user-management.go (lines 299–302, 333–335, 346–348)
- services/participant-api/apihandlers/authentication.go (lines 713–715)
- pkg/user-management/types/user.go (line 46)
There are no remaining literal
"email"arguments. The migration to typedContactInfoTypeis complete. LGTM.
…tionally generate login token in SendInstantStudyEmail function
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/user-management/types/contact-info.go (1)
5-12: Enforce invariants: ensure address matches type (email vs. phone), and only one is set.With a typed
Type, we should validate that:
- Type=email => Email non-empty, Phone empty
- Type=phone => Phone non-empty, Email empty
This prevents mixed/invalid documents at the edges (handlers, repo layer).
Add a lightweight validator (outside this file) and call it in create/update paths:
func (c ContactInfo) Validate() error { switch c.Type { case CONTACT_INFO_TYPE_EMAIL: if c.Email == "" { return fmt.Errorf("email required for type=email") } if c.Phone != "" { return fmt.Errorf("phone must be empty for type=email") } case CONTACT_INFO_TYPE_PHONE: if c.Phone == "" { return fmt.Errorf("phone required for type=phone") } if c.Email != "" { return fmt.Errorf("email must be empty for type=phone") } default: return fmt.Errorf("unsupported contact info type: %q", c.Type) } return nil }I can wire this into the repository/service layer and add unit tests covering JSON/BSON round-trips and validation. Want me to push a patch?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pkg/study/studyengine/actions.go(2 hunks)pkg/study/studyengine/sender/sender.go(1 hunks)pkg/user-management/types/contact-info.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/study/studyengine/sender/sender.go
- pkg/study/studyengine/actions.go
🔇 Additional comments (2)
pkg/user-management/types/contact-info.go (2)
7-7: ContactInfoType is already a string alias; backward-compatibility preservedVerified that in pkg/user-management/types/user.go:
- Line 12:
type ContactInfoType string- Lines 15–16:
CONTACT_INFO_TYPE_EMAIL ContactInfoType = "email"andCONTACT_INFO_TYPE_PHONE ContactInfoType = "phone"- No custom JSON/BSON marshalers defined for ContactInfoType (default string alias marshaling emits strings)
No further changes required.
10-11: Ensure JSON shape for ContactInfo email/phone remains stableWe’re exposing
ContactInfo(with bothPhonefields) in our user‐management APIs. Making the JSON tagsomitemptywill drop the empty field entirely, which can break downstream clients that expect those keys to always be present (even if empty). To keep MongoDB records clean while preserving the API contract, we should only omit empty values in BSON but always emit both fields in JSON.Key locations to check and update:
- pkg/user-management/types/contact-info.go (lines 10–11) – the
Phonetags- pkg/user-management/types/user.go –
ContactInfos []ContactInfois served in all user payloads- services/management-api/apihandlers/user-management.go – serializes
User(and thusContactInfo) to JSON- services/participant-api/apihandlers/user-management.go – same serialization in the participant API
Apply this diff:
--- a/pkg/user-management/types/contact-info.go +++ b/pkg/user-management/types/contact-info.go @@ -7,7 +7,7 @@ type ContactInfo struct { ConfirmationLinkSentAt int64 `bson:"confirmationLinkSentAt" json:"confirmationLinkSentAt"` - Email string `bson:"email,omitempty" json:"email,omitempty"` - Phone string `bson:"phone,omitempty" json:"phone,omitempty"` + Email string `bson:"email,omitempty" json:"email"` + Phone string `bson:"phone,omitempty" json:"phone"` }
There was a problem hiding this comment.
Pull Request Overview
This PR implements immediate study-triggered email functionality by adding a new SEND_MESSAGE_NOW action to the study engine and creating a pluggable message sender architecture.
- Adds
StudyMessageSenderinterface for immediate email sending from study rules - Creates
StudyMessageSenderimplementation with template-based email support and optional login tokens - Refactors contact info handling to use typed constants instead of string literals
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/participant-api/init.go | Initializes study message sender and reorders initialization sequence |
| services/participant-api/apihandlers/*.go | Updates contact info methods to use typed constants |
| services/management-api/init.go | Passes nil message sender (not used in this service) |
| pkg/user-management/types/*.go | Introduces typed ContactInfoType and new GetEmail method |
| pkg/study/studyengine/*.go | Adds message sender interface and SEND_MESSAGE_NOW action |
| pkg/study/studyengine/sender/sender.go | Implements message sender with email template support |
| pkg/study/study-service.go | Registers message sender with study engine |
| pkg/messaging/email-sending/utils.go | Fixes parameter order and adds structured logging |
| jobs/*/init.go | Updates study service initialization calls |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Improvements
Reliability
Bug Fixes