From 6b970fafbbf810fce67ac4edc2bf206729bacbc6 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 1 Jul 2026 17:44:01 -0600 Subject: [PATCH 01/17] Import .bloomSource files into a collection, with duplicate-by-id handling (BL-16502) Adds a collection-screen command to import one or more .bloomSource (and legacy .bloom) files into the current editable collection, either as editable books or as new derivatives. When an imported book's bookInstanceId already exists in the collection, the user is prompted to replace the existing book or add an independent numbered copy (new id). - CollectionModel: import/extract/validate/duplicate-resolution pipeline; a pre-flight AnyBloomSourceIsAlreadyInCollection check drives the dialog. - CollectionApi: chooseBloomSourceFilesToImport / anyChosenBloomSourceAlreadyInCollection / importBloomSource endpoints. - CollectionsTabPane + RadioChoiceDialog: edit-vs-derivative and replace-vs-addCopy dialogs. - BloomOpenFileDialog: multi-select support for the import picker. Tests (CollectionModelTests) cover valid import, duplicate detection keyed on bookInstanceId (proving same-title/different-id is NOT a duplicate and same-id/different-title IS), replace/add-copy/cancel resolution, and the malformed-file guards. Fixed FakeCollectionModel so GetBookInfos yields real BookInfos (a null BookSelection was turning every book into an ErrorBookInfo, which silently defeated id-based duplicate detection in tests). Preflight review fixes: - Stage extraction on the collection's volume so the move into the collection can't fail cross-volume (system temp is often on a different drive). - In the Replace case, recycle the existing book only after the imported book is safely in place, so a mid-import failure can't lose the original. Co-Authored-By: Claude Opus 4.8 (1M context) --- DistFiles/localization/en/Bloom.xlf | 30 + .../collectionsTab/CollectionsTabPane.tsx | 99 ++++ .../collectionsTab/RadioChoiceDialog.tsx | 87 +++ src/BloomExe/CollectionTab/CollectionModel.cs | 520 ++++++++++++++++++ src/BloomExe/MiscUI/BloomOpenFileDialog.cs | 20 +- src/BloomExe/web/controllers/CollectionApi.cs | 47 ++ .../CollectionTab/CollectionModelTests.cs | 415 ++++++++++++++ .../CollectionTab/FakeCollectionModel.cs | 15 +- 8 files changed, 1229 insertions(+), 4 deletions(-) create mode 100644 src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index d656c02980ad..c518878cb0c5 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -792,6 +792,36 @@ Shell Books are books that are meant to be translated. ID: CollectionTab.MakeBloomPackOfShellBooks + + Import .bloomSource File(s) + Menu command (and dialog title) that imports one or more books from .bloomSource files into the current collection. Keep ".bloomSource" unchanged; it is a file extension. + ID: CollectionTab.ImportBloomSource + + + I want to edit the book(s) I am importing. + Radio-button choice in the import dialog: import the book(s) to edit them directly. + ID: CollectionTab.ImportBloomSource.EditChoice + + + I want to make new book(s) based on the book(s) I am importing (derivatives). + Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). + ID: CollectionTab.ImportBloomSource.DerivativeChoice + + + Some of the book(s) you are importing are already in this collection. Do you want to replace them, or add the imported book(s) as new copies? + Shown once when importing .bloomSource file(s), if any of the books is already in the collection. + ID: CollectionTab.ImportBloomSource.DuplicatePrompt + + + Replace the existing book(s) + Radio-button choice in the import duplicate prompt: replace the existing book(s) with the imported one(s). + ID: CollectionTab.ImportBloomSource.Replace + + + Add them as new copies + Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. + ID: CollectionTab.ImportBloomSource.AddCopy + Make a book using this source ID: CollectionTab.MakeBookUsingThisTemplate diff --git a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx index 6ee785e41b46..392356ac347a 100644 --- a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx +++ b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx @@ -37,6 +37,7 @@ import { showMakeReaderTemplateBloomPackDialog, } from "../react_components/makeReaderTemplateBloomPackDialog"; import { AboutDialogLauncher } from "../react_components/aboutDialog"; +import { RadioChoiceDialog } from "./RadioChoiceDialog"; const kResizerSize = 10; @@ -197,6 +198,69 @@ export const CollectionsTabPane: React.FunctionComponent = () => { | undefined >(); + // The two dialogs that make up the .bloomSource import decision. The .bloomSource file(s) are + // chosen first (via C#); then the user is asked once (for the whole batch) whether to edit the + // book(s) or make derivatives; then, only in the edit case and only if any book is already in + // the collection, whether to replace those or add them as new copies. + const [showImportSourceChoiceDialog, setShowImportSourceChoiceDialog] = + useState(false); + const [showImportDuplicateDialog, setShowImportDuplicateDialog] = + useState(false); + + const importTitle = useL10n( + "Import .bloomSource File(s)", + "CollectionTab.ImportBloomSource", + ); + const importEditChoiceLabel = useL10n( + "I want to edit the book(s) I am importing.", + "CollectionTab.ImportBloomSource.EditChoice", + ); + const importDerivativeChoiceLabel = useL10n( + "I want to make new book(s) based on the book(s) I am importing (derivatives).", + "CollectionTab.ImportBloomSource.DerivativeChoice", + ); + const importDuplicateMessage = useL10n( + "Some of the book(s) you are importing are already in this collection. Do you want to replace them, or add the imported book(s) as new copies?", + "CollectionTab.ImportBloomSource.DuplicatePrompt", + ); + const importReplaceLabel = useL10n( + "Replace the existing book(s)", + "CollectionTab.ImportBloomSource.Replace", + ); + const importAddCopyLabel = useL10n( + "Add them as new copies", + "CollectionTab.ImportBloomSource.AddCopy", + ); + + // Step 2: the user chose to edit vs. make derivatives (or cancelled). For derivatives there can + // be no duplicates, so import immediately. For editing, first ask C# whether any chosen book is + // already in the collection; if so, show the duplicate dialog, otherwise import right away. + const handleImportSourceChoice = (choice?: string) => { + setShowImportSourceChoiceDialog(false); + if (!choice) return; + if (choice === "derivative") { + post("collections/importBloomSource?mode=derivative"); + return; + } + post("collections/anyChosenBloomSourceAlreadyInCollection", (r) => { + if (r.data === true) { + setShowImportDuplicateDialog(true); + } else { + post( + "collections/importBloomSource?mode=edit&onDuplicate=addCopy", + ); + } + }); + }; + + // Step 3 (edit + duplicates only): the user chose to replace the existing book(s) or add the + // imported ones as new copies. That single choice applies to every duplicate in the batch. + const handleImportDuplicateChoice = (choice?: string) => { + setShowImportDuplicateDialog(false); + if (!choice) return; + post(`collections/importBloomSource?mode=edit&onDuplicate=${choice}`); + }; + const setAdjustedContextMenuPoint = (x: number, y: number) => { setContextMousePoint({ mouseX: x - 2, @@ -277,6 +341,19 @@ export const CollectionsTabPane: React.FunctionComponent = () => { command: "workspace/openOrCreateCollection", addEllipsis: true, }, + { + label: "Import .bloomSource File(s)", + l10nId: "CollectionTab.ImportBloomSource", + onClick: () => { + handleClose(); + // Let the user choose the file(s) first; only then ask (once) whether to edit them + // or make derivatives. + post("collections/chooseBloomSourceFilesToImport", (r) => { + if (r.data === true) setShowImportSourceChoiceDialog(true); + }); + }, + addEllipsis: true, + }, { label: "Make Bloom Pack of Shell Books", l10nId: "CollectionTab.MakeBloomPackOfShellBooks", @@ -587,6 +664,28 @@ export const CollectionsTabPane: React.FunctionComponent = () => { + + ); }; diff --git a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx new file mode 100644 index 000000000000..96853b518a8f --- /dev/null +++ b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx @@ -0,0 +1,87 @@ +import { css } from "@emotion/react"; +import * as React from "react"; +import { useState, useEffect } from "react"; +import { Radio, RadioGroup, FormControlLabel } from "@mui/material"; +import { + BloomDialog, + DialogTitle, + DialogMiddle, + DialogBottomButtons, +} from "../react_components/BloomDialog/BloomDialog"; +import { + DialogOkButton, + DialogCancelButton, +} from "../react_components/BloomDialog/commonDialogComponents"; + +export interface IRadioChoice { + value: string; + label: string; // already localized +} + +// A small, reusable dialog that offers a set of mutually-exclusive radio choices with OK and +// Cancel. OK is disabled until the user selects one; Cancel is always available. It is rendered +// inline as part of its parent screen; the parent controls `open` and receives the chosen value +// (or undefined if the user cancelled) via `onClose`. All strings are passed in already localized. +export const RadioChoiceDialog: React.FunctionComponent<{ + open: boolean; + title: string; + message?: string; + options: IRadioChoice[]; + onClose: (value?: string) => void; +}> = (props) => { + const [choice, setChoice] = useState(undefined); + + // Start with nothing selected each time the dialog opens, so OK is disabled until the user picks. + useEffect(() => { + if (props.open) setChoice(undefined); + }, [props.open]); + + const cancel = () => props.onClose(undefined); + + return ( + + + + {props.message && ( +

+ {props.message} +

+ )} + setChoice(e.target.value)} + > + {props.options.map((o) => ( + } + label={o.label} + /> + ))} + + + + props.onClose(choice)} + /> + + + + ); +}; diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index dd673f4b8fd4..a8c27ddcf86b 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -48,6 +48,11 @@ public partial class CollectionModel private LocalizationChangedEvent _localizationChangedEvent; private BookCollectionHolder _bookCollectionHolder; + // The .bloomSource files the user chose to import, remembered between the file-picker step + // (ChooseBloomSourceFilesToImport) and the actual import (ImportBloomSourceFiles), so the + // collection screen can ask edit-vs-derivative in between. + private string[] _bloomSourceFilesToImport; + public CollectionModel( string pathToCollection, CollectionSettings collectionSettings, @@ -189,6 +194,521 @@ public void DuplicateBook(Book.Book book) } } + /// + /// What the user chose to do when an imported .bloomSource book is already present + /// (same bookInstanceId) in the editable collection. + /// + internal enum ImportDuplicateChoice + { + Replace, + AddCopy, + Cancel, + } + + /// + /// A problem with an imported .bloomSource file that we can explain to the user + /// (e.g. corrupt file, wrong kind of file, no book inside). The Message is shown + /// directly to the user, so it should be friendly and not require a bug report. + /// + internal class BloomSourceImportException : Exception + { + public BloomSourceImportException(string userMessage) + : base(userMessage) { } + } + + /// + /// Prompts the user to pick one or more .bloomSource files, remembering them for a following + /// call. Returns true if at least one file was chosen. + /// This is separate from the import itself so the collection screen can ask (once, for the + /// whole batch) whether to edit or make derivatives *after* the files have been chosen. + /// + public bool ChooseBloomSourceFilesToImport() + { + using (var dlg = new BloomOpenFileDialog()) + { + dlg.Multiselect = true; + dlg.CheckFileExists = true; + dlg.Title = LocalizationManager.GetString( + "CollectionTab.ImportBloomSource", + "Import .bloomSource File(s)" + ); + dlg.Filter = + "Bloom Source files (*.bloomSource)|*.bloomSource|Team Collection books (*.bloom)|*.bloom|All files (*.*)|*.*"; + if (dlg.ShowDialog() != DialogResult.OK) + { + _bloomSourceFilesToImport = null; + return false; + } + _bloomSourceFilesToImport = dlg.FileNames; + return _bloomSourceFilesToImport.Length > 0; + } + } + + /// + /// Imports the .bloomSource files previously chosen via + /// into the current editable collection. The + /// user has already made two choices (in the collection screen) that apply to the whole + /// batch: when is true, each imported book is used to make + /// a new derivative book (otherwise each is imported as an editable book); and, for the edit + /// case, when is true any book already in the + /// collection is replaced (otherwise it is added as a numbered copy). The last successfully + /// imported book is selected when done. + /// + public void ImportBloomSourceFiles(bool makeDerivatives, bool replaceExistingDuplicates) + { + var paths = _bloomSourceFilesToImport; + _bloomSourceFilesToImport = null; + if (paths == null || paths.Length == 0) + return; + + // The user has already chosen, once for the whole batch, what to do when an imported + // book is already in the collection (only relevant when editing, not making derivatives). + var duplicateChoice = replaceExistingDuplicates + ? ImportDuplicateChoice.Replace + : ImportDuplicateChoice.AddCopy; + + Book.Book lastImported = null; + foreach (var path in paths) + { + try + { + var book = makeDerivatives + ? MakeDerivativeFromBloomSourceFile(path) + : ImportOneBloomSourceFile(path, _ => duplicateChoice); + if (book != null) + lastImported = book; + } + catch (BloomSourceImportException e) + { + // A problem we can explain to the user; no bug report needed. + ErrorReport.NotifyUserOfProblem( + "{0}\r\n\r\n{1}", + Path.GetFileName(path), + e.Message + ); + } + catch (Exception e) + { + // Something unexpected; let the user report it. + NonFatalProblem.Report( + ModalIf.All, + PassiveIf.None, + shortUserLevelMessage: string.Format( + "Bloom was not able to import \"{0}\".", + Path.GetFileName(path) + ), + moreDetails: null, + exception: e + ); + } + } + + if (lastImported != null) + SelectBook(lastImported); + } + + /// + /// Imports the single book contained in one .bloomSource (or legacy .bloom) file into the + /// editable collection and returns the new Book, or null if the user cancelled. + /// The callback decides what to do when the book is + /// already in the collection; it is a parameter so tests can drive it without a dialog. + /// Throws for problems we can explain to the user. + /// + internal Book.Book ImportOneBloomSourceFile( + string sourcePath, + Func resolveDuplicate + ) + { + var destFolder = ImportBloomSourceFileToCollectionFolder( + sourcePath, + resolveDuplicate, + out var addNumberPrefix + ); + if (destFolder == null) + return null; // user cancelled + + ReloadEditableCollection(); + var newInfo = TheOneEditableCollection + .GetBookInfos() + .FirstOrDefault(i => i.FolderPath == destFolder); + if (newInfo == null) + throw new BloomSourceImportException( + "The book was extracted but did not show up in the collection." + ); + var newBook = GetBookFromBookInfo(newInfo); + + if (addNumberPrefix) + PrefixCaptionWithUniqueNumber(newBook); + + BookHistory.AddEvent( + newBook, + BookHistoryEventType.Created, + $"Imported from \"{Path.GetFileName(sourcePath)}\"" + ); + return newBook; + } + + /// + /// Does the file-system part of importing a .bloomSource: validates it, extracts it, removes + /// the stray .bloomCollection and any Team-Collection status files, handles the + /// already-in-collection case (Replace/Add-a-copy/Cancel), and moves the book into the + /// editable collection folder. Returns the destination folder, or null if the user cancelled. + /// is set true when the caller should give the new book a + /// numbered caption (the "Add a copy" case). This is separated from the Book-level work so it + /// can be unit tested without loading a Book. + /// + internal string ImportBloomSourceFileToCollectionFolder( + string sourcePath, + Func resolveDuplicate, + out bool addNumberPrefix + ) + { + addNumberPrefix = false; + + var editable = TheOneEditableCollection; + var tempFolder = ExtractAndPrepareBloomSourceToTemp( + sourcePath, + out var htmlPath, + out var instanceId + ); + string destFolder = null; + string folderToRecycleAfterImport = null; + try + { + var baseName = Path.GetFileNameWithoutExtension(htmlPath); + + var existing = string.IsNullOrEmpty(instanceId) + ? null + : editable.GetBookInfos().FirstOrDefault(b => b.Id == instanceId); + if (existing != null) + { + switch (resolveDuplicate(existing.Title)) + { + case ImportDuplicateChoice.Cancel: + return null; + case ImportDuplicateChoice.Replace: + // Keep the imported book's id and recycle the existing book, but only + // *after* the imported book is safely in place (below); recycling first + // would lose the original if the move then failed. The existing book + // (same id) stays on disk for now, so GetUniqueBookFolderName just picks + // a fresh folder name to avoid the transient collision. + folderToRecycleAfterImport = existing.FolderPath; + break; + case ImportDuplicateChoice.AddCopy: + // Make the copy independent: new id (so the two books don't collide) + // and a numbered caption (handled after the book is loaded). + addNumberPrefix = true; + instanceId = Guid.NewGuid().ToString(); + var meta = BookMetaData.FromFolder(tempFolder); + if (meta != null) + { + meta.Id = instanceId; + meta.WriteToFolder(tempFolder); + } + break; + } + } + + // Pick a folder name that doesn't collide with an existing book on disk, and make + // the main htm file match it (Bloom's convention: folder name == book htm name). + var destName = BookStorage.GetUniqueBookFolderName( + editable.PathToDirectory, + BookStorage.SanitizeNameForFileSystem(baseName), + instanceId, + "-" + ); + destFolder = Path.Combine(editable.PathToDirectory, destName); + var renamedHtmlPath = Path.Combine(tempFolder, destName + ".htm"); + if (!string.Equals(htmlPath, renamedHtmlPath, StringComparison.OrdinalIgnoreCase)) + RobustFile.Move(htmlPath, renamedHtmlPath); + SIL.IO.RobustIO.MoveDirectory(tempFolder, destFolder); + + // The imported book is now safely in place; for the Replace case it is finally safe + // to recycle the book it duplicates. + if (folderToRecycleAfterImport != null) + { + if (_bookSelection.CurrentSelection?.FolderPath == folderToRecycleAfterImport) + _bookSelection.SelectBook(null); + PathUtilities.DeleteToRecycleBin(folderToRecycleAfterImport); + editable.HandleBookDeletedFromCollection(folderToRecycleAfterImport); + } + return destFolder; + } + catch + { + // If we failed after starting to create the destination folder, don't leave a + // half-imported book behind in the collection. + if (destFolder != null && Directory.Exists(destFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(destFolder); + throw; + } + finally + { + if (Directory.Exists(tempFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); + } + } + + /// + /// Validates a .bloomSource file, extracts it to a fresh temp folder, removes the stray + /// .bloomCollection settings file and any Team-Collection status files, and confirms the + /// folder contains a book. Returns the temp folder path (the caller owns deleting it), the + /// book's main htm path, and its bookInstanceId (may be null). On any failure the temp + /// folder is cleaned up and the exception is rethrown. + /// + private string ExtractAndPrepareBloomSourceToTemp( + string sourcePath, + out string htmlPath, + out string instanceId + ) + { + // A .bloomSource is a zip of a single book folder's *contents* (flat at the root), + // plus the collection's .bloomCollection settings file. Validate that shape and guard + // against malicious entries before we extract anything. + ValidateBloomSourceZip(sourcePath); + + // Extract onto the same volume as the collection. The book is later moved into the + // collection folder with a directory move, which cannot cross volumes; the system + // temp folder is frequently on a different drive than the user's collection (e.g. %TEMP% + // on C: but the collection on D: or a removable/network drive), which would make every + // edit-mode import fail. Staging in the collection's parent guarantees a same-volume move. + // The name is dot-prefixed so it is ignored by the collection's book scan if it ever ends + // up inside a scanned folder. + var collectionDir = TheOneEditableCollection.PathToDirectory; + var stagingParent = Directory.GetParent(collectionDir)?.FullName ?? Path.GetTempPath(); + var tempFolder = Path.Combine(stagingParent, ".BloomImport-" + Guid.NewGuid()); + try + { + ZipUtils.ExpandZip(sourcePath, tempFolder); + + // The .bloomCollection file (there should be exactly one, but delete any) is + // collection-level and must not end up inside the book folder. + foreach (var settingsFile in Directory.GetFiles(tempFolder, "*.bloomCollection")) + RobustFile.Delete(settingsFile); + + htmlPath = BookStorage.FindBookHtmlInFolder(tempFolder); + if (string.IsNullOrEmpty(htmlPath)) + throw new BloomSourceImportException("No book was found in this file."); + + // An imported book should not carry over Team Collection status from wherever it came from. + BookStorage.RemoveLocalOnlyFiles(tempFolder); + + instanceId = BookMetaData.FromFolder(tempFolder)?.Id; + return tempFolder; + } + catch + { + if (Directory.Exists(tempFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); + throw; + } + } + + /// + /// Imports one .bloomSource file by making a NEW derivative book (new id and lineage) from + /// the book it contains, placed in the editable collection. Returns the new Book, or null if + /// creation was cancelled (e.g. a template configuration dialog). Because a derivative always + /// gets a fresh id, there is never a duplicate to resolve. + /// + internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath) + { + var tempFolder = ExtractAndPrepareBloomSourceToTemp(sourcePath, out _, out _); + try + { + var newBook = _bookServer.CreateFromSourceBook( + tempFolder, + TheOneEditableCollection.PathToDirectory + ); + if (newBook == null) + return null; // e.g. the source had a configuration dialog and the user cancelled + + newBook.BringBookUpToDate(new NullProgress(), false); + BookHistory.AddEvent( + newBook, + BookHistoryEventType.Created, + $"Created from imported source file \"{Path.GetFileName(sourcePath)}\"" + ); + + var newFolderPath = newBook.FolderPath; + ReloadEditableCollection(); + var newInfo = TheOneEditableCollection + .GetBookInfos() + .FirstOrDefault(i => i.FolderPath == newFolderPath); + return newInfo != null ? GetBookFromBookInfo(newInfo) : newBook; + } + finally + { + if (Directory.Exists(tempFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); + } + } + + /// + /// Opens the file as a zip and confirms it looks like a single-book .bloomSource: it must + /// open as a zip, must not contain path-traversal/absolute entries (zip-slip), and must not + /// be a Bloom Pack (a zip whose sole top-level entry is a collection folder). + /// Throws otherwise. + /// + private void ValidateBloomSourceZip(string sourcePath) + { + ICSharpCode.SharpZipLib.Zip.ZipFile zip; + try + { + zip = new ICSharpCode.SharpZipLib.Zip.ZipFile(sourcePath); + } + catch (Exception) + { + throw new BloomSourceImportException("This is not a valid Bloom source file."); + } + try + { + var topLevelDirs = new HashSet(); + var hasTopLevelFile = false; + foreach (ICSharpCode.SharpZipLib.Zip.ZipEntry entry in zip) + { + var name = entry.Name.Replace('\\', '/'); + var parts = name.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries); + // Zip-slip / absolute path guard: reject anything that could escape the target folder. + if (Path.IsPathRooted(entry.Name) || parts.Contains("..")) + throw new BloomSourceImportException( + "This file contains invalid entries and cannot be imported." + ); + if (parts.Length == 0) + continue; + if (parts.Length == 1 && !entry.IsDirectory) + hasTopLevelFile = true; + else + topLevelDirs.Add(parts[0]); + } + + // A single book always has files at the root (meta.json, the book htm). A Bloom Pack + // instead wraps everything in one collection folder, so it has no top-level files. + if (!hasTopLevelFile && topLevelDirs.Count == 1) + throw new BloomSourceImportException( + "This looks like a Bloom Pack (a whole collection), not a single book. " + + "To install a Bloom Pack, double-click it or use \"Open or Create Another Collection\"." + ); + } + finally + { + zip.Close(); + } + } + + /// + /// Returns true if any of the .bloomSource files the user chose (via + /// ) contains a book whose bookInstanceId is + /// already present in the editable collection. The collection screen uses this to decide + /// whether to ask the user how duplicates should be handled before importing. + /// + public bool AnyChosenBloomSourceIsAlreadyInCollection() + { + return AnyBloomSourceIsAlreadyInCollection(_bloomSourceFilesToImport); + } + + /// + /// Returns true if any of the given .bloomSource files contains a book whose bookInstanceId + /// is already present in the editable collection. Detection is purely by bookInstanceId, not + /// by title or folder name: two books with the same name but different ids are not duplicates, + /// while two books with the same id are (that id also controls re-uploading to the Bloom + /// library, so we must never end up with two books sharing one). Split out from + /// so it can be unit tested without a + /// file-picker dialog. + /// + internal bool AnyBloomSourceIsAlreadyInCollection(string[] paths) + { + if (paths == null || paths.Length == 0) + return false; + + var existingIds = new HashSet( + TheOneEditableCollection + .GetBookInfos() + .Select(b => b.Id) + .Where(id => !string.IsNullOrEmpty(id)) + ); + foreach (var path in paths) + { + var id = ReadBookInstanceIdFromBloomSource(path); + if (!string.IsNullOrEmpty(id) && existingIds.Contains(id)) + return true; + } + return false; + } + + /// + /// Reads the bookInstanceId from a .bloomSource file's meta.json (which is at the zip root) + /// without extracting the whole book. Returns null if it can't be read (e.g. the file is + /// not a valid single-book source); such files simply aren't treated as duplicates here. + /// + private static string ReadBookInstanceIdFromBloomSource(string sourcePath) + { + try + { + using (var zip = new ICSharpCode.SharpZipLib.Zip.ZipFile(sourcePath)) + { + var index = zip.FindEntry("meta.json", true); + if (index < 0) + return null; + using (var stream = zip.GetInputStream(index)) + using (var reader = new StreamReader(stream)) + { + var json = Newtonsoft.Json.Linq.JObject.Parse(reader.ReadToEnd()); + return (string)json["bookInstanceId"]; + } + } + } + catch (Exception) + { + return null; + } + } + + /// + /// Prefixes the book's displayed title with the smallest unused number starting at 2 + /// (e.g. "2 Moon and Cap") so an added copy is distinguishable from the book it duplicates. + /// The caption for an editable book comes from the in-HTML bookTitle, so we set that (not + /// just meta.json) and then save. + /// + private void PrefixCaptionWithUniqueNumber(Book.Book newBook) + { + var baseTitle = newBook.NameBestForUserDisplay; + var existingTitles = TheOneEditableCollection + .GetBookInfos() + .Where(i => i.FolderPath != newBook.FolderPath) + .Select(i => i.Title); + var number = ComputeUniqueCaptionNumber(baseTitle, existingTitles); + + // Prefix every language form so whichever one drives the caption starts with the number. + var titleForms = newBook.BookData.GetMultiTextVariableOrEmpty("bookTitle"); + foreach (var form in titleForms.Forms) + { + newBook.BookData.Set( + "bookTitle", + XmlString.FromXml($"{number} {form.Form}"), + form.WritingSystemId + ); + } + newBook.Save(); + UpdateLabelOfBookInEditableCollection(newBook); + } + + /// + /// Returns the smallest whole number, starting at 2, such that "{number} {baseTitle}" is not + /// already the title of a book in the collection. Used to give an added copy of an imported + /// book a distinguishable caption ("2 Moon and Cap", then "3 Moon and Cap", ...). + /// + internal static int ComputeUniqueCaptionNumber( + string baseTitle, + IEnumerable existingTitles + ) + { + var titles = new HashSet(existingTitles); + var number = 2; + while (titles.Contains($"{number} {baseTitle}")) + number++; + return number; + } + public void moveBookIntoThisCollection(Book.Book origBook, BookCollection origCollection) { var possibleTCFilePath = TeamCollectionManager.GetTcLinkPathFromLcPath( diff --git a/src/BloomExe/MiscUI/BloomOpenFileDialog.cs b/src/BloomExe/MiscUI/BloomOpenFileDialog.cs index 53d4c2455ff5..8a9e4cb1e6c0 100644 --- a/src/BloomExe/MiscUI/BloomOpenFileDialog.cs +++ b/src/BloomExe/MiscUI/BloomOpenFileDialog.cs @@ -18,8 +18,15 @@ public BloomOpenFileDialog() _dialog.FileOk += (sender, args) => { // Truly enforce the filter. See BL-12929 and BL-13552. - if (!DoubleCheckFileFilter(_dialog.Filter, _dialog.FileName)) - args.Cancel = true; + // When Multiselect is on, check every selected file, not just the first one. + foreach (var path in _dialog.FileNames) + { + if (!DoubleCheckFileFilter(_dialog.Filter, path)) + { + args.Cancel = true; + return; + } + } }; } @@ -77,6 +84,15 @@ public string FileName set { _dialog.FileName = value; } } + /// + /// The full paths of all files the user selected. When Multiselect is false this + /// contains just the single selected file (or is empty if none was chosen). + /// + public string[] FileNames + { + get { return _dialog.FileNames; } + } + public DialogResult ShowDialog() { return _dialog.ShowDialog(); diff --git a/src/BloomExe/web/controllers/CollectionApi.cs b/src/BloomExe/web/controllers/CollectionApi.cs index 90246ab27a93..9b4c6a37913d 100644 --- a/src/BloomExe/web/controllers/CollectionApi.cs +++ b/src/BloomExe/web/controllers/CollectionApi.cs @@ -309,6 +309,53 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) true ); + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "chooseBloomSourceFilesToImport/", + (request) => + { + // Opens the file picker so the user can select the .bloomSource file(s) to import. + // Replies true if at least one was chosen; the collection screen then asks whether + // to edit or make derivatives before calling importBloomSource. + request.ReplyWithBoolean(_collectionModel.ChooseBloomSourceFilesToImport()); + }, + true + ); + + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "anyChosenBloomSourceAlreadyInCollection/", + (request) => + { + // Replies true if any book the user chose to import is already in the collection, + // so the collection screen knows whether to ask how duplicates should be handled. + request.ReplyWithBoolean( + _collectionModel.AnyChosenBloomSourceIsAlreadyInCollection() + ); + }, + true + ); + + apiHandler.RegisterEndpointHandler( + kApiUrlPart + "importBloomSource/", + (request) => + { + // Imports the .bloomSource file(s) the user already chose (via + // chooseBloomSourceFilesToImport) into the current editable collection. The + // collection screen has since asked the user whether they want to edit the + // imported book(s) or make derivatives ("mode"), and, when editing, what to do + // with any book already in the collection ("onDuplicate"). Each single choice + // applies to every chosen file. + var makeDerivatives = request.RequiredParam("mode") == "derivative"; + var replaceExistingDuplicates = + request.GetParamOrNull("onDuplicate") == "replace"; + _collectionModel.ImportBloomSourceFiles( + makeDerivatives, + replaceExistingDuplicates + ); + request.PostSucceeded(); + }, + true + ); + apiHandler.RegisterEndpointHandler( kApiUrlPart + "doChecksOfAllBooks/", (request) => diff --git a/src/BloomTests/CollectionTab/CollectionModelTests.cs b/src/BloomTests/CollectionTab/CollectionModelTests.cs index 6fc52cae3387..8b0e85da5e19 100644 --- a/src/BloomTests/CollectionTab/CollectionModelTests.cs +++ b/src/BloomTests/CollectionTab/CollectionModelTests.cs @@ -5,6 +5,8 @@ using System.Linq; using System.Text; using Bloom.Book; +using Bloom.CollectionTab; +using Bloom.Utils; using BloomTests.TestDoubles.CollectionTab; using ICSharpCode.SharpZipLib.Zip; using NUnit.Framework; @@ -114,6 +116,419 @@ private static List GetActualFilenamesFromZipfile(string bloomPackName) return actualFiles; } + // ---- Import .bloomSource tests ---- + + private static string MetaJson(string instanceId, string title) + { + return $"{{\"bookInstanceId\":\"{instanceId}\",\"title\":\"{title}\"}}"; + } + + // Build a .bloomSource file the same way Bloom's export does: a zip of a book folder's + // contents (flat at the root) plus a .bloomCollection settings file at the root. + private string MakeBloomSourceFile(string title, string instanceId, bool includeHtm = true) + { + var src = Path.Combine(_folder.Path, "srcbook-" + Guid.NewGuid()); + Directory.CreateDirectory(src); + if (includeHtm) + File.WriteAllText( + Path.Combine(src, title + ".htm"), + "" + ); + File.WriteAllText(Path.Combine(src, "book.css"), "/*css*/"); + File.WriteAllText(Path.Combine(src, "meta.json"), MetaJson(instanceId, title)); + + var collectionFile = Path.Combine(_folder.Path, "SourceCollection.bloomCollection"); + if (!File.Exists(collectionFile)) + File.WriteAllText(collectionFile, ""); + + var dest = Path.Combine(_folder.Path, title + "-" + Guid.NewGuid() + ".bloomSource"); + var ok = CollectionModel.SaveAsBloomSourceFile( + src, + dest, + out var ex, + new[] { collectionFile } + ); + Assert.That(ok, Is.True, "Test setup failed to create .bloomSource: " + ex?.Message); + return dest; + } + + // Put a book directly in the editable collection folder so import can find it as a duplicate. + private string MakeExistingBookInCollection(string title, string instanceId) + { + var folder = Path.Combine(_collection.Path, title); + Directory.CreateDirectory(folder); + File.WriteAllText(Path.Combine(folder, title + ".htm"), ""); + File.WriteAllText(Path.Combine(folder, "meta.json"), MetaJson(instanceId, title)); + return folder; + } + + private static CollectionModel.ImportDuplicateChoice NeverCalled(string title) + { + throw new Exception("resolveDuplicate should not have been called"); + } + + [Test] + public void ImportBloomSource_ValidFile_PlacesBookInCollectionWithoutBloomCollection() + { + var src = MakeBloomSourceFile("Moon", Guid.NewGuid().ToString()); + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + NeverCalled, + out var addNumberPrefix + ); + + Assert.That(dest, Is.Not.Null); + Assert.That(Directory.Exists(dest), Is.True, "book folder should exist"); + Assert.That( + Directory.GetParent(dest).FullName, + Is.EqualTo(_collection.Path), + "book should be placed directly in the editable collection" + ); + Assert.That(File.Exists(Path.Combine(dest, "meta.json")), Is.True); + // The main htm file is renamed to match the (unique) folder name. + Assert.That( + File.Exists(Path.Combine(dest, Path.GetFileName(dest) + ".htm")), + Is.True, + "book htm should be renamed to match the folder" + ); + // The stray collection settings file must not end up in the book folder. + Assert.That( + Directory.GetFiles(dest, "*.bloomCollection"), + Is.Empty, + "the .bloomCollection file should have been removed" + ); + Assert.That(addNumberPrefix, Is.False); + } + + [Test] + public void ImportBloomSource_DuplicateId_AddCopy_MakesIndependentCopyWithNewId() + { + var sharedId = Guid.NewGuid().ToString(); + MakeExistingBookInCollection("Moon", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + title => CollectionModel.ImportDuplicateChoice.AddCopy, + out var addNumberPrefix + ); + + Assert.That(dest, Is.Not.Null); + Assert.That(addNumberPrefix, Is.True, "an added copy should get a numbered caption"); + var importedId = BookMetaData.FromFolder(dest).Id; + Assert.That( + importedId, + Is.Not.EqualTo(sharedId), + "an added copy should get a new instance id so it is independent" + ); + // The original book is still there, unchanged. + Assert.That(Directory.Exists(Path.Combine(_collection.Path, "Moon")), Is.True); + } + + [Test] + public void ImportBloomSource_DuplicateId_Replace_RecyclesExistingBook() + { + var sharedId = Guid.NewGuid().ToString(); + var existingFolder = MakeExistingBookInCollection("Moon", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + title => CollectionModel.ImportDuplicateChoice.Replace, + out var addNumberPrefix + ); + + Assert.That(dest, Is.Not.Null); + Assert.That(addNumberPrefix, Is.False, "replacing should not renumber the caption"); + Assert.That( + Directory.Exists(existingFolder), + Is.False, + "the existing book should have been recycled" + ); + Assert.That(BookMetaData.FromFolder(dest).Id, Is.EqualTo(sharedId)); + } + + [Test] + public void ImportBloomSource_DuplicateId_Cancel_ImportsNothing() + { + var sharedId = Guid.NewGuid().ToString(); + MakeExistingBookInCollection("Moon", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + var foldersBefore = Directory.GetDirectories(_collection.Path).Length; + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + title => CollectionModel.ImportDuplicateChoice.Cancel, + out _ + ); + + Assert.That(dest, Is.Null, "cancel should return no imported book"); + Assert.That( + Directory.GetDirectories(_collection.Path).Length, + Is.EqualTo(foldersBefore), + "cancel should not add or remove any book" + ); + } + + [Test] + public void ImportBloomSource_SameTitleDifferentId_NotTreatedAsDuplicate() + { + // Duplicate detection is by bookInstanceId, NOT by title/folder name. A book with the + // same name but a different id is a different book, so it must import silently (the + // resolveDuplicate callback must never be invoked) rather than prompting the user. + MakeExistingBookInCollection("Moon", Guid.NewGuid().ToString()); + var importedId = Guid.NewGuid().ToString(); + var src = MakeBloomSourceFile("Moon", importedId); + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + NeverCalled, // if this is called, the code wrongly treated a same-name book as a duplicate + out var addNumberPrefix + ); + + Assert.That(dest, Is.Not.Null); + Assert.That( + addNumberPrefix, + Is.False, + "a non-duplicate import should not be renumbered" + ); + Assert.That( + BookMetaData.FromFolder(dest).Id, + Is.EqualTo(importedId), + "a non-duplicate import should keep its own id" + ); + // The same-named existing book is still there, and the import landed in its own folder. + Assert.That(Directory.Exists(Path.Combine(_collection.Path, "Moon")), Is.True); + Assert.That( + dest, + Is.Not.EqualTo(Path.Combine(_collection.Path, "Moon")), + "the import should land in a distinct folder from the same-named existing book" + ); + } + + [Test] + public void ImportBloomSource_SameIdDifferentTitle_IsTreatedAsDuplicate() + { + // The flip side: a matching bookInstanceId means it is the same book even when the + // titles differ, so we must resolve the duplicate rather than silently create a second + // book that shares an id. + var sharedId = Guid.NewGuid().ToString(); + var existingFolder = MakeExistingBookInCollection("Sun", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + var resolveCalled = false; + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + title => + { + resolveCalled = true; + return CollectionModel.ImportDuplicateChoice.Replace; + }, + out _ + ); + + Assert.That( + resolveCalled, + Is.True, + "a book with a matching id but a different title should still be detected as a duplicate" + ); + Assert.That(dest, Is.Not.Null); + Assert.That( + Directory.Exists(existingFolder), + Is.False, + "Replace should have recycled the existing (same-id) book" + ); + } + + [Test] + public void ImportBloomSource_DuplicateId_AddCopy_LeavesNoTwoBooksSharingAnId() + { + // The whole point of giving the added copy a new id: we must never end up with two + // books that share a bookInstanceId. That id also controls re-uploading to the Bloom + // library, so a collision would cause real bugs. + var sharedId = Guid.NewGuid().ToString(); + var originalFolder = MakeExistingBookInCollection("Moon", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + // Sanity check: before importing, the original really has the shared id. + Assert.That(BookMetaData.FromFolder(originalFolder).Id, Is.EqualTo(sharedId)); + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + title => CollectionModel.ImportDuplicateChoice.AddCopy, + out _ + ); + + var originalId = BookMetaData.FromFolder(originalFolder).Id; + var importedId = BookMetaData.FromFolder(dest).Id; + Assert.That( + originalId, + Is.EqualTo(sharedId), + "the original book should be left untouched with its id" + ); + Assert.That( + importedId, + Is.Not.EqualTo(originalId), + "the added copy must have a different id from the book it duplicates" + ); + } + + [Test] + public void AnyBloomSourceIsAlreadyInCollection_MatchingId_ReturnsTrue() + { + // This is the pre-flight check that decides whether to show the duplicate dialog. + var sharedId = Guid.NewGuid().ToString(); + MakeExistingBookInCollection("Moon", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + + Assert.That( + _testCollectionModel.AnyBloomSourceIsAlreadyInCollection(new[] { src }), + Is.True + ); + } + + [Test] + public void AnyBloomSourceIsAlreadyInCollection_SameTitleDifferentId_ReturnsFalse() + { + // The dialog is triggered by a matching bookInstanceId, not a matching name. + MakeExistingBookInCollection("Moon", Guid.NewGuid().ToString()); + var src = MakeBloomSourceFile("Moon", Guid.NewGuid().ToString()); + + Assert.That( + _testCollectionModel.AnyBloomSourceIsAlreadyInCollection(new[] { src }), + Is.False, + "a same-named book with a different id is not a duplicate" + ); + } + + [Test] + public void AnyBloomSourceIsAlreadyInCollection_DifferentIdAndTitle_ReturnsFalse() + { + MakeExistingBookInCollection("Sun", Guid.NewGuid().ToString()); + var src = MakeBloomSourceFile("Moon", Guid.NewGuid().ToString()); + + Assert.That( + _testCollectionModel.AnyBloomSourceIsAlreadyInCollection(new[] { src }), + Is.False + ); + } + + [Test] + public void AnyBloomSourceIsAlreadyInCollection_NoFilesChosen_ReturnsFalse() + { + Assert.That(_testCollectionModel.AnyBloomSourceIsAlreadyInCollection(null), Is.False); + Assert.That( + _testCollectionModel.AnyBloomSourceIsAlreadyInCollection(new string[0]), + Is.False + ); + } + + [Test] + public void ImportBloomSource_CorruptFile_Throws() + { + var bad = Path.Combine(_folder.Path, "corrupt.bloomSource"); + File.WriteAllText(bad, "this is not a zip file"); + + Assert.Throws(() => + _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + bad, + NeverCalled, + out _ + ) + ); + } + + [Test] + public void ImportBloomSource_NoBookInside_Throws() + { + var src = MakeBloomSourceFile("Moon", Guid.NewGuid().ToString(), includeHtm: false); + + Assert.Throws(() => + _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + NeverCalled, + out _ + ) + ); + } + + [Test] + public void ImportBloomSource_BloomPackShapedZip_Throws() + { + // A Bloom Pack wraps everything in a single collection folder (no files at the root). + var collDir = Path.Combine(_folder.Path, "MyCollection"); + var bookDir = Path.Combine(collDir, "SomeBook"); + Directory.CreateDirectory(bookDir); + File.WriteAllText(Path.Combine(bookDir, "SomeBook.htm"), ""); + File.WriteAllText(Path.Combine(bookDir, "meta.json"), MetaJson("id-1", "SomeBook")); + var packPath = Path.Combine(_folder.Path, "pack.bloomSource"); + var zip = new BloomZipFile(packPath); + zip.AddDirectory(collDir); // includes "MyCollection" as the single root folder + zip.Save(); + + Assert.Throws(() => + _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + packPath, + NeverCalled, + out _ + ) + ); + } + + [Test] + public void ImportBloomSource_ZipSlipEntry_Throws_AndWritesNothingOutside() + { + var slipPath = Path.Combine(_folder.Path, "slip.bloomSource"); + using (var fs = File.Create(slipPath)) + using (var zos = new ZipOutputStream(fs)) + { + var bytes = Encoding.UTF8.GetBytes("x"); + zos.PutNextEntry(new ZipEntry("../evil.txt")); + zos.Write(bytes, 0, bytes.Length); + zos.CloseEntry(); + zos.PutNextEntry(new ZipEntry("Book.htm")); + zos.Write(bytes, 0, bytes.Length); + zos.CloseEntry(); + zos.IsStreamOwner = true; + } + + Assert.Throws(() => + _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + slipPath, + NeverCalled, + out _ + ) + ); + // The traversal target must never be written. + Assert.That( + File.Exists(Path.Combine(_folder.Path, "evil.txt")), + Is.False, + "zip-slip entry must not be extracted outside the target folder" + ); + } + + [Test] + public void ComputeUniqueCaptionNumber_StartsAtTwoAndSkipsUsed() + { + Assert.That( + CollectionModel.ComputeUniqueCaptionNumber("Moon", new string[0]), + Is.EqualTo(2) + ); + Assert.That( + CollectionModel.ComputeUniqueCaptionNumber("Moon", new[] { "2 Moon" }), + Is.EqualTo(3) + ); + Assert.That( + CollectionModel.ComputeUniqueCaptionNumber("Moon", new[] { "2 Moon", "3 Moon" }), + Is.EqualTo(4) + ); + // A gap at 2 is filled first. + Assert.That( + CollectionModel.ComputeUniqueCaptionNumber("Moon", new[] { "3 Moon" }), + Is.EqualTo(2) + ); + } + [Test] public void MakeBloomPack_DoesntIncludePdfFile() { diff --git a/src/BloomTests/TestDoubles/CollectionTab/FakeCollectionModel.cs b/src/BloomTests/TestDoubles/CollectionTab/FakeCollectionModel.cs index f5a9d16e4b97..63061f9c5583 100644 --- a/src/BloomTests/TestDoubles/CollectionTab/FakeCollectionModel.cs +++ b/src/BloomTests/TestDoubles/CollectionTab/FakeCollectionModel.cs @@ -1,4 +1,5 @@ using Bloom; +using Bloom.Api; using Bloom.Book; using Bloom.Collection; using Bloom.CollectionTab; @@ -37,7 +38,11 @@ public FakeCollectionModel( null, null ), - null, + // A real (unstarted) web socket server: CollectionModel's CollectionChanged handler + // calls _webSocketServer.SendEvent when a book is added/removed (e.g. when an import + // replaces an existing book), which would NullReferenceException if this were null. + // With no sockets connected, SendEvent is a harmless no-op. + new BloomWebSocketServer(), null, null ) @@ -59,7 +64,13 @@ public static BookCollection BookCollectionFactory( CollectionSettings settings = null ) { - return new BookCollection(path, collectionType, null); + // A real BookSelection (not null) is required: BookCollection.AddBookInfo dereferences + // it (_bookSelection.CurrentSelection) while enumerating books, so passing null makes + // every book come back as an ErrorBookInfo (with a random Id), which in turn breaks + // duplicate-by-bookInstanceId detection in tests that rely on GetBookInfos(). + // We deliberately leave collectionSettings null: with it set, GetBestDisplayTitle tries + // to build a BookData from the (deliberately minimal) test book html and throws. + return new BookCollection(path, collectionType, new BookSelection()); } } } From 10ea1e0b2db2027ee35ef68f6b16856fb7938f6d Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 1 Jul 2026 18:02:44 -0600 Subject: [PATCH 02/17] Address Greptile style nits: using-scope the zip, tidy useState (BL-16502) - ValidateBloomSourceZip: dispose the ZipFile via `using` instead of a finally/Close (matches ReadBookInstanceIdFromBloomSource). - RadioChoiceDialog: use `useState()` per src/BloomBrowserUI/AGENTS.md rather than the explicit `useState(undefined)` form. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx | 2 +- src/BloomExe/CollectionTab/CollectionModel.cs | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx index 96853b518a8f..5087b90bb608 100644 --- a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx +++ b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx @@ -29,7 +29,7 @@ export const RadioChoiceDialog: React.FunctionComponent<{ options: IRadioChoice[]; onClose: (value?: string) => void; }> = (props) => { - const [choice, setChoice] = useState(undefined); + const [choice, setChoice] = useState(); // Start with nothing selected each time the dialog opens, so OK is disabled until the user picks. useEffect(() => { diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index a8c27ddcf86b..205776676935 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -560,7 +560,7 @@ private void ValidateBloomSourceZip(string sourcePath) { throw new BloomSourceImportException("This is not a valid Bloom source file."); } - try + using (zip) { var topLevelDirs = new HashSet(); var hasTopLevelFile = false; @@ -589,10 +589,6 @@ private void ValidateBloomSourceZip(string sourcePath) + "To install a Bloom Pack, double-click it or use \"Open or Create Another Collection\"." ); } - finally - { - zip.Close(); - } } /// From 3e20160a1118ab425c44e25a807e9dc1945da5bd Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 1 Jul 2026 18:24:40 -0600 Subject: [PATCH 03/17] Address preflight review items for .bloomSource import (BL-16502) Following review feedback: 1. Import no longer blocks the UI thread. importBloomSource is now an async, non-UI-thread endpoint that runs the batch behind the collection tab's existing EmbeddedProgressDialog (per-file progress), so a large batch neither freezes the collection screen nor lets the browser request time out. SelectBook is marshaled back to the UI thread. 2. "Add a copy" now reuses the Duplicate Book convention instead of a parallel scheme: the copy gets a new id and the " - Copy-" folder name, and the title is left unchanged (as Duplicate does). Removed the hand-rolled PrefixCaptionWithUniqueNumber / ComputeUniqueCaptionNumber and the addNumberPrefix plumbing. 3. The derivative-import path now mirrors the normal "make a book from this source" flow (CreateFromSourceBook): defer the Created history event via PendingCreationSource and add the new book in memory, instead of an immediate history event plus a reload-and-lookup with a silent not-found fallback. 5. GetUniqueBookFolderName no longer throws on an imported book carrying a malformed/short instance id (guarded the Substring). 6. The pre-flight duplicate check reads the id with BookMetaData (same parser as the import), so it can't disagree with the import about a book's id. Tests updated for the new add-copy behavior (folder-name convention, no caption renumbering) and the removed caption helper. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/Book/BookStorage.cs | 9 +- src/BloomExe/CollectionTab/CollectionModel.cs | 160 +++++++++--------- src/BloomExe/web/controllers/CollectionApi.cs | 11 +- .../CollectionTab/CollectionModelTests.cs | 82 ++------- 4 files changed, 111 insertions(+), 151 deletions(-) diff --git a/src/BloomExe/Book/BookStorage.cs b/src/BloomExe/Book/BookStorage.cs index 43ebb4661bba..deb83680f6c7 100644 --- a/src/BloomExe/Book/BookStorage.cs +++ b/src/BloomExe/Book/BookStorage.cs @@ -3799,8 +3799,13 @@ internal static string GetUniqueBookFolderName( string proposedName; do { - // Use first 8 characters of the instance ID (without hyphens) as the unique suffix - var instanceSuffix = instanceId.Replace("-", "").Substring(0, 8).ToLowerInvariant(); + // Use up to the first 8 characters of the instance ID (without hyphens) as the + // unique suffix. Normal ids are GUIDs, but an imported book could carry a + // malformed/short id, so guard the length rather than throwing. + var idDigits = instanceId.Replace("-", ""); + var instanceSuffix = idDigits + .Substring(0, Math.Min(8, idDigits.Length)) + .ToLowerInvariant(); proposedName = baseName + separator + instanceSuffix; if (!Directory.Exists(Path.Combine(parentPath, proposedName))) diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index 205776676935..0eaa814c82f0 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -6,6 +6,7 @@ using System.IO; using System.Linq; using System.Text.RegularExpressions; +using System.Threading.Tasks; using System.Windows.Forms; using System.Xml; using Bloom.Api; @@ -19,6 +20,7 @@ using Bloom.ToPalaso; using Bloom.ToPalaso.Experimental; using Bloom.Utils; +using Bloom.web; using Bloom.web.controllers; using DesktopAnalytics; using L10NSharp; @@ -254,7 +256,38 @@ public bool ChooseBloomSourceFilesToImport() /// collection is replaced (otherwise it is added as a numbered copy). The last successfully /// imported book is selected when done. /// - public void ImportBloomSourceFiles(bool makeDerivatives, bool replaceExistingDuplicates) + /// + /// Runs behind the embedded progress dialog on a + /// background thread, so a large batch neither freezes the collection screen nor lets the + /// awaiting browser request time out. The front-end already hosts the "collectionTab" + /// EmbeddedProgressDialog, so it needs no extra wiring. + /// + public async Task ImportBloomSourceFilesWithProgressAsync( + bool makeDerivatives, + bool replaceExistingDuplicates + ) + { + await BrowserProgressDialog.DoWorkWithProgressDialogAsync( + _webSocketServer, + (progress, worker) => + { + ImportBloomSourceFiles(makeDerivatives, replaceExistingDuplicates, progress); + return Task.FromResult(false); // false => close the dialog when we finish + }, + "collectionTab", + LocalizationManager.GetString( + "CollectionTab.ImportBloomSource.Importing", + "Importing Books" + ), + showCancelButton: false + ); + } + + public void ImportBloomSourceFiles( + bool makeDerivatives, + bool replaceExistingDuplicates, + IWebSocketProgress progress = null + ) { var paths = _bloomSourceFilesToImport; _bloomSourceFilesToImport = null; @@ -272,6 +305,7 @@ public void ImportBloomSourceFiles(bool makeDerivatives, bool replaceExistingDup { try { + progress?.MessageWithoutLocalizing($"Importing {Path.GetFileName(path)}..."); var book = makeDerivatives ? MakeDerivativeFromBloomSourceFile(path) : ImportOneBloomSourceFile(path, _ => duplicateChoice); @@ -304,7 +338,22 @@ public void ImportBloomSourceFiles(bool makeDerivatives, bool replaceExistingDup } if (lastImported != null) - SelectBook(lastImported); + SelectBookOnUiThread(lastImported); + } + + /// + /// Selects a book, marshaling to the UI thread when necessary. Import can run on a background + /// thread (behind the progress dialog), but SelectBook raises SelectionChanged synchronously + /// to WinForms views, so it must happen on the UI thread. When no window is open (e.g. unit + /// tests) this just runs inline. + /// + private void SelectBookOnUiThread(Book.Book book) + { + var form = Shell.GetShellOrOtherOpenForm(); + if (form != null && form.InvokeRequired) + form.Invoke((Action)(() => _bookSelection.SelectBook(book))); + else + _bookSelection.SelectBook(book); } /// @@ -319,11 +368,7 @@ internal Book.Book ImportOneBloomSourceFile( Func resolveDuplicate ) { - var destFolder = ImportBloomSourceFileToCollectionFolder( - sourcePath, - resolveDuplicate, - out var addNumberPrefix - ); + var destFolder = ImportBloomSourceFileToCollectionFolder(sourcePath, resolveDuplicate); if (destFolder == null) return null; // user cancelled @@ -337,9 +382,6 @@ out var addNumberPrefix ); var newBook = GetBookFromBookInfo(newInfo); - if (addNumberPrefix) - PrefixCaptionWithUniqueNumber(newBook); - BookHistory.AddEvent( newBook, BookHistoryEventType.Created, @@ -353,18 +395,13 @@ out var addNumberPrefix /// the stray .bloomCollection and any Team-Collection status files, handles the /// already-in-collection case (Replace/Add-a-copy/Cancel), and moves the book into the /// editable collection folder. Returns the destination folder, or null if the user cancelled. - /// is set true when the caller should give the new book a - /// numbered caption (the "Add a copy" case). This is separated from the Book-level work so it - /// can be unit tested without loading a Book. + /// This is separated from the Book-level work so it can be unit tested without loading a Book. /// internal string ImportBloomSourceFileToCollectionFolder( string sourcePath, - Func resolveDuplicate, - out bool addNumberPrefix + Func resolveDuplicate ) { - addNumberPrefix = false; - var editable = TheOneEditableCollection; var tempFolder = ExtractAndPrepareBloomSourceToTemp( sourcePath, @@ -373,6 +410,10 @@ out var instanceId ); string destFolder = null; string folderToRecycleAfterImport = null; + // "Add a copy" produces an independent copy exactly like the Duplicate Book command: + // a new id and the " - Copy-" folder convention (the caption is left alone, + // just as Duplicate leaves it). + var folderSeparator = "-"; try { var baseName = Path.GetFileNameWithoutExtension(htmlPath); @@ -395,9 +436,10 @@ out var instanceId folderToRecycleAfterImport = existing.FolderPath; break; case ImportDuplicateChoice.AddCopy: - // Make the copy independent: new id (so the two books don't collide) - // and a numbered caption (handled after the book is loaded). - addNumberPrefix = true; + // Make the copy independent with a new id (so the two books don't collide), + // and name its folder with the same " - Copy-" convention the Duplicate + // command uses. + folderSeparator = " - Copy-"; instanceId = Guid.NewGuid().ToString(); var meta = BookMetaData.FromFolder(tempFolder); if (meta != null) @@ -415,7 +457,7 @@ out var instanceId editable.PathToDirectory, BookStorage.SanitizeNameForFileSystem(baseName), instanceId, - "-" + folderSeparator ); destFolder = Path.Combine(editable.PathToDirectory, destName); var renamedHtmlPath = Path.Combine(tempFolder, destName + ".htm"); @@ -428,7 +470,7 @@ out var instanceId if (folderToRecycleAfterImport != null) { if (_bookSelection.CurrentSelection?.FolderPath == folderToRecycleAfterImport) - _bookSelection.SelectBook(null); + SelectBookOnUiThread(null); PathUtilities.DeleteToRecycleBin(folderToRecycleAfterImport); editable.HandleBookDeletedFromCollection(folderToRecycleAfterImport); } @@ -522,19 +564,20 @@ internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath) if (newBook == null) return null; // e.g. the source had a configuration dialog and the user cancelled - newBook.BringBookUpToDate(new NullProgress(), false); - BookHistory.AddEvent( - newBook, - BookHistoryEventType.Created, - $"Created from imported source file \"{Path.GetFileName(sourcePath)}\"" + // Mirror the normal "make a book from this source" flow (CreateFromSourceBook): + // defer the "Created" history event via a pending marker so it captures the title the + // user eventually gives the book (and suppresses spurious "Renamed" events meanwhile), + // and add the new book to the collection in memory rather than reloading and looking it + // up again (which was the source of a silent not-found fallback). + newBook.PendingCreationSource = Path.GetFileName(sourcePath); + newBook.PendingCreationSourceTitle = newBook.BookInfo.GetTitleForLanguage( + newBook.BookData.Language1Tag ); - var newFolderPath = newBook.FolderPath; - ReloadEditableCollection(); - var newInfo = TheOneEditableCollection - .GetBookInfos() - .FirstOrDefault(i => i.FolderPath == newFolderPath); - return newInfo != null ? GetBookFromBookInfo(newInfo) : newBook; + newBook.BringBookUpToDate(new NullProgress(), false); + + TheOneEditableCollection.AddBookInfo(newBook.BookInfo); + return newBook; } finally { @@ -635,6 +678,8 @@ internal bool AnyBloomSourceIsAlreadyInCollection(string[] paths) /// Reads the bookInstanceId from a .bloomSource file's meta.json (which is at the zip root) /// without extracting the whole book. Returns null if it can't be read (e.g. the file is /// not a valid single-book source); such files simply aren't treated as duplicates here. + /// Parses the metadata the same way the import itself does (BookMetaData) so the pre-flight + /// duplicate check can't disagree with the actual import about a book's id. /// private static string ReadBookInstanceIdFromBloomSource(string sourcePath) { @@ -648,8 +693,7 @@ private static string ReadBookInstanceIdFromBloomSource(string sourcePath) using (var stream = zip.GetInputStream(index)) using (var reader = new StreamReader(stream)) { - var json = Newtonsoft.Json.Linq.JObject.Parse(reader.ReadToEnd()); - return (string)json["bookInstanceId"]; + return BookMetaData.FromString(reader.ReadToEnd())?.Id; } } } @@ -659,52 +703,6 @@ private static string ReadBookInstanceIdFromBloomSource(string sourcePath) } } - /// - /// Prefixes the book's displayed title with the smallest unused number starting at 2 - /// (e.g. "2 Moon and Cap") so an added copy is distinguishable from the book it duplicates. - /// The caption for an editable book comes from the in-HTML bookTitle, so we set that (not - /// just meta.json) and then save. - /// - private void PrefixCaptionWithUniqueNumber(Book.Book newBook) - { - var baseTitle = newBook.NameBestForUserDisplay; - var existingTitles = TheOneEditableCollection - .GetBookInfos() - .Where(i => i.FolderPath != newBook.FolderPath) - .Select(i => i.Title); - var number = ComputeUniqueCaptionNumber(baseTitle, existingTitles); - - // Prefix every language form so whichever one drives the caption starts with the number. - var titleForms = newBook.BookData.GetMultiTextVariableOrEmpty("bookTitle"); - foreach (var form in titleForms.Forms) - { - newBook.BookData.Set( - "bookTitle", - XmlString.FromXml($"{number} {form.Form}"), - form.WritingSystemId - ); - } - newBook.Save(); - UpdateLabelOfBookInEditableCollection(newBook); - } - - /// - /// Returns the smallest whole number, starting at 2, such that "{number} {baseTitle}" is not - /// already the title of a book in the collection. Used to give an added copy of an imported - /// book a distinguishable caption ("2 Moon and Cap", then "3 Moon and Cap", ...). - /// - internal static int ComputeUniqueCaptionNumber( - string baseTitle, - IEnumerable existingTitles - ) - { - var titles = new HashSet(existingTitles); - var number = 2; - while (titles.Contains($"{number} {baseTitle}")) - number++; - return number; - } - public void moveBookIntoThisCollection(Book.Book origBook, BookCollection origCollection) { var possibleTCFilePath = TeamCollectionManager.GetTcLinkPathFromLcPath( diff --git a/src/BloomExe/web/controllers/CollectionApi.cs b/src/BloomExe/web/controllers/CollectionApi.cs index 9b4c6a37913d..9b168944e366 100644 --- a/src/BloomExe/web/controllers/CollectionApi.cs +++ b/src/BloomExe/web/controllers/CollectionApi.cs @@ -334,9 +334,9 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) true ); - apiHandler.RegisterEndpointHandler( + apiHandler.RegisterAsyncEndpointHandler( kApiUrlPart + "importBloomSource/", - (request) => + async (request) => { // Imports the .bloomSource file(s) the user already chose (via // chooseBloomSourceFilesToImport) into the current editable collection. The @@ -344,16 +344,19 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) // imported book(s) or make derivatives ("mode"), and, when editing, what to do // with any book already in the collection ("onDuplicate"). Each single choice // applies to every chosen file. + // Runs behind the collection tab's progress dialog on a background thread (not the + // UI thread) so a large batch neither freezes the screen nor lets this request + // time out. var makeDerivatives = request.RequiredParam("mode") == "derivative"; var replaceExistingDuplicates = request.GetParamOrNull("onDuplicate") == "replace"; - _collectionModel.ImportBloomSourceFiles( + await _collectionModel.ImportBloomSourceFilesWithProgressAsync( makeDerivatives, replaceExistingDuplicates ); request.PostSucceeded(); }, - true + handleOnUiThread: false ); apiHandler.RegisterEndpointHandler( diff --git a/src/BloomTests/CollectionTab/CollectionModelTests.cs b/src/BloomTests/CollectionTab/CollectionModelTests.cs index 8b0e85da5e19..fe6136b60acc 100644 --- a/src/BloomTests/CollectionTab/CollectionModelTests.cs +++ b/src/BloomTests/CollectionTab/CollectionModelTests.cs @@ -174,8 +174,7 @@ public void ImportBloomSource_ValidFile_PlacesBookInCollectionWithoutBloomCollec var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( src, - NeverCalled, - out var addNumberPrefix + NeverCalled ); Assert.That(dest, Is.Not.Null); @@ -198,7 +197,6 @@ out var addNumberPrefix Is.Empty, "the .bloomCollection file should have been removed" ); - Assert.That(addNumberPrefix, Is.False); } [Test] @@ -210,18 +208,23 @@ public void ImportBloomSource_DuplicateId_AddCopy_MakesIndependentCopyWithNewId( var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( src, - title => CollectionModel.ImportDuplicateChoice.AddCopy, - out var addNumberPrefix + title => CollectionModel.ImportDuplicateChoice.AddCopy ); Assert.That(dest, Is.Not.Null); - Assert.That(addNumberPrefix, Is.True, "an added copy should get a numbered caption"); var importedId = BookMetaData.FromFolder(dest).Id; Assert.That( importedId, Is.Not.EqualTo(sharedId), "an added copy should get a new instance id so it is independent" ); + // The added copy follows the same " - Copy-" folder convention as the + // Duplicate Book command. + Assert.That( + Path.GetFileName(dest), + Does.Contain(" - Copy-"), + "an added copy should use the Duplicate-style folder name" + ); // The original book is still there, unchanged. Assert.That(Directory.Exists(Path.Combine(_collection.Path, "Moon")), Is.True); } @@ -235,12 +238,10 @@ public void ImportBloomSource_DuplicateId_Replace_RecyclesExistingBook() var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( src, - title => CollectionModel.ImportDuplicateChoice.Replace, - out var addNumberPrefix + title => CollectionModel.ImportDuplicateChoice.Replace ); Assert.That(dest, Is.Not.Null); - Assert.That(addNumberPrefix, Is.False, "replacing should not renumber the caption"); Assert.That( Directory.Exists(existingFolder), Is.False, @@ -259,8 +260,7 @@ public void ImportBloomSource_DuplicateId_Cancel_ImportsNothing() var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( src, - title => CollectionModel.ImportDuplicateChoice.Cancel, - out _ + title => CollectionModel.ImportDuplicateChoice.Cancel ); Assert.That(dest, Is.Null, "cancel should return no imported book"); @@ -283,16 +283,10 @@ public void ImportBloomSource_SameTitleDifferentId_NotTreatedAsDuplicate() var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( src, - NeverCalled, // if this is called, the code wrongly treated a same-name book as a duplicate - out var addNumberPrefix + NeverCalled // if this is called, the code wrongly treated a same-name book as a duplicate ); Assert.That(dest, Is.Not.Null); - Assert.That( - addNumberPrefix, - Is.False, - "a non-duplicate import should not be renumbered" - ); Assert.That( BookMetaData.FromFolder(dest).Id, Is.EqualTo(importedId), @@ -324,8 +318,7 @@ public void ImportBloomSource_SameIdDifferentTitle_IsTreatedAsDuplicate() { resolveCalled = true; return CollectionModel.ImportDuplicateChoice.Replace; - }, - out _ + } ); Assert.That( @@ -355,8 +348,7 @@ public void ImportBloomSource_DuplicateId_AddCopy_LeavesNoTwoBooksSharingAnId() var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( src, - title => CollectionModel.ImportDuplicateChoice.AddCopy, - out _ + title => CollectionModel.ImportDuplicateChoice.AddCopy ); var originalId = BookMetaData.FromFolder(originalFolder).Id; @@ -430,11 +422,7 @@ public void ImportBloomSource_CorruptFile_Throws() File.WriteAllText(bad, "this is not a zip file"); Assert.Throws(() => - _testCollectionModel.ImportBloomSourceFileToCollectionFolder( - bad, - NeverCalled, - out _ - ) + _testCollectionModel.ImportBloomSourceFileToCollectionFolder(bad, NeverCalled) ); } @@ -444,11 +432,7 @@ public void ImportBloomSource_NoBookInside_Throws() var src = MakeBloomSourceFile("Moon", Guid.NewGuid().ToString(), includeHtm: false); Assert.Throws(() => - _testCollectionModel.ImportBloomSourceFileToCollectionFolder( - src, - NeverCalled, - out _ - ) + _testCollectionModel.ImportBloomSourceFileToCollectionFolder(src, NeverCalled) ); } @@ -467,11 +451,7 @@ public void ImportBloomSource_BloomPackShapedZip_Throws() zip.Save(); Assert.Throws(() => - _testCollectionModel.ImportBloomSourceFileToCollectionFolder( - packPath, - NeverCalled, - out _ - ) + _testCollectionModel.ImportBloomSourceFileToCollectionFolder(packPath, NeverCalled) ); } @@ -493,11 +473,7 @@ public void ImportBloomSource_ZipSlipEntry_Throws_AndWritesNothingOutside() } Assert.Throws(() => - _testCollectionModel.ImportBloomSourceFileToCollectionFolder( - slipPath, - NeverCalled, - out _ - ) + _testCollectionModel.ImportBloomSourceFileToCollectionFolder(slipPath, NeverCalled) ); // The traversal target must never be written. Assert.That( @@ -507,28 +483,6 @@ out _ ); } - [Test] - public void ComputeUniqueCaptionNumber_StartsAtTwoAndSkipsUsed() - { - Assert.That( - CollectionModel.ComputeUniqueCaptionNumber("Moon", new string[0]), - Is.EqualTo(2) - ); - Assert.That( - CollectionModel.ComputeUniqueCaptionNumber("Moon", new[] { "2 Moon" }), - Is.EqualTo(3) - ); - Assert.That( - CollectionModel.ComputeUniqueCaptionNumber("Moon", new[] { "2 Moon", "3 Moon" }), - Is.EqualTo(4) - ); - // A gap at 2 is filled first. - Assert.That( - CollectionModel.ComputeUniqueCaptionNumber("Moon", new[] { "3 Moon" }), - Is.EqualTo(2) - ); - } - [Test] public void MakeBloomPack_DoesntIncludePdfFile() { From 3341445d669d82d666140dec98362828265965b7 Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 1 Jul 2026 21:17:22 -0600 Subject: [PATCH 04/17] Add an icon to the "Import .bloomSource File(s)" collection menu item (BL-16502) New ImportIcon (arrow-into-a-page glyph, from the provided design SVG) rendered via MUI SvgIcon with currentColor so it follows the menu text color. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../collectionsTab/CollectionsTabPane.tsx | 2 ++ .../react_components/icons/ImportIcon.tsx | 15 +++++++++++++++ 2 files changed, 17 insertions(+) create mode 100644 src/BloomBrowserUI/react_components/icons/ImportIcon.tsx diff --git a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx index 392356ac347a..c5ea656c6e0f 100644 --- a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx +++ b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx @@ -25,6 +25,7 @@ import { EmbeddedProgressDialog } from "../react_components/Progress/ProgressDia import { useSubscribeToWebSocketForObject } from "../utils/WebSocketManager"; import CloseIcon from "@mui/icons-material/Close"; import FolderOpenOutlinedIcon from "@mui/icons-material/FolderOpenOutlined"; +import { ImportIcon } from "../react_components/icons/ImportIcon"; import { kBloomBlue } from "../bloomMaterialUITheme"; import { BloomTooltip } from "../react_components/BloomToolTip"; import { Link } from "../react_components/link"; @@ -344,6 +345,7 @@ export const CollectionsTabPane: React.FunctionComponent = () => { { label: "Import .bloomSource File(s)", l10nId: "CollectionTab.ImportBloomSource", + icon: , onClick: () => { handleClose(); // Let the user choose the file(s) first; only then ask (once) whether to edit them diff --git a/src/BloomBrowserUI/react_components/icons/ImportIcon.tsx b/src/BloomBrowserUI/react_components/icons/ImportIcon.tsx new file mode 100644 index 000000000000..9d1eb549358a --- /dev/null +++ b/src/BloomBrowserUI/react_components/icons/ImportIcon.tsx @@ -0,0 +1,15 @@ +import SvgIcon, { SvgIconProps } from "@mui/material/SvgIcon"; + +// An "import into a document" icon (arrow pointing into a page), used for the +// "Import .bloomSource File(s)" collection menu item. Uses currentColor so it follows +// the menu text color. +export const ImportIcon = (props: SvgIconProps) => ( + + + +); + +export default ImportIcon; From 1951248c57ea61100a648cf938478f8d65c8bd1e Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 11:19:34 -0600 Subject: [PATCH 05/17] Address Greptile review: prevent import data loss, fix doc comment (BL-16502) - ImportBloomSourceFileToCollectionFolder: guard the catch-block cleanup with an importSucceeded flag so that a failure while recycling the replaced duplicate (in the Replace path) can no longer delete the book that was just successfully imported. - Move the misplaced ImportBloomSourceFiles doc comment down onto that method (the async wrapper had two summaries and the real method had none). - Add the missing CollectionTab.ImportBloomSource.Importing XLF entry for the import progress-dialog title (previously only had a code fallback). Co-Authored-By: Claude Opus 4.8 (1M context) --- DistFiles/localization/en/Bloom.xlf | 5 +++ src/BloomExe/CollectionTab/CollectionModel.cs | 32 +++++++++++-------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index c518878cb0c5..80554daf7f80 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -822,6 +822,11 @@ Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. ID: CollectionTab.ImportBloomSource.AddCopy + + Importing Books + Title of the progress dialog shown while .bloomSource file(s) are being imported. + ID: CollectionTab.ImportBloomSource.Importing + Make a book using this source ID: CollectionTab.MakeBookUsingThisTemplate diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index 0eaa814c82f0..6d62dfe3d417 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -246,16 +246,6 @@ public bool ChooseBloomSourceFilesToImport() } } - /// - /// Imports the .bloomSource files previously chosen via - /// into the current editable collection. The - /// user has already made two choices (in the collection screen) that apply to the whole - /// batch: when is true, each imported book is used to make - /// a new derivative book (otherwise each is imported as an editable book); and, for the edit - /// case, when is true any book already in the - /// collection is replaced (otherwise it is added as a numbered copy). The last successfully - /// imported book is selected when done. - /// /// /// Runs behind the embedded progress dialog on a /// background thread, so a large batch neither freezes the collection screen nor lets the @@ -283,6 +273,16 @@ await BrowserProgressDialog.DoWorkWithProgressDialogAsync( ); } + /// + /// Imports the .bloomSource files previously chosen via + /// into the current editable collection. The + /// user has already made two choices (in the collection screen) that apply to the whole + /// batch: when is true, each imported book is used to make + /// a new derivative book (otherwise each is imported as an editable book); and, for the edit + /// case, when is true any book already in the + /// collection is replaced (otherwise it is added as a numbered copy). The last successfully + /// imported book is selected when done. + /// public void ImportBloomSourceFiles( bool makeDerivatives, bool replaceExistingDuplicates, @@ -410,6 +410,9 @@ out var instanceId ); string destFolder = null; string folderToRecycleAfterImport = null; + // Once the book has been moved into the collection the import has succeeded; a failure + // after that point (e.g. recycling the replaced duplicate) must not delete it. + var importSucceeded = false; // "Add a copy" produces an independent copy exactly like the Duplicate Book command: // a new id and the " - Copy-" folder convention (the caption is left alone, // just as Duplicate leaves it). @@ -464,6 +467,7 @@ out var instanceId if (!string.Equals(htmlPath, renamedHtmlPath, StringComparison.OrdinalIgnoreCase)) RobustFile.Move(htmlPath, renamedHtmlPath); SIL.IO.RobustIO.MoveDirectory(tempFolder, destFolder); + importSucceeded = true; // The imported book is now safely in place; for the Replace case it is finally safe // to recycle the book it duplicates. @@ -478,9 +482,11 @@ out var instanceId } catch { - // If we failed after starting to create the destination folder, don't leave a - // half-imported book behind in the collection. - if (destFolder != null && Directory.Exists(destFolder)) + // If we failed partway through creating the destination folder, don't leave a + // half-imported book behind in the collection. But once the import has succeeded + // (the book is fully moved into place), a later failure such as recycling the + // replaced duplicate must not delete the book we just imported. + if (!importSucceeded && destFolder != null && Directory.Exists(destFolder)) SIL.IO.RobustIO.DeleteDirectoryAndContents(destFolder); throw; } From 033d12bd443eca17255a2ec11dde05ffdb9c3403 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 13:10:43 -0600 Subject: [PATCH 06/17] Reload the collection once per import batch, not once per file (BL-16502) ImportBloomSourceFiles previously called ReloadEditableCollection() inside the per-file helper (ImportOneBloomSourceFile), so importing N files re-scanned the whole collection from disk N times (O(files x collection size)). Restructure the edit path to move every book into place first, then reload exactly once and turn each imported folder into a Book with its "Imported from" history event. The derivative path was already reload-free (it adds its Book in memory) and is unchanged. Removes the now-unused ImportOneBloomSourceFile; the file-system method the tests drive (ImportBloomSourceFileToCollectionFolder) is untouched. Addresses preflight decision B on PR #8029. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/CollectionTab/CollectionModel.cs | 95 +++++++++++-------- 1 file changed, 56 insertions(+), 39 deletions(-) diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index 6d62dfe3d417..132000226a1d 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -301,16 +301,33 @@ public void ImportBloomSourceFiles( : ImportDuplicateChoice.AddCopy; Book.Book lastImported = null; + // For the edit path we move every book into place first and reload the collection just + // once at the end, instead of reloading (which rescans the whole collection from disk) + // once per file. Each entry pairs the destination folder with its source file name so we + // can record an accurate "Imported from" history event after that single reload. + var importedEditFolders = new List<(string destFolder, string sourceName)>(); foreach (var path in paths) { try { progress?.MessageWithoutLocalizing($"Importing {Path.GetFileName(path)}..."); - var book = makeDerivatives - ? MakeDerivativeFromBloomSourceFile(path) - : ImportOneBloomSourceFile(path, _ => duplicateChoice); - if (book != null) - lastImported = book; + if (makeDerivatives) + { + // The derivative path builds its Book in memory and adds it to the collection + // itself, so it needs no reload. + var book = MakeDerivativeFromBloomSourceFile(path); + if (book != null) + lastImported = book; + } + else + { + var destFolder = ImportBloomSourceFileToCollectionFolder( + path, + _ => duplicateChoice + ); + if (destFolder != null) + importedEditFolders.Add((destFolder, Path.GetFileName(path))); + } } catch (BloomSourceImportException e) { @@ -337,6 +354,40 @@ public void ImportBloomSourceFiles( } } + // Now that every edit-import's folder is in place, reload the collection a single time + // and turn each imported folder into a Book with its Created history event. + if (importedEditFolders.Count > 0) + { + ReloadEditableCollection(); + foreach (var (destFolder, sourceName) in importedEditFolders) + { + var newInfo = TheOneEditableCollection + .GetBookInfos() + .FirstOrDefault(i => i.FolderPath == destFolder); + if (newInfo == null) + { + // Should not happen after a successful move; surface it but don't abort the + // rest of the batch. + NonFatalProblem.Report( + ModalIf.All, + PassiveIf.None, + shortUserLevelMessage: string.Format( + "Bloom imported \"{0}\" but could not find it in the collection afterward.", + sourceName + ) + ); + continue; + } + var newBook = GetBookFromBookInfo(newInfo); + BookHistory.AddEvent( + newBook, + BookHistoryEventType.Created, + $"Imported from \"{sourceName}\"" + ); + lastImported = newBook; + } + } + if (lastImported != null) SelectBookOnUiThread(lastImported); } @@ -356,40 +407,6 @@ private void SelectBookOnUiThread(Book.Book book) _bookSelection.SelectBook(book); } - /// - /// Imports the single book contained in one .bloomSource (or legacy .bloom) file into the - /// editable collection and returns the new Book, or null if the user cancelled. - /// The callback decides what to do when the book is - /// already in the collection; it is a parameter so tests can drive it without a dialog. - /// Throws for problems we can explain to the user. - /// - internal Book.Book ImportOneBloomSourceFile( - string sourcePath, - Func resolveDuplicate - ) - { - var destFolder = ImportBloomSourceFileToCollectionFolder(sourcePath, resolveDuplicate); - if (destFolder == null) - return null; // user cancelled - - ReloadEditableCollection(); - var newInfo = TheOneEditableCollection - .GetBookInfos() - .FirstOrDefault(i => i.FolderPath == destFolder); - if (newInfo == null) - throw new BloomSourceImportException( - "The book was extracted but did not show up in the collection." - ); - var newBook = GetBookFromBookInfo(newInfo); - - BookHistory.AddEvent( - newBook, - BookHistoryEventType.Created, - $"Imported from \"{Path.GetFileName(sourcePath)}\"" - ); - return newBook; - } - /// /// Does the file-system part of importing a .bloomSource: validates it, extracts it, removes /// the stray .bloomCollection and any Team-Collection status files, handles the From cf2214512ed356bcd9a2836106ea4c4d9f3aadc2 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 13:20:32 -0600 Subject: [PATCH 07/17] Don't fail a successful import when recycling the replaced book fails (BL-16502) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile P1: in Replace mode, once the imported book is moved into place the import has succeeded. Previously the post-import cleanup (deselect, recycle the old book, drop it from the collection) ran in the main try block, so if any step threw (locked folder, recycle bin unavailable on a network drive, etc.) the exception bubbled up as a generic "Bloom was not able to import" error even though the book was imported — and because the method threw instead of returning the destination folder, that folder was never reloaded, so the imported book stayed invisible until a manual refresh. Wrap the recycle cleanup in its own try/catch that reports a NonFatalProblem ("imported, but couldn't remove the older copy") and still returns the destination folder, so the import is reported accurately and the book shows up. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/CollectionTab/CollectionModel.cs | 34 ++++++++++++++++--- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index 132000226a1d..351a95c9f731 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -487,13 +487,37 @@ out var instanceId importSucceeded = true; // The imported book is now safely in place; for the Replace case it is finally safe - // to recycle the book it duplicates. + // to recycle the book it duplicates. The import itself has already succeeded, so a + // failure recycling the old book (locked folder, recycle bin unavailable on a network + // drive, etc.) is reported as its own minor problem rather than being allowed to + // bubble up as an "import failed" error — which would both mislead the user and, since + // this method would then throw instead of returning destFolder, keep the + // successfully-imported book from being reloaded into view. if (folderToRecycleAfterImport != null) { - if (_bookSelection.CurrentSelection?.FolderPath == folderToRecycleAfterImport) - SelectBookOnUiThread(null); - PathUtilities.DeleteToRecycleBin(folderToRecycleAfterImport); - editable.HandleBookDeletedFromCollection(folderToRecycleAfterImport); + try + { + if ( + _bookSelection.CurrentSelection?.FolderPath + == folderToRecycleAfterImport + ) + SelectBookOnUiThread(null); + PathUtilities.DeleteToRecycleBin(folderToRecycleAfterImport); + editable.HandleBookDeletedFromCollection(folderToRecycleAfterImport); + } + catch (Exception e) + { + NonFatalProblem.Report( + ModalIf.All, + PassiveIf.None, + shortUserLevelMessage: string.Format( + "The book was imported, but Bloom could not remove the existing copy it replaced. You may need to delete the older copy of \"{0}\" manually.", + Path.GetFileName(destFolder) + ), + moreDetails: null, + exception: e + ); + } } return destFolder; } From 9e9a96db4f94f7d9d2f0ccffabf9625b33ff1a71 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 15:48:29 -0600 Subject: [PATCH 08/17] Prevent same-id books in one import batch from colliding (BL-16502) When two .bloomSource files selected together carry the same bookInstanceId, the collection is only reloaded once (after the whole batch), so the per-file duplicate check can't see a book imported earlier in the same batch. Track the ids imported so far in the batch and treat a within-batch repeat as an independent copy (fresh id, " - Copy-" folder), the same as the AddCopy case. Refactors the AddCopy id/folder logic into a shared makeIndependentCopy path and adds a unit test plus the missing param doc. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomExe/CollectionTab/CollectionModel.cs | 75 ++++++++++++++----- src/BloomExe/web/controllers/CollectionApi.cs | 8 +- .../CollectionTab/CollectionModelTests.cs | 44 +++++++++++ 3 files changed, 106 insertions(+), 21 deletions(-) diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index 351a95c9f731..9a1c14010b43 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -52,7 +52,7 @@ public partial class CollectionModel // The .bloomSource files the user chose to import, remembered between the file-picker step // (ChooseBloomSourceFilesToImport) and the actual import (ImportBloomSourceFiles), so the - // collection screen can ask edit-vs-derivative in between. + // collection screen can show the appropriate import dialog in between. private string[] _bloomSourceFilesToImport; public CollectionModel( @@ -221,8 +221,9 @@ public BloomSourceImportException(string userMessage) /// /// Prompts the user to pick one or more .bloomSource files, remembering them for a following /// call. Returns true if at least one file was chosen. - /// This is separate from the import itself so the collection screen can ask (once, for the - /// whole batch) whether to edit or make derivatives *after* the files have been chosen. + /// This is separate from the import itself so the collection screen can, once the files are + /// chosen, show the single import dialog appropriate to the batch (edit-vs-derivative when + /// none are already present, replace-vs-add-copy when any are). /// public bool ChooseBloomSourceFilesToImport() { @@ -276,7 +277,7 @@ await BrowserProgressDialog.DoWorkWithProgressDialogAsync( /// /// Imports the .bloomSource files previously chosen via /// into the current editable collection. The - /// user has already made two choices (in the collection screen) that apply to the whole + /// user has already made the choice (in the collection screen) that applies to the whole /// batch: when is true, each imported book is used to make /// a new derivative book (otherwise each is imported as an editable book); and, for the edit /// case, when is true any book already in the @@ -306,6 +307,11 @@ public void ImportBloomSourceFiles( // once per file. Each entry pairs the destination folder with its source file name so we // can record an accurate "Imported from" history event after that single reload. var importedEditFolders = new List<(string destFolder, string sourceName)>(); + // Because the collection isn't reloaded between files, the per-file duplicate check can't + // see books imported earlier in this same batch. Track their ids here so two files that + // share a bookInstanceId don't both land with that id (which would defeat the whole point + // of duplicate handling). + var idsImportedThisBatch = new HashSet(); foreach (var path in paths) { try @@ -323,7 +329,8 @@ public void ImportBloomSourceFiles( { var destFolder = ImportBloomSourceFileToCollectionFolder( path, - _ => duplicateChoice + _ => duplicateChoice, + idsImportedThisBatch ); if (destFolder != null) importedEditFolders.Add((destFolder, Path.GetFileName(path))); @@ -414,9 +421,15 @@ private void SelectBookOnUiThread(Book.Book book) /// editable collection folder. Returns the destination folder, or null if the user cancelled. /// This is separated from the Book-level work so it can be unit tested without loading a Book. /// + /// Ids of books already imported earlier in this + /// same batch. Because the collection is reloaded only once, after the whole batch, a book + /// whose id is in this set isn't yet visible to the in-collection check above; we treat it as + /// a within-batch duplicate and give the incoming book a fresh id so the two don't collide. + /// When null (e.g. single-file tests), within-batch tracking is skipped. internal string ImportBloomSourceFileToCollectionFolder( string sourcePath, - Func resolveDuplicate + Func resolveDuplicate, + ISet idsAlreadyImportedThisBatch = null ) { var editable = TheOneEditableCollection; @@ -425,6 +438,10 @@ Func resolveDuplicate out var htmlPath, out var instanceId ); + // The id this book arrived with. We record it (once the import succeeds, below) so that a + // later file in the same batch carrying the same id is recognized as a duplicate even + // though the collection is only reloaded once, after the whole batch. + var originalInstanceId = instanceId; string destFolder = null; string folderToRecycleAfterImport = null; // Once the book has been moved into the collection the import has succeeded; a failure @@ -441,6 +458,19 @@ out var instanceId var existing = string.IsNullOrEmpty(instanceId) ? null : editable.GetBookInfos().FirstOrDefault(b => b.Id == instanceId); + // A book with this id isn't in the collection, but an earlier file in this same batch + // already brought it in (the collection is only reloaded once, after the batch, so it + // isn't visible above yet). Replacing a book from the same batch is meaningless, so we + // always make this one an independent copy so the two don't end up sharing an id. + var isDuplicateWithinBatch = + existing == null + && !string.IsNullOrEmpty(instanceId) + && idsAlreadyImportedThisBatch != null + && idsAlreadyImportedThisBatch.Contains(instanceId); + + // Whether to turn the incoming book into an independent copy: a new id (so it can't + // collide) plus the " - Copy-" folder convention the Duplicate command uses. + var makeIndependentCopy = false; if (existing != null) { switch (resolveDuplicate(existing.Title)) @@ -456,20 +486,26 @@ out var instanceId folderToRecycleAfterImport = existing.FolderPath; break; case ImportDuplicateChoice.AddCopy: - // Make the copy independent with a new id (so the two books don't collide), - // and name its folder with the same " - Copy-" convention the Duplicate - // command uses. - folderSeparator = " - Copy-"; - instanceId = Guid.NewGuid().ToString(); - var meta = BookMetaData.FromFolder(tempFolder); - if (meta != null) - { - meta.Id = instanceId; - meta.WriteToFolder(tempFolder); - } + makeIndependentCopy = true; break; } } + else if (isDuplicateWithinBatch) + { + makeIndependentCopy = true; + } + + if (makeIndependentCopy) + { + folderSeparator = " - Copy-"; + instanceId = Guid.NewGuid().ToString(); + var meta = BookMetaData.FromFolder(tempFolder); + if (meta != null) + { + meta.Id = instanceId; + meta.WriteToFolder(tempFolder); + } + } // Pick a folder name that doesn't collide with an existing book on disk, and make // the main htm file match it (Bloom's convention: folder name == book htm name). @@ -486,6 +522,11 @@ out var instanceId SIL.IO.RobustIO.MoveDirectory(tempFolder, destFolder); importSucceeded = true; + // Remember the id this book arrived with so a later file in the same batch carrying + // the same id is caught as a within-batch duplicate above. + if (!string.IsNullOrEmpty(originalInstanceId)) + idsAlreadyImportedThisBatch?.Add(originalInstanceId); + // The imported book is now safely in place; for the Replace case it is finally safe // to recycle the book it duplicates. The import itself has already succeeded, so a // failure recycling the old book (locked folder, recycle bin unavailable on a network diff --git a/src/BloomExe/web/controllers/CollectionApi.cs b/src/BloomExe/web/controllers/CollectionApi.cs index 9b168944e366..d0f6bf37d355 100644 --- a/src/BloomExe/web/controllers/CollectionApi.cs +++ b/src/BloomExe/web/controllers/CollectionApi.cs @@ -340,10 +340,10 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) { // Imports the .bloomSource file(s) the user already chose (via // chooseBloomSourceFilesToImport) into the current editable collection. The - // collection screen has since asked the user whether they want to edit the - // imported book(s) or make derivatives ("mode"), and, when editing, what to do - // with any book already in the collection ("onDuplicate"). Each single choice - // applies to every chosen file. + // collection screen has since shown the one dialog that fit the batch: when no + // chosen book was already present it asked edit-vs-derivative ("mode"); when any + // was present it asked replace-vs-add-copy ("onDuplicate", always mode=edit). + // Each single choice applies to every chosen file. // Runs behind the collection tab's progress dialog on a background thread (not the // UI thread) so a large batch neither freezes the screen nor lets this request // time out. diff --git a/src/BloomTests/CollectionTab/CollectionModelTests.cs b/src/BloomTests/CollectionTab/CollectionModelTests.cs index fe6136b60acc..094e74474157 100644 --- a/src/BloomTests/CollectionTab/CollectionModelTests.cs +++ b/src/BloomTests/CollectionTab/CollectionModelTests.cs @@ -365,6 +365,50 @@ public void ImportBloomSource_DuplicateId_AddCopy_LeavesNoTwoBooksSharingAnId() ); } + [Test] + public void ImportBloomSource_SameIdTwiceInOneBatch_SecondBecomesIndependentCopy() + { + // Two files in the same import batch can carry the same bookInstanceId (e.g. two exports + // of the same book selected together). The collection is only reloaded once, after the + // whole batch, so when the second file is imported the per-file duplicate check can't yet + // see the first. The shared id set the batch passes in must still keep the two from + // landing with the same id — the collision the whole feature exists to prevent. + var sharedId = Guid.NewGuid().ToString(); + var src1 = MakeBloomSourceFile("Moon", sharedId); + var src2 = MakeBloomSourceFile("Moon", sharedId); + var batchIds = new HashSet(); + + // Neither book is in the collection, so the duplicate dialog is never consulted for + // either (NeverCalled throws if it is); the within-batch collision must be handled + // automatically, not by prompting. + var dest1 = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src1, + NeverCalled, + batchIds + ); + var dest2 = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src2, + NeverCalled, + batchIds + ); + + Assert.That(dest1, Is.Not.Null); + Assert.That(dest2, Is.Not.Null); + Assert.That( + dest1, + Is.Not.EqualTo(dest2), + "the two imports must land in distinct folders" + ); + var id1 = BookMetaData.FromFolder(dest1).Id; + var id2 = BookMetaData.FromFolder(dest2).Id; + Assert.That(id1, Is.EqualTo(sharedId), "the first import keeps the original id"); + Assert.That( + id2, + Is.Not.EqualTo(id1), + "the second import of the same id in one batch must get a fresh id, not collide" + ); + } + [Test] public void AnyBloomSourceIsAlreadyInCollection_MatchingId_ReturnsTrue() { From 2c1d8b4947c9ab6b9adc66419bfb108130f52971 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 15:49:02 -0600 Subject: [PATCH 09/17] Refine .bloomSource import dialogs and use Bloom radio component (BL-16502) Show a single import dialog chosen up front by whether any book is already in the collection: edit-vs-derivative when none are present, replace-vs-add-copy when any are. Refresh the dialog copy ("Import books", shorter option labels) and give the replace/add-copy options an explanatory secondary line. Switch RadioChoiceDialog from raw MUI Radio to the Bloom MuiRadio component, extending MuiRadio with an optional already-localized description line. Co-Authored-By: Claude Opus 4.8 (1M context) --- DistFiles/localization/en/Bloom.xlf | 23 ++++- .../collectionsTab/CollectionsTabPane.tsx | 89 +++++++++++-------- .../collectionsTab/RadioChoiceDialog.tsx | 11 ++- .../react_components/muiRadio.tsx | 27 +++++- 4 files changed, 106 insertions(+), 44 deletions(-) diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index 80554daf7f80..6a82d22344c1 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -797,31 +797,46 @@ Menu command (and dialog title) that imports one or more books from .bloomSource files into the current collection. Keep ".bloomSource" unchanged; it is a file extension. ID: CollectionTab.ImportBloomSource + + Import books + Title of the dialogs shown while importing .bloomSource file(s): the one that asks whether to import for editing or as derivatives, and the one that asks how to handle books already in the collection. + ID: CollectionTab.ImportBloomSource.ChoiceTitle + - I want to edit the book(s) I am importing. + Import for editing Radio-button choice in the import dialog: import the book(s) to edit them directly. ID: CollectionTab.ImportBloomSource.EditChoice - I want to make new book(s) based on the book(s) I am importing (derivatives). + Import as derivatives — new books based on the originals Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). ID: CollectionTab.ImportBloomSource.DerivativeChoice - Some of the book(s) you are importing are already in this collection. Do you want to replace them, or add the imported book(s) as new copies? + Some of these books are already in this collection. Shown once when importing .bloomSource file(s), if any of the books is already in the collection. ID: CollectionTab.ImportBloomSource.DuplicatePrompt - Replace the existing book(s) + Replace the existing books Radio-button choice in the import duplicate prompt: replace the existing book(s) with the imported one(s). ID: CollectionTab.ImportBloomSource.Replace + + The current copies will be overwritten. + Secondary line under the "Replace the existing books" choice in the import duplicate prompt. + ID: CollectionTab.ImportBloomSource.ReplaceDescription + Add them as new copies Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. ID: CollectionTab.ImportBloomSource.AddCopy + + The existing books stay as they are. + Secondary line under the "Add them as new copies" choice in the import duplicate prompt. + ID: CollectionTab.ImportBloomSource.AddCopyDescription + Importing Books Title of the progress dialog shown while .bloomSource file(s) are being imported. diff --git a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx index c5ea656c6e0f..b514d1397bcd 100644 --- a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx +++ b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx @@ -199,63 +199,72 @@ export const CollectionsTabPane: React.FunctionComponent = () => { | undefined >(); - // The two dialogs that make up the .bloomSource import decision. The .bloomSource file(s) are - // chosen first (via C#); then the user is asked once (for the whole batch) whether to edit the - // book(s) or make derivatives; then, only in the edit case and only if any book is already in - // the collection, whether to replace those or add them as new copies. + // The .bloomSource import decision. The file(s) are chosen first (via C#); then we check (via + // C#) whether any chosen book is already in the collection and, based on that, show the user + // exactly one dialog for the whole batch. If none are present, Dialog 1 asks whether to edit + // the book(s) or make derivatives. If any are present, Dialog 2 asks whether to replace the + // existing book(s) or add the imported ones as new copies. The user never sees both. const [showImportSourceChoiceDialog, setShowImportSourceChoiceDialog] = useState(false); const [showImportDuplicateDialog, setShowImportDuplicateDialog] = useState(false); - const importTitle = useL10n( - "Import .bloomSource File(s)", - "CollectionTab.ImportBloomSource", + const importChoiceTitle = useL10n( + "Import books", + "CollectionTab.ImportBloomSource.ChoiceTitle", ); const importEditChoiceLabel = useL10n( - "I want to edit the book(s) I am importing.", + "Import for editing", "CollectionTab.ImportBloomSource.EditChoice", ); const importDerivativeChoiceLabel = useL10n( - "I want to make new book(s) based on the book(s) I am importing (derivatives).", + "Import as derivatives — new books based on the originals", "CollectionTab.ImportBloomSource.DerivativeChoice", ); const importDuplicateMessage = useL10n( - "Some of the book(s) you are importing are already in this collection. Do you want to replace them, or add the imported book(s) as new copies?", + "Some of these books are already in this collection.", "CollectionTab.ImportBloomSource.DuplicatePrompt", ); const importReplaceLabel = useL10n( - "Replace the existing book(s)", + "Replace the existing books", "CollectionTab.ImportBloomSource.Replace", ); + const importReplaceDescription = useL10n( + "The current copies will be overwritten.", + "CollectionTab.ImportBloomSource.ReplaceDescription", + ); const importAddCopyLabel = useL10n( "Add them as new copies", "CollectionTab.ImportBloomSource.AddCopy", ); + const importAddCopyDescription = useL10n( + "The existing books stay as they are.", + "CollectionTab.ImportBloomSource.AddCopyDescription", + ); + + // Decide which single dialog to show after the file(s) have been chosen: Dialog 2 (replace vs. + // add-copy) if any chosen book is already in the collection, otherwise Dialog 1 (edit vs. + // derivative). + const chooseImportDialog = () => { + post("collections/anyChosenBloomSourceAlreadyInCollection", (r) => { + if (r.data === true) setShowImportDuplicateDialog(true); + else setShowImportSourceChoiceDialog(true); + }); + }; - // Step 2: the user chose to edit vs. make derivatives (or cancelled). For derivatives there can - // be no duplicates, so import immediately. For editing, first ask C# whether any chosen book is - // already in the collection; if so, show the duplicate dialog, otherwise import right away. + // Dialog 1 (shown only when no chosen book is already in the collection): the user chose to + // edit the book(s) as-is or make derivatives (or cancelled). Neither path has a duplicate to + // resolve, so import immediately. const handleImportSourceChoice = (choice?: string) => { setShowImportSourceChoiceDialog(false); if (!choice) return; - if (choice === "derivative") { - post("collections/importBloomSource?mode=derivative"); - return; - } - post("collections/anyChosenBloomSourceAlreadyInCollection", (r) => { - if (r.data === true) { - setShowImportDuplicateDialog(true); - } else { - post( - "collections/importBloomSource?mode=edit&onDuplicate=addCopy", - ); - } - }); + const mode = choice === "derivative" ? "derivative" : "edit"; + post(`collections/importBloomSource?mode=${mode}`); }; - // Step 3 (edit + duplicates only): the user chose to replace the existing book(s) or add the - // imported ones as new copies. That single choice applies to every duplicate in the batch. + // Dialog 2 (shown only when at least one chosen book is already in the collection): the user + // chose to replace the existing book(s) or add the imported ones as new copies. That single + // choice applies to every duplicate in the batch. const handleImportDuplicateChoice = (choice?: string) => { setShowImportDuplicateDialog(false); if (!choice) return; @@ -348,10 +357,10 @@ export const CollectionsTabPane: React.FunctionComponent = () => { icon: , onClick: () => { handleClose(); - // Let the user choose the file(s) first; only then ask (once) whether to edit them - // or make derivatives. + // Choose the file(s) first; then show the one dialog that fits, based on whether + // any chosen book is already in the collection. post("collections/chooseBloomSourceFilesToImport", (r) => { - if (r.data === true) setShowImportSourceChoiceDialog(true); + if (r.data === true) chooseImportDialog(); }); }, addEllipsis: true, @@ -668,7 +677,7 @@ export const CollectionsTabPane: React.FunctionComponent = () => { { /> diff --git a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx index 5087b90bb608..9f992241da37 100644 --- a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx +++ b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx @@ -1,7 +1,8 @@ import { css } from "@emotion/react"; import * as React from "react"; import { useState, useEffect } from "react"; -import { Radio, RadioGroup, FormControlLabel } from "@mui/material"; +import { RadioGroup } from "@mui/material"; +import { MuiRadio } from "../react_components/muiRadio"; import { BloomDialog, DialogTitle, @@ -16,6 +17,7 @@ import { export interface IRadioChoice { value: string; label: string; // already localized + description?: string; // optional already-localized secondary line shown under the label } // A small, reusable dialog that offers a set of mutually-exclusive radio choices with OK and @@ -65,11 +67,14 @@ export const RadioChoiceDialog: React.FunctionComponent<{ onChange={(e) => setChoice(e.target.value)} > {props.options.map((o) => ( - } label={o.label} + description={o.description} + // The labels arrive already localized from the caller. + alreadyLocalized={true} + l10nKey="" /> ))} diff --git a/src/BloomBrowserUI/react_components/muiRadio.tsx b/src/BloomBrowserUI/react_components/muiRadio.tsx index ec38c715e221..c9e04709ab85 100644 --- a/src/BloomBrowserUI/react_components/muiRadio.tsx +++ b/src/BloomBrowserUI/react_components/muiRadio.tsx @@ -9,6 +9,9 @@ import { ILocalizationProps } from "./l10nComponents"; export const MuiRadio: React.FunctionComponent< ILocalizationProps & { label: string; + // An optional, already-localized secondary line shown in smaller, muted text + // beneath the main label (e.g. to explain the consequence of the choice). + description?: string; value?: string; disabled?: boolean; onChanged?: (v: boolean | undefined) => void; @@ -24,6 +27,28 @@ export const MuiRadio: React.FunctionComponent< props.l10nParam1, ); + const labelContent = + props.description === undefined ? ( + localizedLabel + ) : ( +
+ {localizedLabel} + + {props.description} + +
+ ); + // Work has been done below to ensure that a multiline (wrapped) label will align with the control correctly. // If messing with the layout, be sure you didn't break this by checking the storybook story. return ( @@ -49,7 +74,7 @@ export const MuiRadio: React.FunctionComponent< /> } - label={localizedLabel} + label={labelContent} /> ); }; From 9548eb58f5cfb706f73be3ea3568fbc4df44f81f Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 16:00:22 -0600 Subject: [PATCH 10/17] Add an explanatory line under the "Import as derivatives" choice (BL-16502) Gives the derivative radio option the same secondary-line treatment as the replace/add-copy options, explaining that original credits are preserved and attributed to the source book. Co-Authored-By: Claude Opus 4.8 (1M context) --- DistFiles/localization/en/Bloom.xlf | 5 +++++ src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index 6a82d22344c1..74ca98b77a4b 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -812,6 +812,11 @@ Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). ID: CollectionTab.ImportBloomSource.DerivativeChoice + + Credits from the original are preserved and attributed to the source book. + Explanation shown under the "Import as derivatives" radio-button choice in the import dialog. + ID: CollectionTab.ImportBloomSource.DerivativeChoiceDescription + Some of these books are already in this collection. Shown once when importing .bloomSource file(s), if any of the books is already in the collection. diff --git a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx index b514d1397bcd..8cc7e4a0ff69 100644 --- a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx +++ b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx @@ -221,6 +221,10 @@ export const CollectionsTabPane: React.FunctionComponent = () => { "Import as derivatives — new books based on the originals", "CollectionTab.ImportBloomSource.DerivativeChoice", ); + const importDerivativeChoiceDescription = useL10n( + "Credits from the original are preserved and attributed to the source book.", + "CollectionTab.ImportBloomSource.DerivativeChoiceDescription", + ); const importDuplicateMessage = useL10n( "Some of these books are already in this collection.", "CollectionTab.ImportBloomSource.DuplicatePrompt", @@ -683,6 +687,7 @@ export const CollectionsTabPane: React.FunctionComponent = () => { { value: "derivative", label: importDerivativeChoiceLabel, + description: importDerivativeChoiceDescription, }, ]} onClose={handleImportSourceChoice} From 5abcdc097bae5cb59645cfed616e14e5ba3387cf Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 16:15:15 -0600 Subject: [PATCH 11/17] Tweak messages --- DistFiles/localization/en/Bloom.xlf | 7 ++++++- .../collectionsTab/CollectionsTabPane.tsx | 12 ++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index 74ca98b77a4b..90a9eca0caf3 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -807,13 +807,18 @@ Radio-button choice in the import dialog: import the book(s) to edit them directly. ID: CollectionTab.ImportBloomSource.EditChoice + + Preserve everything. + Explanation shown under the "Import for editing" radio-button choice in the import dialog. + ID: CollectionTab.ImportBloomSource.EditChoiceDescription + Import as derivatives — new books based on the originals Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). ID: CollectionTab.ImportBloomSource.DerivativeChoice - Credits from the original are preserved and attributed to the source book. + Use a new book ID & make room for new copyright and credits while preserving those of the original. Explanation shown under the "Import as derivatives" radio-button choice in the import dialog. ID: CollectionTab.ImportBloomSource.DerivativeChoiceDescription diff --git a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx index 8cc7e4a0ff69..b6890e7a0e47 100644 --- a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx +++ b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx @@ -217,12 +217,16 @@ export const CollectionsTabPane: React.FunctionComponent = () => { "Import for editing", "CollectionTab.ImportBloomSource.EditChoice", ); + const importEditChoiceDescription = useL10n( + "Preserve everything.", + "CollectionTab.ImportBloomSource.EditChoiceDescription", + ); const importDerivativeChoiceLabel = useL10n( "Import as derivatives — new books based on the originals", "CollectionTab.ImportBloomSource.DerivativeChoice", ); const importDerivativeChoiceDescription = useL10n( - "Credits from the original are preserved and attributed to the source book.", + "Use a new book ID & make room for new copyright and credits while preserving those of the original.", "CollectionTab.ImportBloomSource.DerivativeChoiceDescription", ); const importDuplicateMessage = useL10n( @@ -683,7 +687,11 @@ export const CollectionsTabPane: React.FunctionComponent = () => { open={showImportSourceChoiceDialog} title={importChoiceTitle} options={[ - { value: "edit", label: importEditChoiceLabel }, + { + value: "edit", + label: importEditChoiceLabel, + description: importEditChoiceDescription, + }, { value: "derivative", label: importDerivativeChoiceLabel, From f63a1b683cac743ad0fdd613c9c11a6a7b39e8f9 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 2 Jul 2026 19:56:19 -0600 Subject: [PATCH 12/17] Disable "Replace" for un-checked-out Team Collection books on import (BL-16502) When importing a .bloomSource whose book is already in a Team Collection, the existing book can only be recycled if it is checked out here. The collection screen now asks C# (via the new bloomSourceImportDuplicateInfo endpoint) both whether any chosen book is a duplicate and whether every duplicate is currently replaceable; when any is not checked out it disables the "Replace" choice and explains why ("All books must be checked out"). ImportBloomSourceFileToCollectionFolder also guards the Replace path (throws if !IsSaveable) so a not-checked-out book can never be deleted even if the UI guard is bypassed. Adds RadioChoice/MuiRadio "disabled" support and five unit tests. Also documents (per review) why we don't offer edit-vs-derivative when a book is already in the collection. Co-Authored-By: Claude Opus 4.8 (1M context) --- DistFiles/localization/en/Bloom.xlf | 5 + .../collectionsTab/CollectionsTabPane.tsx | 26 +++- .../collectionsTab/RadioChoiceDialog.tsx | 2 + .../react_components/muiRadio.tsx | 1 + src/BloomExe/CollectionTab/CollectionModel.cs | 51 ++++++++ src/BloomExe/web/controllers/CollectionApi.cs | 17 ++- .../CollectionTab/CollectionModelTests.cs | 114 ++++++++++++++++++ 7 files changed, 207 insertions(+), 9 deletions(-) diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index 90a9eca0caf3..d3b348df95f8 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -837,6 +837,11 @@ Secondary line under the "Replace the existing books" choice in the import duplicate prompt. ID: CollectionTab.ImportBloomSource.ReplaceDescription + + All books must be checked out + Secondary line under the "Replace the existing books" choice in the import duplicate prompt, shown (and the choice disabled) when the collection is a Team Collection and one or more of the books being replaced is not checked out. + ID: CollectionTab.ImportBloomSource.ReplaceNeedsCheckout + Add them as new copies Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. diff --git a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx index b6890e7a0e47..92a391147bd4 100644 --- a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx +++ b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx @@ -208,6 +208,9 @@ export const CollectionsTabPane: React.FunctionComponent = () => { useState(false); const [showImportDuplicateDialog, setShowImportDuplicateDialog] = useState(false); + // Whether the "Replace" duplicate choice is allowed for this batch. In a Team Collection a book + // can only be replaced if it is checked out here; when any duplicate isn't, Replace is disabled. + const [importCanReplace, setImportCanReplace] = useState(true); const importChoiceTitle = useL10n( "Import books", @@ -241,6 +244,10 @@ export const CollectionsTabPane: React.FunctionComponent = () => { "The current copies will be overwritten.", "CollectionTab.ImportBloomSource.ReplaceDescription", ); + const importReplaceNeedsCheckoutDescription = useL10n( + "All books must be checked out", + "CollectionTab.ImportBloomSource.ReplaceNeedsCheckout", + ); const importAddCopyLabel = useL10n( "Add them as new copies", "CollectionTab.ImportBloomSource.AddCopy", @@ -253,10 +260,18 @@ export const CollectionsTabPane: React.FunctionComponent = () => { // Decide which single dialog to show after the file(s) have been chosen: Dialog 2 (replace vs. // add-copy) if any chosen book is already in the collection, otherwise Dialog 1 (edit vs. // derivative). + // + // Note that when a chosen book is already in the collection we deliberately do NOT offer the + // edit-vs-derivative choice: the duplicate dialog always imports in edit mode, so you can't + // make a derivative of a book that's already in your collection. We couldn't think of a + // plausible scenario where you have a book and a true derivative of it in the same collection, + // so this path isn't worth the extra complexity. Easy to reverse if that turns out to be wrong. const chooseImportDialog = () => { - post("collections/anyChosenBloomSourceAlreadyInCollection", (r) => { - if (r.data === true) setShowImportDuplicateDialog(true); - else setShowImportSourceChoiceDialog(true); + post("collections/bloomSourceImportDuplicateInfo", (r) => { + if (r.data.anyDuplicates) { + setImportCanReplace(r.data.canReplace); + setShowImportDuplicateDialog(true); + } else setShowImportSourceChoiceDialog(true); }); }; @@ -708,7 +723,10 @@ export const CollectionsTabPane: React.FunctionComponent = () => { { value: "replace", label: importReplaceLabel, - description: importReplaceDescription, + description: importCanReplace + ? importReplaceDescription + : importReplaceNeedsCheckoutDescription, + disabled: !importCanReplace, }, { value: "addCopy", diff --git a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx index 9f992241da37..d64f9841f9cc 100644 --- a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx +++ b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx @@ -18,6 +18,7 @@ export interface IRadioChoice { value: string; label: string; // already localized description?: string; // optional already-localized secondary line shown under the label + disabled?: boolean; // when true the choice can't be selected (e.g. not currently allowed) } // A small, reusable dialog that offers a set of mutually-exclusive radio choices with OK and @@ -72,6 +73,7 @@ export const RadioChoiceDialog: React.FunctionComponent<{ value={o.value} label={o.label} description={o.description} + disabled={o.disabled} // The labels arrive already localized from the caller. alreadyLocalized={true} l10nKey="" diff --git a/src/BloomBrowserUI/react_components/muiRadio.tsx b/src/BloomBrowserUI/react_components/muiRadio.tsx index c9e04709ab85..fe096d9218e8 100644 --- a/src/BloomBrowserUI/react_components/muiRadio.tsx +++ b/src/BloomBrowserUI/react_components/muiRadio.tsx @@ -53,6 +53,7 @@ export const MuiRadio: React.FunctionComponent< // If messing with the layout, be sure you didn't break this by checking the storybook story. return ( + /// Returns true only if every chosen .bloomSource book that is already in the collection may + /// currently be replaced. Replacing recycles the existing book, so in a Team Collection each + /// duplicate must be checked out here (BookInfo.IsSaveable); outside a Team Collection books + /// are always saveable. The collection screen uses this to disable the "Replace" duplicate + /// choice (and explain why) when any duplicate is not checked out, so the user can't delete a + /// book they haven't checked out. When there are no duplicates the question is moot, so this + /// returns true. + /// + public bool AllChosenBloomSourceDuplicatesAreReplaceable() + { + return AllBloomSourceDuplicatesAreReplaceable(_bloomSourceFilesToImport); + } + + /// + /// The testable core of . Duplicates + /// are detected purely by bookInstanceId, exactly as in + /// . + /// + internal bool AllBloomSourceDuplicatesAreReplaceable(string[] paths) + { + if (paths == null || paths.Length == 0) + return true; + + var existingById = TheOneEditableCollection + .GetBookInfos() + .Where(b => !string.IsNullOrEmpty(b.Id)) + .GroupBy(b => b.Id) + .ToDictionary(g => g.Key, g => g.First()); + foreach (var path in paths) + { + var id = ReadBookInstanceIdFromBloomSource(path); + if ( + !string.IsNullOrEmpty(id) + && existingById.TryGetValue(id, out var existing) + && !existing.IsSaveable + ) + return false; + } + return true; + } + /// /// Returns true if any of the given .bloomSource files contains a book whose bookInstanceId /// is already present in the editable collection. Detection is purely by bookInstanceId, not diff --git a/src/BloomExe/web/controllers/CollectionApi.cs b/src/BloomExe/web/controllers/CollectionApi.cs index d0f6bf37d355..c5bbdca10f34 100644 --- a/src/BloomExe/web/controllers/CollectionApi.cs +++ b/src/BloomExe/web/controllers/CollectionApi.cs @@ -322,13 +322,20 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) ); apiHandler.RegisterEndpointHandler( - kApiUrlPart + "anyChosenBloomSourceAlreadyInCollection/", + kApiUrlPart + "bloomSourceImportDuplicateInfo/", (request) => { - // Replies true if any book the user chose to import is already in the collection, - // so the collection screen knows whether to ask how duplicates should be handled. - request.ReplyWithBoolean( - _collectionModel.AnyChosenBloomSourceIsAlreadyInCollection() + // Tells the collection screen what it needs to pick and configure the duplicate + // dialog: whether any book the user chose to import is already in the collection + // (so it knows whether to ask how duplicates should be handled) and, when it is, + // whether every such duplicate may currently be replaced (false when, in a Team + // Collection, any is not checked out here, so the screen disables "Replace"). + request.ReplyWithJson( + new + { + anyDuplicates = _collectionModel.AnyChosenBloomSourceIsAlreadyInCollection(), + canReplace = _collectionModel.AllChosenBloomSourceDuplicatesAreReplaceable(), + } ); }, true diff --git a/src/BloomTests/CollectionTab/CollectionModelTests.cs b/src/BloomTests/CollectionTab/CollectionModelTests.cs index 094e74474157..6368076ab65b 100644 --- a/src/BloomTests/CollectionTab/CollectionModelTests.cs +++ b/src/BloomTests/CollectionTab/CollectionModelTests.cs @@ -459,6 +459,120 @@ public void AnyBloomSourceIsAlreadyInCollection_NoFilesChosen_ReturnsFalse() ); } + [Test] + public void AllBloomSourceDuplicatesAreReplaceable_NoFilesChosen_ReturnsTrue() + { + // With nothing to import there is no duplicate that could block "Replace". + Assert.That(_testCollectionModel.AllBloomSourceDuplicatesAreReplaceable(null), Is.True); + Assert.That( + _testCollectionModel.AllBloomSourceDuplicatesAreReplaceable(new string[0]), + Is.True + ); + } + + [Test] + public void AllBloomSourceDuplicatesAreReplaceable_NoDuplicate_ReturnsTrue() + { + // A book that isn't already in the collection isn't a duplicate, so "Replace" is moot. + MakeExistingBookInCollection("Sun", Guid.NewGuid().ToString()); + var src = MakeBloomSourceFile("Moon", Guid.NewGuid().ToString()); + + Assert.That( + _testCollectionModel.AllBloomSourceDuplicatesAreReplaceable(new[] { src }), + Is.True + ); + } + + [Test] + public void AllBloomSourceDuplicatesAreReplaceable_SaveableDuplicate_ReturnsTrue() + { + // Outside a Team Collection, books in the editable collection are always saveable, so a + // duplicate can be replaced. (The not-checked-out case that returns false requires a + // Team Collection; it is guarded both here and by ImportBloomSourceFileToCollectionFolder.) + var sharedId = Guid.NewGuid().ToString(); + var existingFolder = MakeExistingBookInCollection("Moon", sharedId); + Assert.That( + BookMetaData.FromFolder(existingFolder).Id, + Is.EqualTo(sharedId), + "test setup: the existing book should carry the shared id" + ); + var src = MakeBloomSourceFile("Moon", sharedId); + + Assert.That( + _testCollectionModel.AllBloomSourceDuplicatesAreReplaceable(new[] { src }), + Is.True + ); + } + + // A save context that reports a book can't currently be saved, standing in for a + // Team-Collection book that isn't checked out here (so we can test that path without + // setting up a real Team Collection). + private class NotCheckedOutSaveContext : ISaveContext + { + public bool CanSaveChanges(BookInfo info) => false; + + public bool CanChangeBookInstanceId(BookInfo info) => false; + } + + [Test] + public void AllBloomSourceDuplicatesAreReplaceable_DuplicateNotCheckedOut_ReturnsFalse() + { + var sharedId = Guid.NewGuid().ToString(); + var existingFolder = MakeExistingBookInCollection("Moon", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + + // Load the collection, then swap the existing book's info for one that reports it can't + // be saved, exactly as a Team-Collection book that isn't checked out here would. + var collection = _testCollectionModel.TheOneEditableCollection; + Assert.That( + collection.GetBookInfos().First(b => b.Id == sharedId).IsSaveable, + Is.True, + "test setup: the book should start out saveable (this is not a Team Collection)" + ); + collection.UpdateBookInfo( + new BookInfo(existingFolder, true, new NotCheckedOutSaveContext()) + ); + Assert.That( + collection.GetBookInfos().First(b => b.Id == sharedId).IsSaveable, + Is.False, + "test setup: the swapped-in book should report it is not saveable" + ); + + Assert.That( + _testCollectionModel.AllBloomSourceDuplicatesAreReplaceable(new[] { src }), + Is.False, + "a duplicate that isn't checked out must not be replaceable" + ); + } + + [Test] + public void ImportBloomSource_Replace_NotCheckedOut_Throws() + { + // Safety net: even if the UI's "Replace" guard were bypassed, importing must never + // delete a book that isn't checked out. + var sharedId = Guid.NewGuid().ToString(); + var existingFolder = MakeExistingBookInCollection("Moon", sharedId); + var src = MakeBloomSourceFile("Moon", sharedId); + + var collection = _testCollectionModel.TheOneEditableCollection; + collection.GetBookInfos(); // build the cache before we swap an entry + collection.UpdateBookInfo( + new BookInfo(existingFolder, true, new NotCheckedOutSaveContext()) + ); + + Assert.Throws(() => + _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + title => CollectionModel.ImportDuplicateChoice.Replace + ) + ); + Assert.That( + Directory.Exists(existingFolder), + Is.True, + "the not-checked-out book must not have been deleted" + ); + } + [Test] public void ImportBloomSource_CorruptFile_Throws() { From 8273d077a7611049c28b9a77b7bcb5c201ce4284 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 6 Jul 2026 20:20:04 -0600 Subject: [PATCH 13/17] Overwrite in place when replacing a book on .bloomSource import (BL-16502) Replacing a book that's already in the collection now overwrites the existing book folder in place, keeping its folder name and Team Collection checkout status, instead of importing into a new folder and deleting the old one. In a Team Collection the replaced book stays the same checked-out repo book (its new content is pushed on the next check-in) rather than becoming a new local book while the old one resurrects from the shared repo at the next sync. The set-aside old content is dot-prefixed and its cleanup is non-fatal, so a failed cleanup can't leave a scannable duplicate-id book or turn a successful replace into an "import failed" error. Also from preflight review decisions: - Record a derivative import's "Created" history event immediately, so every book in a multi-file batch gets one (only the last was ever selected, and the pending event only flushed on deselect). - Justify the RadioChoiceDialog reset useEffect per the repo useEffect rule. - When the collection is at a filesystem root, stage inside the collection folder rather than %TEMP% (which may be on another volume). Updated CollectionModelTests to cover overwrite-in-place and TC status preservation. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../collectionsTab/RadioChoiceDialog.tsx | 4 + src/BloomExe/CollectionTab/CollectionModel.cs | 192 +++++++++++++----- .../CollectionTab/CollectionModelTests.cs | 77 ++++++- 3 files changed, 215 insertions(+), 58 deletions(-) diff --git a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx index d64f9841f9cc..8a653b7589f5 100644 --- a/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx +++ b/src/BloomBrowserUI/collectionsTab/RadioChoiceDialog.tsx @@ -35,6 +35,10 @@ export const RadioChoiceDialog: React.FunctionComponent<{ const [choice, setChoice] = useState(); // Start with nothing selected each time the dialog opens, so OK is disabled until the user picks. + // A useEffect is warranted here (per the repo's useEffect guidance) rather than the usual + // key-prop remount: this dialog is rendered inline and permanently mounted by its parent, which + // toggles `open` rather than mounting/unmounting us, so there is no remount to hang a key on. + // Keying the reset off `open` keeps that concern owned by the component that holds the state. useEffect(() => { if (props.open) setChoice(undefined); }, [props.open]); diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index be09efb7e76f..523fac832532 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -443,7 +443,7 @@ out var instanceId // though the collection is only reloaded once, after the whole batch. var originalInstanceId = instanceId; string destFolder = null; - string folderToRecycleAfterImport = null; + string replaceTargetFolder = null; // Once the book has been moved into the collection the import has succeeded; a failure // after that point (e.g. recycling the replaced duplicate) must not delete it. var importSucceeded = false; @@ -478,21 +478,22 @@ out var instanceId case ImportDuplicateChoice.Cancel: return null; case ImportDuplicateChoice.Replace: - // Replacing recycles the existing book, so we must be allowed to delete - // it: in a Team Collection that means it has to be checked out here. The - // collection screen already disables "Replace" when any duplicate is not - // (see AllChosenBloomSourceDuplicatesAreReplaceable), but guard here too - // so we can never delete a book the user hasn't checked out. + // Replacing overwrites the existing book in place, so we must be allowed + // to modify it: in a Team Collection that means it has to be checked out + // here. The collection screen already disables "Replace" when any + // duplicate is not (see AllChosenBloomSourceDuplicatesAreReplaceable), but + // guard here too so we can never overwrite a book the user hasn't checked + // out. if (!existing.IsSaveable) throw new ApplicationException( $"Cannot replace \"{existing.Title}\" because it is not checked out of the Team Collection." ); - // Keep the imported book's id and recycle the existing book, but only - // *after* the imported book is safely in place (below); recycling first - // would lose the original if the move then failed. The existing book - // (same id) stays on disk for now, so GetUniqueBookFolderName just picks - // a fresh folder name to avoid the transient collision. - folderToRecycleAfterImport = existing.FolderPath; + // Overwrite this book's folder in place (below), keeping its folder name + // and — in a Team Collection — its checkout status, so it stays the same + // checked-out repo book (the new content is pushed to the shared repo on + // the next check-in) rather than becoming a new local book while the old + // one resurrects from the repo. The imported book keeps the same id. + replaceTargetFolder = existing.FolderPath; break; case ImportDuplicateChoice.AddCopy: makeIndependentCopy = true; @@ -516,6 +517,30 @@ out var instanceId } } + if (replaceTargetFolder != null) + { + // "Replace" overwrites the existing book's folder in place rather than making a + // new folder and deleting the old one. This keeps the folder name and (in a Team + // Collection) the checkout status, so it stays the same checked-out repo book; see + // the Replace case above. + // + // If the book we're about to overwrite is the current selection, deselect it first + // so its files aren't locked (e.g. open in a preview) while we swap the folder. + if (_bookSelection.CurrentSelection?.FolderPath == replaceTargetFolder) + SelectBookOnUiThread(null); + destFolder = ReplaceBookInPlaceKeepingTeamStatus( + tempFolder, + htmlPath, + replaceTargetFolder + ); + importSucceeded = true; + // Remember the id this book arrived with so a later file in the same batch carrying + // the same id is caught as a within-batch duplicate above. + if (!string.IsNullOrEmpty(originalInstanceId)) + idsAlreadyImportedThisBatch?.Add(originalInstanceId); + return destFolder; + } + // Pick a folder name that doesn't collide with an existing book on disk, and make // the main htm file match it (Bloom's convention: folder name == book htm name). var destName = BookStorage.GetUniqueBookFolderName( @@ -536,47 +561,14 @@ out var instanceId if (!string.IsNullOrEmpty(originalInstanceId)) idsAlreadyImportedThisBatch?.Add(originalInstanceId); - // The imported book is now safely in place; for the Replace case it is finally safe - // to recycle the book it duplicates. The import itself has already succeeded, so a - // failure recycling the old book (locked folder, recycle bin unavailable on a network - // drive, etc.) is reported as its own minor problem rather than being allowed to - // bubble up as an "import failed" error — which would both mislead the user and, since - // this method would then throw instead of returning destFolder, keep the - // successfully-imported book from being reloaded into view. - if (folderToRecycleAfterImport != null) - { - try - { - if ( - _bookSelection.CurrentSelection?.FolderPath - == folderToRecycleAfterImport - ) - SelectBookOnUiThread(null); - PathUtilities.DeleteToRecycleBin(folderToRecycleAfterImport); - editable.HandleBookDeletedFromCollection(folderToRecycleAfterImport); - } - catch (Exception e) - { - NonFatalProblem.Report( - ModalIf.All, - PassiveIf.None, - shortUserLevelMessage: string.Format( - "The book was imported, but Bloom could not remove the existing copy it replaced. You may need to delete the older copy of \"{0}\" manually.", - Path.GetFileName(destFolder) - ), - moreDetails: null, - exception: e - ); - } - } return destFolder; } catch { // If we failed partway through creating the destination folder, don't leave a - // half-imported book behind in the collection. But once the import has succeeded - // (the book is fully moved into place), a later failure such as recycling the - // replaced duplicate must not delete the book we just imported. + // half-imported book behind in the collection. (The Replace/overwrite path handles + // its own rollback in ReplaceBookInPlaceKeepingTeamStatus and only sets destFolder + // once it has fully succeeded, so this never deletes a restored original.) if (!importSucceeded && destFolder != null && Directory.Exists(destFolder)) SIL.IO.RobustIO.DeleteDirectoryAndContents(destFolder); throw; @@ -588,6 +580,90 @@ out var instanceId } } + /// + /// Replaces the book in with the freshly-extracted imported + /// book in , overwriting its content in place. The target folder + /// keeps its name and its Team Collection files (checkout status etc.), so in a Team + /// Collection it stays the same checked-out book — the new content is pushed to the shared + /// repo on the next check-in — rather than becoming a new local book while the old one + /// resurrects from the repo. The imported book already carries the same bookInstanceId + /// (Replace keeps the id). The swap goes through a set-aside backup so a mid-swap failure + /// restores the original rather than losing the book. + /// + /// the target folder path, now holding the imported book + private string ReplaceBookInPlaceKeepingTeamStatus( + string tempFolder, + string htmlPath, + string targetFolder + ) + { + // Bloom's convention is folder name == main htm name; keep the target's existing folder + // name (which is what maps it to its Team Collection repo entry), so rename the incoming + // htm to match it. + var targetName = Path.GetFileName(targetFolder); + var renamedHtmlPath = Path.Combine(tempFolder, targetName + ".htm"); + if (!string.Equals(htmlPath, renamedHtmlPath, StringComparison.OrdinalIgnoreCase)) + RobustFile.Move(htmlPath, renamedHtmlPath); + + // Swap on the same volume: move the existing book aside, move the imported content into + // its place, then carry the Team Collection files over from the original so the book stays + // checked out. If anything fails mid-swap, restore the original and rethrow so we never + // lose the book. (Outside a Team Collection there are no TC files, so that step does + // nothing.) + // The set-aside name is dot-prefixed so that, if final cleanup ever fails and it lingers, + // the collection scan ignores it (it only skips dot-prefixed folders) rather than picking + // it up as a book — which, since it holds the old content with the same bookInstanceId, + // would be a duplicate-id book. + var backupFolder = Path.Combine( + Path.GetDirectoryName(targetFolder), + "." + Path.GetFileName(targetFolder) + ".replacing-" + Guid.NewGuid() + ); + SIL.IO.RobustIO.MoveDirectory(targetFolder, backupFolder); + try + { + SIL.IO.RobustIO.MoveDirectory(tempFolder, targetFolder); + var teamFiles = new List(); + Bloom.TeamCollection.TeamCollection.AddTCSpecificFiles(backupFolder, teamFiles); + foreach (var teamFile in teamFiles) + RobustFile.Copy( + teamFile, + Path.Combine(targetFolder, Path.GetFileName(teamFile)), + true + ); + } + catch + { + if (Directory.Exists(targetFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(targetFolder); + SIL.IO.RobustIO.MoveDirectory(backupFolder, targetFolder); + throw; + } + // The replace has fully succeeded; deleting the set-aside old content is just cleanup. If + // it fails (locked files, flaky network/removable drive, AV), don't let that turn a + // successful replace into an "import failed" error and keep the replaced book from being + // reloaded into view — report it as a minor problem instead. (The backup is dot-prefixed, + // so even if it lingers the collection scan ignores it.) + try + { + SIL.IO.RobustIO.DeleteDirectoryAndContents(backupFolder); + } + catch (Exception e) + { + NonFatalProblem.Report( + ModalIf.All, + PassiveIf.None, + shortUserLevelMessage: string.Format( + "The book \"{0}\" was replaced, but Bloom could not remove a temporary copy of the old version. You may delete the folder \"{1}\" manually.", + Path.GetFileName(targetFolder), + Path.GetFileName(backupFolder) + ), + moreDetails: null, + exception: e + ); + } + return targetFolder; + } + /// /// Validates a .bloomSource file, extracts it to a fresh temp folder, removes the stray /// .bloomCollection settings file and any Team-Collection status files, and confirms the @@ -611,10 +687,13 @@ out string instanceId // temp folder is frequently on a different drive than the user's collection (e.g. %TEMP% // on C: but the collection on D: or a removable/network drive), which would make every // edit-mode import fail. Staging in the collection's parent guarantees a same-volume move. + // If the collection is at a filesystem root (e.g. "D:\"), it has no parent directory, so + // stage inside the collection folder itself instead — still same-volume, and never %TEMP% + // (which could be on another drive and reintroduce the cross-volume failure). // The name is dot-prefixed so it is ignored by the collection's book scan if it ever ends // up inside a scanned folder. var collectionDir = TheOneEditableCollection.PathToDirectory; - var stagingParent = Directory.GetParent(collectionDir)?.FullName ?? Path.GetTempPath(); + var stagingParent = Directory.GetParent(collectionDir)?.FullName ?? collectionDir; var tempFolder = Path.Combine(stagingParent, ".BloomImport-" + Guid.NewGuid()); try { @@ -661,11 +740,17 @@ internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath) if (newBook == null) return null; // e.g. the source had a configuration dialog and the user cancelled - // Mirror the normal "make a book from this source" flow (CreateFromSourceBook): - // defer the "Created" history event via a pending marker so it captures the title the - // user eventually gives the book (and suppresses spurious "Renamed" events meanwhile), - // and add the new book to the collection in memory rather than reloading and looking it + // Add the new book to the collection in memory rather than reloading and looking it // up again (which was the source of a silent not-found fallback). + // + // The normal "make a book from this source" flow (CreateFromSourceBook) defers the + // "Created" history event via a pending marker and flushes it when the book is later + // deselected, so it can capture a title the user types afterward. That does not work + // for a batch import: only the last-imported book is ever selected, so every earlier + // book's pending event would be lost once its Book object is discarded (a freshly + // rebuilt Book has no pending marker). An imported book already has its title, so we + // set the marker and flush it immediately, giving every imported derivative its own + // "Created" history entry. newBook.PendingCreationSource = Path.GetFileName(sourcePath); newBook.PendingCreationSourceTitle = newBook.BookInfo.GetTitleForLanguage( newBook.BookData.Language1Tag @@ -674,6 +759,7 @@ internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath) newBook.BringBookUpToDate(new NullProgress(), false); TheOneEditableCollection.AddBookInfo(newBook.BookInfo); + newBook.RecordPendingCreatedHistoryEvent(); return newBook; } finally diff --git a/src/BloomTests/CollectionTab/CollectionModelTests.cs b/src/BloomTests/CollectionTab/CollectionModelTests.cs index 6368076ab65b..383f24c4c208 100644 --- a/src/BloomTests/CollectionTab/CollectionModelTests.cs +++ b/src/BloomTests/CollectionTab/CollectionModelTests.cs @@ -230,10 +230,13 @@ public void ImportBloomSource_DuplicateId_AddCopy_MakesIndependentCopyWithNewId( } [Test] - public void ImportBloomSource_DuplicateId_Replace_RecyclesExistingBook() + public void ImportBloomSource_DuplicateId_Replace_OverwritesInPlace() { var sharedId = Guid.NewGuid().ToString(); var existingFolder = MakeExistingBookInCollection("Moon", sharedId); + // A file that exists only in the old book, to prove the folder is overwritten, not merged. + File.WriteAllText(Path.Combine(existingFolder, "old-only.txt"), "old"); + var foldersBefore = Directory.GetDirectories(_collection.Path).Length; var src = MakeBloomSourceFile("Moon", sharedId); var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( @@ -241,15 +244,74 @@ public void ImportBloomSource_DuplicateId_Replace_RecyclesExistingBook() title => CollectionModel.ImportDuplicateChoice.Replace ); - Assert.That(dest, Is.Not.Null); + // Replace overwrites the existing book in place: same folder, no second folder, the old + // content is gone, the imported content is present, and the id is unchanged. + Assert.That( + dest, + Is.EqualTo(existingFolder), + "replace should overwrite the existing folder in place" + ); Assert.That( Directory.Exists(existingFolder), + Is.True, + "the existing folder should be kept, not recycled" + ); + Assert.That( + Directory.GetDirectories(_collection.Path).Length, + Is.EqualTo(foldersBefore), + "replace should not create a second book folder" + ); + Assert.That( + File.Exists(Path.Combine(existingFolder, "old-only.txt")), Is.False, - "the existing book should have been recycled" + "the old book's content should have been replaced, not merged" + ); + Assert.That( + File.Exists(Path.Combine(existingFolder, "book.css")), + Is.True, + "the imported book's content should now be in the folder" ); Assert.That(BookMetaData.FromFolder(dest).Id, Is.EqualTo(sharedId)); } + [Test] + public void ImportBloomSource_DuplicateId_Replace_PreservesTeamCollectionStatus() + { + var sharedId = Guid.NewGuid().ToString(); + var existingFolder = MakeExistingBookInCollection("Moon", sharedId); + // Simulate a checked-out Team Collection book by giving it a TeamCollection.status file. + var statusPath = Path.Combine(existingFolder, "TeamCollection.status"); + File.WriteAllText(statusPath, "{\"lockedBy\":\"me@example.com\"}"); + var src = MakeBloomSourceFile("Moon", sharedId); + + var dest = _testCollectionModel.ImportBloomSourceFileToCollectionFolder( + src, + title => CollectionModel.ImportDuplicateChoice.Replace + ); + + Assert.That(dest, Is.EqualTo(existingFolder)); + // The checkout status must survive the replace, so the book stays the same checked-out + // repo book rather than becoming a new local book (which would let the old one resurrect + // from the shared repo). + var newStatusPath = Path.Combine(existingFolder, "TeamCollection.status"); + Assert.That( + File.Exists(newStatusPath), + Is.True, + "the Team Collection status (checkout) file must be preserved across a replace" + ); + Assert.That( + File.ReadAllText(newStatusPath), + Does.Contain("lockedBy"), + "the preserved status file should keep its original content" + ); + // Sanity check that the content really was replaced by the import. + Assert.That( + File.Exists(Path.Combine(existingFolder, "book.css")), + Is.True, + "the imported book's content should now be in the folder" + ); + } + [Test] public void ImportBloomSource_DuplicateId_Cancel_ImportsNothing() { @@ -327,10 +389,15 @@ public void ImportBloomSource_SameIdDifferentTitle_IsTreatedAsDuplicate() "a book with a matching id but a different title should still be detected as a duplicate" ); Assert.That(dest, Is.Not.Null); + Assert.That( + dest, + Is.EqualTo(existingFolder), + "Replace should overwrite the existing (same-id) book in place, keeping its folder" + ); Assert.That( Directory.Exists(existingFolder), - Is.False, - "Replace should have recycled the existing (same-id) book" + Is.True, + "Replace overwrites in place, so the existing folder is kept" ); } From be36527aeb1924a7fa7ff95860b9d4a0a45ea805 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 7 Jul 2026 10:52:02 -0600 Subject: [PATCH 14/17] Move .bloomSource import strings to medium priority (BL-16502) Relocates the CollectionTab.ImportBloomSource* translation units from Bloom.xlf (high priority) to BloomMediumPriority.xlf. This commit lands a pre-existing uncommitted working-tree change that was already on the branch; the content was authored by the developer, not generated here. --- DistFiles/localization/en/Bloom.xlf | 65 ------------------- .../localization/en/BloomMediumPriority.xlf | 65 +++++++++++++++++++ 2 files changed, 65 insertions(+), 65 deletions(-) diff --git a/DistFiles/localization/en/Bloom.xlf b/DistFiles/localization/en/Bloom.xlf index d3b348df95f8..d656c02980ad 100644 --- a/DistFiles/localization/en/Bloom.xlf +++ b/DistFiles/localization/en/Bloom.xlf @@ -792,71 +792,6 @@ Shell Books are books that are meant to be translated. ID: CollectionTab.MakeBloomPackOfShellBooks - - Import .bloomSource File(s) - Menu command (and dialog title) that imports one or more books from .bloomSource files into the current collection. Keep ".bloomSource" unchanged; it is a file extension. - ID: CollectionTab.ImportBloomSource - - - Import books - Title of the dialogs shown while importing .bloomSource file(s): the one that asks whether to import for editing or as derivatives, and the one that asks how to handle books already in the collection. - ID: CollectionTab.ImportBloomSource.ChoiceTitle - - - Import for editing - Radio-button choice in the import dialog: import the book(s) to edit them directly. - ID: CollectionTab.ImportBloomSource.EditChoice - - - Preserve everything. - Explanation shown under the "Import for editing" radio-button choice in the import dialog. - ID: CollectionTab.ImportBloomSource.EditChoiceDescription - - - Import as derivatives — new books based on the originals - Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). - ID: CollectionTab.ImportBloomSource.DerivativeChoice - - - Use a new book ID & make room for new copyright and credits while preserving those of the original. - Explanation shown under the "Import as derivatives" radio-button choice in the import dialog. - ID: CollectionTab.ImportBloomSource.DerivativeChoiceDescription - - - Some of these books are already in this collection. - Shown once when importing .bloomSource file(s), if any of the books is already in the collection. - ID: CollectionTab.ImportBloomSource.DuplicatePrompt - - - Replace the existing books - Radio-button choice in the import duplicate prompt: replace the existing book(s) with the imported one(s). - ID: CollectionTab.ImportBloomSource.Replace - - - The current copies will be overwritten. - Secondary line under the "Replace the existing books" choice in the import duplicate prompt. - ID: CollectionTab.ImportBloomSource.ReplaceDescription - - - All books must be checked out - Secondary line under the "Replace the existing books" choice in the import duplicate prompt, shown (and the choice disabled) when the collection is a Team Collection and one or more of the books being replaced is not checked out. - ID: CollectionTab.ImportBloomSource.ReplaceNeedsCheckout - - - Add them as new copies - Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. - ID: CollectionTab.ImportBloomSource.AddCopy - - - The existing books stay as they are. - Secondary line under the "Add them as new copies" choice in the import duplicate prompt. - ID: CollectionTab.ImportBloomSource.AddCopyDescription - - - Importing Books - Title of the progress dialog shown while .bloomSource file(s) are being imported. - ID: CollectionTab.ImportBloomSource.Importing - Make a book using this source ID: CollectionTab.MakeBookUsingThisTemplate diff --git a/DistFiles/localization/en/BloomMediumPriority.xlf b/DistFiles/localization/en/BloomMediumPriority.xlf index 059ac20d08f2..7930b4ccac2f 100644 --- a/DistFiles/localization/en/BloomMediumPriority.xlf +++ b/DistFiles/localization/en/BloomMediumPriority.xlf @@ -739,6 +739,71 @@ Problem CollectionTab.OnBlorgBadge.Problem + + Import .bloomSource File(s) + Menu command (and dialog title) that imports one or more books from .bloomSource files into the current collection. Keep ".bloomSource" unchanged; it is a file extension. + ID: CollectionTab.ImportBloomSource + + + Import books + Title of the dialogs shown while importing .bloomSource file(s): the one that asks whether to import for editing or as derivatives, and the one that asks how to handle books already in the collection. + ID: CollectionTab.ImportBloomSource.ChoiceTitle + + + Import for editing + Radio-button choice in the import dialog: import the book(s) to edit them directly. + ID: CollectionTab.ImportBloomSource.EditChoice + + + Preserve everything. + Explanation shown under the "Import for editing" radio-button choice in the import dialog. + ID: CollectionTab.ImportBloomSource.EditChoiceDescription + + + Import as derivatives — new books based on the originals + Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). + ID: CollectionTab.ImportBloomSource.DerivativeChoice + + + Use a new book ID & make room for new copyright and credits while preserving those of the original. + Explanation shown under the "Import as derivatives" radio-button choice in the import dialog. + ID: CollectionTab.ImportBloomSource.DerivativeChoiceDescription + + + Some of these books are already in this collection. + Shown once when importing .bloomSource file(s), if any of the books is already in the collection. + ID: CollectionTab.ImportBloomSource.DuplicatePrompt + + + Replace the existing books + Radio-button choice in the import duplicate prompt: replace the existing book(s) with the imported one(s). + ID: CollectionTab.ImportBloomSource.Replace + + + The current copies will be overwritten. + Secondary line under the "Replace the existing books" choice in the import duplicate prompt. + ID: CollectionTab.ImportBloomSource.ReplaceDescription + + + All books must be checked out + Secondary line under the "Replace the existing books" choice in the import duplicate prompt, shown (and the choice disabled) when the collection is a Team Collection and one or more of the books being replaced is not checked out. + ID: CollectionTab.ImportBloomSource.ReplaceNeedsCheckout + + + Add them as new copies + Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. + ID: CollectionTab.ImportBloomSource.AddCopy + + + The existing books stay as they are. + Secondary line under the "Add them as new copies" choice in the import duplicate prompt. + ID: CollectionTab.ImportBloomSource.AddCopyDescription + + + Importing Books + Title of the progress dialog shown while .bloomSource file(s) are being imported. + ID: CollectionTab.ImportBloomSource.Importing + Locked by {0} Branding BookSettings.LockedByBranding From cd6dc01eccd27c0415fa7d5323767c8fd53cc000 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 7 Jul 2026 10:52:27 -0600 Subject: [PATCH 15/17] Extract .bloomSource import to its own file and make its API stateless (BL-16502) Two refactors of the .bloomSource import feature, no behavior change: 1. Factor the import code (~740 lines) out of the already-bloated CollectionModel.cs into a new partial-class file, CollectionModel.BloomSourceImport.cs: the file picker, the duplicate pre-flight check, and the import itself (edit / derivative / replace / add-copy). Saving/exporting a book AS a .bloomSource stays in CollectionModel.cs. Also drops two now-unused usings there. 2. Remove the server-side state the feature carried between three API calls. Previously the chosen file paths lived in a _bloomSourceFilesToImport field across chooseBloomSourceFilesToImport -> bloomSourceImportDuplicateInfo -> importBloomSource. Now the backend is stateless commands that the front-end drives: - ChooseBloomSourceFilesToImport() returns the chosen string[] paths. - The choose endpoint returns { files, anyDuplicates, canReplace } in one reply, so the separate bloomSourceImportDuplicateInfo endpoint is gone. - importBloomSource reads the paths from the POST body; the front-end holds them in React state and passes them back. The file-system mechanics (zip, validation, folder moves, Team Collection handling, rollback) stay in C# where they belong; only the orchestration state moved to the front-end. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../collectionsTab/CollectionsTabPane.tsx | 57 +- .../CollectionModel.BloomSourceImport.cs | 744 ++++++++++++++++++ src/BloomExe/CollectionTab/CollectionModel.cs | 740 +---------------- src/BloomExe/web/controllers/CollectionApi.cs | 56 +- 4 files changed, 803 insertions(+), 794 deletions(-) create mode 100644 src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs diff --git a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx index 92a391147bd4..6bda79035bbc 100644 --- a/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx +++ b/src/BloomBrowserUI/collectionsTab/CollectionsTabPane.tsx @@ -1,6 +1,6 @@ import { css } from "@emotion/react"; import * as React from "react"; -import { get, post, postString } from "../utils/bloomApi"; +import { get, post, postData, postString } from "../utils/bloomApi"; import { BooksOfCollection, IBookInfo } from "./BooksOfCollection"; import { makeMenuItems, MenuItemSpec } from "./menuHelpers"; import { Transition } from "react-transition-group"; @@ -199,11 +199,14 @@ export const CollectionsTabPane: React.FunctionComponent = () => { | undefined >(); - // The .bloomSource import decision. The file(s) are chosen first (via C#); then we check (via - // C#) whether any chosen book is already in the collection and, based on that, show the user - // exactly one dialog for the whole batch. If none are present, Dialog 1 asks whether to edit + // The .bloomSource import decision. The file(s) are chosen first (via C#), which in the same + // reply tells us whether any chosen book is already in the collection; based on that we show the + // user exactly one dialog for the whole batch. If none are present, Dialog 1 asks whether to edit // the book(s) or make derivatives. If any are present, Dialog 2 asks whether to replace the - // existing book(s) or add the imported ones as new copies. The user never sees both. + // existing book(s) or add the imported ones as new copies. The user never sees both. We hold the + // chosen paths here and pass them back to the import call; the backend keeps no state between + // calls. + const [importFiles, setImportFiles] = useState([]); const [showImportSourceChoiceDialog, setShowImportSourceChoiceDialog] = useState(false); const [showImportDuplicateDialog, setShowImportDuplicateDialog] = @@ -257,24 +260,6 @@ export const CollectionsTabPane: React.FunctionComponent = () => { "CollectionTab.ImportBloomSource.AddCopyDescription", ); - // Decide which single dialog to show after the file(s) have been chosen: Dialog 2 (replace vs. - // add-copy) if any chosen book is already in the collection, otherwise Dialog 1 (edit vs. - // derivative). - // - // Note that when a chosen book is already in the collection we deliberately do NOT offer the - // edit-vs-derivative choice: the duplicate dialog always imports in edit mode, so you can't - // make a derivative of a book that's already in your collection. We couldn't think of a - // plausible scenario where you have a book and a true derivative of it in the same collection, - // so this path isn't worth the extra complexity. Easy to reverse if that turns out to be wrong. - const chooseImportDialog = () => { - post("collections/bloomSourceImportDuplicateInfo", (r) => { - if (r.data.anyDuplicates) { - setImportCanReplace(r.data.canReplace); - setShowImportDuplicateDialog(true); - } else setShowImportSourceChoiceDialog(true); - }); - }; - // Dialog 1 (shown only when no chosen book is already in the collection): the user chose to // edit the book(s) as-is or make derivatives (or cancelled). Neither path has a duplicate to // resolve, so import immediately. @@ -282,7 +267,7 @@ export const CollectionsTabPane: React.FunctionComponent = () => { setShowImportSourceChoiceDialog(false); if (!choice) return; const mode = choice === "derivative" ? "derivative" : "edit"; - post(`collections/importBloomSource?mode=${mode}`); + postData(`collections/importBloomSource?mode=${mode}`, importFiles); }; // Dialog 2 (shown only when at least one chosen book is already in the collection): the user @@ -291,7 +276,10 @@ export const CollectionsTabPane: React.FunctionComponent = () => { const handleImportDuplicateChoice = (choice?: string) => { setShowImportDuplicateDialog(false); if (!choice) return; - post(`collections/importBloomSource?mode=edit&onDuplicate=${choice}`); + postData( + `collections/importBloomSource?mode=edit&onDuplicate=${choice}`, + importFiles, + ); }; const setAdjustedContextMenuPoint = (x: number, y: number) => { @@ -380,10 +368,23 @@ export const CollectionsTabPane: React.FunctionComponent = () => { icon: , onClick: () => { handleClose(); - // Choose the file(s) first; then show the one dialog that fits, based on whether - // any chosen book is already in the collection. + // Choose the file(s) first; the reply tells us both the chosen paths and whether any + // chosen book is already in the collection, so we can show the one dialog that fits. + // + // Note that when a chosen book is already in the collection we deliberately do NOT + // offer the edit-vs-derivative choice: the duplicate dialog always imports in edit + // mode, so you can't make a derivative of a book that's already in your collection. We + // couldn't think of a plausible scenario where you have a book and a true derivative + // of it in the same collection, so this path isn't worth the extra complexity. Easy + // to reverse if that turns out to be wrong. post("collections/chooseBloomSourceFilesToImport", (r) => { - if (r.data === true) chooseImportDialog(); + const files: string[] = r.data.files; + if (!files || files.length === 0) return; // the user cancelled the picker + setImportFiles(files); + if (r.data.anyDuplicates) { + setImportCanReplace(r.data.canReplace); + setShowImportDuplicateDialog(true); + } else setShowImportSourceChoiceDialog(true); }); }, addEllipsis: true, diff --git a/src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs b/src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs new file mode 100644 index 000000000000..213350446dc2 --- /dev/null +++ b/src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs @@ -0,0 +1,744 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Threading.Tasks; +using System.Windows.Forms; +using Bloom.Book; +using Bloom.History; +using Bloom.MiscUI; +using Bloom.Utils; +using Bloom.web; +using L10NSharp; +using SIL.IO; +using SIL.Progress; +using SIL.Reporting; + +namespace Bloom.CollectionTab +{ + // The .bloomSource import feature lives in this partial-class file so it doesn't bloat + // CollectionModel.cs. It covers choosing the file(s), the pre-flight duplicate check the + // collection screen uses to pick which dialog to show, and the import itself (edit-as-is, + // make-derivative, replace-existing, or add-as-copy). Saving/exporting a book AS a .bloomSource + // is a separate concern and stays in CollectionModel.cs (SaveAsBloomSourceFile). + // + // These methods are stateless commands: the front-end owns the flow, so the chosen file paths + // are returned to it (by ChooseBloomSourceFilesToImport) and passed back in for the duplicate + // check and the import, rather than being remembered here between the several API calls. + public partial class CollectionModel + { + /// + /// What the user chose to do when an imported .bloomSource book is already present + /// (same bookInstanceId) in the editable collection. + /// + internal enum ImportDuplicateChoice + { + Replace, + AddCopy, + Cancel, + } + + /// + /// A problem with an imported .bloomSource file that we can explain to the user + /// (e.g. corrupt file, wrong kind of file, no book inside). The Message is shown + /// directly to the user, so it should be friendly and not require a bug report. + /// + internal class BloomSourceImportException : Exception + { + public BloomSourceImportException(string userMessage) + : base(userMessage) { } + } + + /// + /// Prompts the user to pick one or more .bloomSource files and returns the chosen paths (an + /// empty array if the user cancelled). The caller (the collection screen) holds onto these and + /// passes them back to the duplicate check and to . This + /// is separate from the import itself so the collection screen can, once the files are chosen, + /// show the single import dialog appropriate to the batch (edit-vs-derivative when none are + /// already present, replace-vs-add-copy when any are). + /// + public string[] ChooseBloomSourceFilesToImport() + { + using (var dlg = new BloomOpenFileDialog()) + { + dlg.Multiselect = true; + dlg.CheckFileExists = true; + dlg.Title = LocalizationManager.GetString( + "CollectionTab.ImportBloomSource", + "Import .bloomSource File(s)" + ); + dlg.Filter = + "Bloom Source files (*.bloomSource)|*.bloomSource|Team Collection books (*.bloom)|*.bloom|All files (*.*)|*.*"; + if (dlg.ShowDialog() != DialogResult.OK) + return new string[0]; + return dlg.FileNames; + } + } + + /// + /// Runs behind the embedded progress dialog on a + /// background thread, so a large batch neither freezes the collection screen nor lets the + /// awaiting browser request time out. The front-end already hosts the "collectionTab" + /// EmbeddedProgressDialog, so it needs no extra wiring. + /// + public async Task ImportBloomSourceFilesWithProgressAsync( + string[] paths, + bool makeDerivatives, + bool replaceExistingDuplicates + ) + { + await BrowserProgressDialog.DoWorkWithProgressDialogAsync( + _webSocketServer, + (progress, worker) => + { + ImportBloomSourceFiles( + paths, + makeDerivatives, + replaceExistingDuplicates, + progress + ); + return Task.FromResult(false); // false => close the dialog when we finish + }, + "collectionTab", + LocalizationManager.GetString( + "CollectionTab.ImportBloomSource.Importing", + "Importing Books" + ), + showCancelButton: false + ); + } + + /// + /// Imports the .bloomSource files at (the ones the front-end got + /// from ) into the current editable collection. + /// The user has already made the choice (in the collection screen) that applies to the whole + /// batch: when is true, each imported book is used to make + /// a new derivative book (otherwise each is imported as an editable book); and, for the edit + /// case, when is true any book already in the + /// collection is replaced (otherwise it is added as a numbered copy). The last successfully + /// imported book is selected when done. + /// + public void ImportBloomSourceFiles( + string[] paths, + bool makeDerivatives, + bool replaceExistingDuplicates, + IWebSocketProgress progress = null + ) + { + if (paths == null || paths.Length == 0) + return; + + // The user has already chosen, once for the whole batch, what to do when an imported + // book is already in the collection (only relevant when editing, not making derivatives). + var duplicateChoice = replaceExistingDuplicates + ? ImportDuplicateChoice.Replace + : ImportDuplicateChoice.AddCopy; + + Book.Book lastImported = null; + // For the edit path we move every book into place first and reload the collection just + // once at the end, instead of reloading (which rescans the whole collection from disk) + // once per file. Each entry pairs the destination folder with its source file name so we + // can record an accurate "Imported from" history event after that single reload. + var importedEditFolders = new List<(string destFolder, string sourceName)>(); + // Because the collection isn't reloaded between files, the per-file duplicate check can't + // see books imported earlier in this same batch. Track their ids here so two files that + // share a bookInstanceId don't both land with that id (which would defeat the whole point + // of duplicate handling). + var idsImportedThisBatch = new HashSet(); + foreach (var path in paths) + { + try + { + progress?.MessageWithoutLocalizing($"Importing {Path.GetFileName(path)}..."); + if (makeDerivatives) + { + // The derivative path builds its Book in memory and adds it to the collection + // itself, so it needs no reload. + var book = MakeDerivativeFromBloomSourceFile(path); + if (book != null) + lastImported = book; + } + else + { + var destFolder = ImportBloomSourceFileToCollectionFolder( + path, + _ => duplicateChoice, + idsImportedThisBatch + ); + if (destFolder != null) + importedEditFolders.Add((destFolder, Path.GetFileName(path))); + } + } + catch (BloomSourceImportException e) + { + // A problem we can explain to the user; no bug report needed. + ErrorReport.NotifyUserOfProblem( + "{0}\r\n\r\n{1}", + Path.GetFileName(path), + e.Message + ); + } + catch (Exception e) + { + // Something unexpected; let the user report it. + NonFatalProblem.Report( + ModalIf.All, + PassiveIf.None, + shortUserLevelMessage: string.Format( + "Bloom was not able to import \"{0}\".", + Path.GetFileName(path) + ), + moreDetails: null, + exception: e + ); + } + } + + // Now that every edit-import's folder is in place, reload the collection a single time + // and turn each imported folder into a Book with its Created history event. + if (importedEditFolders.Count > 0) + { + ReloadEditableCollection(); + foreach (var (destFolder, sourceName) in importedEditFolders) + { + var newInfo = TheOneEditableCollection + .GetBookInfos() + .FirstOrDefault(i => i.FolderPath == destFolder); + if (newInfo == null) + { + // Should not happen after a successful move; surface it but don't abort the + // rest of the batch. + NonFatalProblem.Report( + ModalIf.All, + PassiveIf.None, + shortUserLevelMessage: string.Format( + "Bloom imported \"{0}\" but could not find it in the collection afterward.", + sourceName + ) + ); + continue; + } + var newBook = GetBookFromBookInfo(newInfo); + BookHistory.AddEvent( + newBook, + BookHistoryEventType.Created, + $"Imported from \"{sourceName}\"" + ); + lastImported = newBook; + } + } + + if (lastImported != null) + SelectBookOnUiThread(lastImported); + } + + /// + /// Selects a book, marshaling to the UI thread when necessary. Import can run on a background + /// thread (behind the progress dialog), but SelectBook raises SelectionChanged synchronously + /// to WinForms views, so it must happen on the UI thread. When no window is open (e.g. unit + /// tests) this just runs inline. + /// + private void SelectBookOnUiThread(Book.Book book) + { + var form = Shell.GetShellOrOtherOpenForm(); + if (form != null && form.InvokeRequired) + form.Invoke((Action)(() => _bookSelection.SelectBook(book))); + else + _bookSelection.SelectBook(book); + } + + /// + /// Does the file-system part of importing a .bloomSource: validates it, extracts it, removes + /// the stray .bloomCollection and any Team-Collection status files, handles the + /// already-in-collection case (Replace/Add-a-copy/Cancel), and moves the book into the + /// editable collection folder. Returns the destination folder, or null if the user cancelled. + /// This is separated from the Book-level work so it can be unit tested without loading a Book. + /// + /// Ids of books already imported earlier in this + /// same batch. Because the collection is reloaded only once, after the whole batch, a book + /// whose id is in this set isn't yet visible to the in-collection check above; we treat it as + /// a within-batch duplicate and give the incoming book a fresh id so the two don't collide. + /// When null (e.g. single-file tests), within-batch tracking is skipped. + internal string ImportBloomSourceFileToCollectionFolder( + string sourcePath, + Func resolveDuplicate, + ISet idsAlreadyImportedThisBatch = null + ) + { + var editable = TheOneEditableCollection; + var tempFolder = ExtractAndPrepareBloomSourceToTemp( + sourcePath, + out var htmlPath, + out var instanceId + ); + // The id this book arrived with. We record it (once the import succeeds, below) so that a + // later file in the same batch carrying the same id is recognized as a duplicate even + // though the collection is only reloaded once, after the whole batch. + var originalInstanceId = instanceId; + string destFolder = null; + string replaceTargetFolder = null; + // Once the book has been moved into the collection the import has succeeded; a failure + // after that point (e.g. recycling the replaced duplicate) must not delete it. + var importSucceeded = false; + // "Add a copy" produces an independent copy exactly like the Duplicate Book command: + // a new id and the " - Copy-" folder convention (the caption is left alone, + // just as Duplicate leaves it). + var folderSeparator = "-"; + try + { + var baseName = Path.GetFileNameWithoutExtension(htmlPath); + + var existing = string.IsNullOrEmpty(instanceId) + ? null + : editable.GetBookInfos().FirstOrDefault(b => b.Id == instanceId); + // A book with this id isn't in the collection, but an earlier file in this same batch + // already brought it in (the collection is only reloaded once, after the batch, so it + // isn't visible above yet). Replacing a book from the same batch is meaningless, so we + // always make this one an independent copy so the two don't end up sharing an id. + var isDuplicateWithinBatch = + existing == null + && !string.IsNullOrEmpty(instanceId) + && idsAlreadyImportedThisBatch != null + && idsAlreadyImportedThisBatch.Contains(instanceId); + + // Whether to turn the incoming book into an independent copy: a new id (so it can't + // collide) plus the " - Copy-" folder convention the Duplicate command uses. + var makeIndependentCopy = false; + if (existing != null) + { + switch (resolveDuplicate(existing.Title)) + { + case ImportDuplicateChoice.Cancel: + return null; + case ImportDuplicateChoice.Replace: + // Replacing overwrites the existing book in place, so we must be allowed + // to modify it: in a Team Collection that means it has to be checked out + // here. The collection screen already disables "Replace" when any + // duplicate is not (see AllBloomSourceDuplicatesAreReplaceable), but + // guard here too so we can never overwrite a book the user hasn't checked + // out. + if (!existing.IsSaveable) + throw new ApplicationException( + $"Cannot replace \"{existing.Title}\" because it is not checked out of the Team Collection." + ); + // Overwrite this book's folder in place (below), keeping its folder name + // and — in a Team Collection — its checkout status, so it stays the same + // checked-out repo book (the new content is pushed to the shared repo on + // the next check-in) rather than becoming a new local book while the old + // one resurrects from the repo. The imported book keeps the same id. + replaceTargetFolder = existing.FolderPath; + break; + case ImportDuplicateChoice.AddCopy: + makeIndependentCopy = true; + break; + } + } + else if (isDuplicateWithinBatch) + { + makeIndependentCopy = true; + } + + if (makeIndependentCopy) + { + folderSeparator = " - Copy-"; + instanceId = Guid.NewGuid().ToString(); + var meta = BookMetaData.FromFolder(tempFolder); + if (meta != null) + { + meta.Id = instanceId; + meta.WriteToFolder(tempFolder); + } + } + + if (replaceTargetFolder != null) + { + // "Replace" overwrites the existing book's folder in place rather than making a + // new folder and deleting the old one. This keeps the folder name and (in a Team + // Collection) the checkout status, so it stays the same checked-out repo book; see + // the Replace case above. + // + // If the book we're about to overwrite is the current selection, deselect it first + // so its files aren't locked (e.g. open in a preview) while we swap the folder. + if (_bookSelection.CurrentSelection?.FolderPath == replaceTargetFolder) + SelectBookOnUiThread(null); + destFolder = ReplaceBookInPlaceKeepingTeamStatus( + tempFolder, + htmlPath, + replaceTargetFolder + ); + importSucceeded = true; + // Remember the id this book arrived with so a later file in the same batch carrying + // the same id is caught as a within-batch duplicate above. + if (!string.IsNullOrEmpty(originalInstanceId)) + idsAlreadyImportedThisBatch?.Add(originalInstanceId); + return destFolder; + } + + // Pick a folder name that doesn't collide with an existing book on disk, and make + // the main htm file match it (Bloom's convention: folder name == book htm name). + var destName = BookStorage.GetUniqueBookFolderName( + editable.PathToDirectory, + BookStorage.SanitizeNameForFileSystem(baseName), + instanceId, + folderSeparator + ); + destFolder = Path.Combine(editable.PathToDirectory, destName); + var renamedHtmlPath = Path.Combine(tempFolder, destName + ".htm"); + if (!string.Equals(htmlPath, renamedHtmlPath, StringComparison.OrdinalIgnoreCase)) + RobustFile.Move(htmlPath, renamedHtmlPath); + SIL.IO.RobustIO.MoveDirectory(tempFolder, destFolder); + importSucceeded = true; + + // Remember the id this book arrived with so a later file in the same batch carrying + // the same id is caught as a within-batch duplicate above. + if (!string.IsNullOrEmpty(originalInstanceId)) + idsAlreadyImportedThisBatch?.Add(originalInstanceId); + + return destFolder; + } + catch + { + // If we failed partway through creating the destination folder, don't leave a + // half-imported book behind in the collection. (The Replace/overwrite path handles + // its own rollback in ReplaceBookInPlaceKeepingTeamStatus and only sets destFolder + // once it has fully succeeded, so this never deletes a restored original.) + if (!importSucceeded && destFolder != null && Directory.Exists(destFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(destFolder); + throw; + } + finally + { + if (Directory.Exists(tempFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); + } + } + + /// + /// Replaces the book in with the freshly-extracted imported + /// book in , overwriting its content in place. The target folder + /// keeps its name and its Team Collection files (checkout status etc.), so in a Team + /// Collection it stays the same checked-out book — the new content is pushed to the shared + /// repo on the next check-in — rather than becoming a new local book while the old one + /// resurrects from the repo. The imported book already carries the same bookInstanceId + /// (Replace keeps the id). The swap goes through a set-aside backup so a mid-swap failure + /// restores the original rather than losing the book. + /// + /// the target folder path, now holding the imported book + private string ReplaceBookInPlaceKeepingTeamStatus( + string tempFolder, + string htmlPath, + string targetFolder + ) + { + // Bloom's convention is folder name == main htm name; keep the target's existing folder + // name (which is what maps it to its Team Collection repo entry), so rename the incoming + // htm to match it. + var targetName = Path.GetFileName(targetFolder); + var renamedHtmlPath = Path.Combine(tempFolder, targetName + ".htm"); + if (!string.Equals(htmlPath, renamedHtmlPath, StringComparison.OrdinalIgnoreCase)) + RobustFile.Move(htmlPath, renamedHtmlPath); + + // Swap on the same volume: move the existing book aside, move the imported content into + // its place, then carry the Team Collection files over from the original so the book stays + // checked out. If anything fails mid-swap, restore the original and rethrow so we never + // lose the book. (Outside a Team Collection there are no TC files, so that step does + // nothing.) + // The set-aside name is dot-prefixed so that, if final cleanup ever fails and it lingers, + // the collection scan ignores it (it only skips dot-prefixed folders) rather than picking + // it up as a book — which, since it holds the old content with the same bookInstanceId, + // would be a duplicate-id book. + var backupFolder = Path.Combine( + Path.GetDirectoryName(targetFolder), + "." + Path.GetFileName(targetFolder) + ".replacing-" + Guid.NewGuid() + ); + SIL.IO.RobustIO.MoveDirectory(targetFolder, backupFolder); + try + { + SIL.IO.RobustIO.MoveDirectory(tempFolder, targetFolder); + var teamFiles = new List(); + Bloom.TeamCollection.TeamCollection.AddTCSpecificFiles(backupFolder, teamFiles); + foreach (var teamFile in teamFiles) + RobustFile.Copy( + teamFile, + Path.Combine(targetFolder, Path.GetFileName(teamFile)), + true + ); + } + catch + { + if (Directory.Exists(targetFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(targetFolder); + SIL.IO.RobustIO.MoveDirectory(backupFolder, targetFolder); + throw; + } + // The replace has fully succeeded; deleting the set-aside old content is just cleanup. If + // it fails (locked files, flaky network/removable drive, AV), don't let that turn a + // successful replace into an "import failed" error and keep the replaced book from being + // reloaded into view — report it as a minor problem instead. (The backup is dot-prefixed, + // so even if it lingers the collection scan ignores it.) + try + { + SIL.IO.RobustIO.DeleteDirectoryAndContents(backupFolder); + } + catch (Exception e) + { + NonFatalProblem.Report( + ModalIf.All, + PassiveIf.None, + shortUserLevelMessage: string.Format( + "The book \"{0}\" was replaced, but Bloom could not remove a temporary copy of the old version. You may delete the folder \"{1}\" manually.", + Path.GetFileName(targetFolder), + Path.GetFileName(backupFolder) + ), + moreDetails: null, + exception: e + ); + } + return targetFolder; + } + + /// + /// Validates a .bloomSource file, extracts it to a fresh temp folder, removes the stray + /// .bloomCollection settings file and any Team-Collection status files, and confirms the + /// folder contains a book. Returns the temp folder path (the caller owns deleting it), the + /// book's main htm path, and its bookInstanceId (may be null). On any failure the temp + /// folder is cleaned up and the exception is rethrown. + /// + private string ExtractAndPrepareBloomSourceToTemp( + string sourcePath, + out string htmlPath, + out string instanceId + ) + { + // A .bloomSource is a zip of a single book folder's *contents* (flat at the root), + // plus the collection's .bloomCollection settings file. Validate that shape and guard + // against malicious entries before we extract anything. + ValidateBloomSourceZip(sourcePath); + + // Extract onto the same volume as the collection. The book is later moved into the + // collection folder with a directory move, which cannot cross volumes; the system + // temp folder is frequently on a different drive than the user's collection (e.g. %TEMP% + // on C: but the collection on D: or a removable/network drive), which would make every + // edit-mode import fail. Staging in the collection's parent guarantees a same-volume move. + // If the collection is at a filesystem root (e.g. "D:\"), it has no parent directory, so + // stage inside the collection folder itself instead — still same-volume, and never %TEMP% + // (which could be on another drive and reintroduce the cross-volume failure). + // The name is dot-prefixed so it is ignored by the collection's book scan if it ever ends + // up inside a scanned folder. + var collectionDir = TheOneEditableCollection.PathToDirectory; + var stagingParent = Directory.GetParent(collectionDir)?.FullName ?? collectionDir; + var tempFolder = Path.Combine(stagingParent, ".BloomImport-" + Guid.NewGuid()); + try + { + ZipUtils.ExpandZip(sourcePath, tempFolder); + + // The .bloomCollection file (there should be exactly one, but delete any) is + // collection-level and must not end up inside the book folder. + foreach (var settingsFile in Directory.GetFiles(tempFolder, "*.bloomCollection")) + RobustFile.Delete(settingsFile); + + htmlPath = BookStorage.FindBookHtmlInFolder(tempFolder); + if (string.IsNullOrEmpty(htmlPath)) + throw new BloomSourceImportException("No book was found in this file."); + + // An imported book should not carry over Team Collection status from wherever it came from. + BookStorage.RemoveLocalOnlyFiles(tempFolder); + + instanceId = BookMetaData.FromFolder(tempFolder)?.Id; + return tempFolder; + } + catch + { + if (Directory.Exists(tempFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); + throw; + } + } + + /// + /// Imports one .bloomSource file by making a NEW derivative book (new id and lineage) from + /// the book it contains, placed in the editable collection. Returns the new Book, or null if + /// creation was cancelled (e.g. a template configuration dialog). Because a derivative always + /// gets a fresh id, there is never a duplicate to resolve. + /// + internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath) + { + var tempFolder = ExtractAndPrepareBloomSourceToTemp(sourcePath, out _, out _); + try + { + var newBook = _bookServer.CreateFromSourceBook( + tempFolder, + TheOneEditableCollection.PathToDirectory + ); + if (newBook == null) + return null; // e.g. the source had a configuration dialog and the user cancelled + + // Add the new book to the collection in memory rather than reloading and looking it + // up again (which was the source of a silent not-found fallback). + // + // The normal "make a book from this source" flow (CreateFromSourceBook) defers the + // "Created" history event via a pending marker and flushes it when the book is later + // deselected, so it can capture a title the user types afterward. That does not work + // for a batch import: only the last-imported book is ever selected, so every earlier + // book's pending event would be lost once its Book object is discarded (a freshly + // rebuilt Book has no pending marker). An imported book already has its title, so we + // set the marker and flush it immediately, giving every imported derivative its own + // "Created" history entry. + newBook.PendingCreationSource = Path.GetFileName(sourcePath); + newBook.PendingCreationSourceTitle = newBook.BookInfo.GetTitleForLanguage( + newBook.BookData.Language1Tag + ); + + newBook.BringBookUpToDate(new NullProgress(), false); + + TheOneEditableCollection.AddBookInfo(newBook.BookInfo); + newBook.RecordPendingCreatedHistoryEvent(); + return newBook; + } + finally + { + if (Directory.Exists(tempFolder)) + SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); + } + } + + /// + /// Opens the file as a zip and confirms it looks like a single-book .bloomSource: it must + /// open as a zip, must not contain path-traversal/absolute entries (zip-slip), and must not + /// be a Bloom Pack (a zip whose sole top-level entry is a collection folder). + /// Throws otherwise. + /// + private void ValidateBloomSourceZip(string sourcePath) + { + ICSharpCode.SharpZipLib.Zip.ZipFile zip; + try + { + zip = new ICSharpCode.SharpZipLib.Zip.ZipFile(sourcePath); + } + catch (Exception) + { + throw new BloomSourceImportException("This is not a valid Bloom source file."); + } + using (zip) + { + var topLevelDirs = new HashSet(); + var hasTopLevelFile = false; + foreach (ICSharpCode.SharpZipLib.Zip.ZipEntry entry in zip) + { + var name = entry.Name.Replace('\\', '/'); + var parts = name.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries); + // Zip-slip / absolute path guard: reject anything that could escape the target folder. + if (Path.IsPathRooted(entry.Name) || parts.Contains("..")) + throw new BloomSourceImportException( + "This file contains invalid entries and cannot be imported." + ); + if (parts.Length == 0) + continue; + if (parts.Length == 1 && !entry.IsDirectory) + hasTopLevelFile = true; + else + topLevelDirs.Add(parts[0]); + } + + // A single book always has files at the root (meta.json, the book htm). A Bloom Pack + // instead wraps everything in one collection folder, so it has no top-level files. + if (!hasTopLevelFile && topLevelDirs.Count == 1) + throw new BloomSourceImportException( + "This looks like a Bloom Pack (a whole collection), not a single book. " + + "To install a Bloom Pack, double-click it or use \"Open or Create Another Collection\"." + ); + } + } + + /// + /// Returns true only if every chosen .bloomSource book that is already in the collection may + /// currently be replaced. Replacing recycles the existing book, so in a Team Collection each + /// duplicate must be checked out here (BookInfo.IsSaveable); outside a Team Collection books + /// are always saveable. The collection screen uses this to disable the "Replace" duplicate + /// choice (and explain why) when any duplicate is not checked out, so the user can't delete a + /// book they haven't checked out. When there are no duplicates the question is moot, so this + /// returns true. Duplicates are detected purely by bookInstanceId, exactly as in + /// . + /// + internal bool AllBloomSourceDuplicatesAreReplaceable(string[] paths) + { + if (paths == null || paths.Length == 0) + return true; + + var existingById = TheOneEditableCollection + .GetBookInfos() + .Where(b => !string.IsNullOrEmpty(b.Id)) + .GroupBy(b => b.Id) + .ToDictionary(g => g.Key, g => g.First()); + foreach (var path in paths) + { + var id = ReadBookInstanceIdFromBloomSource(path); + if ( + !string.IsNullOrEmpty(id) + && existingById.TryGetValue(id, out var existing) + && !existing.IsSaveable + ) + return false; + } + return true; + } + + /// + /// Returns true if any of the given .bloomSource files contains a book whose bookInstanceId + /// is already present in the editable collection. Detection is purely by bookInstanceId, not + /// by title or folder name: two books with the same name but different ids are not duplicates, + /// while two books with the same id are (that id also controls re-uploading to the Bloom + /// library, so we must never end up with two books sharing one). The collection screen uses + /// this (on the paths it got from ) to decide + /// whether to ask the user how duplicates should be handled before importing. + /// + internal bool AnyBloomSourceIsAlreadyInCollection(string[] paths) + { + if (paths == null || paths.Length == 0) + return false; + + var existingIds = new HashSet( + TheOneEditableCollection + .GetBookInfos() + .Select(b => b.Id) + .Where(id => !string.IsNullOrEmpty(id)) + ); + foreach (var path in paths) + { + var id = ReadBookInstanceIdFromBloomSource(path); + if (!string.IsNullOrEmpty(id) && existingIds.Contains(id)) + return true; + } + return false; + } + + /// + /// Reads the bookInstanceId from a .bloomSource file's meta.json (which is at the zip root) + /// without extracting the whole book. Returns null if it can't be read (e.g. the file is + /// not a valid single-book source); such files simply aren't treated as duplicates here. + /// Parses the metadata the same way the import itself does (BookMetaData) so the pre-flight + /// duplicate check can't disagree with the actual import about a book's id. + /// + private static string ReadBookInstanceIdFromBloomSource(string sourcePath) + { + try + { + using (var zip = new ICSharpCode.SharpZipLib.Zip.ZipFile(sourcePath)) + { + var index = zip.FindEntry("meta.json", true); + if (index < 0) + return null; + using (var stream = zip.GetInputStream(index)) + using (var reader = new StreamReader(stream)) + { + return BookMetaData.FromString(reader.ReadToEnd())?.Id; + } + } + } + catch (Exception) + { + return null; + } + } + } +} diff --git a/src/BloomExe/CollectionTab/CollectionModel.cs b/src/BloomExe/CollectionTab/CollectionModel.cs index 523fac832532..b56c427971b3 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.cs +++ b/src/BloomExe/CollectionTab/CollectionModel.cs @@ -6,7 +6,6 @@ using System.IO; using System.Linq; using System.Text.RegularExpressions; -using System.Threading.Tasks; using System.Windows.Forms; using System.Xml; using Bloom.Api; @@ -20,7 +19,6 @@ using Bloom.ToPalaso; using Bloom.ToPalaso.Experimental; using Bloom.Utils; -using Bloom.web; using Bloom.web.controllers; using DesktopAnalytics; using L10NSharp; @@ -50,11 +48,6 @@ public partial class CollectionModel private LocalizationChangedEvent _localizationChangedEvent; private BookCollectionHolder _bookCollectionHolder; - // The .bloomSource files the user chose to import, remembered between the file-picker step - // (ChooseBloomSourceFilesToImport) and the actual import (ImportBloomSourceFiles), so the - // collection screen can show the appropriate import dialog in between. - private string[] _bloomSourceFilesToImport; - public CollectionModel( string pathToCollection, CollectionSettings collectionSettings, @@ -196,737 +189,8 @@ public void DuplicateBook(Book.Book book) } } - /// - /// What the user chose to do when an imported .bloomSource book is already present - /// (same bookInstanceId) in the editable collection. - /// - internal enum ImportDuplicateChoice - { - Replace, - AddCopy, - Cancel, - } - - /// - /// A problem with an imported .bloomSource file that we can explain to the user - /// (e.g. corrupt file, wrong kind of file, no book inside). The Message is shown - /// directly to the user, so it should be friendly and not require a bug report. - /// - internal class BloomSourceImportException : Exception - { - public BloomSourceImportException(string userMessage) - : base(userMessage) { } - } - - /// - /// Prompts the user to pick one or more .bloomSource files, remembering them for a following - /// call. Returns true if at least one file was chosen. - /// This is separate from the import itself so the collection screen can, once the files are - /// chosen, show the single import dialog appropriate to the batch (edit-vs-derivative when - /// none are already present, replace-vs-add-copy when any are). - /// - public bool ChooseBloomSourceFilesToImport() - { - using (var dlg = new BloomOpenFileDialog()) - { - dlg.Multiselect = true; - dlg.CheckFileExists = true; - dlg.Title = LocalizationManager.GetString( - "CollectionTab.ImportBloomSource", - "Import .bloomSource File(s)" - ); - dlg.Filter = - "Bloom Source files (*.bloomSource)|*.bloomSource|Team Collection books (*.bloom)|*.bloom|All files (*.*)|*.*"; - if (dlg.ShowDialog() != DialogResult.OK) - { - _bloomSourceFilesToImport = null; - return false; - } - _bloomSourceFilesToImport = dlg.FileNames; - return _bloomSourceFilesToImport.Length > 0; - } - } - - /// - /// Runs behind the embedded progress dialog on a - /// background thread, so a large batch neither freezes the collection screen nor lets the - /// awaiting browser request time out. The front-end already hosts the "collectionTab" - /// EmbeddedProgressDialog, so it needs no extra wiring. - /// - public async Task ImportBloomSourceFilesWithProgressAsync( - bool makeDerivatives, - bool replaceExistingDuplicates - ) - { - await BrowserProgressDialog.DoWorkWithProgressDialogAsync( - _webSocketServer, - (progress, worker) => - { - ImportBloomSourceFiles(makeDerivatives, replaceExistingDuplicates, progress); - return Task.FromResult(false); // false => close the dialog when we finish - }, - "collectionTab", - LocalizationManager.GetString( - "CollectionTab.ImportBloomSource.Importing", - "Importing Books" - ), - showCancelButton: false - ); - } - - /// - /// Imports the .bloomSource files previously chosen via - /// into the current editable collection. The - /// user has already made the choice (in the collection screen) that applies to the whole - /// batch: when is true, each imported book is used to make - /// a new derivative book (otherwise each is imported as an editable book); and, for the edit - /// case, when is true any book already in the - /// collection is replaced (otherwise it is added as a numbered copy). The last successfully - /// imported book is selected when done. - /// - public void ImportBloomSourceFiles( - bool makeDerivatives, - bool replaceExistingDuplicates, - IWebSocketProgress progress = null - ) - { - var paths = _bloomSourceFilesToImport; - _bloomSourceFilesToImport = null; - if (paths == null || paths.Length == 0) - return; - - // The user has already chosen, once for the whole batch, what to do when an imported - // book is already in the collection (only relevant when editing, not making derivatives). - var duplicateChoice = replaceExistingDuplicates - ? ImportDuplicateChoice.Replace - : ImportDuplicateChoice.AddCopy; - - Book.Book lastImported = null; - // For the edit path we move every book into place first and reload the collection just - // once at the end, instead of reloading (which rescans the whole collection from disk) - // once per file. Each entry pairs the destination folder with its source file name so we - // can record an accurate "Imported from" history event after that single reload. - var importedEditFolders = new List<(string destFolder, string sourceName)>(); - // Because the collection isn't reloaded between files, the per-file duplicate check can't - // see books imported earlier in this same batch. Track their ids here so two files that - // share a bookInstanceId don't both land with that id (which would defeat the whole point - // of duplicate handling). - var idsImportedThisBatch = new HashSet(); - foreach (var path in paths) - { - try - { - progress?.MessageWithoutLocalizing($"Importing {Path.GetFileName(path)}..."); - if (makeDerivatives) - { - // The derivative path builds its Book in memory and adds it to the collection - // itself, so it needs no reload. - var book = MakeDerivativeFromBloomSourceFile(path); - if (book != null) - lastImported = book; - } - else - { - var destFolder = ImportBloomSourceFileToCollectionFolder( - path, - _ => duplicateChoice, - idsImportedThisBatch - ); - if (destFolder != null) - importedEditFolders.Add((destFolder, Path.GetFileName(path))); - } - } - catch (BloomSourceImportException e) - { - // A problem we can explain to the user; no bug report needed. - ErrorReport.NotifyUserOfProblem( - "{0}\r\n\r\n{1}", - Path.GetFileName(path), - e.Message - ); - } - catch (Exception e) - { - // Something unexpected; let the user report it. - NonFatalProblem.Report( - ModalIf.All, - PassiveIf.None, - shortUserLevelMessage: string.Format( - "Bloom was not able to import \"{0}\".", - Path.GetFileName(path) - ), - moreDetails: null, - exception: e - ); - } - } - - // Now that every edit-import's folder is in place, reload the collection a single time - // and turn each imported folder into a Book with its Created history event. - if (importedEditFolders.Count > 0) - { - ReloadEditableCollection(); - foreach (var (destFolder, sourceName) in importedEditFolders) - { - var newInfo = TheOneEditableCollection - .GetBookInfos() - .FirstOrDefault(i => i.FolderPath == destFolder); - if (newInfo == null) - { - // Should not happen after a successful move; surface it but don't abort the - // rest of the batch. - NonFatalProblem.Report( - ModalIf.All, - PassiveIf.None, - shortUserLevelMessage: string.Format( - "Bloom imported \"{0}\" but could not find it in the collection afterward.", - sourceName - ) - ); - continue; - } - var newBook = GetBookFromBookInfo(newInfo); - BookHistory.AddEvent( - newBook, - BookHistoryEventType.Created, - $"Imported from \"{sourceName}\"" - ); - lastImported = newBook; - } - } - - if (lastImported != null) - SelectBookOnUiThread(lastImported); - } - - /// - /// Selects a book, marshaling to the UI thread when necessary. Import can run on a background - /// thread (behind the progress dialog), but SelectBook raises SelectionChanged synchronously - /// to WinForms views, so it must happen on the UI thread. When no window is open (e.g. unit - /// tests) this just runs inline. - /// - private void SelectBookOnUiThread(Book.Book book) - { - var form = Shell.GetShellOrOtherOpenForm(); - if (form != null && form.InvokeRequired) - form.Invoke((Action)(() => _bookSelection.SelectBook(book))); - else - _bookSelection.SelectBook(book); - } - - /// - /// Does the file-system part of importing a .bloomSource: validates it, extracts it, removes - /// the stray .bloomCollection and any Team-Collection status files, handles the - /// already-in-collection case (Replace/Add-a-copy/Cancel), and moves the book into the - /// editable collection folder. Returns the destination folder, or null if the user cancelled. - /// This is separated from the Book-level work so it can be unit tested without loading a Book. - /// - /// Ids of books already imported earlier in this - /// same batch. Because the collection is reloaded only once, after the whole batch, a book - /// whose id is in this set isn't yet visible to the in-collection check above; we treat it as - /// a within-batch duplicate and give the incoming book a fresh id so the two don't collide. - /// When null (e.g. single-file tests), within-batch tracking is skipped. - internal string ImportBloomSourceFileToCollectionFolder( - string sourcePath, - Func resolveDuplicate, - ISet idsAlreadyImportedThisBatch = null - ) - { - var editable = TheOneEditableCollection; - var tempFolder = ExtractAndPrepareBloomSourceToTemp( - sourcePath, - out var htmlPath, - out var instanceId - ); - // The id this book arrived with. We record it (once the import succeeds, below) so that a - // later file in the same batch carrying the same id is recognized as a duplicate even - // though the collection is only reloaded once, after the whole batch. - var originalInstanceId = instanceId; - string destFolder = null; - string replaceTargetFolder = null; - // Once the book has been moved into the collection the import has succeeded; a failure - // after that point (e.g. recycling the replaced duplicate) must not delete it. - var importSucceeded = false; - // "Add a copy" produces an independent copy exactly like the Duplicate Book command: - // a new id and the " - Copy-" folder convention (the caption is left alone, - // just as Duplicate leaves it). - var folderSeparator = "-"; - try - { - var baseName = Path.GetFileNameWithoutExtension(htmlPath); - - var existing = string.IsNullOrEmpty(instanceId) - ? null - : editable.GetBookInfos().FirstOrDefault(b => b.Id == instanceId); - // A book with this id isn't in the collection, but an earlier file in this same batch - // already brought it in (the collection is only reloaded once, after the batch, so it - // isn't visible above yet). Replacing a book from the same batch is meaningless, so we - // always make this one an independent copy so the two don't end up sharing an id. - var isDuplicateWithinBatch = - existing == null - && !string.IsNullOrEmpty(instanceId) - && idsAlreadyImportedThisBatch != null - && idsAlreadyImportedThisBatch.Contains(instanceId); - - // Whether to turn the incoming book into an independent copy: a new id (so it can't - // collide) plus the " - Copy-" folder convention the Duplicate command uses. - var makeIndependentCopy = false; - if (existing != null) - { - switch (resolveDuplicate(existing.Title)) - { - case ImportDuplicateChoice.Cancel: - return null; - case ImportDuplicateChoice.Replace: - // Replacing overwrites the existing book in place, so we must be allowed - // to modify it: in a Team Collection that means it has to be checked out - // here. The collection screen already disables "Replace" when any - // duplicate is not (see AllChosenBloomSourceDuplicatesAreReplaceable), but - // guard here too so we can never overwrite a book the user hasn't checked - // out. - if (!existing.IsSaveable) - throw new ApplicationException( - $"Cannot replace \"{existing.Title}\" because it is not checked out of the Team Collection." - ); - // Overwrite this book's folder in place (below), keeping its folder name - // and — in a Team Collection — its checkout status, so it stays the same - // checked-out repo book (the new content is pushed to the shared repo on - // the next check-in) rather than becoming a new local book while the old - // one resurrects from the repo. The imported book keeps the same id. - replaceTargetFolder = existing.FolderPath; - break; - case ImportDuplicateChoice.AddCopy: - makeIndependentCopy = true; - break; - } - } - else if (isDuplicateWithinBatch) - { - makeIndependentCopy = true; - } - - if (makeIndependentCopy) - { - folderSeparator = " - Copy-"; - instanceId = Guid.NewGuid().ToString(); - var meta = BookMetaData.FromFolder(tempFolder); - if (meta != null) - { - meta.Id = instanceId; - meta.WriteToFolder(tempFolder); - } - } - - if (replaceTargetFolder != null) - { - // "Replace" overwrites the existing book's folder in place rather than making a - // new folder and deleting the old one. This keeps the folder name and (in a Team - // Collection) the checkout status, so it stays the same checked-out repo book; see - // the Replace case above. - // - // If the book we're about to overwrite is the current selection, deselect it first - // so its files aren't locked (e.g. open in a preview) while we swap the folder. - if (_bookSelection.CurrentSelection?.FolderPath == replaceTargetFolder) - SelectBookOnUiThread(null); - destFolder = ReplaceBookInPlaceKeepingTeamStatus( - tempFolder, - htmlPath, - replaceTargetFolder - ); - importSucceeded = true; - // Remember the id this book arrived with so a later file in the same batch carrying - // the same id is caught as a within-batch duplicate above. - if (!string.IsNullOrEmpty(originalInstanceId)) - idsAlreadyImportedThisBatch?.Add(originalInstanceId); - return destFolder; - } - - // Pick a folder name that doesn't collide with an existing book on disk, and make - // the main htm file match it (Bloom's convention: folder name == book htm name). - var destName = BookStorage.GetUniqueBookFolderName( - editable.PathToDirectory, - BookStorage.SanitizeNameForFileSystem(baseName), - instanceId, - folderSeparator - ); - destFolder = Path.Combine(editable.PathToDirectory, destName); - var renamedHtmlPath = Path.Combine(tempFolder, destName + ".htm"); - if (!string.Equals(htmlPath, renamedHtmlPath, StringComparison.OrdinalIgnoreCase)) - RobustFile.Move(htmlPath, renamedHtmlPath); - SIL.IO.RobustIO.MoveDirectory(tempFolder, destFolder); - importSucceeded = true; - - // Remember the id this book arrived with so a later file in the same batch carrying - // the same id is caught as a within-batch duplicate above. - if (!string.IsNullOrEmpty(originalInstanceId)) - idsAlreadyImportedThisBatch?.Add(originalInstanceId); - - return destFolder; - } - catch - { - // If we failed partway through creating the destination folder, don't leave a - // half-imported book behind in the collection. (The Replace/overwrite path handles - // its own rollback in ReplaceBookInPlaceKeepingTeamStatus and only sets destFolder - // once it has fully succeeded, so this never deletes a restored original.) - if (!importSucceeded && destFolder != null && Directory.Exists(destFolder)) - SIL.IO.RobustIO.DeleteDirectoryAndContents(destFolder); - throw; - } - finally - { - if (Directory.Exists(tempFolder)) - SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); - } - } - - /// - /// Replaces the book in with the freshly-extracted imported - /// book in , overwriting its content in place. The target folder - /// keeps its name and its Team Collection files (checkout status etc.), so in a Team - /// Collection it stays the same checked-out book — the new content is pushed to the shared - /// repo on the next check-in — rather than becoming a new local book while the old one - /// resurrects from the repo. The imported book already carries the same bookInstanceId - /// (Replace keeps the id). The swap goes through a set-aside backup so a mid-swap failure - /// restores the original rather than losing the book. - /// - /// the target folder path, now holding the imported book - private string ReplaceBookInPlaceKeepingTeamStatus( - string tempFolder, - string htmlPath, - string targetFolder - ) - { - // Bloom's convention is folder name == main htm name; keep the target's existing folder - // name (which is what maps it to its Team Collection repo entry), so rename the incoming - // htm to match it. - var targetName = Path.GetFileName(targetFolder); - var renamedHtmlPath = Path.Combine(tempFolder, targetName + ".htm"); - if (!string.Equals(htmlPath, renamedHtmlPath, StringComparison.OrdinalIgnoreCase)) - RobustFile.Move(htmlPath, renamedHtmlPath); - - // Swap on the same volume: move the existing book aside, move the imported content into - // its place, then carry the Team Collection files over from the original so the book stays - // checked out. If anything fails mid-swap, restore the original and rethrow so we never - // lose the book. (Outside a Team Collection there are no TC files, so that step does - // nothing.) - // The set-aside name is dot-prefixed so that, if final cleanup ever fails and it lingers, - // the collection scan ignores it (it only skips dot-prefixed folders) rather than picking - // it up as a book — which, since it holds the old content with the same bookInstanceId, - // would be a duplicate-id book. - var backupFolder = Path.Combine( - Path.GetDirectoryName(targetFolder), - "." + Path.GetFileName(targetFolder) + ".replacing-" + Guid.NewGuid() - ); - SIL.IO.RobustIO.MoveDirectory(targetFolder, backupFolder); - try - { - SIL.IO.RobustIO.MoveDirectory(tempFolder, targetFolder); - var teamFiles = new List(); - Bloom.TeamCollection.TeamCollection.AddTCSpecificFiles(backupFolder, teamFiles); - foreach (var teamFile in teamFiles) - RobustFile.Copy( - teamFile, - Path.Combine(targetFolder, Path.GetFileName(teamFile)), - true - ); - } - catch - { - if (Directory.Exists(targetFolder)) - SIL.IO.RobustIO.DeleteDirectoryAndContents(targetFolder); - SIL.IO.RobustIO.MoveDirectory(backupFolder, targetFolder); - throw; - } - // The replace has fully succeeded; deleting the set-aside old content is just cleanup. If - // it fails (locked files, flaky network/removable drive, AV), don't let that turn a - // successful replace into an "import failed" error and keep the replaced book from being - // reloaded into view — report it as a minor problem instead. (The backup is dot-prefixed, - // so even if it lingers the collection scan ignores it.) - try - { - SIL.IO.RobustIO.DeleteDirectoryAndContents(backupFolder); - } - catch (Exception e) - { - NonFatalProblem.Report( - ModalIf.All, - PassiveIf.None, - shortUserLevelMessage: string.Format( - "The book \"{0}\" was replaced, but Bloom could not remove a temporary copy of the old version. You may delete the folder \"{1}\" manually.", - Path.GetFileName(targetFolder), - Path.GetFileName(backupFolder) - ), - moreDetails: null, - exception: e - ); - } - return targetFolder; - } - - /// - /// Validates a .bloomSource file, extracts it to a fresh temp folder, removes the stray - /// .bloomCollection settings file and any Team-Collection status files, and confirms the - /// folder contains a book. Returns the temp folder path (the caller owns deleting it), the - /// book's main htm path, and its bookInstanceId (may be null). On any failure the temp - /// folder is cleaned up and the exception is rethrown. - /// - private string ExtractAndPrepareBloomSourceToTemp( - string sourcePath, - out string htmlPath, - out string instanceId - ) - { - // A .bloomSource is a zip of a single book folder's *contents* (flat at the root), - // plus the collection's .bloomCollection settings file. Validate that shape and guard - // against malicious entries before we extract anything. - ValidateBloomSourceZip(sourcePath); - - // Extract onto the same volume as the collection. The book is later moved into the - // collection folder with a directory move, which cannot cross volumes; the system - // temp folder is frequently on a different drive than the user's collection (e.g. %TEMP% - // on C: but the collection on D: or a removable/network drive), which would make every - // edit-mode import fail. Staging in the collection's parent guarantees a same-volume move. - // If the collection is at a filesystem root (e.g. "D:\"), it has no parent directory, so - // stage inside the collection folder itself instead — still same-volume, and never %TEMP% - // (which could be on another drive and reintroduce the cross-volume failure). - // The name is dot-prefixed so it is ignored by the collection's book scan if it ever ends - // up inside a scanned folder. - var collectionDir = TheOneEditableCollection.PathToDirectory; - var stagingParent = Directory.GetParent(collectionDir)?.FullName ?? collectionDir; - var tempFolder = Path.Combine(stagingParent, ".BloomImport-" + Guid.NewGuid()); - try - { - ZipUtils.ExpandZip(sourcePath, tempFolder); - - // The .bloomCollection file (there should be exactly one, but delete any) is - // collection-level and must not end up inside the book folder. - foreach (var settingsFile in Directory.GetFiles(tempFolder, "*.bloomCollection")) - RobustFile.Delete(settingsFile); - - htmlPath = BookStorage.FindBookHtmlInFolder(tempFolder); - if (string.IsNullOrEmpty(htmlPath)) - throw new BloomSourceImportException("No book was found in this file."); - - // An imported book should not carry over Team Collection status from wherever it came from. - BookStorage.RemoveLocalOnlyFiles(tempFolder); - - instanceId = BookMetaData.FromFolder(tempFolder)?.Id; - return tempFolder; - } - catch - { - if (Directory.Exists(tempFolder)) - SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); - throw; - } - } - - /// - /// Imports one .bloomSource file by making a NEW derivative book (new id and lineage) from - /// the book it contains, placed in the editable collection. Returns the new Book, or null if - /// creation was cancelled (e.g. a template configuration dialog). Because a derivative always - /// gets a fresh id, there is never a duplicate to resolve. - /// - internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath) - { - var tempFolder = ExtractAndPrepareBloomSourceToTemp(sourcePath, out _, out _); - try - { - var newBook = _bookServer.CreateFromSourceBook( - tempFolder, - TheOneEditableCollection.PathToDirectory - ); - if (newBook == null) - return null; // e.g. the source had a configuration dialog and the user cancelled - - // Add the new book to the collection in memory rather than reloading and looking it - // up again (which was the source of a silent not-found fallback). - // - // The normal "make a book from this source" flow (CreateFromSourceBook) defers the - // "Created" history event via a pending marker and flushes it when the book is later - // deselected, so it can capture a title the user types afterward. That does not work - // for a batch import: only the last-imported book is ever selected, so every earlier - // book's pending event would be lost once its Book object is discarded (a freshly - // rebuilt Book has no pending marker). An imported book already has its title, so we - // set the marker and flush it immediately, giving every imported derivative its own - // "Created" history entry. - newBook.PendingCreationSource = Path.GetFileName(sourcePath); - newBook.PendingCreationSourceTitle = newBook.BookInfo.GetTitleForLanguage( - newBook.BookData.Language1Tag - ); - - newBook.BringBookUpToDate(new NullProgress(), false); - - TheOneEditableCollection.AddBookInfo(newBook.BookInfo); - newBook.RecordPendingCreatedHistoryEvent(); - return newBook; - } - finally - { - if (Directory.Exists(tempFolder)) - SIL.IO.RobustIO.DeleteDirectoryAndContents(tempFolder); - } - } - - /// - /// Opens the file as a zip and confirms it looks like a single-book .bloomSource: it must - /// open as a zip, must not contain path-traversal/absolute entries (zip-slip), and must not - /// be a Bloom Pack (a zip whose sole top-level entry is a collection folder). - /// Throws otherwise. - /// - private void ValidateBloomSourceZip(string sourcePath) - { - ICSharpCode.SharpZipLib.Zip.ZipFile zip; - try - { - zip = new ICSharpCode.SharpZipLib.Zip.ZipFile(sourcePath); - } - catch (Exception) - { - throw new BloomSourceImportException("This is not a valid Bloom source file."); - } - using (zip) - { - var topLevelDirs = new HashSet(); - var hasTopLevelFile = false; - foreach (ICSharpCode.SharpZipLib.Zip.ZipEntry entry in zip) - { - var name = entry.Name.Replace('\\', '/'); - var parts = name.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries); - // Zip-slip / absolute path guard: reject anything that could escape the target folder. - if (Path.IsPathRooted(entry.Name) || parts.Contains("..")) - throw new BloomSourceImportException( - "This file contains invalid entries and cannot be imported." - ); - if (parts.Length == 0) - continue; - if (parts.Length == 1 && !entry.IsDirectory) - hasTopLevelFile = true; - else - topLevelDirs.Add(parts[0]); - } - - // A single book always has files at the root (meta.json, the book htm). A Bloom Pack - // instead wraps everything in one collection folder, so it has no top-level files. - if (!hasTopLevelFile && topLevelDirs.Count == 1) - throw new BloomSourceImportException( - "This looks like a Bloom Pack (a whole collection), not a single book. " - + "To install a Bloom Pack, double-click it or use \"Open or Create Another Collection\"." - ); - } - } - - /// - /// Returns true if any of the .bloomSource files the user chose (via - /// ) contains a book whose bookInstanceId is - /// already present in the editable collection. The collection screen uses this to decide - /// whether to ask the user how duplicates should be handled before importing. - /// - public bool AnyChosenBloomSourceIsAlreadyInCollection() - { - return AnyBloomSourceIsAlreadyInCollection(_bloomSourceFilesToImport); - } - - /// - /// Returns true only if every chosen .bloomSource book that is already in the collection may - /// currently be replaced. Replacing recycles the existing book, so in a Team Collection each - /// duplicate must be checked out here (BookInfo.IsSaveable); outside a Team Collection books - /// are always saveable. The collection screen uses this to disable the "Replace" duplicate - /// choice (and explain why) when any duplicate is not checked out, so the user can't delete a - /// book they haven't checked out. When there are no duplicates the question is moot, so this - /// returns true. - /// - public bool AllChosenBloomSourceDuplicatesAreReplaceable() - { - return AllBloomSourceDuplicatesAreReplaceable(_bloomSourceFilesToImport); - } - - /// - /// The testable core of . Duplicates - /// are detected purely by bookInstanceId, exactly as in - /// . - /// - internal bool AllBloomSourceDuplicatesAreReplaceable(string[] paths) - { - if (paths == null || paths.Length == 0) - return true; - - var existingById = TheOneEditableCollection - .GetBookInfos() - .Where(b => !string.IsNullOrEmpty(b.Id)) - .GroupBy(b => b.Id) - .ToDictionary(g => g.Key, g => g.First()); - foreach (var path in paths) - { - var id = ReadBookInstanceIdFromBloomSource(path); - if ( - !string.IsNullOrEmpty(id) - && existingById.TryGetValue(id, out var existing) - && !existing.IsSaveable - ) - return false; - } - return true; - } - - /// - /// Returns true if any of the given .bloomSource files contains a book whose bookInstanceId - /// is already present in the editable collection. Detection is purely by bookInstanceId, not - /// by title or folder name: two books with the same name but different ids are not duplicates, - /// while two books with the same id are (that id also controls re-uploading to the Bloom - /// library, so we must never end up with two books sharing one). Split out from - /// so it can be unit tested without a - /// file-picker dialog. - /// - internal bool AnyBloomSourceIsAlreadyInCollection(string[] paths) - { - if (paths == null || paths.Length == 0) - return false; - - var existingIds = new HashSet( - TheOneEditableCollection - .GetBookInfos() - .Select(b => b.Id) - .Where(id => !string.IsNullOrEmpty(id)) - ); - foreach (var path in paths) - { - var id = ReadBookInstanceIdFromBloomSource(path); - if (!string.IsNullOrEmpty(id) && existingIds.Contains(id)) - return true; - } - return false; - } - - /// - /// Reads the bookInstanceId from a .bloomSource file's meta.json (which is at the zip root) - /// without extracting the whole book. Returns null if it can't be read (e.g. the file is - /// not a valid single-book source); such files simply aren't treated as duplicates here. - /// Parses the metadata the same way the import itself does (BookMetaData) so the pre-flight - /// duplicate check can't disagree with the actual import about a book's id. - /// - private static string ReadBookInstanceIdFromBloomSource(string sourcePath) - { - try - { - using (var zip = new ICSharpCode.SharpZipLib.Zip.ZipFile(sourcePath)) - { - var index = zip.FindEntry("meta.json", true); - if (index < 0) - return null; - using (var stream = zip.GetInputStream(index)) - using (var reader = new StreamReader(stream)) - { - return BookMetaData.FromString(reader.ReadToEnd())?.Id; - } - } - } - catch (Exception) - { - return null; - } - } + // The .bloomSource import feature (choosing files, the duplicate pre-flight check, and the + // import itself) lives in CollectionModel.BloomSourceImport.cs. public void moveBookIntoThisCollection(Book.Book origBook, BookCollection origCollection) { diff --git a/src/BloomExe/web/controllers/CollectionApi.cs b/src/BloomExe/web/controllers/CollectionApi.cs index c5bbdca10f34..82c0a40bde27 100644 --- a/src/BloomExe/web/controllers/CollectionApi.cs +++ b/src/BloomExe/web/controllers/CollectionApi.cs @@ -313,28 +313,26 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) kApiUrlPart + "chooseBloomSourceFilesToImport/", (request) => { - // Opens the file picker so the user can select the .bloomSource file(s) to import. - // Replies true if at least one was chosen; the collection screen then asks whether - // to edit or make derivatives before calling importBloomSource. - request.ReplyWithBoolean(_collectionModel.ChooseBloomSourceFilesToImport()); - }, - true - ); - - apiHandler.RegisterEndpointHandler( - kApiUrlPart + "bloomSourceImportDuplicateInfo/", - (request) => - { - // Tells the collection screen what it needs to pick and configure the duplicate - // dialog: whether any book the user chose to import is already in the collection - // (so it knows whether to ask how duplicates should be handled) and, when it is, - // whether every such duplicate may currently be replaced (false when, in a Team - // Collection, any is not checked out here, so the screen disables "Replace"). + // Opens the file picker so the user can select the .bloomSource file(s) to import, + // and replies with everything the collection screen needs to show the one dialog + // that fits the batch: the chosen file paths (empty if the user cancelled), + // whether any chosen book is already in the collection (so it knows whether to ask + // how duplicates should be handled), and — when it is — whether every such + // duplicate may currently be replaced (false when, in a Team Collection, any is + // not checked out here, so the screen disables "Replace"). The front-end holds the + // returned paths and passes them back to importBloomSource; nothing is remembered + // on this side between calls. + var files = _collectionModel.ChooseBloomSourceFilesToImport(); request.ReplyWithJson( new { - anyDuplicates = _collectionModel.AnyChosenBloomSourceIsAlreadyInCollection(), - canReplace = _collectionModel.AllChosenBloomSourceDuplicatesAreReplaceable(), + files, + anyDuplicates = _collectionModel.AnyBloomSourceIsAlreadyInCollection( + files + ), + canReplace = _collectionModel.AllBloomSourceDuplicatesAreReplaceable( + files + ), } ); }, @@ -345,19 +343,21 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) kApiUrlPart + "importBloomSource/", async (request) => { - // Imports the .bloomSource file(s) the user already chose (via - // chooseBloomSourceFilesToImport) into the current editable collection. The - // collection screen has since shown the one dialog that fit the batch: when no - // chosen book was already present it asked edit-vs-derivative ("mode"); when any - // was present it asked replace-vs-add-copy ("onDuplicate", always mode=edit). - // Each single choice applies to every chosen file. - // Runs behind the collection tab's progress dialog on a background thread (not the - // UI thread) so a large batch neither freezes the screen nor lets this request - // time out. + // Imports the .bloomSource file(s) whose paths the front-end passes in the POST + // body (the ones it got from chooseBloomSourceFilesToImport) into the current + // editable collection. The collection screen has since shown the one dialog that + // fit the batch: when no chosen book was already present it asked + // edit-vs-derivative ("mode"); when any was present it asked replace-vs-add-copy + // ("onDuplicate", always mode=edit). Each single choice applies to every chosen + // file. Runs behind the collection tab's progress dialog on a background thread + // (not the UI thread) so a large batch neither freezes the screen nor lets this + // request time out. + var paths = request.RequiredPostObject(); var makeDerivatives = request.RequiredParam("mode") == "derivative"; var replaceExistingDuplicates = request.GetParamOrNull("onDuplicate") == "replace"; await _collectionModel.ImportBloomSourceFilesWithProgressAsync( + paths, makeDerivatives, replaceExistingDuplicates ); From 5e0027a333906e9932a49b81e92ed998eab123b3 Mon Sep 17 00:00:00 2001 From: Hatton Date: Tue, 7 Jul 2026 11:47:10 -0600 Subject: [PATCH 16/17] Fix XLF note order: ID note before context note (BL-16502) The new CollectionTab.ImportBloomSource.* entries placed the translator context before the ID: ... line. The project's xlf-strings skill (and the rest of this file) require the ID note first, with context notes after it. Reorder all 13 entries; pure reordering, no text changes. Addresses the Devin re-review bug finding. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../localization/en/BloomMediumPriority.xlf | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/DistFiles/localization/en/BloomMediumPriority.xlf b/DistFiles/localization/en/BloomMediumPriority.xlf index 7930b4ccac2f..7fd32b5e8386 100644 --- a/DistFiles/localization/en/BloomMediumPriority.xlf +++ b/DistFiles/localization/en/BloomMediumPriority.xlf @@ -741,68 +741,68 @@ Import .bloomSource File(s) - Menu command (and dialog title) that imports one or more books from .bloomSource files into the current collection. Keep ".bloomSource" unchanged; it is a file extension. ID: CollectionTab.ImportBloomSource + Menu command (and dialog title) that imports one or more books from .bloomSource files into the current collection. Keep ".bloomSource" unchanged; it is a file extension. Import books - Title of the dialogs shown while importing .bloomSource file(s): the one that asks whether to import for editing or as derivatives, and the one that asks how to handle books already in the collection. ID: CollectionTab.ImportBloomSource.ChoiceTitle + Title of the dialogs shown while importing .bloomSource file(s): the one that asks whether to import for editing or as derivatives, and the one that asks how to handle books already in the collection. Import for editing - Radio-button choice in the import dialog: import the book(s) to edit them directly. ID: CollectionTab.ImportBloomSource.EditChoice + Radio-button choice in the import dialog: import the book(s) to edit them directly. Preserve everything. - Explanation shown under the "Import for editing" radio-button choice in the import dialog. ID: CollectionTab.ImportBloomSource.EditChoiceDescription + Explanation shown under the "Import for editing" radio-button choice in the import dialog. Import as derivatives — new books based on the originals - Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). ID: CollectionTab.ImportBloomSource.DerivativeChoice + Radio-button choice in the import dialog: use the imported book(s) as the basis for new derivative books (e.g. translations). Use a new book ID & make room for new copyright and credits while preserving those of the original. - Explanation shown under the "Import as derivatives" radio-button choice in the import dialog. ID: CollectionTab.ImportBloomSource.DerivativeChoiceDescription + Explanation shown under the "Import as derivatives" radio-button choice in the import dialog. Some of these books are already in this collection. - Shown once when importing .bloomSource file(s), if any of the books is already in the collection. ID: CollectionTab.ImportBloomSource.DuplicatePrompt + Shown once when importing .bloomSource file(s), if any of the books is already in the collection. Replace the existing books - Radio-button choice in the import duplicate prompt: replace the existing book(s) with the imported one(s). ID: CollectionTab.ImportBloomSource.Replace + Radio-button choice in the import duplicate prompt: replace the existing book(s) with the imported one(s). The current copies will be overwritten. - Secondary line under the "Replace the existing books" choice in the import duplicate prompt. ID: CollectionTab.ImportBloomSource.ReplaceDescription + Secondary line under the "Replace the existing books" choice in the import duplicate prompt. All books must be checked out - Secondary line under the "Replace the existing books" choice in the import duplicate prompt, shown (and the choice disabled) when the collection is a Team Collection and one or more of the books being replaced is not checked out. ID: CollectionTab.ImportBloomSource.ReplaceNeedsCheckout + Secondary line under the "Replace the existing books" choice in the import duplicate prompt, shown (and the choice disabled) when the collection is a Team Collection and one or more of the books being replaced is not checked out. Add them as new copies - Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. ID: CollectionTab.ImportBloomSource.AddCopy + Radio-button choice in the import duplicate prompt: keep the existing book(s) and add the imported one(s) as new copies. The existing books stay as they are. - Secondary line under the "Add them as new copies" choice in the import duplicate prompt. ID: CollectionTab.ImportBloomSource.AddCopyDescription + Secondary line under the "Add them as new copies" choice in the import duplicate prompt. Importing Books - Title of the progress dialog shown while .bloomSource file(s) are being imported. ID: CollectionTab.ImportBloomSource.Importing + Title of the progress dialog shown while .bloomSource file(s) are being imported. Locked by {0} Branding From 638cab58a91516d753a311a136c14d62f750504d Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 9 Jul 2026 11:17:45 -0600 Subject: [PATCH 17/17] Clarify misleading MakeBloomSourceFile comment (BL-16502) Per Stephen McConnel's review: the old comment ("a zip of a book folder's contents...") read as if the test were hand-zipping files instead of using Bloom's existing export method. Reword to make clear the helper builds a .bloomSource from minimal dummy files via the real SaveAsBloomSourceFile export path. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/BloomTests/CollectionTab/CollectionModelTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/BloomTests/CollectionTab/CollectionModelTests.cs b/src/BloomTests/CollectionTab/CollectionModelTests.cs index 383f24c4c208..3f8704141756 100644 --- a/src/BloomTests/CollectionTab/CollectionModelTests.cs +++ b/src/BloomTests/CollectionTab/CollectionModelTests.cs @@ -123,8 +123,8 @@ private static string MetaJson(string instanceId, string title) return $"{{\"bookInstanceId\":\"{instanceId}\",\"title\":\"{title}\"}}"; } - // Build a .bloomSource file the same way Bloom's export does: a zip of a book folder's - // contents (flat at the root) plus a .bloomCollection settings file at the root. + // Create a .bloomSource file for unit testing from a minimal set of dummy files, using + // Bloom's real export method (SaveAsBloomSourceFile) so the result matches production. private string MakeBloomSourceFile(string title, string instanceId, bool includeHtm = true) { var src = Path.Combine(_folder.Path, "srcbook-" + Guid.NewGuid());