diff --git a/docs/project-default-cover-reset.md b/docs/project-default-cover-reset.md new file mode 100644 index 00000000..3c85ab75 --- /dev/null +++ b/docs/project-default-cover-reset.md @@ -0,0 +1,127 @@ +# Возврат стандартной обложки legacy-проекта + +База DEV: `8ed5670d2123dd1f1b53668567ad60bde8641bcc`. +Задача относится к Angular и `Project`; модели и контракты React не изменены. + +## Причина + +Пользователь подтвердил на DEV попытку удалить стандартную обложку через +`DELETE /files/?link=...` и ответ 403: её `UserFile` принадлежит другому пользователю. +Второй конфликт — физическое удаление пользовательской обложки очищает обязательный +Angular-control. Глобальное удаление файлов и `Project.save()` не меняются. +На живом DEV автор этой правки удаление не выполнял. Сохранность общего файла и 403 +для чужого `UserFile` дополнительно проверены локальными API-тестами. + +## Контракт + +`POST /projects/{project_id}/reset-cover/`, тело `{}`. ID относится к `Project`, +а не `PartnerProgramProject`. + +Успех 200: + +```json +{ + "cover_image_address": "https://cdn.example/default.png", + "is_default_cover": true +} +``` + +Если доступных записей с непустой ссылкой `DefaultProjectCover.image` нет: + +```json +{ + "code": "default_cover_unavailable", + "detail": "Стандартная обложка временно недоступна." +} +``` + +Статус 409; проект и прежний файл сохраняются. Проверка доступности означает +наличие ссылки в БД: отдельный сетевой HEAD в CDN не выполняется. +Уже стандартная обложка возвращается без переназначения и удаления. +Сокращённые списки проектов сохраняют прежний контракт. + +`GET /projects/{id}/` и ответы изменения проекта содержат read-only +`is_default_cover`. Источник — точное совпадение URL с `DefaultProjectCover.image_id`. +Сериализатор загружает набор ссылок один раз на экземпляр, в том числе при `many=True`. +Это не глобальный кеш; после PUT проверяется фактическое значение, включая fallback +существующего `Project.save()`. + +## Права и порядок записи + +Команда использует непосредственно `ProjectDetail.permission_classes`: +`ProjectVisibilityPermission`, `HasInvolvementInProjectOrReadOnly`, +`TimingAfterEndsProgramPermission`. Object permissions проверяются на заблокированном +`select_for_update()` проекте внутри транзакции. + +- Лидер сохраняет прежние права. +- Collaborator/invite могут менять только доступный черновик по прежним правилам. +- Менеджер, эксперт или администратор не получают право записи только из-за роли. +- Сохраняется временной запрет редактирования после завершения программы. + +Сначала записываются новая обложка и время изменения проекта. Только после коммита +`on_commit` запускает best-effort очистку. При откате транзакции очистки нет. + +| Прежний файл | Поведение | +| --- | --- | +| Стандартная обложка | Идемпотентный ответ, без удаления | +| Собственный свободный `UserFile` | После коммита — CDN DELETE, затем удаление записи | +| Чужой `UserFile` | Сброс разрешён по правам проекта; файл сохраняется | +| Запись файла отсутствует | Сброс успешен, CDN не вызывается | +| Системный аватар или файл, используемый другим проектом | Очистка пропускается | +| CDN возвращает 404 | Очистка считается идемпотентной | +| CDN/network/БД очистки выбрасывает ошибку | Новая обложка сохраняется, ответ остаётся 200, запись файла остаётся при сбое CDN | + +Предупреждение очистки не содержит URL, пользователя, HTTP body или текста исключения. +Автоматическая очередь повторной очистки не добавлялась. Неудачная очистка может +оставить неиспользуемый файл; она не возвращает проекту удалённую обложку. +Общие права `/files/` и правила публикации/сдачи проекта не изменены. + +## Проверки + +Локально: Python 3.11, PostgreSQL, изолированная конфигурация на основе +`procollab.settings_ci`, тестовая БД `test_procollab_cover_reset_20260917`. +Зависимости, workflows и миграции не менялись. + +| Команда | Фактический результат | +| --- | --- | +| `python manage.py test projects.tests.test_project_cover_reset --verbosity 2 --keepdb` | 18/18, exit 0 | +| `python manage.py test projects.tests.test_project_cover_reset projects.tests.test_program_project_detail_access projects.tests.test_project_access --verbosity 1 --keepdb` | 38/38, exit 0 | +| `python manage.py test --verbosity 1 --keepdb` | 867 тестов, 1 failure в существующем `feed.tests.test_feed_api.FeedAPITests.test_feed_returns_project_news_as_news_content` | +| Та же полная команда на неизменённой базе | 849/849, exit 0 | +| 30 повторов упавшего теста на неизменённой базе | 2 failures с тем же `project != news`; нестабильность воспроизведена вне изменений | +| `python manage.py check` / `check --tag models` | Без замечаний | +| `python manage.py makemigrations --check --dry-run` | No changes detected | +| `python -m black --check procollab/settings_ci.py projects/cover_reset.py projects/views.py projects/urls.py projects/serializers.py projects/tests/test_project_cover_reset.py` | Успешно | +| `python -m flake8` для того же списка | Успешно | +| `git diff --check` | Успешно | + +Тесты проверяют матрицу прав сравнением с обычным PUT, сохранность собственных, +чужих и системных файлов, выполнение очистки после фиксации, отсутствие очистки +при rollback, корректный флаг и ровно один добавочный запрос для списка любой длины. +Результат штатного PostgreSQL CI GitHub фиксируется отдельно в Draft PR. + +## Реальная локальная интеграция с Angular + +Отдельная БД `procollab_cover_reset_20260917` создана из предыдущего синтетического +локального стенда. Никаких DEV/PROD данных. Работали настоящий Angular, Django, +JWT, `Project`, `UserFile`, связи программы и права доступа. Только внешний CDN +заменён локальным файловым транспортом; его сбой задавался явно. + +| Сценарий | Результат | +| --- | --- | +| A: стандартная обложка | В редакторе нет удаления/сброса; DELETE отсутствует | +| B: upload → save → reopen | POST файла 201, PUT реального проекта 200; GET возвращает пользовательский URL и false | +| C: reset → GET → reopen | POST 200; фактическая стандартная ссылка и true; прежний собственный файл удалён после смены ссылки | +| D: чужой файл при законном редактировании | POST 200; стандартная ссылка; `UserFile` другого владельца сохранён | +| E: нет DefaultProjectCover | POST 409; понятная ошибка UI; GET сохраняет прежний URL и false | +| F: отказ CDN после переключения | POST 200; GET возвращает стандартную ссылку и true; прежний файл сохранён, предупреждение записано | + +В браузерной цепочке A–F не было `DELETE /files/`, `/projects/NaN/`, `/0/`, `/null/` +и ответов 5xx. Кейс связи 73 с программой 21 сохранился при сохранении проекта 31. +Скриншоты и обезличенный сетевой протокол находятся в Angular Draft PR этой задачи. + +## Зависимость для будущего DEV-развёртывания + +Сначала нужен этот backend-контракт, затем Angular-изменение, использующее +`is_default_cover` и reset-cover. Без флага frontend не предлагает опасное удаление. +Сейчас подготовлены только Draft PR; merge и deploy не выполнялись. diff --git a/projects/cover_reset.py b/projects/cover_reset.py new file mode 100644 index 00000000..c8d6eaa4 --- /dev/null +++ b/projects/cover_reset.py @@ -0,0 +1,85 @@ +"""Сброс обложки legacy-проекта без изменения глобальных прав на файлы.""" + +import logging +from functools import partial + +from django.db import transaction +from django.db.models import Q +from rest_framework.exceptions import APIException + +from files.models import UserFile +from files.service import CDN, SelectelSwiftStorage +from projects.models import DefaultProjectAvatar, DefaultProjectCover, Project + +logger = logging.getLogger(__name__) +cdn = CDN(storage=SelectelSwiftStorage()) + + +class DefaultCoverUnavailable(APIException): + status_code = 409 + default_detail = { + "code": "default_cover_unavailable", + "detail": "Стандартная обложка временно недоступна.", + } + + +def reset_project_cover(project: Project, user_id: int) -> dict: + """Назначает обложку уже заблокированному и проверенному на запись проекту. + + Вызывается внутри atomic после проверки обычных прав ProjectDetail. + Очистка запускается только после коммита: сбой CDN не должен вернуть + проекту старый URL или представить успешный сброс как ошибку. + """ + old_url = project.cover_image_address + defaults = DefaultProjectCover.objects.filter(image__isnull=False).exclude( + image_id="" + ) + if defaults.filter(image_id=old_url).exists(): + return {"cover_image_address": old_url, "is_default_cover": True} + + default_url = defaults.order_by("?").values_list("image_id", flat=True).first() + if not default_url: + raise DefaultCoverUnavailable() + + project.cover_image_address = default_url + project.save(update_fields=["cover_image_address", "datetime_updated"]) + transaction.on_commit(partial(cleanup_previous_cover, old_url, user_id)) + return {"cover_image_address": default_url, "is_default_cover": True} + + +def cleanup_previous_cover(old_url: str | None, user_id: int) -> None: + """Удаляет только собственный неиспользуемый файл после фиксации сброса. + + Владение проектом не даёт владения UserFile. Системные обложки/аватары + и файлы других проектов сохраняются даже при совпадении владельца. + Исключения и HTTP-ошибки CDN не откатывают смену обложки; URL и текст + исключений не попадают в лог, поскольку могут содержать закрытые данные. + """ + if not old_url: + return + try: + with transaction.atomic(): + old_file = ( + UserFile.objects.select_for_update() + .filter(link=old_url, user_id=user_id) + .first() + ) + if old_file is None: + return + if ( + DefaultProjectCover.objects.filter(image_id=old_url).exists() + or DefaultProjectAvatar.objects.filter(image_id=old_url).exists() + or Project.objects.filter( + Q(cover_image_address=old_url) + | Q(image_address=old_url) + | Q(presentation_address=old_url) + | Q(cover_id=old_url) + ).exists() + ): + return + response = cdn.delete(old_url) + if response.status_code != 404: + response.raise_for_status() + old_file.delete() + except Exception: + logger.warning("Не удалось очистить прежний файл после сброса обложки проекта.") diff --git a/projects/serializers.py b/projects/serializers.py index 0209b114..dc6f5aef 100644 --- a/projects/serializers.py +++ b/projects/serializers.py @@ -235,6 +235,7 @@ def update(self, instance, validated_data): class ProjectDetailSerializer(serializers.ModelSerializer): + is_default_cover = serializers.SerializerMethodField() achievements = AchievementListSerializer(many=True, read_only=True) cover = UserFileSerializer(required=False) collaborators = CollaboratorSerializer( @@ -260,6 +261,23 @@ class ProjectDetailSerializer(serializers.ModelSerializer): implementation_deadline = serializers.DateField(required=False, allow_null=True) trl = serializers.IntegerField(required=False, allow_null=True) + def get_is_default_cover(self, project): + """Сверяет URL с системными обложками, а не с владельцем или доменом. + + Набор живёт только в этом экземпляре сериализатора: один запрос даже + при many=True, без глобального устаревающего кеша и N+1 на новый флаг. + После PUT проверяется фактический URL, назначенный Project.save(). + """ + from projects.models import DefaultProjectCover + + if not hasattr(self, "_default_cover_urls"): + self._default_cover_urls = set( + DefaultProjectCover.objects.filter(image__isnull=False) + .exclude(image_id="") + .values_list("image_id", flat=True) + ) + return project.cover_image_address in self._default_cover_urls + def get_partner_program(self, project): """Keep the legacy singular contract: choose the earliest link by pk.""" link = ( @@ -322,6 +340,7 @@ class Meta: "is_public", "cover", "cover_image_address", + "is_default_cover", "actuality", "problem", "target_audience", diff --git a/projects/tests/test_project_cover_reset.py b/projects/tests/test_project_cover_reset.py new file mode 100644 index 00000000..128df9e8 --- /dev/null +++ b/projects/tests/test_project_cover_reset.py @@ -0,0 +1,241 @@ +"""Права legacy-проекта и сохранность файлов при возврате стандартной обложки.""" + +from unittest.mock import Mock, patch + +from django.db import connection, transaction +from django.test import TestCase +from django.test.utils import CaptureQueriesContext +from rest_framework.test import APIClient + +from files.models import UserFile +from invites.models import Invite +from project_rates.tests.helpers import create_rate_expert +from projects.cover_reset import cleanup_previous_cover, reset_project_cover +from projects.models import DefaultProjectCover, Project +from projects.serializers import ProjectDetailSerializer +from projects.views import ProjectDetail, ProjectResetCover + +from .helpers import ( + add_program_member, + create_collaborator, + create_partner_program, + create_project, + create_user, + link_project_to_program, +) + + +class ProjectCoverResetTests(TestCase): + @classmethod + def setUpTestData(cls): + cls.project = create_project(is_public=False) + cls.leader = cls.project.leader + cls.other = create_user() + cls.custom = UserFile.objects.create( + link="https://files.example.com/custom.png", user=cls.leader + ) + cls.default = UserFile.objects.create( + link="https://files.example.com/default.png", user=cls.other + ) + DefaultProjectCover.objects.create(image=cls.default) + cls.project.cover_image_address = cls.custom.link + cls.project.save(update_fields=["cover_image_address"]) + + def setUp(self): + self.client = APIClient() + self.client.force_authenticate(self.leader) + self.url = f"/projects/{self.project.pk}/reset-cover/" + self.detail_url = f"/projects/{self.project.pk}/" + self.cdn_patch = patch("projects.cover_reset.cdn") + self.cdn = self.cdn_patch.start() + self.addCleanup(self.cdn_patch.stop) + self.cdn.delete.return_value = Mock(status_code=204) + + def reset(self): + with self.captureOnCommitCallbacks(execute=True): + return self.client.post(self.url, {}, format="json") + + def test_custom_reset_commits_before_owned_file_cleanup(self): + def delete_after_switch(url): + self.project.refresh_from_db() + self.assertEqual(self.project.cover_image_address, self.default.link) + return Mock(status_code=204) + + self.cdn.delete.side_effect = delete_after_switch + response = self.reset() + self.assertEqual(response.status_code, 200, response.data) + self.assertEqual( + response.data, + {"cover_image_address": self.default.link, "is_default_cover": True}, + ) + self.cdn.delete.assert_called_once_with(self.custom.link) + self.assertFalse(UserFile.objects.filter(pk=self.custom.pk).exists()) + self.assertTrue(UserFile.objects.filter(pk=self.default.pk).exists()) + self.assertTrue(self.client.get(self.detail_url).data["is_default_cover"]) + + def test_default_reset_is_idempotent_even_when_file_owned_by_caller(self): + UserFile.objects.filter(pk=self.default.pk).update(user=self.leader) + Project.objects.filter(pk=self.project.pk).update( + cover_image_address=self.default.link + ) + for _ in range(2): + response = self.reset() + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["cover_image_address"], self.default.link) + self.cdn.delete.assert_not_called() + self.assertTrue(DefaultProjectCover.objects.filter(image=self.default).exists()) + self.assertTrue(UserFile.objects.filter(pk=self.default.pk).exists()) + + def test_no_available_default_returns_controlled_conflict_without_changes(self): + DefaultProjectCover.objects.all().delete() + DefaultProjectCover.objects.create(image=None) + response = self.reset() + self.assertEqual(response.status_code, 409) + self.assertEqual(response.data["code"], "default_cover_unavailable") + self.project.refresh_from_db() + self.assertEqual(self.project.cover_image_address, self.custom.link) + self.cdn.delete.assert_not_called() + self.assertTrue(UserFile.objects.filter(pk=self.custom.pk).exists()) + + def test_unavailable_rows_do_not_hide_an_available_default(self): + DefaultProjectCover.objects.create(image=None) + self.assertEqual(self.reset().status_code, 200) + + def test_foreign_file_is_preserved(self): + UserFile.objects.filter(pk=self.custom.pk).update(user=self.other) + self.assertEqual(self.reset().status_code, 200) + self.cdn.delete.assert_not_called() + self.assertTrue(UserFile.objects.filter(pk=self.custom.pk).exists()) + + def test_missing_old_file_does_not_prevent_reset(self): + self.custom.delete() + self.assertEqual(self.reset().status_code, 200) + self.cdn.delete.assert_not_called() + + def test_file_referenced_by_another_project_is_preserved(self): + other_project = create_project(leader=self.leader) + Project.objects.filter(pk=other_project.pk).update( + cover_image_address=self.custom.link + ) + self.assertEqual(self.reset().status_code, 200) + self.cdn.delete.assert_not_called() + self.assertTrue(UserFile.objects.filter(pk=self.custom.pk).exists()) + + def test_cleanup_network_failure_keeps_success_and_does_not_log_exception(self): + self.cdn.delete.side_effect = RuntimeError("private-url-or-token") + with self.assertLogs("projects.cover_reset", level="WARNING") as logs: + response = self.reset() + self.assertEqual(response.status_code, 200) + self.project.refresh_from_db() + self.assertEqual(self.project.cover_image_address, self.default.link) + self.assertTrue(UserFile.objects.filter(pk=self.custom.pk).exists()) + self.assertNotIn("private-url-or-token", " ".join(logs.output)) + + def test_cleanup_http_failure_keeps_success_and_file_record(self): + self.cdn.delete.return_value.raise_for_status.side_effect = RuntimeError() + with self.assertLogs("projects.cover_reset", level="WARNING"): + self.assertEqual(self.reset().status_code, 200) + self.assertTrue(UserFile.objects.filter(pk=self.custom.pk).exists()) + self.project.refresh_from_db() + self.assertEqual(self.project.cover_image_address, self.default.link) + + def test_cleanup_404_is_idempotent(self): + self.cdn.delete.return_value = Mock(status_code=404) + self.assertEqual(self.reset().status_code, 200) + self.assertFalse(UserFile.objects.filter(pk=self.custom.pk).exists()) + + def test_cleanup_rechecks_system_file_status(self): + DefaultProjectCover.objects.create(image=self.custom) + cleanup_previous_cover(self.custom.link, self.leader.pk) + self.cdn.delete.assert_not_called() + self.assertTrue(UserFile.objects.filter(pk=self.custom.pk).exists()) + + def test_rollback_never_runs_cleanup(self): + with self.captureOnCommitCallbacks(execute=True) as callbacks: + with self.assertRaises(RuntimeError): + with transaction.atomic(): + reset_project_cover(self.project, self.leader.pk) + raise RuntimeError("rollback") + self.assertEqual(callbacks, []) + self.cdn.delete.assert_not_called() + self.project.refresh_from_db() + self.assertEqual(self.project.cover_image_address, self.custom.link) + + def test_permissions_are_identical_to_project_update(self): + self.assertIs( + ProjectResetCover.permission_classes, ProjectDetail.permission_classes + ) + collaborator = create_collaborator(self.project).user + invited = create_user() + Invite.objects.create(project=self.project, user=invited) + program = create_partner_program() + link_project_to_program(self.project, program) + manager = create_user() + program.managers.add(manager) + expert = create_rate_expert(program=program) + for draft in (True, False): + for user in (self.leader, collaborator, invited, manager, expert, self.other): + with self.subTest(draft=draft, user=user.pk): + Project.objects.filter(pk=self.project.pk).update(draft=draft) + self.client.force_authenticate(user) + expected = ( + 200 + if user == self.leader + or (draft and user in (collaborator, invited)) + else 403 + ) + update = self.client.put( + self.detail_url, + {"name": "Проверка прав", "draft": draft}, + format="json", + ) + self.assertEqual(update.status_code, expected, update.data) + reset = self.reset() + self.assertEqual(reset.status_code, expected, reset.data) + + def test_program_timing_restriction_remains_in_force(self): + program = create_partner_program(finished=True) + add_program_member(program, self.leader, project=self.project) + self.assertEqual(self.reset().status_code, 403) + self.project.refresh_from_db() + self.assertEqual(self.project.cover_image_address, self.custom.link) + + def test_anonymous_and_missing_project(self): + self.client.force_authenticate(None) + self.assertIn(self.reset().status_code, (401, 403)) + self.client.force_authenticate(self.leader) + self.assertEqual( + self.client.post("/projects/99999999/reset-cover/").status_code, 404 + ) + + def test_global_delete_foreign_file_still_forbidden(self): + response = self.client.delete(f"/files/?link={self.default.link}") + self.assertEqual(response.status_code, 403) + self.assertTrue(UserFile.objects.filter(pk=self.default.pk).exists()) + + def test_detail_flag_tracks_actual_url_and_is_read_only(self): + self.assertFalse(self.client.get(self.detail_url).data["is_default_cover"]) + response = self.client.put( + self.detail_url, + { + "draft": True, + "cover_image_address": self.default.link, + "is_default_cover": False, + }, + format="json", + ) + self.assertEqual(response.status_code, 200) + self.assertTrue(response.data["is_default_cover"]) + Project.objects.filter(pk=self.project.pk).update(cover_image_address=None) + self.assertFalse(self.client.get(self.detail_url).data["is_default_cover"]) + + def test_detail_many_adds_one_default_query_independent_of_list_size(self): + projects = [self.project] + [create_project() for _ in range(4)] + for count in (1, 5): + with self.subTest(count=count), CaptureQueriesContext(connection) as queries: + data = ProjectDetailSerializer(projects[:count], many=True).data + self.assertEqual(len(data), count) + default_queries = [ + q for q in queries if "projects_defaultprojectcover" in q["sql"] + ] + self.assertEqual(len(default_queries), 1) diff --git a/projects/urls.py b/projects/urls.py index b9810e61..a7319e18 100644 --- a/projects/urls.py +++ b/projects/urls.py @@ -16,6 +16,7 @@ ProjectDetail, ProjectList, ProjectRecommendedUsers, + ProjectResetCover, ProjectSubscribe, ProjectSubscribers, ProjectUnsubscribe, @@ -57,6 +58,7 @@ } ) urlpatterns = [ + path("/reset-cover/", ProjectResetCover.as_view(), name="reset-cover"), path("", ProjectList.as_view()), path("/like/", SetLikeOnProject.as_view()), path("/news/", NewsList.as_view()), diff --git a/projects/views.py b/projects/views.py index 8a16f146..4e22b1d7 100644 --- a/projects/views.py +++ b/projects/views.py @@ -24,6 +24,7 @@ PartnerProgramUserProfile, ) from projects.exceptions import CollaboratorDoesNotExist +from projects.cover_reset import reset_project_cover from projects.filters import ProjectFilter from projects.helpers import ( check_related_fields_update, @@ -201,6 +202,21 @@ def patch(self, request, pk, **kwargs): return super(ProjectDetail, self).put(request, pk) +class ProjectResetCover(generics.GenericAPIView): + """Команда Angular-редактора с теми же ограничениями, что и запись проекта.""" + + queryset = Project.objects.all() + permission_classes = ProjectDetail.permission_classes + serializer_class = EmptySerializer + + @transaction.atomic + def post(self, request, pk): + """Проверяет права на актуальном проекте и сохраняет обложку до очистки файла.""" + project = get_object_or_404(self.get_queryset().select_for_update(), pk=pk) + self.check_object_permissions(request, project) + return Response(reset_project_cover(project, request.user.id)) + + class ProjectRecommendedUsers(generics.RetrieveAPIView): queryset = Project.objects.all() permission_classes = [IsProjectLeader] @@ -261,7 +277,10 @@ class ProjectCollaborators(generics.GenericAPIView): Project collaborator retrieve/add/delete view """ - permission_classes = [ProjectVisibilityPermission, IsProjectLeaderOrReadOnlyForNonDrafts] + permission_classes = [ + ProjectVisibilityPermission, + IsProjectLeaderOrReadOnlyForNonDrafts, + ] queryset = Project.objects.all() serializer_class = ProjectCollaboratorSerializer @@ -588,7 +607,9 @@ def _copy_collaborators(original_project: Project, new_project: Project) -> None collaborators_to_create: list[Collaborator] = [] leader_collaborator = None - for collaborator in original_project.collaborator_set.select_related("user").all(): + for collaborator in original_project.collaborator_set.select_related( + "user" + ).all(): if collaborator.user_id == leader_id: leader_collaborator = collaborator continue