Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backfilled missing offer redemptions #20647

Conversation

cmraible
Copy link
Collaborator

@cmraible cmraible commented Jul 24, 2024

ref https://linear.app/tryghost/issue/ENG-1440/backfill-offer-redemption-data-with-a-migration

There was a bug that caused offer redemptions to not be recorded in the database for some subscriptions that were created with an offer. However, we still have the offer_id attached to the subscriptions, so we are able to backfill the missing redemptions. The bug was fixed in bf895e6

This commit only contains a migration, which queries for subscriptions that have an offer_id but do not have any offer redemptions recorded, and adds any missing redemptions to the offer_redemptions table.

@github-actions github-actions bot added the migration [pull request] Includes migration for review label Jul 24, 2024
Copy link
Contributor

github-actions bot commented Jul 24, 2024

It looks like this PR contains a migration 👀
Here's the checklist for reviewing migrations:

General requirements

  • Satisfies idempotency requirement (both up() and down()) — This is a data backfill, so down() no-ops — we wouldn't want to (or be able to) un-backfill this data. But up() can safely be run multiple times without any adverse effects
  • Does not reference models
  • Filename is in the correct format (and correctly ordered)
  • Targets the next minor version
  • All code paths have appropriate log messages
  • Uses the correct utils
  • Contains a minimal changeset
  • Does not mix DDL/DML operations

Schema changes

N/A

Data changes

  • Mass updates/inserts are batched appropriately
  • Does not loop over large tables/datasets — the only loop is to generate ObjectIDs for the offer_redemptions before batch inserting them, which should be very quick, and should only run on ~4k rows
  • Defends against missing or invalid data
  • For settings updates: follows the appropriate guidelines (N/A)

@cmraible cmraible force-pushed the chris-eng-1440-backfill-offer-redemption-data-with-a-migration branch 2 times, most recently from f4981c3 to d0dd82e Compare July 30, 2024 19:52
@cmraible cmraible changed the title 🚧 Added migration to backfill missing offer redemptions Added migration to backfill missing offer redemptions Jul 31, 2024
@cmraible cmraible marked this pull request as ready for review July 31, 2024 00:39
@cmraible cmraible requested review from 9larsons and ErisDS July 31, 2024 00:39
if (rows && rows.length > 0) {
logging.info(`Backfilling ${rows.length} offer redemptions`);
// Generate IDs for each row
const offerRedemptions = rows.map((row) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daniellockyer as someone who has seen a lot of these go by, do you have any concerns with this loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own due diligence, I did a search and found this is a pretty common pattern in migrations, specifically for generating object IDs. I checked a couple of them, and did a search in Slack for their rollout and haven't found any drama.

Also tested on staging with 40k+ redemptions to backfill (max in production will be ~4k) and it completed in <3 seconds

@cmraible cmraible force-pushed the chris-eng-1440-backfill-offer-redemption-data-with-a-migration branch 3 times, most recently from ed6774c to e953489 Compare August 1, 2024 22:11
ref https://linear.app/tryghost/issue/ENG-1440/backfill-offer-redemption-data-with-a-migration
ref TryGhost@bf895e6

There was a bug that caused offer redemptions to not be recorded in the database for some subscriptions that were created with an offer. The bug was fixed in the references commit above.

This commit backfills the missing redemptions, based on the subscriptions that have an `offer_id` but no corresponding offer redemptions.
@cmraible cmraible force-pushed the chris-eng-1440-backfill-offer-redemption-data-with-a-migration branch from e953489 to d946577 Compare August 1, 2024 22:48
@cmraible cmraible changed the title Added migration to backfill missing offer redemptions Backfilled missing offer redemptions Aug 1, 2024
@cmraible cmraible merged commit 7522b74 into TryGhost:main Aug 1, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migration [pull request] Includes migration for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants