Skip to content

Commit de378fb

Browse files
jaredlockhartclaude
jaredlockhart
and
claude
committed
fix(nimbus): capture branch feature value changes in history changelog
Because * NimbusBranchesForm.save() called super().save() which triggered NimbusChangeLogFormMixin.save() to generate the changelog snapshot before self.branches.save() ran, causing the changelog to capture stale feature values instead of the updated ones * Feature value changes were invisible on the History page This commit * Moves self.branches.save() before super().save() in NimbusBranchesForm so that branch feature values are persisted before the changelog snapshot is taken * Adds a test that saves an experiment with initial feature values, updates the values, and asserts the changelog captures the change Fixes #14998 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 88efe41 commit de378fb

File tree

2 files changed

+167
-1
lines changed

2 files changed

+167
-1
lines changed

‎experimenter/experimenter/nimbus_ui/forms.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -829,8 +829,8 @@ def clean(self):
829829

830830
@transaction.atomic
831831
def save(self, *args, **kwargs):
832-
experiment = super().save(*args, **kwargs)
833832
self.branches.save()
833+
experiment = super().save(*args, **kwargs)
834834

835835
if experiment.is_rollout:
836836
branches = experiment.branches.all()

‎experimenter/experimenter/nimbus_ui/tests/test_forms.py‎

Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4201,6 +4201,172 @@ def test_branches_ordered_by_id_with_reference_branch_first(self):
42014201
expected_branch_ids, [branch_b.slug, branch_a.slug, branch_c.slug]
42024202
)
42034203

