diff --git a/apps/api/plane/api/views/issue.py b/apps/api/plane/api/views/issue.py index da9edc66d66..e51de84189b 100644 --- a/apps/api/plane/api/views/issue.py +++ b/apps/api/plane/api/views/issue.py @@ -871,6 +871,8 @@ def delete(self, request, slug, project_id, pk): project_id=str(project_id), current_instance=current_instance, epoch=int(timezone.now().timestamp()), + notification=True, + origin=base_host(request=request, is_app=True), ) return Response(status=status.HTTP_204_NO_CONTENT) @@ -1208,6 +1210,8 @@ def post(self, request, slug, project_id, issue_id): actor_id=str(link.created_by_id), current_instance=None, epoch=int(timezone.now().timestamp()), + notification=True, + origin=base_host(request=request, is_app=True), ) serializer = IssueLinkSerializer(link) return Response(serializer.data, status=status.HTTP_201_CREATED) @@ -1319,6 +1323,8 @@ def patch(self, request, slug, project_id, issue_id, pk): project_id=str(project_id), current_instance=current_instance, epoch=int(timezone.now().timestamp()), + notification=True, + origin=base_host(request=request, is_app=True), ) serializer = IssueLinkSerializer(issue_link) return Response(serializer.data, status=status.HTTP_200_OK) @@ -1352,6 +1358,8 @@ def delete(self, request, slug, project_id, issue_id, pk): project_id=str(project_id), current_instance=current_instance, epoch=int(timezone.now().timestamp()), + notification=True, + origin=base_host(request=request, is_app=True), ) issue_link.delete() return Response(status=status.HTTP_204_NO_CONTENT) @@ -1489,12 +1497,20 @@ def post(self, request, slug, project_id, issue_id): issue_activity.delay( type="comment.activity.created", - requested_data=json.dumps(serializer.data, cls=DjangoJSONEncoder), + # Serialize the saved comment rather than the write serializer: the + # latter carries no ``id``, so ``create_comment_activity`` stores the + # activity with ``issue_comment_id=None`` and ``notification_task`` + # then skips comment-mention extraction entirely, which is gated on + # that field. The update path already reads the id out of + # ``current_instance``, which is why only creates lose mentions. + requested_data=json.dumps(IssueCommentSerializer(issue_comment).data, cls=DjangoJSONEncoder), actor_id=str(issue_comment.created_by_id), issue_id=str(self.kwargs.get("issue_id")), project_id=str(self.kwargs.get("project_id")), current_instance=None, epoch=int(timezone.now().timestamp()), + notification=True, + origin=base_host(request=request, is_app=True), ) # Send the model activity @@ -1635,6 +1651,8 @@ def patch(self, request, slug, project_id, issue_id, pk): project_id=str(project_id), current_instance=current_instance, epoch=int(timezone.now().timestamp()), + notification=True, + origin=base_host(request=request, is_app=True), ) # Send the model activity model_activity.delay( @@ -1681,6 +1699,8 @@ def delete(self, request, slug, project_id, issue_id, pk): project_id=str(project_id), current_instance=current_instance, epoch=int(timezone.now().timestamp()), + notification=True, + origin=base_host(request=request, is_app=True), ) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/apps/api/plane/tests/conftest.py b/apps/api/plane/tests/conftest.py index 870779c42d6..7fa564373d3 100644 --- a/apps/api/plane/tests/conftest.py +++ b/apps/api/plane/tests/conftest.py @@ -9,6 +9,33 @@ from plane.db.models import User, Workspace, WorkspaceMember from plane.db.models.api import APIToken +# Every external-API test authenticates with this one token, so they all share a +# single ``ApiKeyRateThrottle`` bucket keyed on it. +TEST_API_TOKEN = "test-api-token-12345" + + +@pytest.fixture(autouse=True) +def reset_api_key_throttle(): + """Give each test the full external-API rate limit. + + ``ApiKeyRateThrottle`` keys its bucket on the API token and stores the + request history in the Django cache, which is a real Redis in CI and is not + rolled back with the database. Because every test reuses ``TEST_API_TOKEN``, + the history accumulates across the session until the suite crosses + ``API_KEY_RATE_LIMIT`` (60/minute) and later tests start failing with 429s + that have nothing to do with what they assert. + + That makes the suite order- and size-dependent: adding tests anywhere can + break unrelated ones further down. Clearing the bucket around each test + keeps the limit per-test, which is what these tests assume. + """ + from django.core.cache import cache + + key = f"api_key:{TEST_API_TOKEN}" + cache.delete(key) + yield + cache.delete(key) + @pytest.fixture(scope="session") def django_db_setup(django_db_setup): # noqa: F811 @@ -52,7 +79,7 @@ def api_token(db, create_user): token = APIToken.objects.create( user=create_user, label="Test API Token", - token="test-api-token-12345", + token=TEST_API_TOKEN, ) return token diff --git a/apps/api/plane/tests/contract/api/test_comment_link_notifications.py b/apps/api/plane/tests/contract/api/test_comment_link_notifications.py new file mode 100644 index 00000000000..2d4572ff87f --- /dev/null +++ b/apps/api/plane/tests/contract/api/test_comment_link_notifications.py @@ -0,0 +1,227 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Notification parity for the remaining external REST API write paths. + +makeplane/plane#9307 fixed this for work item create/update, but the same +omission was left on comments, links, and work item deletion: those endpoints +dispatched ``issue_activity`` without ``notification=True``, so the activity was +recorded and webhooks fired while subscribers were never notified. The web app +(``plane.app.views.issue``) passes the flag on every one of these paths, so the +external API was silently the odd one out. + +These assert the dispatch contract rather than the delivered notification. +``notification=True`` is what gates ``notifications.delay(...)`` in +``issue_activities_task``; the fan-out itself is already covered downstream, and +mocking at the dispatch boundary keeps these tests from depending on Celery. +""" + +import json +from unittest.mock import patch + +import pytest +from rest_framework import status + +from plane.db.models import Issue, IssueComment, IssueLink, Project, ProjectMember, State + + +@pytest.fixture +def project(db, workspace, create_user): + """A project with the user as admin and a default state.""" + project = Project.objects.create( + name="Test Project", + identifier="TP", + workspace=workspace, + created_by=create_user, + ) + ProjectMember.objects.create(project=project, member=create_user, role=20, is_active=True) + State.objects.create( + name="Backlog", + color="#000000", + group="backlog", + default=True, + project=project, + workspace=workspace, + created_by=create_user, + ) + return project + + +@pytest.fixture +def create_issue(db, project, workspace, create_user): + return Issue.objects.create( + name="Existing Issue", + project=project, + workspace=workspace, + created_by=create_user, + ) + + +@pytest.fixture +def create_comment(db, project, workspace, create_issue, create_user): + return IssueComment.objects.create( + comment_html="

Existing comment

", + issue=create_issue, + project=project, + workspace=workspace, + actor=create_user, + created_by=create_user, + ) + + +@pytest.fixture +def create_link(db, project, workspace, create_issue, create_user): + return IssueLink.objects.create( + url="https://example.com/original", + issue=create_issue, + project=project, + workspace=workspace, + created_by=create_user, + ) + + +def _dispatched(mock): + """The single ``issue_activity.delay`` kwargs dict for this request.""" + mock.delay.assert_called_once() + return mock.delay.call_args.kwargs + + +@pytest.mark.contract +class TestCommentNotificationContract: + """Comment writes through ``/api/v1/...`` must notify, as the web app does.""" + + def url(self, workspace, project, issue, pk=None): + base = f"/api/v1/workspaces/{workspace.slug}/projects/{project.id}/issues/{issue.id}/comments/" + return f"{base}{pk}/" if pk else base + + @pytest.mark.django_db + def test_create_comment_notifies(self, api_key_client, workspace, project, create_issue): + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.post( + self.url(workspace, project, create_issue), + {"comment_html": "

New comment

"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + kwargs = _dispatched(activity) + assert kwargs["type"] == "comment.activity.created" + assert kwargs["notification"] is True + assert kwargs["origin"] + + @pytest.mark.django_db + def test_create_comment_activity_carries_the_comment_id(self, api_key_client, workspace, project, create_issue): + """Without the id, notifying is not enough — @mentions are still dropped. + + ``create_comment_activity`` sets ``issue_comment_id`` from + ``requested_data["id"]``, and ``notification_task`` gates *all* + comment-mention extraction on that field being set. Serializing the write + serializer leaves it null, so a mention in an API-created comment notifies + nobody even once ``notification=True`` is passed. Comment *updates* are + unaffected, because they read the id from ``current_instance``. + """ + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.post( + self.url(workspace, project, create_issue), + {"comment_html": "

Mentioning someone

"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + requested = json.loads(_dispatched(activity)["requested_data"]) + assert requested.get("id") == response.json()["id"] + + @pytest.mark.django_db + def test_update_comment_notifies(self, api_key_client, workspace, project, create_issue, create_comment): + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.patch( + self.url(workspace, project, create_issue, create_comment.id), + {"comment_html": "

Edited comment

"}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK + kwargs = _dispatched(activity) + assert kwargs["type"] == "comment.activity.updated" + assert kwargs["notification"] is True + assert kwargs["origin"] + + @pytest.mark.django_db + def test_delete_comment_notifies(self, api_key_client, workspace, project, create_issue, create_comment): + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.delete(self.url(workspace, project, create_issue, create_comment.id)) + + assert response.status_code == status.HTTP_204_NO_CONTENT + kwargs = _dispatched(activity) + assert kwargs["type"] == "comment.activity.deleted" + assert kwargs["notification"] is True + assert kwargs["origin"] + + +@pytest.mark.contract +class TestLinkNotificationContract: + """Link writes through ``/api/v1/...`` must notify, as the web app does.""" + + def url(self, workspace, project, issue, pk=None): + base = f"/api/v1/workspaces/{workspace.slug}/projects/{project.id}/issues/{issue.id}/links/" + return f"{base}{pk}/" if pk else base + + @pytest.mark.django_db + def test_create_link_notifies(self, api_key_client, workspace, project, create_issue): + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.post( + self.url(workspace, project, create_issue), + {"url": "https://example.com/spec"}, + format="json", + ) + + assert response.status_code == status.HTTP_201_CREATED + kwargs = _dispatched(activity) + assert kwargs["type"] == "link.activity.created" + assert kwargs["notification"] is True + assert kwargs["origin"] + + @pytest.mark.django_db + def test_update_link_notifies(self, api_key_client, workspace, project, create_issue, create_link): + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.patch( + self.url(workspace, project, create_issue, create_link.id), + {"url": "https://example.com/updated"}, + format="json", + ) + + assert response.status_code == status.HTTP_200_OK + kwargs = _dispatched(activity) + assert kwargs["type"] == "link.activity.updated" + assert kwargs["notification"] is True + assert kwargs["origin"] + + @pytest.mark.django_db + def test_delete_link_notifies(self, api_key_client, workspace, project, create_issue, create_link): + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.delete(self.url(workspace, project, create_issue, create_link.id)) + + assert response.status_code == status.HTTP_204_NO_CONTENT + kwargs = _dispatched(activity) + assert kwargs["type"] == "link.activity.deleted" + assert kwargs["notification"] is True + assert kwargs["origin"] + + +@pytest.mark.contract +class TestIssueDeleteNotificationContract: + """#9307 covered create and update; deletion was left behind.""" + + @pytest.mark.django_db + def test_delete_issue_notifies(self, api_key_client, workspace, project, create_issue): + url = f"/api/v1/workspaces/{workspace.slug}/projects/{project.id}/issues/{create_issue.id}/" + + with patch("plane.api.views.issue.issue_activity") as activity: + response = api_key_client.delete(url) + + assert response.status_code == status.HTTP_204_NO_CONTENT + kwargs = _dispatched(activity) + assert kwargs["type"] == "issue.activity.deleted" + assert kwargs["notification"] is True + assert kwargs["origin"]