Fix/backfill duplicate handling

[adekbadek]

All Submissions:

• Have you followed the Newspack Contributing guideline?
• Does your code follow the WordPress' coding standards and VIP Go coding standards?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Fixes two issues:

1. If and event is not persisted in the event log, it should not be processed.
2. When backfilling reader_registration events, disregard current_page_url metadata.

By accident this branch also contains #71, but it won't hurt to add that to the alpha release.

How to test the changes in this Pull Request:

0. On alpha,
1. Trigger a new reader_registered event by registering as a new reader on the site
2. Observe in the Event Log that the event has a metadata.current_page_url key in the data property
3. Run the backfill for reader_registered events (e.g. wp newspack-network data-backfill reader_registered --start=2024-02-26 --end=2024-12-31 --verbose)
4. Observe a new reader_registered event added, even though a reader_registered for this reader is already present
5. Observe that the new event does not have the metadata.current_page_url key
6. Switch to this branch, repeat the testing steps, observe the duplicate event is not added

Other information:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes, as applicable?
• Have you successfully ran tests with your changes locally?

[chickenn00dle]

Tested and looks good for the most part. I am running into one issue while testing though. With these changes checked out, I see that all events will trigger exactly one duplicate before being ignore by the data backfill script.

For example, Here I have created 3 users (test4, test5, and test6@example.com), and each have triggered an initial reader_registered event:

When I run the backfill script the first time, each one triggers an additional reader_registered event:

It isn't until the second time I run this command that duplicates are actually ignored:

Is this expected?

[adekbadek]

Not expected, you've uncovered a shortcoming of this solution. It assumes the only metadata missing will be current_page_url, but in your example there's also a referer metadata key. The solution should be to disregard all data. 62fdf8d should fix it.

[chickenn00dle]

Hmm. I'm still running into an issue, although the behavior has slightly changed. This time, everything but the first of a batch of user registrations is being duplicated. For example, here is test7, 8, and 9. Running the backfill script creates duplicate events for test 8 and 9:

[adekbadek]

This is a bigger issue than I thought! The code is comparing timestamps. The event will be saved a few milliseconds after the actual registration. Thus when looking for an event at time of the registration, it won't be found. Fixed in ee7be33

[chickenn00dle]

Nice work @adekbadek!

This is working now so gonna give a thumbs up.

I did have one question/nit which is can we simply disregard/drop timestamp as well if the only relevant information we need to consider are email and date? Definitely not a blocker though and something we can revisit if necessary.

[adekbadek]

Good suggestion! For registrations that makes sense, because there should be just one per email. The timestamp-range solution should also work for other events, though, and for these disregarding the timestamp altogether is not desirable. There might be many of them, with only the timestamp as differentiator (e.g. donations).

[matticbot]

🎉 This PR is included in version 1.3.0-alpha.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 1.4.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 1.5.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 1.5.0-epic-data-integrity-improvements.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[matticbot]

🎉 This PR is included in version 1.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