diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 000000000..c9ebf2d27 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,3 @@ +{ + "python-envs.defaultEnvManager": "ms-python.python:system" +} \ No newline at end of file diff --git a/taiga/projects/notifications/mixins.py b/taiga/projects/notifications/mixins.py index 2a9816381..9c63a5d71 100644 --- a/taiga/projects/notifications/mixins.py +++ b/taiga/projects/notifications/mixins.py @@ -42,7 +42,7 @@ def pre_conditions_on_save(self, obj) """ _not_notify = False - _old_watchers = None + _old_watchers = [] _old_mentions = [] @detail_route(methods=["POST"]) @@ -294,8 +294,14 @@ def restore_object(self, attrs, instance=None): new_watcher_ids = attrs.pop("watchers", None) obj = super(EditableWatchedResourceSerializer, self).restore_object(attrs, instance) - # A partial update can exclude the watchers field or if the new instance can still not be saved - if instance is None or new_watcher_ids is None: + # A partial update can exclude the watchers field + if new_watcher_ids is None: + return obj + + # On creation the object hasn't been persisted yet so M2M relations can't be set. + # Defer watcher assignment to save(). + if instance is None: + self._pending_watcher_ids = list(new_watcher_ids) return obj new_watcher_ids = set(new_watcher_ids) @@ -330,6 +336,15 @@ def to_native(self, obj): def save(self, **kwargs): obj = super(EditableWatchedResourceSerializer, self).save(**kwargs) + + # Apply watchers that were deferred during object creation (when instance was None) + pending_watcher_ids = getattr(self, '_pending_watcher_ids', None) + if pending_watcher_ids is not None: + adding_users = get_user_model().objects.filter(id__in=pending_watcher_ids) + for user in adding_users: + services.add_watcher(obj, user) + self._pending_watcher_ids = None + self.fields["watchers"] = WatchersField(required=False) obj.watchers = [user.id for user in obj.get_watchers()] return obj diff --git a/tests/integration/test_watch_issues.py b/tests/integration/test_watch_issues.py index c71539d41..710e20001 100644 --- a/tests/integration/test_watch_issues.py +++ b/tests/integration/test_watch_issues.py @@ -137,3 +137,94 @@ def test_remove_issue_watcher(client): assert response.status_code == 200 assert response.data['watchers'] == [] assert response.data['is_watcher'] == False + + +def test_create_issue_with_watchers_via_api(client): + """ + Regression test for: watchers specified at creation time via the API + must be persisted (bug: they were silently discarded). + """ + owner = f.UserFactory.create() + watcher = f.UserFactory.create() + project = f.ProjectFactory.create(owner=owner) + f.MembershipFactory.create(project=project, user=owner, is_admin=True) + f.MembershipFactory.create(project=project, user=watcher, is_admin=True) + url = reverse("issues-list") + + data = { + "subject": "Issue with watchers", + "project": project.id, + "watchers": [watcher.id], + } + client.login(owner) + response = client.json.post(url, json.dumps(data)) + + assert response.status_code == 201 + assert watcher.id in response.data['watchers'] + assert response.data['total_watchers'] >= 1 + + +def test_create_issue_with_multiple_watchers_via_api(client): + """ + Multiple watchers specified at creation time should all be persisted. + """ + owner = f.UserFactory.create() + watcher1 = f.UserFactory.create() + watcher2 = f.UserFactory.create() + project = f.ProjectFactory.create(owner=owner) + f.MembershipFactory.create(project=project, user=owner, is_admin=True) + f.MembershipFactory.create(project=project, user=watcher1, is_admin=True) + f.MembershipFactory.create(project=project, user=watcher2, is_admin=True) + url = reverse("issues-list") + + data = { + "subject": "Issue with multiple watchers", + "project": project.id, + "watchers": [watcher1.id, watcher2.id], + } + client.login(owner) + response = client.json.post(url, json.dumps(data)) + + assert response.status_code == 201 + assert watcher1.id in response.data['watchers'] + assert watcher2.id in response.data['watchers'] + + +def test_create_issue_without_watchers_still_works(client): + """ + Regression: creating an issue without specifying watchers should still succeed. + """ + owner = f.UserFactory.create() + project = f.ProjectFactory.create(owner=owner) + f.MembershipFactory.create(project=project, user=owner, is_admin=True) + url = reverse("issues-list") + + data = { + "subject": "Issue without watchers", + "project": project.id, + } + client.login(owner) + response = client.json.post(url, json.dumps(data)) + + assert response.status_code == 201 + assert response.data['watchers'] == [] or response.data['watchers'] is not None + + +def test_update_issue_watchers_still_works(client): + """ + Regression: updating watchers on an existing issue via PATCH must still work. + """ + owner = f.UserFactory.create() + watcher = f.UserFactory.create() + project = f.ProjectFactory.create(owner=owner) + f.MembershipFactory.create(project=project, user=owner, is_admin=True) + f.MembershipFactory.create(project=project, user=watcher, is_admin=True) + issue = f.create_issue(owner=owner, project=project) + url = reverse("issues-detail", args=(issue.id,)) + + client.login(owner) + data = {"version": issue.version, "watchers": [watcher.id]} + response = client.json.patch(url, json.dumps(data)) + + assert response.status_code == 200 + assert watcher.id in response.data['watchers'] diff --git a/tests/integration/test_watch_tasks.py b/tests/integration/test_watch_tasks.py index 4b6d55886..350be5dfa 100644 --- a/tests/integration/test_watch_tasks.py +++ b/tests/integration/test_watch_tasks.py @@ -135,3 +135,28 @@ def test_remove_task_watcher(client): assert response.status_code == 200 assert response.data['watchers'] == [] assert response.data['is_watcher'] == False + + +def test_create_task_with_watchers_via_api(client): + """ + Regression test: watchers specified at task creation time via the API + must be persisted (same root bug as issues). + """ + owner = f.UserFactory.create() + watcher = f.UserFactory.create() + project = f.ProjectFactory.create(owner=owner) + f.MembershipFactory.create(project=project, user=owner, is_admin=True) + f.MembershipFactory.create(project=project, user=watcher, is_admin=True) + url = reverse("tasks-list") + + data = { + "subject": "Task with watchers", + "project": project.id, + "watchers": [watcher.id], + } + client.login(owner) + response = client.json.post(url, json.dumps(data)) + + assert response.status_code == 201 + assert watcher.id in response.data['watchers'] + assert response.data['total_watchers'] >= 1 diff --git a/tests/integration/test_watch_userstories.py b/tests/integration/test_watch_userstories.py index a088d55ff..c4a38fc99 100644 --- a/tests/integration/test_watch_userstories.py +++ b/tests/integration/test_watch_userstories.py @@ -135,3 +135,28 @@ def test_remove_user_story_watcher(client): assert response.status_code == 200 assert response.data['watchers'] == [] assert response.data['is_watcher'] == False + + +def test_create_userstory_with_watchers_via_api(client): + """ + Regression test: watchers specified at user story creation time via the API + must be persisted (same root bug as issues). + """ + owner = f.UserFactory.create() + watcher = f.UserFactory.create() + project = f.ProjectFactory.create(owner=owner) + f.MembershipFactory.create(project=project, user=owner, is_admin=True) + f.MembershipFactory.create(project=project, user=watcher, is_admin=True) + url = reverse("userstories-list") + + data = { + "subject": "User story with watchers", + "project": project.id, + "watchers": [watcher.id], + } + client.login(owner) + response = client.json.post(url, json.dumps(data)) + + assert response.status_code == 201 + assert watcher.id in response.data['watchers'] + assert response.data['total_watchers'] >= 1 diff --git a/tests/unit/test_watched_resource_serializer.py b/tests/unit/test_watched_resource_serializer.py new file mode 100644 index 000000000..a9d844965 --- /dev/null +++ b/tests/unit/test_watched_resource_serializer.py @@ -0,0 +1,246 @@ +# -*- coding: utf-8 -*- +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this +# file, You can obtain one at http://mozilla.org/MPL/2.0/. +# +# Copyright (c) 2021-present Kaleidos INC + +""" +Unit tests for EditableWatchedResourceSerializer watcher creation fix. + +These tests verify the serializer-level logic in isolation using mocks, +without needing HTTP requests or a real database. + +Bug fixed: watchers passed at creation time were silently discarded because +restore_object() returned early when instance=None (new object), before +add_watcher() could be called. +""" + +import pytest +from unittest.mock import MagicMock, patch, call + +from taiga.projects.notifications.mixins import EditableWatchedResourceSerializer, WatchedResourceMixin + + +class TestRestoreObjectWatcherDeferral: + """ + Unit tests for EditableWatchedResourceSerializer.restore_object(). + + Verifies that: + - On CREATE (instance=None): watcher IDs are stashed in _pending_watcher_ids + instead of being discarded. + - On UPDATE (instance is an object): watchers are applied immediately. + - When no watchers are provided: nothing is stashed or applied. + """ + + def _make_serializer(self): + """Build a minimal concrete subclass of EditableWatchedResourceSerializer.""" + serializer = EditableWatchedResourceSerializer.__new__(EditableWatchedResourceSerializer) + serializer.fields = {} + # validate_watchers is contributed by WatchersValidator at the concrete + # subclass level (e.g. IssueValidator). Provide a no-op here so we can + # test the serializer in isolation. + serializer.validate_watchers = lambda attrs, source: attrs + return serializer + + def test_restore_object_create_stashes_pending_watcher_ids(self): + """ + On creation (instance=None), watchers must be stashed in + _pending_watcher_ids and NOT discarded. + """ + serializer = self._make_serializer() + fake_obj = MagicMock() + watcher_ids = [1, 2, 3] + + with patch( + 'taiga.base.api.serializers.ModelSerializer.restore_object', + return_value=fake_obj + ): + attrs = {'watchers': watcher_ids, 'subject': 'Test'} + result = serializer.restore_object(attrs, instance=None) + + # Object is returned + assert result is fake_obj + # Watcher IDs were stashed, not lost + assert hasattr(serializer, '_pending_watcher_ids') + assert set(serializer._pending_watcher_ids) == set(watcher_ids) + # watchers must be removed from attrs before super() call (not a real model field) + assert 'watchers' not in attrs + + def test_restore_object_create_no_watchers_nothing_stashed(self): + """ + On creation with no watchers field, _pending_watcher_ids must not be set. + """ + serializer = self._make_serializer() + fake_obj = MagicMock() + + with patch( + 'taiga.base.api.serializers.ModelSerializer.restore_object', + return_value=fake_obj + ): + attrs = {'subject': 'Test'} # no 'watchers' key + result = serializer.restore_object(attrs, instance=None) + + assert result is fake_obj + # Nothing was stashed + assert not getattr(serializer, '_pending_watcher_ids', None) + + def test_restore_object_update_applies_watchers_immediately(self): + """ + On update (instance is not None), watchers must be applied immediately + via add_watcher / remove_watcher — not deferred. + """ + serializer = self._make_serializer() + fake_instance = MagicMock() + fake_obj = MagicMock() + + existing_user = MagicMock(id=10) + fake_obj.get_watchers.return_value.values_list = MagicMock(return_value=[10]) + + new_user = MagicMock(id=20) + + with patch( + 'taiga.base.api.serializers.ModelSerializer.restore_object', + return_value=fake_obj + ), patch( + 'taiga.projects.notifications.mixins.get_user_model' + ) as mock_get_user_model, patch( + 'taiga.projects.notifications.mixins.services.add_watcher' + ) as mock_add_watcher, patch( + 'taiga.projects.notifications.mixins.services.remove_watcher' + ) as mock_remove_watcher: + + mock_get_user_model.return_value.objects.filter.side_effect = [ + [new_user], # adding_users + [], # removing_users + ] + + attrs = {'watchers': [20], 'subject': 'Test'} + serializer.restore_object(attrs, instance=fake_instance) + + # add_watcher was called for the new watcher + mock_add_watcher.assert_called_once_with(fake_obj, new_user) + # Nothing was deferred + assert not getattr(serializer, '_pending_watcher_ids', None) + + +class TestSaveAppliesPendingWatchers: + """ + Unit tests for EditableWatchedResourceSerializer.save(). + + Verifies that pending watchers stashed by restore_object() are correctly + applied after the object is persisted (has a PK). + """ + + def _make_serializer_with_pending(self, pending_ids): + serializer = EditableWatchedResourceSerializer.__new__(EditableWatchedResourceSerializer) + serializer.fields = {} + serializer._pending_watcher_ids = list(pending_ids) + return serializer + + def test_save_applies_pending_watcher_ids(self): + """ + After save(), watchers that were deferred during create must be + applied via services.add_watcher(). + """ + watcher_user = MagicMock(id=5) + saved_obj = MagicMock() + saved_obj.get_watchers.return_value = [watcher_user] + serializer = self._make_serializer_with_pending([5]) + + with patch( + 'taiga.base.api.serializers.ModelSerializer.save', + return_value=saved_obj + ), patch( + 'taiga.projects.notifications.mixins.get_user_model' + ) as mock_get_user_model, patch( + 'taiga.projects.notifications.mixins.services.add_watcher' + ) as mock_add_watcher: + + mock_get_user_model.return_value.objects.filter.return_value = [watcher_user] + + result = serializer.save() + + # add_watcher was called with the persisted object and the user + mock_add_watcher.assert_called_once_with(saved_obj, watcher_user) + # Pending list is cleared after processing + assert serializer._pending_watcher_ids is None + # Returned object has watchers attribute set + assert result is saved_obj + + def test_save_no_pending_watchers_does_not_call_add_watcher(self): + """ + If no watchers were deferred (normal case without watchers in payload), + add_watcher must NOT be called. + """ + saved_obj = MagicMock() + saved_obj.get_watchers.return_value = [] + serializer = EditableWatchedResourceSerializer.__new__(EditableWatchedResourceSerializer) + serializer.fields = {} + # No _pending_watcher_ids set + + with patch( + 'taiga.base.api.serializers.ModelSerializer.save', + return_value=saved_obj + ), patch( + 'taiga.projects.notifications.mixins.services.add_watcher' + ) as mock_add_watcher: + + serializer.save() + + mock_add_watcher.assert_not_called() + + def test_save_clears_pending_after_applying(self): + """ + _pending_watcher_ids must be reset to None after save() applies them, + so a second save() call does not re-apply watchers. + """ + watcher_user = MagicMock(id=7) + saved_obj = MagicMock() + saved_obj.get_watchers.return_value = [watcher_user] + serializer = self._make_serializer_with_pending([7]) + + with patch( + 'taiga.base.api.serializers.ModelSerializer.save', + return_value=saved_obj + ), patch( + 'taiga.projects.notifications.mixins.get_user_model' + ) as mock_get_user_model, patch( + 'taiga.projects.notifications.mixins.services.add_watcher' + ): + mock_get_user_model.return_value.objects.filter.return_value = [watcher_user] + serializer.save() + + assert serializer._pending_watcher_ids is None + + +class TestOldWatchersDefault: + """ + Unit tests for WatchedResourceMixin._old_watchers default value. + + Before the fix, _old_watchers = None caused a TypeError in + create_web_notifications_for_added_watchers() during creation: + 'watcher_id not in None' → TypeError + """ + + def test_old_watchers_default_is_empty_list_not_none(self): + """ + _old_watchers must default to [] so membership tests + (watcher_id not in self._old_watchers) never raise TypeError. + """ + assert WatchedResourceMixin._old_watchers == [] + assert WatchedResourceMixin._old_watchers is not None + + def test_old_watchers_supports_membership_test(self): + """ + Membership test on the default value must not raise TypeError. + """ + mixin = WatchedResourceMixin() + try: + result = 99 not in mixin._old_watchers + except TypeError: + pytest.fail( + "_old_watchers default caused TypeError on membership test. " + "It must be [] not None." + ) + assert result is True