Merge pull request #417 from dandi/validation-error-ux

Improve validation error UX

dandiapi/api/migrations/0015_validation_fields.py

@@ -0,0 +1,31 @@
+# Generated by Django 3.2.4 on 2021-07-26 15:52
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('api', '0014_alter_stagingapplication_skip_authorization'),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name='asset',
+            name='validation_error',
+        ),
+        migrations.RemoveField(
+            model_name='version',
+            name='validation_error',
+        ),
+        migrations.AddField(
+            model_name='asset',
+            name='validation_errors',
+            field=models.JSONField(default=list),
+        ),
+        migrations.AddField(
+            model_name='version',
+            name='validation_errors',
+            field=models.JSONField(default=list),
+        ),
+    ]

dandiapi/api/models/asset.py

@@ -105,7 +105,7 @@ class Status(models.TextChoices):
         default=Status.PENDING,
         choices=Status.choices,
     )
-    validation_error = models.TextField(default='', blank=True)
+    validation_errors = models.JSONField(default=list)
     previous = models.ForeignKey(
         'Asset',
         blank=True,

dandiapi/api/models/version.py

@@ -72,7 +72,7 @@ class Status(models.TextChoices):
         default=Status.PENDING,
         choices=Status.choices,
     )
-    validation_error = models.TextField(default='', blank=True)
+    validation_errors = models.JSONField(default=list)
 
     class Meta:
         unique_together = ['dandiset', 'version']
@@ -115,27 +115,33 @@ def publish_status(self) -> Version.Status:
         return Version.Status.VALID
 
     @property
-    def publish_validation_error(self) -> str:
-        if self.validation_error:
-            return self.validation_error
-
+    def asset_validation_errors(self) -> list[str]:
         # Import here to avoid dependency cycle
         from .asset import Asset
 
         # Assets that are not VALID (could be pending, validating, or invalid)
-        invalid_assets = self.assets.filter(status=Asset.Status.INVALID).count()
-        if invalid_assets > 0:
-            return f'{invalid_assets} invalid asset metadatas'
+        invalid_assets = self.assets.filter(status=Asset.Status.INVALID).values('validation_errors')
+
+        errors = [
+            error
+            for invalid_asset in invalid_assets
+            for error in invalid_asset['validation_errors']
+        ]
 
         # Assets that have not yet been validated (could be pending or validating)
         unvalidated_assets = self.assets.filter(
             ~(models.Q(status=Asset.Status.VALID) | models.Q(status=Asset.Status.INVALID))
-        ).count()
-        if unvalidated_assets > 0:
-            return f'{unvalidated_assets} assets have not been validated yet'
+        ).values('path')
+
+        errors += [
+            {
+                'field': unvalidated_asset['path'],
+                'message': 'asset is currently being validated, please wait.',
+            }
+            for unvalidated_asset in unvalidated_assets
+        ]
 
-        # No error == ''
-        return ''
+        return errors
 
     @staticmethod
     def datetime_to_version(time: datetime.datetime) -> str:

dandiapi/api/tasks.py

@@ -100,12 +100,20 @@ def validate_asset_metadata(asset_id: int) -> None:
         logger.error('Error while validating asset %s', asset_id)
         logger.error(str(e))
         asset.status = Asset.Status.INVALID
-        asset.validation_error = str(e)
+
+        if type(e) is ValueError:
+            asset.validation_errors = [{'field': '', 'message': str(e)}]
+        else:
+            # parse the pydantic ValidationError:
+            asset.validation_errors = [
+                {'field': err['loc'][0], 'message': err['msg']} for err in e.errors()
+            ]
+
         asset.save()
         return
     logger.info('Successfully validated asset %s', asset_id)
     asset.status = Asset.Status.VALID
-    asset.validation_error = ''
+    asset.validation_errors = []
     asset.save()
 
 
@@ -130,10 +138,18 @@ def validate_version_metadata(version_id: int) -> None:
         logger.error('Error while validating version %s', version_id)
         logger.error(str(e))
         version.status = Version.Status.INVALID
-        version.validation_error = str(e)
+
+        if type(e) is ValueError:
+            version.validation_errors = [{'field': '', 'message': str(e)}]
+        else:
+            # parse the pydantic ValidationError:
+            version.validation_errors = [
+                {'field': err['loc'][0], 'message': err['msg']} for err in e.errors()
+            ]
+
         version.save()
         return
     logger.info('Successfully validated version %s', version_id)
     version.status = Version.Status.VALID
-    version.validation_error = ''
+    version.validation_errors = []
     version.save()

dandiapi/api/tests/test_asset.py

