diff --git a/kobo/apps/audit_log/models.py b/kobo/apps/audit_log/models.py index c0827f5af2..7fa16d8f0b 100644 --- a/kobo/apps/audit_log/models.py +++ b/kobo/apps/audit_log/models.py @@ -19,6 +19,7 @@ get_client_ip, get_human_readable_client_user_agent, ) +from kobo.apps.subsequences.constants import Action from kpi.constants import ( ACCESS_LOG_LOGINAS_AUTH_TYPE, ACCESS_LOG_SUBMISSION_AUTH_TYPE, @@ -404,6 +405,8 @@ def create_from_request(cls, request: WSGIRequest): 'submissions-list': cls._create_from_submission_request, 'submission-detail': cls._create_from_submission_request, 'advanced-submission-post': cls._create_from_submission_extra_request, + 'advanced-features-list': cls._create_from_question_advanced_action_request, + 'advanced-features-detail': cls._create_from_question_advanced_action_request, } url_name = request.resolver_match.url_name method = url_name_to_action.get(url_name, None) @@ -787,6 +790,32 @@ def _create_from_permissions_request(cls, request): ) ProjectHistoryLog.objects.bulk_create(logs) + @classmethod + def _create_from_question_advanced_action_request(cls, request): + initial_data = getattr(request, 'initial_data', None) + updated_data = getattr(request, 'updated_data', None) + source_data = updated_data if updated_data else initial_data + asset_uid = request.resolver_match.kwargs['parent_lookup_asset'] + if not source_data: + return + if source_data.get('action') != Action.QUAL: + return + owner = source_data.pop('asset.owner.username') + object_id = source_data.pop('object_id') + metadata = { + 'asset_uid': asset_uid, + 'log_subtype': PROJECT_HISTORY_LOG_PROJECT_SUBTYPE, + 'ip_address': get_client_ip(request), + 'source': get_human_readable_client_user_agent(request), + 'project_owner': owner, + } + action = AuditAction.UPDATE_QA + metadata['qa'] = {PROJECT_HISTORY_LOG_METADATA_FIELD_NEW: source_data['params']} + user = get_database_user(request.user) + ProjectHistoryLog.objects.create( + user=user, object_id=object_id, action=action, metadata=metadata + ) + @classmethod def _create_from_v1_export(cls, request): updated_data = getattr(request, 'updated_data', None) diff --git a/kobo/apps/audit_log/tests/test_project_history_logs.py b/kobo/apps/audit_log/tests/test_project_history_logs.py index b83292cda9..dcfce08e07 100644 --- a/kobo/apps/audit_log/tests/test_project_history_logs.py +++ b/kobo/apps/audit_log/tests/test_project_history_logs.py @@ -28,6 +28,8 @@ remove_uuid_prefix, ) from kobo.apps.openrosa.libs.utils.logger_tools import dict2xform +from kobo.apps.subsequences.models import QuestionAdvancedAction +from kobo.apps.subsequences.constants import Action from kpi.constants import ( ASSET_TYPE_TEMPLATE, CLONE_ARG_NAME, @@ -559,37 +561,35 @@ def test_update_content_creates_log(self, use_v2): use_v2=use_v2, ) - def test_update_qa_creates_log(self): - request_data = { - 'advanced_features': { - 'qual': { - 'qual_survey': [ - { - 'type': 'qual_note', - 'uuid': '12345', - 'scope': 'by_question#survey', - 'xpath': 'q1', - 'labels': {'_default': 'QA Question'}, - # requests to remove a question just add this - # option rather than actually deleting anything - 'options': {'deleted': True}, - } - ] - } - } - } - - log_metadata = self._base_asset_detail_endpoint_test( - patch=True, - url_name=self.detail_url, - request_data=request_data, - expected_action=AuditAction.UPDATE_QA, + def test_add_qa_creates_log(self): + request_data = {"action":"qual", "question_xpath":"Audio", "params": [{"labels": {"_default":"wherefore?"},"uuid":"12345", "type":"qualText"}]} + metadata = self._base_project_history_log_test( + self.client.post, + reverse('api_v2:advanced-features-list', args=[self.asset.uid]), + request_data = request_data, + expected_action = AuditAction.UPDATE_QA, + expected_subtype=PROJECT_HISTORY_LOG_PROJECT_SUBTYPE ) + self.assertEqual(metadata['qa']['new'], request_data['params']) - self.assertEqual( - log_metadata['qa'][PROJECT_HISTORY_LOG_METADATA_FIELD_NEW], - request_data['advanced_features']['qual']['qual_survey'], + def test_update_qa_creates_log(self): + question_qual_action = QuestionAdvancedAction.objects.create( + asset=self.asset, + action=Action.QUAL, + question_xpath='q1', + params=[{"labels": {"_default": "why?"}, "uuid":"12345", "type":"qualText"}] + ) + request_data = {"params": [{"labels": {"_default":"wherefore?"},"uuid":"12345", "type":"qualText"}]} + metadata = self._base_project_history_log_test( + self.client.patch, + reverse('api_v2:advanced-features-detail', args=[self.asset.uid, question_qual_action.uid]), + request_data = request_data, + expected_action = AuditAction.UPDATE_QA, + expected_subtype=PROJECT_HISTORY_LOG_PROJECT_SUBTYPE ) + self.assertEqual(metadata['qa']['new'], request_data['params']) + + def test_failed_qa_update_does_not_create_log(self): # badly formatted QA dict should result in an error before update diff --git a/kobo/apps/subsequences/actions/__init__.py b/kobo/apps/subsequences/actions/__init__.py index e514a954ff..79f339f559 100644 --- a/kobo/apps/subsequences/actions/__init__.py +++ b/kobo/apps/subsequences/actions/__init__.py @@ -2,6 +2,7 @@ from .automatic_google_translation import AutomaticGoogleTranslationAction from .manual_transcription import ManualTranscriptionAction from .manual_translation import ManualTranslationAction +from .qual import QualAction # TODO, what about using a loader for every class in "actions" folder (except base.py)? ACTIONS = ( @@ -9,6 +10,7 @@ AutomaticGoogleTranslationAction, ManualTranscriptionAction, ManualTranslationAction, + QualAction, ) ACTION_IDS_TO_CLASSES = {a.ID: a for a in ACTIONS} diff --git a/kobo/apps/subsequences/actions/base.py b/kobo/apps/subsequences/actions/base.py index 6ec6abd893..15f9e61c6e 100644 --- a/kobo/apps/subsequences/actions/base.py +++ b/kobo/apps/subsequences/actions/base.py @@ -322,6 +322,17 @@ def get_output_fields(self) -> list[dict]: # raise NotImplementedError() return [] + def update_params(self, incoming_params): + """ + Returns the result of updating current params with incoming ones from + a request. May be overridden, eg, to prevent deletion of existing lanugages + for transcriptions/translations + Defaults to replacing the existing params with the new ones. + Should raise an error if the incoming params are not well-formatted + """ + self.validate_params(incoming_params) + self.params = incoming_params + def validate_external_data(self, data): jsonschema.validate(data, self.external_data_schema) @@ -594,6 +605,13 @@ def languages(self) -> list[str]: languages.append(individual_params['language']) return languages + def update_params(self, incoming_params): + self.validate_params(incoming_params) + current_languages = self.languages + for language_obj in incoming_params: + if language_obj['language'] not in current_languages: + self.params.append(language_obj) + class BaseAutomaticNLPAction(BaseManualNLPAction): """ diff --git a/kobo/apps/subsequences/constants.py b/kobo/apps/subsequences/constants.py index 2445db6e01..e1dd518a0f 100644 --- a/kobo/apps/subsequences/constants.py +++ b/kobo/apps/subsequences/constants.py @@ -1,3 +1,5 @@ +from django.db import models + SUBMISSION_UUID_FIELD = 'meta/rootUuid' # FIXME: import from elsewhere SUPPLEMENT_KEY = '_supplementalDetails' # leave unchanged for backwards compatibility @@ -20,3 +22,16 @@ '20250820', None ] + + +class Action(models.TextChoices): + MANUAL_TRANSCRIPTION = 'manual_transcription' + MANUAL_TRANSLATION = 'manual_translation' + AUTOMATIC_GOOGLE_TRANSLATION = 'automatic_google_translation' + AUTOMATIC_GOOGLE_TRANSCRIPTION = 'automatic_google_transcription' + QUAL = 'qual' + + +def set_version(schema: dict) -> dict: + schema['_version'] = SCHEMA_VERSIONS[0] + return schema diff --git a/kobo/apps/subsequences/docs/api/v2/subsequences/create.md b/kobo/apps/subsequences/docs/api/v2/subsequences/create.md new file mode 100644 index 0000000000..bf56302976 --- /dev/null +++ b/kobo/apps/subsequences/docs/api/v2/subsequences/create.md @@ -0,0 +1,74 @@ +## Add an advanced action to an asset + +Enables a new type of advanced action on a question in the asset. +* `action`, `params`, and `question_xpath` are required +* `params` must match the expected param_schema of the `action` + +Accepted `action`s include: +* `manual_transcription` +* `automatic_google_transcription` +* `manual_translation` +* `automatic_google_translation` +* `qual` + +For all actions except `qual`, `params` must look like +> '[{"language": "es"}, {"language": "en"}, ...]' + +For `qual`, `params` must look like +``` + [ + { + 'type': 'qualInteger', + 'uuid': '1a2c8eb0-e2ec-4b3c-942a-c1a5410c081a', + 'labels': {'_default': 'How many characters appear in the story?'}, + }, + { + 'type': 'qualSelectMultiple', + 'uuid': '2e30bec7-4843-43c7-98bc-13114af230c5', + 'labels': {'_default': "What themes were present in the story?"}, + 'choices': [ + { + 'uuid': '2e24e6b4-bc3b-4e8e-b0cd-d8d3b9ca15b6', + 'labels': {'_default': 'Empathy'}, + }, + { + 'uuid': 'cb82919d-2948-4ccf-a488-359c5d5ee53a', + 'labels': {'_default': 'Competition'}, + }, + { + 'uuid': '8effe3b1-619e-4ada-be45-ebcea5af0aaf', + 'labels': {'_default': 'Apathy'}, + }, + ], + }, + { + 'type': 'qualSelectOne', + 'uuid': '1a8b748b-f470-4c40-bc09-ce2b1197f503', + 'labels': {'_default': 'Was this a first-hand account?'}, + 'choices': [ + { + 'uuid': '3c7aacdc-8971-482a-9528-68e64730fc99', + 'labels': {'_default': 'Yes'}, + }, + { + 'uuid': '7e31c6a5-5eac-464c-970c-62c383546a94', + 'labels': {'_default': 'No'}, + }, + ], + }, + { + 'type': 'qualTags', + 'uuid': 'e9b4e6d1-fdbb-4dc9-8b10-a9c3c388322f', + 'labels': {'_default': 'Tag any landmarks mentioned in the story'}, + }, + { + 'type': 'qualText', + 'uuid': '83acf2a7-8edc-4fd8-8b9f-f832ca3f18ad', + 'labels': {'_default': 'Add any further remarks'}, + }, + { + 'type': 'qualNote', + 'uuid': '5ef11d48-d7a3-432e-af83-8c2e9b1feb72', + 'labels': {'_default': 'Thanks for your diligence'}, + }, + ]``` diff --git a/kobo/apps/subsequences/docs/api/v2/subsequences/list.md b/kobo/apps/subsequences/docs/api/v2/subsequences/list.md new file mode 100644 index 0000000000..ddebff474a --- /dev/null +++ b/kobo/apps/subsequences/docs/api/v2/subsequences/list.md @@ -0,0 +1,3 @@ +## List all advanced features on an asset + +Lists all advanced features on all questions in an asset diff --git a/kobo/apps/subsequences/docs/api/v2/subsequences/retrieve.md b/kobo/apps/subsequences/docs/api/v2/subsequences/retrieve.md new file mode 100644 index 0000000000..369db0251c --- /dev/null +++ b/kobo/apps/subsequences/docs/api/v2/subsequences/retrieve.md @@ -0,0 +1,3 @@ +## Retrieve advanced feature configuration for a question on an asset + +Gets the params for one advanced action for one question in an asset diff --git a/kobo/apps/subsequences/docs/api/v2/subsequences/update.md b/kobo/apps/subsequences/docs/api/v2/subsequences/update.md new file mode 100644 index 0000000000..a514e4ce21 --- /dev/null +++ b/kobo/apps/subsequences/docs/api/v2/subsequences/update.md @@ -0,0 +1,67 @@ +## Update an advanced action on an asset + +Update the params of an advanced action on a question in the asset. +* `params` is required +* `params` must match the expected param_schema of the action being updated + +For all actions except `qual`, `params` must look like +> '[{"language": "es"}, {"language": "en"}, ...]' + +For `qual`, `params` must look like +``` + [ + { + 'type': 'qualInteger', + 'uuid': '1a2c8eb0-e2ec-4b3c-942a-c1a5410c081a', + 'labels': {'_default': 'How many characters appear in the story?'}, + }, + { + 'type': 'qualSelectMultiple', + 'uuid': '2e30bec7-4843-43c7-98bc-13114af230c5', + 'labels': {'_default': "What themes were present in the story?"}, + 'choices': [ + { + 'uuid': '2e24e6b4-bc3b-4e8e-b0cd-d8d3b9ca15b6', + 'labels': {'_default': 'Empathy'}, + }, + { + 'uuid': 'cb82919d-2948-4ccf-a488-359c5d5ee53a', + 'labels': {'_default': 'Competition'}, + }, + { + 'uuid': '8effe3b1-619e-4ada-be45-ebcea5af0aaf', + 'labels': {'_default': 'Apathy'}, + }, + ], + }, + { + 'type': 'qualSelectOne', + 'uuid': '1a8b748b-f470-4c40-bc09-ce2b1197f503', + 'labels': {'_default': 'Was this a first-hand account?'}, + 'choices': [ + { + 'uuid': '3c7aacdc-8971-482a-9528-68e64730fc99', + 'labels': {'_default': 'Yes'}, + }, + { + 'uuid': '7e31c6a5-5eac-464c-970c-62c383546a94', + 'labels': {'_default': 'No'}, + }, + ], + }, + { + 'type': 'qualTags', + 'uuid': 'e9b4e6d1-fdbb-4dc9-8b10-a9c3c388322f', + 'labels': {'_default': 'Tag any landmarks mentioned in the story'}, + }, + { + 'type': 'qualText', + 'uuid': '83acf2a7-8edc-4fd8-8b9f-f832ca3f18ad', + 'labels': {'_default': 'Add any further remarks'}, + }, + { + 'type': 'qualNote', + 'uuid': '5ef11d48-d7a3-432e-af83-8c2e9b1feb72', + 'labels': {'_default': 'Thanks for your diligence'}, + }, + ]``` diff --git a/kobo/apps/subsequences/migrations/0005_submissionsupplement_questionadvancedaction.py b/kobo/apps/subsequences/migrations/0005_submissionsupplement_questionadvancedaction.py new file mode 100644 index 0000000000..b47321c0bb --- /dev/null +++ b/kobo/apps/subsequences/migrations/0005_submissionsupplement_questionadvancedaction.py @@ -0,0 +1,63 @@ +# Generated by Django 4.2.24 on 2025-11-19 11:45 + +from django.db import migrations, models +import django.db.models.deletion +import kpi.fields.kpi_uid +import kpi.fields.lazy_default_jsonb + + +class Migration(migrations.Migration): + + dependencies = [ + ('subsequences', '0004_increase_subsequences_submission_uuid'), + ] + + operations = [ + migrations.CreateModel( + name='QuestionAdvancedAction', + fields=[ + ( + 'uid', + kpi.fields.kpi_uid.KpiUidField( + _null=False, primary_key=True, uid_prefix='qaa' + ), + ), + ( + 'action', + models.CharField( + choices=[ + ('manual_transcription', 'Manual Transcription'), + ('manual_translation', 'Manual Translation'), + ( + 'automatic_google_translation', + 'Automatic Google Translation', + ), + ( + 'automatic_google_transcription', + 'Automatic Google Transcription', + ), + ('qual', 'Qual'), + ], + db_index=True, + max_length=60, + ), + ), + ('question_xpath', models.CharField(max_length=2000)), + ( + 'params', + kpi.fields.lazy_default_jsonb.LazyDefaultJSONBField(default=dict), + ), + ( + 'asset', + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name='advanced_features_set', + to='kpi.asset', + ), + ), + ], + options={ + 'unique_together': {('asset_id', 'question_xpath', 'action')}, + }, + ), + ] diff --git a/kobo/apps/subsequences/models.py b/kobo/apps/subsequences/models.py index d66e97f25f..7c8552a9e2 100644 --- a/kobo/apps/subsequences/models.py +++ b/kobo/apps/subsequences/models.py @@ -1,11 +1,12 @@ -from django.db import models +import jsonschema +from django.db import models, transaction from kobo.apps.openrosa.apps.logger.xform_instance_parser import remove_uuid_prefix +from kpi.fields import KpiUidField, LazyDefaultJSONBField from kpi.models.abstract_models import AbstractTimeStampedModel -from .actions import ACTION_IDS_TO_CLASSES -from .constants import SUBMISSION_UUID_FIELD, SCHEMA_VERSIONS +from .constants import SCHEMA_VERSIONS, SUBMISSION_UUID_FIELD, Action from .exceptions import InvalidAction, InvalidXPath -from .schemas import validate_submission_supplement +from .utils.action_conversion import question_advanced_action_to_action class SubmissionExtras(AbstractTimeStampedModel): @@ -34,8 +35,8 @@ def __repr__(self): @staticmethod def revise_data(asset: 'kpi.Asset', submission: dict, incoming_data: dict) -> dict: - if not asset.advanced_features: - raise InvalidAction + if not asset.advanced_features_set.exists(): + migrate_advanced_features(asset) schema_version = incoming_data.get('_version') @@ -47,10 +48,6 @@ def revise_data(asset: 'kpi.Asset', submission: dict, incoming_data: dict) -> di # TODO: migrate from old per-submission schema raise NotImplementedError - if asset.advanced_features.get('_version') != schema_version: - # TODO: migrate from old per-asset schema - raise NotImplementedError - submission_uuid = remove_uuid_prefix(submission[SUBMISSION_UUID_FIELD]) # constant? supplemental_data = SubmissionExtras.objects.get_or_create( asset=asset, submission_uuid=submission_uuid @@ -65,24 +62,17 @@ def revise_data(asset: 'kpi.Asset', submission: dict, incoming_data: dict) -> di # FIXME: what's a better way? skip all leading underscore keys? # pop off the known special keys first? continue - try: - action_configs_for_this_question = asset.advanced_features[ - '_actionConfigs' - ][question_xpath] - except KeyError as e: - raise InvalidXPath from e + action_configs_for_this_question = asset.advanced_features_set.filter(question_xpath=question_xpath) + if not action_configs_for_this_question.exists(): + raise InvalidXPath for action_id, action_data in data_for_this_question.items(): try: - action_class = ACTION_IDS_TO_CLASSES[action_id] - except KeyError as e: - raise InvalidAction from e - try: - action_params = action_configs_for_this_question[action_id] - except KeyError as e: + question_advanced_action = action_configs_for_this_question.get(action=action_id) + except QuestionAdvancedAction.DoesNotExist as e: raise InvalidAction from e - action = action_class(question_xpath, action_params, asset) + action = question_advanced_action_to_action(question_advanced_action) action.check_limits(asset.owner) question_supplemental_data = supplemental_data.setdefault( @@ -161,9 +151,9 @@ def retrieve_data( # TODO: migrate from old per-submission schema raise NotImplementedError - if asset.advanced_features.get('_version') != schema_version: + if not asset.advanced_features_set.exists(): # TODO: migrate from old per-asset schema - raise NotImplementedError + migrate_advanced_features(asset) retrieved_supplemental_data = {} data_for_output = {} @@ -172,42 +162,14 @@ def retrieve_data( processed_data_for_this_question = retrieved_supplemental_data.setdefault( question_xpath, {} ) - action_configs = asset.advanced_features['_actionConfigs'] - try: - action_configs_for_this_question = action_configs[question_xpath] - except KeyError: - # There's still supplemental data for this question at the - # submission level, but the question is no longer configured at the - # asset level. - # Allow this for now, but maybe forbid later and also forbid - # removing things from the asset-level action configuration? - # Actions could be disabled or hidden instead of being removed - - # FIXME: divergence between the asset-level configuration and - # submission-level supplemental data is going to cause schema - # validation failures! We defo need to forbid removal of actions - # and instead provide a way to mark them as deleted + action_configs_for_this_question = asset.advanced_features_set.filter(question_xpath=question_xpath) + if not action_configs_for_this_question.exists(): continue for action_id, action_data in data_for_this_question.items(): - try: - action_class = ACTION_IDS_TO_CLASSES[action_id] - except KeyError: - # An action class present in the submission data no longer - # exists in the application code - # TODO: log an error - continue - try: - action_params = action_configs_for_this_question[action_id] - except KeyError: - # An action class present in the submission data is no longer - # configured at the asset level for this question - # Allow this for now, but maybe forbid later and also forbid - # removing things from the asset-level action configuration? - # Actions could be disabled or hidden instead of being removed - continue + question_advanced_action = action_configs_for_this_question.get(action=action_id) - action = action_class(question_xpath, action_params) + action = question_advanced_action_to_action(question_advanced_action) retrieved_data = action.retrieve_data(action_data) processed_data_for_this_question[action_id] = retrieved_data @@ -237,3 +199,90 @@ def retrieve_data( return data_for_output return retrieved_supplemental_data + + +class QuestionAdvancedAction(models.Model): + uid = KpiUidField(uid_prefix='qaa', primary_key=True) + asset = models.ForeignKey('kpi.Asset', related_name='advanced_features_set', + null=False, blank=False, on_delete=models.CASCADE) + action = models.CharField( + max_length=60, + choices=Action.choices, + db_index=True, + null=False, + blank=False, + ) + question_xpath = models.CharField( + null=False, blank=False, max_length=2000 + ) + params = LazyDefaultJSONBField(default=dict) + + class Meta: + unique_together = ('asset_id', 'question_xpath', 'action') + +def migrate_advanced_features(asset: 'kpi.models.Asset') -> dict | None: + advanced_features = asset.advanced_features + known_cols = set([col.split(':')[0] for col in asset.known_cols]) + + if advanced_features == {}: + return + + with transaction.atomic(): + for key, value in advanced_features.items(): + if ( + key == 'transcript' + and value + and 'languages' in value + and value['languages'] + ): + for q in known_cols: + QuestionAdvancedAction.objects.create( + question_xpath=q, + asset=asset, + action=Action.MANUAL_TRANSCRIPTION, + params=[ + {'language': language} for language in value['languages'] + ] + ) + + if ( + key == 'translation' + and value + and 'languages' in value + and value['languages'] + ): + for q in known_cols: + QuestionAdvancedAction.objects.create( + question_xpath=q, + asset=asset, + action=Action.MANUAL_TRANSCRIPTION, + params=[ + {'language': language} for language in value['languages'] + ] + ) + if key == 'qual': + # TODO: DEV-1295 + pass + asset.advanced_features = {} + asset.save(update_fields=['advanced_features'], adjust_content=False) + +def validate_submission_supplement(asset: 'kpi.models.Asset', supplement: dict): + jsonschema.validate(get_submission_supplement_schema(asset), supplement) + + +def get_submission_supplement_schema(asset: 'kpi.models.Asset') -> dict: + if asset.advanced_features != {}: + migrate_advanced_features(asset) + + submission_supplement_schema = { + 'additionalProperties': False, + 'properties': {'_version': {'const': SCHEMA_VERSIONS[0]}}, + 'type': 'object', + } + for question_advanced_action in asset.advanced_features_set.all(): + action = question_advanced_action_to_action(question_advanced_action) + submission_supplement_schema['properties'].setdefault( + question_advanced_action.question_xpath, {} + )[question_advanced_action.action] = action.result_schema + + return submission_supplement_schema diff --git a/kobo/apps/subsequences/schema_extensions/__init__.py b/kobo/apps/subsequences/schema_extensions/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/kobo/apps/subsequences/schema_extensions/v2/__init__.py b/kobo/apps/subsequences/schema_extensions/v2/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/kobo/apps/subsequences/schema_extensions/v2/subsequences/extensions.py b/kobo/apps/subsequences/schema_extensions/v2/subsequences/extensions.py new file mode 100644 index 0000000000..6a1d7b3b74 --- /dev/null +++ b/kobo/apps/subsequences/schema_extensions/v2/subsequences/extensions.py @@ -0,0 +1,13 @@ +# This drf-extension made for the metadata field of AccessLog targets the external class +# and tells it what it should return when generating the schema. +from drf_spectacular.extensions import OpenApiSerializerFieldExtension +from drf_spectacular.plumbing import build_array_type + +from kpi.schema_extensions.v2.generic.schema import GENERIC_OBJECT_SCHEMA + + +class SubsequenceParamsFieldExtension(OpenApiSerializerFieldExtension): + target_class = 'kobo.apps.subsequences.schema_extensions.v2.subsequences.fields.AdvancedFeatureParamsField' # noqa + + def map_serializer_field(self, auto_schema, direction): + return build_array_type(schema=GENERIC_OBJECT_SCHEMA) diff --git a/kobo/apps/subsequences/schema_extensions/v2/subsequences/fields.py b/kobo/apps/subsequences/schema_extensions/v2/subsequences/fields.py new file mode 100644 index 0000000000..fbcbd0efe0 --- /dev/null +++ b/kobo/apps/subsequences/schema_extensions/v2/subsequences/fields.py @@ -0,0 +1,5 @@ +from rest_framework import serializers + + +class AdvancedFeatureParamsField(serializers.JSONField): + pass diff --git a/kobo/apps/subsequences/schema_extensions/v2/subsequences/serializers.py b/kobo/apps/subsequences/schema_extensions/v2/subsequences/serializers.py new file mode 100644 index 0000000000..c614eb9184 --- /dev/null +++ b/kobo/apps/subsequences/schema_extensions/v2/subsequences/serializers.py @@ -0,0 +1,14 @@ +from rest_framework import serializers + +from kobo.apps.subsequences.schema_extensions.v2.subsequences.fields import AdvancedFeatureParamsField +from kpi.utils.schema_extensions.serializers import inline_serializer_class + +AdvancedFeatureResponse = inline_serializer_class( + name='AdvancedFeatureResponse', + fields={ + 'question_xpath': serializers.CharField(), + 'action': serializers.CharField(), + 'params': AdvancedFeatureParamsField(), + 'asset': serializers.CharField(), + }, +) diff --git a/kobo/apps/subsequences/schemas.py b/kobo/apps/subsequences/schemas.py index 333ff79c32..ad1108daf6 100644 --- a/kobo/apps/subsequences/schemas.py +++ b/kobo/apps/subsequences/schemas.py @@ -1,8 +1,5 @@ -import jsonschema -from .actions import ACTION_IDS_TO_CLASSES, ACTIONS -from .constants import SCHEMA_VERSIONS -from .utils.versioning import migrate_advanced_features +from .actions import ACTIONS # not the full complexity of XPath, but a slash-delimited path of valid XML tag # names to convey group hierarchy @@ -28,32 +25,3 @@ } -def validate_submission_supplement(asset: 'kpi.models.Asset', supplement: dict): - jsonschema.validate(get_submission_supplement_schema(asset), supplement) - - -def get_submission_supplement_schema(asset: 'kpi.models.Asset') -> dict: - - if migrated_schema := migrate_advanced_features(asset.advanced_features): - asset.advanced_features = migrated_schema - - submission_supplement_schema = { - 'additionalProperties': False, - 'properties': {'_version': {'const': SCHEMA_VERSIONS[0]}}, - 'type': 'object', - } - - for ( - question_xpath, - action_configs_for_this_question, - ) in asset.advanced_features['_actionConfigs'].items(): - for ( - action_id, - action_params, - ) in action_configs_for_this_question.items(): - action = ACTION_IDS_TO_CLASSES[action_id](question_xpath, action_params) - submission_supplement_schema['properties'].setdefault(question_xpath, {})[ - action_id - ] = action.result_schema - - return submission_supplement_schema diff --git a/kobo/apps/subsequences/serializers.py b/kobo/apps/subsequences/serializers.py new file mode 100644 index 0000000000..d62b796497 --- /dev/null +++ b/kobo/apps/subsequences/serializers.py @@ -0,0 +1,37 @@ +import jsonschema.exceptions +from rest_framework import serializers + +from kobo.apps.subsequences.models import QuestionAdvancedAction +from kobo.apps.subsequences.utils.action_conversion import ( + question_advanced_action_to_action, +) + + +class QuestionAdvancedActionUpdateSerializer(serializers.ModelSerializer): + class Meta: + model = QuestionAdvancedAction + fields = ['params', 'question_xpath', 'action', 'asset', 'uid'] + read_only_fields = ['question_xpath', 'action', 'asset', 'uid'] + + def validate(self, attrs): + data = super().validate(attrs) + action = question_advanced_action_to_action(self.instance) + try: + action.__class__.validate_params(attrs.get('params')) + except jsonschema.exceptions.ValidationError as ve: + raise serializers.ValidationError(ve) + return data + + def update(self, instance, validated_data): + action = question_advanced_action_to_action(instance) + action.update_params(validated_data['params']) + instance.params = action.params + instance.save(update_fields=['params']) + return instance + + +class QuestionAdvancedActionSerializer(serializers.ModelSerializer): + class Meta: + model = QuestionAdvancedAction + fields = ['question_xpath', 'action', 'params', 'uid'] + read_only_fields = ['uid'] diff --git a/kobo/apps/subsequences/tasks.py b/kobo/apps/subsequences/tasks.py index 8086402cbd..5927183d56 100644 --- a/kobo/apps/subsequences/tasks.py +++ b/kobo/apps/subsequences/tasks.py @@ -4,9 +4,9 @@ from kobo.celery import celery_app from kpi.utils.django_orm_helper import UpdateJSONFieldAttributes from kobo.apps.subsequences.exceptions import SubsequenceTimeoutError -from .constants import SUBMISSION_UUID_FIELD +from .constants import SUBMISSION_UUID_FIELD, set_version from kobo.apps.openrosa.apps.logger.xform_instance_parser import remove_uuid_prefix -from .utils.versioning import set_version + # With retry_backoff=5 and retry_backoff_max=60, each retry waits: # min(5 * 2^(n-1), 60) seconds. diff --git a/kobo/apps/subsequences/tests/api/v2/test_permissions.py b/kobo/apps/subsequences/tests/api/v2/test_permissions.py index 603a51128c..52fab2a8ff 100644 --- a/kobo/apps/subsequences/tests/api/v2/test_permissions.py +++ b/kobo/apps/subsequences/tests/api/v2/test_permissions.py @@ -4,15 +4,17 @@ from zoneinfo import ZoneInfo from ddt import data, ddt, unpack +from django.urls import reverse from freezegun import freeze_time from rest_framework import status from kobo.apps.kobo_auth.shortcuts import User +from kobo.apps.subsequences.models import QuestionAdvancedAction from kobo.apps.subsequences.tests.api.v2.base import SubsequenceBaseTestCase from kpi.constants import ( PERM_CHANGE_SUBMISSIONS, PERM_PARTIAL_SUBMISSIONS, - PERM_VIEW_SUBMISSIONS, + PERM_VIEW_SUBMISSIONS, PERM_VIEW_ASSET, PERM_CHANGE_METADATA_ASSET, PERM_MANAGE_ASSET, ) from kpi.utils.object_permission import get_anonymous_user @@ -137,17 +139,11 @@ def test_can_write(self, username, shared, status_code): self.client.force_login(user) # Activate advanced features for the project - self.set_asset_advanced_features( - { - '_version': '20250820', - '_actionConfigs': { - 'q1': { - 'manual_transcription': [ - {'language': 'es'}, - ] - } - }, - } + QuestionAdvancedAction.objects.create( + question_xpath='q1', + action='manual_transcription', + params=[{'language': 'es'}], + asset=self.asset, ) if shared: @@ -172,10 +168,12 @@ def test_can_write(self, username, shared, status_code): '_dateModified': '2024-04-08T15:27:00Z', '_versions': [ { + '_data': { + 'language': 'es', + 'value': 'buenas noches', + }, '_dateCreated': '2024-04-08T15:27:00Z', '_dateAccepted': '2024-04-08T15:27:00Z', - 'language': 'es', - 'value': 'buenas noches', '_uuid': '11111111-2222-3333-4444-555555555555', } ], @@ -226,3 +224,152 @@ def test_cannot_read_data(self): self.client.force_login(anotheruser) response = self.client.get(self.supplement_details_url) assert response.status_code == status.HTTP_404_NOT_FOUND + +@ddt +class AdvancedFeaturesPermissionTestCase(SubsequenceBaseTestCase): + + + def setUp(self): + super().setUp() + self.advanced_features_url = reverse('api_v2:advanced-features-list', args=(self.asset.uid,)) + with patch('kobo.apps.subsequences.models.KpiUidField.generate_uid', return_value='12345'): + QuestionAdvancedAction.objects.create( + asset=self.asset, + action='manual_transcription', + question_xpath='q1', + params=[{'language': 'en'}] + ) + + @data( + # owner: Obviously, no need to share. + ( + 'someuser', + False, + status.HTTP_200_OK, + ), + # regular user with no permissions + ( + 'anotheruser', + False, + status.HTTP_404_NOT_FOUND, + ), + # regular user with view permission + ( + 'anotheruser', + True, + status.HTTP_200_OK, + ), + # admin user with no permissions + ( + 'adminuser', + False, + status.HTTP_200_OK, + ), + # admin user with view permissions + ( + 'adminuser', + True, + status.HTTP_200_OK, + ), + # anonymous user with no permissions + ( + 'anonymous', + False, + status.HTTP_404_NOT_FOUND, + ), + # anonymous user with view permissions + ( + 'anonymous', + True, + status.HTTP_200_OK, + ), + ) + @unpack + def test_can_read(self, username, shared, status_code): + user = get_anonymous_user() + self.client.logout() + if username != 'anonymous': + user = User.objects.get(username=username) + self.client.force_login(user) + + if shared: + self.asset.assign_perm(user, PERM_VIEW_ASSET) + + response = self.client.get(self.advanced_features_url) + assert response.status_code == status_code + if status_code == status.HTTP_200_OK: + assert response.data == [{ + 'question_xpath': 'q1', + 'uid': '12345', + 'params': [{'language': 'en'}], + 'action': 'manual_transcription', + }] + + @data( + # owner: Obviously, no need to share. + ( + 'someuser', + False, + status.HTTP_201_CREATED, + ), + # regular user with no permissions + ( + 'anotheruser', + False, + status.HTTP_404_NOT_FOUND, + ), + # regular user with view permission + ( + 'anotheruser', + True, + status.HTTP_201_CREATED, + ), + # admin user with no permissions + ( + 'adminuser', + False, + status.HTTP_201_CREATED, + ), + # admin user with view permissions + ( + 'adminuser', + True, + status.HTTP_201_CREATED, + ), + # anonymous user with no permissions + ( + 'anonymous', + False, + status.HTTP_404_NOT_FOUND, + ), + ) + @unpack + def test_can_write(self, username, shared, status_code): + payload = { + 'action': 'manual_translation', + 'question_xpath': 'q1', + 'params': [{'language':'es'}] + } + + user = get_anonymous_user() + self.client.logout() + if username != 'anonymous': + user = User.objects.get(username=username) + self.client.force_login(user) + + if shared: + self.asset.assign_perm(user, PERM_CHANGE_SUBMISSIONS) + + frozen_datetime_now = datetime(2024, 4, 8, 15, 27, 0, tzinfo=ZoneInfo('UTC')) + with freeze_time(frozen_datetime_now): + response = self.client.post( + self.advanced_features_url, data=payload, format='json' + ) + + assert response.status_code == status_code + if response.status_code == status.HTTP_201_CREATED: + assert QuestionAdvancedAction.objects.filter( + asset=self.asset, + action='manual_translation' + ).exists() + diff --git a/kobo/apps/subsequences/tests/api/v2/test_validation.py b/kobo/apps/subsequences/tests/api/v2/test_validation.py index f6a25eaa0a..21879802a4 100644 --- a/kobo/apps/subsequences/tests/api/v2/test_validation.py +++ b/kobo/apps/subsequences/tests/api/v2/test_validation.py @@ -2,7 +2,7 @@ from rest_framework import status -from kobo.apps.subsequences.models import SubmissionSupplement +from kobo.apps.subsequences.models import QuestionAdvancedAction, SubmissionSupplement from kobo.apps.subsequences.tests.api.v2.base import SubsequenceBaseTestCase from kobo.apps.subsequences.tests.constants import QUESTION_SUPPLEMENT @@ -25,21 +25,16 @@ def test_cannot_patch_if_action_is_invalid(self): self.supplement_details_url, data=payload, format='json' ) assert response.status_code == status.HTTP_400_BAD_REQUEST - assert 'Invalid action' in str(response.data) + assert 'Invalid question' in str(response.data) # Activate manual transcription (even if payload asks for translation) - self.set_asset_advanced_features( - { - '_version': '20250820', - '_actionConfigs': { - 'q1': { - 'manual_transcription': [ - {'language': 'es'}, - ] - } - }, - } + QuestionAdvancedAction.objects.create( + action='manual_transcription', + params=[{'language': 'es'}], + asset=self.asset, + question_xpath='q1', ) + response = self.client.patch( self.supplement_details_url, data=payload, format='json' ) @@ -47,17 +42,11 @@ def test_cannot_patch_if_action_is_invalid(self): assert 'Invalid action' in str(response.data) def test_cannot_patch_with_invalid_payload(self): - self.set_asset_advanced_features( - { - '_version': '20250820', - '_actionConfigs': { - 'q1': { - 'manual_transcription': [ - {'language': 'es'}, - ] - } - }, - } + QuestionAdvancedAction.objects.create( + action='manual_transcription', + params=[{'language': 'es'}], + asset=self.asset, + question_xpath='q1', ) payload = { @@ -79,20 +68,19 @@ def test_cannot_patch_with_invalid_payload(self): def test_cannot_set_value_with_automatic_actions(self): # First, set up the asset to allow automatic actions - advanced_features = { - '_version': '20250820', - '_actionConfigs': { - 'q1': { - 'automatic_google_transcription': [ - {'language': 'en'}, - ], - 'automatic_google_translation': [ - {'language': 'fr'}, - ] - } - }, - } - self.set_asset_advanced_features(advanced_features) + + QuestionAdvancedAction.objects.create( + action='automatic_google_transcription', + params=[{'language': 'en'}], + asset=self.asset, + question_xpath='q1', + ) + QuestionAdvancedAction.objects.create( + action='automatic_google_translation', + params=[{'language': 'fr'}], + asset=self.asset, + question_xpath='q1', + ) # Simulate a completed transcription, first. mock_submission_supplement = { @@ -105,7 +93,10 @@ def test_cannot_set_value_with_automatic_actions(self): content=mock_submission_supplement, asset=self.asset, ) - automatic_actions = advanced_features['_actionConfigs']['q1'].keys() + automatic_actions = [ + 'automatic_google_translation', + 'automatic_google_transcription', + ] for automatic_action in automatic_actions: payload = { '_version': '20250820', @@ -125,17 +116,11 @@ def test_cannot_set_value_with_automatic_actions(self): def test_cannot_accept_incomplete_automatic_transcription(self): # Set up the asset to allow automatic google transcription - self.set_asset_advanced_features( - { - '_version': '20250820', - '_actionConfigs': { - 'q1': { - 'automatic_google_transcription': [ - {'language': 'es'}, - ] - } - }, - } + QuestionAdvancedAction.objects.create( + action='automatic_google_transcription', + params=[{'language': 'en'}], + asset=self.asset, + question_xpath='q1', ) # Try to set 'accepted' status when translation is not complete @@ -165,20 +150,17 @@ def test_cannot_accept_incomplete_automatic_transcription(self): def test_cannot_accept_incomplete_automatic_translation(self): # Set up the asset to allow automatic google actions - self.set_asset_advanced_features( - { - '_version': '20250820', - '_actionConfigs': { - 'q1': { - 'automatic_google_transcription': [ - {'language': 'en'}, - ], - 'automatic_google_translation': [ - {'language': 'fr'}, - ] - } - }, - } + QuestionAdvancedAction.objects.create( + action='automatic_google_transcription', + params=[{'language': 'en'}], + asset=self.asset, + question_xpath='q1', + ) + QuestionAdvancedAction.objects.create( + action='automatic_google_translation', + params=[{'language': 'fr'}], + asset=self.asset, + question_xpath='q1', ) # Simulate a completed transcription, first. @@ -219,20 +201,17 @@ def test_cannot_accept_incomplete_automatic_translation(self): def test_cannot_request_translation_without_transcription(self): # Set up the asset to allow automatic google actions - self.set_asset_advanced_features( - { - '_version': '20250820', - '_actionConfigs': { - 'q1': { - 'automatic_google_transcription': [ - {'language': 'en'}, - ], - 'automatic_google_translation': [ - {'language': 'fr'}, - ] - } - }, - } + QuestionAdvancedAction.objects.create( + action='automatic_google_transcription', + params=[{'language': 'en'}], + asset=self.asset, + question_xpath='q1', + ) + QuestionAdvancedAction.objects.create( + action='automatic_google_translation', + params=[{'language': 'fr'}], + asset=self.asset, + question_xpath='q1', ) # Try to ask for translation diff --git a/kobo/apps/subsequences/tests/api/v2/test_views.py b/kobo/apps/subsequences/tests/api/v2/test_views.py new file mode 100644 index 0000000000..45884f9e54 --- /dev/null +++ b/kobo/apps/subsequences/tests/api/v2/test_views.py @@ -0,0 +1,128 @@ +import json + +from django.test import Client +from django.urls import reverse +from rest_framework import status + +from kobo.apps.kobo_auth.shortcuts import User +from kobo.apps.subsequences.models import QuestionAdvancedAction +from kpi.models import Asset +from kpi.tests.base_test_case import BaseTestCase + + +class QuestionAdvancedActionViewSetTestCase(BaseTestCase): + fixtures = ['test_data'] + + def setUp(self): + user = User.objects.get(username='someuser') + self.asset = Asset.objects.create( + owner=user, + content={'survey': [{'type': 'audio', 'label': 'q1', 'name': 'q1'}]}, + ) + self.action = QuestionAdvancedAction.objects.create( + asset=self.asset, + question_xpath='q1', + action='manual_transcription', + params=[{'language': 'en'}], + ) + self.list_actions_url = reverse( + 'api_v2:advanced-features-list', + kwargs={'parent_lookup_asset': self.asset.uid}, + ) + self.action_detail_url = reverse( + 'api_v2:advanced-features-detail', + kwargs={'parent_lookup_asset': self.asset.uid, 'pk': self.action.uid}, + ) + self.client = Client(raise_request_exception=False) + self.client.force_login(user) + + def test_list_advanced_features(self): + res = self.client.get(self.list_actions_url) + assert res.status_code == status.HTTP_200_OK + assert res.json() == [ + { + 'action': 'manual_transcription', + 'question_xpath': 'q1', + 'params': [{'language': 'en'}], + 'uid': self.action.uid, + } + ] + + def test_update_action(self): + res = self.client.patch( + self.action_detail_url, + content_type='application/json', + data=json.dumps({'params': [{'language': 'es'}]}), + ) + assert res.status_code == status.HTTP_200_OK + self.action.refresh_from_db() + assert self.action.params == [{'language': 'en'}, {'language': 'es'}] + + def test_cannot_update_action_with_invalid_params(self): + res = self.client.patch( + self.action_detail_url, + content_type='application/json', + data=json.dumps({'params': [{'bad': 'stuff'}]}), + ) + assert res.status_code == status.HTTP_400_BAD_REQUEST + self.action.refresh_from_db() + assert self.action.params == [{'language': 'en'}] + + def test_create_action(self): + res = self.client.post( + self.list_actions_url, + data={ + 'action': 'manual_translation', + 'params': json.dumps([{'language': 'de'}]), + 'question_xpath': 'q1', + }, + ) + assert res.status_code == status.HTTP_201_CREATED + new_action = QuestionAdvancedAction.objects.get( + asset=self.asset, action='manual_translation' + ) + assert new_action.params == [{'language': 'de'}] + assert new_action.question_xpath == 'q1' + + def test_cannot_delete_actions(self): + res = self.client.delete(self.action_detail_url) + assert res.status_code == status.HTTP_405_METHOD_NOT_ALLOWED + assert QuestionAdvancedAction.objects.filter( + asset=self.asset, action=self.action.action + ).exists() + + def test_on_the_fly_migration(self): + self.action.delete() + self.asset.advanced_features = {'transcript': {'languages': ['en']}} + self.asset.known_cols = ['q1'] + self.asset.save() + res = self.client.get(self.list_actions_url) + assert QuestionAdvancedAction.objects.filter( + asset=self.asset, action=self.action.action + ).exists() + action = QuestionAdvancedAction.objects.get( + asset=self.asset, action=self.action.action, question_xpath='q1' + ) + assert res.json() == [ + { + 'action': 'manual_transcription', + 'question_xpath': 'q1', + 'params': [{'language': 'en'}], + 'uid': action.uid, + } + ] + + def test_cannot_post_to_unmigrated_asset(self): + self.action.delete() + self.asset.advanced_features = {'transcript': {'languages': ['en']}} + self.asset.known_cols = ['q1'] + self.asset.save() + res = self.client.post( + self.list_actions_url, + data={ + 'action': 'manual_translation', + 'params': json.dumps([{'language': 'de'}]), + 'question_xpath': 'q1', + }, + ) + assert res.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR diff --git a/kobo/apps/subsequences/tests/test_automatic_google_transcription.py b/kobo/apps/subsequences/tests/test_automatic_google_transcription.py index 8b62ce3361..701281b341 100644 --- a/kobo/apps/subsequences/tests/test_automatic_google_transcription.py +++ b/kobo/apps/subsequences/tests/test_automatic_google_transcription.py @@ -336,3 +336,21 @@ def test_latest_version_is_first(): assert mock_sup_det['_versions'][0]['_data']['value'] == 'trois' assert mock_sup_det['_versions'][1]['_data']['value'] == 'deux' assert mock_sup_det['_versions'][2]['_data']['value'] == 'un' + + +def test_update_params_only_adds_new_languages(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = AutomaticGoogleTranscriptionAction(xpath, params) + incoming_params = [{'language': 'en'}, {'language': 'es'}] + action.update_params(incoming_params) + assert sorted(action.languages) == ['en', 'es', 'fr'] + + +def test_update_params_fails_if_new_params_invalid(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = AutomaticGoogleTranscriptionAction(xpath, params) + incoming_params = [{'bad': 'things'}] + with pytest.raises(jsonschema.exceptions.ValidationError): + action.update_params(incoming_params) diff --git a/kobo/apps/subsequences/tests/test_automatic_google_translation.py b/kobo/apps/subsequences/tests/test_automatic_google_translation.py index 8df7dc1540..39807fdec5 100644 --- a/kobo/apps/subsequences/tests/test_automatic_google_translation.py +++ b/kobo/apps/subsequences/tests/test_automatic_google_translation.py @@ -6,8 +6,8 @@ import pytest from ..actions.automatic_google_translation import AutomaticGoogleTranslationAction -from .constants import EMPTY_SUBMISSION, EMPTY_SUPPLEMENT, QUESTION_SUPPLEMENT from ..exceptions import TranscriptionNotFound +from .constants import EMPTY_SUBMISSION, EMPTY_SUPPLEMENT, QUESTION_SUPPLEMENT def test_valid_params_pass_validation(): @@ -414,6 +414,24 @@ def test_action_is_updated_in_background_if_in_progress(): task_mock.apply_async.assert_called_once() +def test_update_params_only_adds_new_languages(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = AutomaticGoogleTranslationAction(xpath, params) + incoming_params = [{'language': 'en'}, {'language': 'es'}] + action.update_params(incoming_params) + assert sorted(action.languages) == ['en', 'es', 'fr'] + + +def test_update_params_fails_if_new_params_invalid(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = AutomaticGoogleTranslationAction(xpath, params) + incoming_params = [{'bad': 'things'}] + with pytest.raises(jsonschema.exceptions.ValidationError): + action.update_params(incoming_params) + + def _get_action(fetch_action_dependencies=True): xpath = 'group_name/question_name' # irrelevant for this test params = [{'language': 'fr'}, {'language': 'es'}] diff --git a/kobo/apps/subsequences/tests/test_manual_transcription.py b/kobo/apps/subsequences/tests/test_manual_transcription.py index a04fb129eb..4016a67212 100644 --- a/kobo/apps/subsequences/tests/test_manual_transcription.py +++ b/kobo/apps/subsequences/tests/test_manual_transcription.py @@ -173,3 +173,21 @@ def test_latest_revision_is_first(): assert mock_sup_det['_versions'][0]['_data']['value'] == 'trois' assert mock_sup_det['_versions'][1]['_data']['value'] == 'deux' assert mock_sup_det['_versions'][2]['_data']['value'] == 'un' + + +def test_update_params_only_adds_new_languages(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = ManualTranscriptionAction(xpath, params) + incoming_params = [{'language': 'en'}, {'language': 'es'}] + action.update_params(incoming_params) + assert sorted(action.languages) == ['en', 'es', 'fr'] + + +def test_update_params_fails_if_new_params_invalid(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = ManualTranscriptionAction(xpath, params) + incoming_params = [{'bad': 'things'}] + with pytest.raises(jsonschema.exceptions.ValidationError): + action.update_params(incoming_params) diff --git a/kobo/apps/subsequences/tests/test_manual_translation.py b/kobo/apps/subsequences/tests/test_manual_translation.py index 5dbc9d5a6d..f62661512d 100644 --- a/kobo/apps/subsequences/tests/test_manual_translation.py +++ b/kobo/apps/subsequences/tests/test_manual_translation.py @@ -2,8 +2,8 @@ import jsonschema import pytest -from ..exceptions import TranscriptionNotFound from ..actions.manual_translation import ManualTranslationAction +from ..exceptions import TranscriptionNotFound from .constants import EMPTY_SUBMISSION, EMPTY_SUPPLEMENT, QUESTION_SUPPLEMENT @@ -177,6 +177,22 @@ def test_cannot_revise_data_without_transcription(): ) +def test_update_params_only_adds_new_languages(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = ManualTranslationAction(xpath, params) + incoming_params = [{'language': 'en'}, {'language': 'es'}] + action.update_params(incoming_params) + assert sorted(action.languages) == ['en', 'es', 'fr'] + + +def test_update_params_fails_if_new_params_invalid(): + xpath = 'group_name/question_name' + params = [{'language': 'fr'}, {'language': 'en'}] + action = ManualTranslationAction(xpath, params) + incoming_params = [{'bad': 'things'}] + with pytest.raises(jsonschema.exceptions.ValidationError): + action.update_params(incoming_params) def _get_action(fetch_action_dependencies=True): diff --git a/kobo/apps/subsequences/tests/test_models.py b/kobo/apps/subsequences/tests/test_models.py index fc13c97f18..55f4a6bf35 100644 --- a/kobo/apps/subsequences/tests/test_models.py +++ b/kobo/apps/subsequences/tests/test_models.py @@ -1,5 +1,4 @@ import uuid -from copy import deepcopy from datetime import datetime from unittest.mock import patch from zoneinfo import ZoneInfo @@ -12,7 +11,7 @@ from kpi.models import Asset from ..constants import SUBMISSION_UUID_FIELD from ..exceptions import InvalidAction, InvalidXPath -from ..models import SubmissionSupplement +from ..models import QuestionAdvancedAction, SubmissionSupplement from .constants import EMPTY_SUPPLEMENT @@ -21,15 +20,6 @@ class SubmissionSupplementTestCase(TestCase): # Asset-level config. # - Allow manual transcription for Arabic # - Allow manual translation for English and Spanish - ADVANCED_FEATURES = { - '_version': '20250820', - '_actionConfigs': { - 'group_name/question_name': { - 'manual_transcription': [{'language': 'ar'}], - 'manual_translation': [{'language': 'en'}, {'language': 'es'}], - } - }, - } EXPECTED_SUBMISSION_SUPPLEMENT = { '_version': '20250820', @@ -124,7 +114,18 @@ def setUp(self): self.asset = Asset.objects.create( owner=self.owner, name='Test Asset', - advanced_features=self.ADVANCED_FEATURES, + ) + QuestionAdvancedAction.objects.create( + asset=self.asset, + question_xpath='group_name/question_name', + action='manual_transcription', + params=[{'language': 'ar'}], + ) + QuestionAdvancedAction.objects.create( + asset=self.asset, + question_xpath='group_name/question_name', + action='manual_translation', + params=[{'language': 'en'}, {'language': 'es'}], ) # Mock submission with minimal info needed for subsequence actions @@ -146,20 +147,6 @@ def test_retrieve_data_with_invalid_arguments(self): self.asset, submission_root_uuid=None, prefetched_supplement=None ) - def test_retrieve_data_with_stale_questions(self): - SubmissionSupplement.objects.create( - asset=self.asset, - submission_uuid=self.submission_root_uuid, - content=self.EXPECTED_SUBMISSION_SUPPLEMENT, - ) - advanced_features = deepcopy(self.ADVANCED_FEATURES) - config = advanced_features['_actionConfigs'].pop('group_name/question_name') - advanced_features['_actionConfigs']['group_name/renamed_question_name'] = config - submission_supplement = SubmissionSupplement.retrieve_data( - self.asset, self.submission_root_uuid - ) - assert submission_supplement == EMPTY_SUPPLEMENT - def test_retrieve_data_from_migrated_data(self): submission_supplement = { 'group_name/question_name': { diff --git a/kobo/apps/subsequences/utils/action_conversion.py b/kobo/apps/subsequences/utils/action_conversion.py new file mode 100644 index 0000000000..2a8caf3de9 --- /dev/null +++ b/kobo/apps/subsequences/utils/action_conversion.py @@ -0,0 +1,10 @@ +from kobo.apps.subsequences.actions import ACTION_IDS_TO_CLASSES + + +def question_advanced_action_to_action(qaa) : + action_class = ACTION_IDS_TO_CLASSES[qaa.action] + return action_class( + source_question_xpath=qaa.question_xpath, + params=qaa.params, + asset=qaa.asset, + ) diff --git a/kobo/apps/subsequences/utils/supplement_data.py b/kobo/apps/subsequences/utils/supplement_data.py index 613bed6611..be53dc1f05 100644 --- a/kobo/apps/subsequences/utils/supplement_data.py +++ b/kobo/apps/subsequences/utils/supplement_data.py @@ -1,11 +1,14 @@ from typing import Generator from kobo.apps.openrosa.apps.logger.xform_instance_parser import remove_uuid_prefix -from kobo.apps.subsequences.actions import ACTION_IDS_TO_CLASSES -from kobo.apps.subsequences.constants import SUBMISSION_UUID_FIELD, SUPPLEMENT_KEY, \ - SCHEMA_VERSIONS -from kobo.apps.subsequences.models import SubmissionSupplement -from kobo.apps.subsequences.utils.versioning import migrate_advanced_features +from kobo.apps.subsequences.constants import SUBMISSION_UUID_FIELD, SUPPLEMENT_KEY +from kobo.apps.subsequences.models import ( + SubmissionSupplement, + migrate_advanced_features, +) +from kobo.apps.subsequences.utils.action_conversion import ( + question_advanced_action_to_action, +) def get_supplemental_output_fields(asset: 'kpi.models.Asset') -> list[dict]: @@ -37,21 +40,15 @@ def get_supplemental_output_fields(asset: 'kpi.models.Asset') -> list[dict]: submission. We'll do that by looking at the acceptance dates and letting the most recent win """ - advanced_features = asset.advanced_features - - if migrated_schema := migrate_advanced_features(advanced_features): - asset.advanced_features = migrated_schema + if asset.advanced_features != {}: + migrate_advanced_features(asset) + advanced_features = asset.advanced_features_set.all() output_fields_by_name = {} # FIXME: `_actionConfigs` is 👎 and should be dropped in favor of top-level configs, eh? # data already exists at the top level alongisde leading-underscore metadata like _version - for source_question_xpath, per_question_actions in advanced_features[ - '_actionConfigs' - ].items(): - for action_id, action_config in per_question_actions.items(): - action = ACTION_IDS_TO_CLASSES[action_id]( - source_question_xpath, action_config - ) + for question_advanced_action in advanced_features: + action = question_advanced_action_to_action(question_advanced_action) for field in action.get_output_fields(): try: existing = output_fields_by_name[field['name']] @@ -98,3 +95,4 @@ def stream_with_supplements( prefetched_supplement=extras.get(submission_uuid, {}), ) yield submission + diff --git a/kobo/apps/subsequences/utils/versioning.py b/kobo/apps/subsequences/utils/versioning.py deleted file mode 100644 index aba7b21852..0000000000 --- a/kobo/apps/subsequences/utils/versioning.py +++ /dev/null @@ -1,44 +0,0 @@ -from ..constants import SCHEMA_VERSIONS - - -def migrate_advanced_features(advanced_features: dict) -> dict | None: - - if advanced_features.get('_version') == SCHEMA_VERSIONS[0]: - return - - migrated_advanced_features = { - '_version': SCHEMA_VERSIONS[0], - '_actionConfigs': {} - } - - actionConfigs = migrated_advanced_features['_actionConfigs'] - for key, value in advanced_features.items(): - if ( - key == 'transcript' - and value - and 'languages' in value - and value['languages'] - ): - actionConfigs['manual_transcription'] = [ - {'language': language} for language in value['languages'] - ] - - if ( - key == 'translation' - and value - and 'languages' in value - and value['languages'] - ): - actionConfigs['manual_translation'] = [ - {'language': language} for language in value['languages'] - ] - - if key == 'qual': - raise NotImplementedError - - return migrated_advanced_features - - -def set_version(schema: dict) -> dict: - schema['_version'] = SCHEMA_VERSIONS[0] - return schema diff --git a/kobo/apps/subsequences/views.py b/kobo/apps/subsequences/views.py new file mode 100644 index 0000000000..44761a8a8e --- /dev/null +++ b/kobo/apps/subsequences/views.py @@ -0,0 +1,121 @@ +from drf_spectacular.utils import OpenApiParameter, extend_schema, extend_schema_view +from rest_framework import mixins +from rest_framework_extensions.mixins import NestedViewSetMixin + +from kobo.apps.audit_log.base_views import AuditLoggedViewSet +from kobo.apps.audit_log.models import AuditType +from kobo.apps.subsequences.models import ( + QuestionAdvancedAction, + migrate_advanced_features, +) +from kobo.apps.subsequences.schema_extensions.v2.subsequences.serializers import ( + AdvancedFeatureResponse, +) +from kobo.apps.subsequences.serializers import ( + QuestionAdvancedActionSerializer, + QuestionAdvancedActionUpdateSerializer, +) +from kpi.permissions import AssetAdvancedFeaturesPermission +from kpi.utils.schema_extensions.markdown import read_md +from kpi.utils.schema_extensions.response import ( + open_api_200_ok_response, + open_api_201_created_response, +) +from kpi.utils.viewset_mixins import AssetNestedObjectViewsetMixin + + +@extend_schema( + tags=['Advanced Features'], + parameters=[ + OpenApiParameter( + name='parent_lookup_asset', + type=str, + location=OpenApiParameter.PATH, + required=True, + description='UID of the parent assets', + ), + ], +) +@extend_schema_view( + create=extend_schema( + description=read_md('subsequences', 'subsequences/create.md'), + responses=open_api_201_created_response( + AdvancedFeatureResponse, + require_auth=False, + raise_access_forbidden=False, + ), + ), + list=extend_schema( + description=read_md('subsequences', 'subsequences/list.md'), + responses=open_api_200_ok_response( + AdvancedFeatureResponse, + require_auth=False, + raise_access_forbidden=False, + validate_payload=False, + ), + ), + partial_update=extend_schema( + description=read_md('subsequences', 'subsequences/update.md'), + responses=open_api_200_ok_response( + QuestionAdvancedActionUpdateSerializer, + require_auth=False, + raise_access_forbidden=False, + ), + parameters=[ + OpenApiParameter( + name='uid', + type=str, + location=OpenApiParameter.PATH, + required=True, + description='UID of the action', + ), + ], + ), + retrieve=extend_schema( + description=read_md('subsequences', 'subsequences/retrieve.md'), + responses=open_api_200_ok_response( + AdvancedFeatureResponse, + require_auth=False, + raise_access_forbidden=False, + validate_payload=False, + ), + parameters=[ + OpenApiParameter( + name='uid', + type=str, + location=OpenApiParameter.PATH, + required=True, + description='UID of the action', + ), + ], + ), + update=extend_schema( + exclude=True, + ), +) +class QuestionAdvancedActionViewSet( + AuditLoggedViewSet, + mixins.CreateModelMixin, + mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, + mixins.ListModelMixin, + AssetNestedObjectViewsetMixin, + NestedViewSetMixin, +): + log_type = AuditType.PROJECT_HISTORY + logged_fields = ['asset.owner.username', 'action', 'params',('object_id', 'asset.id'),] + pagination_class = None + permission_classes = (AssetAdvancedFeaturesPermission,) + def get_queryset(self): + if self.asset.advanced_features != {}: + migrate_advanced_features(self.asset) + return QuestionAdvancedAction.objects.filter(asset=self.asset) + def perform_create_override(self, serializer): + if self.asset.advanced_features != {}: + raise Exception('Migrate advanced features before creating new actions') + serializer.save(asset=self.asset) + def get_serializer_class(self): + if self.action in ['update', 'partial_update']: + return QuestionAdvancedActionUpdateSerializer + else: + return QuestionAdvancedActionSerializer diff --git a/kpi/models/asset.py b/kpi/models/asset.py index 87b0f35e50..7375061170 100644 --- a/kpi/models/asset.py +++ b/kpi/models/asset.py @@ -19,9 +19,7 @@ from kobo.apps.data_collectors.models import DataCollectorGroup from kobo.apps.reports.constants import DEFAULT_REPORTS_KEY, SPECIFIC_REPORTS_KEY -from kobo.apps.subsequences.schemas import ACTION_PARAMS_SCHEMA from kobo.apps.subsequences.utils.supplement_data import get_supplemental_output_fields -from kobo.apps.subsequences.utils.versioning import migrate_advanced_features from kpi.constants import ( ASSET_TYPE_BLOCK, ASSET_TYPE_COLLECTION, @@ -929,11 +927,6 @@ def save( if adjust_content: self.adjust_content_on_save() - if ( - not update_fields - or update_fields and 'advanced_features' in update_fields - ): - self.validate_advanced_features() # standardize settings (only when required) if ( @@ -1152,19 +1145,8 @@ def update_languages(self, children=None): self.save(update_fields=['summary']) def validate_advanced_features(self): - if self.advanced_features is None: - self.advanced_features = {} - - if migrated_schema := migrate_advanced_features(self.advanced_features): - self.advanced_features = migrated_schema - # We should save the new schema, but for debugging purposes, - # we don't yet! - # self.save(update_fields=['advanced_features']) - - jsonschema.validate( - instance=self.advanced_features, - schema=ACTION_PARAMS_SCHEMA, - ) + # TODO: remove this once subsequences__old is deleted + pass @property def version__content_hash(self): diff --git a/kpi/permissions.py b/kpi/permissions.py index f326c141e3..906020a889 100644 --- a/kpi/permissions.py +++ b/kpi/permissions.py @@ -222,6 +222,19 @@ def has_permission(self, request, view): # permission to view it raise Http404 +class AssetAdvancedFeaturesPermission(AssetNestedObjectPermission): + """ + Owner, managers and editors can write. + i.e.: + - Reads need 'view_asset' permission + - Writes need 'change_submissions' permission + """ + + perms_map = deepcopy(AssetNestedObjectPermission.perms_map) + perms_map['POST'] = ['%(app_label)s.change_submissions'] + perms_map['PUT'] = perms_map['POST'] + perms_map['PATCH'] = perms_map['POST'] + perms_map['DELETE'] = perms_map['POST'] class AssetEditorPermission(AssetNestedObjectPermission): """ diff --git a/kpi/schema_extensions/imports.py b/kpi/schema_extensions/imports.py index 8d6bef6fdd..f18bbdec6a 100644 --- a/kpi/schema_extensions/imports.py +++ b/kpi/schema_extensions/imports.py @@ -1,4 +1,5 @@ # flake8: noqa: F401 +import kobo.apps.subsequences.schema_extensions.v2.subsequences.extensions import kpi.schema_extensions.v2.asset_attachments.extensions import kpi.schema_extensions.v2.asset_permission_assignments.extensions import kpi.schema_extensions.v2.asset_snapshots.extensions diff --git a/kpi/schema_extensions/v2/assets/extensions.py b/kpi/schema_extensions/v2/assets/extensions.py index d2131cf76b..84da262786 100644 --- a/kpi/schema_extensions/v2/assets/extensions.py +++ b/kpi/schema_extensions/v2/assets/extensions.py @@ -38,11 +38,14 @@ def map_serializer_field(self, auto_schema, direction): return GENERIC_ARRAY_SCHEMA -class AdvancedFeatureFieldExtension(OpenApiSerializerFieldExtension): - target_class = 'kpi.schema_extensions.v2.assets.fields.AdvancedFeatureField' +class AdvancedFeaturesLinkFieldExtension(OpenApiSerializerFieldExtension): + target_class = 'kpi.schema_extensions.v2.assets.fields.AdvancedFeaturesLinkField' def map_serializer_field(self, auto_schema, direction): - return GENERIC_OBJECT_SCHEMA + return build_url_type( + 'api_v2:advanced-features-list', + parent_lookup_asset='aBeA23YCYjkGTFvYVHuAyU', + ) class AdvancedSubmissionSchemaFieldExtension(OpenApiSerializerFieldExtension): diff --git a/kpi/schema_extensions/v2/assets/fields.py b/kpi/schema_extensions/v2/assets/fields.py index 50f63b19ae..64c2c42d81 100644 --- a/kpi/schema_extensions/v2/assets/fields.py +++ b/kpi/schema_extensions/v2/assets/fields.py @@ -5,7 +5,7 @@ class AccessTypeField(serializers.ListField): pass -class AdvancedFeatureField(serializers.JSONField): +class AdvancedFeaturesLinkField(serializers.URLField): pass diff --git a/kpi/serializers/v2/asset.py b/kpi/serializers/v2/asset.py index bba8de95fb..c7f2f62246 100644 --- a/kpi/serializers/v2/asset.py +++ b/kpi/serializers/v2/asset.py @@ -62,7 +62,7 @@ ) from ...schema_extensions.v2.assets.fields import ( AccessTypeField, - AdvancedFeatureField, + AdvancedFeaturesLinkField, AdvancedSubmissionSchemaField, AnalysisFormJsonField, AssetHyperlinkedURLField, @@ -91,7 +91,7 @@ SummaryField, UserURLRelativeHyperlinkedRelatedField, XFormLinkField, - XLSLinkField, + XLSLinkField, AdvancedFeaturesLinkField, ) from .asset_export_settings import AssetExportSettingsSerializer from .asset_file import AssetFileSerializer @@ -343,9 +343,6 @@ class AssetSerializer(serializers.HyperlinkedModelSerializer): map_custom = WriteableJsonWithSchemaField( schema_field=MapCustomField, required=False ) - advanced_features = WriteableJsonWithSchemaField( - schema_field=AdvancedFeatureField, required=False - ) files = serializers.SerializerMethodField() xls_link = serializers.SerializerMethodField() summary = ReadOnlyFieldWithSchemaField(schema_field=SummaryField) @@ -403,6 +400,7 @@ class AssetSerializer(serializers.HyperlinkedModelSerializer): paired_data = serializers.SerializerMethodField() project_ownership = serializers.SerializerMethodField() kind = serializers.SerializerMethodField() + advanced_features = serializers.SerializerMethodField() class Meta: model = Asset @@ -435,7 +433,6 @@ class Meta: 'deployment_status', 'report_styles', 'report_custom', - 'advanced_features', 'supplemental_output_fields', 'map_styles', 'map_custom', @@ -464,7 +461,8 @@ class Meta: 'paired_data', 'project_ownership', 'owner_label', - 'last_modified_by' + 'last_modified_by', + 'advanced_features', ) read_only_fields = ('last_modified_by', 'uid') extra_kwargs = { @@ -944,6 +942,12 @@ def get_owner_label(self, asset): return organization.name return asset.owner.username + @extend_schema_field(AdvancedFeaturesLinkField) + def get_advanced_features(self, obj): + return reverse('advanced-features-list', + args=(obj.uid,), + request=self.context.get('request', None)) + def validate_data_sharing(self, data_sharing: dict) -> dict: """ Validates `data_sharing`. It is really basic. diff --git a/kpi/urls/router_api_v2.py b/kpi/urls/router_api_v2.py index 55e358815d..aa2a52d954 100644 --- a/kpi/urls/router_api_v2.py +++ b/kpi/urls/router_api_v2.py @@ -14,6 +14,7 @@ ) from kobo.apps.project_ownership.urls import router as project_ownership_router from kobo.apps.project_views.views import ProjectViewViewSet +from kobo.apps.subsequences.views import QuestionAdvancedActionViewSet from kpi.views.v2.asset import AssetViewSet from kpi.views.v2.asset_counts import AssetCountsViewSet from kpi.views.v2.asset_export_settings import AssetExportSettingsViewSet @@ -138,6 +139,8 @@ def get_urls(self, *args, **kwargs): parents_query_lookups=['asset'], ) +asset_routes.register(r'advanced-features', QuestionAdvancedActionViewSet, basename='advanced-features', parents_query_lookups=['asset']) + data_routes = asset_routes.register(r'data', DataViewSet, basename='submission', diff --git a/kpi/utils/schema_extensions/url_builder.py b/kpi/utils/schema_extensions/url_builder.py index 89a03a5044..0069470cce 100644 --- a/kpi/utils/schema_extensions/url_builder.py +++ b/kpi/utils/schema_extensions/url_builder.py @@ -28,6 +28,8 @@ def build_url_type(viewname: str, **kwargs) -> dict: _, viewname = viewname.split(':') urls_pattern_mapping = { + 'advanced-features-list': '/api/v2/assets/{parent_lookup_asset}/advanced-features', + 'advanced-features-detail': '/api/v2/assets/{parent_lookup_asset}/advanced-features/{uid}', 'asset-detail': '/api/v2/assets/{uid}/', 'asset-permission-assignment-detail': '/api/v2/assets/{parent_lookup_asset}/permission-assignments/{uid}/', # noqa 'permission-detail': '/api/v2/permissions/{codename}/',