[copilot] notifications v2 #1099
Conversation
WalkthroughThis change introduces a comprehensive notification system overhaul, adding support for categorized notifications, user subscription and email preferences per category, and an urgent notifications bar. The backend implements new models, API endpoints, and service logic for managing notification categories, user preferences, and filtered/urgent notification retrieval. Notification creation and delivery now respect user category subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant Service
participant DB
User->>Frontend: Open notification preferences/settings
Frontend->>API: GET /api/notifications/preferences
API->>Service: get_user_notification_preferences()
Service->>DB: Query UserNotificationPreferences
DB-->>Service: Preferences data
Service-->>API: Preferences dict
API-->>Frontend: Preferences JSON
User->>Frontend: Update preferences
Frontend->>API: PUT /api/notifications/preferences
API->>Service: update_user_notification_preferences()
Service->>DB: Update UserNotificationPreferences
DB-->>Service: Updated record
Service-->>API: Success
API-->>Frontend: Success
Note over API,Service: Similar flow for filtered/urgent notification retrieval
participant Webhook
Webhook->>API: Job failure event
API->>Service: create_job_failure_notification()
Service->>DB: Create Notification (category=job_failure)
Service->>DB: Check UserNotificationPreferences for recipients
DB-->>Service: Preferences
Service-->>API: Notification created
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
ddpui/models/userpreferences.py (1)
16-33: I wasn’t able to locate clear definitions of both models in a single glance. To be certain we’re not overlooking nuanced differences (e.g. model fields, relationships, default values), could you confirm:
- The full field list (names and help_text/defaults) on
UserNotificationPreferencesinddpui/models/notifications.py.- Any key business logic differences—e.g., does one model drive email vs. in-app vs. SMS channels while the other drives per-category on/off toggles?
With that side-by-side, we can better decide whether to merge or keep distinct.
ddpui/core/notifications_service.py (2)
572-618: Consider consolidating duplicate filtering functions.This function largely duplicates the functionality of
fetch_user_notifications_by_category(lines 433-480). Both filter notifications by category and urgency. Consider consolidating them into a single, more comprehensive function.Would you like me to help refactor these functions into a single, unified notification filtering function that covers all use cases?
641-689: Well-structured notification creation functions!These specialized functions properly utilize the categorized notification system and follow a consistent pattern. Consider adding a configurable urgency level for these notifications, as some organizations might want job failures to be urgent.
NOTIFICATION_ENHANCEMENTS.md (1)
290-306: Clear deployment instructions!The deployment steps are well-ordered and comprehensive. Consider adding a rollback plan in case issues are encountered during deployment.
Add a rollback section:
4. Test notification flows 5. Enable auto-generation features +### Rollback Plan + +1. Disable AUTO_GENERATE_LLM_FAILURE_SUMMARIES if issues occur +2. Revert database migrations if needed +3. Deploy previous version of the code +4. Restore previous frontend version + This enhancement provides a robust foundation for categorized notifications while maintaining backward compatibility with the existing system.ddpui/schemas/notifications_api_schemas.py (1)
75-85: Consider consistent naming between input and response schemas.The field names differ between
UserNotificationPreferencesSchemaandUserNotificationPreferencesResponseSchema(e.g.,enable_email_notificationsvsemail_notifications_enabled). While this might be intentional for API design, consistent naming could improve maintainability.If consistent naming is desired, consider aligning the field names:
# Option 1: Align response schema to input schema - email_notifications_enabled: bool - incident_notifications: bool + enable_email_notifications: bool + subscribe_incident: boolOr standardize both to a consistent pattern.
Also applies to: 94-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
NOTIFICATION_ENHANCEMENTS.md(1 hunks)ddpui/api/notifications_api.py(3 hunks)ddpui/core/notifications_service.py(6 hunks)ddpui/models/notifications.py(2 hunks)ddpui/models/userpreferences.py(2 hunks)ddpui/schemas/notifications_api_schemas.py(3 hunks)ddpui/utils/webhook_helpers.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ddpui/models/userpreferences.py (1)
ddpui/models/notifications.py (1)
is_subscribed_to_category(41-51)
ddpui/core/notifications_service.py (5)
ddpui/models/notifications.py (3)
UserNotificationPreferences(16-51)NotificationCategory(5-13)is_subscribed_to_category(41-51)ddpui/models/userpreferences.py (3)
is_subscribed_to_category(51-61)UserPreferences(6-61)to_json(38-49)ddpui/utils/awsses.py (1)
send_text_message(14-26)ddpui/api/notifications_api.py (2)
get_user_notification_preferences(265-292)update_user_notification_preferences(296-338)ddpui/schemas/notifications_api_schemas.py (1)
NotificationDataSchema(63-72)
🪛 Ruff (0.11.9)
ddpui/utils/webhook_helpers.py
189-189: Test for membership should be not in
Convert to not in
(E713)
ddpui/api/notifications_api.py
10-10: ddpui.schemas.notifications_api_schemas.NotificationFiltersSchema imported but unused
Remove unused import
(F401)
13-13: ddpui.schemas.notifications_api_schemas.NotificationResponseSchema imported but unused
Remove unused import
(F401)
14-14: ddpui.schemas.notifications_api_schemas.UrgentNotificationSchema imported but unused
Remove unused import
(F401)
ddpui/core/notifications_service.py
534-534: Redefinition of unused get_user_notification_preferences from line 400
(F811)
540-540: Redefinition of unused update_user_notification_preferences from line 413
(F811)
620-620: Redefinition of unused get_urgent_notifications_for_user from line 482
(F811)
🔇 Additional comments (12)
ddpui/models/notifications.py (1)
23-29: Subscription logic uses UserNotificationPreferences, not UserPreferences
The notification service and API exclusively use the UserNotificationPreferences model (with all defaults set to True) for subscription checks. The UserPreferences model defaults (e.g.subscribe_job_failure=False) are not used in this flow and can be ignored here. No changes required.Likely an incorrect or invalid review comment.
ddpui/core/notifications_service.py (5)
8-9: LGTM!The imports for
UserNotificationPreferencesandNotificationCategoryare correctly added to support the new categorized notification system.
127-127: Good implementation of backward compatibility!Using
getattrwith a default value of "system" ensures that existing code without the category field will continue to work correctly.Also applies to: 136-136
433-480: Well-implemented category filtering function!The function properly handles filtering by category and urgency, with appropriate pagination support. The response includes all necessary fields including the new category field.
534-570: Keep these implementations and remove the earlier duplicates.These functions correctly use the
UserNotificationPreferencesmodel and should be the ones retained. Remove the duplicate definitions at lines 400-431.
620-639: This is the correct implementation to keep.This version has a cleaner return type (List instead of Tuple) which is more appropriate for the urgent notifications bar use case.
NOTIFICATION_ENHANCEMENTS.md (2)
1-45: Excellent documentation structure!The overview clearly outlines all the new features and categorizes them logically. The notification categories are well-defined and cover common use cases.
230-235: Clarify the relationship and migration strategy between UserPreferences and UserNotificationPreferencesThe documentation currently states that the existing UserPreferences model remains unchanged and “works alongside” the new UserNotificationPreferences model, but it’s unclear how they interact and which should be authoritative. This overlap—especially around email preferences and category subscriptions—could lead to data inconsistency. Please clarify:
- Which model is the source of truth for each type of notification setting?
- What is the migration strategy for existing data (e.g., consolidating or synchronizing preferences)?
- How should the system resolve conflicts if both models contain differing values?
Update the docs or implementation to define clear ownership, data flow, and conflict-resolution rules.
ddpui/schemas/notifications_api_schemas.py (4)
19-31: Well-structured notification categories enum.The enum provides a comprehensive set of notification categories that cover the common notification scenarios in a data platform. The categories are clearly named and well-documented.
43-43: Proper enum integration with sensible default.The category field is correctly implemented using the enum type with an appropriate default value.
87-92: Clean and focused filtering schema.The schema provides appropriate filtering options with proper enum usage and clear intent.
106-116: Well-designed response schemas for different use cases.Both schemas are appropriately structured for their specific purposes - detailed notification responses and urgent notification bar display respectively.
Also applies to: 118-125
| # Get system user to use for LLM summarization | ||
| from ddpui.utils.constants import SYSTEM_USER_EMAIL | ||
|
|
There was a problem hiding this comment.
Remove redundant import of SYSTEM_USER_EMAIL.
SYSTEM_USER_EMAIL is already imported at line 37. This redundant import should be removed.
Apply this diff:
- # Get system user to use for LLM summarization
- from ddpui.utils.constants import SYSTEM_USER_EMAIL
-
system_user = OrgUser.objects.filter(user__email=SYSTEM_USER_EMAIL).first()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Get system user to use for LLM summarization | |
| from ddpui.utils.constants import SYSTEM_USER_EMAIL | |
| system_user = OrgUser.objects.filter(user__email=SYSTEM_USER_EMAIL).first() |
🤖 Prompt for AI Agents
In ddpui/utils/webhook_helpers.py around lines 195 to 197, remove the redundant
import statement for SYSTEM_USER_EMAIL since it is already imported at line 37.
Simply delete the line "from ddpui.utils.constants import SYSTEM_USER_EMAIL" to
avoid duplicate imports.
| """Generate LLM failure summary for a failed Prefect flow run""" | ||
| try: | ||
| # Check if auto-generation of LLM failure summaries is enabled | ||
| if not os.getenv("AUTO_GENERATE_LLM_FAILURE_SUMMARIES", "").lower() in ["true", "1", "yes"]: |
There was a problem hiding this comment.
Use not in for membership test.
The membership test should use not in for better readability.
Apply this diff:
- if not os.getenv("AUTO_GENERATE_LLM_FAILURE_SUMMARIES", "").lower() in ["true", "1", "yes"]:
+ if os.getenv("AUTO_GENERATE_LLM_FAILURE_SUMMARIES", "").lower() not in ["true", "1", "yes"]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not os.getenv("AUTO_GENERATE_LLM_FAILURE_SUMMARIES", "").lower() in ["true", "1", "yes"]: | |
| if os.getenv("AUTO_GENERATE_LLM_FAILURE_SUMMARIES", "").lower() not in ["true", "1", "yes"]: |
🧰 Tools
🪛 Ruff (0.11.9)
189-189: Test for membership should be not in
Convert to not in
(E713)
🤖 Prompt for AI Agents
In ddpui/utils/webhook_helpers.py at line 189, replace the current condition
using 'not' combined with 'in' by directly using 'not in' for the membership
test to improve readability. Change the expression to use 'not in' followed by
the list of allowed values.
| def is_subscribed_to_category(self, category: str) -> bool: | ||
| """Check if user is subscribed to a specific notification category""" | ||
| subscription_map = { | ||
| "incident": self.subscribe_incident, | ||
| "schema_change": self.subscribe_schema_change, | ||
| "job_failure": self.subscribe_job_failure, | ||
| "late_run": self.subscribe_late_run, | ||
| "dbt_test_failure": self.subscribe_dbt_test_failure, | ||
| "system": self.subscribe_system, | ||
| } | ||
| return subscription_map.get(category, True) # Default to True for unknown categories |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use enum values for category mapping consistency.
The method uses string keys for category mapping while NotificationCategory enum values are used elsewhere in the codebase. Consider using the enum for consistency.
Import the enum and use it for mapping:
from ddpui.models.notifications import NotificationCategoryThen update the method:
def is_subscribed_to_category(self, category: str) -> bool:
"""Check if user is subscribed to a specific notification category"""
subscription_map = {
- "incident": self.subscribe_incident,
- "schema_change": self.subscribe_schema_change,
- "job_failure": self.subscribe_job_failure,
- "late_run": self.subscribe_late_run,
- "dbt_test_failure": self.subscribe_dbt_test_failure,
- "system": self.subscribe_system,
+ NotificationCategory.INCIDENT: self.subscribe_incident,
+ NotificationCategory.SCHEMA_CHANGE: self.subscribe_schema_change,
+ NotificationCategory.JOB_FAILURE: self.subscribe_job_failure,
+ NotificationCategory.LATE_RUN: self.subscribe_late_run,
+ NotificationCategory.DBT_TEST_FAILURE: self.subscribe_dbt_test_failure,
+ NotificationCategory.SYSTEM: self.subscribe_system,
}
return subscription_map.get(category, True) # Default to True for unknown categories🤖 Prompt for AI Agents
In ddpui/models/userpreferences.py around lines 51 to 61, the method
is_subscribed_to_category uses string keys for the subscription_map, but the
codebase uses the NotificationCategory enum for categories. Import
NotificationCategory from ddpui.models.notifications and update the
subscription_map keys to use NotificationCategory enum members instead of
strings to ensure consistency across the codebase.
| NotificationFiltersSchema, | ||
| NotificationCategoryEnum, | ||
| UserNotificationPreferencesResponseSchema, | ||
| NotificationResponseSchema, | ||
| UrgentNotificationSchema, |
There was a problem hiding this comment.
Remove unused imports.
Static analysis detected unused imports that should be removed.
Apply this diff:
from ddpui.schemas.notifications_api_schemas import (
CreateNotificationPayloadSchema,
UpdateReadStatusSchema,
UpdateReadStatusSchemav1,
UserNotificationPreferencesSchema,
- NotificationFiltersSchema,
NotificationCategoryEnum,
UserNotificationPreferencesResponseSchema,
- NotificationResponseSchema,
- UrgentNotificationSchema,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NotificationFiltersSchema, | |
| NotificationCategoryEnum, | |
| UserNotificationPreferencesResponseSchema, | |
| NotificationResponseSchema, | |
| UrgentNotificationSchema, | |
| from ddpui.schemas.notifications_api_schemas import ( | |
| CreateNotificationPayloadSchema, | |
| UpdateReadStatusSchema, | |
| UpdateReadStatusSchemav1, | |
| UserNotificationPreferencesSchema, | |
| NotificationCategoryEnum, | |
| UserNotificationPreferencesResponseSchema, | |
| ) |
🧰 Tools
🪛 Ruff (0.11.9)
10-10: ddpui.schemas.notifications_api_schemas.NotificationFiltersSchema imported but unused
Remove unused import
(F401)
13-13: ddpui.schemas.notifications_api_schemas.NotificationResponseSchema imported but unused
Remove unused import
(F401)
14-14: ddpui.schemas.notifications_api_schemas.UrgentNotificationSchema imported but unused
Remove unused import
(F401)
🤖 Prompt for AI Agents
In ddpui/api/notifications_api.py between lines 10 and 14, some imports are
unused according to static analysis. Review the listed imports and remove any
that are not referenced anywhere in the file to clean up the code and avoid
unnecessary dependencies.
| user_preference, created = UserPreferences.objects.get_or_create(orguser=recipient) | ||
|
|
||
| # Get or create notification preferences | ||
| notification_preferences, _ = UserNotificationPreferences.objects.get_or_create(user=recipient) | ||
|
|
||
| # Check if user is subscribed to this notification category | ||
| if not notification_preferences.is_subscribed_to_category(notification.category): | ||
| # Skip this recipient as they're not subscribed to this category | ||
| return None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the redundant UserPreferences creation.
The function creates both UserPreferences (line 72) and UserNotificationPreferences (line 75) objects, but only UserNotificationPreferences is used in the rest of the function. This creates unnecessary database operations and potential confusion.
Apply this diff to remove the redundant creation:
recipient = OrgUser.objects.get(id=recipient_id)
-user_preference, created = UserPreferences.objects.get_or_create(orguser=recipient)
# Get or create notification preferences
notification_preferences, _ = UserNotificationPreferences.objects.get_or_create(user=recipient)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| user_preference, created = UserPreferences.objects.get_or_create(orguser=recipient) | |
| # Get or create notification preferences | |
| notification_preferences, _ = UserNotificationPreferences.objects.get_or_create(user=recipient) | |
| # Check if user is subscribed to this notification category | |
| if not notification_preferences.is_subscribed_to_category(notification.category): | |
| # Skip this recipient as they're not subscribed to this category | |
| return None | |
| recipient = OrgUser.objects.get(id=recipient_id) | |
| # Get or create notification preferences | |
| notification_preferences, _ = UserNotificationPreferences.objects.get_or_create(user=recipient) | |
| # Check if user is subscribed to this notification category | |
| if not notification_preferences.is_subscribed_to_category(notification.category): | |
| # Skip this recipient as they're not subscribed to this category | |
| return None |
🤖 Prompt for AI Agents
In ddpui/core/notifications_service.py around lines 72 to 81, the code creates a
UserPreferences object that is never used later in the function, causing
unnecessary database operations. Remove the line that creates or gets the
UserPreferences object (line 72) and its associated variable assignment, keeping
only the creation and usage of UserNotificationPreferences. This will simplify
the code and avoid redundant database calls.
| def get_urgent_notifications_for_user( | ||
| orguser: OrgUser, | ||
| ) -> Tuple[Optional[None], Dict[str, Any]]: | ||
| """ | ||
| Get all unread urgent notifications for a user (for notification bar) | ||
| """ | ||
| urgent_notifications = ( | ||
| NotificationRecipient.objects.filter( | ||
| recipient=orguser, read_status=False, notification__urgent=True | ||
| ) | ||
| .select_related("notification") | ||
| .order_by("-notification__timestamp") | ||
| ) | ||
|
|
||
| notification_list = [ | ||
| { | ||
| "id": notification_recipient.notification.id, | ||
| "author": notification_recipient.notification.author, | ||
| "message": notification_recipient.notification.message, | ||
| "timestamp": notification_recipient.notification.timestamp, | ||
| "category": notification_recipient.notification.category, | ||
| } | ||
| for notification_recipient in urgent_notifications | ||
| ] | ||
|
|
||
| return None, {"success": True, "res": notification_list} | ||
|
|
There was a problem hiding this comment.
Critical: Function is redefined later in the file.
This function get_urgent_notifications_for_user is redefined at line 620 with a different return type signature. The first version returns a tuple, while the second returns a list directly.
Remove this definition and keep only the one at line 620, or rename this function to avoid conflicts:
-def get_urgent_notifications_for_user(
+def get_urgent_notifications_for_user_with_response(
orguser: OrgUser,
) -> Tuple[Optional[None], Dict[str, Any]]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_urgent_notifications_for_user( | |
| orguser: OrgUser, | |
| ) -> Tuple[Optional[None], Dict[str, Any]]: | |
| """ | |
| Get all unread urgent notifications for a user (for notification bar) | |
| """ | |
| urgent_notifications = ( | |
| NotificationRecipient.objects.filter( | |
| recipient=orguser, read_status=False, notification__urgent=True | |
| ) | |
| .select_related("notification") | |
| .order_by("-notification__timestamp") | |
| ) | |
| notification_list = [ | |
| { | |
| "id": notification_recipient.notification.id, | |
| "author": notification_recipient.notification.author, | |
| "message": notification_recipient.notification.message, | |
| "timestamp": notification_recipient.notification.timestamp, | |
| "category": notification_recipient.notification.category, | |
| } | |
| for notification_recipient in urgent_notifications | |
| ] | |
| return None, {"success": True, "res": notification_list} | |
| def get_urgent_notifications_for_user_with_response( | |
| orguser: OrgUser, | |
| ) -> Tuple[Optional[None], Dict[str, Any]]: | |
| """ | |
| Get all unread urgent notifications for a user (for notification bar) | |
| """ | |
| urgent_notifications = ( | |
| NotificationRecipient.objects.filter( | |
| recipient=orguser, read_status=False, notification__urgent=True | |
| ) | |
| .select_related("notification") | |
| .order_by("-notification__timestamp") | |
| ) | |
| notification_list = [ | |
| { | |
| "id": notification_recipient.notification.id, | |
| "author": notification_recipient.notification.author, | |
| "message": notification_recipient.notification.message, | |
| "timestamp": notification_recipient.notification.timestamp, | |
| "category": notification_recipient.notification.category, | |
| } | |
| for notification_recipient in urgent_notifications | |
| ] | |
| return None, {"success": True, "res": notification_list} |
🤖 Prompt for AI Agents
In ddpui/core/notifications_service.py between lines 482 and 508, the function
get_urgent_notifications_for_user is redefined later at line 620 with a
different return type. To fix this, remove the entire function definition from
lines 482 to 508 to avoid conflicts and keep only the later version at line 620,
or alternatively rename this earlier function to a unique name if both versions
are needed.
| def get_user_notification_preferences( | ||
| orguser: OrgUser, | ||
| ) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: | ||
| """ | ||
| Get user notification preferences including category subscriptions | ||
| """ | ||
| try: | ||
| user_preference, created = UserPreferences.objects.get_or_create(orguser=orguser) | ||
| return None, {"success": True, "res": user_preference.to_json()} | ||
| except Exception as e: | ||
| return f"Error retrieving user preferences: {str(e)}", None | ||
|
|
||
|
|
||
| def update_user_notification_preferences( | ||
| orguser: OrgUser, preferences_data: dict | ||
| ) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: | ||
| """ | ||
| Update user notification preferences including category subscriptions | ||
| """ | ||
| try: | ||
| user_preference, created = UserPreferences.objects.get_or_create(orguser=orguser) | ||
|
|
||
| # Update preferences based on provided data | ||
| for key, value in preferences_data.items(): | ||
| if hasattr(user_preference, key): | ||
| setattr(user_preference, key, value) | ||
|
|
||
| user_preference.save() | ||
| return None, {"success": True, "message": "Preferences updated successfully"} | ||
| except Exception as e: | ||
| return f"Error updating user preferences: {str(e)}", None | ||
|
|
There was a problem hiding this comment.
Critical: Remove duplicate function definitions.
These functions (get_user_notification_preferences and update_user_notification_preferences) are redefined later in the file (lines 534-570) with different implementations using UserNotificationPreferences instead of UserPreferences. This will cause the first definitions to be completely overwritten.
Either remove these functions or rename them to distinguish between the two preference models:
-def get_user_notification_preferences(
+def get_user_preferences_legacy(
orguser: OrgUser,
) -> Tuple[Optional[str], Optional[Dict[str, Any]]]:
"""
- Get user notification preferences including category subscriptions
+ Get legacy user preferences (deprecated - use get_user_notification_preferences instead)
"""
-def update_user_notification_preferences(
+def update_user_preferences_legacy(
orguser: OrgUser, preferences_data: dict
) -> Tuple[Optional[str], Optional[Dict[str, Any]]]:
"""
- Update user notification preferences including category subscriptions
+ Update legacy user preferences (deprecated - use update_user_notification_preferences instead)
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_user_notification_preferences( | |
| orguser: OrgUser, | |
| ) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: | |
| """ | |
| Get user notification preferences including category subscriptions | |
| """ | |
| try: | |
| user_preference, created = UserPreferences.objects.get_or_create(orguser=orguser) | |
| return None, {"success": True, "res": user_preference.to_json()} | |
| except Exception as e: | |
| return f"Error retrieving user preferences: {str(e)}", None | |
| def update_user_notification_preferences( | |
| orguser: OrgUser, preferences_data: dict | |
| ) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: | |
| """ | |
| Update user notification preferences including category subscriptions | |
| """ | |
| try: | |
| user_preference, created = UserPreferences.objects.get_or_create(orguser=orguser) | |
| # Update preferences based on provided data | |
| for key, value in preferences_data.items(): | |
| if hasattr(user_preference, key): | |
| setattr(user_preference, key, value) | |
| user_preference.save() | |
| return None, {"success": True, "message": "Preferences updated successfully"} | |
| except Exception as e: | |
| return f"Error updating user preferences: {str(e)}", None | |
| def get_user_preferences_legacy( | |
| orguser: OrgUser, | |
| ) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: | |
| """ | |
| Get legacy user preferences (deprecated - use get_user_notification_preferences instead) | |
| """ | |
| try: | |
| user_preference, created = UserPreferences.objects.get_or_create(orguser=orguser) | |
| return None, {"success": True, "res": user_preference.to_json()} | |
| except Exception as e: | |
| return f"Error retrieving user preferences: {str(e)}", None | |
| def update_user_preferences_legacy( | |
| orguser: OrgUser, preferences_data: dict | |
| ) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: | |
| """ | |
| Update legacy user preferences (deprecated - use update_user_notification_preferences instead) | |
| """ | |
| try: | |
| user_preference, created = UserPreferences.objects.get_or_create(orguser=orguser) | |
| # Update preferences based on provided data | |
| for key, value in preferences_data.items(): | |
| if hasattr(user_preference, key): | |
| setattr(user_preference, key, value) | |
| user_preference.save() | |
| return None, {"success": True, "message": "Preferences updated successfully"} | |
| except Exception as e: | |
| return f"Error updating user preferences: {str(e)}", None |
🤖 Prompt for AI Agents
In ddpui/core/notifications_service.py between lines 400 and 431, there are
duplicate definitions of get_user_notification_preferences and
update_user_notification_preferences that conflict with later versions using a
different model. To fix this, remove these earlier duplicate function
definitions or rename them to clearly distinguish from the later versions that
use UserNotificationPreferences instead of UserPreferences, ensuring no function
is overwritten and the correct model is used consistently.
| # Check if user has email notifications enabled | ||
| if notification_preferences.email_notifications_enabled: | ||
| try: | ||
| send_text_message( | ||
| user_preference.orguser.user.email, | ||
| recipient.user.email, | ||
| notification.email_subject, | ||
| notification.message, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the email notification field migration.
The email notification check now uses notification_preferences.email_notifications_enabled instead of the old user_preference.enable_email_notifications. Ensure that all email notification settings have been properly migrated from the old model to the new one.
Run the following script to check if the old email preference field is still being used elsewhere:
🏁 Script executed:
#!/bin/bash
# Description: Check for usage of the old email notification field
# Search for references to the old field name
rg -A 3 "enable_email_notifications" --type py
# Check if UserPreferences model is still being used for email settings
ast-grep --pattern 'UserPreferences.objects.$_($$$).enable_email_notifications'Length of output: 9181
Complete migration of email notification preferences to the new model
Several parts of the codebase still reference the old UserPreferences.enable_email_notifications instead of the new notification_preferences.email_notifications_enabled, which will lead to inconsistent behavior. Please update the following locations:
- ddpui/api/notifications_api.py
• mappingpayload.enable_email_notifications→payload.email_notifications_enabled(and update the request/response schema). - ddpui/api/user_preferences_api.py
• replace all getters/setters and JSON fields forenable_email_notificationswith the new preference model’semail_notifications_enabled. - ddpui/models/userpreferences.py
• deprecate/remove theenable_email_notificationsfield and migrate existing data intoUserNotificationPreferences. - ddpui/core/orguserfunctions.py
• change default creation of user preferences fromUserPreferences.enable_email_notifications=Trueto creating aUserNotificationPreferencesrecord withemail_notifications_enabled=True. - ddpui/celeryworkers/moretasks.py
• replace checks onuser_preference.enable_email_notificationswithnotification_preferences.email_notifications_enabled. - Management commands (
toggle_user_preferences) and related tests (test_user_preferences_api, etc.)
• update flags, command-line arguments, and assertions to use the new field. - Migrations & schemas
• remove the old field from model definitions and API schemas once all code references have been updated.
Once these changes are in place, you can safely remove UserPreferences.enable_email_notifications and its associated tests/migrations.
| ### Updating User Preferences | ||
|
|
||
| ```python | ||
| from ddpui.core.notifications_service import update_user_notification_preferences | ||
|
|
||
| update_user_notification_preferences( | ||
| orguser=user, | ||
| email_notifications_enabled=True, | ||
| job_failure_notifications=False, | ||
| incident_notifications=True | ||
| ) | ||
| ``` |
There was a problem hiding this comment.
Fix the incorrect function parameters in the example.
The update_user_notification_preferences example shows individual boolean parameters (job_failure_notifications, incident_notifications), but according to the implementation, these should be passed as separate parameters, not as shown.
Update the example to match the actual function signature:
update_user_notification_preferences(
orguser=user,
email_notifications_enabled=True,
- job_failure_notifications=False,
- incident_notifications=True
+ job_failure_notifications=False,
+ incident_notifications=True,
+ schema_change_notifications=None, # Optional - only update if provided
+ late_run_notifications=None,
+ dbt_test_failure_notifications=None,
+ system_notifications=None
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Updating User Preferences | |
| ```python | |
| from ddpui.core.notifications_service import update_user_notification_preferences | |
| update_user_notification_preferences( | |
| orguser=user, | |
| email_notifications_enabled=True, | |
| job_failure_notifications=False, | |
| incident_notifications=True | |
| ) | |
| ``` | |
| from ddpui.core.notifications_service import update_user_notification_preferences | |
| update_user_notification_preferences( | |
| orguser=user, | |
| email_notifications_enabled=True, | |
| job_failure_notifications=False, | |
| incident_notifications=True, | |
| schema_change_notifications=None, # Optional - only update if provided | |
| late_run_notifications=None, | |
| dbt_test_failure_notifications=None, | |
| system_notifications=None | |
| ) |
🤖 Prompt for AI Agents
In NOTIFICATION_ENHANCEMENTS.md around lines 185 to 196, the example usage of
update_user_notification_preferences incorrectly passes
job_failure_notifications and incident_notifications as individual boolean
parameters. To fix this, revise the example to pass these notification
preferences according to the actual function signature, likely as a dictionary
or grouped parameter, ensuring the call matches the implemented parameter
structure.
| urgent: Optional[bool] = False | ||
| scheduled_time: Optional[datetime] = None | ||
| recipients: List[int] # list of orguser ids | ||
| category: Optional[str] = "system" |
There was a problem hiding this comment.
Fix type inconsistency with category field.
The category field should use the NotificationCategoryEnum type for consistency with CreateNotificationPayloadSchema. Using different types (string vs enum) could cause integration issues.
Apply this diff to fix the type inconsistency:
- category: Optional[str] = "system"
+ category: Optional[NotificationCategoryEnum] = NotificationCategoryEnum.SYSTEM📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| category: Optional[str] = "system" | |
| category: Optional[NotificationCategoryEnum] = NotificationCategoryEnum.SYSTEM |
🤖 Prompt for AI Agents
In ddpui/schemas/notifications_api_schemas.py at line 72, the category field is
currently typed as Optional[str] with a default of "system". To fix the type
inconsistency with CreateNotificationPayloadSchema, change the type of category
to Optional[NotificationCategoryEnum] and set the default value to
NotificationCategoryEnum.SYSTEM or the appropriate enum member representing
"system". This ensures consistent use of the enum type across schemas.
|
the coderabbit review above was really helpful for finding copilot's mistakes |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores