Skip to content

Commit 08d6662

Browse files
raju-optiraju-opti
committed
fix review comments
1 parent a1cc608 commit 08d6662

File tree

2 files changed

+73
-77
lines changed

2 files changed

+73
-77
lines changed

‎core-api/src/main/java/com/optimizely/ab/Optimizely.java‎

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,21 +1193,6 @@ private OptimizelyUserContext createUserContextCopy(@Nonnull String userId, @Non
11931193
return new OptimizelyUserContext(this, userId, attributes, Collections.EMPTY_MAP, null, false);
11941194
}
11951195

1196-
Optional<FeatureDecision> getForcedDecision(@Nonnull String flagKey,
1197-
@Nonnull DecisionReasons decisionReasons,
1198-
@Nonnull ProjectConfig projectConfig,
1199-
@Nonnull OptimizelyUserContext user) {
1200-
1201-
OptimizelyDecisionContext optimizelyDecisionContext = new OptimizelyDecisionContext(flagKey, null);
1202-
DecisionResponse<Variation> forcedDecisionVariation = decisionService.validatedForcedDecision(optimizelyDecisionContext, projectConfig, user);
1203-
decisionReasons.merge(forcedDecisionVariation.getReasons());
1204-
if (forcedDecisionVariation.getResult() != null) {
1205-
return Optional.of(new FeatureDecision(null, forcedDecisionVariation.getResult(), FeatureDecision.DecisionSource.FEATURE_TEST));
1206-
}
1207-
1208-
return Optional.empty();
1209-
}
1210-
12111196
OptimizelyDecision decide(@Nonnull OptimizelyUserContext user,
12121197
@Nonnull String key,
12131198
@Nonnull List<OptimizelyDecideOption> options) {
@@ -1343,11 +1328,15 @@ private Map<String, OptimizelyDecision> decideForKeys(@Nonnull OptimizelyUserCon
13431328
validKeys.add(key);
13441329

13451330
DecisionReasons decisionReasons = DefaultDecisionReasons.newInstance(allOptions);
1346-
Optional<FeatureDecision> forcedDecision = getForcedDecision(key, decisionReasons, projectConfig, user);
13471331
decisionReasonsMap.put(key, decisionReasons);
13481332

1349-
if (forcedDecision.isPresent()) {
1350-
flagDecisions.put(key, forcedDecision.get());
1333+
OptimizelyDecisionContext optimizelyDecisionContext = new OptimizelyDecisionContext(key, null);
1334+
DecisionResponse<Variation> forcedDecisionVariation = decisionService.validatedForcedDecision(optimizelyDecisionContext, projectConfig, user);
1335+
decisionReasons.merge(forcedDecisionVariation.getReasons());
1336+
1337+
if (forcedDecisionVariation.getResult() != null) {
1338+
flagDecisions.put(key,
1339+
new FeatureDecision(null, forcedDecisionVariation.getResult(), FeatureDecision.DecisionSource.FEATURE_TEST));
13511340
} else {
13521341
flagsWithoutForcedDecision.add(flag);
13531342
}

‎core-api/src/main/java/com/optimizely/ab/bucketing/DecisionService.java‎

Lines changed: 66 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,17 @@ public DecisionService(@Nonnull Bucketer bucketer,
7878
this.userProfileService = userProfileService;
7979
}
8080

81+
/**
82+
* Get a {@link Variation} of an {@link Experiment} for a user to be allocated into.
83+
*
84+
* @param experiment The Experiment the user will be bucketed into.
85+
* @param user The current OptimizelyUserContext
86+
* @param projectConfig The current projectConfig
87+
* @param options An array of decision options
88+
* @param userProfileTracker tracker for reading and updating user profile of the user
89+
* @param reasons Decision reasons
90+
* @return A {@link DecisionResponse} including the {@link Variation} that user is bucketed into (or null) and the decision reasons
91+
*/
8192
@Nonnull
8293
public DecisionResponse<Variation> getVariation(@Nonnull Experiment experiment,
8394
@Nonnull OptimizelyUserContext user,
@@ -164,19 +175,17 @@ public DecisionResponse<Variation> getVariation(@Nonnull Experiment experiment,
164175

165176
// fetch the user profile map from the user profile service
166177
boolean ignoreUPS = options.contains(OptimizelyDecideOption.IGNORE_USER_PROFILE_SERVICE);
167-
// UserProfile userProfile = null;
168178
UserProfileTracker userProfileTracker = null;
169179

170180
if (userProfileService != null && !ignoreUPS) {
171-
UserProfile userProfile = getUserProfile(user.getUserId(), reasons);
172-
userProfileTracker = new UserProfileTracker(userProfile, false);
181+
userProfileTracker = new UserProfileTracker(user.getUserId());
182+
userProfileTracker.loadUserProfile(reasons);
173183
}
174184

175-
176185
DecisionResponse<Variation> response = getVariation(experiment, user, projectConfig, options, userProfileTracker, reasons);
177186

178-
if(userProfileService != null && !ignoreUPS && userProfileTracker.profileUpdated) {
179-
saveUserProfile(userProfileTracker.userProfile);
187+
if(userProfileService != null && !ignoreUPS) {
188+
userProfileTracker.saveUserProfile();
180189
}
181190
return response;
182191
}
@@ -205,45 +214,42 @@ public DecisionResponse<FeatureDecision> getVariationForFeature(@Nonnull Feature
205214
return getVariationsForFeatureList(Arrays.asList(featureFlag), user, projectConfig, options).get(0);
206215
}
207216

208-
private UserProfile getUserProfile(String userId, DecisionReasons reasons) {
209-
UserProfile userProfile = null;
217+
class UserProfileTracker {
218+
private UserProfile userProfile;
219+
private boolean profileUpdated;
220+
private String userId;
210221

211-
try {
212-
Map<String, Object> userProfileMap = userProfileService.lookup(userId);
213-
if (userProfileMap == null) {
214-
String message = reasons.addInfo("We were unable to get a user profile map from the UserProfileService.");
215-
logger.info(message);
216-
} else if (UserProfileUtils.isValidUserProfileMap(userProfileMap)) {
217-
userProfile = UserProfileUtils.convertMapToUserProfile(userProfileMap);
218-
} else {
219-
String message = reasons.addInfo("The UserProfileService returned an invalid map.");
220-
logger.warn(message);
221-
}
222-
} catch (Exception exception) {
223-
String message = reasons.addInfo(exception.getMessage());
224-
logger.error(message);
225-
errorHandler.handleError(new OptimizelyRuntimeException(exception));
222+
UserProfileTracker(String userId) {
223+
this.userId = userId;
224+
this.profileUpdated = false;
225+
this.userProfile = null;
226226
}
227227

228-
if (userProfile == null) {
229-
userProfile = new UserProfile(userId, new HashMap<String, Decision>());
230-
}
231-
232-
return userProfile;
233-
}
234-
235-
static class UserProfileTracker {
236-
public UserProfile userProfile;
237-
public boolean profileUpdated;
228+
public void loadUserProfile(DecisionReasons reasons) {
229+
try {
230+
Map<String, Object> userProfileMap = userProfileService.lookup(userId);
231+
if (userProfileMap == null) {
232+
String message = reasons.addInfo("We were unable to get a user profile map from the UserProfileService.");
233+
logger.info(message);
234+
} else if (UserProfileUtils.isValidUserProfileMap(userProfileMap)) {
235+
userProfile = UserProfileUtils.convertMapToUserProfile(userProfileMap);
236+
} else {
237+
String message = reasons.addInfo("The UserProfileService returned an invalid map.");
238+
logger.warn(message);
239+
}
240+
} catch (Exception exception) {
241+
String message = reasons.addInfo(exception.getMessage());
242+
logger.error(message);
243+
errorHandler.handleError(new OptimizelyRuntimeException(exception));
244+
}
238245

239-
UserProfileTracker(UserProfile userProfile, boolean profileUpdated) {
240-
this.userProfile = userProfile;
241-
this.profileUpdated = profileUpdated;
246+
if (userProfile == null) {
247+
userProfile = new UserProfile(userId, new HashMap<String, Decision>());
248+
}
242249
}
243250

244-
void updateUserProfile(@Nonnull Experiment experiment,
251+
public void updateUserProfile(@Nonnull Experiment experiment,
245252
@Nonnull Variation variation) {
246-
247253
String experimentId = experiment.getId();
248254
String variationId = variation.getId();
249255
Decision decision;
@@ -258,10 +264,27 @@ void updateUserProfile(@Nonnull Experiment experiment,
258264
logger.info("Updated variation \"{}\" of experiment \"{}\" for user \"{}\".",
259265
variationId, experimentId, userProfile.userId);
260266
}
267+
268+
public void saveUserProfile() {
269+
// if there were no updates, no need to save
270+
if (!this.profileUpdated) {
271+
return;
272+
}
273+
274+
try {
275+
userProfileService.save(userProfile.toMap());
276+
logger.info("Saved user profile of user \"{}\".",
277+
userProfile.userId);
278+
} catch (Exception exception) {
279+
logger.warn("Failed to save user profile of user \"{}\".",
280+
userProfile.userId);
281+
errorHandler.handleError(new OptimizelyRuntimeException(exception));
282+
}
283+
}
261284
}
262285

263286
/**
264-
* Get the variations the user is bucketed into for the the list of feature flags
287+
* Get the variations the user is bucketed into for the list of feature flags
265288
*
266289
* @param featureFlags The feature flag list the user wants to access.
267290
* @param user The current OptimizelyuserContext
@@ -280,8 +303,8 @@ public List<DecisionResponse<FeatureDecision>> getVariationsForFeatureList(@Non
280303
UserProfileTracker userProfileTracker = null;
281304

282305
if (userProfileService != null && !ignoreUPS) {
283-
UserProfile userProfile = getUserProfile(user.getUserId(), upsReasons);
284-
userProfileTracker = new UserProfileTracker(userProfile, false);
306+
userProfileTracker = new UserProfileTracker(user.getUserId());
307+
userProfileTracker.loadUserProfile(upsReasons);
285308
}
286309

287310
List<DecisionResponse<FeatureDecision>> decisions = new ArrayList<>();
@@ -316,8 +339,8 @@ public List<DecisionResponse<FeatureDecision>> getVariationsForFeatureList(@Non
316339
decisions.add(new DecisionResponse(decision, reasons));
317340
}
318341

319-
if (userProfileService != null && !ignoreUPS && userProfileTracker.profileUpdated) {
320-
saveUserProfile(userProfileTracker.userProfile);
342+
if (userProfileService != null && !ignoreUPS) {
343+
userProfileTracker.saveUserProfile();
321344
}
322345

323346
return decisions;
@@ -548,22 +571,6 @@ void saveVariation(@Nonnull Experiment experiment,
548571
}
549572
}
550573

551-
void saveUserProfile(@Nonnull UserProfile userProfile) {
552-
if (userProfileService == null) {
553-
return;
554-
}
555-
556-
try {
557-
userProfileService.save(userProfile.toMap());
558-
logger.info("Saved user profile of user \"{}\".",
559-
userProfile.userId);
560-
} catch (Exception exception) {
561-
logger.warn("Failed to save user profile of user \"{}\".",
562-
userProfile.userId);
563-
errorHandler.handleError(new OptimizelyRuntimeException(exception));
564-
}
565-
}
566-
567574
/**
568575
* Get the bucketingId of a user if a bucketingId exists in attributes, or else default to userId.
569576
*

0 commit comments

Comments
 (0)