Merge pull request #222 from praekeltfoundation/surface-import-errors

Import error propagation

home/import_content_pages.py

@@ -32,6 +32,17 @@
 PageId = tuple[str, Locale]
 
 
+class ImportException(Exception):
+    """
+    Base exception for all import related issues.
+    """
+
+    def __init__(self, message: str, row_num: int | None = None):
+        self.row_num = row_num
+        self.message = message
+        super().__init__()
+
+
 class ContentImporter:
     def __init__(
         self,
@@ -61,9 +72,11 @@ def locale_from_display_name(self, langname: str) -> Locale:
                 if lang_dn == langname:
                     codes.append(lang_code)
             if not codes:
-                raise ValueError(f"Language not found: {langname}")
+                raise ImportException(f"Language not found: {langname}")
             if len(codes) > 1:
-                raise ValueError(f"Multiple codes for language: {langname} -> {codes}")
+                raise ImportException(
+                    f"Multiple codes for language: {langname} -> {codes}"
+                )
             self.locale_map[langname] = Locale.objects.get(language_code=codes[0])
         return self.locale_map[langname]
 
@@ -84,20 +97,24 @@ def process_rows(self, rows: list["ContentRow"]) -> None:
         # Non-page rows don't have a locale, so we need to remember the last
         # row that does have a locale.
         prev_locale: Locale | None = None
-        for row in rows:
-            if row.is_page_index:
-                if self.locale and row.locale != self.locale.get_display_name():
-                    # This page index isn't for the locale we're importing, so skip it.
-                    continue
-                self.create_content_page_index_from_row(row)
-                prev_locale = self.locale_from_display_name(row.locale)
-            elif row.is_content_page:
-                self.create_shadow_content_page_from_row(row)
-                prev_locale = self.locale_from_display_name(row.locale)
-            elif row.is_variation_message:
-                self.add_variation_to_shadow_content_page_from_row(row, prev_locale)
-            else:
-                self.add_message_to_shadow_content_page_from_row(row, prev_locale)
+        for i, row in enumerate(rows, start=2):
+            try:
+                if row.is_page_index:
+                    if self.locale and row.locale != self.locale.get_display_name():
+                        # This page index isn't for the locale we're importing, so skip it.
+                        continue
+                    self.create_content_page_index_from_row(row)
+                    prev_locale = self.locale_from_display_name(row.locale)
+                elif row.is_content_page:
+                    self.create_shadow_content_page_from_row(row, i)
+                    prev_locale = self.locale_from_display_name(row.locale)
+                elif row.is_variation_message:
+                    self.add_variation_to_shadow_content_page_from_row(row, prev_locale)
+                else:
+                    self.add_message_to_shadow_content_page_from_row(row, prev_locale)
+            except ImportException as e:
+                e.row_num = i
+                raise e
 
     def save_pages(self) -> None:
         for i, page in enumerate(self.shadow_pages.values()):
@@ -106,7 +123,23 @@ def save_pages(self) -> None:
                 continue
             if page.parent:
                 # TODO: We should need to use something unique for `parent`
-                parent = Page.objects.get(title=page.parent, locale=page.locale)
+                try:
+                    parent = Page.objects.get(title=page.parent, locale=page.locale)
+                except Page.DoesNotExist:
+                    raise ImportException(
+                        f"Cannot find parent page with title '{page.parent}' and "
+                        f"locale '{page.locale}'",
+                        page.row_num,
+                    )
+                except Page.MultipleObjectsReturned:
+                    parents = Page.objects.filter(
+                        title=page.parent, locale=page.locale
+                    ).values_list("slug", flat=True)
+                    raise ImportException(
+                        f"Multiple pages with title '{page.parent}' and locale "
+                        f"'{page.locale}' for parent page: {list(parents)}",
+                        page.row_num,
+                    )
             else:
                 parent = self.home_page(page.locale)
             page.save(parent)
@@ -126,7 +159,18 @@ def add_go_to_page_buttons(self) -> None:
             for message_index, buttons in messages.items():
                 for button in buttons:
                     title = button["title"]
-                    related_page = Page.objects.get(slug=button["slug"], locale=locale)
+                    try:
+                        related_page = Page.objects.get(
+                            slug=button["slug"], locale=locale
+                        )
+                    except Page.DoesNotExist:
+                        row = self.shadow_pages[(slug, locale)]
+                        raise ImportException(
+                            f"No pages found with slug '{button['slug']}' and locale "
+                            f"'{locale}' for go_to_page button '{button['title']}' on "
+                            f"page '{slug}'",
+                            row.row_num,
+                        )
                     page.whatsapp_body[message_index].value["buttons"].append(
                         ("go_to_page", {"page": related_page, "title": title})
                     )
@@ -190,9 +234,12 @@ def create_content_page_index_from_row(self, row: "ContentRow") -> None:
 
         index.save_revision().publish()
 
-    def create_shadow_content_page_from_row(self, row: "ContentRow") -> None:
+    def create_shadow_content_page_from_row(
+        self, row: "ContentRow", row_num: int
+    ) -> None:
         locale = self.locale_from_display_name(row.locale)
         page = ShadowContentPage(
+            row_num=row_num,
             slug=row.slug,
             title=row.web_title,
             locale=locale,
@@ -287,6 +334,7 @@ class ShadowContentPage:
     slug: str
     parent: str
     locale: Locale
+    row_num: int
     enable_web: bool = False
     title: str = ""
     subtitle: str = ""
@@ -382,7 +430,16 @@ def link_related_pages(self) -> None:
         page = ContentPage.objects.get(slug=self.slug, locale=self.locale)
         related_pages = []
         for related_page_slug in self.related_pages:
-            related_page = Page.objects.get(slug=related_page_slug, locale=self.locale)
+            try:
+                related_page = Page.objects.get(
+                    slug=related_page_slug, locale=self.locale
+                )
+            except Page.DoesNotExist:
+                raise ImportException(
+                    f"Cannot find related page with slug '{related_page_slug}' and "
+                    f"locale '{self.locale}'",
+                    self.row_num,
+                )
             related_pages.append(("related_page", related_page))
         page.related_pages = related_pages
         page.save_revision().publish()

home/tests/invalid-locale-name.csv

@@ -0,0 +1,2 @@
+structure,message,page_id,slug,parent,web_title,web_subtitle,web_body,whatsapp_title,whatsapp_body,whatsapp_template_name,whatsapp_template_category,example_values,variation_title,variation_body,messenger_title,messenger_body,viber_title,viber_body,translation_tag,tags,quick_replies,triggers,locale,next_prompt,buttons,image_link,doc_link,media_link,related_pages
+Menu 1,0,659,ma_import-export,,MA_import export,,,,,,,,,,,,,,38a22433-e474-4f5a-b06b-7367d1a7f664,,,,NotEnglish,,,,,,

home/tests/missing-gotopage.csv

@@ -0,0 +1,2 @@
+structure,message,page_id,slug,parent,web_title,web_subtitle,web_body,whatsapp_title,whatsapp_body,whatsapp_template_name,whatsapp_template_category,example_values,variation_title,variation_body,messenger_title,messenger_body,viber_title,viber_body,translation_tag,tags,quick_replies,triggers,locale,next_prompt,buttons,image_link,doc_link,media_link,related_pages
+Menu 1,0,659,ma_import-export,,MA_import export,,,Missing,GoToPage,,,,,,,,,,38a22433-e474-4f5a-b06b-7367d1a7f664,,,,English,,"[{""type"":""go_to_page"",""title"":""Missing"",""slug"":""missing""}]",,,,

home/tests/missing-parent.csv

@@ -0,0 +1,2 @@
+structure,message,page_id,slug,parent,web_title,web_subtitle,web_body,whatsapp_title,whatsapp_body,whatsapp_template_name,whatsapp_template_category,example_values,variation_title,variation_body,messenger_title,messenger_body,viber_title,viber_body,translation_tag,tags,quick_replies,triggers,locale,next_prompt,buttons,image_link,doc_link,media_link,related_pages
+Menu 1,0,659,ma_import-export,missing-parent,MA_import export,,,,,,,,,,,,,,38a22433-e474-4f5a-b06b-7367d1a7f664,,,,English,,,,,,

home/tests/missing-related-page.csv

@@ -0,0 +1,2 @@
+structure,message,page_id,slug,parent,web_title,web_subtitle,web_body,whatsapp_title,whatsapp_body,whatsapp_template_name,whatsapp_template_category,example_values,variation_title,variation_body,messenger_title,messenger_body,viber_title,viber_body,translation_tag,tags,quick_replies,triggers,locale,next_prompt,buttons,image_link,doc_link,media_link,related_pages
+Menu 1,0,659,ma_import-export,,MA_import export,,Test body,,,,,,,,,,,,38a22433-e474-4f5a-b06b-7367d1a7f664,,,,English,,,,,,missing related

home/tests/test_content_import_export.py

@@ -19,6 +19,7 @@
 from wagtail.models import Locale, Page  # type: ignore
 
 from home.content_import_export import import_content, old_import_content
+from home.import_content_pages import ImportException
 from home.models import ContentPage, ContentPageIndex, HomePage
 
 from .helpers import set_profile_field_options
@@ -792,6 +793,102 @@ def test_no_translation_key_nondefault(self, newcsv_impexp: ImportExport) -> Non
         with pytest.raises(ValidationError):
             newcsv_impexp.import_file("no-translation-key-cp.csv")
 
+    def test_invalid_locale_name(self, newcsv_impexp: ImportExport) -> None:
+        """
+        Importing pages with invalid locale names should raise an error that results
+        in an error message that gets sent back to the user
+        """
+        with pytest.raises(ImportException) as e:
+            newcsv_impexp.import_file("invalid-locale-name.csv")
+
+        assert e.value.row_num == 2
+        assert e.value.message == "Language not found: NotEnglish"
+
+    def test_multiple_locales_for_name(
+        self, newcsv_impexp: ImportExport, settings: SettingsWrapper
+    ) -> None:
+        """
+        Importing pages with locale names that represent multiple locales should raise
+        an error that results in an error message that gets sent back to the user
+        """
+        settings.WAGTAIL_CONTENT_LANGUAGES = settings.LANGUAGES = [
+            ("en1", "NotEnglish"),
+            ("en2", "NotEnglish"),
+        ]
+        with pytest.raises(ImportException) as e:
+            newcsv_impexp.import_file("invalid-locale-name.csv")
+
+        assert e.value.row_num == 2
+        assert (
+            e.value.message
+            == "Multiple codes for language: NotEnglish -> ['en1', 'en2']"
+        )
+
+    def test_missing_parent(self, newcsv_impexp: ImportExport) -> None:
+        """
+        If the import file specifies a parent title, but there are no pages with that
+        title, then an error message should get sent back to the user.
+        """
+        with pytest.raises(ImportException) as e:
+            newcsv_impexp.import_file("missing-parent.csv")
+
+        assert e.value.row_num == 2
+        assert (
+            e.value.message
+            == "Cannot find parent page with title 'missing-parent' and locale "
+            "'English'"
+        )
+
+    def test_multiple_parents(self, newcsv_impexp: ImportExport) -> None:
+        """
+        Because we use the title to find a parent page, and it's possible to have
+        multiple pages with the same title, it's possible to have the situation where
+        we don't know which parent this import points to. In that case we should show
+        the user an error message, with information that will allow them to fix it.
+        """
+        home_page = HomePage.objects.first()
+        PageBuilder.build_cpi(home_page, "missing-parent1", "missing-parent")
+        PageBuilder.build_cpi(home_page, "missing-parent2", "missing-parent")
+
+        with pytest.raises(ImportException) as e:
+            newcsv_impexp.import_file("missing-parent.csv", purge=False)
+        assert e.value.row_num == 2
+        assert (
+            e.value.message
+            == "Multiple pages with title 'missing-parent' and locale 'English' for "
+            "parent page: ['missing-parent1', 'missing-parent2']"
+        )
+
+    def test_go_to_page_button_missing_page(self, newcsv_impexp: ImportExport) -> None:
+        """
+        Go to page buttons in the import file link to other pages using the slug. But
+        if no page with that slug exists, then we should give the user an error message
+        that tells them where and how to fix it.
+        """
+        with pytest.raises(ImportException) as e:
+            newcsv_impexp.import_file("missing-gotopage.csv")
+        assert e.value.row_num == 2
+        assert (
+            e.value.message
+            == "No pages found with slug 'missing' and locale 'English' for go_to_page "
+            "button 'Missing' on page 'ma_import-export'"
+        )
+
+    def test_missing_related_pages(self, newcsv_impexp: ImportExport) -> None:
+        """
+        Related pages are listed as comma separated slugs in imported files. If there
+        is a slug listed that we cannot find the page for, then we should show the
+        user an error with information about the missing page.
+        """
+        with pytest.raises(ImportException) as e:
+            newcsv_impexp.import_file("missing-related-page.csv")
+        assert e.value.row_num == 2
+        assert (
+            e.value.message
+            == "Cannot find related page with slug 'missing related' and locale "
+            "'English'"
+        )
+
 
 # "old-xlsx" has at least three bugs, so we don't bother testing it.
 @pytest.fixture(params=["old-csv", "new-csv", "new-xlsx"])

home/views.py

@@ -28,6 +28,7 @@
 
 from .content_import_export import import_content, import_ordered_sets
 from .forms import UploadContentFileForm, UploadOrderedContentSetFileForm
+from .import_content_pages import ImportException
 from .mixins import SpreadsheetExportMixin
 from .models import ContentPage, ContentPageRating, OrderedContentSet, PageView
 from .serializers import ContentPageRatingSerializer, PageViewSerializer
@@ -146,6 +147,13 @@ def run(self):
             import_content(
                 self.file, self.file_type, self.progress_queue, self.purge, self.locale
             )
+        except ImportException as e:
+            self.result_queue.put(
+                (
+                    messages.ERROR,
+                    f"Content import failed on row {e.row_num}: {e.message}",
+                )
+            )
         except Exception:
             self.result_queue.put((messages.ERROR, "Content import failed"))
             logger.exception("Content import failed")