Merge pull request #419 from praekeltfoundation/assessment-flexible-i…

…mport-misisng-error-fields

CMS Forms Flexible Imports

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

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,20 +372,20 @@ 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 = ""
     medium_inflection: str = ""
     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,
             )
 
 

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]]:

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

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,

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,

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,

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,

home/tests/import-export-data/empty.csv

@@ -0,0 +1 @@
+

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,

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()