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(backend): schema updates, migration, queries for Email Notification Service #9445

Merged

Conversation

ntindle
Copy link
Member

@ntindle ntindle commented Feb 7, 2025

The email service has requirements to

  • Email users when some activity has happened on their account on some scheduled basis -> We need a way to get active users and the executions that happened while they were active
  • Allow users to configure what emails they get -> Need a user preference
  • Get User email by Id so that we can email them -> Pretty self-explanatory

We need to add a few new backend queries + db models for the notification to start handling these details. This is the first set of those changes based on experience building the app service

Changes 🏗️

  • Adds a new DB Model, UserNotificationPreferences, with related migration
  • Adds a new DB Model NotificationEvent with related migration to track what notifications we've sent and how many and such (how much we add here is open to change depending on what limits on data we want)
  • Adds a new DB Model UserNotificationBatch with related migration to handle batching of like models
  • Adds queries to get users and executions by datetime ranges as ISO strings
  • Adds new queries to the DatabaseManager and exposes them to the other AppServices
  • Exposes all new queries plus an existing one get_user_by_id

Checklist 📋

For code changes:

  • I have clearly listed my changes in the PR description
  • I have made a test plan
  • I have tested my changes according to the test plan:
    • I extracted these changes from a working implementation of the service, and tested they don't bring down the service by being uncalled by running the standard agent tests we do on release

@ntindle ntindle requested a review from a team as a code owner February 7, 2025 22:19
@ntindle ntindle requested review from Pwuts and aarushik93 and removed request for a team February 7, 2025 22:19
@github-actions github-actions bot added the platform/backend AutoGPT Platform - Back end label Feb 7, 2025
Copy link

qodo-merge-pro bot commented Feb 7, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 Security concerns

SQL Injection:
The get_executions_in_timerange and get_active_user_ids_in_timerange functions accept date strings and parse them using datetime.fromisoformat(). While this provides some validation, additional input sanitization may be needed to prevent SQL injection through malformed date strings.

⚡ Recommended focus areas for review

Default Values

The get_user_notification_preference function creates a new NotificationPreference with hardcoded default values (daily_limit=3) and empty preferences. Consider moving these defaults to constants or configuration.

notification_preference = NotificationPreference(
    user_id=user.id,
    email=user.email,
    # TODO with the UI when it comes in
    preferences={},
    daily_limit=3,
    emails_sent_today=0,
    last_reset_date=datetime.now(),
)
Timezone Handling

The NotificationPreference model uses datetime.now() as default for last_reset_date without timezone information. This could cause issues with timezone-dependent features like daily limits.

class NotificationPreference(BaseModel):
    user_id: str
    email: EmailStr
    preferences: dict[NotificationType, bool] = {}  # Which notifications they want
    daily_limit: int = 10  # Max emails per day
    emails_sent_today: int = 0
    last_reset_date: datetime = datetime.now()

@github-actions github-actions bot added the size/l label Feb 7, 2025
Copy link

deepsource-io bot commented Feb 7, 2025

Here's the code health analysis summary for commits 016ec0f..20267d6. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource JavaScript LogoJavaScript✅ SuccessView Check ↗
DeepSource Python LogoPython✅ Success
❗ 2 occurences introduced
View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

Base automatically changed from ntindle/secrt-1087-attach-rabbit-mq-to-the-services-processes-similar-to-how to dev February 8, 2025 21:49
@github-actions github-actions bot added the conflicts Automatically applied to PRs with merge conflicts label Feb 12, 2025
@github-actions github-actions bot removed the conflicts Automatically applied to PRs with merge conflicts label Feb 12, 2025
Copy link
Contributor

Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly.

@ntindle ntindle requested review from Swiftyos and Pwuts February 12, 2025 02:34
Swiftyos
Swiftyos previously approved these changes Feb 12, 2025
@ntindle ntindle added this pull request to the merge queue Feb 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 12, 2025
@ntindle ntindle added this pull request to the merge queue Feb 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 12, 2025
@github-actions github-actions bot added the platform/frontend AutoGPT Platform - Front end label Feb 12, 2025
@github-actions github-actions bot removed the platform/frontend AutoGPT Platform - Front end label Feb 12, 2025
@ntindle ntindle enabled auto-merge February 12, 2025 22:29
@itsababseh itsababseh self-assigned this Feb 12, 2025
Copy link

@itsababseh itsababseh left a comment

Choose a reason for hiding this comment

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

If this breaks anything I John am not liable cc @ntindle :)

@ntindle ntindle added this pull request to the merge queue Feb 12, 2025
Merged via the queue into dev with commit 7e04fbd Feb 12, 2025
23 checks passed
@ntindle ntindle deleted the ntindle/secrt-1088-add-db-models-for-the-notification-service branch February 12, 2025 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants