[forge] notifications v2 #1100
[forge] notifications v2 #1100
Conversation
WalkthroughAdds a notification category enum/field and per-category subscription flags; propagates category through schemas, service recipient filtering, creation, history/user retrieval, CLI, Celery/webhook calls, tests, and migrations; adds urgent notifications endpoint and doc updates. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(230,245,255)
participant Client
participant API as notifications_api
participant Service as notifications_service
participant DB as Database
end
Client->>API: POST /notifications (message, recipients, category?)
API->>Service: create_notification(payload with category)
Service->>DB: get_recipients(sent_to, org_slug, user_email, manager_or_above, category)
Note right of DB: recipient filtering applies UserPreferences.is_subscribed_to_category
DB-->>Service: filtered recipient user IDs
Service->>DB: insert Notification (includes category)
Service-->>API: result (notification data with category)
API-->>Client: 200 OK
sequenceDiagram
participant Client
participant API as notifications_api
participant Service as notifications_service
participant DB as Database
Client->>API: GET /notifications/urgent
API->>Service: get_urgent_notifications(orguser)
Service->>DB: query unread urgent notifications (may filter by category)
DB-->>Service: notifications list
Service-->>API: list (each item includes category)
API-->>Client: 200 OK
sequenceDiagram
participant Client
participant API as notifications_api
participant Service as notifications_service
participant DB as Database
Client->>API: GET /notifications?category={category}&page&limit
API->>Service: fetch_user_notifications_v1(orguser, page, limit, read_status, category)
Service->>DB: fetch notifications filtered by category
DB-->>Service: paged results
Service-->>API: data (category echoed)
API-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview 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: 2
🧹 Nitpick comments (7)
agent_forge.yaml (2)
40-41: Prefer block scalars over quoted strings with escaped new-linesThe
descriptionandsystem_promptforspec_ogare supplied as one huge quoted string littered with\n.
This is hard to read, diff, and maintain. Use the same|-block-scalar style you already use for the other agents.- description: "An expert agent that helps translate high-level feature requests, \n… - system_prompt: "You are Forge, an expert at creating detailed, actionable specifications …\n" + description: |- + An expert agent that helps translate high-level feature requests, + enhancement ideas, and bug reports into detailed, actionable + specifications. This agent excels at breaking down complex + requirements, identifying edge cases, defining acceptance criteria, + and creating comprehensive technical specifications that development + teams can implement effectively. + system_prompt: |- + You are Forge, an expert at creating detailed, actionable + specifications for software features, enhancements, and fixes. + …This keeps the YAML readable while remaining functionally identical.
9-32: Minor naming confusion: system prompt says “You are Forge” while agent id iscode_ogPurely cosmetic, but calling the agent “Forge” in the prompt while its public id is
code_ogcan confuse users reading logs or traces. Consider replacing the self-reference with “You are Code-OG” (or similar) for clarity.ddpui/models/notifications.py (1)
25-30: Consider increasing max_length for the category field.The current
max_length=20may be insufficient. The longest category value "dbt_test_failure" is 16 characters, leaving only 4 characters of buffer. Consider increasing to 25 or 30 for future-proofing.- category = models.CharField( - max_length=20, + category = models.CharField( + max_length=30,ddpui/api/user_preferences_api.py (1)
52-69: Consider refactoring verbose update logic.The current approach with individual null checks for each subscription field is correct but verbose. Consider using a loop or helper function to reduce code duplication.
- if payload.subscribe_incident_notifications is not None: - user_preferences.subscribe_incident_notifications = payload.subscribe_incident_notifications - if payload.subscribe_schema_change_notifications is not None: - user_preferences.subscribe_schema_change_notifications = ( - payload.subscribe_schema_change_notifications - ) - if payload.subscribe_job_failure_notifications is not None: - user_preferences.subscribe_job_failure_notifications = ( - payload.subscribe_job_failure_notifications - ) - if payload.subscribe_late_runs_notifications is not None: - user_preferences.subscribe_late_runs_notifications = ( - payload.subscribe_late_runs_notifications - ) - if payload.subscribe_dbt_test_failure_notifications is not None: - user_preferences.subscribe_dbt_test_failure_notifications = ( - payload.subscribe_dbt_test_failure_notifications - ) + # Update subscription preferences + subscription_fields = [ + 'subscribe_incident_notifications', + 'subscribe_schema_change_notifications', + 'subscribe_job_failure_notifications', + 'subscribe_late_runs_notifications', + 'subscribe_dbt_test_failure_notifications' + ] + + for field in subscription_fields: + value = getattr(payload, field, None) + if value is not None: + setattr(user_preferences, field, value)ddpui/models/userpreferences.py (1)
47-56: Consider using constants for category mapping.The
is_subscribed_to_categorymethod is well-designed with a safe default, but consider using constants from theNotificationCategoryenum instead of string literals to ensure consistency and prevent typos.+from ddpui.models.notifications import NotificationCategory def is_subscribed_to_category(self, category: str) -> bool: """Check if user is subscribed to a specific notification category""" category_mapping = { - "incident": self.subscribe_incident_notifications, - "schema_change": self.subscribe_schema_change_notifications, - "job_failure": self.subscribe_job_failure_notifications, - "late_runs": self.subscribe_late_runs_notifications, - "dbt_test_failure": self.subscribe_dbt_test_failure_notifications, + NotificationCategory.INCIDENT: self.subscribe_incident_notifications, + NotificationCategory.SCHEMA_CHANGE: self.subscribe_schema_change_notifications, + NotificationCategory.JOB_FAILURE: self.subscribe_job_failure_notifications, + NotificationCategory.LATE_RUNS: self.subscribe_late_runs_notifications, + NotificationCategory.DBT_TEST_FAILURE: self.subscribe_dbt_test_failure_notifications, } return category_mapping.get(category, True)NOTIFICATIONS_V2_DOCUMENTATION.md (1)
93-95: Add language specifiers to fenced code blocks.The static analysis tool correctly identified missing language specifiers in code blocks. This improves syntax highlighting and readability.
Apply these changes:
Line 93:
-``` +```bash GET /notifications/history?category=incident&page=1&limit=10Line 102: ```diff -``` +```bash GET /notifications/v1?category=job_failure&read_status=0Line 145: ```diff -``` +```bash GET /notifications/categories/job_failure?page=1&limit=10Also applies to: 102-104, 145-147 </blockquote></details> <details> <summary>ddpui/core/notifications_service.py (1)</summary><blockquote> `72-102`: **Consider differentiating return values for skipped notifications.** The function returns `None` both when skipping notifications (line 74) and on success (line 102). This makes it difficult for callers to distinguish between the two cases. Consider returning a specific indicator when skipping: ```diff # Check if user is subscribed to this notification category if not user_preference.is_subscribed_to_category(notification.category): - return None # Skip sending notification if user is not subscribed + return {"skipped": True, "reason": "User not subscribed to category"}Then update the caller in
create_notificationto handle this case appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
NOTIFICATIONS_V2_DOCUMENTATION.md(1 hunks)agent_forge.yaml(1 hunks)ddpui/api/notifications_api.py(6 hunks)ddpui/api/user_preferences_api.py(4 hunks)ddpui/core/notifications_service.py(9 hunks)ddpui/models/notifications.py(2 hunks)ddpui/models/userpreferences.py(2 hunks)ddpui/schemas/notifications_api_schemas.py(3 hunks)ddpui/schemas/userpreferences_schema.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ddpui/models/notifications.py (1)
ddpui/models/org_user.py (2)
choices(52-54)OrgUser(66-78)
ddpui/api/user_preferences_api.py (1)
ddpui/tests/api_tests/test_user_preferences_api.py (1)
user_preferences(60-65)
🪛 markdownlint-cli2 (0.17.2)
NOTIFICATIONS_V2_DOCUMENTATION.md
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
ddpui/models/notifications.py (2)
5-12: Well-structured notification categories.The
NotificationCategoryenum is properly implemented using Django'sTextChoicespattern with clear value-label pairs. The categories cover the main notification types effectively.
31-36: Appropriate implementation for urgent notification dismissals.The
dismissed_byManyToManyField correctly models the relationship between users and dismissed notifications. The field configuration withblank=Trueand descriptiverelated_namefollows Django best practices.ddpui/schemas/userpreferences_schema.py (2)
10-14: Good default subscription behavior.The opt-out model (defaulting to
True) ensures users receive notifications by default while allowing them to unsubscribe from specific categories. This is a user-friendly approach that prevents missing important notifications.
22-26: Appropriate handling for partial updates.Using
Noneas defaults in the update schema correctly supports partial updates where users can modify only specific subscription preferences without affecting others.ddpui/api/user_preferences_api.py (1)
120-120: Appropriate category assignment for LLM analysis requests.Adding
category="incident"to the LLM analysis feature request notification is appropriate, as these requests can be considered incidents requiring account manager attention.ddpui/models/userpreferences.py (1)
16-30: Well-structured subscription fields.The subscription fields are properly defined with appropriate defaults, clear help text, and consistent naming. The opt-out model (default=True) ensures users don't miss important notifications initially.
ddpui/schemas/notifications_api_schemas.py (3)
74-77: Simple and appropriate schema for dismissing notifications.The
DismissUrgentNotificationSchemacorrectly requires only the notification ID for dismissal operations. The simplicity is appropriate for this specific action.
80-87: Consistent subscription schema design.The
CategorySubscriptionSchemaproperly mirrors the user preferences fields and allows partial updates withOptional[bool]fields defaulting toNone.
19-29: All NotificationCategoryEnum values match the model’s NotificationCategory choices. No changes needed; the schema and model are fully synchronized.ddpui/api/notifications_api.py (2)
72-98: Well-implemented category filtering support.The addition of the optional
categoryparameter maintains backward compatibility while enabling category-based filtering.
344-346: Verified CategorySubscriptionSchema enforces allowed subscription flags only
CategorySubscriptionSchema extends Pydantic’s BaseModel (aliased asSchema), so any unknown keys passed in will be ignored and omitted frompayload.dict(exclude_none=True). No additional validation is required to prevent invalid fields. If you’d prefer to error on unexpected keys, you can add:class CategorySubscriptionSchema(Schema): # existing subscription flags… model_config = {"extra": "forbid"}NOTIFICATIONS_V2_DOCUMENTATION.md (1)
1-303: Excellent documentation for the notification v2 enhancements.The documentation is comprehensive, well-structured, and provides clear examples for all the new features. The migration notes and backward compatibility section are particularly helpful.
ddpui/core/notifications_service.py (2)
283-293: Excellent implementation of dynamic filter building.The use of
filter_kwargsdictionary makes the code clean and maintainable while properly handling optional parameters.
463-475: UserPreferences.to_json() confirmedThe
to_json()method is defined on theUserPreferencesmodel (ddpui/models/userpreferences.py), so the call inupdate_category_subscriptionsis safe.
| notification_data = { | ||
| "author": payload.author, | ||
| "message": payload.message, | ||
| "email_subject": payload.message, # Use message as email subject if not provided | ||
| "urgent": payload.urgent, | ||
| "scheduled_time": payload.scheduled_time, | ||
| "recipients": recipients, | ||
| "category": payload.category, | ||
| } |
There was a problem hiding this comment.
Fix email_subject assignment logic to match comment.
The comment states "Use message as email subject if not provided", but the code unconditionally assigns payload.message to email_subject. This overrides any email_subject provided in the payload.
Apply this diff to fix the logic:
notification_data = {
"author": payload.author,
"message": payload.message,
- "email_subject": payload.message, # Use message as email subject if not provided
+ "email_subject": payload.email_subject or payload.message, # Use message as email subject if not provided
"urgent": payload.urgent,
"scheduled_time": payload.scheduled_time,
"recipients": recipients,
"category": payload.category,
}📝 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.
| notification_data = { | |
| "author": payload.author, | |
| "message": payload.message, | |
| "email_subject": payload.message, # Use message as email subject if not provided | |
| "urgent": payload.urgent, | |
| "scheduled_time": payload.scheduled_time, | |
| "recipients": recipients, | |
| "category": payload.category, | |
| } | |
| notification_data = { | |
| "author": payload.author, | |
| "message": payload.message, | |
| "email_subject": payload.email_subject or payload.message, # Use message as email subject if not provided | |
| "urgent": payload.urgent, | |
| "scheduled_time": payload.scheduled_time, | |
| "recipients": recipients, | |
| "category": payload.category, | |
| } |
🤖 Prompt for AI Agents
In ddpui/api/notifications_api.py around lines 53 to 61, the email_subject field
is always set to payload.message, ignoring any email_subject provided in the
payload. Update the assignment to set email_subject to payload.email_subject if
it exists; otherwise, fallback to payload.message as the email subject, matching
the comment's intent.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
The schema CreateNotificationPayloadSchema needs to be update to put email_subject.
|
wow coderabbit had much less to say about this PR as compared to the copilot PR |
@Ishankoradia, @nairabhishek73, @himanshudube97
This is how a user can subscribe to category of notifications, from current user preference form.
|
filter option.
|
|
Some api's are not helpful, as their work can be done by other api's, so need to do cleanup at the end. |
|
nice progress we will release this weekend |
|
Need to update the logic of get_recipients based on whether the user is subscribed or not. |
|
@Ishankoradia, We have to add category code in create_notification.py. |
… API and service as this can be done from user preference.
|
@Ishankoradia, I have made changes according to the above comments. |
ddpui/core/notifications_service.py
Outdated
|
|
||
| # Map category string to the correct field name | ||
| category_field_mapping = { | ||
| "incident": "subscribe_incident_notifications", |
There was a problem hiding this comment.
Can we use NotificationCategory to get the keys or the categories ?
So we dont have to change things at multiple places if we decide to the change.
If TextChoices is giving trouble in doing this, you can convert the class to an Enum
There was a problem hiding this comment.
Since we want that 'subscribe_' field , we can generate this on the basis of category from notification category.
So we need to write one function to do this change that will change like this subscribe_(category value)_notifications.
| ) | ||
|
|
||
| if error: | ||
| print(f"Error in getting recipients: {error}") |
There was a problem hiding this comment.
This is good.
But how are we setting the category of notifications on various job failures, schema change etc ? I dont see any changes related to that.
Where we create a notification in the app, we will need to add a appropriate category to the logic.
There was a problem hiding this comment.
I didn't get your question.
When we create a notification, we can add a category by '-- category' in the app.
There was a problem hiding this comment.
We need to trigger notifications for
SCHEMA_CHANGE, JOB_FAILURE, DBT_TEST_FAILURE
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (4)
ddpui/schemas/notifications_api_schemas.py (1)
32-47: CreateNotificationPayloadSchema is missing email_subjectAPI logic and prior review expect email_subject. Add it as optional to avoid silently overwriting subjects and to align with the endpoint behavior.
Apply this diff:
class CreateNotificationPayloadSchema(BaseModel): """Schema for creating a new notification api.""" author: str message: str + email_subject: Optional[str] = None sent_to: SentToEnum urgent: Optional[bool] = False scheduled_time: Optional[datetime] = None user_email: Optional[str] = None manager_or_above: Optional[bool] = False org_slug: Optional[str] = None category: Optional[NotificationCategoryEnum] = Noneddpui/management/commands/create_notification.py (2)
29-33: Make --audience required to prevent runtime ValueErrorSentToEnum(options["audience"]) will raise if audience isn’t provided. Make it required at parse time.
Proposed change (outside the selected diff range):
parser.add_argument( "--audience", type=str, choices=[sent_to.value for sent_to in SentToEnum], required=True, help="Target audience (all_users | all_org_users | single_user)", )
93-100: Ensurecategoryis passed toget_recipientsin the CLI commandThe
notifications_service.get_recipientsmethod signature includes an optionalcategoryparameter (defaultNone), and the API endpoint already passespayload.category(ddpui/api/notifications_api.py:46–52). However, the management command in ddpui/management/commands/create_notification.py currently omits this argument, so per-category subscriptions won’t be honored when sending via the CLI.Please update the call in the
handle()method:• File: ddpui/management/commands/create_notification.py
• Lines: ~94Diff:
- error, recipients = notifications_service.get_recipients( - SentToEnum(options["audience"]), - options.get("org"), - options.get("email"), - options.get("manager_or_above", False), - ) + error, recipients = notifications_service.get_recipients( + SentToEnum(options["audience"]), + options.get("org"), + options.get("email"), + options.get("manager_or_above", False), + options.get("category"), + )This ensures that CLI-triggered notifications respect category-based subscriptions just like the API path.
ddpui/migrations/0128_allow_null_category.py (1)
1-31: Update NotificationDataSchema default to NoneThe new migration (0128) makes
categorynullable with a default ofNone, but inNotificationDataSchemathe default is still"incident". Please update the schema to match:• File:
ddpui/schemas/notifications_api_schemas.py(around line 72)- category: Optional[str] = "incident" + category: Optional[str] = NoneNo
maintenancechoice was found in any migrations.
♻️ Duplicate comments (1)
ddpui/api/notifications_api.py (1)
57-65: Fix email_subject assignment to honor client-provided valueCurrently, email_subject is always set to message, ignoring a provided subject. This regresses UX and contradicts prior review.
Apply this diff:
notification_data = { "author": payload.author, "message": payload.message, - "email_subject": payload.message, # Use message as email subject if not provided + "email_subject": getattr(payload, "email_subject", None) or payload.message, # Use message as email subject if not provided "urgent": payload.urgent, "scheduled_time": payload.scheduled_time, "recipients": recipients, "category": payload.category, }Note: This depends on adding email_subject to CreateNotificationPayloadSchema (see schema review).
🧹 Nitpick comments (14)
.gitignore (2)
8-9: Nit: Anchor to repo root to avoid ignoring nested dbt-venv directories.If the intent is to ignore only the top-level virtualenv, anchor the pattern.
Apply this diff:
-dbt-venv/ +/dbt-venv/
8-9: Optional: Co-locate with “Environments” section and consider dbt-specific ignores.For consistency, consider moving this entry under the existing “# Environments” block. If dbt artifacts are present in this repo, you may also want to ignore common dbt outputs (only if applicable to your structure): dbt_packages/, logs/, target/. Note target/ already exists in the PyBuilder section.
ddpui/models/notifications.py (1)
26-33: Index the category field to speed up category-based filteringAPIs now filter by category frequently. Adding a DB index will improve lookup performance with minimal write overhead.
Apply this diff:
category = models.CharField( max_length=20, choices=NotificationCategory.choices, null=True, blank=True, default=None, help_text="Category of the notification", + db_index=True, )ddpui/management/commands/create_notification.py (2)
56-56: Leverage argparse choices for category and drop manual validationUse the enum values to validate at parse time and simplify handle().
Apply this diff:
- parser.add_argument("--category", type=str, help="Category of the notification") + parser.add_argument( + "--category", + type=str, + choices=list(NotificationCategory.values), + help=f"Category of the notification (one of: {', '.join(NotificationCategory.values)})", + )
105-112: Remove redundant category validation (handled by argparse choices)Now that argparse enforces allowed values, this block is unnecessary.
Apply this diff:
- # Validate category - category = options.get("category") - if category and category not in NotificationCategory.values: - print( - f"Invalid category: '{category}'. Must be one of: {', '.join(NotificationCategory.values)}" - ) - sys.exit(1)ddpui/api/notifications_api.py (4)
5-10: Remove unused import flagged by Ruff (F401)CategorySubscriptionSchema is imported but unused.
Apply this diff:
from ddpui.schemas.notifications_api_schemas import ( CreateNotificationPayloadSchema, UpdateReadStatusSchema, UpdateReadStatusSchemav1, - CategorySubscriptionSchema, )
16-43: Docstring nit: avoid implying default category is INCIDENTProduct guidance is to not default to INCIDENT. Update the docstring accordingly.
Apply this diff:
- - category: Notification category (optional, defaults to INCIDENT) + - category: Notification category (optional)
251-274: Docstring nit: “dismissed” may be misleading; we track read statusSince dismissed_by was removed, say “not marked as read” instead.
Apply this diff:
- Get urgent notifications that haven't been dismissed by the user. + Get urgent notifications that are not marked as read by the user. @@ - Returns urgent notifications that should be displayed in the prominent - notification bar at the top of the page. + Returns urgent notifications that should be displayed in the prominent + notification bar at the top of the page. @@ - dict: List of urgent notifications that haven't been dismissed + dict: List of urgent notifications not marked as read
276-301: Constrain category path param to the enum for validation at the API layerUse NotificationCategoryEnum to automatically validate incoming category.
Apply these diffs:
-from ddpui.schemas.notifications_api_schemas import ( +from ddpui.schemas.notifications_api_schemas import ( CreateNotificationPayloadSchema, UpdateReadStatusSchema, UpdateReadStatusSchemav1, ) +from ddpui.schemas.notifications_api_schemas import NotificationCategoryEnum-@notification_router.get("/categories/{category}") -def get_notifications_by_category(request, category: str, page: int = 1, limit: int = 10): +@notification_router.get("/categories/{category}") +def get_notifications_by_category( + request, + category: NotificationCategoryEnum, + page: int = 1, + limit: int = 10, +): @@ - error, result = notifications_service.get_notifications_by_category( - orguser, category, page, limit - ) + error, result = notifications_service.get_notifications_by_category( + orguser, category.value, page, limit + )ddpui/migrations/0128_allow_null_category.py (1)
15-29: Consider adding an index if category will be used for frequent filteringIf you adopt db_index=True on the model, generate the corresponding migration to add the index at the DB level.
Would you like me to open a follow-up PR to generate and include an index migration for Notification.category?
ddpui/migrations/0127_remove_notification_dismissed_by_and_more.py (2)
19-31: Future-proof max_length for category (optional).max_length=20 fits current choices but is tight if more verbose categories are added later. Consider 32 to avoid another schema change.
- max_length=20, + max_length=32,
16-33: Collapse duplicate migrations and consider indexingcategoryBoth 0127 and 0128 apply an identical
AlterFieldonnotification.category(blank/null/default=None with the same choices, help_text, and max_length). To avoid unnecessary migration churn:
- Merge the
AlterFieldchanges from 0128 into 0127 and delete 0128.- Optionally, add an index on
categoryif you frequently filter notifications by it.Apply this diff in your consolidated migration to add the index:
field=models.CharField( blank=True, choices=[ ("incident", "Incident"), ("schema_change", "Schema Change"), ("job_failure", "Job Failure"), ("late_runs", "Late Runs"), ("dbt_test_failure", "DBT Test Failure"), ], default=None, help_text="Category of the notification", max_length=20, null=True, + db_index=True, ),ddpui/migrations/0126_notification_category_notification_dismissed_by_and_more.py (2)
11-27: Defaultingcategoryto "incident" will silently misclassify existing rows; prefer nullable with no default (or backfill).0127 changes this to
null=True, blank=True, default=None, which is correct. Rows created between applying 0126 and 0127 will have category="incident" even if unknown. If these migrations ever ran separately, consider a backfill to reset those auto-populated values.Option A (if 0126 hasn’t been applied anywhere): change 0126 to align with 0127 and drop the redundant alter in 0127/0128.
- field=models.CharField( - choices=[ + field=models.CharField( + blank=True, + choices=[ ("incident", "Incident"), ("schema_change", "Schema Change"), ("job_failure", "Job Failure"), ("late_runs", "Late Runs"), ("dbt_test_failure", "DBT Test Failure"), ], - default="incident", + default=None, help_text="Category of the notification", - max_length=20, + max_length=20, + null=True, ),Option B (if 0126 might already be applied): keep 0127 as-is and add a data migration to set unknown categories to NULL where appropriate (only if it’s safe in your data). I can draft a guarded
RunPythonbackfill if you confirm the criteria to distinguish legit "incident" rows from auto-defaulted ones.Do you have any environments where 0126 was applied independently of 0127? If yes, we should plan a backfill.
28-37: Avoid add-then-remove churn fordismissed_by.This field is immediately removed in 0127, causing needless table creation and drop. If this PR hasn’t shipped, remove the
dismissed_byAddField here and the RemoveField in 0127 to keep migration history clean. If it’s already run, proceed as-is but consider squashing before release if your process allows it.- migrations.AddField( - model_name="notification", - name="dismissed_by", - field=models.ManyToManyField( - blank=True, - help_text="Users who have dismissed this urgent notification", - related_name="dismissed_notifications", - to="ddpui.orguser", - ), - ), + # dismissed_by M2M intentionally omitted (feature dropped before release)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.gitignore(1 hunks)ddpui/api/notifications_api.py(9 hunks)ddpui/core/notifications_service.py(11 hunks)ddpui/management/commands/create_notification.py(4 hunks)ddpui/migrations/0126_notification_category_notification_dismissed_by_and_more.py(1 hunks)ddpui/migrations/0127_remove_notification_dismissed_by_and_more.py(1 hunks)ddpui/migrations/0128_allow_null_category.py(1 hunks)ddpui/models/notifications.py(2 hunks)ddpui/schemas/notifications_api_schemas.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/core/notifications_service.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
ddpui/management/commands/create_notification.py (1)
ddpui/models/notifications.py (1)
NotificationCategory(5-13)
ddpui/migrations/0128_allow_null_category.py (1)
ddpui/migrations/0127_remove_notification_dismissed_by_and_more.py (1)
Migration(6-34)
ddpui/migrations/0127_remove_notification_dismissed_by_and_more.py (1)
ddpui/migrations/0128_allow_null_category.py (1)
Migration(6-30)
ddpui/models/notifications.py (5)
ddpui/models/org_user.py (1)
choices(52-54)ddpui/models/tasks.py (1)
choices(47-49)ddpui/models/org.py (2)
choices(16-18)choices(28-30)ddpui/models/dbt_workflow.py (1)
choices(28-30)ddpui/models/llm.py (1)
choices(36-38)
ddpui/api/notifications_api.py (1)
ddpui/schemas/notifications_api_schemas.py (1)
CategorySubscriptionSchema(75-82)
🪛 Ruff (0.12.2)
ddpui/api/notifications_api.py
9-9: ddpui.schemas.notifications_api_schemas.CategorySubscriptionSchema imported but unused
Remove unused import
(F401)
🔇 Additional comments (4)
.gitignore (1)
8-9: LGTM: Ignore dbt-venv is a sensible hygiene addition.Adding dbt-venv/ avoids accidentally committing a dbt-specific virtualenv. No functional impact on the PR’s core changes.
ddpui/models/notifications.py (1)
5-15: NotificationCategory enum and addition of MAINTENANCE look goodThe category taxonomy is clear, extensible, and matches product intent.
ddpui/migrations/0127_remove_notification_dismissed_by_and_more.py (1)
12-15: Confirm removal ofdismissed_byfield and through-tableNo references to
Notification.dismissed_bywere found outside of migrations. Dropping this M2M will delete its through-table and all stored records—please verify that this is intentional and that any necessary data has been exported or is no longer required.ddpui/migrations/0126_notification_category_notification_dismissed_by_and_more.py (1)
38-73: All per-category subscription flags are implemented and referenced consistentlyVerified that the new Boolean fields are declared and used across:
- Schemas (
userpreferences_schema.py,notifications_api_schemas.py)- Model (
models/userpreferences.py+get_subscribed_to_categorymapping)- Service (
core/notifications_service.pycategory_field_mapping)- API handlers (
api/user_preferences_api.py)Approving these changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ddpui/migrations/0126_notification_category_and_more.py (2)
12-30: Index category and avoid a nullable default to reduce ambiguity.
If you plan to filter/sort by category (very likely), add an index. Also, keeping both null=True and blank=True with default=None introduces three possible “unset” states: NULL, empty string, and None at the model level. Consider removing the default to rely on NULL and valid choices only.Apply this minimal diff to add an index and drop the default:
field=models.CharField( blank=True, choices=[ ("incident", "Incident"), ("schema_change", "Schema Change"), ("job_failure", "Job Failure"), ("late_runs", "Late Runs"), ("dbt_test_failure", "DBT Test Failure"), ("maintenance", "Maintenance"), ], - default=None, help_text="Category of the notification", max_length=20, null=True, + db_index=True, ),Optional (if you want to avoid the empty-string state entirely via forms/DRF validation):
- blank=True, + blank=False,If you take the optional route, ensure serializers/forms treat “no category” as None explicitly.
32-65: Subscription flags are enforced in both recipient selection and delivery—confirm default opt-in
Allsubscribe_*_notificationsfields default toTrue, and the code correctly gates recipients by these flags:• In
ddpui/core/notifications_service.py:
– Lines 59–66 definecategory_field_mapping→ maps categories to subscription fields.
– Lines 68–75 filterOrgUserrecipients by the mappedsubscribe_*flag.
– Lines 96–100 (in the scheduled task) re-checkuser_preference.is_subscribed_to_category()before sending.• In
ddpui/models/userpreferences.py:
–is_subscribed_to_category()returns the appropriate flag (defaulting toTrueif unknown).No code changes needed—please:
- Confirm with Product/Comms that default-on for all categories matches requirements (and document it).
- Ensure any user-facing docs or release notes clearly state the opt-in defaults.
Optional refactor (non-blocking): consider a JSONField or dedicated subscription table keyed by category to simplify future additions and reduce migration churn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/migrations/0126_notification_category_and_more.py(1 hunks)
🔇 Additional comments (2)
ddpui/migrations/0126_notification_category_and_more.py (2)
1-9: Migration scaffold and dependency look correct.
Header, import, and dependency chain are standard and consistent with Django 4.2.
17-24: Migration choices align with NotificationCategory enumI’ve verified that the
choiceslist in0126_notification_category_and_more.pyexactly matches the members and labels of theNotificationCategoryTextChoices inddpui/models/notifications.py, and all creation and filtering paths use the same enum values. No further changes needed.
ddpui/core/notifications_service.py
Outdated
| "schema_change": "subscribe_schema_change_notifications", | ||
| "job_failure": "subscribe_job_failure_notifications", | ||
| "late_runs": "subscribe_late_runs_notifications", | ||
| "dbt_test_failure": "subscribe_dbt_test_failure_notifications", |
There was a problem hiding this comment.
We should remove Incident notifications from here. Those are sent by platform admins, when something has crashed on Dalgo and a fix is in works. Users should not be able to subscribe out of it.
Also lets remove Late Run, we dont have a provision to track late runs and send notifications at the moment. We can add this later when we do set that up
| class CategorySubscriptionSchema(Schema): | ||
| """Schema for updating category subscription preferences""" | ||
|
|
||
| subscribe_incident_notifications: Optional[bool] = None |
There was a problem hiding this comment.
We should remove Incident notifications from here. Those are sent by platform admins, when something has crashed on Dalgo and a fix is in works. Users should not be able to subscribe out of it.
Also lets remove Late Run, we dont have a provision to track late runs and send notifications at the moment. We can add this later when we do set that up
| subscribe_schema_change_notifications: Optional[bool] = True | ||
| subscribe_job_failure_notifications: Optional[bool] = True | ||
| subscribe_late_runs_notifications: Optional[bool] = True | ||
| subscribe_dbt_test_failure_notifications: Optional[bool] = True |
There was a problem hiding this comment.
We should remove Incident notifications from here. Those are sent by platform admins, when something has crashed on Dalgo and a fix is in works. Users should not be able to subscribe out of it.
Also lets remove Late Run, we dont have a provision to track late runs and send notifications at the moment. We can add this later when we do set that up
| subscribe_schema_change_notifications: Optional[bool] = None | ||
| subscribe_job_failure_notifications: Optional[bool] = None | ||
| subscribe_late_runs_notifications: Optional[bool] = None | ||
| subscribe_dbt_test_failure_notifications: Optional[bool] = None |
There was a problem hiding this comment.
We should remove Incident notifications from here. Those are sent by platform admins, when something has crashed on Dalgo and a fix is in works. Users should not be able to subscribe out of it.
Also lets remove Late Run, we dont have a provision to track late runs and send notifications at the moment. We can add this later when we do set that up
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ddpui/api/notifications_api.py (1)
57-65: Honor provided email_subject; only fallback to message.The code currently overwrites any provided email_subject with message, contradicting the inline comment and prior feedback.
Apply this diff:
notification_data = { "author": payload.author, "message": payload.message, - "email_subject": payload.message, # Use message as email subject if not provided + "email_subject": getattr(payload, "email_subject", None) or payload.message, # Use provided subject, fallback to message "urgent": payload.urgent, "scheduled_time": payload.scheduled_time, "recipients": recipients, - "category": payload.category, + "category": payload.category, }Run this script to verify the schema includes an optional email_subject field:
#!/bin/bash # Verify CreateNotificationPayloadSchema has `email_subject` and NotificationCategoryEnum exists. echo "Checking CreateNotificationPayloadSchema for email_subject..." rg -n -C2 'class\s+CreateNotificationPayloadSchema\b|email_subject' ddpui/schemas/notifications_api_schemas.py echo echo "Checking for NotificationCategoryEnum definition..." rg -n -C2 'class\s+NotificationCategoryEnum\b' ddpui/schemas/notifications_api_schemas.py
🧹 Nitpick comments (3)
ddpui/api/notifications_api.py (3)
5-10: Remove unused import: CategorySubscriptionSchema (Ruff F401).This symbol isn’t used in this module and is flagged by static analysis. Please remove it.
Apply this diff:
from ddpui.schemas.notifications_api_schemas import ( CreateNotificationPayloadSchema, UpdateReadStatusSchema, UpdateReadStatusSchemav1, - CategorySubscriptionSchema, )
76-78: Type category as an Enum for early validation.Using a concrete enum prevents invalid category values from reaching the service.
Apply this diff:
-def get_notification_history( - request, page: int = 1, limit: int = 10, read_status: int = None, category: str = None -): +def get_notification_history( + request, page: int = 1, limit: int = 10, read_status: int = None, category: "NotificationCategoryEnum | None" = None +):Add this import at the top alongside existing schema imports:
from ddpui.schemas.notifications_api_schemas import NotificationCategoryEnum
135-137: Type category as an Enum in the user notifications endpoint.Align with history endpoint to validate query param at the boundary.
Apply this diff:
-def get_user_notifications_v1( - request, page: int = 1, limit: int = 10, read_status: int = None, category: str = None -): +def get_user_notifications_v1( + request, page: int = 1, limit: int = 10, read_status: int = None, category: "NotificationCategoryEnum | None" = None +): @@ - error, result = notifications_service.fetch_user_notifications_v1( - orguser, page, limit, read_status, category - ) + error, result = notifications_service.fetch_user_notifications_v1( + orguser, page, limit, read_status, category + )Ensure this import exists:
from ddpui.schemas.notifications_api_schemas import NotificationCategoryEnumAlso applies to: 161-161
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.gitignore(1 hunks)ddpui/api/airbyte_api.py(14 hunks)ddpui/api/notifications_api.py(10 hunks)ddpui/core/notifications_service.py(11 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- ddpui/api/airbyte_api.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/core/notifications_service.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
📚 Learning: 2025-08-18T07:13:00.905Z
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
Applied to files:
ddpui/api/notifications_api.py
🧬 Code Graph Analysis (1)
ddpui/api/notifications_api.py (2)
ddpui/schemas/notifications_api_schemas.py (1)
CategorySubscriptionSchema(75-82)ddpui/core/notifications_service.py (5)
get_recipients(20-85)get_notification_history(198-235)fetch_user_notifications_v1(305-354)get_urgent_notifications(449-480)get_notifications_by_category(484-490)
🪛 Ruff (0.12.2)
ddpui/api/notifications_api.py
9-9: ddpui.schemas.notifications_api_schemas.CategorySubscriptionSchema imported but unused
Remove unused import
(F401)
🔇 Additional comments (7)
ddpui/api/notifications_api.py (7)
18-43: Doc claims category defaults to INCIDENT—ensure a concrete default is applied.The docstring says category is optional and defaults to INCIDENT. Verify the default is enforced (schema default or service-level fallback). Otherwise, a None category could be stored or bypass filters unintentionally.
If the schema doesn’t set a default, consider a local fallback before using it:
def post_create_notification(request, payload: CreateNotificationPayloadSchema): @@ - error, recipients = notifications_service.get_recipients( - payload.sent_to, - payload.org_slug, - payload.user_email, - payload.manager_or_above, - payload.category, - ) + category = payload.category or "incident" + error, recipients = notifications_service.get_recipients( + payload.sent_to, + payload.org_slug, + payload.user_email, + payload.manager_or_above, + category, + )
47-52: Confirm maintenance category bypasses preference filtering.Per product requirement, users must always receive maintenance notifications regardless of preferences. Please confirm that passing category="maintenance" to get_recipients results in no preference-based filtering (as implemented via mapping in notifications_service).
112-126: Docs improvement LGTM.Clearer description of recipient retrieval behavior.
172-188: Bulk read/unread docs LGTM.Documentation is clear and consistent with service method.
202-217: Delete scheduled notification docs LGTM.Behavior and constraints are clearly communicated.
227-242: Unread count endpoint docs LGTM.Concise and aligned with UI usage.
251-274: New urgent endpoint: service call and error handling look correct.Endpoint follows established patterns and aligns with service behavior (returns unread urgent notifications).
ddpui/api/notifications_api.py
Outdated
| @notification_router.get("/categories/{category}") | ||
| def get_notifications_by_category(request, category: str, page: int = 1, limit: int = 10): | ||
| """ | ||
| Get notifications for a specific category for the authenticated user. | ||
| Args: | ||
| request: HTTP request object containing orguser authentication data | ||
| category: The notification category to filter by | ||
| page (int, optional): Page number for pagination. Defaults to 1 | ||
| limit (int, optional): Number of notifications per page. Defaults to 10 | ||
| Returns: | ||
| dict: Paginated notifications for the specified category | ||
| Raises: | ||
| HttpError: 400 if retrieval fails | ||
| """ | ||
| orguser: OrgUser = request.orguser | ||
| error, result = notifications_service.get_notifications_by_category( | ||
| orguser, category, page, limit | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Missing return and error handling in get_notifications_by_category.
The endpoint calls the service but neither returns the result nor handles errors. This will return None and break the API contract.
Apply this diff to match the error-handling pattern used elsewhere:
def get_notifications_by_category(request, category: str, page: int = 1, limit: int = 10):
@@
- orguser: OrgUser = request.orguser
- error, result = notifications_service.get_notifications_by_category(
- orguser, category, page, limit
- )
+ orguser: OrgUser = request.orguser
+ error, result = notifications_service.get_notifications_by_category(
+ orguser, category, page, limit
+ )
+ if error is not None:
+ raise HttpError(400, error)
+
+ return resultOptional: Type category as an enum for input validation consistency with other endpoints.
Also consider adding tests for:
- Happy-path pagination for a given category.
- Invalid category (when typed as Enum) returns 422.
- Service error surfaces as HTTP 400.
I can scaffold these tests if helpful.
📝 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.
| @notification_router.get("/categories/{category}") | |
| def get_notifications_by_category(request, category: str, page: int = 1, limit: int = 10): | |
| """ | |
| Get notifications for a specific category for the authenticated user. | |
| Args: | |
| request: HTTP request object containing orguser authentication data | |
| category: The notification category to filter by | |
| page (int, optional): Page number for pagination. Defaults to 1 | |
| limit (int, optional): Number of notifications per page. Defaults to 10 | |
| Returns: | |
| dict: Paginated notifications for the specified category | |
| Raises: | |
| HttpError: 400 if retrieval fails | |
| """ | |
| orguser: OrgUser = request.orguser | |
| error, result = notifications_service.get_notifications_by_category( | |
| orguser, category, page, limit | |
| ) | |
| @notification_router.get("/categories/{category}") | |
| def get_notifications_by_category(request, category: str, page: int = 1, limit: int = 10): | |
| """ | |
| Get notifications for a specific category for the authenticated user. | |
| Args: | |
| request: HTTP request object containing orguser authentication data | |
| category: The notification category to filter by | |
| page (int, optional): Page number for pagination. Defaults to 1 | |
| limit (int, optional): Number of notifications per page. Defaults to 10 | |
| Returns: | |
| dict: Paginated notifications for the specified category | |
| Raises: | |
| HttpError: 400 if retrieval fails | |
| """ | |
| orguser: OrgUser = request.orguser | |
| error, result = notifications_service.get_notifications_by_category( | |
| orguser, category, page, limit | |
| ) | |
| if error is not None: | |
| raise HttpError(400, error) | |
| return result |
🤖 Prompt for AI Agents
In ddpui/api/notifications_api.py around lines 276-298, the endpoint calls
notifications_service.get_notifications_by_category but neither returns the
service result nor handles errors; update the handler to check the returned
(error, result) tuple, if error raise HttpError(400, str(error)) (matching the
project pattern), otherwise return the result (or a dict with pagination/meta
and items to match other endpoints); optionally annotate category as the
existing Enum type for validation and add tests for pagination, invalid
category, and service errors.
There was a problem hiding this comment.
@Ishankoradia , I think there is no need of this api, as we are getting notification by category from 'get_user_notifications_v1' only.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…ate notification category handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ddpui/api/user_preferences_api.py (1)
112-112: Do not label LLM-feature requests as ‘incident’This is not an incident-class notification. Leave category unset to avoid misclassification and unintended filtering, consistent with prior guidance to avoid defaulting to incident.
Apply this diff:
recipients=[acc_manager.id for acc_manager in acc_managers], - category="incident", # Adding category field
🧹 Nitpick comments (2)
ddpui/schemas/notifications_api_schemas.py (1)
72-73: Unify category typing: use the enum in NotificationDataSchemaNotificationDataSchema currently types category as str while CreateNotificationPayloadSchema uses NotificationCategoryEnum. Prefer the enum end-to-end for type safety and to prevent typos.
Apply this diff:
class NotificationDataSchema(Schema): """Schema use to call the notification service function for creating a notification""" @@ recipients: List[int] # list of orguser ids - category: Optional[str] + category: Optional[NotificationCategoryEnum] = None + + class Config: + use_enum_values = Trueddpui/utils/webhook_helpers.py (1)
151-166: Harden notify_org_managers: handle empty recipient lists and surface errorsIf get_recipients returns an empty list, we should skip creating a notification. Also consider propagating errors up for observability.
Apply this diff:
def notify_org_managers(org: Org, message: str, email_subject: str, category: str = None): @@ - if error: + if error: logger.error(f"Error getting recipients: {error}") return + if not recipients: + logger.info("No recipients found for org %s; skipping notification", org.slug) + return @@ - if error: + if error: logger.error(f"Error creating notification: {error}") return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
ddpui/api/user_preferences_api.py(4 hunks)ddpui/celeryworkers/tasks.py(2 hunks)ddpui/core/notifications_service.py(12 hunks)ddpui/migrations/0128_merge_20250818_0733.py(1 hunks)ddpui/schemas/notifications_api_schemas.py(3 hunks)ddpui/schemas/userpreferences_schema.py(1 hunks)ddpui/utils/webhook_helpers.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- ddpui/migrations/0128_merge_20250818_0733.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/core/notifications_service.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
📚 Learning: 2025-08-18T07:13:00.905Z
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
Applied to files:
ddpui/schemas/userpreferences_schema.pyddpui/api/user_preferences_api.pyddpui/schemas/notifications_api_schemas.py
🧬 Code Graph Analysis (3)
ddpui/celeryworkers/tasks.py (1)
ddpui/models/notifications.py (1)
NotificationCategory(5-13)
ddpui/api/user_preferences_api.py (1)
ddpui/tests/api_tests/test_user_preferences_api.py (1)
user_preferences(60-65)
ddpui/utils/webhook_helpers.py (1)
ddpui/models/notifications.py (1)
NotificationCategory(5-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (11)
ddpui/schemas/userpreferences_schema.py (2)
10-12: Create schema: new category toggles look goodDefaults to True on create are fine and align with typical onboarding behavior.
20-22: Update schema: partial-update fields are appropriateUsing Optional[bool] with None defaults enables clean PATCH semantics. LGTM.
ddpui/schemas/notifications_api_schemas.py (3)
19-29: Category enum matches model choicesEnum members mirror ddpui.models.notifications.NotificationCategory values. LGTM.
43-43: Correct: category defaults to None in create payloadNot defaulting to "incident" prevents misclassification. Good fix.
75-80: CategorySubscriptionSchema: correct omission of incident/late_runs/maintenanceLimiting user-configurable toggles to schema change, job failure, and dbt test failure aligns with product guidance: incident and maintenance should not be opt-out, and late runs aren’t supported yet.
ddpui/celeryworkers/tasks.py (2)
57-57: Importing NotificationCategory is appropriateUsed for consistent category tagging downstream. LGTM.
505-506: Correct category tagging for schema change notificationsPassing NotificationCategory.SCHEMA_CHANGE.value ensures category-aware filtering works as intended.
ddpui/utils/webhook_helpers.py (2)
13-13: Import of NotificationCategory is consistent with enum usageUsed later to tag job failure notifications. All good.
376-377: Correct: job failures are categorized for recipient filteringTagging JOB_FAILURE will respect per-category subscriptions.
ddpui/api/user_preferences_api.py (2)
52-63: Update endpoint: only updating supported categories is correctRestricting updates to the three exposed categories matches the schema and product decision to exclude incident/late runs from user control.
80-82: Get endpoint: returns only supported togglesResponse payload aligns with the current shape of user-configurable categories.
…pdate notification manager calls to include category
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ddpui/tests/api_tests/test_webhook_api.py (3)
293-299: LGTM; consistent propagation of JOB_FAILURE category in manual-with-connection-id path.Minor: To reduce duplication across these tests, consider a tiny helper that asserts notify_org_managers was called with the expected org/body/subject/category. Keeps the tests DRY.
347-353: LGTM; category assertion added for manual-with-orgtask-id path.Same optional refactor note: a shared helper for asserting notify_org_managers invocation would keep these three tests concise and consistent.
239-245: Add autospec=True to notify_org_managers patch to catch signature driftUsing autospec=True ensures these tests will fail if notify_org_managers ever drops or renames the category parameter.
ddpui/tests/api_tests/test_webhook_api.py (around lines 239–245)
- with patch("ddpui.utils.webhook_helpers.notify_org_managers") as mock_notify_org_managers: + with patch("ddpui.utils.webhook_helpers.notify_org_managers", autospec=True) as mock_notify_org_managers: … mock_notify_org_managers.assert_called_once_with( org, "\n".join(email_body), email_subject, category=NotificationCategory.JOB_FAILURE.value, )ddpui/tests/api_tests/test_webhook_api.py
Intest_post_notification_v1_webhook_scheduled_pipeline(failure section), strengthen the call‐assertion for parity:from unittest.mock import ANY … mock_notify_org_managers.assert_called_once_with( org, ANY, ANY, category=NotificationCategory.JOB_FAILURE.value, )These changes will guard against unintended signature changes and keep your Webhook tests consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ddpui/api/user_preferences_api.py(4 hunks)ddpui/tests/api_tests/test_webhook_api.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/api/user_preferences_api.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
🧬 Code Graph Analysis (1)
ddpui/tests/api_tests/test_webhook_api.py (1)
ddpui/models/notifications.py (1)
NotificationCategory(5-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (1)
ddpui/tests/api_tests/test_webhook_api.py (1)
40-40: Importing NotificationCategory is correct and consistent with new category-aware flow.Using TextChoices’ .value downstream in assertions is appropriate if notify_org_managers expects a string. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ddpui/utils/webhook_helpers.py (2)
151-167: Clarify docstring and accept both Enum and str for category; normalize before use
- Docstring currently says “all users in the org” but logic targets managers and above. Update for accuracy.
- Accepting NotificationCategory | str | None improves call-site ergonomics and type-safety. Normalize to a string before passing to get_recipients and NotificationDataSchema to avoid subtle TextChoices vs str lookup issues and to enforce consistent storage.
- This also preserves the “maintenance is mandatory” behavior (no preference filtering) because get_recipients only maps specific categories; MAINTENANCE remains unfiltered as intended per prior learnings.
Apply this diff:
-def notify_org_managers(org: Org, message: str, email_subject: str, category: str = None): - """send a notification to all users in the org""" - error, recipients = get_recipients( - SentToEnum.ALL_ORG_USERS, org.slug, None, manager_or_above=True, category=category - ) +def notify_org_managers( + org: Org, + message: str, + email_subject: str, + category: NotificationCategory | str | None = None, +): + """Send a notification to managers and above in the org. Optionally tag with a category.""" + # Normalize category to a string value for downstream services + category_value = category.value if isinstance(category, NotificationCategory) else category + error, recipients = get_recipients( + SentToEnum.ALL_ORG_USERS, org.slug, None, manager_or_above=True, category=category_value + ) @@ - error, response = create_notification( + error, response = create_notification( NotificationDataSchema( author="Dalgo", message=message, email_subject=email_subject, recipients=recipients, - category=category, + category=category_value, ) )
376-377: Prefer passing the Enum member over.value(pairs well with normalization above)Minor readability/type-safety improvement. If you accept the normalization refactor in notify_org_managers, pass the Enum directly here.
- org, "\n".join(email_body), email_subject, category=NotificationCategory.JOB_FAILURE.value + org, "\n".join(email_body), email_subject, category=NotificationCategory.JOB_FAILURE
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/utils/webhook_helpers.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
🧬 Code Graph Analysis (1)
ddpui/utils/webhook_helpers.py (3)
ddpui/models/notifications.py (1)
NotificationCategory(5-13)ddpui/core/notifications_service.py (2)
get_recipients(17-80)create_notification(128-189)ddpui/schemas/notifications_api_schemas.py (2)
SentToEnum(8-16)NotificationDataSchema(63-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (1)
ddpui/utils/webhook_helpers.py (1)
13-13: LGTM: Category enum import is appropriate and used correctlyThe NotificationCategory import is necessary and used below to tag job-failure notifications. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ddpui/api/notifications_api.py (2)
57-68: create_notification expects a schema object, not a dict; also fix email_subject fallbackPassing a dict will break because the service accesses attributes (notification_data.author, ...). Instantiate NotificationDataSchema and honor payload.email_subject when provided.
Apply this diff:
- notification_data = { - "author": payload.author, - "message": payload.message, - "email_subject": payload.message, # Use message as email subject if not provided - "urgent": payload.urgent, - "scheduled_time": payload.scheduled_time, - "recipients": recipients, - "category": payload.category, - } - - error, result = notifications_service.create_notification(notification_data) + notification_data = NotificationDataSchema( + author=payload.author, + message=payload.message, + email_subject=payload.email_subject or payload.message, # Use message as fallback + urgent=payload.urgent, + scheduled_time=payload.scheduled_time, + recipients=recipients, + category=payload.category, + ) + + error, result = notifications_service.create_notification(notification_data)
75-106: Fix invalid read_status filter in history endpointThe
get_notification_historyservice currently callsnotifications = Notification.objects if read_status: notifications = notifications.filter(read_status=(read_status == 1))but the Notification model does not have a
read_statusfield (onlyNotificationRecipientdoes). This will error at runtime. Please choose one:
- Remove the
read_statusparameter from the/historyendpoint (handler, service, and related schema), or- Change the service to join/filter on
NotificationRecipient.read_statusinstead ofNotification.Files to update:
- ddpui/core/notifications_service.py
– Remove or refactor lines ~199–201 (theread_statusfilter inget_notification_history).- ddpui/api/notifications_api.py
– Dropread_statusfrom the/historyhandler signature (line 76) and its doc/schema.- ddpui/schemas/notifications_api_schemas.py
– Remove or adjust theread_statusfield in the history‐request schema.Please address before merging.
ddpui/core/notifications_service.py (1)
76-80: Avoid boolean evaluation of QuerySetDjango QuerySet truthiness is ambiguous. Use exists() before converting to list.
Apply this diff:
- if not recipients: + if not recipients.exists(): return "No users found for the given information", None
🧹 Nitpick comments (4)
ddpui/utils/webhook_helpers.py (1)
397-431: Avoid magic string "DBT_TEST_FAILED"; consider centralizing as a constantThe new DBT test failure branch is solid and includes pipeline context. To reduce drift, define DBT_TEST_FAILED alongside other Prefect state constants (in ddpui.ddpprefect) and import it here.
ddpui/api/notifications_api.py (2)
9-10: Remove unused CategorySubscriptionSchema import (ruff F401)Import is unused. Replace it with NotificationDataSchema which you actually need below.
Apply this diff:
from ddpui.schemas.notifications_api_schemas import ( CreateNotificationPayloadSchema, UpdateReadStatusSchema, UpdateReadStatusSchemav1, - CategorySubscriptionSchema, + NotificationDataSchema, )
18-43: Docstring claim “category defaults to INCIDENT” is inaccurateNo defaulting to INCIDENT happens in code or schema; the field is optional and can be None. Update the docs to avoid misleading API consumers.
Apply this diff:
- - category: Notification category (optional, defaults to INCIDENT) + - category: Notification category (optional)ddpui/core/notifications_service.py (1)
65-75: Include users with no preferences record (default-subscribe) in recipient filterCurrently you exclude users with no preferences via preferences__isnull=False. Given defaults should be “subscribed unless explicitly opted out”, include those users too.
Apply this diff:
- recipients = ( - OrgUser.objects.filter( - id__in=recipients, - preferences__isnull=False, - ) - .filter(**{f"preferences__{preference_field}": True}) - .values_list("id", flat=True) - ) + from django.db.models import Q + recipients = ( + OrgUser.objects.filter(id__in=recipients) + .filter(Q(preferences__isnull=True) | Q(**{f"preferences__{preference_field}": True})) + .values_list("id", flat=True) + )Note: Add the import if not present at the top of this file:
from django.db.models import Q
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
ddpui/api/notifications_api.py(10 hunks)ddpui/core/notifications_service.py(12 hunks)ddpui/tests/api_tests/test_webhook_api.py(5 hunks)ddpui/utils/webhook_helpers.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ddpui/tests/api_tests/test_webhook_api.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
📚 Learning: 2025-08-18T07:13:00.905Z
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
Applied to files:
ddpui/api/notifications_api.pyddpui/core/notifications_service.py
🧬 Code Graph Analysis (3)
ddpui/api/notifications_api.py (2)
ddpui/schemas/notifications_api_schemas.py (1)
CategorySubscriptionSchema(75-80)ddpui/core/notifications_service.py (4)
get_recipients(17-80)get_notification_history(193-230)fetch_user_notifications_v1(300-349)get_urgent_notifications(444-475)
ddpui/utils/webhook_helpers.py (4)
ddpui/models/notifications.py (1)
NotificationCategory(5-13)ddpui/models/org.py (2)
Org(61-83)OrgDataFlowv1(177-203)ddpui/core/notifications_service.py (2)
get_recipients(17-80)create_notification(128-189)ddpui/schemas/notifications_api_schemas.py (2)
SentToEnum(8-16)NotificationDataSchema(63-72)
ddpui/core/notifications_service.py (3)
ddpui/models/notifications.py (3)
Notification(16-33)NotificationRecipient(36-44)NotificationCategory(5-13)ddpui/models/userpreferences.py (1)
is_subscribed_to_category(47-56)ddpui/api/notifications_api.py (1)
get_urgent_notifications(252-273)
🪛 Ruff (0.12.2)
ddpui/api/notifications_api.py
9-9: ddpui.schemas.notifications_api_schemas.CategorySubscriptionSchema imported but unused
Remove unused import
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (4)
ddpui/utils/webhook_helpers.py (2)
151-166: Category propagation to recipients and creation looks goodForwarding category into get_recipients and NotificationDataSchema is correct and keeps the surface area small. Good error handling on both recipient resolution and creation.
375-377: Correctly tags job failures with categoryPassing NotificationCategory.JOB_FAILURE.value ensures downstream subscription filtering works (once service mapping uses string values, see separate comment).
ddpui/core/notifications_service.py (2)
94-97: Per-recipient subscription gating is correct (maintenance stays mandatory)The is_subscribed_to_category check here ensures opt-outs are respected. Since maintenance isn’t part of the mapping and defaults to True, mandatory maintenance notifications are unaffected.
441-476: Urgent notifications endpoint/service looks goodEfficient query, correct constraints (urgent, sent, unread), and minimal payload. LGTM.
| # get notification history | ||
| def get_notification_history( | ||
| page: int, limit: int, read_status: Optional[int] = None | ||
| page: int, limit: int, read_status: Optional[int] = None, category: Optional[str] = None | ||
| ) -> Tuple[Optional[None], Dict[str, Any]]: | ||
| """returns history of sent notifications""" | ||
| notifications = Notification.objects | ||
|
|
||
| if read_status: | ||
| notifications = notifications.filter(read_status=(read_status == 1)) | ||
|
|
||
| if category: | ||
| notifications = notifications.filter(category=category) | ||
|
|
||
| notifications = notifications.all().order_by("-timestamp") |
There was a problem hiding this comment.
read_status filtering on Notification is invalid; remove or rework
Notification doesn’t have read_status (it’s on NotificationRecipient). Filtering here will error or mislead. For “history” (system-wide), drop read_status filtering.
Apply this diff:
- if read_status:
- notifications = notifications.filter(read_status=(read_status == 1))
-
if category:
notifications = notifications.filter(category=category)Follow-up: Align the API doc/parameters to reflect that history doesn’t support read_status. If per-user read status is needed, use fetch_user_notifications_v1 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.
| # get notification history | |
| def get_notification_history( | |
| page: int, limit: int, read_status: Optional[int] = None | |
| page: int, limit: int, read_status: Optional[int] = None, category: Optional[str] = None | |
| ) -> Tuple[Optional[None], Dict[str, Any]]: | |
| """returns history of sent notifications""" | |
| notifications = Notification.objects | |
| if read_status: | |
| notifications = notifications.filter(read_status=(read_status == 1)) | |
| if category: | |
| notifications = notifications.filter(category=category) | |
| notifications = notifications.all().order_by("-timestamp") | |
| # get notification history | |
| def get_notification_history( | |
| page: int, limit: int, read_status: Optional[int] = None, category: Optional[str] = None | |
| ) -> Tuple[Optional[None], Dict[str, Any]]: | |
| """returns history of sent notifications""" | |
| notifications = Notification.objects | |
| if category: | |
| notifications = notifications.filter(category=category) | |
| notifications = notifications.all().order_by("-timestamp") |
🤖 Prompt for AI Agents
In ddpui/core/notifications_service.py around lines 192 to 205, the code
attempts to filter Notification objects by read_status but the read_status field
exists on NotificationRecipient, not Notification; remove the read_status branch
from get_notification_history so it does not filter notifications by read_status
(drop the conditional and any related parameter use), and update the function
signature and docstring to remove read_status from this history API; if per-user
read filtering is required, direct callers to use fetch_user_notifications_v1
which operates on NotificationRecipient.
…ation preferences
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ddpui/core/notifications_service.py (2)
56-75: Subscription pre-filter excludes users without preferences; include them by default (opt-in) and keep maintenance mandatory
- Current filter requires preferences__isnull=False, which silently drops users who don’t yet have a UserPreferences row. This likely reduces recipients unintentionally. Default should be “subscribed” when no preferences exist.
- Also, nice fix using .value for the mapping keys; that addresses the earlier type-mismatch issue.
- Optional: to avoid drift, you can derive the field name from the category (subscribe_{category}_notifications) instead of maintaining a manual dict, as previously suggested by the team. Only do that for categories that actually have a preference field.
Apply this refactor to include users with no preferences as subscribed by default:
- recipients = ( - OrgUser.objects.filter( - id__in=recipients, - preferences__isnull=False, - ) - .filter(**{f"preferences__{preference_field}": True}) - .values_list("id", flat=True) - ) + recipients = ( + OrgUser.objects.filter(id__in=recipients) + .filter( + Q(preferences__isnull=True) + | Q(**{f"preferences__{preference_field}": True}) + ) + .values_list("id", flat=True) + )Add the import:
from django.db.models import QNote: Intentionally do not map “maintenance” here so it stays mandatory (see next comment).
194-205: read_status filtering here is invalid; Notification has no read_status — remove this blockFiltering Notification by read_status will error or mislead. read_status is on NotificationRecipient. Keep category filtering here; for read filters, use user-scoped endpoints (e.g., fetch_user_notifications_v1).
Apply:
- if read_status: - notifications = notifications.filter(read_status=(read_status == 1)) - if category: notifications = notifications.filter(category=category)Optionally, drop the read_status parameter from this function and its API surface. If callers rely on it, ignore it and document that it’s unsupported for history.
Run to find history callers passing read_status:
#!/bin/bash rg -n -C2 "\bget_notification_history\s*\(" --type=py
🧹 Nitpick comments (4)
ddpui/core/notifications_service.py (4)
18-23: Validate category input early to prevent persisting invalid valuesRight now, an invalid category string can flow through and be saved (choices aren’t enforced at model save). Add a quick guard to validate against TextChoices.
Consider this insertion right after the function signature:
def get_recipients( sent_to: str, org_slug: str, user_email: str, manager_or_above: bool, category: Optional[str] = None, ) -> Tuple[Optional[str], Optional[List[int]]]: + # Validate category against allowed values + if category and category not in NotificationCategory.values: + return "Invalid category supplied.", NoneIf the schema already enforces this, feel free to skip. Otherwise, this prevents invalid data from being saved downstream.
76-80: Prefer QuerySet.exists() for emptiness checkUsing the QuerySet in a boolean context forces evaluation of all rows. Use exists() for an efficient emptiness check.
Apply:
- if not recipients: + if not recipients.exists(): return "No users found for the given information", None
300-315: Validate category filter input before queryingTo avoid silent no-op or bad values, validate the incoming category against TextChoices before applying the filter.
Apply:
- if category: + if category: + if category not in NotificationCategory.values: + return None, {"success": True, "res": [], "page": 1, "total_pages": 0, "total_notifications": 0} filter_kwargs["notification__category"] = categoryAlternative: raise a 400 in the API layer if category is invalid (preferred), and trust the contract here.
443-476: Docstring wording and (optional) pagination for urgent notifications
- The docstring says “haven’t been dismissed”, but the implementation returns unread notifications (read_status=False). Consider rewording to “unread urgent notifications”.
- If the urgent list could ever grow large, consider adding a reasonable cap or pagination. If it’s guaranteed small, current approach is fine.
Apply this small docstring tweak:
- Returns urgent notifications that haven't been dismissed by the user. + Returns unread urgent notifications for the user.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ddpui/core/notifications_service.py(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
📚 Learning: 2025-08-18T07:13:00.905Z
Learnt from: Ishankoradia
PR: DalgoT4D/DDP_backend#1100
File: ddpui/schemas/notifications_api_schemas.py:75-82
Timestamp: 2025-08-18T07:13:00.905Z
Learning: In the DDP_backend notifications system, users should always be subscribed to maintenance type notifications regardless of their preference settings. The CategorySubscriptionSchema intentionally does not include a subscribe_maintenance_notifications field because maintenance notifications are mandatory for all users.
Applied to files:
ddpui/core/notifications_service.py
🧬 Code Graph Analysis (1)
ddpui/core/notifications_service.py (3)
ddpui/models/notifications.py (3)
Notification(16-33)NotificationRecipient(36-44)NotificationCategory(5-13)ddpui/models/userpreferences.py (1)
is_subscribed_to_category(47-56)ddpui/api/notifications_api.py (1)
get_urgent_notifications(252-273)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (4)
ddpui/core/notifications_service.py (4)
5-5: LGTM: Importing NotificationCategory is appropriate hereThis import is needed for category choices and mapping below. No issues.
142-143: LGTM: Category propagation during create and in the responseStoring and surfacing the category on creation (and in the response) is correct and aligns with the new model field.
Also applies to: 151-152, 183-184
219-220: LGTM: Including category in history payloadConsistent with the new field; no issues.
339-340: LGTM: Category included in per-user notification payloadThis aligns the v1 endpoint with the rest of the API.
| # Check if user is subscribed to this notification category | ||
| if not user_preference.is_subscribed_to_category(notification.category): | ||
| return None # Skip sending notification if user is not subscribed | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce maintenance notifications as mandatory (skip preference check)
Per the product rule, maintenance notifications must always be delivered regardless of user preferences. Bypass the preference check for maintenance.
Apply:
- # Check if user is subscribed to this notification category
- if not user_preference.is_subscribed_to_category(notification.category):
- return None # Skip sending notification if user is not subscribed
+ # Enforce mandatory delivery for maintenance; otherwise honor user preference
+ should_send = (
+ notification.category == NotificationCategory.MAINTENANCE.value
+ or user_preference.is_subscribed_to_category(notification.category)
+ )
+ if not should_send:
+ return None # Skip sending notification if user is not subscribedIf “incident” should also be mandatory (as previously discussed), extend the condition similarly. I can update both here and in the UserPreferences model to centralize this rule if you prefer.
📝 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.
| # Check if user is subscribed to this notification category | |
| if not user_preference.is_subscribed_to_category(notification.category): | |
| return None # Skip sending notification if user is not subscribed | |
| # Enforce mandatory delivery for maintenance; otherwise honor user preference | |
| should_send = ( | |
| notification.category == NotificationCategory.MAINTENANCE.value | |
| or user_preference.is_subscribed_to_category(notification.category) | |
| ) | |
| if not should_send: | |
| return None # Skip sending notification if user is not subscribed |
🤖 Prompt for AI Agents
In ddpui/core/notifications_service.py around lines 94 to 97, the current
preference check unconditionally skips sending when the user is unsubscribed;
change it so maintenance notifications are always delivered by bypassing the
preference check for that category (and include "incident" as well if product
requires it). Concretely, only perform the is_subscribed_to_category check for
non-mandatory categories (or define a small set of mandatory categories and
always allow those through), so maintenance (and optionally incident)
notifications are not blocked by user preferences.
Summary by CodeRabbit
New Features
Preferences
Documentation
Tests
Chores