diff --git a/contentcuration/contentcuration/frontend/shared/data/serverSync.js b/contentcuration/contentcuration/frontend/shared/data/serverSync.js index 68b0e7f02b..ad814f9349 100644 --- a/contentcuration/contentcuration/frontend/shared/data/serverSync.js +++ b/contentcuration/contentcuration/frontend/shared/data/serverSync.js @@ -331,6 +331,8 @@ const debouncedSyncChanges = debounce(() => { if (!syncActive) { return syncChanges(); } + // TODO: actually return promise that resolves when active sync completes + return new Promise(resolve => setTimeout(resolve, 1000)); }, SYNC_IF_NO_CHANGES_FOR * 1000); if (process.env.NODE_ENV !== 'production' && typeof window !== 'undefined') { @@ -391,6 +393,9 @@ export function stopSyncing() { db.on('changes').unsubscribe(handleChanges); } +/** + * @return {Promise} + */ export function forceServerSync() { debouncedSyncChanges(); return debouncedSyncChanges.flush(); diff --git a/contentcuration/contentcuration/migrations/0142_remove_file_file_media_duration_int.py b/contentcuration/contentcuration/migrations/0142_remove_file_file_media_duration_int.py new file mode 100644 index 0000000000..e497fbd398 --- /dev/null +++ b/contentcuration/contentcuration/migrations/0142_remove_file_file_media_duration_int.py @@ -0,0 +1,16 @@ +# Generated by Django 3.2.18 on 2023-04-26 18:55 +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('contentcuration', '0141_add_task_signature'), + ] + + operations = [ + migrations.RemoveConstraint( + model_name='file', + name='file_media_duration_int', + ), + ] diff --git a/contentcuration/contentcuration/migrations/0143_file_file_media_duration_int.py b/contentcuration/contentcuration/migrations/0143_file_file_media_duration_int.py new file mode 100644 index 0000000000..3a7dbae1a0 --- /dev/null +++ b/contentcuration/contentcuration/migrations/0143_file_file_media_duration_int.py @@ -0,0 +1,17 @@ +# Generated by Django 3.2.18 on 2023-05-03 15:28 +from django.db import migrations +from django.db import models + + +class Migration(migrations.Migration): + + dependencies = [ + ('contentcuration', '0142_remove_file_file_media_duration_int'), + ] + + operations = [ + migrations.AddConstraint( + model_name='file', + constraint=models.CheckConstraint(check=models.Q(models.Q(('duration__gt', 0), ('preset__in', ['audio', 'audio_dependency', 'high_res_video', 'low_res_video', 'video_dependency'])), ('duration__isnull', True), _connector='OR'), name='file_media_duration_int'), + ), + ] diff --git a/contentcuration/contentcuration/models.py b/contentcuration/contentcuration/models.py index 6aa8ca4eef..133aebcb0c 100644 --- a/contentcuration/contentcuration/models.py +++ b/contentcuration/contentcuration/models.py @@ -75,7 +75,6 @@ from contentcuration.db.models.manager import CustomManager from contentcuration.statistics import record_channel_stats from contentcuration.utils.cache import delete_public_channel_cache_keys -from contentcuration.utils.celery.tasks import generate_task_signature from contentcuration.utils.parser import load_json_string from contentcuration.viewsets.sync.constants import ALL_CHANGES from contentcuration.viewsets.sync.constants import ALL_TABLES @@ -2077,7 +2076,13 @@ class StagedFile(models.Model): FILE_DISTINCT_INDEX_NAME = "file_checksum_file_size_idx" FILE_MODIFIED_DESC_INDEX_NAME = "file_modified_desc_idx" FILE_DURATION_CONSTRAINT = "file_media_duration_int" -MEDIA_PRESETS = [format_presets.AUDIO, format_presets.VIDEO_HIGH_RES, format_presets.VIDEO_LOW_RES] +MEDIA_PRESETS = [ + format_presets.AUDIO, + format_presets.AUDIO_DEPENDENCY, + format_presets.VIDEO_HIGH_RES, + format_presets.VIDEO_LOW_RES, + format_presets.VIDEO_DEPENDENCY, +] class File(models.Model): @@ -2214,7 +2219,12 @@ class Meta: models.Index(fields=["-modified"], name=FILE_MODIFIED_DESC_INDEX_NAME), ] constraints = [ - models.CheckConstraint(check=(Q(preset__in=MEDIA_PRESETS, duration__gt=0) | Q(duration__isnull=True)), name=FILE_DURATION_CONSTRAINT) + # enforces that duration is null when not a media preset, but the duration may be null for media presets + # but if not-null, should be greater than 0 + models.CheckConstraint( + check=(Q(preset__in=MEDIA_PRESETS, duration__gt=0) | Q(duration__isnull=True)), + name=FILE_DURATION_CONSTRAINT + ) ] @@ -2451,7 +2461,6 @@ class TaskResultCustom(object): signature = models.CharField(null=True, blank=False, max_length=32) super_as_dict = TaskResult.as_dict - super_save = TaskResult.save def as_dict(self): """ @@ -2465,22 +2474,6 @@ def as_dict(self): ) return super_dict - def set_signature(self): - """ - Generates and sets the signature for the task if it isn't set - """ - if self.signature is not None: - # nothing to do - return - self.signature = generate_task_signature(self.task_name, task_kwargs=self.task_kwargs, channel_id=self.channel_id) - - def save(self, *args, **kwargs): - """ - Override save to ensure signature is generated - """ - self.set_signature() - return self.super_save(*args, **kwargs) - @classmethod def contribute_to_class(cls, model_class=TaskResult): """ diff --git a/contentcuration/contentcuration/tests/test_asynctask.py b/contentcuration/contentcuration/tests/test_asynctask.py index bbd2714f47..92bb3fdb4f 100644 --- a/contentcuration/contentcuration/tests/test_asynctask.py +++ b/contentcuration/contentcuration/tests/test_asynctask.py @@ -234,7 +234,7 @@ def test_fetch_or_enqueue_task__channel_id__uuid_then_hex(self): self.assertEqual(expected_task.task_id, async_result.task_id) def test_requeue_task(self): - signature = requeue_test_task._generate_signature({}) + signature = requeue_test_task.generate_signature({}) existing_task_ids = requeue_test_task.find_ids(signature) self.assertEqual(len(existing_task_ids), 0) diff --git a/contentcuration/contentcuration/tests/test_node_views.py b/contentcuration/contentcuration/tests/views/test_nodes.py similarity index 51% rename from contentcuration/contentcuration/tests/test_node_views.py rename to contentcuration/contentcuration/tests/views/test_nodes.py index 922675c721..2367263275 100644 --- a/contentcuration/contentcuration/tests/test_node_views.py +++ b/contentcuration/contentcuration/tests/views/test_nodes.py @@ -1,22 +1,39 @@ -from __future__ import absolute_import - import datetime import json import pytz from django.conf import settings from django.core.cache import cache +from django.urls import reverse +from mock import Mock from mock import patch -from rest_framework.reverse import reverse -from .base import BaseAPITestCase -from .testdata import tree -from contentcuration.models import Channel +from contentcuration.tasks import generatenodediff_task +from contentcuration.tests.base import BaseAPITestCase + + +class NodesViewsTestCase(BaseAPITestCase): + def test_get_node_diff__missing_contentnode(self): + response = self.get(reverse("get_node_diff", kwargs=dict(updated_id="abc123", original_id="def456"))) + self.assertEqual(response.status_code, 404) + + def test_get_node_diff__no_task_processing(self): + pk = self.channel.main_tree.pk + response = self.get(reverse("get_node_diff", kwargs=dict(updated_id=pk, original_id=pk))) + self.assertEqual(response.status_code, 404) + + @patch.object(generatenodediff_task, 'find_incomplete_ids') + def test_get_node_diff__task_processing(self, mock_find_incomplete_ids): + qs = Mock(spec="django.db.models.query.QuerySet") + mock_find_incomplete_ids.return_value = qs() + mock_find_incomplete_ids.return_value.exists.return_value = True + pk = self.channel.main_tree.pk + response = self.get(reverse("get_node_diff", kwargs=dict(updated_id=pk, original_id=pk))) + self.assertEqual(response.status_code, 302) -class NodeViewsUtilityTestCase(BaseAPITestCase): def test_get_channel_details(self): - url = reverse('get_channel_details', [self.channel.id]) + url = reverse('get_channel_details', kwargs={"channel_id": self.channel.id}) response = self.get(url) details = json.loads(response.content) @@ -33,13 +50,13 @@ def test_get_channel_details_cached(self): cache.set(cache_key, json.dumps(data)) with patch("contentcuration.views.nodes.getnodedetails_task") as task_mock: - url = reverse('get_channel_details', [self.channel.id]) + url = reverse('get_channel_details', kwargs={"channel_id": self.channel.id}) self.get(url) # Check that the outdated cache prompts an asynchronous cache update task_mock.enqueue.assert_called_once_with(self.user, node_id=self.channel.main_tree.id) -class GetTopicDetailsEndpointTestCase(BaseAPITestCase): +class ChannelDetailsEndpointTestCase(BaseAPITestCase): def test_200_post(self): response = self.get( reverse("get_channel_details", kwargs={"channel_id": self.channel.id}) @@ -47,10 +64,8 @@ def test_200_post(self): self.assertEqual(response.status_code, 200) def test_404_no_permission(self): - new_channel = Channel.objects.create() - new_channel.main_tree = tree() - new_channel.save() + self.channel.editors.remove(self.user) response = self.get( - reverse("get_channel_details", kwargs={"channel_id": new_channel.id}), + reverse("get_channel_details", kwargs={"channel_id": self.channel.id}), ) self.assertEqual(response.status_code, 404) diff --git a/contentcuration/contentcuration/utils/celery/tasks.py b/contentcuration/contentcuration/utils/celery/tasks.py index 5b55d83bd9..1045be52f2 100644 --- a/contentcuration/contentcuration/utils/celery/tasks.py +++ b/contentcuration/contentcuration/utils/celery/tasks.py @@ -10,6 +10,7 @@ from celery.app.task import Task from celery.result import AsyncResult from django.db import transaction +from django.db.utils import IntegrityError from contentcuration.constants.locking import TASK_LOCK from contentcuration.db.advisory_lock import advisory_lock @@ -139,7 +140,7 @@ def _prepare_kwargs(self, kwargs): for key, value in kwargs.items() ) - def _generate_signature(self, kwargs): + def generate_signature(self, kwargs): """ :param kwargs: A dictionary of task kwargs :return: An hex string representing an md5 hash of task metadata @@ -207,7 +208,7 @@ def enqueue(self, user, **kwargs): signature = kwargs.pop('signature', None) if signature is None: - signature = self._generate_signature(kwargs) + signature = self.generate_signature(kwargs) task_id = uuid.uuid4().hex prepared_kwargs = self._prepare_kwargs(kwargs) @@ -224,14 +225,24 @@ def enqueue(self, user, **kwargs): # ensure the result is saved to the backend (database) self.backend.add_pending_result(async_result) - # after calling apply, we should have task result model, so get it and set our custom fields - task_result = get_task_model(self, task_id) - task_result.task_name = self.name - task_result.task_kwargs = self.backend.encode(prepared_kwargs) - task_result.user = user - task_result.channel_id = channel_id - task_result.signature = signature - task_result.save() + saved = False + tries = 0 + while not saved: + # after calling apply, we should ideally have a task result model saved to the DB, but it relies on celery's + # event consumption, and we might try to retrieve it before it has actually saved, so we retry + try: + task_result = get_task_model(self, task_id) + task_result.task_name = self.name + task_result.task_kwargs = self.backend.encode(prepared_kwargs) + task_result.user = user + task_result.channel_id = channel_id + task_result.signature = signature + task_result.save() + saved = True + except IntegrityError as e: + tries += 1 + if tries > 3: + raise e return async_result def fetch_or_enqueue(self, user, **kwargs): @@ -249,7 +260,7 @@ def fetch_or_enqueue(self, user, **kwargs): if self.app.conf.task_always_eager: return self.enqueue(user, **kwargs) - signature = self._generate_signature(kwargs) + signature = self.generate_signature(kwargs) # create an advisory lock to obtain exclusive control on preventing task duplicates with self._lock_signature(signature): @@ -278,7 +289,7 @@ def requeue(self, **kwargs): task_result = get_task_model(self, request.id) task_kwargs = request.kwargs.copy() task_kwargs.update(kwargs) - signature = self._generate_signature(kwargs) + signature = self.generate_signature(kwargs) logging.info(f"Re-queuing task {self.name} for user {task_result.user.pk} from {request.id} | {signature}") return self.enqueue(task_result.user, signature=signature, **task_kwargs) @@ -289,7 +300,7 @@ def revoke(self, exclude_task_ids=None, **kwargs): :param kwargs: Task keyword arguments that will be used to match against tasks :return: The number of tasks revoked """ - signature = self._generate_signature(kwargs) + signature = self.generate_signature(kwargs) task_ids = self.find_incomplete_ids(signature) if exclude_task_ids is not None: diff --git a/contentcuration/contentcuration/views/nodes.py b/contentcuration/contentcuration/views/nodes.py index 0d83b29dd4..a1e924c0c7 100644 --- a/contentcuration/contentcuration/views/nodes.py +++ b/contentcuration/contentcuration/views/nodes.py @@ -98,8 +98,9 @@ def get_node_diff(request, updated_id, original_id): if data: return Response(data) + signature = generatenodediff_task.generate_signature(dict(updated_id=updated_id, original_id=original_id)) # See if there's already a staging task in progress - if generatenodediff_task.find_incomplete_ids(updated_id=updated_id, original_id=original_id).exists(): + if generatenodediff_task.find_incomplete_ids(signature).exists(): return Response('Diff is being generated', status=status.HTTP_302_FOUND) except ContentNode.DoesNotExist: pass diff --git a/contentcuration/contentcuration/viewsets/file.py b/contentcuration/contentcuration/viewsets/file.py index 132bfe0769..922d6f276d 100644 --- a/contentcuration/contentcuration/viewsets/file.py +++ b/contentcuration/contentcuration/viewsets/file.py @@ -148,6 +148,8 @@ def upload_url(self, request): if not isinstance(duration, (int, float)): return HttpResponseBadRequest(reason="File duration must be a number") duration = math.floor(duration) + if duration <= 0: + return HttpResponseBadRequest(reason="File duration is equal to or less than 0") try: request.user.check_space(float(size), checksum) diff --git a/requirements-dev.in b/requirements-dev.in index 6c3b1033f8..b2bfb0931c 100644 --- a/requirements-dev.in +++ b/requirements-dev.in @@ -14,7 +14,6 @@ pytest-pythonpath pytest-timeout pytest-watch pre-commit==1.15.1 -codecov coverage pytest-cov nodeenv diff --git a/requirements-dev.txt b/requirements-dev.txt index 383ee77801..07d875e98f 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -35,8 +35,6 @@ click==8.1.3 # -c requirements.txt # flask # pip-tools -codecov==2.1.12 - # via -r requirements-dev.in colorama==0.4.4 # via pytest-watch configargparse==1.5.3 @@ -50,7 +48,6 @@ coreschema==0.0.4 coverage[toml]==6.2 # via # -r requirements-dev.in - # codecov # pytest-cov customizable-django-profiler @ git+https://github.com/someshchaturvedi/customizable-django-profiler.git # via -r requirements-dev.in @@ -220,7 +217,6 @@ pyzmq==23.1.0 requests==2.25.1 # via # -c requirements.txt - # codecov # coreapi # locust roundrobin==0.0.2