The new constraint seems semantically different from the check constraint as the check constraint was preventing the usage of old_person_id as override_person_id while allowing repeated override_person_ids.

However, the new constraint would allow existing old_person_id (as these exist in the Person model) to be used as override_person_id or viceversa. The original constraint included a fiddle to account for each possible case, I've adapted it to a new fiddle which shows that test cases now pass when they used to fail.

This is not necessarily an issue, just wondering what is motivating the change in semantics.

The new constraint seems semantically different from the check constraint as the check constraint was preventing the usage of old_person_id as override_person_id while allowing repeated override_person_ids.

However, the new constraint would allow existing old_person_id (as these exist in the Person model) to be used as override_person_id or viceversa. The original constraint included a fiddle to account for each possible case, I've adapted it to a new fiddle which shows that test cases now pass when they used to fail.

This is not necessarily an issue, just wondering what is motivating the change in semantics.

The new constraint relies on the assumptions:

1. Something being used as an old_person_id implies that it was deleted from the person table, meaning that it will not be possible that it is in the override_person_id column.
2. Something in the override_person_id column implies that it is in the person table, which we assume means that it is not in the old_person_id column.

That's obviously dependent on the application implementing the assumptions correctly.

I couldn't find a way that would provide the constraint that would handle the test case that I've included, which is roughly how the plugin server will be handling the merge. The test cases in the fiddle would need to include a DELETE on the person table to get the desired behaviour.

Not ideal. If you can find a way to get the desired constraints on the personoverrides table on it's own that would be great. It would likely need to use a unique index or excludes in some way rather than a check constraint as these don't apply at commit time afaict.

@tomasfarias I've also added a corresponding test in the plugin-server here fwiw.

I'm on pipeline team support this sprint so keen to get this handed over! Let me know if you want to hangout to do the handover.

I've rebased this PR on origin/master to get the changes to CI plugin server in.

Thought about deletions ... it's fine for us to leave them in the overrides table with broken keys (as in person was deleted entry is still in the overrides table), but we should add a test to the follow-up PR to make sure that doesn't break adding a future override:

1. merge X -> Y
2. delete Y
3. merge Y (distinct_id) -> Z and make sure it does the right thing (we can probably just update X -> Z here too 🤷‍♀️ )

When we have soft deletes I wonder if this would work: https://docs.djangoproject.com/en/4.1/ref/models/fields/#django.db.models.ForeignKey.limit_choices_to - old_person being limited_to is_deleted: true and override_person being limited to is_deleted: false
though I suspect this doesn't have guarantees on concurrent transactions

Rebased on master to push our rework migration to number 0302 as number 0301 was taken.