diff --git a/Makefile b/Makefile index dc1e70b51e..3f164c5c26 100644 --- a/Makefile +++ b/Makefile @@ -38,7 +38,7 @@ migrate: # 4) Remove the management command from this `deploy-migrate` recipe # 5) Repeat! deploy-migrate: - echo "Nothing to do here!" + python contentcuration/manage.py fix_exercise_extra_fields contentnodegc: python contentcuration/manage.py garbage_collect diff --git a/contentcuration/contentcuration/frontend/shared/views/GlobalSnackbar.vue b/contentcuration/contentcuration/frontend/shared/views/GlobalSnackbar.vue index d3d0031499..8ca20f568b 100644 --- a/contentcuration/contentcuration/frontend/shared/views/GlobalSnackbar.vue +++ b/contentcuration/contentcuration/frontend/shared/views/GlobalSnackbar.vue @@ -5,6 +5,7 @@ :key="key" :timeout="snackbarOptions.duration" left + multi-line :value="snackbarIsVisible" @input="visibilityToggled" > diff --git a/contentcuration/contentcuration/frontend/shared/views/policies/TermsOfServiceModal.vue b/contentcuration/contentcuration/frontend/shared/views/policies/TermsOfServiceModal.vue index d5a952f2da..668466ba45 100644 --- a/contentcuration/contentcuration/frontend/shared/views/policies/TermsOfServiceModal.vue +++ b/contentcuration/contentcuration/frontend/shared/views/policies/TermsOfServiceModal.vue @@ -185,7 +185,7 @@