4204+
def test_branches_form_feature_value_changes_captured_in_changelog(self):
4205+
application = NimbusExperiment.Application.DESKTOP
4206+
feature_config = NimbusFeatureConfigFactory.create(application=application)
4207+
experiment = NimbusExperimentFactory.create_with_lifecycle(
4208+
NimbusExperimentFactory.Lifecycles.CREATED,
4209+
application=application,
4210+
feature_configs=[feature_config],
4211+
equal_branch_ratio=False,
4212+
is_localized=False,
4213+
is_rollout=False,
4214+
localizations=None,
4215+
prevent_pref_conflicts=False,
4216+
warn_feature_schema=False,
4217+
)
4218+
experiment.branches.all().delete()
4219+
experiment.changes.all().delete()
4220+
4221+
reference_branch = NimbusBranchFactory.create(experiment=experiment, ratio=1)
4222+
treatment_branch = NimbusBranchFactory.create(experiment=experiment, ratio=1)
4223+
experiment.reference_branch = reference_branch
4224+
experiment.save()
4225+
4226+
ref_fv = reference_branch.feature_values.get(feature_config=feature_config)
4227+
treat_fv = treatment_branch.feature_values.get(feature_config=feature_config)
4228+
4229+
ref_screenshot = reference_branch.screenshots.first()
4230+
treat_screenshot = treatment_branch.screenshots.first()
4231+
4232+
initial_ref_value = json.dumps({"key": "initial-ref-value"})
4233+
initial_treat_value = json.dumps({"key": "initial-treat-value"})
4234+
4235+
form = NimbusBranchesForm(
4236+
instance=experiment,
4237+
data={
4238+
"feature_configs": [feature_config.id],
4239+
"equal_branch_ratio": False,
4240+
"is_rollout": False,
4241+
"prevent_pref_conflicts": False,
4242+
"warn_feature_schema": False,
4243+
"branches-TOTAL_FORMS": "2",
4244+
"branches-INITIAL_FORMS": "2",
4245+
"branches-MIN_NUM_FORMS": "0",
4246+
"branches-MAX_NUM_FORMS": "1000",
4247+
"branches-0-id": reference_branch.id,
4248+
"branches-0-name": reference_branch.name,
4249+
"branches-0-description": reference_branch.description,
4250+
"branches-0-ratio": 1,
4251+
"branches-0-feature-value-TOTAL_FORMS": "1",
4252+
"branches-0-feature-value-INITIAL_FORMS": "1",
4253+
"branches-0-feature-value-MIN_NUM_FORMS": "0",
4254+
"branches-0-feature-value-MAX_NUM_FORMS": "1000",
4255+
"branches-0-feature-value-0-id": ref_fv.id,
4256+
"branches-0-feature-value-0-value": initial_ref_value,
4257+
"branches-0-screenshots-TOTAL_FORMS": "1",
4258+
"branches-0-screenshots-INITIAL_FORMS": "1",
4259+
"branches-0-screenshots-MIN_NUM_FORMS": "0",
4260+
"branches-0-screenshots-MAX_NUM_FORMS": "1000",
4261+
"branches-0-screenshots-0-id": ref_screenshot.id,
4262+
"branches-0-screenshots-0-description": ref_screenshot.description,
4263+
"branches-1-id": treatment_branch.id,
4264+
"branches-1-name": treatment_branch.name,
4265+
"branches-1-description": treatment_branch.description,
4266+
"branches-1-ratio": 1,
4267+
"branches-1-feature-value-TOTAL_FORMS": "1",
4268+
"branches-1-feature-value-INITIAL_FORMS": "1",
4269+
"branches-1-feature-value-MIN_NUM_FORMS": "0",
4270+
"branches-1-feature-value-MAX_NUM_FORMS": "1000",
4271+
"branches-1-feature-value-0-id": treat_fv.id,
4272+
"branches-1-feature-value-0-value": initial_treat_value,
4273+
"branches-1-screenshots-TOTAL_FORMS": "1",
4274+
"branches-1-screenshots-INITIAL_FORMS": "1",
4275+
"branches-1-screenshots-MIN_NUM_FORMS": "0",
4276+
"branches-1-screenshots-MAX_NUM_FORMS": "1000",
4277+
"branches-1-screenshots-0-id": treat_screenshot.id,
4278+
"branches-1-screenshots-0-description": treat_screenshot.description,
4279+
},
4280+
request=self.request,
4281+
)
4282+
self.assertTrue(form.is_valid(), form.errors)
4283+
form.save()
4284+
4285+
ref_fv = reference_branch.feature_values.get(feature_config=feature_config)
4286+
treat_fv = treatment_branch.feature_values.get(feature_config=feature_config)
4287+
4288+
updated_ref_value = json.dumps({"key": "updated-ref-value"})
4289+
updated_treat_value = json.dumps({"key": "updated-treat-value"})
4290+
4291+
form = NimbusBranchesForm(
4292+
instance=experiment,
4293+
data={
4294+
"feature_configs": [feature_config.id],
4295+
"equal_branch_ratio": False,
4296+
"is_rollout": False,
4297+
"prevent_pref_conflicts": False,
4298+
"warn_feature_schema": False,
4299+
"branches-TOTAL_FORMS": "2",
4300+
"branches-INITIAL_FORMS": "2",
4301+
"branches-MIN_NUM_FORMS": "0",
4302+
"branches-MAX_NUM_FORMS": "1000",
4303+
"branches-0-id": reference_branch.id,
4304+
"branches-0-name": reference_branch.name,
4305+
"branches-0-description": reference_branch.description,
4306+
"branches-0-ratio": 1,
4307+
"branches-0-feature-value-TOTAL_FORMS": "1",
4308+
"branches-0-feature-value-INITIAL_FORMS": "1",
4309+
"branches-0-feature-value-MIN_NUM_FORMS": "0",
4310+
"branches-0-feature-value-MAX_NUM_FORMS": "1000",
4311+
"branches-0-feature-value-0-id": ref_fv.id,
4312+
"branches-0-feature-value-0-value": updated_ref_value,
4313+
"branches-0-screenshots-TOTAL_FORMS": "1",
4314+
"branches-0-screenshots-INITIAL_FORMS": "1",
4315+
"branches-0-screenshots-MIN_NUM_FORMS": "0",
4316+
"branches-0-screenshots-MAX_NUM_FORMS": "1000",
4317+
"branches-0-screenshots-0-id": ref_screenshot.id,
4318+
"branches-0-screenshots-0-description": ref_screenshot.description,
4319+
"branches-1-id": treatment_branch.id,
4320+
"branches-1-name": treatment_branch.name,
4321+
"branches-1-description": treatment_branch.description,
4322+
"branches-1-ratio": 1,
4323+
"branches-1-feature-value-TOTAL_FORMS": "1",
4324+
"branches-1-feature-value-INITIAL_FORMS": "1",
4325+
"branches-1-feature-value-MIN_NUM_FORMS": "0",
4326+
"branches-1-feature-value-MAX_NUM_FORMS": "1000",
4327+
"branches-1-feature-value-0-id": treat_fv.id,
4328+
"branches-1-feature-value-0-value": updated_treat_value,
4329+
"branches-1-screenshots-TOTAL_FORMS": "1",
4330+
"branches-1-screenshots-INITIAL_FORMS": "1",
4331+
"branches-1-screenshots-MIN_NUM_FORMS": "0",
4332+
"branches-1-screenshots-MAX_NUM_FORMS": "1000",
4333+
"branches-1-screenshots-0-id": treat_screenshot.id,
4334+
"branches-1-screenshots-0-description": treat_screenshot.description,
4335+
},
4336+
request=self.request,
4337+
)
4338+
self.assertTrue(form.is_valid(), form.errors)
4339+
form.save()
4340+
4341+
changelogs = experiment.get_changelogs_by_date()
4342+
all_changes = []
4343+
for entry in changelogs:
4344+
all_changes.extend(entry["changes"])
4345+
4346+
branch_changes = [
4347+
c
4348+
for c in all_changes
4349+
if c.get("event_message") and "branches" in c["event_message"].lower()
4350+
]
4351+
ref_branch_changes = [
4352+
c
4353+
for c in all_changes
4354+
if c.get("event_message") and "reference branch" in c["event_message"].lower()
4355+
]
4356+
4357+
feature_value_changed = False
4358+
for change in branch_changes + ref_branch_changes:
4359+
new_value = change.get("new_value", "")
4360+
if new_value and "updated-ref-value" in str(new_value):
4361+
feature_value_changed = True
4362+
break
4363+
4364+
self.assertTrue(
4365+
feature_value_changed,
4366+
"Feature value changes should be captured in the changelog. "
4367+
f"Found changes: {[c.get('event_message') for c in all_changes]}",
4368+
)
4369+
42044370

42054371
class TestNimbusBranchCreateForm(RequestFormTestCase):
42064372
def test_form_saves_branches(self):

0 commit comments

Comments
 (0)