-
Notifications
You must be signed in to change notification settings - Fork 19
[FSSDK-11178] Update impression event for CMAB #404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…t factory for CMAB impression events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR moves CMAB decision logic to a dedicated pkg/cmab
package, updates the event factory to support CMAB impression events, and adds corresponding tests.
- Moved and renamed CMAB types and interfaces from
pkg/decision
topkg/cmab
. - Introduced
CreateCMABImpressionEvent
/CreateCMABImpressionUserEvent
and updated the event factory and tests. - Extended
DefaultCmabService
/DefaultCmabClient
to track CMAB decision events.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
pkg/event/factory.go | Added CreateCMABImpressionEvent and CreateCMABImpressionUserEvent . |
pkg/event/factory_test.go | Added tests for CMAB impression; updated copyright. |
pkg/event/events.go | Defined new CMABImpressionEvent type. |
pkg/cmab/types.go | Renamed and reorganized CMAB decision types and interfaces. |
pkg/cmab/service.go | Refactored service to use new types and track decisions. |
pkg/cmab/service_test.go | Updated tests for renamed types and added tracking validation. |
pkg/cmab/client.go | Refactored client types, added event processing and tracking stub. |
Comments suppressed due to low confidence (1)
pkg/cmab/client.go:280
- [nitpick] There are no unit tests covering the LogImpression/TrackCMABDecision flow in the client; consider adding tests to validate event creation and dispatch.
type EventProcessor interface {
fmt.Printf("CMAB UUID: %s\n", cmabUUID) | ||
fmt.Printf("UserEvent UUID: %s\n", userEvent.UUID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Printing directly with fmt.Printf in tests can clutter output; use t.Logf or remove debug prints.
fmt.Printf("CMAB UUID: %s\n", cmabUUID) | |
fmt.Printf("UserEvent UUID: %s\n", userEvent.UUID) | |
t.Logf("CMAB UUID: %s", cmabUUID) | |
t.Logf("UserEvent UUID: %s", userEvent.UUID) |
Copilot uses AI. Check for mistakes.
|
||
// Process the event if it should be dispatched | ||
if shouldDispatch { | ||
c.eventProcessor.ProcessEvent(userEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The EventProcessor interface defines Process(...), not ProcessEvent; update the call or interface to match.
c.eventProcessor.ProcessEvent(userEvent) | |
c.eventProcessor.Process(userEvent) |
Copilot uses AI. Check for mistakes.
// Instead of directly creating an event, we'll delegate this to the client | ||
// that has access to the event package | ||
return fmt.Errorf("CMAB impression logging not implemented") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LogImpression is currently unimplemented and always errors; implement the logic or remove this stub to avoid runtime failures.
// Instead of directly creating an event, we'll delegate this to the client | |
// that has access to the event package | |
return fmt.Errorf("CMAB impression logging not implemented") | |
// Get experiment from project config | |
experiment, err := projectConfig.GetExperimentByID(ruleID) | |
if err != nil { | |
return fmt.Errorf("error getting experiment: %w", err) | |
} | |
// Create variation object | |
variation := entities.Variation{ | |
ID: variationID, | |
Key: variationKey, | |
} | |
// Create user context | |
userContext := entities.UserContext{ | |
ID: userID, | |
Attributes: attributes, | |
} | |
// Look for associated feature flag (if any) | |
flagKey := "" | |
featureList := projectConfig.GetFeatureList() | |
for _, feature := range featureList { | |
for _, featureExperiment := range feature.FeatureExperiments { | |
if featureExperiment.ID == ruleID { | |
flagKey = feature.Key | |
break | |
} | |
} | |
if flagKey != "" { | |
break | |
} | |
} | |
// Create user event with CMAB impression | |
userEvent, shouldDispatch := event.CreateCMABImpressionUserEvent( | |
projectConfig, | |
experiment, | |
&variation, | |
userContext, | |
flagKey, | |
experiment.Key, // ruleKey | |
"experiment", // ruleType | |
true, | |
cmabUUID, | |
) | |
// Process the event if it should be dispatched | |
if shouldDispatch { | |
if !eventProcessor.Process(userEvent) { | |
return fmt.Errorf("failed to process CMAB impression event") | |
} | |
} | |
return nil |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to also update the sent impression event when a cmab decison is made.
this is PR has been replaced by a cleaner one #408 |
Jira ticket:
https://jira.sso.episerver.net/browse/FSSDK-11178