diff --git a/contentcuration/contentcuration/management/commands/fix_exercise_extra_fields.py b/contentcuration/contentcuration/management/commands/fix_exercise_extra_fields.py new file mode 100644 index 0000000000..97142b60ab --- /dev/null +++ b/contentcuration/contentcuration/management/commands/fix_exercise_extra_fields.py @@ -0,0 +1,158 @@ +import json +import logging as logmodule +import time + +from django.core.management.base import BaseCommand +from le_utils.constants import content_kinds +from le_utils.constants import exercises + +from contentcuration.models import ContentNode +from contentcuration.utils.nodes import migrate_extra_fields + +logging = logmodule.getLogger("command") + +CHUNKSIZE = 5000 + + +def _needs_m_n_fix(extra_fields): + """ + Check if already-migrated extra_fields have non-null m/n + on a non-m_of_n mastery model. + """ + try: + threshold = extra_fields["options"]["completion_criteria"]["threshold"] + except (KeyError, TypeError): + return False + mastery_model = threshold.get("mastery_model") + if mastery_model is None or mastery_model == exercises.M_OF_N: + return False + return threshold.get("m") is not None or threshold.get("n") is not None + + +def _needs_old_style_migration(extra_fields): + """ + Check if extra_fields still has old-style top-level mastery_model. + """ + return isinstance(extra_fields, dict) and "mastery_model" in extra_fields + + +class Command(BaseCommand): + help = ( + "Fix exercise extra_fields that were migrated with invalid m/n values " + "in their completion criteria threshold. Non-m_of_n mastery models " + "require m and n to be null, but old data may have had non-null values " + "that were carried over during migration. Also migrates any remaining " + "old-style extra_fields to the new format." + ) + + def add_arguments(self, parser): + parser.add_argument( + "--dry-run", + action="store_true", + help="Report what would be changed without modifying the database.", + ) + + def handle(self, *args, **options): + dry_run = options.get("dry_run", False) + start = time.time() + + # Single pass over all exercises, filtering in Python to avoid + # expensive nested JSON field queries in the database. + queryset = ContentNode.objects.filter(kind_id=content_kinds.EXERCISE) + + total = ContentNode.objects.filter(kind_id="exercise").count() + migrated_fixed = 0 + migrated_complete = 0 + old_style_fixed = 0 + old_style_complete = 0 + incomplete_fixed = 0 + exercises_checked = 0 + + for node in queryset.iterator(chunk_size=CHUNKSIZE): + fix_type, complete = self._process_node(node, dry_run) + if fix_type == "old_style": + old_style_fixed += 1 + if complete: + old_style_complete += 1 + elif fix_type == "m_n_fix": + migrated_fixed += 1 + if complete: + migrated_complete += 1 + elif fix_type == "incomplete" and complete: + incomplete_fixed += 1 + exercises_checked += 1 + if exercises_checked % CHUNKSIZE == 0: + logging.info( + "{} / {} exercises checked".format(exercises_checked, total) + ) + logging.info( + "{} marked complete out of {} old style fixed".format( + old_style_complete, old_style_fixed + ) + ) + logging.info( + "{} marked complete out of {} migrated fixed".format( + migrated_complete, migrated_fixed + ) + ) + logging.info( + "{} marked complete that were previously incomplete".format( + incomplete_fixed + ) + ) + + logging.info("{} / {} exercises checked".format(exercises_checked, total)) + logging.info( + "{} marked complete out of {} old style fixed".format( + old_style_complete, old_style_fixed + ) + ) + logging.info( + "{} marked complete out of {} migrated fixed".format( + migrated_complete, migrated_fixed + ) + ) + logging.info( + "{} marked complete that were previously incomplete".format( + incomplete_fixed + ) + ) + logging.info( + "Done in {:.1f}s. Fixed {} migrated exercises, " + "migrated {} old-style exercises." + "marked {} previously incomplete exercises complete. {}".format( + time.time() - start, + migrated_fixed, + old_style_fixed, + incomplete_fixed, + " (dry run)" if dry_run else "", + ) + ) + + def _process_node(self, node, dry_run): + ef = node.extra_fields + was_complete = node.complete + if isinstance(ef, str): + try: + ef = json.loads(ef) + except (json.JSONDecodeError, ValueError): + return None, None + if not isinstance(ef, dict): + return None, None + + if _needs_old_style_migration(ef): + ef = migrate_extra_fields(ef) + fix_type = "old_style" + elif _needs_m_n_fix(ef): + ef["options"]["completion_criteria"]["threshold"]["m"] = None + ef["options"]["completion_criteria"]["threshold"]["n"] = None + fix_type = "m_n_fix" + elif not was_complete: + fix_type = "incomplete" + else: + return None, None + node.extra_fields = ef + complete = not node.mark_complete() + if not dry_run: + node.save(update_fields=["extra_fields", "complete"]) + return fix_type, complete diff --git a/contentcuration/contentcuration/tests/test_contentnodes.py b/contentcuration/contentcuration/tests/test_contentnodes.py index 5ce8b472a4..0de23ab008 100644 --- a/contentcuration/contentcuration/tests/test_contentnodes.py +++ b/contentcuration/contentcuration/tests/test_contentnodes.py @@ -1,3 +1,4 @@ +import json import random import string import time @@ -17,6 +18,10 @@ from . import testdata from .base import StudioTestCase from .testdata import create_studio_file +from contentcuration.constants import completion_criteria as studio_completion_criteria +from contentcuration.management.commands.fix_exercise_extra_fields import ( + Command as FixExerciseExtraFieldsCommand, +) from contentcuration.models import AssessmentItem from contentcuration.models import Channel from contentcuration.models import ContentKind @@ -29,6 +34,7 @@ from contentcuration.models import License from contentcuration.utils.db_tools import TreeBuilder from contentcuration.utils.files import create_thumbnail_from_base64 +from contentcuration.utils.nodes import migrate_extra_fields from contentcuration.utils.sync import sync_node @@ -1423,6 +1429,53 @@ def test_create_exercise_old_extra_fields(self): new_obj.mark_complete() self.assertTrue(new_obj.complete) + def test_migrate_extra_fields_do_all_with_non_null_m_n(self): + """Migrated do_all exercises with non-null m/n must pass completion criteria validation.""" + extra_fields = { + "mastery_model": exercises.DO_ALL, + "m": 0, + "n": 0, + "randomize": False, + } + result = migrate_extra_fields(extra_fields) + criterion = result["options"]["completion_criteria"] + # Should not raise + studio_completion_criteria.validate(criterion, kind=content_kinds.EXERCISE) + + def test_migrate_extra_fields_num_correct_in_a_row_with_non_null_m_n(self): + """Migrated num_correct_in_a_row exercises with leftover m/n must pass validation.""" + for mastery_model in ( + exercises.NUM_CORRECT_IN_A_ROW_2, + exercises.NUM_CORRECT_IN_A_ROW_3, + exercises.NUM_CORRECT_IN_A_ROW_5, + exercises.NUM_CORRECT_IN_A_ROW_10, + ): + extra_fields = { + "mastery_model": mastery_model, + "m": 5, + "n": 10, + "randomize": False, + } + result = migrate_extra_fields(extra_fields) + criterion = result["options"]["completion_criteria"] + # Should not raise + studio_completion_criteria.validate(criterion, kind=content_kinds.EXERCISE) + + def test_migrate_extra_fields_m_of_n_preserves_m_n(self): + """Migrated m_of_n exercises must preserve m and n values.""" + extra_fields = { + "mastery_model": exercises.M_OF_N, + "m": 3, + "n": 5, + "randomize": False, + } + result = migrate_extra_fields(extra_fields) + criterion = result["options"]["completion_criteria"] + self.assertEqual(criterion["threshold"]["m"], 3) + self.assertEqual(criterion["threshold"]["n"], 5) + # Should not raise + studio_completion_criteria.validate(criterion, kind=content_kinds.EXERCISE) + def test_create_exercise_bad_new_extra_fields(self): licenses = list( License.objects.filter( @@ -1571,7 +1624,9 @@ def test_create_topic_no_modality_with_completion_criteria_incomplete(self): new_obj.mark_complete() self.assertFalse(new_obj.complete) - def test_create_topic_unit_modality_without_completion_criteria_incomplete(self): + def test_create_topic_unit_modality_without_completion_criteria_incomplete( + self, + ): """Topic with UNIT modality MUST have completion criteria - it's not optional.""" channel = testdata.channel() new_obj = ContentNode( @@ -1588,3 +1643,231 @@ def test_create_topic_unit_modality_without_completion_criteria_incomplete(self) new_obj.save() new_obj.mark_complete() self.assertFalse(new_obj.complete) + + +class FixExerciseExtraFieldsTestCase(StudioTestCase): + def setUp(self): + super(FixExerciseExtraFieldsTestCase, self).setUpBase() + self.licenses = list( + License.objects.filter( + copyright_holder_required=False, is_custom=False + ).values_list("pk", flat=True) + ) + self.channel = testdata.channel() + + def _create_exercise(self, extra_fields, with_assessment=True): + node = ContentNode( + title="Exercise", + kind_id=content_kinds.EXERCISE, + parent=self.channel.main_tree, + license_id=self.licenses[0], + extra_fields=extra_fields, + ) + node.save() + if with_assessment: + AssessmentItem.objects.create( + contentnode=node, + question="A question", + answers='[{"correct": true, "text": "answer"}]', + ) + return node + + def test_fixes_migrated_do_all_with_non_null_m_n(self): + """Command should null out m/n for already-migrated do_all exercises.""" + node = self._create_exercise( + { + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.DO_ALL, + "m": 0, + "n": 0, + }, + "model": completion_criteria.MASTERY, + } + }, + } + ) + node.mark_complete() + node.save() + + command = FixExerciseExtraFieldsCommand() + command.handle() + + node.refresh_from_db() + threshold = node.extra_fields["options"]["completion_criteria"]["threshold"] + self.assertIsNone(threshold["m"]) + self.assertIsNone(threshold["n"]) + self.assertEqual(threshold["mastery_model"], exercises.DO_ALL) + # Should now pass schema validation + studio_completion_criteria.validate( + node.extra_fields["options"]["completion_criteria"], + kind=content_kinds.EXERCISE, + ) + + def test_fixes_migrated_num_correct_in_a_row_with_non_null_m_n(self): + """Command should null out m/n for num_correct_in_a_row exercises.""" + node = self._create_exercise( + { + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.NUM_CORRECT_IN_A_ROW_5, + "m": 5, + "n": 10, + }, + "model": completion_criteria.MASTERY, + } + }, + } + ) + node.mark_complete() + node.save() + + command = FixExerciseExtraFieldsCommand() + command.handle() + + node.refresh_from_db() + threshold = node.extra_fields["options"]["completion_criteria"]["threshold"] + self.assertIsNone(threshold["m"]) + self.assertIsNone(threshold["n"]) + self.assertEqual(threshold["mastery_model"], exercises.NUM_CORRECT_IN_A_ROW_5) + + def test_does_not_touch_m_of_n(self): + """Command should leave m_of_n exercises untouched.""" + node = self._create_exercise( + { + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.M_OF_N, + "m": 3, + "n": 5, + }, + "model": completion_criteria.MASTERY, + } + }, + } + ) + node.mark_complete() + node.save() + + command = FixExerciseExtraFieldsCommand() + command.handle() + + node.refresh_from_db() + threshold = node.extra_fields["options"]["completion_criteria"]["threshold"] + self.assertEqual(threshold["m"], 3) + self.assertEqual(threshold["n"], 5) + + def test_migrates_old_style_extra_fields(self): + """Command should migrate old-style top-level mastery_model to new format.""" + node = self._create_exercise( + { + "mastery_model": exercises.DO_ALL, + "m": 0, + "n": 0, + "randomize": False, + } + ) + + command = FixExerciseExtraFieldsCommand() + command.handle() + + node.refresh_from_db() + # Should have new-style structure + criterion = node.extra_fields["options"]["completion_criteria"] + self.assertEqual(criterion["model"], completion_criteria.MASTERY) + self.assertEqual(criterion["threshold"]["mastery_model"], exercises.DO_ALL) + # m and n should be null for do_all + self.assertIsNone(criterion["threshold"]["m"]) + self.assertIsNone(criterion["threshold"]["n"]) + # Old keys should be gone + self.assertNotIn("mastery_model", node.extra_fields) + self.assertNotIn("m", node.extra_fields) + self.assertNotIn("n", node.extra_fields) + # randomize should be preserved + self.assertFalse(node.extra_fields["randomize"]) + + def test_dry_run_does_not_modify(self): + """Dry run should report counts but not modify data.""" + node = self._create_exercise( + { + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.DO_ALL, + "m": 0, + "n": 0, + }, + "model": completion_criteria.MASTERY, + } + }, + } + ) + node.mark_complete() + node.save() + + command = FixExerciseExtraFieldsCommand() + command.handle(dry_run=True) + + node.refresh_from_db() + threshold = node.extra_fields["options"]["completion_criteria"]["threshold"] + # Should still have the invalid values + self.assertEqual(threshold["m"], 0) + self.assertEqual(threshold["n"], 0) + + def test_incomplete_node_with_valid_fields_gets_marked_complete(self): + """An incomplete exercise with valid extra_fields should be marked complete.""" + node = self._create_exercise( + { + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.DO_ALL, + "m": None, + "n": None, + }, + "model": completion_criteria.MASTERY, + } + }, + } + ) + # Force incomplete status even though fields are valid + node.complete = False + node.save() + + command = FixExerciseExtraFieldsCommand() + command.handle() + + node.refresh_from_db() + self.assertTrue(node.complete) + + def test_migrates_string_extra_fields(self): + """Command should parse and migrate extra_fields stored as a JSON string.""" + node = self._create_exercise( + json.dumps( + { + "mastery_model": exercises.DO_ALL, + "m": 0, + "n": 0, + "randomize": False, + } + ), + ) + + command = FixExerciseExtraFieldsCommand() + command.handle() + + node.refresh_from_db() + extra_fields = node.extra_fields + # Should now be a dict, not a string + self.assertIsInstance(extra_fields, dict) + threshold = extra_fields["options"]["completion_criteria"]["threshold"] + self.assertIsNone(threshold["m"]) + self.assertIsNone(threshold["n"]) + self.assertEqual(threshold["mastery_model"], exercises.DO_ALL) + studio_completion_criteria.validate( + extra_fields["options"]["completion_criteria"], + kind=content_kinds.EXERCISE, + ) diff --git a/contentcuration/contentcuration/tests/views/test_views_internal.py b/contentcuration/contentcuration/tests/views/test_views_internal.py index e43d4fdd75..f503ebe538 100644 --- a/contentcuration/contentcuration/tests/views/test_views_internal.py +++ b/contentcuration/contentcuration/tests/views/test_views_internal.py @@ -9,6 +9,7 @@ from django.db import connections from django.urls import reverse_lazy from le_utils.constants import content_kinds +from le_utils.constants import exercises from le_utils.constants import format_presets from le_utils.constants.labels.accessibility_categories import ( ACCESSIBILITYCATEGORIESLIST, @@ -33,6 +34,7 @@ from ..testdata import thumbnail_bytes from ..testdata import user from contentcuration import ricecooker_versions as rc +from contentcuration.constants import completion_criteria as studio_completion_criteria from contentcuration.db.models.manager import ALLOWED_OVERRIDES from contentcuration.db.models.manager import EDIT_ALLOWED_OVERRIDES from contentcuration.models import Channel @@ -472,6 +474,112 @@ def test_duplicate_assessment_item_returns_400_status_code(self): ) +class ApiAddExerciseExtraFieldsMigrationTestCase(StudioTestCase): + """ + Tests that exercise extra_fields are properly migrated when nodes + are added through the ricecooker import endpoint. + """ + + def setUp(self): + super(ApiAddExerciseExtraFieldsMigrationTestCase, self).setUp() + self.channel = channel() + self.root_node = self.channel.main_tree + + def _make_exercise_data(self, extra_fields): + return { + "title": "Test Exercise", + "language": "en", + "description": "An exercise for testing extra_fields migration", + "node_id": uuid.uuid4().hex, + "content_id": uuid.uuid4().hex, + "source_domain": "test.com", + "source_id": "test", + "author": "Test Author", + "files": [], + "kind": content_kinds.EXERCISE, + "license": "CC BY", + "license_description": None, + "copyright_holder": "Test", + "questions": [], + "extra_fields": extra_fields, + } + + def _add_exercise(self, extra_fields): + data = self._make_exercise_data(extra_fields) + request_data = { + "root_id": self.root_node.id, + "content_data": [data], + } + response = self.admin_client().post( + reverse_lazy("api_add_nodes_to_tree"), + data=request_data, + format="json", + ) + self.assertEqual(response.status_code, 200, response.content) + return ContentNode.objects.get(node_id=data["node_id"]) + + def test_old_style_extra_fields_migrated_on_import(self): + """ + Old-style extra_fields (top-level mastery_model, m, n) should be + migrated to new-style options.completion_criteria format with m/n + set to null for non-m_of_n mastery models. + """ + node = self._add_exercise( + { + "mastery_model": exercises.DO_ALL, + "m": 0, + "n": 0, + "randomize": False, + } + ) + extra_fields = node.extra_fields + + assert "options" in extra_fields + criteria = extra_fields["options"]["completion_criteria"] + threshold = criteria["threshold"] + + self.assertEqual(threshold["mastery_model"], exercises.DO_ALL) + self.assertIsNone(threshold["m"]) + self.assertIsNone(threshold["n"]) + + # Old-style keys should not remain at top level + self.assertNotIn("mastery_model", extra_fields) + self.assertNotIn("m", extra_fields) + self.assertNotIn("n", extra_fields) + + studio_completion_criteria.validate(criteria, kind=content_kinds.EXERCISE) + + def test_new_style_extra_fields_preserved_on_import(self): + """ + New-style extra_fields (options.completion_criteria) should pass + through unchanged. + """ + node = self._add_exercise( + { + "options": { + "completion_criteria": { + "threshold": { + "mastery_model": exercises.M_OF_N, + "m": 3, + "n": 5, + }, + "model": "mastery", + } + }, + } + ) + extra_fields = node.extra_fields + + criteria = extra_fields["options"]["completion_criteria"] + threshold = criteria["threshold"] + + self.assertEqual(threshold["mastery_model"], exercises.M_OF_N) + self.assertEqual(threshold["m"], 3) + self.assertEqual(threshold["n"], 5) + + studio_completion_criteria.validate(criteria, kind=content_kinds.EXERCISE) + + class PublishEndpointTestCase(BaseAPITestCase): @classmethod def setUpClass(cls): diff --git a/contentcuration/contentcuration/utils/assessment/base.py b/contentcuration/contentcuration/utils/assessment/base.py index c78547707b..368d9ad1c4 100644 --- a/contentcuration/contentcuration/utils/assessment/base.py +++ b/contentcuration/contentcuration/utils/assessment/base.py @@ -274,7 +274,7 @@ def _sort_by_order(self, items, item_type): try: return sorted(items, key=lambda x: x.get("order")) except TypeError: - logging.error(f"Unable to sort {item_type}, leaving unsorted.") + logging.warning(f"Unable to sort {item_type}, leaving unsorted.") return items def _process_answers(self, assessment_item): diff --git a/contentcuration/contentcuration/utils/nodes.py b/contentcuration/contentcuration/utils/nodes.py index aa424ff27e..9660f52d57 100644 --- a/contentcuration/contentcuration/utils/nodes.py +++ b/contentcuration/contentcuration/utils/nodes.py @@ -15,6 +15,7 @@ from django.utils import timezone from le_utils.constants import completion_criteria from le_utils.constants import content_kinds +from le_utils.constants import exercises from le_utils.constants import format_presets from contentcuration.models import AssessmentItem @@ -525,6 +526,9 @@ def migrate_extra_fields(extra_fields): not extra_fields.get("options", {}).get("completion_criteria", {}) and mastery_model is not None ): + if mastery_model != exercises.M_OF_N: + m = None + n = None extra_fields["options"] = extra_fields.get("options", {}) extra_fields["options"]["completion_criteria"] = { "threshold": { diff --git a/contentcuration/contentcuration/views/internal.py b/contentcuration/contentcuration/views/internal.py index 07b4014b00..71c3716e40 100644 --- a/contentcuration/contentcuration/views/internal.py +++ b/contentcuration/contentcuration/views/internal.py @@ -53,6 +53,7 @@ from contentcuration.utils.nodes import map_files_to_assessment_item from contentcuration.utils.nodes import map_files_to_node from contentcuration.utils.nodes import map_files_to_slideshow_slide_item +from contentcuration.utils.nodes import migrate_extra_fields from contentcuration.utils.sentry import report_exception from contentcuration.viewsets.sync.constants import CHANNEL from contentcuration.viewsets.sync.utils import generate_publish_event @@ -835,6 +836,11 @@ def create_node(node_data, parent_node, sort_order): # noqa: C901 if isinstance(extra_fields, str): extra_fields = json.loads(extra_fields) + # Migrate old-style extra_fields (top-level mastery_model, m, n) + # to new-style options.completion_criteria format + if node_data["kind"] == content_kinds.EXERCISE: + extra_fields = migrate_extra_fields(extra_fields) + # validate completion criteria if "options" in extra_fields and "completion_criteria" in extra_fields["options"]: try: