🐛 Bug: Issues created or updated via REST API send no notifications or emails#9307
🐛 Bug: Issues created or updated via REST API send no notifications or emails#9307wildsurfer wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe API issue create, update, and assignee paths now pass ChangesIssue API Notification Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
I assume the |
|
Hi @wildsurfer, Thank you for your contribution. The changes are looking good. Just one small change. could you rename the test file to Thanks. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/tests/contract/api/test_issue_notifications.py (1)
116-135: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptionally assert the assignment actually persisted.
The create/update tests verify the side effect (issue exists / renamed) before checking dispatch, but the assign test only checks status and the notification kwargs. Adding an assertion that the assignee is attached would make the test resilient to a regression where the response is 200 yet the assignment silently doesn't persist.
♻️ Proposed assertion
assert response.status_code == status.HTTP_200_OK + create_issue.refresh_from_db() + assert create_issue.assignees.filter(id=assignee_user.id).exists() mock_issue_activity.delay.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/contract/api/test_issue_notifications.py` around lines 116 - 135, The assign-issue test only verifies the notification dispatch and HTTP status, so it should also confirm the assignment persisted on the issue. In test_assign_issue_triggers_notification, after the api_key_client.patch call and before checking issue_activity.delay, assert the issue returned by create_issue has the assignee_user attached (using the issue/assignees relationship on the issue model) so a 200 response cannot hide a failed assignment update.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/plane/tests/contract/api/test_issue_notifications.py`:
- Around line 116-135: The assign-issue test only verifies the notification
dispatch and HTTP status, so it should also confirm the assignment persisted on
the issue. In test_assign_issue_triggers_notification, after the
api_key_client.patch call and before checking issue_activity.delay, assert the
issue returned by create_issue has the assignee_user attached (using the
issue/assignees relationship on the issue model) so a 200 response cannot hide a
failed assignment update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fefc8d0-6670-4ce9-a6c8-df32328192be
📒 Files selected for processing (2)
apps/api/plane/api/views/issue.pyapps/api/plane/tests/contract/api/test_issue_notifications.py
|
@sangeethailango done ✅ |
Description
When a work item is created, updated, or assigned through the external REST API
(
/api/v1/...), no in-app notification and no email are sent to the assigneesand subscribers. The same actions in the web app send them correctly, and SMTP
works (OTP and web-app notification emails arrive), so the notification pipeline
itself is fine.
Root cause: the notification fan-out is gated by the
notificationflag passedto the
issue_activitybackground task, which defaults tonotification=False(
apps/api/plane/bgtasks/issue_activities_task.py). The external API issuecreate and update endpoints in
apps/api/plane/api/views/issue.pycallissue_activity.delay(...)withoutnotification=True(and withoutorigin),so
notifications.delay(...)is never scheduled. The web app views(
apps/api/plane/app/views/issue/base.py) passnotification=Trueandorigin=base_host(request=request, is_app=True), which is why the web appnotifies. In the same API file the attachment and relation endpoints already
pass
notification=True; only issue create/update did not.Fix: pass
notification=Trueandorigin=base_host(request=request, is_app=True)to the
issue_activity.delay(...)calls for issue create and update/assign, thesame way the web app views do.
base_hostis already imported in the file and isalready used by the sibling
model_activity.delay(...)calls in the same methods.Type of Change
Screenshots and Media (if applicable)
N/A
Test Scenarios
Added a contract test file
apps/api/plane/tests/contract/api/test_issues.py(class
TestIssueNotificationContract, marked@pytest.mark.contract). Each testpatches
plane.api.views.issue.issue_activityand asserts the dispatched activityhas
notification=True:test_create_issue_triggers_notification—POST /api/v1/.../issues/returns201 and dispatches
issue_activity.delayonce withtype="issue.activity.created"and
notification=True.test_update_issue_triggers_notification—PATCH /api/v1/.../issues/{id}/(rename) returns 200 and dispatches
issue_activity.delayonce withtype="issue.activity.updated"andnotification=True.test_assign_issue_triggers_notification—PATCH /api/v1/.../issues/{id}/(assign to a project member) returns 200 and dispatches
issue_activity.delayonce with
type="issue.activity.updated"andnotification=True.The tests fail before the change (the
notificationkwarg is absent) and passafter it. Ran them against Postgres, Redis and RabbitMQ: 3 passed. Also ran
ruff(rulesE,Fperapps/api/pyproject.toml) andruff formaton thechanged files: no issues.
References
Fixes #9306
Summary by CodeRabbit