Execute merge_people in plugin server #5452
Conversation
|
forgot to tag you for review yesterday @macobo |
|
Nice work! This looks correct but haven't tested this out. I wonder if some sort of a cross plugin-server & posthog test would make sense here, wdyt? |
There was a problem hiding this comment.
Q: What assumptions exist around idempotency / ordering of events?
I know that right now, $set & $set_once face this, where the order matters. (And we've been thinking of finding a way around it, (for https://github.com/PostHog/plugin-server/issues/406 ? ).
I think this event definitely falls into the same category, right?
Edit: ok, create_alias already exists, so this isn't a "new" addition. This bit is not important then I guess.
cypress/integration/person.js
Outdated
| cy.contains('amalia.potts@hotmail.com').click() | ||
| cy.contains('OK').click() | ||
|
|
||
| cy.wait(2000) |
There was a problem hiding this comment.
This is quite optimistic on github actions settings.
Increasing the timeout just slows down tests as well.
A better solution might be to reload-and-check until the below condition is true UP TO e.g. 20s. https://docs.cypress.io/guides/core-concepts/retry-ability
See https://github.com/PostHog/posthog-js/blob/master/cypress/support/commands.js#L44-L49 for a custom command which retries until it succeeds.
Aside - we don't reset the db between tests right? In that case couldn't this start causing test-ordering related issues?
There was a problem hiding this comment.
Aside - we don't reset the db between tests right? In that case couldn't this start causing test-ordering related issues?
It's a good point, I would have thought so too but tests seem to have passed fine?
Can configure this to be more general
There was a problem hiding this comment.
It might be fine as long as these users aren't directly or indirectly asserted against in other tests :)
e3b9dd0 to
7523b0f
Compare
Changes
Fixes #5145
merge_peoplenow delegates work to the plugin server by creating a$create_aliasevent.Tests make sure that the event is created properly but tests for the merging logic are ofc in the plugin server code.
Screen.Recording.2021-08-04.at.14.50.40.mov
Checklist