@@ -240,15 +240,15 @@ def test_asset_rest_validation(api_client, version, asset, status, validation_er
     version.assets.add(asset)
 
     asset.status = status
-    asset.validation_error = validation_error
+    asset.validation_errors = validation_error
     asset.save()
 
     assert api_client.get(
         f'/api/dandisets/{version.dandiset.identifier}/'
         f'versions/{version.version}/assets/{asset.asset_id}/validation/'
     ).data == {
         'status': status,
-        'validation_error': validation_error,
+        'validation_errors': validation_error,
     }
 
 

dandiapi/api/tests/test_tasks.py

@@ -73,7 +73,7 @@ def test_validate_asset_metadata(asset: Asset):
     asset.refresh_from_db()
 
     assert asset.status == Asset.Status.VALID
-    assert asset.validation_error == ''
+    assert asset.validation_errors == []
 
 
 @pytest.mark.django_db
@@ -86,7 +86,9 @@ def test_validate_asset_metadata_no_schema_version(asset: Asset):
     asset.refresh_from_db()
 
     assert asset.status == Asset.Status.INVALID
-    assert asset.validation_error.startswith('Metadata version None is not allowed.')
+    assert len(asset.validation_errors) == 1
+    assert asset.validation_errors[0]['field'] == ''
+    assert asset.validation_errors[0]['message'].startswith('Metadata version None is not allowed.')
 
 
 @pytest.mark.django_db
@@ -99,7 +101,9 @@ def test_validate_asset_metadata_malformed_schema_version(asset: Asset):
     asset.refresh_from_db()
 
     assert asset.status == Asset.Status.INVALID
-    assert asset.validation_error.startswith('Metadata version xxx is not allowed.')
+    assert len(asset.validation_errors) == 1
+    assert asset.validation_errors[0]['field'] == ''
+    assert asset.validation_errors[0]['message'].startswith('Metadata version xxx is not allowed.')
 
 
 @pytest.mark.django_db
@@ -112,11 +116,7 @@ def test_validate_asset_metadata_no_encoding_format(asset: Asset):
     asset.refresh_from_db()
 
     assert asset.status == Asset.Status.INVALID
-    assert asset.validation_error == (
-        '1 validation error for PublishedAsset\n'
-        'encodingFormat\n'
-        '  field required (type=value_error.missing)'
-    )
+    assert asset.validation_errors == [{'field': 'encodingFormat', 'message': 'field required'}]
 
 
 @pytest.mark.django_db
@@ -129,11 +129,9 @@ def test_validate_asset_metadata_no_digest(asset: Asset):
     asset.refresh_from_db()
 
     assert asset.status == Asset.Status.INVALID
-    assert asset.validation_error == (
-        '1 validation error for PublishedAsset\n'
-        'digest\n'
-        '  Digest is missing dandi-etag or sha256 keys. (type=value_error)'
-    )
+    assert asset.validation_errors == [
+        {'field': 'digest', 'message': 'Digest is missing dandi-etag or sha256 keys.'}
+    ]
 
 
 @pytest.mark.django_db
@@ -146,11 +144,9 @@ def test_validate_asset_metadata_malformed_keywords(asset: Asset):
     asset.refresh_from_db()
 
     assert asset.status == Asset.Status.INVALID
-    assert asset.validation_error == (
-        '1 validation error for PublishedAsset\n'
-        'keywords\n'
-        '  value is not a valid list (type=type_error.list)'
-    )
+    assert asset.validation_errors == [
+        {'field': 'keywords', 'message': 'value is not a valid list'}
+    ]
 
 
 @pytest.mark.django_db
@@ -162,7 +158,7 @@ def test_validate_version_metadata(version: Version, asset: Asset):
     version.refresh_from_db()
 
     assert version.status == Version.Status.VALID
-    assert version.validation_error == ''
+    assert version.validation_errors == []
 
 
 @pytest.mark.django_db
@@ -177,7 +173,11 @@ def test_validate_version_metadata_no_schema_version(version: Version, asset: As
     version.refresh_from_db()
 
     assert version.status == Version.Status.INVALID
-    assert version.validation_error.startswith('Metadata version None is not allowed.')
+    assert len(version.validation_errors) == 1
+    assert version.validation_errors[0]['field'] == ''
+    assert version.validation_errors[0]['message'].startswith(
+        'Metadata version None is not allowed.'
+    )
 
 
 @pytest.mark.django_db
@@ -192,7 +192,10 @@ def test_validate_version_metadata_malformed_schema_version(version: Version, as
     version.refresh_from_db()
 
     assert version.status == Version.Status.INVALID
-    assert version.validation_error.startswith('Metadata version xxx is not allowed.')
+    assert len(version.validation_errors) == 1
+    assert version.validation_errors[0]['message'].startswith(
+        'Metadata version xxx is not allowed.'
+    )
 
 
 @pytest.mark.django_db
@@ -207,11 +210,7 @@ def test_validate_version_metadata_no_description(version: Version, asset: Asset
     version.refresh_from_db()
 
     assert version.status == Version.Status.INVALID
-    assert version.validation_error == (
-        '1 validation error for PublishedDandiset\n'
-        'description\n'
-        '  field required (type=value_error.missing)'
-    )
+    assert version.validation_errors == [{'field': 'description', 'message': 'field required'}]
 
 
 @pytest.mark.django_db
@@ -226,8 +225,6 @@ def test_validate_version_metadata_malformed_license(version: Version, asset: As
     version.refresh_from_db()
 
     assert version.status == Version.Status.INVALID
-    assert version.validation_error == (
-        '1 validation error for PublishedDandiset\n'
-        'license\n'
-        '  value is not a valid list (type=type_error.list)'
-    )
+    assert version.validation_errors == [
+        {'field': 'license', 'message': 'value is not a valid list'}
+    ]

dandiapi/api/tests/test_version.py

@@ -354,7 +354,8 @@ def test_version_rest_info(api_client, version):
         'metadata': version.metadata.metadata,
         'size': version.size,
         'status': 'Pending',
-        'validation_error': '',
+        'asset_validation_errors': [],
+        'version_validation_errors': [],
         'contact_person': '',
     }
 
@@ -363,10 +364,16 @@ def test_version_rest_info(api_client, version):
 @pytest.mark.parametrize(
     'asset_status,expected_validation_error',
     [
-        (Asset.Status.PENDING, '1 assets have not been validated yet'),
-        (Asset.Status.VALIDATING, '1 assets have not been validated yet'),
-        (Asset.Status.VALID, ''),
-        (Asset.Status.INVALID, '1 invalid asset metadatas'),
+        (
+            Asset.Status.PENDING,
+            [{'field': '', 'message': 'asset is currently being validated, please wait.'}],
+        ),
+        (
+            Asset.Status.VALIDATING,
+            [{'field': '', 'message': 'asset is currently being validated, please wait.'}],
+        ),
+        (Asset.Status.VALID, []),
+        (Asset.Status.INVALID, []),
     ],
 )
 def test_version_rest_info_with_asset(
@@ -380,6 +387,10 @@ def test_version_rest_info_with_asset(
     asset = asset_factory(status=asset_status)
     version.assets.add(asset)
 
+    # These validation error types should have the asset path prepended to them:
+    if asset_status == Asset.Status.PENDING or asset_status == Asset.Status.VALIDATING:
+        expected_validation_error[0]['field'] = asset.path
+
     assert api_client.get(
         f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/info/'
     ).data == {
@@ -397,7 +408,8 @@ def test_version_rest_info_with_asset(
         'metadata': version.metadata.metadata,
         'size': version.size,
         'status': 'Valid' if asset_status == Asset.Status.VALID else 'Invalid',
-        'validation_error': expected_validation_error,
+        'asset_validation_errors': expected_validation_error,
+        'version_validation_errors': [],
         'contact_person': '',
     }
 
@@ -451,7 +463,8 @@ def test_version_rest_update(api_client, user, draft_version):
         'metadata': saved_metadata,
         'size': draft_version.size,
         'status': 'Pending',
-        'validation_error': '',
+        'asset_validation_errors': [],
+        'version_validation_errors': [],
         'contact_person': '',
     }
 
@@ -519,7 +532,8 @@ def test_version_rest_update_large(api_client, user, draft_version):
         'metadata': saved_metadata,
         'size': draft_version.size,
         'status': 'Pending',
-        'validation_error': '',
+        'asset_validation_errors': [],
+        'version_validation_errors': [],
         'contact_person': '',
     }
 

dandiapi/api/views/serializers.py

@@ -91,11 +91,18 @@ class VersionDetailSerializer(VersionSerializer):
     contact_person = serializers.SerializerMethodField(method_name='get_contact_person')
 
     class Meta(VersionSerializer.Meta):
-        fields = VersionSerializer.Meta.fields + ['validation_error', 'metadata', 'contact_person']
+        fields = VersionSerializer.Meta.fields + [
+            'asset_validation_errors',
+            'version_validation_errors',
+            'metadata',
+            'contact_person',
+        ]
 
     metadata = serializers.SlugRelatedField(read_only=True, slug_field='metadata')
     status = serializers.CharField(source='publish_status')
-    validation_error = serializers.CharField(source='publish_validation_error')
+
+    # rename this field in the serializer to differentiate from asset_validation_errors
+    version_validation_errors = serializers.JSONField(source='validation_errors')
 
     def get_contact_person(self, obj):
         return extract_contact_person(obj)
@@ -121,7 +128,7 @@ class Meta:
 class AssetValidationSerializer(serializers.ModelSerializer):
     class Meta:
         model = Asset
-        fields = ['status', 'validation_error']
+        fields = ['status', 'validation_errors']
 
 
 class AssetSerializer(serializers.ModelSerializer):