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

feat: change unique idx on messsaging table #17636

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

raquelmsmith
Copy link
Member

Problem

In #17365 we want to change the messaging record so that we can send multiple emails with the same campaign key, but only on some sort of frequency. We still need to make sure the campaign send number is is unique though so we don't try to send multiple emails at once.

Problem with that PR is that we are changing the constraint on the table, and changing constraints can have issues. So, my understanding is that we need to:

  1. Create a new index with the constraint including the new field (this PR)
  2. Drop the old constraint in feat: add field to never drop data for a customer #17365

Changes

Adds a new field campaign_count to the MessagingRecord model.
Creates a new index concurrently with a constraint including that field.

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

How did you test this code?

@raquelmsmith raquelmsmith requested a review from frankh September 26, 2023 18:48
@raquelmsmith
Copy link
Member Author

@frankh I think this is what you had in mind but lmk if not. The existing PR will need to be updated to drop the old index.

One question I have is around Django's makemigrations command. If I change this on the messaging record:

class Meta:
        unique_together = ("email_hash", "campaign_key")  # can only send campaign once to each email

Then makemigrations will notice that it has been changed and will try to create a migration with AlterUniqueTogether. But in creating / dropping the indexes manually that won't be necessary.... so do I remove this line completely? Then we have no record of the uniqueness in the codebase, aside from the migrations.

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@raquelmsmith raquelmsmith merged commit 025ed3d into master Oct 5, 2023
@raquelmsmith raquelmsmith deleted the feat/messaging-unique-constraint branch October 5, 2023 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants