Skip to content

Conversation

tiina303
Copy link
Contributor

Problem

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

@posthog-bot
Copy link
Contributor

Hey @tiina303! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

@tiina303 tiina303 force-pushed the tiina/person-contstraint-wip branch from 35b6e22 to dfeb531 Compare February 21, 2023 11:24
@tiina303 tiina303 force-pushed the tiina/person-contstraint-wip branch from dfeb531 to d944c53 Compare February 21, 2023 11:25
@tiina303 tiina303 force-pushed the tiina/person-contstraint-wip branch from 9e600f1 to f88d3db Compare February 21, 2023 12:52
@tiina303 tiina303 changed the title wip: person constraint in person overrides table fix: person constraint in person overrides table Feb 21, 2023
@tiina303 tiina303 marked this pull request as ready for review February 21, 2023 15:04
@tiina303 tiina303 merged commit ddffe0f into fix/add-person-consrtaint-on-override Feb 21, 2023
@tiina303 tiina303 deleted the tiina/person-contstraint-wip branch February 21, 2023 15:06
@tiina303 tiina303 mentioned this pull request Feb 27, 2023
tomasfarias added a commit that referenced this pull request Mar 13, 2023
* feat(person-override): Add a helper model to indirectly reference person overrides

This allows us to use an exclusion constraint on the person overrides table instead of directly using a FK on posthog_person.

* test(person-overrides): Update tests to match new constraint

* fix(migration): Add drop extension query to reverse migration

* fix(migration): Use correct table name

* refactor(person-overrides): Make team a regular bigint field without FK

* refactor(person-overrides): Rename Helper model to Mapping

* feat(person-override): Add a helper model to indirectly reference person overrides

This allows us to use an exclusion constraint on the person overrides table instead of directly using a FK on posthog_person.

* wip: add test for concurrent updates to posthog_personoverride table

* refactor

* Update snapshots

* fix(isort): Correctly sort imports

* fix(ee-cohort-test): Delete person after creating it

* fix: person constraint in person overrides table (#14319)

* nits

* fix migration tests

* chore(migration): Bump migration number to 0302

* Update snapshots

* feat: person-overrides writes

* test(person-overrides): Add concurrent tests to person-overrides model

* feat(person-merge): Update merge to use new helper table

* fix(tests): Pass poEEmbraceJoin to updatePersonState in test

* fix(person-state): Format person-overrides message for ClickHouse

* test(api): Add function to reload dictionary for person overrides

* fix: poe final test failure

* refactor(person-state): Make failed attempts a class variable to

This allows us to mock it during testing as some tests require
immediate failures.

* fix(postgres-utils): Apply some magic changes lost to time

* fix(person-state): Join with helper table to return UUIDs

* fix(person-state): Use single quotes for UUID queries

* test(person-overrides): Skip test that doesn't work without a merge command

* test(person-state): Add a very complicated query to get UUIDs in a test

* test(person-state): Try waiting longer, this is flaky

* fix(migrations): Remove unused migration

* fix(person): Undo unneeded person model changes

* chore: Clean-up artifacts from rebase on model branch

* refactor(person-state): Rename mergeAttempts and don't read from ENV

* chore: Better clarify oldest_event usage in comment

Co-authored-by: Tiina Turban <tiina303@gmail.com>

* refactor(person-state): Update version in queries

* fix(person-state): Use new mapping model instead of helper

* fix(migrations): Re-add constraint deleted on field drop

* revert: Re-enable test that was skipped

Test was originally skipped due to missing a $merge command, but now
$merge_dangerously is available, so the test should pass.

* test: Attempt to refresh dictionary in test

* Update query snapshots

* Update query snapshots

* test: Expect dictionaries to be refreshed before resuming test

* fix(test): Use alias property for $merge_dangerously in test

* fix(test): Expect any string like in all other tests

* test: Fix order of events to determine merged persons

* chore: Update comment regarding overrides mapping query

* chore: Remove out of date comment

* test: Set number of retries to 0 when updating persons

* test: Delete comment

* test: Assert clickhouse state after identify

* chore: Move comment

* chore: Delete TODO regarding oldest_event updates

* test: Assert events are still processed if merge fails

* test: Assert properties are processed if merge fails

* test: Don't expect to throw when failing merges

* fix: Indent RETURNING query clause

Co-authored-by: Harry Waye <harry@posthog.com>

* chore: Remove out of date comment

Co-authored-by: Tiina Turban <tiina303@gmail.com>

* test: Resume merges after test suite

* test: Run processPersonStep tests for both poEEmbraceJoin modes

---------

Co-authored-by: Harry Waye <harry@posthog.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tiina Turban <tiina303@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants