refactor: localmenus (#5564)

add function to get localised files
add tests for localised file func

Code is quite a lot cleaner, and easier to understand, should be less errors in future.

Fixes localization of links in menus, which is apparently broken atm

Co-authored-by: Darragh Elliott <me@delliott.net>
Reviewed-on: #5564
Co-authored-by: delliott <delliott@fsfe.org>
Co-committed-by: delliott <delliott@fsfe.org>

build/fsfe_website_build/lib/misc.py

@@ -123,3 +123,24 @@ def get_basepath(file: Path) -> Path:
 def get_basename(file: Path) -> str:
     """Return the name of the file with the last two suffixes removed."""
     return file.with_suffix("").with_suffix("").name
+
+
+def get_localised_file(base_file: Path, lang: str, suffix: str) -> Path | None:
+    """Return basefile localised if exists, else fallback if exists, else none."""
+    # ensure the suffix has a leading .
+    normalised_suffix = "." + suffix.removeprefix(".")
+    return (
+        localised
+        if (
+            localised := base_file.with_suffix(
+                base_file.suffix + f".{lang}" + normalised_suffix
+            )
+        ).exists()
+        else fallback
+        if (
+            fallback := base_file.with_suffix(
+                base_file.suffix + ".en" + normalised_suffix
+            )
+        ).exists()
+        else None
+    )

build/fsfe_website_build/phase1/update_localmenus.py

@@ -14,7 +14,11 @@ from typing import TYPE_CHECKING
 
 from lxml import etree
 
-from fsfe_website_build.lib.misc import get_basepath, sort_dict, update_if_changed
+from fsfe_website_build.lib.misc import (
+    get_basepath,
+    get_localised_file,
+    update_if_changed,
+)
 
 if TYPE_CHECKING:
     import multiprocessing.pool
@@ -24,62 +28,45 @@ logger = logging.getLogger(__name__)
 
 def _write_localmenus(
     source_dir: Path,
-    directory: str,
-    files_by_dir: dict[str, list[Path]],
+    directory: Path,
+    files: list[Path],
     languages: list[str],
 ) -> None:
     """Write localmenus for a given directory."""
     # Set of files with no langcode or xhtml extension
-    base_files = {get_basepath(filter_file) for filter_file in files_by_dir[directory]}
+    base_files = {get_basepath(file) for file in files}
     for lang in languages:
-        file = Path(directory).joinpath(f".localmenu.{lang}.xml")
-        logger.debug("Creating %s", file)
+        localmenu_file = directory.joinpath(f".localmenu.{lang}.xml")
+        logger.debug("Creating %s", localmenu_file)
         page = etree.Element("feed")
 
         # Add the subelements
-        version = etree.SubElement(page, "version")
-        version.text = "1"
+        etree.SubElement(page, "version").text = "1"
 
-        for source_file in [
-            path
-            for path in (
-                base_file.with_suffix(f".{lang}.xhtml")
-                if base_file.with_suffix(f".{lang}.xhtml").exists()
-                else (
-                    base_file.with_suffix(".en.xhtml")
-                    if base_file.with_suffix(".en.xhtml").exists()
-                    else None
-                )
-                for base_file in base_files
-            )
-            if path is not None
-        ]:
+        for base_file in base_files:
+            # source file to get localmenu data from
+            source_file = get_localised_file(base_file, lang, ".xhtml")
+            if source_file is None:
+                logger.debug("No source for basefile %s", base_file)
+                continue
+            # the file we are linking to in the localmenu
+            link_file = base_file.with_suffix(base_file.suffix + ".html")
+            # now generate a localmenu entry for each localmenu entry in the source file
             for localmenu in etree.parse(source_file).xpath("//localmenu"):
                 etree.SubElement(
                     page,
                     "localmenuitem",
-                    set=(
-                        str(localmenu.xpath("./@set")[0])
-                        if localmenu.xpath("./@set") != []
-                        else "default"
-                    ),
-                    id=(
-                        str(localmenu.xpath("./@id")[0])
-                        if localmenu.xpath("./@id") != []
-                        else "default"
-                    ),
+                    set=localmenu.get("set", "default"),
+                    id=localmenu.get("id", "default"),
                     link=(
                         "/"
                         + str(
-                            source_file.with_suffix(".html").relative_to(source_dir),
+                            link_file.relative_to(source_dir),
                         )
                     ),
                 ).text = localmenu.text
 
-        update_if_changed(
-            file,
-            etree.tostring(page, encoding="utf-8").decode("utf-8"),
-        )
+        update_if_changed(localmenu_file, etree.tostring(page, encoding="unicode"))
 
 
 def update_localmenus(
@@ -91,44 +78,38 @@ def update_localmenus(
     """Update all the .localmenu.*.xml files containing the local menus."""
     logger.info("Updating local menus")
     # Get a dict of all source files containing local menus
-    files_by_dir: dict[str, set[Path]] = defaultdict(set)
-    for file in filter(
-        lambda path: "-template" not in path.name,
-        source_dir.glob("**/*.??.xhtml"),
+    files_by_dir: dict[Path, list[Path]] = defaultdict(list)
+    for file in (
+        file
+        for file in source_dir.glob("**/*.??.xhtml")
+        if "-template" not in file.name
     ):
         xslt_root = etree.parse(file)
-        if xslt_root.xpath("//localmenu"):
-            directory_xpath = xslt_root.xpath("//localmenu/@dir")
-            directory = str(
-                source.joinpath(directory_xpath[0])
-                if directory_xpath
-                else file.parent.resolve().relative_to(source.resolve())
+        for localmenu_elem in xslt_root.xpath("//localmenu"):
+            directory = Path(
+                localmenu_elem.get(
+                    "dir", str(file.parent.resolve().relative_to(source.resolve()))
+                )
             )
-            files_by_dir[directory].add(file)
-    files_by_dir = sort_dict(files_by_dir)
+            files_by_dir[directory].append(file)
 
     # If any of the source files has been updated, rebuild all .localmenu.*.xml
-    dirs = filter(
-        lambda directory: (
-            any(
+    dirs = [
+        (directory, files)
+        for directory, files in files_by_dir.items()
+        if any(
+            [
                 (
-                    (
-                        (not Path(directory).joinpath(".localmenu.en.xml").exists())
-                        or (
-                            file.stat().st_mtime
-                            > Path(directory)
-                            .joinpath(".localmenu.en.xml")
-                            .stat()
-                            .st_mtime
-                        )
-                    )
-                    for file in files_by_dir[directory]
-                ),
-            )
-        ),
-        files_by_dir,
-    )
+                    not (
+                        localmenu_path := directory.joinpath(".localmenu.en.xml")
+                    ).exists()
+                )
+                or (file.stat().st_mtime > localmenu_path.stat().st_mtime)
+            ]
+            for file in files
+        )
+    ]
     pool.starmap(
         _write_localmenus,
-        ((source_dir, directory, files_by_dir, languages) for directory in dirs),
+        ((source_dir, directory, files, languages) for directory, files in dirs),
     )

build/fsfe_website_build_tests_pre_commit/lib/misc_test.py

@@ -10,6 +10,7 @@ from fsfe_website_build.lib.misc import (
     delete_file,
     get_basename,
     get_basepath,
+    get_localised_file,
     get_version,
     keys_exists,
     lang_from_filename,
@@ -111,3 +112,68 @@ def get_basepath_test() -> None:
 def get_basename_test() -> None:
     assert get_basename(Path("a.b.c")) == "a"
     assert get_basename(Path("a/b.c.d")) == "b"
+
+
+def get_localised_file_localized__test(tmp_path: Path) -> None:
+    base_file = tmp_path / "test"
+    base_file.write_text("content")
+
+    localized_file = tmp_path / "test.fr.xhtml"
+    localized_file.write_text("french content")
+
+    result = get_localised_file(base_file, "fr", "xhtml")
+    assert result == localized_file
+    assert result is not None
+    assert result.exists()
+
+
+def get_localised_file_localized_missing_test(
+    tmp_path: Path,
+) -> None:
+    base_file = tmp_path / "test"
+    base_file.write_text("content")
+
+    fallback_file = tmp_path / "test.en.xhtml"
+    fallback_file.write_text("english content")
+
+    result = get_localised_file(base_file, "de", "xhtml")
+    assert result == fallback_file
+    assert result is not None
+    assert result.exists()
+
+
+def get_localised_file_neither_exists_test(tmp_path: Path) -> None:
+    base_file = tmp_path / "test"
+    base_file.write_text("content")
+
+    result = get_localised_file(base_file, "fr", "xhtml")
+    assert result is None
+
+
+def get_localised_file_existing_suffix_test(tmp_path: Path) -> None:
+    base_file = tmp_path / "test.suffix.test"
+    base_file.write_text("content")
+
+    localized_file = tmp_path / "test.suffix.test.fr.xml"
+    localized_file.write_text("xml content")
+
+    result = get_localised_file(base_file, "fr", "xml")
+    assert result == localized_file
+    assert result is not None
+    assert result.exists()
+
+
+def get_localised_file_suffix_normalization_test(tmp_path: Path) -> None:
+    base_file = tmp_path / "test"
+    base_file.write_text("content")
+
+    localized_file = tmp_path / "test.fr.xml"
+    localized_file.write_text("xml content")
+
+    result = get_localised_file(base_file, "fr", "xml")
+    assert result == localized_file
+    assert result is not None
+    assert result.exists()
+
+    result2 = get_localised_file(base_file, "fr", ".xml")
+    assert result2 == localized_file