From 9c2bb856b55c16a75022efe2675854fe7982c2bb Mon Sep 17 00:00:00 2001 From: Miles Mason Winther <42948872+mmwinther@users.noreply.github.com> Date: Wed, 17 Apr 2024 12:39:24 +0200 Subject: [PATCH] Fixes, refactoring, clean up dead code (#297) * Remove unnecessary None check * Collapse two union types to one * Don't create language objects for empty strings * Strip whitespace from user input file path * Add test for empty value language string --- src/datadoc/app.py | 4 --- src/datadoc/frontend/callbacks/dataset.py | 26 +++++++++---------- src/datadoc/frontend/callbacks/utils.py | 13 ++++++---- src/datadoc/frontend/components/alerts.py | 14 ---------- src/datadoc/frontend/components/builders.py | 14 +++------- src/datadoc/frontend/fields/display_base.py | 10 +------ .../frontend/fields/display_dataset.py | 13 ++++++---- .../frontend/fields/display_variables.py | 4 +-- .../callbacks/test_callbacks_utils.py | 18 ++++++++++--- .../callbacks/test_dataset_callbacks.py | 17 +++++++++--- .../test_build_dataset_edit_section.py | 10 +++---- 11 files changed, 67 insertions(+), 76 deletions(-) diff --git a/src/datadoc/app.py b/src/datadoc/app.py index 80cc259f..58321143 100644 --- a/src/datadoc/app.py +++ b/src/datadoc/app.py @@ -21,12 +21,10 @@ from datadoc.backend.statistic_subject_mapping import StatisticSubjectMapping from datadoc.enums import SupportedLanguages from datadoc.frontend.callbacks.register_callbacks import register_callbacks -from datadoc.frontend.components.alerts import dataset_validation_error from datadoc.frontend.components.alerts import naming_convention_warning from datadoc.frontend.components.alerts import opened_dataset_error from datadoc.frontend.components.alerts import opened_dataset_success from datadoc.frontend.components.alerts import saved_metadata_success -from datadoc.frontend.components.alerts import variables_validation_error from datadoc.frontend.components.control_bars import build_controls_bar from datadoc.frontend.components.control_bars import build_language_dropdown from datadoc.frontend.components.control_bars import header @@ -61,8 +59,6 @@ def build_app(app: type[Dash]) -> Dash: [ progress_bar, build_controls_bar(), - variables_validation_error, - dataset_validation_error, opened_dataset_error, saved_metadata_success, opened_dataset_success, diff --git a/src/datadoc/frontend/callbacks/dataset.py b/src/datadoc/frontend/callbacks/dataset.py index 769c87fa..08c9d868 100644 --- a/src/datadoc/frontend/callbacks/dataset.py +++ b/src/datadoc/frontend/callbacks/dataset.py @@ -20,8 +20,13 @@ from datadoc.frontend.callbacks.utils import parse_and_validate_dates from datadoc.frontend.callbacks.utils import update_global_language_state from datadoc.frontend.fields.display_dataset import DISPLAYED_DATASET_METADATA -from datadoc.frontend.fields.display_dataset import DISPLAYED_DROPDOWN_DATASET_METADATA -from datadoc.frontend.fields.display_dataset import MULTIPLE_LANGUAGE_DATASET_METADATA +from datadoc.frontend.fields.display_dataset import DROPDOWN_DATASET_METADATA +from datadoc.frontend.fields.display_dataset import ( + DROPDOWN_DATASET_METADATA_IDENTIFIERS, +) +from datadoc.frontend.fields.display_dataset import ( + MULTIPLE_LANGUAGE_DATASET_IDENTIFIERS, +) from datadoc.frontend.fields.display_dataset import DatasetIdentifiers from datadoc.utils import METADATA_DOCUMENT_FILE_SUFFIX @@ -40,14 +45,14 @@ def open_file(file_path: str | None = None) -> DataDocMetadata: logger.info("Opening existing metadata document %s", file_path) return DataDocMetadata( state.statistic_subject_mapping, - metadata_document_path=file_path, + metadata_document_path=file_path.strip(), ) dataset = file_path or get_dataset_path() logger.info("Opening dataset %s", dataset) return DataDocMetadata( state.statistic_subject_mapping, - dataset_path=str(dataset) if dataset else None, + dataset_path=str(dataset).strip() if dataset else None, ) @@ -113,7 +118,7 @@ def process_special_cases( updated_value = process_keyword(value) elif metadata_identifier == DatasetIdentifiers.VERSION.value: updated_value = str(value) - elif metadata_identifier in MULTIPLE_LANGUAGE_DATASET_METADATA and isinstance( + elif metadata_identifier in MULTIPLE_LANGUAGE_DATASET_IDENTIFIERS and isinstance( value, str, ): @@ -124,12 +129,7 @@ def process_special_cases( metadata_identifier, language, ) - elif ( - metadata_identifier == DatasetIdentifiers.DATASET_STATUS.value - and value == "" - or metadata_identifier == DatasetIdentifiers.DATASET_STATE.value - and value == "" - ): + elif metadata_identifier in DROPDOWN_DATASET_METADATA_IDENTIFIERS and value == "": updated_value = None else: updated_value = value @@ -150,8 +150,6 @@ def accept_dataset_metadata_input( metadata_identifier, ) try: - if language is None: - value = process_special_cases(value, metadata_identifier) value = process_special_cases(value, metadata_identifier, language) # Update the value in the model setattr( @@ -198,7 +196,7 @@ def change_language_dataset_metadata( """ update_global_language_state(language) return ( - *(e.options_getter(language) for e in DISPLAYED_DROPDOWN_DATASET_METADATA), + *(e.options_getter(language) for e in DROPDOWN_DATASET_METADATA), update_dataset_metadata_language(), ) diff --git a/src/datadoc/frontend/callbacks/utils.py b/src/datadoc/frontend/callbacks/utils.py index f49fb650..aad9f299 100644 --- a/src/datadoc/frontend/callbacks/utils.py +++ b/src/datadoc/frontend/callbacks/utils.py @@ -106,10 +106,8 @@ def find_existing_language_string( value: str, metadata_identifier: str, language: str, -) -> model.LanguageStringType: +) -> model.LanguageStringType | None: """Get or create a LanguageStrings object and return it.""" - # In this case we need to set the string to the correct language code - language_strings = getattr(metadata_model_object, metadata_identifier) if language_strings is not None: @@ -122,13 +120,15 @@ def find_existing_language_string( language, value, ) - else: + elif value != "": _add_language_string_item( language_strings, language, value, ) - else: + else: + return None + elif value != "": language_strings = model.LanguageStringType( root=[ model.LanguageStringTypeItem( @@ -137,6 +137,9 @@ def find_existing_language_string( ), ], ) + else: + # Don't create an item if the value is empty + return None return language_strings diff --git a/src/datadoc/frontend/components/alerts.py b/src/datadoc/frontend/components/alerts.py index ae8b4a4a..bf3fdb51 100644 --- a/src/datadoc/frontend/components/alerts.py +++ b/src/datadoc/frontend/components/alerts.py @@ -6,20 +6,6 @@ from datadoc.frontend.components.builders import AlertTypes from datadoc.frontend.components.builders import build_ssb_alert -dataset_validation_error = build_ssb_alert( - AlertTypes.ERROR, - "dataset-validation-error", - "Validering feilet", - "dataset-validation-explanation", -) - -variables_validation_error = build_ssb_alert( - AlertTypes.ERROR, - "variables-validation-error", - "Validering feilet", - "variables-validation-explanation", -) - opened_dataset_error = build_ssb_alert( AlertTypes.ERROR, "opened-dataset-error", diff --git a/src/datadoc/frontend/components/builders.py b/src/datadoc/frontend/components/builders.py index 70731148..34f6d954 100644 --- a/src/datadoc/frontend/components/builders.py +++ b/src/datadoc/frontend/components/builders.py @@ -14,8 +14,7 @@ from datadoc.frontend.fields.display_base import DATASET_METADATA_INPUT from datadoc.frontend.fields.display_base import VARIABLES_METADATA_INPUT -from datadoc.frontend.fields.display_base import DatasetFieldTypes -from datadoc.frontend.fields.display_base import VariablesFieldTypes +from datadoc.frontend.fields.display_base import FieldTypes if TYPE_CHECKING: from datadoc_model import model @@ -104,7 +103,7 @@ def build_ssb_alert( # noqa: PLR0913 not immediately obvious how to improve thi def build_input_field_section( - metadata_fields: list[VariablesFieldTypes], + metadata_fields: list[FieldTypes], variable: model.Variable, language: str, ) -> dbc.Form: @@ -155,13 +154,6 @@ def build_ssb_accordion( header=header, id=key, children=[ - html.Section( - id={ - "type": "variable-input-alerts", - "variable_short_name": variable_short_name, - }, - className="alert-section", - ), html.Section( id={ "type": "variable-inputs", @@ -176,7 +168,7 @@ def build_ssb_accordion( def build_dataset_edit_section( title: str, - metadata_inputs: list[DatasetFieldTypes], + metadata_inputs: list[FieldTypes], language: str, dataset: model.Dataset, key: dict, diff --git a/src/datadoc/frontend/fields/display_base.py b/src/datadoc/frontend/fields/display_base.py index cbb91bc5..53ca9490 100644 --- a/src/datadoc/frontend/fields/display_base.py +++ b/src/datadoc/frontend/fields/display_base.py @@ -378,18 +378,10 @@ def render( ) -VariablesFieldTypes = ( +FieldTypes = ( MetadataInputField | MetadataDropdownField | MetadataCheckboxField | MetadataPeriodField | MetadataMultiLanguageField ) - -DatasetFieldTypes = ( - MetadataInputField - | MetadataDropdownField - | MetadataPeriodField - | MetadataCheckboxField - | MetadataMultiLanguageField -) diff --git a/src/datadoc/frontend/fields/display_dataset.py b/src/datadoc/frontend/fields/display_dataset.py index 2039f2b4..5333befa 100644 --- a/src/datadoc/frontend/fields/display_dataset.py +++ b/src/datadoc/frontend/fields/display_dataset.py @@ -11,7 +11,7 @@ from datadoc import state from datadoc.frontend.fields.display_base import DATASET_METADATA_DATE_INPUT from datadoc.frontend.fields.display_base import DATASET_METADATA_MULTILANGUAGE_INPUT -from datadoc.frontend.fields.display_base import DatasetFieldTypes +from datadoc.frontend.fields.display_base import FieldTypes from datadoc.frontend.fields.display_base import MetadataCheckboxField from datadoc.frontend.fields.display_base import MetadataDropdownField from datadoc.frontend.fields.display_base import MetadataInputField @@ -108,7 +108,7 @@ class DatasetIdentifiers(str, Enum): DISPLAY_DATASET: dict[ DatasetIdentifiers, - DatasetFieldTypes, + FieldTypes, ] = { DatasetIdentifiers.SHORT_NAME: MetadataInputField( identifier=DatasetIdentifiers.SHORT_NAME.value, @@ -326,7 +326,7 @@ class DatasetIdentifiers(str, Enum): if v.multiple_language_support: v.value_getter = get_multi_language_metadata -MULTIPLE_LANGUAGE_DATASET_METADATA = [ +MULTIPLE_LANGUAGE_DATASET_IDENTIFIERS = [ m.identifier for m in DISPLAY_DATASET.values() if m.multiple_language_support ] @@ -342,15 +342,18 @@ class DatasetIdentifiers(str, Enum): # The order of this list MUST match the order of display components, as defined in DatasetTab.py -DISPLAYED_DATASET_METADATA: list[DatasetFieldTypes] = ( +DISPLAYED_DATASET_METADATA: list[FieldTypes] = ( OBLIGATORY_EDITABLE_DATASET_METADATA + OPTIONAL_DATASET_METADATA + NON_EDITABLE_DATASET_METADATA ) -DISPLAYED_DROPDOWN_DATASET_METADATA: list[MetadataDropdownField] = [ +DROPDOWN_DATASET_METADATA: list[MetadataDropdownField] = [ m for m in DISPLAYED_DATASET_METADATA if isinstance(m, MetadataDropdownField) ] +DROPDOWN_DATASET_METADATA_IDENTIFIERS: list[str] = [ + m.identifier for m in DROPDOWN_DATASET_METADATA +] OBLIGATORY_DATASET_METADATA_IDENTIFIERS: list[str] = [ m.identifier for m in DISPLAY_DATASET.values() if m.obligatory and m.editable diff --git a/src/datadoc/frontend/fields/display_variables.py b/src/datadoc/frontend/fields/display_variables.py index e95a0780..44e3e400 100644 --- a/src/datadoc/frontend/fields/display_variables.py +++ b/src/datadoc/frontend/fields/display_variables.py @@ -8,12 +8,12 @@ from datadoc import enums from datadoc.frontend.fields.display_base import VARIABLES_METADATA_DATE_INPUT from datadoc.frontend.fields.display_base import VARIABLES_METADATA_MULTILANGUAGE_INPUT +from datadoc.frontend.fields.display_base import FieldTypes from datadoc.frontend.fields.display_base import MetadataCheckboxField from datadoc.frontend.fields.display_base import MetadataDropdownField from datadoc.frontend.fields.display_base import MetadataInputField from datadoc.frontend.fields.display_base import MetadataMultiLanguageField from datadoc.frontend.fields.display_base import MetadataPeriodField -from datadoc.frontend.fields.display_base import VariablesFieldTypes from datadoc.frontend.fields.display_base import get_enum_options_for_language from datadoc.frontend.fields.display_base import get_multi_language_metadata @@ -44,7 +44,7 @@ class VariableIdentifiers(str, Enum): DISPLAY_VARIABLES: dict[ VariableIdentifiers, - VariablesFieldTypes, + FieldTypes, ] = { VariableIdentifiers.SHORT_NAME: MetadataInputField( identifier=VariableIdentifiers.SHORT_NAME.value, diff --git a/tests/frontend/callbacks/test_callbacks_utils.py b/tests/frontend/callbacks/test_callbacks_utils.py index 00eae9c1..2468b43d 100644 --- a/tests/frontend/callbacks/test_callbacks_utils.py +++ b/tests/frontend/callbacks/test_callbacks_utils.py @@ -16,17 +16,29 @@ def test_find_existing_language_string_no_existing_strings(bokmål_name: str): dataset_metadata = model.Dataset() - language_strings = find_existing_language_string( + assert find_existing_language_string( dataset_metadata, bokmål_name, "name", "nb", - ) - assert language_strings == model.LanguageStringType( + ) == model.LanguageStringType( [model.LanguageStringTypeItem(languageCode="nb", languageText=bokmål_name)], ) +def test_find_existing_language_string_no_existing_strings_empty_value(): + dataset_metadata = model.Dataset() + assert ( + find_existing_language_string( + dataset_metadata, + "", + "name", + "nb", + ) + is None + ) + + def test_find_existing_language_string_pre_existing_strings( english_name: str, bokmål_name: str, diff --git a/tests/frontend/callbacks/test_dataset_callbacks.py b/tests/frontend/callbacks/test_dataset_callbacks.py index 1d05415b..cea4bb37 100644 --- a/tests/frontend/callbacks/test_dataset_callbacks.py +++ b/tests/frontend/callbacks/test_dataset_callbacks.py @@ -23,7 +23,9 @@ from datadoc.frontend.callbacks.dataset import open_dataset_handling from datadoc.frontend.callbacks.dataset import process_special_cases from datadoc.frontend.callbacks.dataset import update_dataset_metadata_language -from datadoc.frontend.fields.display_dataset import MULTIPLE_LANGUAGE_DATASET_METADATA +from datadoc.frontend.fields.display_dataset import ( + MULTIPLE_LANGUAGE_DATASET_IDENTIFIERS, +) from datadoc.frontend.fields.display_dataset import DatasetIdentifiers from tests.utils import TEST_PARQUET_FILEPATH @@ -312,17 +314,24 @@ def test_change_language_dataset_metadata_options_enums( assert {d["id"] for d in test_without_empty_option} == {e.name for e in enum_for_options} # type: ignore [attr-defined] +@pytest.mark.parametrize( + "path", + [ + "valid/path/to/person_data_v1.parquet", + " path/with/extra/whitespace/person_data_v1.parquet ", + ], +) @patch(f"{DATASET_CALLBACKS_MODULE}.open_file") def test_open_dataset_handling_normal( open_file_mock: Mock, # noqa: ARG001 n_clicks_1: int, - file_path: str, + path: str, ): state.current_metadata_language = SupportedLanguages.ENGLISH opened, show_error, naming_standard, error_msg, language = open_dataset_handling( n_clicks_1, - file_path, + path, ) assert opened assert not show_error @@ -422,7 +431,7 @@ def test_process_special_cases_language_string( language = "en" value = "Test language string" identifier = random.choice( # noqa: S311 - MULTIPLE_LANGUAGE_DATASET_METADATA, + MULTIPLE_LANGUAGE_DATASET_IDENTIFIERS, ) expected = model.LanguageStringType( [ diff --git a/tests/frontend/components/test_build_dataset_edit_section.py b/tests/frontend/components/test_build_dataset_edit_section.py index 8efab853..04c58662 100644 --- a/tests/frontend/components/test_build_dataset_edit_section.py +++ b/tests/frontend/components/test_build_dataset_edit_section.py @@ -8,7 +8,7 @@ from datadoc.enums import SupportedLanguages from datadoc.frontend.components.builders import build_dataset_edit_section -from datadoc.frontend.fields.display_base import DatasetFieldTypes +from datadoc.frontend.fields.display_base import FieldTypes from datadoc.frontend.fields.display_base import MetadataDropdownField from datadoc.frontend.fields.display_base import MetadataInputField from datadoc.frontend.fields.display_base import MetadataPeriodField @@ -260,21 +260,21 @@ def test_build_dataset_edit_section_dropdown_component_props( # Test data sorted by DatasetFieldTypes -DATASET_INPUT_FIELD_LIST: list[DatasetFieldTypes] = [ +DATASET_INPUT_FIELD_LIST: list[FieldTypes] = [ m for m in DISPLAY_DATASET.values() if isinstance(m, MetadataInputField) ] -DATASET_INPUT_URL_FIELD_LIST: list[DatasetFieldTypes] = [ +DATASET_INPUT_URL_FIELD_LIST: list[FieldTypes] = [ m for m in DISPLAY_DATASET.values() if isinstance(m, MetadataInputField) and m.type == "url" ] -DATASET_DATE_FIELD_LIST: list[DatasetFieldTypes] = [ +DATASET_DATE_FIELD_LIST: list[FieldTypes] = [ m for m in DISPLAY_DATASET.values() if isinstance(m, MetadataPeriodField) ] -DATASET_DROPDOWN_FIELD_LIST_MINUS_ATYPICAL: list[DatasetFieldTypes] = [ +DATASET_DROPDOWN_FIELD_LIST_MINUS_ATYPICAL: list[FieldTypes] = [ m for m in DISPLAY_DATASET.values() if isinstance(m, MetadataDropdownField)