From 12a4bb522a64af87ab6d5fb790d6bb9045d3c665 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 24 Apr 2023 17:01:40 -0400 Subject: [PATCH 1/6] Return object for asset validation errors The object maps asset paths to their corresponding errors --- dandiapi/api/models/version.py | 30 +++++++++++++---------- dandiapi/api/tests/test_version.py | 38 ++++++++++++------------------ 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/dandiapi/api/models/version.py b/dandiapi/api/models/version.py index c3ce3e17e..118ebddc3 100644 --- a/dandiapi/api/models/version.py +++ b/dandiapi/api/models/version.py @@ -81,7 +81,7 @@ def valid(self) -> bool: return not self.assets.exclude(status=Asset.Status.VALID).exists() @property - def asset_validation_errors(self) -> list[str]: + def asset_validation_errors(self) -> dict[str, list[dict[str, str]]]: # Import here to avoid dependency cycle from .asset import Asset @@ -90,20 +90,24 @@ def asset_validation_errors(self) -> list[str]: status=Asset.Status.VALID ).values('status', 'path', 'validation_errors') - # Aggregate errors - errors = [] + asset_validating_error = { + 'field': '', + 'message': 'asset is currently being validated, please wait.', + } + + # Aggregate errors, ensuring the path of the asset is included + asset_error_map = {} for asset in invalid_assets: - if asset['status'] == Asset.Status.INVALID: - errors.extend(asset['validation_errors']) - else: - errors.append( - { - 'field': asset['path'], - 'message': 'asset is currently being validated, please wait.', - } - ) + # Must be pending or validating + if asset['status'] != Asset.Status.INVALID: + asset_error_map[asset['path']] = [asset_validating_error] + continue + + # Must be invalid, only add entry in map if it has any errors + if asset['validation_errors']: + asset_error_map[asset['path']] = asset['validation_errors'] - return errors + return asset_error_map @staticmethod def datetime_to_version(time: datetime.datetime) -> str: diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index eb33ce59c..05ebd3cfd 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -414,7 +414,7 @@ def test_version_rest_info(api_client, version): 'metadata': version.metadata, 'size': version.size, 'status': 'Pending', - 'asset_validation_errors': [], + 'asset_validation_errors': {}, 'version_validation_errors': [], 'contact_person': version.metadata['contributor'][0]['name'], } @@ -422,35 +422,27 @@ def test_version_rest_info(api_client, version): @pytest.mark.django_db @pytest.mark.parametrize( - 'asset_status,expected_validation_error', - [ - ( - 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, []), - ], + 'asset_status', + [Asset.Status.PENDING, Asset.Status.VALIDATING, Asset.Status.VALID, Asset.Status.INVALID], ) def test_version_rest_info_with_asset( - api_client, - draft_version_factory, - draft_asset_factory, - asset_status: Asset.Status, - expected_validation_error: str, + api_client, draft_version_factory, draft_asset_factory, asset_status: Asset.Status ): version = draft_version_factory(status=Version.Status.VALID) asset = draft_asset_factory(status=asset_status) version.assets.add(asset) add_version_asset_paths(version=version) - # 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 + # Create expected validation errors for pending/validating assets + asset_validating_error = { + 'field': '', + 'message': 'asset is currently being validated, please wait.', + } + expected_validation_error = ( + {asset.path: [asset_validating_error]} + if asset_status in [Asset.Status.PENDING, Asset.Status.VALIDATING] + else {} + ) assert api_client.get( f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/info/' @@ -545,7 +537,7 @@ def test_version_rest_update(api_client, user, draft_version): 'metadata': saved_metadata, 'size': draft_version.size, 'status': 'Pending', - 'asset_validation_errors': [], + 'asset_validation_errors': {}, 'version_validation_errors': [], 'contact_person': 'Vargas, GetĂșlio', } From 3a2337977bf78fe208846f5b4189683f4a9efb2b Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 24 Apr 2023 17:03:46 -0400 Subject: [PATCH 2/6] Move version/asset validation errors into dialog --- .../components/DLP/ValidationErrorDialog.vue | 152 +++++++++++ web/src/types/index.ts | 2 +- .../DandisetLandingView/DandisetPublish.vue | 252 +++++++----------- 3 files changed, 247 insertions(+), 159 deletions(-) create mode 100644 web/src/components/DLP/ValidationErrorDialog.vue diff --git a/web/src/components/DLP/ValidationErrorDialog.vue b/web/src/components/DLP/ValidationErrorDialog.vue new file mode 100644 index 000000000..fae705d08 --- /dev/null +++ b/web/src/components/DLP/ValidationErrorDialog.vue @@ -0,0 +1,152 @@ + + + diff --git a/web/src/types/index.ts b/web/src/types/index.ts index 550ea4d08..bdba4b10c 100644 --- a/web/src/types/index.ts +++ b/web/src/types/index.ts @@ -43,7 +43,7 @@ export interface Version { modified: string, dandiset: Dandiset, metadata?: DandisetMetadata, - asset_validation_errors: ValidationError[], + asset_validation_errors: Record, version_validation_errors: ValidationError[], contact_person?: string, } diff --git a/web/src/views/DandisetLandingView/DandisetPublish.vue b/web/src/views/DandisetLandingView/DandisetPublish.vue index 0caf78b6b..d59f36aab 100644 --- a/web/src/views/DandisetLandingView/DandisetPublish.vue +++ b/web/src/views/DandisetLandingView/DandisetPublish.vue @@ -200,167 +200,78 @@ - + + + + + + - - - - + + -
- - - - {{ getValidationErrorIcon(error.field) }} - - - - - - {{ error.message }} - - - -
-
- + + + +
- Fix issues - - - - - - + + + + + + - - - - + + -
- - - - {{ getValidationErrorIcon(error.field) }} - - - - - - {{ error.message }} - - - -
-
-
-
- + mdi-database-remove + + + + +
+ This Dandiset has {{ numAssetValidationErrors }} + asset validation error(s). +
+
+ +
+ This Version @@ -449,7 +360,9 @@ import router from '@/router'; import type { User, Version } from '@/types'; import { draftVersion, VALIDATION_ICONS } from '@/utils/constants'; -import { open as openMeditor } from '@/components/Meditor/state'; +import { open as meditorOpen } from '@/components/Meditor/state'; + +import ValidationErrorDialog from '@/components/DLP/ValidationErrorDialog.vue'; const PUBLISH_CHECKLIST = [ 'A descriptive title (e.g., Data related to foraging behavior in bees rather than Smith et al 2022)', @@ -586,9 +499,32 @@ onUnmounted(() => { window.clearInterval(timer); }); +// Error dialog +const errorDialogOpen = ref(false); +type ErrorCategory = 'metadata' | 'assets'; +const selectedTab = ref('metadata'); +function openErrorDialog(tab: ErrorCategory) { + errorDialogOpen.value = true; + selectedTab.value = tab; +} + +function openMeditor() { + errorDialogOpen.value = false; + meditorOpen.value = true; +} + +const numAssetValidationErrors = computed(() => { + if (currentDandiset.value === null) { + return 0; + } + + return Object.values( + currentDandiset.value.asset_validation_errors, + ).reduce((sum, errs) => sum + errs.length, 0); +}); const publishButtonDisabled = computed(() => !!( currentDandiset.value?.version_validation_errors.length - || currentDandiset.value?.asset_validation_errors.length + || numAssetValidationErrors.value || currentDandiset.value?.dandiset.embargo_status !== 'OPEN' || publishDisabledMessage.value )); From 19ef5945a5405b36c4ab944f59ed9f471783b4e4 Mon Sep 17 00:00:00 2001 From: Jacob Nesbitt Date: Mon, 24 Apr 2023 17:26:38 -0400 Subject: [PATCH 3/6] Only group errors if multiple per asset --- .../components/DLP/ValidationErrorDialog.vue | 82 +++++++++++++------ 1 file changed, 59 insertions(+), 23 deletions(-) diff --git a/web/src/components/DLP/ValidationErrorDialog.vue b/web/src/components/DLP/ValidationErrorDialog.vue index fae705d08..8a58ad859 100644 --- a/web/src/components/DLP/ValidationErrorDialog.vue +++ b/web/src/components/DLP/ValidationErrorDialog.vue @@ -72,32 +72,62 @@ class="overflow-y-auto" > - - - {{ path }} - - - - - - {{ getValidationErrorIcon(error.field) }} - - +