diff --git a/CHANGELOG.md b/CHANGELOG.md index cb962f6f..72880303 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Consistent labelling on import forms - Fix class_name warning: RemovedInWagtail60Warning - Helper texts for some ordered content sets +- CMS Forms Flexible imports and Better Error Handling ### Removed - Locale field on exports - Search app diff --git a/home/import_assessments.py b/home/import_assessments.py index 00fedccf..0b23c9b3 100644 --- a/home/import_assessments.py +++ b/home/import_assessments.py @@ -11,10 +11,18 @@ from wagtail.coreutils import get_content_languages # type: ignore from wagtail.models import Locale, Page # type: ignore -from home.import_helpers import ImportException, parse_file, validate_using_form +from home.import_helpers import ( + ImportException, + convert_headers_to_snake_case, + validate_using_form, +) +from home.import_helpers import ( + parse_file as helper_parse_file, +) from home.models import Assessment, ContentPage, HomePage # type: ignore AssessmentId = tuple[str, Locale] +MANDATORY_HEADERS = ["title", "question", "slug", "generic_error", "locale"] class ImportAssessmentException(Exception): @@ -96,9 +104,31 @@ def save_assessments(self) -> None: ) def parse_file(self) -> list["AssessmentRow"]: + """ + Converts the iterator to a list of rows. + If there are rows: + a. Extracts the original headers from the first row. + b. Converts the original headers to snake_case and creates a mapping from original headers to snake_case headers. + c. Validates that the snake_case headers contain all mandatory headers. + d. Transforms each row to use snake_case headers. + """ + row_iterator = helper_parse_file(self.file_content, self.file_type) + rows = [row for _, row in row_iterator] + + original_headers = rows[0].keys() + headers_mapping = convert_headers_to_snake_case( + list(original_headers), row_num=1 + ) + + snake_case_headers = list(headers_mapping.values()) + self.validate_headers(snake_case_headers, row_num=1) + transformed_rows = [ + {headers_mapping[key]: value for key, value in row.items()} for row in rows + ] + return [ - AssessmentRow.from_flat(row, i) - for i, row in parse_file(self.file_content, self.file_type) + AssessmentRow.from_flat(row, i + 2) + for i, row in enumerate(transformed_rows) ] def set_progress(self, message: str, progress: int) -> None: @@ -185,6 +215,16 @@ def create_shadow_assessment_from_row( ) assessment.questions.append(question) + def validate_headers(self, headers: list[str], row_num: int) -> None: + missing_headers = [ + header for header in MANDATORY_HEADERS if header not in headers + ] + if missing_headers: + raise ImportAssessmentException( + message=f"Missing mandatory headers: {', '.join(missing_headers)}", + row_num=row_num, + ) + # Since wagtail page models are so difficult to create and modify programatically, # we instead store all the pages as these shadow objects, which we can then at the end @@ -332,11 +372,13 @@ class AssessmentRow: """ slug: str - title: str = "" + title: str + question: str + generic_error: str + locale: str version: str = "" tags: list[str] = field(default_factory=list) question_type: str = "" - locale: str = "" high_result_page: str = "" high_inflection: str = "" medium_result_page: str = "" @@ -344,8 +386,6 @@ class AssessmentRow: low_result_page: str = "" skip_threshold: str = "" skip_high_result_page: str = "" - generic_error: str = "" - question: str = "" explainer: str = "" error: str = "" min: str = "" @@ -376,7 +416,6 @@ def from_flat(cls, row: dict[str, str], row_num: int) -> "AssessmentRow": key: value for key, value in row.items() if value and key in cls.fields() } - # Create default value with correct length for answer responses if not specified answers = deserialise_list(row.pop("answers", "")) answer_responses = deserialise_list(row.pop("answer_responses", "")) if not answer_responses: @@ -394,8 +433,14 @@ def from_flat(cls, row: dict[str, str], row_num: int) -> "AssessmentRow": **row, ) except TypeError: + missing_fields = [ + field + for field in MANDATORY_HEADERS + if field not in row or row[field] == "" + ] raise ImportAssessmentException( - "The import file is missing some required fields." + f"Row missing values for required fields: {', '.join(missing_fields)}", + row_num, ) diff --git a/home/import_helpers.py b/home/import_helpers.py index 75e00aba..a7203a02 100644 --- a/home/import_helpers.py +++ b/home/import_helpers.py @@ -165,11 +165,42 @@ def extract_errors(data: dict[str | int, Any] | list[str]) -> dict[str, str]: return error_message +def check_empty_rows(rows: list[dict[str, Any]], row_num: int) -> None: + """ + Checks if the list of rows is empty and raises an exception if true. + """ + if not rows: + raise ImportException( + "The import file is empty or contains no valid rows.", row_num=row_num + ) + + +# TODO: +# Move to shared code once we're able to work on the contentpage import. +# Contentsets uses pascal case headers +def convert_headers_to_snake_case(headers: list[str], row_num: int) -> dict[str, str]: + """ + Converts a list of headers to snake_case and returns a mapping. + """ + return {header: to_snake_case(header, row_num) for header in headers} + + +def to_snake_case(s: str, row_num: int) -> str: + """ + Converts a given string to snake_case if it contains spaces or hyphens. + """ + return s.replace(" ", "_").replace("-", "_").strip("_") + + def fix_rows(rows: Iterator[dict[str | Any, Any]]) -> Iterator[dict[str, str | None]]: """ Fix keys for all rows by lowercasing keys and removing whitespace from keys and values """ - first_row = next(rows) + try: + first_row = next(rows) + except StopIteration: + return iter([]) + if len(first_row) != len(fix_row(first_row)): raise ImportException( "Invalid format. Please check that there are no duplicate headers." @@ -206,7 +237,11 @@ def parse_file( file_content: bytes, file_type: str ) -> Iterator[tuple[int, dict[str, Any]]]: read_rows = read_xlsx if file_type == "XLSX" else read_csv - return enumerate(fix_rows(read_rows(file_content)), start=2) + rows = list(fix_rows(read_rows(file_content))) + + check_empty_rows(rows, row_num=1) + + return enumerate(rows, start=2) def read_csv(file_content: bytes) -> Iterator[dict[str, Any]]: diff --git a/home/tests/import-export-data/assessment_missing_related_page.csv b/home/tests/import-export-data/assessment_missing_related_page.csv index 549edc67..8f12ce79 100644 --- a/home/tests/import-export-data/assessment_missing_related_page.csv +++ b/home/tests/import-export-data/assessment_missing_related_page.csv @@ -1,4 +1,4 @@ -title,question_type,tags,slug,locale,high_result_page,high_inflection,medium_result_page,medium_inflection,low_result_page,generic_error,questions,error,min,max,answers,scores,answer_semantic_ids,question_semantic_id -Fake Test,categorical_question,"Random, selection",fake-test,en,fake-page,4,fake-page,3,fake-page,We did not quite get that,Select a number for the first question,Please retry,,,"A,B","4.0,1.0","test-1,test-2",question_semantic_id -Fake Test,categorical_question,"Random, selection",fake-test,en,fake-page,4,fake-page,3,fake-page,We did not quite get that,Select a number for the second question,We did not quite get that,,,"A,B,C","1.0,3.0,4.0","test-1,test-2,test-3",question_semantic_id -Fake Test,categorical_question,"Random, selection",fake-test,en,fake-page,4,fake-page,3,fake-page,We did not quite get that,Select a number for the third question,Sorry we did not quite get that,,,"A,B,C","2.0,1.0,2.0","test-1,test-2,test-3",question_semantic_id +title,question_type,tags,slug,locale,high_result_page,high_inflection,medium_result_page,medium_inflection,low_result_page,skip_threshold,generic_error,question,error,min,max,answers,scores,answer_semantic_ids,question_semantic_id +Fake Test,categorical_question,"Random, selection",fake-test,en,fake-page,4,fake-page,3,fake-page,0,We did not quite get that,Select a number for the first question,Please retry,,,"A,B","4.0,1.0","test-1,test-2",question_semantic_id +Fake Test,categorical_question,"Random, selection",fake-test,en,fake-page,4,fake-page,3,fake-page,0,We did not quite get that,Select a number for the second question,We did not quite get that,,,"A,B,C","1.0,3.0,4.0","test-1,test-2,test-3",question_semantic_id +Fake Test,categorical_question,"Random, selection",fake-test,en,fake-page,4,fake-page,3,fake-page,0,We did not quite get that,Select a number for the third question,Sorry we did not quite get that,,,"A,B,C","2.0,1.0,2.0","test-1,test-2,test-3",question_semantic_id diff --git a/home/tests/import-export-data/assessments_missing_row_values.csv b/home/tests/import-export-data/assessments_missing_row_values.csv new file mode 100644 index 00000000..015afc4a --- /dev/null +++ b/home/tests/import-export-data/assessments_missing_row_values.csv @@ -0,0 +1,6 @@ +title,question_type,tags,slug,version,locale,high_result_page,high_inflection,medium_result_page,medium_inflection,low_result_page,skip_threshold,skip_high_result_page,generic_error,question,explainer,error,min,max,answers,scores,answer_semantic_ids,question_semantic_id,answer_responses +Freetext Question,freetext_question,draft-assessment,Draft-assessment,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, +Random French,freetext_question,,random-french,v1.0,fr,high-inflection,5,,3,,0,,Sorry we didn't quite get that.,What do you not like about France,We need to know this,,,,,,,france-notlike, +,integer_question,test-min-max-range,,v1.0,,,5,,3,,0,,,Lowest temeprature you're experienced,We need to know some things,This is an error message,0,30,,,,lowest-temperature, +Weather Trivia,integer_question,weather-trivia,weather-trivia,v1.0,en,high-inflection,5,medium-score,1,low-score,0,,"Sorry, we didn't quite get that.",What's the coldest weather you're experienced?,We need to know some things,Your reply should be between {min} and {max},50,70,,,,coldest-weather, +Draft Assessment 2,freetext_question,draft-assessment,Draft-assessment-2,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, diff --git a/home/tests/import-export-data/assessments_missing_title.csv b/home/tests/import-export-data/assessments_missing_title.csv new file mode 100644 index 00000000..2005aaea --- /dev/null +++ b/home/tests/import-export-data/assessments_missing_title.csv @@ -0,0 +1,6 @@ +title,question_type,tags,slug,version,locale,high_result_page,high_inflection,medium_result_page,medium_inflection,low_result_page,skip_threshold,skip_high_result_page,generic_error,question,explainer,error,min,max,answers,scores,answer_semantic_ids,question_semantic_id,answer_responses +Freetext Question,freetext_question,draft-assessment,Draft-assessment,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, +Random French,freetext_question,,random-french,v1.0,fr,high-inflection,5,,3,,0,,Sorry we didn't quite get that.,What do you not like about France,We need to know this,,,,,,,france-notlike, +,integer_question,test-min-max-range,test-min-max-range,v1.0,en,,5,,3,,0,,This is a generic error,Lowest temeprature you're experienced,We need to know some things,This is an error message,0,30,,,,lowest-temperature, +Weather Trivia,integer_question,weather-trivia,weather-trivia,v1.0,en,high-inflection,5,medium-score,1,low-score,0,,"Sorry, we didn't quite get that.",What's the coldest weather you're experienced?,We need to know some things,Your reply should be between {min} and {max},50,70,,,,coldest-weather, +Draft Assessment 2,freetext_question,draft-assessment,Draft-assessment-2,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, diff --git a/home/tests/import-export-data/assessments_multiple_missing_headers.csv b/home/tests/import-export-data/assessments_multiple_missing_headers.csv new file mode 100644 index 00000000..fc96ded0 --- /dev/null +++ b/home/tests/import-export-data/assessments_multiple_missing_headers.csv @@ -0,0 +1,6 @@ +,question_type,tags,,version,,high_result_page,high_inflection,medium_result_page,medium_inflection,low_result_page,skip_threshold,skip_high_result_page,,question,explainer,error,min,max,answers,scores,answer_semantic_ids,question_semantic_id,answer_responses +Freetext Question,freetext_question,draft-assessment,Draft-assessment,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, +Random French,freetext_question,,random-french,v1.0,fr,high-inflection,5,,3,,0,,Sorry we didn't quite get that.,What do you not like about France,We need to know this,,,,,,,france-notlike, +Test min max range,integer_question,test-min-max-range,test-min-max-range,v1.0,en,,5,,3,,0,,This is a generic error,Lowest temeprature you're experienced,We need to know some things,This is an error message,0,30,,,,lowest-temperature, +Weather Trivia,integer_question,weather-trivia,weather-trivia,v1.0,en,high-inflection,5,medium-score,1,low-score,0,,"Sorry, we didn't quite get that.",What's the coldest weather you're experienced?,We need to know some things,Your reply should be between {min} and {max},50,70,,,,coldest-weather, +Draft Assessment 2,freetext_question,draft-assessment,Draft-assessment-2,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, diff --git a/home/tests/import-export-data/assessments_single_header_multiple_fields.csv b/home/tests/import-export-data/assessments_single_header_multiple_fields.csv new file mode 100644 index 00000000..9846e2d2 --- /dev/null +++ b/home/tests/import-export-data/assessments_single_header_multiple_fields.csv @@ -0,0 +1,6 @@ +,question_type,tags,slug,version,locale,high_result_page,high_inflection,medium_result_page,medium_inflection,low_result_page,skip_threshold,skip_high_result_page,generic_error,question,explainer,error,min,max,answers,scores,answer_semantic_ids,question_semantic_id,answer_responses +Freetext Question,freetext_question,draft-assessment,Draft-assessment,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, +,freetext_question,,,v1.0,,high-inflection,5,,3,,0,,Sorry we didn't quite get that.,What do you not like about France,We need to know this,,,,,,,france-notlike, +Test min max range,integer_question,test-min-max-range,test-min-max-range,v1.0,en,,5,,3,,0,,This is a generic error,Lowest temeprature you're experienced,We need to know some things,This is an error message,0,30,,,,lowest-temperature, +Weather Trivia,integer_question,weather-trivia,weather-trivia,v1.0,en,high-inflection,5,medium-score,1,low-score,0,,"Sorry, we didn't quite get that.",What's the coldest weather you're experienced?,We need to know some things,Your reply should be between {min} and {max},50,70,,,,coldest-weather, +Draft Assessment 2,freetext_question,draft-assessment,Draft-assessment-2,v1.0,en,,,,,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, diff --git a/home/tests/import-export-data/empty.csv b/home/tests/import-export-data/empty.csv new file mode 100644 index 00000000..8b137891 --- /dev/null +++ b/home/tests/import-export-data/empty.csv @@ -0,0 +1 @@ + diff --git a/home/tests/import-export-data/snake_case_assessments.csv b/home/tests/import-export-data/snake_case_assessments.csv new file mode 100644 index 00000000..26c67a61 --- /dev/null +++ b/home/tests/import-export-data/snake_case_assessments.csv @@ -0,0 +1,5 @@ +title,question type,tags,slug,version,locale,High result page,High inflection,Medium result page,Medium inflection,Low result page,Skip threshold,Skip high result page,Generic error,question,explainer,error,min,max,answers,scores,Answer semantic ids,Question semantic id,Answer responses +Freetext Question,freetext_question,draft-assessment,Draft-assessment,v1.0,en,high-inflection,5,medium-score,3,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, +Test min max range,integer_question,test-min-max-range,test-min-max-range,v1.0,en,high-inflection,5,medium-score,3,,0,,This is a generic error,Lowest temeprature you're experienced,We need to know some things,This is an error message,0,30,,,,lowest-temperature, +Weather Trivia,integer_question,weather-trivia,weather-trivia,v1.0,en,high-inflection,5,medium-score,1,low-score,0,,"Sorry, we didn't quite get that.",What's the coldest weather you're experienced?,We need to know some things,Your reply should be between {min} and {max},50,70,,,,coldest-weather, +Draft Assessment 2,freetext_question,draft-assessment,Draft-assessment-2,v1.0,en,high-inflection,5,medium-score,3,,0,,This is a generic error for draft page,Is this a draft assessment,,,,,,,,draf-assessment, diff --git a/home/tests/test_assessment_import_export.py b/home/tests/test_assessment_import_export.py index 59956512..3f6bb410 100644 --- a/home/tests/test_assessment_import_export.py +++ b/home/tests/test_assessment_import_export.py @@ -425,6 +425,40 @@ def test_single_assessment(self, impexp: ImportExport) -> None: imported = impexp.get_assessment_json() assert imported == orig + def test_snake_case_assessments(self, csv_impexp: ImportExport) -> None: + """ + Importing a csv with spaces in header names and uppercase text should be converted to snake case + and pass, provided that the converted text are valid cms headers. + Here we check that the headers are changed to snake case, the assessment is saved + and finally we check that the saved assessment has the correct headers. + + (This uses snake_case_assessments.csv.) + """ + csv_impexp.import_file("snake_case_assessments.csv") + imported_assessments = Assessment.objects.all() + assert imported_assessments.exists() + assessment_model = Assessment + model_fields = [field.name for field in assessment_model._meta.get_fields()] + + expected_fields = [ + "title", + "slug", + "high_inflection", + "medium_inflection", + "high_result_page", + "medium_result_page", + "low_result_page", + "skip_high_result_page", + "skip_threshold", + "generic_error", + "questions", + ] + + for field_name in expected_fields: + assert ( + field_name in model_fields + ), f"Field '{field_name}' not found in Assessment model." + @pytest.mark.usefixtures("result_content_pages") @pytest.mark.django_db() @@ -462,7 +496,10 @@ def test_import_error(self, csv_impexp: ImportExport) -> None: csv_bytes = csv_impexp.import_file("assessment_simple.csv") with pytest.raises(ImportAssessmentException) as e: csv_impexp.import_file("broken_assessment.csv") - assert e.value.message == "The import file is missing some required fields." + assert ( + e.value.message + == "Missing mandatory headers: title, question, slug, generic_error, locale" + ) content = csv_impexp.export_assessment() src, dst = csv_impexp.csvs2dicts(csv_bytes, content) assert dst == src @@ -618,6 +655,66 @@ def test_invalid_medium_score(self, csv_impexp: ImportExport) -> None: ) assert e.value.row_num == 2 + def test_multiple_missing_headers(self, csv_impexp: ImportExport) -> None: + """ + Importing a CSV with multiple missing headers should return an error + stating all the mandatory headers that are missing + """ + with pytest.raises(ImportAssessmentException) as e: + csv_impexp.import_file("assessments_multiple_missing_headers.csv") + assert ( + e.value.message == "Missing mandatory headers: title, slug, " + "generic_error, locale" + ) + assert e.value.row_num == 1 + + def test_missing_title(self, csv_impexp: ImportExport) -> None: + """ + Importing a CSV with a missing title field should return an error + that a title is missing + """ + with pytest.raises(ImportAssessmentException) as e: + csv_impexp.import_file("assessments_missing_title.csv") + assert e.value.message == "Row missing values for required fields: title" + assert e.value.row_num == 4 + + def test_empty_rows(self, csv_impexp: ImportExport) -> None: + """ + Importing an empty CSV should return an error that the + import file has no rows. + """ + with pytest.raises(ImportException) as e: + csv_impexp.import_file("empty.csv") + assert e.value.message == [ + "The import file is empty or contains no valid rows." + ] + assert e.value.row_num == 1 + + def test_multiple_missing_values(self, csv_impexp: ImportExport) -> None: + """ + Importing an empty CSV should return an error with all the missing + values in a row + """ + with pytest.raises(ImportAssessmentException) as e: + csv_impexp.import_file("assessments_missing_row_values.csv") + assert ( + e.value.message + == "Row missing values for required fields: title, slug, generic_error, locale" + ) + assert e.value.row_num == 4 + + def test_single_missing_header_multiple_missing_rows( + self, csv_impexp: ImportExport + ) -> None: + """ + Importing a CSV with a single missing header and multiple missing rows + should return an error that a header is missing + """ + with pytest.raises(ImportAssessmentException) as e: + csv_impexp.import_file("assessments_single_header_multiple_fields.csv") + assert e.value.message == "Missing mandatory headers: title" + assert e.value.row_num == 1 + @pytest.mark.usefixtures("result_content_pages") @pytest.mark.django_db()