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: only send one digest email per org with all team info #27851

Merged
merged 3 commits into from
Jan 25, 2025

Conversation

raquelmsmith
Copy link
Member

Problem

For orgs with multiple teams, sending an email per team was overwhelming.

Changes

  • Collects all team info first
  • Organizes it into organizations
  • For each user, filters the reports to include only the teams they are members of
  • Sends the filtered org report for each user

I'm somewhat concerned about memory utilization with this approach because we're now keeping a lot more in memory than we were before. So any suggestions there are welcome - otherwise we will just see how it goes.

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

Does this work well for both Cloud and self-hosted?

Should..

How did you test this code?

Updated tests

@raquelmsmith raquelmsmith requested review from joethreepwood and a team January 24, 2025 03:39
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR consolidates multiple team digest emails into a single organization-wide email, improving the notification experience for organizations with multiple teams while maintaining proper access controls.

  • Introduced new OrgDigestReport and TeamDigestReport data classes in report_utils.py to structure organization-level reporting
  • Added user-specific report filtering in periodic_digest.py to ensure users only see data from teams they have access to and notifications enabled for
  • Implemented organization-level idempotency using MessagingRecord with campaign keys that include period length
  • Added comprehensive test coverage in test_periodic_digest.py for multi-team scenarios, access controls, and notification preferences
  • Potential memory concern with upfront data loading for large organizations, as noted in PR description

💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

3 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

posthog/tasks/report_utils.py Outdated Show resolved Hide resolved
Comment on lines 318 to 319
team_id=int(next(iter(user_report.teams.keys()))), # Use first team for compatibility
organization_id=org_id,
Copy link

Choose a reason for hiding this comment

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

logic: using first team arbitrarily could cause issues if that team has different settings or configuration than other teams in the org - consider using a more deterministic selection method

posthog/tasks/report_utils.py Show resolved Hide resolved
posthog/tasks/test/test_periodic_digest.py Outdated Show resolved Hide resolved
@joethreepwood
Copy link
Contributor

Going to remove my review and subscribe instead. I can follow the summary, but actually approving code like this is a bit of a stretch for me!

@joethreepwood joethreepwood removed their request for review January 24, 2025 10:03
Copy link
Contributor

@joshsny joshsny left a comment

Choose a reason for hiding this comment

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

Looks great 🚢

posthog/tasks/periodic_digest.py Show resolved Hide resolved
@raquelmsmith raquelmsmith enabled auto-merge (squash) January 25, 2025 21:11
@raquelmsmith raquelmsmith merged commit adfc75a into master Jan 25, 2025
92 checks passed
@raquelmsmith raquelmsmith deleted the feat/digest-single-email-per-org branch January 25, 2025 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants