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: resend email campaigns on a frequency #18148

Closed
wants to merge 10 commits into from

Conversation

raquelmsmith
Copy link
Member

@raquelmsmith raquelmsmith commented Oct 24, 2023

Supersedes #17365 as it was split into parts.

Problem

We sometimes want to send the same email, but do so on some sort of frequency, and no more than that frequency.

For example, we want to notify CS when a never_drop_data customer go over their quota - but we don't want to email CS every time we check the quota limits. But, if the problem is resolved but happens again later, we want the email to be able to be sent again.

Changes

Drops the existing constraint and leverages the new one that was added in #17636.

Sets up resending based on email resend_frequency.

Adds the specific email in question described above.

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

How did you test this code?

Added some tests.

@raquelmsmith raquelmsmith requested a review from frankh October 24, 2023 03:43
),
migrations.RunSQL(
sql="""
ALTER TABLE posthog_messagingrecord DROP CONSTRAINT "posthog_messagingrecord_email_hash_campaign_key_6639a13f_uniq";
Copy link
Member Author

@raquelmsmith raquelmsmith Oct 24, 2023

Choose a reason for hiding this comment

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

@frankh can you confirm this is the constraint name? the 6639a13f has me concerned..

also please triple check my sql 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

that's definitely the name on dev, will check prod too

posthog_messagingrecord_email_hash_campaign_key_6639a13f_uniq

Copy link
Contributor

Choose a reason for hiding this comment

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

same on prod:

posthog_messagingrecord_email_hash_campaign_key_6639a13f_uniq

sql="""
ALTER TABLE posthog_messagingrecord DROP CONSTRAINT "posthog_messagingrecord_email_hash_campaign_key_6639a13f_uniq";
""",
reverse_sql="""
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we should have the reverse_sql here, if we need to rollback we should manage it manually (creating this constraint could cause the lockup issues we were trying to avoid by doing it all concurrently)

Copy link
Contributor

@frankh frankh left a comment

Choose a reason for hiding this comment

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

looks good just remove the reverse_sql=... part and LGTM

@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.

@posthog-bot
Copy link
Contributor

It looks like the code of hogql-parser has changed since last push, but its version stayed the same at 1.0.2. 👀
Make sure to resolve this in hogql_parser/setup.py before merging!

@raquelmsmith raquelmsmith requested a review from frankh November 6, 2023 21:38
@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.

@posthog-bot
Copy link
Contributor

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

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