fix: ensure individually_overridden is reliably cleared#283
Conversation
| end | ||
| end | ||
|
|
||
| it "remains during a bulk assignment" do |
There was a problem hiding this comment.
This seemed like "testing behavior that isn't part of the requirements" when I paged through the archived repository. I couldn't find any documented reason that this would be desirable behavior. It basically committed to what I think is a bug here.
| UPDATE assignments | ||
| SET individually_overridden = false | ||
| WHERE individually_overridden = true | ||
| AND context IS DISTINCT FROM 'individually_overridden' |
There was a problem hiding this comment.
I had an ephemeral test to validate that this migration behaved as expected, but it felt exotic to test a migration so I deleted it.
jmileham
force-pushed
the
fix_individually_overridden_leakage
branch
from
865b298 to
e7c1d60
Compare
…tions individually_overridden was not being maintained as a trustworthy signal for "this assignment is currently a manual per-visitor override." Two related bugs caused it to persist stale true values after bulk operations: 1. BulkReassignment's SQL UPDATE never included individually_overridden in its SET clause, leaving the flag true after bulk reassignment. 2. ArbitraryAssignmentCreation#individual_override? had a stickiness term (assignment.individually_overridden || !bulk_assignment?) that carried the flag forward onto the result even when a bulk operation superseded an individually-overridden record via the ArbitraryAssignmentCreation path. With the BulkReassignment bug fixed, this stickiness has no legitimate purpose — the flag will now be correctly false on any record a bulk operation has touched, so the term simplifies to !bulk_assignment?. A backfill migration corrects existing records where individually_overridden is true but context shows the last operation was not an individual override. The history of individual overrides is preserved in previous_assignments, though those records may themselves be tainted by these bugs.
jmileham
force-pushed
the
fix_individually_overridden_leakage
branch
from
e7c1d60 to
6d7bde1
Compare
|
@jmileham we're leaning on what I think is a server-side fired "splitAssigned" event (here), and it doesn't care about |
|
Thanks @jonmauney - I think I've found another data quality problem here where this was only ever set on supersession of a previous explicit assignment as well which also made it non-complete for "first time override" case, so I'm gonna add to this PR for that case as well, and backfill this column to a complete set of actual individual overrides |
2 participants
individually_overridden was not being maintained as a trustworthy signal for "this assignment is currently a manual per-visitor override." Two related bugs caused it to persist stale true values after bulk operations:
BulkReassignment's SQL UPDATE never included individually_overridden in its SET clause, leaving the flag true after bulk reassignment.
ArbitraryAssignmentCreation#individual_override? had a stickiness term (assignment.individually_overridden || !bulk_assignment?) that carried the flag forward onto the result even when a bulk operation superseded an individually-overridden record via the ArbitraryAssignmentCreation path. With the BulkReassignment bug fixed, this stickiness has no legitimate purpose — the flag will now be correctly false on any record a bulk operation has touched, so the term simplifies to !bulk_assignment?.
A backfill migration corrects existing records where individually_overridden is true but context shows the last operation was not an individual override. The history of individual overrides is preserved in previous_assignments, though those records may themselves be tainted by these bugs.
@samandmoore this is some prework buggy data identified before I take a stab at making stale individual overrides time out.
@jonmauney is there any possible impact on downstream processing? I don't think this value (which had to my eye buggy values after some bulk operations) is exposed on any API or used in filtering, and I believe all A/B test analysis is performed based on client-generated events now, so this feels low risk, but let me know if anything feels like it might be depending on this!