From 585de7b1f3e95e00257cc4c1caf01f051f618828 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Tue, 8 Sep 2026 14:16:19 -0500 Subject: [PATCH 1/4] Let e2e tests sign in to Bloom Library for real: per-instance user settings and sandbox uploads under --e2e (BL-16831) Bloom's end-to-end suite could not test anything that needs a real BloomLibrary.org login, above all uploading books. Signing in went out to an external browser, and the login lived in machine-wide settings shared with the developer's own Bloom, so a test could sign in or out only by disturbing the developer's session. The same machine-wide user.config (one per build version) also meant a test inherited and overwrote unrelated settings from whatever Bloom ran last. So the "Bulk Upload Quick Test" (Notion test case 211) and every other real-upload case stayed manual. Each launched Bloom gets its own user settings. A new --user-settings-folder argument points Settings.Default (the user.config) at a named folder for that process only (BloomSettingsProvider). The e2e launch fixture gives every Bloom a private folder inside the run's temp tree, so a run starts from defaults and its settings, the BloomLibrary login included, die with the run; it fails the launch if a stale Bloom.exe ignores the flag, and common/instanceInfo reports the folder. A launch Bloom rejects for a bad startup argument reports the error before anything reads settings, so it touches neither the named folder nor the shared profile. ExportImportFileFolder, the one setting that used .NET's default provider, now uses Bloom's, so the isolation is complete; a user's remembered spreadsheet folder resets once. Without the flag nothing changes. Uploads to the sandbox are allowed under --e2e; production stays blocked. RefuseUploadWhileRunningE2eTests now refuses only bloomlibrary.org, so an automated run can upload to dev.bloomlibrary.org for real while no automated click can ever publish where the public would see it. The bulk-upload child process inherits the same private settings folder and login, and under --e2e runs windowless. A real bulk-upload e2e test (Notion 211, which absorbed the retired 212) signs a dedicated test account into dev the way the website does (Firebase, then parse-server), bulk-uploads a four-book collection, and checks the manual test's outcomes: a bookshelf-less collection is refused; four books upload new and sit on the collection's bookshelf with its front/back matter pack; an unchanged re-upload skips all four; changing one updates only it; and moving the collection to the second bookshelf with a different pack updates all four, which then sit on the new shelf with the new pack, read from the HTML Bloom actually uploaded to S3, though nobody opened the books after the pack changed. It deletes what it uploads afterwards. The account email is a constant; its password comes from BLOOM_E2E_TESTER_EMAIL_BLORG_PASSWORD (a repository secret in CI; the test skips locally when it is unset). Also: helpers for reading a book's front/back matter pack and the uploaded copy from S3; one uploadCollection helper per upload round; the collection fixture's subscription-code support unified under kEnterpriseSubscriptionCode with the bookshelf alongside it; two automation-debt entries paid down and the one piece left (the Sign in button's trip through the system browser) recorded; and the nightly workflow's e2e step, which still needs the password secret. Co-Authored-By: Claude Fable 5.1 --- .github/skills/add-e2e-test/SKILL.md | 9 +- .github/workflows/nightly.yml | 7 + src/BloomE2E/AUTOMATION-DEBT.md | 65 +--- src/BloomE2E/README.md | 14 + src/BloomE2E/fixtures/bloomTest.ts | 7 + src/BloomE2E/fixtures/launchBloom.ts | 80 ++++- src/BloomE2E/helpers/bloomLibraryAccount.ts | 176 +++++++++++ src/BloomE2E/helpers/bloomLibraryServer.ts | 212 +++++++++++++ src/BloomE2E/helpers/bookHtml.ts | 32 ++ src/BloomE2E/helpers/bulkUpload.ts | 144 +++++++++ src/BloomE2E/helpers/collectionSettings.ts | 11 +- src/BloomE2E/helpers/copyrightAndLicense.ts | 5 + src/BloomE2E/helpers/libraryPublish.ts | 16 +- src/BloomE2E/helpers/userSettings.ts | 44 +++ .../tests/bulk-upload-quick-test.spec.ts | 290 ++++++++++++++++++ .../tests/user-settings-isolation.spec.ts | 103 +++++++ src/BloomExe/BloomSettingsProvider.cs | 60 ++++ src/BloomExe/Program.cs | 115 ++++++- src/BloomExe/Properties/Settings.Designer.cs | 81 ++--- src/BloomExe/Properties/Settings.settings | 86 +++--- .../BloomLibrary/BloomLibraryPublishModel.cs | 41 ++- src/BloomExe/web/controllers/CommonApi.cs | 5 + .../web/controllers/LibraryPublishApi.cs | 16 +- src/BloomTests/BloomSettingsProviderTests.cs | 120 ++++++++ src/BloomTests/ProgramTests.cs | 142 +++++++++ src/BloomVisualRegressionTests/index.spec.ts | 13 +- 26 files changed, 1715 insertions(+), 179 deletions(-) create mode 100644 src/BloomE2E/helpers/bloomLibraryAccount.ts create mode 100644 src/BloomE2E/helpers/bloomLibraryServer.ts create mode 100644 src/BloomE2E/helpers/bulkUpload.ts create mode 100644 src/BloomE2E/helpers/userSettings.ts create mode 100644 src/BloomE2E/tests/bulk-upload-quick-test.spec.ts create mode 100644 src/BloomE2E/tests/user-settings-isolation.spec.ts create mode 100644 src/BloomExe/BloomSettingsProvider.cs create mode 100644 src/BloomTests/BloomSettingsProviderTests.cs diff --git a/.github/skills/add-e2e-test/SKILL.md b/.github/skills/add-e2e-test/SKILL.md index c6f713010c4f..892c74b8c54d 100644 --- a/.github/skills/add-e2e-test/SKILL.md +++ b/.github/skills/add-e2e-test/SKILL.md @@ -191,10 +191,13 @@ The worker-scoped fixture launches Bloom on a temp copy of that collection and y - `page` — Playwright's `page`, overridden to be Bloom's shell document (the top bar and the showing tab). Most tests need nothing else. -- `bloomApp` — `{ page, httpPort, cdpPort, bloomPid, collectionDir, restart }`. Build book - paths from `collectionDir`, never from `output/testing-inputs`. `restart(callback)` +- `bloomApp` — `{ page, httpPort, cdpPort, bloomPid, collectionDir, userSettingsDir, restart }`. + Build book paths from `collectionDir`, never from `output/testing-inputs`. `restart(callback)` stops Bloom, runs the callback, and starts it again, which is how a test changes what - Bloom reads only at startup, such as the collection's languages. + Bloom reads only at startup, such as the collection's languages. `userSettingsDir` is where + this Bloom keeps its user settings (`user.config`): a folder of its own in the temp folder, + empty at launch, so a test's Bloom starts from default settings and shares none with the + developer's Bloom or the previous run. `helpers/userSettings.ts` reads what it saved there. The fixture also watches for the "Bloom had a problem" dialog and fails the test with the exception it scrapes from behind the dialog's own "Learn More" link. See diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 4e82412c6600..3ccdcd2b4129 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -302,6 +302,13 @@ jobs: # also work, but asking for it here says what the run needs instead of leaving # it to whatever the runner happens to do. BLOOM_AUTOMATION_MONITOR: headless + # The password of the Bloom Library test account (e2e-tester@example.org on + # dev.bloomlibrary.org), for the tests that sign in and upload for real. The + # account exists only for this; the email is a constant in the suite + # (src/BloomE2E/helpers/bloomLibraryAccount.ts). Locally the same variable is set + # at User scope; a developer without it has those tests skipped, while a run on CI + # without it fails, so a missing secret is noticed. + BLOOM_E2E_TESTER_EMAIL_BLORG_PASSWORD: ${{ secrets.BLOOM_E2E_TESTER_EMAIL_BLORG_PASSWORD }} run: pnpm test --reporter=list,junit,html # What explains an e2e failure: Playwright's HTML report, plus the trace and failure diff --git a/src/BloomE2E/AUTOMATION-DEBT.md b/src/BloomE2E/AUTOMATION-DEBT.md index ef43888e4f0d..65570b21411e 100644 --- a/src/BloomE2E/AUTOMATION-DEBT.md +++ b/src/BloomE2E/AUTOMATION-DEBT.md @@ -23,8 +23,7 @@ and ask its author. | --- | --- | --- | | #8299 | `BL-16799-page-change` | `editView/jumpToPage` refuses a jump it cannot do, and every page-changing helper waits for the Edit tab to settle. | | #8300 | `BL-16799-collection-languages` | The `e2e/setCollectionLanguages` hook, so no test composes `.bloomCollection` XML. | -| not yet open | `e2e-private-user-settings` | Every Bloom a test launches keeps its user settings in a folder of its own, named on the command line, so a run starts from defaults and its settings die with its temp folder. | -| not yet open | `e2e-real-library-login` | A test can sign in to dev.bloomlibrary.org for real, with a test account whose credentials the run supplies, so the upload cases can run to the end. Branches off the one above. | +| #8306 | `e2e-real-library-login` | A test can sign in to dev.bloomlibrary.org for real, with a test account whose credentials the run supplies, so the upload cases can run to the end. Branches off `e2e-private-user-settings`, which gave every Bloom a test launches a settings folder of its own and removed its entry from this file. | Three of these also add entries of their own, for the debt that is left after the fix. The stack replaces PR #8276, which did all of this at once. @@ -142,29 +141,19 @@ use. it, so it was left alone -- but it is the same shape, and worth remembering if a test ever finds that api reporting a stale subscription. -## The Bloom Library login cannot be done for real in a test - -Bloom's login state lives in machine-wide settings (`Settings.Default.WebUserId`), which an e2e -Bloom shares with the developer's own Bloom, and signing in goes out to an external browser with -real credentials. So a test can drive neither half of it: posting `account/logout` would sign the -developer out of their own Bloom, and `account/login` would sit waiting for a human. The e2e hook -`e2e/loginState` therefore makes Bloom *report* a login state without touching the real one, which -is enough for the gate the upload screen enforces (Upload is offered only to a signed-in user) but -covers neither the real sign-in and sign-out buttons nor anything that needs a real account — -which is every manual case that uploads for real (#204, #205, #211-#213, #215, #217, #218, #220), -so none of those can be automated either. Fix direction: a test account plus a per-instance login -store (a login the `--e2e` instance keeps to itself), so a run can sign in for real and upload to -dev.bloomlibrary.org without touching the developer's settings. The per-instance half of that is -the same fix "Every Bloom of one build shares one user.config" asks for, below; the test account -is the rest. Note that the pretense changes only what Bloom reports, so Bloom under `--e2e` now -refuses to upload at all rather than let an automated click publish under the developer's real -account. -(Found 2026-09-02 automating Test Case ID 606, `upload-required-items.spec.ts`.) - -being fixed on `e2e-real-library-login`, once `e2e-private-user-settings` (the user.config entry -below) has landed: with a settings folder of its own, a test's Bloom can be given a real test -account's login before it starts, and can sign out for real without touching anyone else's login. -The test account, and where its credentials live, are the rest of this branch. +## The Sign in button's trip through the system browser cannot be driven + +A test can now sign a Bloom in to dev.bloomlibrary.org for real (`signBloomIntoLibraryForReal`, +`helpers/bloomLibraryAccount.ts`): it signs the test account in the way the website does and posts +the result to `external/login`, the endpoint the website posts a browser login back to, and Bloom +keeps it in the run's own settings folder. What stays out of reach is the button itself: Sign in +opens the system browser on bloomlibrary.org's login page, and Playwright drives only Bloom's +WebView2, so the click, the browser round trip, and the hand-back to Bloom are never exercised. +Sign out is a plain button and could be, but no test does yet. Cost: the manual cases about the +sign-in and sign-out buttons stay manual; every case that merely needs to be signed in (the upload +cases) can run. Fix direction: none cheap. A Bloom-hosted login page under `--e2e` would test a +different flow from the one users have. +(Found 2026-09-04 automating Test Case ID 211, `bulk-upload-quick-test.spec.ts`.) ## Which front end the e2e suite tests depends on what else is running @@ -348,28 +337,6 @@ copy is a feature we want; if it is, put the page on the real clipboard, and giv fixture a way to run a second instance. (Found 2026-09-01 while automating Test Case ID 348.) -## Every Bloom of one build shares one user.config, so a run inherits another Bloom's settings - -Bloom keeps its user settings (UI language, page zoom, and the rest of `Settings.Default`) in -`%LOCALAPPDATA%\SIL\Bloom\\user.config`, one file per build version, and `--e2e` does -nothing to change that. So the Bloom a test launches starts from whatever the last Bloom of the -same version saved, and saves its own changes for the next one. The e2e lock keeps suites from -running at once, but a developer's own Bloom from a worktree of the same version is outside the -lock and shares the file all the same, and so does the previous run of any suite. - -Seen 2026-09-02 (Test Case ID 356, `format-gear-positioning.spec.ts`): two runs found every -factory template named in Turkish, then in French, and failed in `makeBookFromTemplate`, which -matches the English title; a Bloom nobody in the suite had started was running at the time, and -the file said `en` again a moment later. The same test has to restore the zoom it changes, because -that setting is shared too. Fix direction: under `--e2e`, point the settings provider at a -per-instance folder (a sibling of the temp collection would do), so a test's Bloom starts from -defaults and its changes die with it. - -being fixed on `e2e-private-user-settings`: a command-line argument names the folder Bloom keeps -its user settings in, and the launch fixture gives every Bloom it starts a folder inside the run's -temp folder, so the settings start from defaults, or from whatever the test puts there first, and -are deleted with the rest of the run. - ## No way to run the suite at a chosen monitor resolution and scale factor Every run takes the resolution and the scale factor of whatever monitor it lands on, so a @@ -401,8 +368,8 @@ Fix direction, cheapest first, none of it tried yet: driver is told to offer. Setting that monitor's *scale factor* is the harder half: Windows exposes per-monitor scale only through display-config calls Microsoft does not document. Worth an afternoon of investigation before committing to it. -- **A virtual machine or Windows Sandbox** at a chosen resolution and scale. Heaviest, but it - is the only one that also isolates the shared `user.config` described above. +- **A virtual machine or Windows Sandbox** at a chosen resolution and scale. Heaviest, and the + only one that would also isolate everything else on the machine a run could inherit. Whatever the mechanism, the suite needs the same thing from it: a way to say "run these tests at 1920x1080 at 150%" and have the run either honour it or refuse, rather than silently using diff --git a/src/BloomE2E/README.md b/src/BloomE2E/README.md index 6bf72578c8f3..940cf276228f 100644 --- a/src/BloomE2E/README.md +++ b/src/BloomE2E/README.md @@ -69,8 +69,20 @@ and whatever tab is showing. Most tests need nothing else. | `cdpPort` | The port the embedded WebView2 answers CDP on. | | `bloomPid` | The process id of the Bloom serving this collection. | | `collectionDir` | The temp copy of the collection. Build book paths from this, never from `output/testing-inputs`. | +| `userSettingsDir` | The folder this Bloom keeps its user settings in (its `user.config`), beside the collection in the temp folder. | | `restart` | Stop Bloom, run an optional callback, start it again on the same collection, and return the new page. | +Every Bloom the fixture launches keeps its user settings, the contents of `user.config` (UI +language, page zoom, the Bloom Library login, and the rest of `Settings.Default`), in +`userSettingsDir`, passed to Bloom as `--user-settings-folder`. Every Bloom of one build otherwise +shares one `user.config` in `%LOCALAPPDATA%\SIL\Bloom\`, so a run would start from +whatever the developer's Bloom, or the previous run, saved last, and leave its own changes behind +for them. Instead the folder starts empty, so Bloom starts from default settings, and it is deleted +with the rest of the run. A restart keeps it, so what one launch saved the next one reads, as on a +real machine. `helpers/userSettings.ts` reads what Bloom has saved there. The fixture checks that +the Bloom it started really is using the folder, so a stale `Bloom.exe` fails the launch rather +than quietly sharing settings. + `restart(betweenStopAndStart)` is how a test changes something Bloom only reads at startup. The collection's languages are the case that needed it: the collection Settings dialog is a WinForms surface CDP cannot reach, and `collectionSettings/changeLanguage` only answers while that dialog @@ -112,6 +124,8 @@ real bug in the code under test; read the message and fix it rather than working - `helpers/addPageDialog.ts` — open, read, scroll and close the real Add Page dialog, and add a page through it. Tests that only need a page in their book call `addPage` instead. - `helpers/files.ts` — `fingerprintFolder`, `isInsideFolder`, for checks against the disk. +- `helpers/userSettings.ts` — `getUserSettingsFolder`, `readSavedUserSetting`: where the Bloom + under test keeps its user settings, and what it has saved there. - `helpers/api.ts` — `apiGet`, `apiPost`, `apiGetJson`. These run `fetch` inside the page with a relative URL, which is not a style choice: Bloom's server rejects a `127.0.0.1` Host header, and the CDP endpoint does not answer on `localhost`. The file explains it. diff --git a/src/BloomE2E/fixtures/bloomTest.ts b/src/BloomE2E/fixtures/bloomTest.ts index 2404dcec276d..e67e030a1ad7 100644 --- a/src/BloomE2E/fixtures/bloomTest.ts +++ b/src/BloomE2E/fixtures/bloomTest.ts @@ -44,6 +44,12 @@ export interface IBloomApp { bloomPid: number; /** The collection folder Bloom has open: a temp folder, never the inputs repository itself. */ collectionDir: string; + /** + * The folder this Bloom keeps its user settings in (its user.config), beside the collection in + * the temp folder, so nothing it saves reaches the developer's own Bloom or the next run. It + * starts empty, and a restart keeps it. See helpers/userSettings.ts to read what is in it. + */ + userSettingsDir: string; /** * Quit Bloom and start it again on the same collection folder, and return the new shell page. * @@ -256,6 +262,7 @@ export const test = base.extend({ cdpPort: launched.cdpPort, bloomPid: launched.bloomPid, collectionDir: launched.collectionDir, + userSettingsDir: launched.userSettingsDir, restart: async (betweenStopAndStart) => { // Close the old CDP connection first: it holds a socket into the process // that is about to be killed. diff --git a/src/BloomE2E/fixtures/launchBloom.ts b/src/BloomE2E/fixtures/launchBloom.ts index de82593169d4..25e247769342 100644 --- a/src/BloomE2E/fixtures/launchBloom.ts +++ b/src/BloomE2E/fixtures/launchBloom.ts @@ -12,6 +12,12 @@ // 3. Discovery matches on the OPEN COLLECTION FOLDER, not on a port. Bloom takes the next free // port block, and a developer's own Bloom may already hold 8089, so the folder is the only // reliable way to tell our instance from theirs. +// 4. Every Bloom we launch keeps its user settings (user.config: UI language, page zoom, the Bloom +// Library login, and the rest of Settings.Default) in a folder of its own inside the temp +// folder, passed as --user-settings-folder. Every Bloom of one build otherwise shares one +// user.config, so a run would start from whatever the developer's Bloom, or the previous run, +// saved last, and leave its own changes behind for them. This way it starts from defaults, or +// from whatever a test puts in the folder first, and its settings die with the temp folder. // // Nothing here knows about Playwright; fixtures/bloomTest.ts adds the CDP attachment on top. @@ -31,6 +37,12 @@ export interface ILaunchedBloom { bloomPid: number; /** The temp copy of the collection folder that this Bloom has open. */ collectionDir: string; + /** + * The folder this Bloom keeps its user settings in (its user.config), a sibling of the + * collection in the temp folder. It starts empty, so Bloom starts from default settings; a + * restart keeps it, so what one launch saved the next one reads, as on a real machine. + */ + userSettingsDir: string; /** Kill the process tree, confirm the HTTP port went dark, and delete the temp copy. */ stop: () => Promise; /** @@ -66,6 +78,12 @@ export interface ICollectionSpec { * feature. */ subscriptionCode?: string; + /** + * The Bloom Library bookshelf every book of the collection is uploaded to, by its url key, e.g. + * "test-bookshelf-1" (see kTestBookshelves). Bloom honours it only under an enterprise + * subscription whose bookshelves include it, which is what the Settings dialog offers a person. + */ + bookshelf?: string; } /** Options for launchBloom. Give exactly one of collectionName and collectionSpec. */ @@ -376,11 +394,16 @@ const delay = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); * window IS visible. */ function environmentForBloom(): NodeJS.ProcessEnv { + // Every Bloom a test launches talks to the sandbox, dev.bloomlibrary.org, never to + // bloomlibrary.org: a test signs in there with a test account and uploads for real. A Debug + // Bloom uses the sandbox anyway; a Release Bloom, which is what CI builds, uses bloomlibrary.org + // unless this variable says otherwise (see BookUpload.UseSandboxWithoutUserChoice). + const env: NodeJS.ProcessEnv = { ...process.env, BloomSandbox: "true" }; const asked = process.env.BLOOM_AUTOMATION_MONITOR?.trim().toLowerCase(); if (process.env.PWDEBUG && (asked === "headless" || asked === "0")) { - return { ...process.env, BLOOM_AUTOMATION_MONITOR: "" }; + env.BLOOM_AUTOMATION_MONITOR = ""; } - return process.env; + return env; } /** @@ -416,6 +439,8 @@ interface IInstanceInfo { editableCollectionFolder?: string; processId?: number; cdpPort?: number; + /** Where this Bloom keeps its user settings; absent from a Bloom built before it reported this. */ + userSettingsFolder?: string; } /** @@ -483,12 +508,20 @@ export function writeNewCollection( fs.mkdirSync(collectionDir, { recursive: true }); fs.writeFileSync( Path.join(collectionDir, `${spec.name}.bloomCollection`), - makeCollectionXml(spec.languages, "Factory", spec.subscriptionCode), + makeCollectionXml(spec.languages, "Factory", spec), "utf8", ); return collectionDir; } +/** The settings makeCollectionXml writes beyond the languages and the front/back matter pack. */ +export interface ICollectionXmlExtras { + /** See ICollectionSpec.subscriptionCode. */ + subscriptionCode?: string; + /** See ICollectionSpec.bookshelf. */ + bookshelf?: string; +} + /** * The XML of a .bloomCollection with these languages and this front/back matter pack. Exported * because a test changes collection settings by rewriting this file between a stop and a start @@ -498,14 +531,16 @@ export function writeNewCollection( * pack the Settings dialog calls Paper Saver), "Traditional", "SuperPaperSaver", "Device", * "SIL-PNG". The default is Factory, which is what the collections here have always had. * - * `subscriptionCode` is written only when given. It is the collection's subscription, and so its - * tier: Bloom parses the tier out of the code as it opens the collection. See - * kEnterpriseSubscriptionCode in helpers/collectionSettings.ts. + * `extras.subscriptionCode` is written only when given. It is the collection's subscription, and + * so its tier: Bloom parses the tier out of the code as it opens the collection (since Bloom 6.1 + * from SubscriptionCode alone; BrandingProjectName is written for older Blooms). See + * kEnterpriseSubscriptionCode in helpers/collectionSettings.ts. `extras.bookshelf`, also written + * only when given, becomes a "bookshelf:" tag in DefaultBookTags, which is how Bloom keeps it. */ export function makeCollectionXml( languages: string[], xmatterPack = "Factory", - subscriptionCode?: string, + extras: ICollectionXmlExtras = {}, ): string { // Bloom treats Language2 as "same as Language1" when a collection names only one language, // which is what its own new-collection code writes. @@ -526,8 +561,11 @@ export function makeCollectionXml( languageElements + `\n ${xmatterPack}\n` + ` Default\n` + - (subscriptionCode - ? ` ${subscriptionCode}\n` + (extras.subscriptionCode + ? ` ${extras.subscriptionCode}\n` + : "") + + (extras.bookshelf + ? ` bookshelf:${extras.bookshelf}\n` : "") + ` True\n` + ` Decimal\n` + @@ -606,6 +644,7 @@ function isPid(pid: number | undefined): pid is number { */ async function startBloomOn( collectionDir: string, + userSettingsDir: string, readyTimeoutMs: number, experimentalFeatures?: string[], ): Promise { @@ -637,6 +676,8 @@ async function startBloomOn( // tree rather than a stale output/browser (see getViteDevPort). const vitePort = getViteDevPort(); if (vitePort) args.push("--vite-port", vitePort); + // --user-settings-folder: keep this Bloom's user settings to itself (point 4 at the top). + args.push("--user-settings-folder", userSettingsDir); // --experimental-features: turn these on for this Bloom alone, without touching the saved // setting the developer's own Bloom shares (see ILaunchBloomOptions.experimentalFeatures). if (experimentalFeatures?.length) @@ -706,6 +747,21 @@ async function startBloomOn( ); } + // A Bloom that is not keeping its settings where we said would share them with the developer's + // own Bloom, which is the very thing the folder prevents; a Bloom.exe built before the argument + // existed reports no folder at all. Either way, no test can be trusted, so stop here. + if ( + !found.info.userSettingsFolder || + !samePath(found.info.userSettingsFolder, userSettingsDir) + ) { + killProcessTree([bloomProcess.pid, found.info.processId].filter(isPid)); + throw new Error( + `Bloom was asked to keep its user settings in ${userSettingsDir} but reports ` + + `${found.info.userSettingsFolder ?? "no user settings folder"}. ` + + `Is ${exe} built from current sources?`, + ); + } + return { httpPort: found.httpPort, cdpPort: found.info.cdpPort, @@ -756,10 +812,13 @@ export async function launchBloom( ); let collectionDir: string; + // Empty, so this Bloom starts from default settings (point 4 at the top). Deleted with tempRoot. + const userSettingsDir = Path.join(tempRoot, "user-settings"); try { collectionDir = options.collectionSpec ? writeNewCollection(tempRoot, options.collectionSpec) : copyPreparedCollection(tempRoot, options.collectionName!); + fs.mkdirSync(userSettingsDir); } catch (error) { fs.rmSync(tempRoot, { recursive: true, force: true }); throw error; @@ -782,6 +841,7 @@ export async function launchBloom( try { running = await startBloomOn( collectionDir, + userSettingsDir, readyTimeoutMs, options.experimentalFeatures, ); @@ -796,6 +856,7 @@ export async function launchBloom( cdpPort: running.cdpPort, bloomPid: running.servingPid, collectionDir, + userSettingsDir, restart: async (betweenStopAndStart) => { await killAndWaitForPortToGoDark(running!); @@ -805,6 +866,7 @@ export async function launchBloom( if (betweenStopAndStart) await betweenStopAndStart(); running = await startBloomOn( collectionDir, + userSettingsDir, readyTimeoutMs, options.experimentalFeatures, ); diff --git a/src/BloomE2E/helpers/bloomLibraryAccount.ts b/src/BloomE2E/helpers/bloomLibraryAccount.ts new file mode 100644 index 000000000000..460257172330 --- /dev/null +++ b/src/BloomE2E/helpers/bloomLibraryAccount.ts @@ -0,0 +1,176 @@ +// Sign a test's Bloom in to dev.bloomlibrary.org for real, as a dedicated test account. +// +// Why this is possible now: every Bloom a test launches keeps its user settings, the Bloom Library +// login included, in a folder of its own (bloomApp.userSettingsDir; see fixtures/launchBloom.ts). +// So a test can sign a real account in without touching the developer's own Bloom, and Bloom's +// bulk-upload child process, which reads the login back from that folder, uploads as the test +// account rather than as the developer. (Before that, the login lived in settings shared with the +// developer's Bloom, so no test could sign in for real, and the upload cases stayed manual.) +// +// The sign-in goes straight to the endpoint the website posts a browser login back to, not through +// Bloom's Sign in button: that button opens the system browser, which no test can drive. See +// AUTOMATION-DEBT.md, "The Sign in button's trip through the system browser cannot be driven". +// +// The account and its password: the email is a constant here, because it is not secret and a reader +// should see which account a test signs in as (an assertion can check it too — the Sign out button +// carries it). Only the password is kept out of the source, in the BLOOM_E2E_TESTER_EMAIL_BLORG_PASSWORD +// environment variable (User scope on a developer's machine, a repository secret in CI). A run +// without it skips the tests that need it locally, and fails in CI, so a missing secret is noticed. +// +// What this file does NOT do: reach the server to read or delete books. That is bloomLibraryServer.ts, +// which owns the sandbox back ends. This file borrows that module's login type and its parse-server +// address, so there is one description of the sandbox, not two. + +import { test, type Page } from "@playwright/test"; +import { apiPost } from "./api"; +import { + DEV_PARSE_APPLICATION_ID, + DEV_PARSE_SERVER_URL, + type IBloomLibraryLogin, +} from "./bloomLibraryServer"; + +/** + * The test account every real-login test signs in as. Not secret: it is here so a reader can see + * which account a test uses, and an assertion can check it. Only the password is kept out of the + * source (see PASSWORD_ENV_VAR). The account exists only for these tests, on dev.bloomlibrary.org. + */ +export const TEST_ACCOUNT_EMAIL = "e2e-tester@example.org"; + +/** The environment variable that carries the test account's dev.bloomlibrary.org password. */ +export const PASSWORD_ENV_VAR = "BLOOM_E2E_TESTER_EMAIL_BLORG_PASSWORD"; + +// dev.bloomlibrary.org's Firebase auth, which the website uses to sign a user in before exchanging +// the result for a parse-server session. The key ships in the website's JS bundle, so it is not a +// secret. The Firebase project is shared with production; it is the parse server (dev, from +// bloomLibraryServer.ts) that makes this a sandbox login. +const FIREBASE_API_KEY = "AIzaSyACJ7fi7_Rg_bFgTIacZef6OQckr6QKoTY"; + +/** + * Whether the test account's password is available. When it is not, a real-login test cannot run. + * See skipIfNoLibraryPassword, which is how a test acts on this. + */ +export function isLibraryPasswordConfigured(): boolean { + return !!process.env[PASSWORD_ENV_VAR]; +} + +/** + * Skip the current test when the test account's password is not set — but only off CI. On CI a + * missing password is a broken secret, not a developer who has not set one up, so there the test is + * left to fail rather than quietly skipped. Call at the top of a test that signs in for real. + */ +export function skipIfNoLibraryPassword(): void { + if (isLibraryPasswordConfigured()) return; + if (process.env.CI) + throw new Error( + `${PASSWORD_ENV_VAR} is not set, so this run cannot sign in to dev.bloomlibrary.org as ` + + `${TEST_ACCOUNT_EMAIL}. On CI this is a missing repository secret; add it.`, + ); + test.skip( + true, + `${PASSWORD_ENV_VAR} is not set. Set it (User environment scope) from the team password ` + + `manager to run the tests that sign in to dev.bloomlibrary.org as ${TEST_ACCOUNT_EMAIL}.`, + ); +} + +/** + * Sign the test account in to dev.bloomlibrary.org and return the login, doing exactly what the + * website's login-for-editor page does: authenticate to Firebase with the email and password, then + * exchange the Firebase token for a parse-server session (the bloomLink cloud function links or + * creates the parse user, then a users POST logs in). Throws with a diagnostic naming the step that + * failed. The account's email must be verified, or the parse server refuses it. + */ +export async function getDevBloomLibraryLogin(): Promise { + const password = process.env[PASSWORD_ENV_VAR]; + if (!password) + throw new Error( + `${PASSWORD_ENV_VAR} is not set; call skipIfNoLibraryPassword first.`, + ); + + const firebase = await postJson( + `https://identitytoolkit.googleapis.com/v1/accounts:signInWithPassword?key=${FIREBASE_API_KEY}`, + { email: TEST_ACCOUNT_EMAIL, password, returnSecureToken: true }, + "Firebase sign-in", + ); + + // bloomLink links the Firebase user to a parse user (creating one if needed) before the users + // POST logs in; the website calls it first for the same reason. + await postJson( + `${DEV_PARSE_SERVER_URL}/functions/bloomLink`, + { token: firebase.idToken, id: TEST_ACCOUNT_EMAIL }, + "parse-server bloomLink", + { "X-Parse-Application-Id": DEV_PARSE_APPLICATION_ID }, + ); + const user = await postJson( + `${DEV_PARSE_SERVER_URL}/users`, + { + authData: { + bloom: { token: firebase.idToken, id: TEST_ACCOUNT_EMAIL }, + }, + username: TEST_ACCOUNT_EMAIL, + email: TEST_ACCOUNT_EMAIL, + }, + "parse-server login", + { "X-Parse-Application-Id": DEV_PARSE_APPLICATION_ID }, + ); + if (!user.sessionToken || !user.objectId) + throw new Error( + `parse-server login for ${TEST_ACCOUNT_EMAIL} returned no session token or user id: ` + + JSON.stringify(user), + ); + return { + email: TEST_ACCOUNT_EMAIL, + userId: user.objectId, + sessionToken: user.sessionToken, + }; +} + +/** + * Sign the running Bloom in to Bloom Library as the test account, for real, by posting the login to + * external/login — the same endpoint the website posts to after a browser login. Bloom saves it in + * its own user-settings folder, so its bulk-upload child process signs in as this account too, and + * the top bar shows the account signed in. Returns the login, which the caller keeps so it can + * delete afterwards what it uploads (see bloomLibraryServer.ts). Uploads go only to the sandbox. + */ +export async function signBloomIntoLibraryForReal( + page: Page, +): Promise { + const login = await getDevBloomLibraryLogin(); + await apiPost( + page, + "external/login", + JSON.stringify({ + sessionToken: login.sessionToken, + email: login.email, + userId: login.userId, + }), + "application/json", + ); + return login; +} + +/** POST JSON and parse the JSON reply, throwing a diagnostic that names the step on any failure. */ +async function postJson( + url: string, + body: unknown, + what: string, + extraHeaders: Record = {}, +): Promise { + let response: Response; + try { + response = await fetch(url, { + method: "POST", + headers: { "Content-Type": "application/json", ...extraHeaders }, + body: JSON.stringify(body), + }); + } catch (error) { + throw new Error(`${what} could not reach ${url}: ${error}`); + } + const text = await response.text(); + if (!response.ok) + throw new Error(`${what} failed (${response.status}): ${text}`); + try { + return JSON.parse(text); + } catch { + throw new Error(`${what} did not return JSON: ${text}`); + } +} diff --git a/src/BloomE2E/helpers/bloomLibraryServer.ts b/src/BloomE2E/helpers/bloomLibraryServer.ts new file mode 100644 index 000000000000..98489084d2ae --- /dev/null +++ b/src/BloomE2E/helpers/bloomLibraryServer.ts @@ -0,0 +1,212 @@ +// Talk to the Bloom Library SANDBOX, dev.bloomlibrary.org, from a test: which books are there, +// and deleting the ones a test uploaded. Nothing here talks to Bloom, and nothing here can reach +// bloomlibrary.org itself: every address below is the sandbox's. +// +// The sandbox has two back ends, the same two the website talks to (see BloomLibrary2's +// src/connection): a parse-server, which holds the book records and answers anonymous reads with +// the application id below, and the Bloom Library API, which takes the writes and wants the +// signed-in user's parse session token. Both ids here are public; the website ships them. +// +// The files of an uploaded book sit in the sandbox's public S3 bucket, at the record's baseUrl; a +// test reads the uploaded HTML from there to see what Bloom actually sent. + +import { xmatterPackInBookHtml } from "./bookHtml"; + +/** The sandbox parse-server, where the book records live. */ +export const DEV_PARSE_SERVER_URL = "https://dev-server.bloomlibrary.org/parse"; +export const DEV_PARSE_APPLICATION_ID = + "yrXftBF6mbAuVu3fO6LnhCJiHxZPIdE7gl1DUVGR"; + +/** The Bloom Library API; "env=dev" on every request keeps it to the sandbox. */ +const BLOOM_LIBRARY_API_URL = "https://api.bloomlibrary.org/v1"; + +/** A signed-in Bloom Library user, as the parse-server describes one. */ +export interface IBloomLibraryLogin { + email: string; + /** The parse-server's id for the user; Bloom keeps it as LastLoginUserId. */ + userId: string; + /** The parse session token; Bloom keeps it as LastLoginSessionToken and sends it with uploads. */ + sessionToken: string; +} + +/** One book record on the sandbox. Only the fields a test reads. */ +export interface IBookOnServer { + /** The record's own id, which the API's delete route takes. */ + objectId: string; + /** The book's id inside Bloom (meta.json's bookInstanceId), which uploads are matched on. */ + bookInstanceId: string; + title: string; + /** Bloom's tags, e.g. "bookshelf:test-bookshelf-1". */ + tags: string[]; + /** The bookshelves the book sits on, by url key: the "bookshelf:" tags without their prefix. */ + bookshelves: string[]; + /** The email of the account that uploaded it. */ + uploaderEmail: string | undefined; + /** + * Where the book's files are on S3, as Bloom wrote it (BloomS3Client.GetBaseUrl): URL-encoded + * with the slashes as %2f and spaces as +. Ends with the book folder's name. + */ + baseUrl: string; +} + +const parseHeaders = { + "X-Parse-Application-Id": DEV_PARSE_APPLICATION_ID, + "Content-Type": "application/json", +}; + +/** + * Query the sandbox's book records with this parse "where" clause, as the signed-in user when a + * login is given (the website sends the session token for a user's own records, so a test does + * too). Returns the records in no particular order. + */ +async function queryBooksOnDevServer( + where: object, + describe: string, + login?: IBloomLibraryLogin, +): Promise { + const url = + `${DEV_PARSE_SERVER_URL}/classes/books?where=${encodeURIComponent(JSON.stringify(where))}` + + `&include=uploader&keys=title,bookInstanceId,tags,uploader,baseUrl&limit=1000`; + const headers = login + ? { ...parseHeaders, "X-Parse-Session-Token": login.sessionToken } + : parseHeaders; + const response = await fetch(url, { headers }); + if (!response.ok) + throw new Error( + `The sandbox parse-server answered ${response.status} to a query for ${describe}: ${await response.text()}`, + ); + const body = (await response.json()) as { + results: { + objectId: string; + bookInstanceId: string; + title: string; + tags?: string[]; + uploader?: { email?: string }; + baseUrl: string; + }[]; + }; + const bookshelfPrefix = "bookshelf:"; + return body.results.map((r) => ({ + objectId: r.objectId, + bookInstanceId: r.bookInstanceId, + title: r.title, + tags: r.tags ?? [], + bookshelves: (r.tags ?? []) + .filter((tag) => tag.startsWith(bookshelfPrefix)) + .map((tag) => tag.substring(bookshelfPrefix.length)), + uploaderEmail: r.uploader?.email, + baseUrl: r.baseUrl, + })); +} + +/** + * The book records on the sandbox for these book instance ids, in no particular order. A book that + * is not there is simply absent from the result, so a test compares lengths. + */ +export async function findBooksOnDevServer( + bookInstanceIds: string[], +): Promise { + return queryBooksOnDevServer( + { bookInstanceId: { $in: bookInstanceIds } }, + "books", + ); +} + +/** + * Every book the signed-in account has on the sandbox, whatever a test named them. Cleanup uses + * this so a run can delete not only the books it just uploaded but any a crashed earlier run left + * behind, which is safe because the account exists only for these tests. + */ +export async function findBooksUploadedBy( + login: IBloomLibraryLogin, +): Promise { + return queryBooksOnDevServer( + { + uploader: { + __type: "Pointer", + className: "_User", + objectId: login.userId, + }, + }, + "the account's books", + login, + ); +} + +/** + * The HTML of a book as it was uploaded, read from the sandbox's S3 bucket. Bloom uploads the book + * folder under the record's baseUrl and names the .htm after the folder, so this reads + * `.htm`. The bucket is public-read; that is how the website shows books. + */ +export async function fetchUploadedBookHtml( + book: IBookOnServer, +): Promise { + // Bloom writes baseUrl with HttpUtility.UrlEncode, which puts spaces as "+", so undo that + // before the general decoding turns the %2f slashes (and any %2b plus) back into themselves. + const folderPath = decodeURIComponent(book.baseUrl.replace(/\+/g, " ")); + const folderName = folderPath.split("/").filter(Boolean).pop()!; + // The URL constructor re-encodes the spaces and the rest of the path for the request. + const url = + new URL(folderPath).href + encodeURIComponent(folderName) + ".htm"; + const response = await fetch(url); + if (!response.ok) + throw new Error( + `S3 answered ${response.status} for the uploaded HTML of "${book.title}" at ${url}`, + ); + return response.text(); +} + +/** + * The front/back matter pack the uploaded copy of this book carries (see xmatterPackInBookHtml): + * how a test sees that an upload sent the book with the collection's current pack. + */ +export async function getXmatterPackOfBookOnServer( + book: IBookOnServer, +): Promise { + return xmatterPackInBookHtml(await fetchUploadedBookHtml(book)); +} + +/** + * Delete every book the signed-in account has on the sandbox, and return how many. The safety net a + * real-upload test calls in cleanup, so nothing it uploaded (or a crashed run uploaded before it) + * is left on dev.bloomlibrary.org. + */ +export async function deleteAllBooksUploadedBy( + login: IBloomLibraryLogin, +): Promise { + const books = await findBooksUploadedBy(login); + for (const book of books) await deleteBookFromDevServer(book, login); + return books.length; +} + +/** + * Delete one book from the sandbox, the way the website's Delete button does (BloomLibrary2's + * deleteBook): through the Bloom Library API, as the signed-in user, who must be its uploader. + * Returns once the parse-server no longer lists the record. + */ +export async function deleteBookFromDevServer( + book: IBookOnServer, + login: IBloomLibraryLogin, +): Promise { + const response = await fetch( + `${BLOOM_LIBRARY_API_URL}/books/${book.objectId}?env=dev`, + { + method: "DELETE", + headers: { "Authentication-Token": login.sessionToken }, + }, + ); + if (!response.ok) + throw new Error( + `The Bloom Library API answered ${response.status} to deleting book ${book.objectId} ` + + `("${book.title}") from the sandbox: ${await response.text()}`, + ); + const deadline = Date.now() + 30000; + while (Date.now() < deadline) { + const left = await findBooksOnDevServer([book.bookInstanceId]); + if (!left.some((b) => b.objectId === book.objectId)) return; + await new Promise((resolve) => setTimeout(resolve, 1000)); + } + throw new Error( + `Book ${book.objectId} ("${book.title}") is still on the sandbox 30 seconds after the API accepted its deletion.`, + ); +} diff --git a/src/BloomE2E/helpers/bookHtml.ts b/src/BloomE2E/helpers/bookHtml.ts index 197ff9ba6034..1cbd447f4918 100644 --- a/src/BloomE2E/helpers/bookHtml.ts +++ b/src/BloomE2E/helpers/bookHtml.ts @@ -153,3 +153,35 @@ export function bookFileExists( const file = relativePath.split("#")[0]; return fs.existsSync(Path.join(bookFolder, file)); } + +/** + * The front/back matter pack a book's HTML carries: the key before "-XMatter" in the one pack + * stylesheet its head links, e.g. "Factory" or "Traditional". Bloom links exactly one such + * stylesheet per book (XMatterHelper.GetStyleSheetFileName) and swaps it when it brings the book up + * to date under a collection whose pack changed, so this says which pack the book was last brought + * up to date with. It is the same answer for a book on disk and for the copy an upload sent to + * Bloom Library, which is why it takes the HTML rather than a path. Throws, naming the stylesheets + * it did find, when the HTML links no pack or more than one. + */ +export function xmatterPackInBookHtml(html: string): string { + const hrefs = [...html.matchAll(/]*\shref="([^"]+)"/g)].map( + (match) => match[1], + ); + const packs = hrefs.flatMap((href) => { + const match = /^(.+)-XMatter\.css$/.exec(href); + return match ? [match[1]] : []; + }); + if (packs.length !== 1) + throw new Error( + `Expected the book's HTML to link exactly one -XMatter.css, found ${packs.length}. ` + + `Stylesheets linked: ${hrefs.join(", ")}`, + ); + return packs[0]; +} + +/** The front/back matter pack of the book saved in `bookFolder`; see xmatterPackInBookHtml. */ +export function readXmatterPackOfBook(bookFolder: string): string { + return xmatterPackInBookHtml( + fs.readFileSync(bookHtmlPath(bookFolder), "utf8"), + ); +} diff --git a/src/BloomE2E/helpers/bulkUpload.ts b/src/BloomE2E/helpers/bulkUpload.ts new file mode 100644 index 000000000000..3eed7ca61306 --- /dev/null +++ b/src/BloomE2E/helpers/bulkUpload.ts @@ -0,0 +1,144 @@ +// Drive Publish: Web's "Upload this collection" bulk upload, and read what it did. +// +// Bulk upload is the split button beside "Upload Book" on the Publish: Web screen: its dropdown +// offers "Upload this collection" and "Upload folder of collections". Picking one sets it as the +// button's action; then the button runs it (see bloomSplitButton.tsx). The button shows only to a +// signed-in user and is enabled only when the selected book is ready to upload and the agreements +// are ticked, which is the same gate a single upload passes. +// +// The upload itself runs in a second Bloom that Bloom starts (BloomLibraryPublishModel.BulkUpload), +// so the result does not come back through the screen. That second Bloom writes BloomBulkUploadLog.txt +// into the collection folder, and its last lines say how many books were uploaded, updated and +// skipped. A test reads that, which is also how a person reads a bulk upload's result. + +import { expect, type Page } from "@playwright/test"; +import * as fs from "node:fs"; +import * as Path from "node:path"; +import { acceptAllAgreements, openPublishToWeb } from "./libraryPublish"; + +/** The name of the log the bulk-upload child process writes into the collection folder. */ +const BULK_UPLOAD_LOG = "BloomBulkUploadLog.txt"; + +/** What one bulk upload did, read from its log's final tally. */ +export interface IBulkUploadResult { + /** Books uploaded for the first time. */ + newBooks: number; + /** Books that had changed and were re-uploaded. */ + updated: number; + /** Books skipped because nothing had changed since the last upload. */ + skipped: number; + /** The whole log, for a failure message when the tally is not what a test expected. */ + log: string; +} + +/** The path of the bulk-upload log in a collection folder. */ +function logPath(collectionDir: string): string { + return Path.join(collectionDir, BULK_UPLOAD_LOG); +} + +/** + * Remove the bulk-upload log, so the next upload's result is read fresh rather than from a tally an + * earlier round left. Call before each upload; each round writes the whole log again. + */ +export function clearBulkUploadLog(collectionDir: string): void { + fs.rmSync(logPath(collectionDir), { force: true }); +} + +/** + * Start a bulk upload of the whole collection through the split button, the way a person does: + * open the dropdown beside Upload Book, pick "Upload this collection", and click the button. A book + * must already be selected, the user signed in, and the agreements ticked, or the button is + * disabled. This does not wait for the upload to finish; see waitForBulkUploadResult. + */ +export async function startCollectionUpload(page: Page): Promise { + const buttons = page.getByTestId("upload-buttons"); + // Open the dropdown. The arrow button is the split button's second button. + await buttons.getByRole("button", { name: "select upload source" }).click(); + // The menu is rendered at the document root, not inside upload-buttons. The label is + // "Upload this Collection"; match case-insensitively so a capitalization tweak does not break it. + await page + .getByRole("menuitem", { name: /upload this collection/i }) + .click(); + // Picking only selected it; the primary button runs it. + const primary = buttons.getByRole("button", { + name: /upload this collection/i, + }); + await expect(primary).toBeEnabled({ timeout: 15000 }); + await primary.click(); +} + +/** + * Wait until the bulk-upload child process has finished and return its tally. It writes the log as + * it goes and ends with the three counts, so this polls for the "Skipped ... books" line — the last + * of the three — then reads all three. Throws with the log when it does not finish in time. + */ +export async function waitForBulkUploadResult( + collectionDir: string, + timeoutMs = 180000, +): Promise { + const file = logPath(collectionDir); + const skippedLine = /Skipped (\d+) books/; + await expect + .poll( + () => (fs.existsSync(file) ? fs.readFileSync(file, "utf8") : ""), + { + timeout: timeoutMs, + message: `The bulk upload never finished: ${file} did not report a final tally.`, + }, + ) + .toMatch(skippedLine); + + const log = fs.readFileSync(file, "utf8"); + const count = (pattern: RegExp): number => { + const match = log.match(pattern); + if (!match) + throw new Error( + `The bulk-upload log did not report "${pattern.source}". Log:\n${log}`, + ); + return Number(match[1]); + }; + return { + newBooks: count(/Uploaded (\d+) new books/), + updated: count(/Updated (\d+) books/), + skipped: count(/Skipped (\d+) books/), + log, + }; +} + +/** + * Upload the whole collection and wait for its result, the way a person does from a selected book: + * go to Publish: Web, tick the agreements, run "Upload this collection", and read the tally the + * child Bloom wrote. A book must be selected and the user signed in. Coming back to Publish: Web + * and ticking already-ticked agreements is harmless, so a test calls this for every upload round, + * wherever it left Bloom in between. + */ +export async function uploadCollection( + page: Page, + collectionDir: string, +): Promise { + await openPublishToWeb(page); + await acceptAllAgreements(page); + clearBulkUploadLog(collectionDir); + await startCollectionUpload(page); + return waitForBulkUploadResult(collectionDir); +} + +/** + * Try to upload the collection with no bookshelf set, and return the message Bloom refuses with. + * Bloom will not bulk-upload a collection that has no Bloom Library bookshelf; it tells the user to + * set one. The message goes to the Upload screen's progress box, which is what this reads. + */ +export async function uploadCollectionExpectingBookshelfWarning( + page: Page, +): Promise { + await startCollectionUpload(page); + const progress = page.getByTestId("progress-box-log"); + await expect(progress).toContainText("bookshelf", { + ignoreCase: true, + timeout: 30000, + }); + return (await progress.innerText()).trim(); +} + +/** Re-export so a test opens the Web screen and drives bulk upload from one import. */ +export { openPublishToWeb }; diff --git a/src/BloomE2E/helpers/collectionSettings.ts b/src/BloomE2E/helpers/collectionSettings.ts index d10784f48332..081053652973 100644 --- a/src/BloomE2E/helpers/collectionSettings.ts +++ b/src/BloomE2E/helpers/collectionSettings.ts @@ -29,6 +29,8 @@ export interface ICollectionSettings { * Left out, the collection has no code and so is Basic. */ subscriptionCode?: string; + /** The Bloom Library bookshelf, by url key; see ICollectionSpec.bookshelf. Left out, none. */ + bookshelf?: string; } /** @@ -66,7 +68,7 @@ export async function restartWithCollectionSettings( makeCollectionXml( settings.languages, settings.xmatterPack, - settings.subscriptionCode, + settings, ), "utf8", ), @@ -113,6 +115,13 @@ export type SubscriptionTier = */ export const kEnterpriseSubscriptionCode = "Test-727011-1339"; +/** + * The bookshelves the Test subscription (kEnterpriseSubscriptionCode) owns on Bloom Library, by + * url key, which Contentful knows about. A collection under that code can name either as the + * bookshelf its books are uploaded to, as the Settings dialog offers a person. + */ +export const kTestBookshelves = ["test-bookshelf-1", "test-bookshelf-2"]; + /** * What Bloom says about one feature, as features/status reports it. It answers with more than * this, all of it about how to word the message offering an upgrade; these are the fields that say diff --git a/src/BloomE2E/helpers/copyrightAndLicense.ts b/src/BloomE2E/helpers/copyrightAndLicense.ts index 7cfb74d262e1..6796e18e9e45 100644 --- a/src/BloomE2E/helpers/copyrightAndLicense.ts +++ b/src/BloomE2E/helpers/copyrightAndLicense.ts @@ -7,6 +7,11 @@ import { expect, type Locator, type Page } from "@playwright/test"; +// There is deliberately no direct-API "set the copyright" helper. The obvious endpoint, +// copyrightAndLicense/bookCopyrightAndLicense, is handled on Bloom's UI thread and deadlocks when a +// test posts it (the fetch comes from the WebView2 whose UI thread the handler then blocks), on the +// Edit tab and off it alike. A test sets copyright the way a person does, through the dialog below. + /** The open dialog, whichever page opened it. */ function dialog(page: Page): Locator { return page diff --git a/src/BloomE2E/helpers/libraryPublish.ts b/src/BloomE2E/helpers/libraryPublish.ts index c72ebeadce66..e4bdd5febb4d 100644 --- a/src/BloomE2E/helpers/libraryPublish.ts +++ b/src/BloomE2E/helpers/libraryPublish.ts @@ -201,9 +201,10 @@ export async function expectUploadStepButtons( } /** - * Click the Upload Book button. Bloom refuses to upload at all under --e2e (see - * LibraryPublishApi.RefuseUploadWhileRunningE2eTests), so this cannot publish anything; what a - * test can see is what the screen does before that point, such as the template warning below. + * Click the Upload Book button. Under --e2e Bloom uploads only to the sandbox and refuses + * bloomlibrary.org itself (see LibraryPublishApi.RefuseUploadWhileRunningE2eTests), and a test that + * has only a pretended login (e2e/loginState) has no real session to upload with; what such a test + * sees is what the screen does before an upload would happen, such as the template warning below. */ export async function clickUploadBook(page: Page): Promise { await page @@ -226,10 +227,11 @@ export async function waitForTemplateUploadWarning( } /** - * Answer No to the template warning. There is deliberately no helper for Yes: Bloom refuses to - * upload under --e2e, so answering Yes would prove nothing beyond that refusal, and what the - * manual test is really checking — that a template is allowed through the copyright rule — is - * already settled by the warning appearing at all. + * Answer No to the template warning. There is deliberately no helper for Yes here: this helper + * serves tests that only pretend to be logged in, which have no real session to upload with, and + * what the manual test is really checking — that a template is allowed through the copyright rule — + * is already settled by the warning appearing at all. A test that signs in for real and means to + * upload drives the Yes path itself. */ export async function declineTemplateUploadWarning(page: Page): Promise { await templateUploadWarning(page) diff --git a/src/BloomE2E/helpers/userSettings.ts b/src/BloomE2E/helpers/userSettings.ts new file mode 100644 index 000000000000..41d58522d442 --- /dev/null +++ b/src/BloomE2E/helpers/userSettings.ts @@ -0,0 +1,44 @@ +// Where the Bloom under test keeps its user settings (user.config: UI language, page zoom, the +// Bloom Library login, and the rest of Settings.Default), and what it has saved there. +// +// The launch fixture gives every Bloom it starts a settings folder of its own inside the run's +// temp folder (bloomApp.userSettingsDir; see fixtures/launchBloom.ts), so a test can look at the +// settings its Bloom saved with nobody else's Bloom in the way, and can put settings there before +// a launch for Bloom to start from. + +import * as fs from "node:fs"; +import * as Path from "node:path"; +import type { Page } from "@playwright/test"; +import { apiGetJson } from "./api"; + +/** Ask Bloom which folder it is keeping its user settings in. */ +export async function getUserSettingsFolder(page: Page): Promise { + const info = await apiGetJson<{ userSettingsFolder: string }>( + page, + "common/instanceInfo", + ); + return info.userSettingsFolder; +} + +/** + * Read one user setting Bloom has saved to the user.config in `folder`, by the name it has in + * Bloom's Settings.settings (e.g. "PageZoom"), as the string in the file; undefined when there is + * no such file yet or Bloom has not saved that setting. Only settings serialized as a string are + * readable this way, which is nearly all of them. + * + * This reads the disk rather than asking Bloom, because what a test usually wants to know is what + * reached the file: that is what the next Bloom to use the folder starts from. Bloom writes some + * settings a moment after they change (the zoom two seconds after the last change), so poll. + */ +export function readSavedUserSetting( + folder: string, + name: string, +): string | undefined { + const file = Path.join(folder, "user.config"); + if (!fs.existsSync(file)) return undefined; + const xml = fs.readFileSync(file, "utf8"); + const match = new RegExp( + `]*>\\s*([^<]*)`, + ).exec(xml); + return match ? match[1] : undefined; +} diff --git a/src/BloomE2E/tests/bulk-upload-quick-test.spec.ts b/src/BloomE2E/tests/bulk-upload-quick-test.spec.ts new file mode 100644 index 000000000000..a89328b856a0 --- /dev/null +++ b/src/BloomE2E/tests/bulk-upload-quick-test.spec.ts @@ -0,0 +1,290 @@ +// Bulk upload a small collection to dev.bloomlibrary.org for real, and check what the manual "Bulk +// Upload Quick Test" watches: a collection with no bookshelf is refused, a first upload sends every +// book, an unchanged re-upload skips them all, changing one book updates only that one, and moving +// the collection to another bookshelf updates them all and puts them on the new shelf with the +// collection's current front/back matter. Automates Notion test case 211, whose last steps absorbed +// the retired "Bulk Upload Across Bookshelves Applies Xmatter" (test case 212). +// +// This is the first e2e test that signs in to Bloom Library for real and uploads for real. It can +// do so because a test's Bloom keeps its login in a settings folder of its own (bloomApp.userSettingsDir), +// so the sign-in, and the upload's child Bloom that reads it back, never touch the developer's own +// Bloom. Everything goes to the sandbox, dev.bloomlibrary.org; Bloom refuses under --e2e to upload +// to production at all. The test deletes what it uploads, and any the account had left from before, +// in an afterAll — see helpers/bloomLibraryServer.ts. +// +// What is NOT covered here, and stays on a manual portion of the card: confirming each uploaded +// book on the website against the screenshot on its front cover, which is a human visual check. +// +// The account's password comes from BLOOM_E2E_TESTER_EMAIL_BLORG_PASSWORD (see +// helpers/bloomLibraryAccount.ts): without it the test skips locally and fails on CI. + +import { expect, test } from "../fixtures/bloomTest"; +import type { Page } from "@playwright/test"; +import type { IBloomApp } from "../fixtures/bloomTest"; +import { + addPage, + findBookFolder, + getContentPages, + getPages, + goToPage, + makeBookFromTemplate, + typeInGroup, +} from "../helpers/bookMaking"; +import { readXmatterPackOfBook } from "../helpers/bookHtml"; +import { setCopyrightHolder } from "../helpers/copyrightAndLicense"; +import { selectBook } from "../helpers/collection"; +import { switchTab } from "../helpers/workspace"; +import { + acceptAllAgreements, + clickToFixMissingItem, + openPublishToWeb, +} from "../helpers/libraryPublish"; +import { + kEnterpriseSubscriptionCode, + kTestBookshelves, + restartWithCollectionSettings, + type ICollectionSettings, +} from "../helpers/collectionSettings"; +import { + signBloomIntoLibraryForReal, + skipIfNoLibraryPassword, + TEST_ACCOUNT_EMAIL, +} from "../helpers/bloomLibraryAccount"; +import { + deleteAllBooksUploadedBy, + findBooksUploadedBy, + getXmatterPackOfBookOnServer, + type IBloomLibraryLogin, +} from "../helpers/bloomLibraryServer"; +import { + uploadCollection, + uploadCollectionExpectingBookshelfWarning, +} from "../helpers/bulkUpload"; + +// A collection under the Test enterprise subscription (bulk upload needs an enterprise tier), with +// no bookshelf yet: the first thing the card checks is that a bookshelf-less collection is refused. +test.use({ + collectionSpec: { + name: "bulk-upload-quick-test", + languages: ["en"], + subscriptionCode: kEnterpriseSubscriptionCode, + }, +}); + +test.describe.configure({ mode: "serial" }); + +// Four books, as the manual test uses. Each title is distinctive so the server records are easy to +// tell apart in a failure message. +const BOOK_TITLES = [ + "Bulk Upload Quick Test Book 1", + "Bulk Upload Quick Test Book 2", + "Bulk Upload Quick Test Book 3", + "Bulk Upload Quick Test Book 4", +]; +const COPYRIGHT_HOLDER = "Bloom Automated Test"; +// The collection goes on the first shelf, then moves to the second. +const [FIRST_BOOKSHELF, SECOND_BOOKSHELF] = kTestBookshelves; +// The front/back matter pack the collection starts with (the fixture's default) and the one it +// moves to along with the bookshelf, so the last upload has to bring every book up to date. +const FIRST_XMATTER_PACK = "Factory"; +const SECOND_XMATTER_PACK = "Traditional"; + +// The login the test signs in with, kept so afterAll can delete what was uploaded. +let login: IBloomLibraryLogin | undefined; +// The folder of each book the test made, in BOOK_TITLES order. Bloom names a book's folder after +// its title, but only after a save, so the folders are looked up rather than assumed. +const bookFolders: string[] = []; + +/** + * Make one book that is ready to upload: a Basic Book with a title, a content page with a word of + * text (so it has a language to publish), and a copyright. Leaves it saved and returns its folder. + * Setup, not the behavior under test, so it takes the fast route to each piece. + */ +async function makeUploadableBook(page: Page, title: string): Promise { + await switchTab(page, "collection"); + await makeBookFromTemplate(page, "Basic Book"); // lands on the cover in the Edit tab + await typeInGroup(page, ".bookTitle", "en", title); + await addPage(page, "Just Text"); + const [contentPage] = await getContentPages(page); + await goToPage(page, contentPage.id); + await typeInGroup(page, ".bloom-translationGroup", "en", "Hello."); + // Leave the content page (back to the cover) so Bloom saves what was typed. + await goToPage(page, (await getPages(page))[0].id); + // Give it a copyright through the real dialog, reached from Publish: Web's "Click to fix" on the + // Missing Copyright warning (the direct copyright API deadlocks; see helpers/copyrightAndLicense.ts). + await openPublishToWeb(page); + await clickToFixMissingItem(page, "Copyright"); + await setCopyrightHolder(page, COPYRIGHT_HOLDER); + return findBookFolder(page, title); +} + +/** + * Give the collection these settings (on top of its language and subscription), the way a person + * does in the Settings dialog, and come back signed in with the first book selected, ready to + * upload. The restart signs Bloom out of the login it kept only in memory, so this signs in again + * for the upload's child process to read from the settings folder. Returns the new shell page. + */ +async function restartReadyToUpload( + bloomApp: IBloomApp, + settings: Pick, +): Promise { + await restartWithCollectionSettings(bloomApp, { + languages: ["en"], + subscriptionCode: kEnterpriseSubscriptionCode, + ...settings, + }); + const page = bloomApp.page; + login = await signBloomIntoLibraryForReal(page); + await selectBook(page, bookFolders[0]); + return page; +} + +/** + * Check the four books on the sandbox, which is the card's "on Blorg, the books are on the bookshelf + * you set and carry the collection's xmatter": all four are there under the test account, each on + * this bookshelf, and each uploaded with this front/back matter pack. (The record's branding is not + * checked: the server fills that field in some time after the upload finishes, so a check right + * after the tally is flaky.) + */ +async function expectBooksOnServer( + account: IBloomLibraryLogin, + bookshelf: string, + xmatterPack: string, +): Promise { + const onServer = await findBooksUploadedBy(account); + expect( + onServer.map((b) => b.title).sort(), + `dev.bloomlibrary.org should list the four uploaded books for ${TEST_ACCOUNT_EMAIL}.`, + ).toEqual([...BOOK_TITLES].sort()); + for (const book of onServer) { + // "On", not "only on": after the move below, the sandbox still lists the first shelf as + // well (seen 2026-09-04). Bloom sends only the current shelf's tag, dropping any earlier + // bookshelf tag (BookUpload.UploadBookAsync), so it is the server that keeps the old + // one when a re-upload lands. Whether that is meant is an open question for the library + // team; this checks what the card asks for, that the books sit on the new shelf. + expect( + book.bookshelves, + `${book.title} should be on the ${bookshelf} bookshelf.`, + ).toContain(bookshelf); + expect( + await getXmatterPackOfBookOnServer(book), + `${book.title} should have been uploaded with the collection's ${xmatterPack} front/back matter.`, + ).toBe(xmatterPack); + } +} + +test.describe("bulk uploading a collection to dev.bloomlibrary.org", () => { + test.afterAll(async () => { + // Delete everything the account has on the sandbox, whether this run uploaded it or a + // crashed earlier run did, so dev.bloomlibrary.org is left clean. + if (login) await deleteAllBooksUploadedBy(login); + }); + + test("bulk upload: refused without a bookshelf, then new / skipped / updated / moved to another bookshelf [Test Case ID 211]", async ({ + page, + bloomApp, + }) => { + skipIfNoLibraryPassword(); + test.setTimeout(900000); + + // Clean the account first, so a crashed earlier run's books do not turn this run's "4 new" + // into "some updated", and get the login this test signs in and cleans up with. + login = await signBloomIntoLibraryForReal(page); + await deleteAllBooksUploadedBy(login); + + // ---- Four uploadable books -------------------------------------------------------------- + for (const title of BOOK_TITLES) + bookFolders.push(await makeUploadableBook(page, title)); + + // ---- A collection with no bookshelf is refused ------------------------------------------ + await selectBook(page, bookFolders[0]); + await openPublishToWeb(page); + await acceptAllAgreements(page); + const warning = await uploadCollectionExpectingBookshelfWarning(page); + expect( + warning, + "The no-bookshelf refusal should name the bookshelf setting.", + ).toContain("bookshelf"); + + // ---- Set the bookshelf, then upload: four new books ------------------------------------- + const pageAfterRestart = await restartReadyToUpload(bloomApp, { + bookshelf: FIRST_BOOKSHELF, + }); + const firstUpload = await uploadCollection( + pageAfterRestart, + bloomApp.collectionDir, + ); + expect( + firstUpload, + `The first bulk upload should have sent all four books as new. Log:\n${firstUpload.log}`, + ).toMatchObject({ newBooks: 4, updated: 0, skipped: 0 }); + // The four books really are on the sandbox now, on the shelf and with the pack the + // collection had. + await expectBooksOnServer(login!, FIRST_BOOKSHELF, FIRST_XMATTER_PACK); + + // ---- Upload again with nothing changed: four skipped ------------------------------------ + const secondUpload = await uploadCollection( + pageAfterRestart, + bloomApp.collectionDir, + ); + expect( + secondUpload, + `An unchanged re-upload should skip all four books. Log:\n${secondUpload.log}`, + ).toMatchObject({ newBooks: 0, updated: 0, skipped: 4 }); + + // ---- Change one book, upload again: one updated, three skipped -------------------------- + await selectBook(pageAfterRestart, bookFolders[0]); + await switchTab(pageAfterRestart, "edit"); + const [firstContentPage] = await getContentPages(pageAfterRestart); + await goToPage(pageAfterRestart, firstContentPage.id); + await typeInGroup( + pageAfterRestart, + ".bloom-translationGroup", + "en", + "Hello again.", + ); + // Leave the page so Bloom saves the change before the upload reads the folder. + await switchTab(pageAfterRestart, "collection"); + const thirdUpload = await uploadCollection( + pageAfterRestart, + bloomApp.collectionDir, + ); + expect( + thirdUpload, + `After changing one book, only that book should be updated. Log:\n${thirdUpload.log}`, + ).toMatchObject({ newBooks: 0, updated: 1, skipped: 3 }); + + // ---- Move the collection to another bookshelf, with another front/back matter pack: + // all four updated, and each lands on the new shelf with the new pack -------------- + // The pack changes along with the shelf because that is what the card's xmatter check needs: + // books nobody has opened since the pack changed must still go up with the collection's + // current pack, which bulk upload gets by bringing each book up to date before it hashes it + // (BulkUploader.UploadBookInternal). The shelf alone would already make every book count as + // changed, since the hash a skip is decided on covers the collection file too. + const pageAfterMove = await restartReadyToUpload(bloomApp, { + bookshelf: SECOND_BOOKSHELF, + xmatterPack: SECOND_XMATTER_PACK, + }); + // Sanity check: the books nobody selected still carry the old pack on disk, so the new pack + // on the server can only come from the upload bringing them up to date. (Selecting the first + // book, above, may already have brought that one up to date, so it is left out.) + for (const folder of bookFolders.slice(1)) + expect( + readXmatterPackOfBook(folder), + `${folder} should still have the old front/back matter until the upload brings it up to date.`, + ).toBe(FIRST_XMATTER_PACK); + const fourthUpload = await uploadCollection( + pageAfterMove, + bloomApp.collectionDir, + ); + expect( + fourthUpload, + `Moving the collection to another bookshelf should update all four books. Log:\n${fourthUpload.log}`, + ).toMatchObject({ newBooks: 0, updated: 4, skipped: 0 }); + await expectBooksOnServer( + login!, + SECOND_BOOKSHELF, + SECOND_XMATTER_PACK, + ); + }); +}); diff --git a/src/BloomE2E/tests/user-settings-isolation.spec.ts b/src/BloomE2E/tests/user-settings-isolation.spec.ts new file mode 100644 index 000000000000..2a7b88e0cfea --- /dev/null +++ b/src/BloomE2E/tests/user-settings-isolation.spec.ts @@ -0,0 +1,103 @@ +// A test's Bloom keeps its user settings (user.config: UI language, page zoom, the Bloom Library +// login, and the rest of Settings.Default) in a folder of its own inside the run's temp folder, +// not in the %LOCALAPPDATA%\SIL\Bloom\ folder that every Bloom of that build shares. So +// a run starts from default settings, and what it saves dies with the run, instead of starting +// from whatever the developer's Bloom saved last and leaving its own changes behind for them. +// +// This spec checks that the machinery holds: Bloom is using the folder the fixture gave it, a +// setting a test changes lands there, and it is still there after a restart within the run. It is +// infrastructure, so it has no "[Test Case ID N]" tag. + +import * as Path from "node:path"; +import { expect, test } from "../fixtures/bloomTest"; +import { makeBookFromTemplate } from "../helpers/bookMaking"; +import { + getUserSettingsFolder, + readSavedUserSetting, +} from "../helpers/userSettings"; +import { getZoom, setZoom } from "../helpers/workspace"; + +test.use({ + collectionSpec: { name: "user-settings-isolation", languages: ["en"] }, +}); + +test.describe.configure({ mode: "serial" }); + +/** Compare two folder paths the way Windows does. */ +function sameFolder(a: string, b: string): boolean { + return Path.resolve(a).toLowerCase() === Path.resolve(b).toLowerCase(); +} + +test.describe("a test's Bloom keeps its user settings to itself", () => { + test("Bloom keeps its user settings in the folder the run gave it", async ({ + page, + bloomApp, + }) => { + const folder = await getUserSettingsFolder(page); + expect( + sameFolder(folder, bloomApp.userSettingsDir), + `Bloom keeps its user settings in ${folder}, not in the run's own ${bloomApp.userSettingsDir}.`, + ).toBe(true); + + // Bloom accepted the license for us at startup (there is nobody to click Accept) and saved + // that, so the folder already holds a user.config, and this is what is in it. + expect( + readSavedUserSetting(bloomApp.userSettingsDir, "LicenseAccepted"), + ).toBe("True"); + }); + + test("a setting changed in the test's Bloom is saved in that folder", async ({ + page, + bloomApp, + }) => { + test.setTimeout(300000); + // The zoom is a user setting Bloom saves on its own, a couple of seconds after it changes, + // and there is a zoom only while a book is being edited. + await makeBookFromTemplate(page, "Basic Book"); + const { zoom, minZoom, maxZoom } = await getZoom(page); + // A fresh settings folder means Bloom starts at its default zoom, whatever the developer's + // own Bloom is zoomed to. + expect(zoom).toBe(100); + const newZoom = Math.min(zoom + 20, maxZoom); + expect(newZoom).toBeGreaterThanOrEqual(minZoom); + + await setZoom(page, newZoom); + + await expect + .poll( + () => + readSavedUserSetting(bloomApp.userSettingsDir, "PageZoom"), + { + timeout: 15000, + message: `Bloom never saved the zoom of ${newZoom}% to ${bloomApp.userSettingsDir}.`, + }, + ) + .toBe(String(newZoom)); + }); + + test("the saved setting is still there after a restart within the run", async ({ + bloomApp, + }) => { + test.setTimeout(300000); + const savedZoom = readSavedUserSetting( + bloomApp.userSettingsDir, + "PageZoom", + ); + expect( + savedZoom, + "test setup: the previous test saved a zoom", + ).toBeDefined(); + + const page = await bloomApp.restart(); + + expect( + sameFolder( + await getUserSettingsFolder(page), + bloomApp.userSettingsDir, + ), + ).toBe(true); + expect(readSavedUserSetting(bloomApp.userSettingsDir, "PageZoom")).toBe( + savedZoom, + ); + }); +}); diff --git a/src/BloomExe/BloomSettingsProvider.cs b/src/BloomExe/BloomSettingsProvider.cs new file mode 100644 index 000000000000..653b6f98f81c --- /dev/null +++ b/src/BloomExe/BloomSettingsProvider.cs @@ -0,0 +1,60 @@ +using System.IO; +using SIL.Settings; + +namespace Bloom +{ + /// + /// The provider behind Bloom's user settings (Settings.Default, the contents of user.config). + /// It is libpalaso's CrossPlatformSettingsProvider, which keeps one user.config per build + /// version in %LOCALAPPDATA%\SIL\Bloom\<version>\, with one addition: a folder named on the + /// command line (--user-settings-folder) replaces that location, for this process only. + /// + /// Every Bloom of one build otherwise shares one user.config, so a Bloom that an automated + /// test launches would start from whatever the last Bloom of that version saved (the UI + /// language, the page zoom, the Bloom Library login) and save its own changes for the next one, + /// including the developer's own Bloom from a worktree of the same version. The e2e launch + /// fixture therefore gives each Bloom it starts a folder inside the run's temp folder, so its + /// settings start from defaults, or from whatever the test put there first, and are deleted + /// with the rest of the run. Program.Main skips the "bring in settings from a previous version" + /// upgrade when a folder is named, for the same reason: the folder holds exactly the settings + /// its owner put there. + /// + public class BloomSettingsProvider : CrossPlatformSettingsProvider + { + /// + /// The folder to keep user.config in instead of the usual per-version one, or null for the + /// usual one. Set once, from the command line, before anything reads Settings.Default: + /// a provider computes its location when it is constructed, and Settings.Default constructs + /// its providers the first time any setting is read. + /// + public static string UserSettingsFolder { get; set; } + + public BloomSettingsProvider() + { + if (UserSettingsFolder != null) + { + UserLocalLocation = UserSettingsFolder; + UserRoamingLocation = UserSettingsFolder; + } + } + + /// + /// The folder this process keeps user.config in: UserSettingsFolder when one was named, + /// otherwise the usual %LOCALAPPDATA%\SIL\Bloom\<version>. Reported through + /// common/instanceInfo so an automated run can check that the Bloom it launched really is + /// keeping its settings where it was told to. + /// + public static string GetUserSettingsFolder() + { + return new BloomSettingsProvider().UserConfigLocation; + } + + /// + /// The path of the user.config file this process reads and writes. + /// + public static string GetUserConfigPath() + { + return Path.Combine(GetUserSettingsFolder(), UserConfigFileName); + } + } +} diff --git a/src/BloomExe/Program.cs b/src/BloomExe/Program.cs index af710d17bfb2..bfda28589858 100644 --- a/src/BloomExe/Program.cs +++ b/src/BloomExe/Program.cs @@ -129,6 +129,9 @@ static class Program StartupLauncherPort.HasValue ? $"launcherPort={StartupLauncherPort.Value}" : null, + BloomSettingsProvider.UserSettingsFolder != null + ? $"userSettingsFolder={BloomSettingsProvider.UserSettingsFolder}" + : null, }.Where(value => value != null) ); @@ -144,6 +147,30 @@ static int Main(string[] args1) // final call to CleanupTempFolder. Also prevents our temp files competing with // other programs for 64K available default temp file names. TempFile.NamePrefix = "bloom"; + + // Parse our own startup arguments before anything reads Settings.Default: + // --user-settings-folder decides where the settings live (it sets + // BloomSettingsProvider.UserSettingsFolder), and a settings provider fixes its location + // when it is constructed, which happens the first time a setting is read. + var args = ParseStartupPortArguments(args1, out var startupPortErrorMessage); + if (startupPortErrorMessage != null) + { + // A rejected launch touches no settings, its own or anyone else's, so the error is + // reported before anything reads Settings.Default. CheckForCorruptUserConfig and + // SetUpLocalization below both do, and by now the parser has cleared any folder a + // --user-settings-folder named, so they would read, and might repair, the shared + // profile of whoever is running Bloom. Only what a message box needs is set up. + Application.EnableVisualStyles(); + Application.SetCompatibleTextRenderingDefault(false); + MessageBox.Show( + startupPortErrorMessage, + "Bloom", + MessageBoxButtons.OK, + MessageBoxIcon.Error + ); + return 1; + } + CheckForCorruptUserConfig(); // Ensure that the registration information is loaded early before Team Collection // needs it. @@ -183,18 +210,6 @@ static int Main(string[] args1) // every startup path calls it. SetUpLocalization(); - var args = ParseStartupPortArguments(args1, out var startupPortErrorMessage); - if (startupPortErrorMessage != null) - { - MessageBox.Show( - startupPortErrorMessage, - "Bloom", - MessageBoxButtons.OK, - MessageBoxIcon.Error - ); - return 1; - } - // Old comment: Firefox60 uses Gtk3, so we need to as well. (BL-10469) // Aug 2023, we've moved away from GeckoFx/Firefox to wv2, but I don't know if this is still needed or not... // Steve says he thinks Gtk3 is still better but that it likely only matters for Linux, @@ -348,8 +363,15 @@ static int Main(string[] args1) if (Settings.Default.NeedUpgrade) { //see http://stackoverflow.com/questions/3498561/net-applicationsettingsbase-should-i-call-upgrade-every-time-i-load - Settings.Default.Upgrade(); - Settings.Default.Reload(); + // A folder named with --user-settings-folder holds exactly the settings its + // owner put there (see BloomSettingsProvider), so nothing is brought in from + // another version's file; an automated run would otherwise inherit the + // developer's settings after all. + if (BloomSettingsProvider.UserSettingsFolder == null) + { + Settings.Default.Upgrade(); + Settings.Default.Reload(); + } Settings.Default.NeedUpgrade = false; Settings.Default.MaximizeWindow = true; // this is needed to force this to be written to the file, where a user can find it to modify it by hand (our video maker) Settings.Default.Save(); @@ -791,6 +813,7 @@ internal static string[] ParseStartupPortArguments(string[] args, out string err StartupLabel = null; StartupAutomation = false; StartupLauncherPort = null; + BloomSettingsProvider.UserSettingsFolder = null; RunningE2eTests = false; StartupExperimentalFeatures = null; @@ -848,10 +871,18 @@ out errorMessage value => StartupExperimentalFeatures = value, out errorMessage ) + || TryHandleUserSettingsFolderArgument(args, ref i, out errorMessage) ) { if (errorMessage != null) + { + // The launch is being rejected, so it must not use a settings folder an + // earlier --user-settings-folder already set. A rejected launch touches + // nothing: Main reports the error before anything reads settings, and + // clearing the folder here means the launch never owned one. + BloomSettingsProvider.UserSettingsFolder = null; return Array.Empty(); + } continue; } @@ -864,12 +895,65 @@ out errorMessage if (StartupExperimentalFeatures != null && !RunningE2eTests) { errorMessage = "Bloom only accepts --experimental-features together with --e2e."; + // Rejected, so it owns no settings folder either (see the rejection above). + BloomSettingsProvider.UserSettingsFolder = null; return Array.Empty(); } return remainingArgs.ToArray(); } + /// + /// Handle --user-settings-folder: the folder to keep user.config in, stored on + /// BloomSettingsProvider.UserSettingsFolder (the provider that consumes it). Stored as a + /// full path, because Bloom changes its working directory during startup + /// (NormalizeWorkingDirectory) and a relative path would otherwise point somewhere else by + /// the time the settings are saved. + /// + private static bool TryHandleUserSettingsFolderArgument( + string[] args, + ref int index, + out string errorMessage + ) + { + const string optionName = "--user-settings-folder"; + if ( + !TryParseStartupStringArgument( + args, + ref index, + optionName, + out var value, + out errorMessage + ) + ) + { + return false; + } + + if (errorMessage != null) + return true; + + if (BloomSettingsProvider.UserSettingsFolder != null) + { + errorMessage = $"Bloom only accepts one {optionName} argument."; + return true; + } + + try + { + BloomSettingsProvider.UserSettingsFolder = Path.GetFullPath(value); + } + catch (Exception e) + when (e is ArgumentException + || e is NotSupportedException + || e is System.IO.PathTooLongException + ) + { + errorMessage = $"Bloom cannot use \"{value}\" as the {optionName}: {e.Message}"; + } + return true; + } + private static bool TryHandleStartupFlagArgument( string[] args, ref int index, @@ -2914,7 +2998,8 @@ public static BloomFileLocator OptimizedFileLocator private static void CheckForCorruptUserConfig() { //First check the user.config we get through using the palaso stuff. This is the one in a folder with a name like Bloom/3.5.0.0 - var palasoSettings = new SIL.Settings.CrossPlatformSettingsProvider(); + // (or the folder --user-settings-folder named; BloomSettingsProvider knows which). + var palasoSettings = new BloomSettingsProvider(); palasoSettings.Initialize(null, null); var error = palasoSettings.CheckForErrorsInSettingsFile(); if (error != null) diff --git a/src/BloomExe/Properties/Settings.Designer.cs b/src/BloomExe/Properties/Settings.Designer.cs index 0886cba900d6..2cef659ec346 100644 --- a/src/BloomExe/Properties/Settings.Designer.cs +++ b/src/BloomExe/Properties/Settings.Designer.cs @@ -24,7 +24,7 @@ public static Settings Default { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("True")] public bool FirstTimeRun { @@ -37,7 +37,7 @@ public bool FirstTimeRun { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] public global::Bloom.CollectionChoosing.MostRecentPathsList MruProjects { get { @@ -49,7 +49,7 @@ public bool FirstTimeRun { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("True")] public bool NeedUpgrade { @@ -62,7 +62,7 @@ public bool NeedUpgrade { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string UserInterfaceLanguage { @@ -75,7 +75,7 @@ public string UserInterfaceLanguage { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool UserInterfaceLanguageSetExplicitly { @@ -88,7 +88,7 @@ public bool UserInterfaceLanguageSetExplicitly { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string LastSourceLanguageViewed { @@ -101,7 +101,7 @@ public string LastSourceLanguageViewed { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string LastSourceLanguageViewed2 { @@ -114,7 +114,7 @@ public string LastSourceLanguageViewed2 { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("http")] public string ImageHandler { @@ -127,7 +127,7 @@ public string ImageHandler { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string ImageGalleryProviderKeys { @@ -140,7 +140,7 @@ public string ImageGalleryProviderKeys { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool ShowLocalizationControls { @@ -153,7 +153,7 @@ public bool ShowLocalizationControls { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool ShowUnapprovedLocalizations { @@ -166,7 +166,7 @@ public bool ShowUnapprovedLocalizations { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string EnabledExperimentalFeatures { @@ -179,7 +179,7 @@ public string EnabledExperimentalFeatures { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string DontShowThisAgain { @@ -192,7 +192,7 @@ public string DontShowThisAgain { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool ShowExperimentalFeatures { @@ -205,7 +205,7 @@ public bool ShowExperimentalFeatures { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string WebUserId { @@ -218,7 +218,7 @@ public string WebUserId { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string WebPassword { @@ -231,7 +231,7 @@ public string WebPassword { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("True")] public bool WebShowPassword { @@ -244,7 +244,7 @@ public bool WebShowPassword { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("True")] public bool MaximizeWindow { @@ -257,7 +257,7 @@ public bool MaximizeWindow { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] public global::SIL.Windows.Forms.PortableSettingsProvider.FormSettings WindowSizeAndLocation { get { @@ -269,7 +269,7 @@ public bool MaximizeWindow { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool LicenseAccepted { @@ -282,7 +282,7 @@ public bool LicenseAccepted { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("0, 0, 1028, 586")] public global::System.Drawing.Rectangle RestoreBounds { @@ -295,7 +295,7 @@ public bool LicenseAccepted { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string ImageSearchLanguage { @@ -308,7 +308,7 @@ public string ImageSearchLanguage { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("1.0")] public string PageZoom { @@ -321,7 +321,7 @@ public string PageZoom { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool AdobeColorProfileEula2003Accepted { @@ -334,7 +334,7 @@ public bool AdobeColorProfileEula2003Accepted { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string BloomDeviceFileExportFolder { @@ -347,7 +347,7 @@ public string BloomDeviceFileExportFolder { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string PublishAndroidMethod { @@ -360,7 +360,7 @@ public string PublishAndroidMethod { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("1")] public int CurrentStage { @@ -373,7 +373,7 @@ public int CurrentStage { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("1")] public int CurrentLevel { @@ -386,7 +386,7 @@ public int CurrentLevel { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string CurrentBookPath { @@ -399,7 +399,7 @@ public string CurrentBookPath { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("0")] public int AutoUpdateDialogShown { @@ -412,7 +412,7 @@ public int AutoUpdateDialogShown { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool AlwaysMeasurePerformance { @@ -425,7 +425,7 @@ public bool AlwaysMeasurePerformance { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("True")] public bool AutoUpdate { @@ -438,7 +438,7 @@ public bool AutoUpdate { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] public global::System.DateTime ForumInvitationLastShown { get { @@ -450,7 +450,7 @@ public bool AutoUpdate { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("False")] public bool ForumInvitationAcknowledged { @@ -463,7 +463,7 @@ public bool ForumInvitationAcknowledged { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string LastLoginDest { @@ -476,7 +476,7 @@ public string LastLoginDest { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string LastLoginSessionToken { @@ -489,7 +489,7 @@ public string LastLoginSessionToken { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string LastLoginUserId { @@ -502,6 +502,7 @@ public string LastLoginUserId { } [global::System.Configuration.UserScopedSettingAttribute()] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string ExportImportFileFolder { @@ -514,7 +515,7 @@ public string ExportImportFileFolder { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string OpenRouterApiKey { @@ -527,7 +528,7 @@ public string OpenRouterApiKey { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string CollectionTabSplitterSizes { @@ -540,7 +541,7 @@ public string CollectionTabSplitterSizes { } [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] + [global::System.Configuration.SettingsProviderAttribute(typeof(Bloom.BloomSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] [global::System.Configuration.DefaultSettingValueAttribute("")] public string WebSiteDestinationOverride { diff --git a/src/BloomExe/Properties/Settings.settings b/src/BloomExe/Properties/Settings.settings index a66fc53c6621..fefb0b0bd779 100644 --- a/src/BloomExe/Properties/Settings.settings +++ b/src/BloomExe/Properties/Settings.settings @@ -2,133 +2,133 @@ - + True - + - + True - + - + False - + - + - + http - + - + False - + False - + False - + - + - + False - + - + - + True - + False - + True - + - + False - + 0, 0, 1028, 586 - + - + 1.0 - + False - + - + - + 1 - + 1 - + - + 0 - + False - + True - + - + False - + - + - + - + - + - + - + diff --git a/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs b/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs index 87bc200bb3a6..32077f612e9d 100644 --- a/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs +++ b/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs @@ -656,15 +656,40 @@ public void BulkUpload(string rootFolderPath, IProgress progress) : UploadDestination.Production; var bloomExePath = Program.BloomExePath; - var command = - $"\"{bloomExePath}\" upload \"{rootFolderPath}\" -u {WebUserId} -d {target}"; + var arguments = $"upload \"{rootFolderPath}\" -u {WebUserId} -d {target}"; + // The upload runs in a second Bloom, which signs in with the login this one saved in + // its user settings. When this Bloom keeps its settings in a folder of its own (see + // BloomSettingsProvider), the second one has to read the same folder, or it finds no + // login at all, or somebody else's. + if (BloomSettingsProvider.UserSettingsFolder != null) + arguments += + $" --user-settings-folder \"{BloomSettingsProvider.UserSettingsFolder}\""; + // An automated run wants the second Bloom to behave as this one does: no modal dialogs. + if (Program.RunningE2eTests) + arguments += " --e2e"; + var command = $"\"{bloomExePath}\" {arguments}"; if (SIL.PlatformUtilities.Platform.IsLinux) command = $"/opt/mono5-sil/bin/mono {command}"; // The Bloom process run as a command line tool for bulk upload is safe for using the current culture. // We don't wait for this to finish, so we don't use the CommandLineRunner methods. ProcessStartInfo startInfo; - if (SIL.PlatformUtilities.Platform.IsWindows) + if (Program.RunningE2eTests) + { + // An automated run has nobody to read a terminal window, and the window below stays + // open (cmd /k) until a person closes it, so it would outlive the test. Run the + // upload with no window at all; BloomBulkUploadLog.txt in the collection folder + // gets everything the window would have shown, and is what a test reads. + startInfo = new ProcessStartInfo() + { + FileName = bloomExePath, + Arguments = arguments, + WorkingDirectory = Path.GetDirectoryName(bloomExePath), + UseShellExecute = false, + CreateNoWindow = true, + }; + } + else if (SIL.PlatformUtilities.Platform.IsWindows) { startInfo = new ProcessStartInfo() { @@ -707,7 +732,15 @@ public void BulkUpload(string rootFolderPath, IProgress progress) BookUploaded = true; // Flag that an upload has occurred BookUploadedId = null; // Multiple books have been uploaded, so a single id doesn't exist: all may need to be updated - ProcessExtra.StartInFront(startInfo); + if (Program.RunningE2eTests) + { + // There is no window to bring to the front. + Process.Start(startInfo); + } + else + { + ProcessExtra.StartInFront(startInfo); + } progress.WriteMessage("Starting bulk upload in a terminal window..."); progress.WriteMessage( "This process will skip books if it can tell that nothing has changed since the last bulk upload." diff --git a/src/BloomExe/web/controllers/CommonApi.cs b/src/BloomExe/web/controllers/CommonApi.cs index 0568c22a1f5a..c7148470b9c5 100644 --- a/src/BloomExe/web/controllers/CommonApi.cs +++ b/src/BloomExe/web/controllers/CommonApi.cs @@ -297,6 +297,11 @@ private void HandleInstanceInfo(ApiRequest request) // Control port of the dev launcher that started us (null when not // launched via go.sh); see .github/skills/bloom-automation. launcherControlPort = Program.StartupLauncherPort, + // Where this instance keeps its user settings (user.config): the folder + // --user-settings-folder named, or the usual per-version one. The e2e launch + // fixture checks this to be sure the Bloom it started is not sharing settings + // with anyone; see BloomSettingsProvider. + userSettingsFolder = BloomSettingsProvider.GetUserSettingsFolder(), } ); } diff --git a/src/BloomExe/web/controllers/LibraryPublishApi.cs b/src/BloomExe/web/controllers/LibraryPublishApi.cs index 1b846f1a0158..ace30720e87e 100644 --- a/src/BloomExe/web/controllers/LibraryPublishApi.cs +++ b/src/BloomExe/web/controllers/LibraryPublishApi.cs @@ -201,19 +201,21 @@ private async Task HandleUpload(ApiRequest request, bool changeUploader) } /// - /// Uploading is refused while Bloom is running e2e tests (the --e2e flag). A test can ask - /// Bloom to REPORT a Bloom Library login (the e2e/loginState hook) so it can check what the - /// upload screen offers a signed-in user; that pretense does not touch the real - /// credentials, so without this an automated click on Upload would publish a real book to - /// a real server under whatever account the developer happens to be signed in as. + /// Uploading to bloomlibrary.org itself is refused while Bloom is running e2e tests (the + /// --e2e flag); an automated run may upload only to the sandbox, dev.bloomlibrary.org. A + /// test signs in there for real, with a test account, by posting the account's session token + /// to external/login, the route the website uses; and a test's Bloom keeps its login in a + /// settings folder of its own (see BloomSettingsProvider), so it is never the developer's + /// account that uploads. Production stays closed all the same, so that no automated click + /// can ever publish a book where the public would see it, whatever account is signed in. /// The refusal goes to the progress box, which is where this screen shows upload trouble. /// private bool RefuseUploadWhileRunningE2eTests() { - if (!Program.RunningE2eTests) + if (!Program.RunningE2eTests || BookUpload.UseSandbox) return false; _webSocketProgress.MessageWithoutLocalizing( - "Uploading is disabled while Bloom is running e2e tests (--e2e).", + "Uploading to bloomlibrary.org is disabled while Bloom is running e2e tests (--e2e); an automated run may upload only to dev.bloomlibrary.org.", ProgressKind.Error ); return true; diff --git a/src/BloomTests/BloomSettingsProviderTests.cs b/src/BloomTests/BloomSettingsProviderTests.cs new file mode 100644 index 000000000000..a6fb5048e5a0 --- /dev/null +++ b/src/BloomTests/BloomSettingsProviderTests.cs @@ -0,0 +1,120 @@ +using System; +using System.Configuration; +using System.IO; +using Bloom; +using Bloom.Properties; +using NUnit.Framework; +using SIL.TestUtilities; + +namespace BloomTests +{ + /// + /// BloomSettingsProvider keeps user.config where libpalaso's provider does, unless a folder is + /// named (--user-settings-folder), in which case that folder is used instead. These tests drive + /// the provider directly, the way ApplicationSettingsBase does, so they need no Bloom running + /// and touch no real settings file. + /// + [TestFixture] + public class BloomSettingsProviderTests + { + private const string kGroupName = "Bloom.Properties.Settings"; + private TemporaryFolder _folder; + + [SetUp] + public void Setup() + { + // Settings.Default constructs its providers the first time any setting is read, and a + // provider fixes its location when it is constructed. Make sure that has already + // happened, so that naming a folder below moves only the providers these tests + // construct, never the test run's own settings. + var _ = Settings.Default.ShowExperimentalFeatures; + _folder = new TemporaryFolder("BloomSettingsProviderTests"); + } + + [TearDown] + public void TearDown() + { + BloomSettingsProvider.UserSettingsFolder = null; + _folder.Dispose(); + } + + [Test] + public void GetUserSettingsFolder_NoFolderNamed_IsThePerVersionFolderUnderLocalAppData() + { + BloomSettingsProvider.UserSettingsFolder = null; + + var folder = BloomSettingsProvider.GetUserSettingsFolder(); + + Assert.That( + folder, + Does.StartWith( + Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData) + ) + ); + // libpalaso names the last folder after the entry assembly's version. + Assert.That( + Path.GetFileName(folder), + Is.EqualTo( + System.Reflection.Assembly.GetEntryAssembly().GetName().Version.ToString() + ) + ); + } + + [Test] + public void GetUserSettingsFolder_FolderNamed_IsThatFolder() + { + BloomSettingsProvider.UserSettingsFolder = _folder.Path; + + Assert.That(BloomSettingsProvider.GetUserSettingsFolder(), Is.EqualTo(_folder.Path)); + Assert.That( + BloomSettingsProvider.GetUserConfigPath(), + Is.EqualTo(Path.Combine(_folder.Path, "user.config")) + ); + } + + [Test] + public void SetPropertyValues_FolderNamed_WritesUserConfigThereAndReadsItBack() + { + BloomSettingsProvider.UserSettingsFolder = _folder.Path; + var userConfig = Path.Combine(_folder.Path, "user.config"); + Assert.That(File.Exists(userConfig), Is.False, "test setup: the folder starts empty"); + + var property = new SettingsProperty("UserInterfaceLanguage") + { + PropertyType = typeof(string), + SerializeAs = SettingsSerializeAs.String, + DefaultValue = "", + }; + var context = new SettingsContext { ["GroupName"] = kGroupName }; + + var writer = new BloomSettingsProvider(); + writer.Initialize(null, null); + writer.SetPropertyValues( + context, + new SettingsPropertyValueCollection + { + new SettingsPropertyValue(property) { SerializedValue = "fr" }, + } + ); + + Assert.That( + File.Exists(userConfig), + Is.True, + "user.config was not written to the named folder" + ); + Assert.That( + File.ReadAllText(userConfig), + Does.Contain(" Date: Thu, 10 Sep 2026 09:08:03 -0500 Subject: [PATCH 2/4] Keep "was a user settings folder named" inside BloomSettingsProvider Review of #8306 found the BloomSettingsProvider.UserSettingsFolder != null checks scattered through Program and BloomLibraryPublishModel leaking an implementation detail: only the provider needs to know whether a folder was named on the command line, yet three other places branched on it, each a spot where an e2e run behaves differently from a normal one. The provider now keeps the folder private and offers what its callers actually need: - SetUserSettingsFolder, which the startup parser calls once, for an accepted command line. The parser collects the folder in a local while it works, so it no longer has to clear the provider on each rejection path; a rejected launch simply never hands one over. The requested folder is reported through a new Program.StartupUserSettingsFolder, alongside the other Startup* properties. - CommandLineArgumentsForChildBloom, the --user-settings-folder argument (or nothing) that a Bloom this one starts needs in order to share its settings. BulkUpload appends it without knowing which case it is in. - A re-implementation of IApplicationSettingsProvider.Upgrade and GetPreviousVersion (libpalaso's are not virtual) that brings nothing in from a previous version when a folder was named. ApplicationSettingsBase drives its providers through that interface, so Settings.Default.Upgrade() in Main is now unconditional, and the second Upgrade call in CheckForCorruptUserConfig, which the old check never covered, is handled too. Tests follow: the provider tests use the setter and cover the child arguments and the no-upgrade rule; the parser tests check what the provider ended up on rather than the removed property. Co-Authored-By: Claude Fable 5.1 --- src/BloomExe/BloomSettingsProvider.cs | 90 +++++++++++++++---- src/BloomExe/Program.cs | 73 +++++++-------- .../BloomLibrary/BloomLibraryPublishModel.cs | 11 +-- src/BloomTests/BloomSettingsProviderTests.cs | 82 ++++++++++++++--- src/BloomTests/ProgramTests.cs | 57 +++++++++--- 5 files changed, 230 insertions(+), 83 deletions(-) diff --git a/src/BloomExe/BloomSettingsProvider.cs b/src/BloomExe/BloomSettingsProvider.cs index 653b6f98f81c..f262e9599fbc 100644 --- a/src/BloomExe/BloomSettingsProvider.cs +++ b/src/BloomExe/BloomSettingsProvider.cs @@ -1,3 +1,4 @@ +using System.Configuration; using System.IO; using SIL.Settings; @@ -15,34 +16,46 @@ namespace Bloom /// including the developer's own Bloom from a worktree of the same version. The e2e launch /// fixture therefore gives each Bloom it starts a folder inside the run's temp folder, so its /// settings start from defaults, or from whatever the test put there first, and are deleted - /// with the rest of the run. Program.Main skips the "bring in settings from a previous version" - /// upgrade when a folder is named, for the same reason: the folder holds exactly the settings - /// its owner put there. + /// with the rest of the run. Such a folder holds exactly the settings its owner put there, so + /// this provider also brings nothing in from a previous version's user.config when one is + /// named (see Upgrade below); an automated run would otherwise inherit the developer's + /// settings after all. + /// + /// Only this class knows whether a folder was named. Everything else asks it for what it + /// needs: where the settings are (GetUserSettingsFolder), or what to put on the command line + /// of another Bloom so that it uses the same ones (CommandLineArgumentsForChildBloom). /// - public class BloomSettingsProvider : CrossPlatformSettingsProvider + public class BloomSettingsProvider : CrossPlatformSettingsProvider, IApplicationSettingsProvider { + // The folder named on the command line, or null for the usual per-version one. + private static string _folderFromCommandLine; + /// - /// The folder to keep user.config in instead of the usual per-version one, or null for the - /// usual one. Set once, from the command line, before anything reads Settings.Default: - /// a provider computes its location when it is constructed, and Settings.Default constructs - /// its providers the first time any setting is read. + /// Keep user.config in this folder instead of the usual per-version one; null restores the + /// usual one. Program's startup argument parser calls this for --user-settings-folder, + /// before anything reads Settings.Default: a provider computes its location when it is + /// constructed, and Settings.Default constructs its providers the first time any setting + /// is read. /// - public static string UserSettingsFolder { get; set; } + public static void SetUserSettingsFolder(string folder) + { + _folderFromCommandLine = folder; + } public BloomSettingsProvider() { - if (UserSettingsFolder != null) + if (_folderFromCommandLine != null) { - UserLocalLocation = UserSettingsFolder; - UserRoamingLocation = UserSettingsFolder; + UserLocalLocation = _folderFromCommandLine; + UserRoamingLocation = _folderFromCommandLine; } } /// - /// The folder this process keeps user.config in: UserSettingsFolder when one was named, - /// otherwise the usual %LOCALAPPDATA%\SIL\Bloom\<version>. Reported through - /// common/instanceInfo so an automated run can check that the Bloom it launched really is - /// keeping its settings where it was told to. + /// The folder this process keeps user.config in: the one named on the command line when + /// there was one, otherwise the usual %LOCALAPPDATA%\SIL\Bloom\<version>. Reported + /// through common/instanceInfo so an automated run can check that the Bloom it launched + /// really is keeping its settings where it was told to. /// public static string GetUserSettingsFolder() { @@ -56,5 +69,50 @@ public static string GetUserConfigPath() { return Path.Combine(GetUserSettingsFolder(), UserConfigFileName); } + + /// + /// What to put on the command line of another Bloom this one starts so that it reads and + /// writes the same user settings as this one: the --user-settings-folder argument when a + /// folder was named, otherwise nothing, since another Bloom of this build shares the usual + /// per-version folder anyway. + /// + public static string CommandLineArgumentsForChildBloom => + _folderFromCommandLine == null + ? "" + : $"--user-settings-folder \"{_folderFromCommandLine}\""; + + // ApplicationSettingsBase drives its providers through IApplicationSettingsProvider, so + // re-implementing the interface here (libpalaso's methods are not virtual) lets this class + // decide what "a previous version's settings" means for it: nothing, when a folder was + // named. Settings.Default.Upgrade() then does exactly what the caller intends in both + // cases, and no caller needs to know which case it is in. + + /// + /// Bring in the settings of a previous Bloom version, unless a folder was named on the + /// command line, which holds exactly the settings its owner put there. + /// + void IApplicationSettingsProvider.Upgrade( + SettingsContext context, + SettingsPropertyCollection properties + ) + { + if (_folderFromCommandLine != null) + return; + base.Upgrade(context, properties); + } + + /// + /// A setting's value from a previous Bloom version, or null when a folder was named on the + /// command line: such a folder has no previous version. + /// + SettingsPropertyValue IApplicationSettingsProvider.GetPreviousVersion( + SettingsContext context, + SettingsProperty property + ) + { + if (_folderFromCommandLine != null) + return null; + return base.GetPreviousVersion(context, property); + } } } diff --git a/src/BloomExe/Program.cs b/src/BloomExe/Program.cs index bfda28589858..47b35750c906 100644 --- a/src/BloomExe/Program.cs +++ b/src/BloomExe/Program.cs @@ -119,6 +119,11 @@ static class Program // rebuild and relaunch us. internal static int? StartupLauncherPort { get; private set; } + // The folder --user-settings-folder asked Bloom to keep its user settings in, or null + // when none was named. BloomSettingsProvider is what acts on it; this copy is only for + // reporting what was requested. + internal static string StartupUserSettingsFolder { get; private set; } + internal static string StartupRequestedPortSummary => string.Join( ", ", @@ -129,8 +134,8 @@ static class Program StartupLauncherPort.HasValue ? $"launcherPort={StartupLauncherPort.Value}" : null, - BloomSettingsProvider.UserSettingsFolder != null - ? $"userSettingsFolder={BloomSettingsProvider.UserSettingsFolder}" + StartupUserSettingsFolder != null + ? $"userSettingsFolder={StartupUserSettingsFolder}" : null, }.Where(value => value != null) ); @@ -149,17 +154,18 @@ static int Main(string[] args1) TempFile.NamePrefix = "bloom"; // Parse our own startup arguments before anything reads Settings.Default: - // --user-settings-folder decides where the settings live (it sets - // BloomSettingsProvider.UserSettingsFolder), and a settings provider fixes its location - // when it is constructed, which happens the first time a setting is read. + // --user-settings-folder decides where the settings live (the parser hands it to + // BloomSettingsProvider), and a settings provider fixes its location when it is + // constructed, which happens the first time a setting is read. var args = ParseStartupPortArguments(args1, out var startupPortErrorMessage); if (startupPortErrorMessage != null) { // A rejected launch touches no settings, its own or anyone else's, so the error is // reported before anything reads Settings.Default. CheckForCorruptUserConfig and - // SetUpLocalization below both do, and by now the parser has cleared any folder a - // --user-settings-folder named, so they would read, and might repair, the shared - // profile of whoever is running Bloom. Only what a message box needs is set up. + // SetUpLocalization below both do, and a rejected launch owns no settings folder + // (the parser hands one to BloomSettingsProvider only for an accepted command + // line), so they would read, and might repair, the shared profile of whoever is + // running Bloom. Only what a message box needs is set up. Application.EnableVisualStyles(); Application.SetCompatibleTextRenderingDefault(false); MessageBox.Show( @@ -363,15 +369,9 @@ static int Main(string[] args1) if (Settings.Default.NeedUpgrade) { //see http://stackoverflow.com/questions/3498561/net-applicationsettingsbase-should-i-call-upgrade-every-time-i-load - // A folder named with --user-settings-folder holds exactly the settings its - // owner put there (see BloomSettingsProvider), so nothing is brought in from - // another version's file; an automated run would otherwise inherit the - // developer's settings after all. - if (BloomSettingsProvider.UserSettingsFolder == null) - { - Settings.Default.Upgrade(); - Settings.Default.Reload(); - } + // (BloomSettingsProvider decides what, if anything, there is to bring in.) + Settings.Default.Upgrade(); + Settings.Default.Reload(); Settings.Default.NeedUpgrade = false; Settings.Default.MaximizeWindow = true; // this is needed to force this to be written to the file, where a user can find it to modify it by hand (our video maker) Settings.Default.Save(); @@ -813,10 +813,15 @@ internal static string[] ParseStartupPortArguments(string[] args, out string err StartupLabel = null; StartupAutomation = false; StartupLauncherPort = null; - BloomSettingsProvider.UserSettingsFolder = null; + StartupUserSettingsFolder = null; + BloomSettingsProvider.SetUserSettingsFolder(null); RunningE2eTests = false; StartupExperimentalFeatures = null; + // Collected here and handed to BloomSettingsProvider only once the whole command line + // has been accepted, so a rejected launch never owns a settings folder: Main reports + // the error before anything reads settings, and there is nothing to undo here. + string userSettingsFolder = null; var remainingArgs = new List(); for (var i = 0; i < args.Length; i++) @@ -871,18 +876,16 @@ out errorMessage value => StartupExperimentalFeatures = value, out errorMessage ) - || TryHandleUserSettingsFolderArgument(args, ref i, out errorMessage) + || TryHandleUserSettingsFolderArgument( + args, + ref i, + ref userSettingsFolder, + out errorMessage + ) ) { if (errorMessage != null) - { - // The launch is being rejected, so it must not use a settings folder an - // earlier --user-settings-folder already set. A rejected launch touches - // nothing: Main reports the error before anything reads settings, and - // clearing the folder here means the launch never owned one. - BloomSettingsProvider.UserSettingsFolder = null; return Array.Empty(); - } continue; } @@ -895,24 +898,24 @@ out errorMessage if (StartupExperimentalFeatures != null && !RunningE2eTests) { errorMessage = "Bloom only accepts --experimental-features together with --e2e."; - // Rejected, so it owns no settings folder either (see the rejection above). - BloomSettingsProvider.UserSettingsFolder = null; return Array.Empty(); } + StartupUserSettingsFolder = userSettingsFolder; + BloomSettingsProvider.SetUserSettingsFolder(userSettingsFolder); return remainingArgs.ToArray(); } /// - /// Handle --user-settings-folder: the folder to keep user.config in, stored on - /// BloomSettingsProvider.UserSettingsFolder (the provider that consumes it). Stored as a - /// full path, because Bloom changes its working directory during startup - /// (NormalizeWorkingDirectory) and a relative path would otherwise point somewhere else by - /// the time the settings are saved. + /// Handle --user-settings-folder: the folder to keep user.config in, which the caller + /// hands to BloomSettingsProvider. Stored as a full path, because Bloom changes its + /// working directory during startup (NormalizeWorkingDirectory) and a relative path would + /// otherwise point somewhere else by the time the settings are saved. /// private static bool TryHandleUserSettingsFolderArgument( string[] args, ref int index, + ref string folder, out string errorMessage ) { @@ -933,7 +936,7 @@ out errorMessage if (errorMessage != null) return true; - if (BloomSettingsProvider.UserSettingsFolder != null) + if (folder != null) { errorMessage = $"Bloom only accepts one {optionName} argument."; return true; @@ -941,7 +944,7 @@ out errorMessage try { - BloomSettingsProvider.UserSettingsFolder = Path.GetFullPath(value); + folder = Path.GetFullPath(value); } catch (Exception e) when (e is ArgumentException diff --git a/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs b/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs index 32077f612e9d..897f5cc5b4a7 100644 --- a/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs +++ b/src/BloomExe/Publish/BloomLibrary/BloomLibraryPublishModel.cs @@ -656,14 +656,11 @@ public void BulkUpload(string rootFolderPath, IProgress progress) : UploadDestination.Production; var bloomExePath = Program.BloomExePath; - var arguments = $"upload \"{rootFolderPath}\" -u {WebUserId} -d {target}"; // The upload runs in a second Bloom, which signs in with the login this one saved in - // its user settings. When this Bloom keeps its settings in a folder of its own (see - // BloomSettingsProvider), the second one has to read the same folder, or it finds no - // login at all, or somebody else's. - if (BloomSettingsProvider.UserSettingsFolder != null) - arguments += - $" --user-settings-folder \"{BloomSettingsProvider.UserSettingsFolder}\""; + // its user settings, so the second one has to read the same settings as this one, or + // it finds no login at all, or somebody else's. + var arguments = + $"upload \"{rootFolderPath}\" -u {WebUserId} -d {target} {BloomSettingsProvider.CommandLineArgumentsForChildBloom}"; // An automated run wants the second Bloom to behave as this one does: no modal dialogs. if (Program.RunningE2eTests) arguments += " --e2e"; diff --git a/src/BloomTests/BloomSettingsProviderTests.cs b/src/BloomTests/BloomSettingsProviderTests.cs index a6fb5048e5a0..b428fde0119a 100644 --- a/src/BloomTests/BloomSettingsProviderTests.cs +++ b/src/BloomTests/BloomSettingsProviderTests.cs @@ -34,14 +34,14 @@ public void Setup() [TearDown] public void TearDown() { - BloomSettingsProvider.UserSettingsFolder = null; + BloomSettingsProvider.SetUserSettingsFolder(null); _folder.Dispose(); } [Test] public void GetUserSettingsFolder_NoFolderNamed_IsThePerVersionFolderUnderLocalAppData() { - BloomSettingsProvider.UserSettingsFolder = null; + BloomSettingsProvider.SetUserSettingsFolder(null); var folder = BloomSettingsProvider.GetUserSettingsFolder(); @@ -63,7 +63,7 @@ public void GetUserSettingsFolder_NoFolderNamed_IsThePerVersionFolderUnderLocalA [Test] public void GetUserSettingsFolder_FolderNamed_IsThatFolder() { - BloomSettingsProvider.UserSettingsFolder = _folder.Path; + BloomSettingsProvider.SetUserSettingsFolder(_folder.Path); Assert.That(BloomSettingsProvider.GetUserSettingsFolder(), Is.EqualTo(_folder.Path)); Assert.That( @@ -72,20 +72,34 @@ public void GetUserSettingsFolder_FolderNamed_IsThatFolder() ); } + [Test] + public void CommandLineArgumentsForChildBloom_NoFolderNamed_IsEmpty() + { + BloomSettingsProvider.SetUserSettingsFolder(null); + + Assert.That(BloomSettingsProvider.CommandLineArgumentsForChildBloom, Is.Empty); + } + + [Test] + public void CommandLineArgumentsForChildBloom_FolderNamed_NamesItForTheChild() + { + BloomSettingsProvider.SetUserSettingsFolder(_folder.Path); + + Assert.That( + BloomSettingsProvider.CommandLineArgumentsForChildBloom, + Is.EqualTo($"--user-settings-folder \"{_folder.Path}\"") + ); + } + [Test] public void SetPropertyValues_FolderNamed_WritesUserConfigThereAndReadsItBack() { - BloomSettingsProvider.UserSettingsFolder = _folder.Path; + BloomSettingsProvider.SetUserSettingsFolder(_folder.Path); var userConfig = Path.Combine(_folder.Path, "user.config"); Assert.That(File.Exists(userConfig), Is.False, "test setup: the folder starts empty"); - var property = new SettingsProperty("UserInterfaceLanguage") - { - PropertyType = typeof(string), - SerializeAs = SettingsSerializeAs.String, - DefaultValue = "", - }; - var context = new SettingsContext { ["GroupName"] = kGroupName }; + var property = MakeStringProperty(); + var context = MakeContext(); var writer = new BloomSettingsProvider(); writer.Initialize(null, null); @@ -116,5 +130,51 @@ public void SetPropertyValues_FolderNamed_WritesUserConfigThereAndReadsItBack() ); Assert.That(values["UserInterfaceLanguage"].SerializedValue, Is.EqualTo("fr")); } + + [Test] + public void Upgrade_FolderNamed_BringsNothingIn() + { + // A named folder holds exactly the settings its owner put there, so upgrading (which + // ApplicationSettingsBase does through IApplicationSettingsProvider) must not copy a + // previous version's user.config into it. On a machine with no earlier Bloom version + // there is nothing to copy anyway; on a developer's machine there usually is, which is + // exactly the case an automated run must be protected from. + BloomSettingsProvider.SetUserSettingsFolder(_folder.Path); + var userConfig = Path.Combine(_folder.Path, "user.config"); + Assert.That(File.Exists(userConfig), Is.False, "test setup: the folder starts empty"); + + var provider = new BloomSettingsProvider(); + provider.Initialize(null, null); + var property = MakeStringProperty(); + var asSettingsProvider = (IApplicationSettingsProvider)provider; + + asSettingsProvider.Upgrade(MakeContext(), new SettingsPropertyCollection { property }); + + Assert.That( + File.Exists(userConfig), + Is.False, + "Upgrade copied a previous version's user.config into the named folder" + ); + Assert.That( + asSettingsProvider.GetPreviousVersion(MakeContext(), property), + Is.Null, + "a named folder has no previous version" + ); + } + + private static SettingsProperty MakeStringProperty() + { + return new SettingsProperty("UserInterfaceLanguage") + { + PropertyType = typeof(string), + SerializeAs = SettingsSerializeAs.String, + DefaultValue = "", + }; + } + + private static SettingsContext MakeContext() + { + return new SettingsContext { ["GroupName"] = kGroupName }; + } } } diff --git a/src/BloomTests/ProgramTests.cs b/src/BloomTests/ProgramTests.cs index 8dcabe263cc2..e0f774fdf024 100644 --- a/src/BloomTests/ProgramTests.cs +++ b/src/BloomTests/ProgramTests.cs @@ -201,9 +201,14 @@ out var errorMessage Assert.That(errorMessage, Is.Null); Assert.That( - BloomSettingsProvider.UserSettingsFolder, + Program.StartupUserSettingsFolder, Is.EqualTo(@"C:\Temp\bloom-e2e-abc\user-settings") ); + Assert.That( + BloomSettingsProvider.GetUserSettingsFolder(), + Is.EqualTo(@"C:\Temp\bloom-e2e-abc\user-settings"), + "the parser did not hand the folder to the settings provider" + ); Assert.That(remainingArgs, Is.EqualTo(new[] { @"C:\Temp\Example.bloomcollection" })); Assert.That( Program.StartupRequestedPortSummary, @@ -223,7 +228,7 @@ out var errorMessage Assert.That(errorMessage, Is.Null); Assert.That( - BloomSettingsProvider.UserSettingsFolder, + BloomSettingsProvider.GetUserSettingsFolder(), Is.EqualTo( System.IO.Path.Combine( System.IO.Directory.GetCurrentDirectory(), @@ -234,7 +239,7 @@ out var errorMessage } [Test] - public void ParseStartupPortArguments_LeavesUserSettingsFolderNullWithoutArgument() + public void ParseStartupPortArguments_LeavesTheUsualUserSettingsFolderWithoutArgument() { Program.ParseStartupPortArguments( new[] { @"C:\Temp\Example.bloomcollection" }, @@ -242,7 +247,24 @@ out var errorMessage ); Assert.That(errorMessage, Is.Null); - Assert.That(BloomSettingsProvider.UserSettingsFolder, Is.Null); + Assert.That(Program.StartupUserSettingsFolder, Is.Null); + AssertProviderIsOnTheUsualFolder(); + } + + /// + /// The settings provider is keeping user.config in libpalaso's per-version folder under + /// %LOCALAPPDATA%, not in any folder a command line named. + /// + private static void AssertProviderIsOnTheUsualFolder() + { + Assert.That( + BloomSettingsProvider.GetUserSettingsFolder(), + Does.StartWith( + System.Environment.GetFolderPath( + System.Environment.SpecialFolder.LocalApplicationData + ) + ) + ); } [Test] @@ -257,7 +279,8 @@ out var errorMessage errorMessage, Is.EqualTo("Bloom requires a value after --user-settings-folder.") ); - Assert.That(BloomSettingsProvider.UserSettingsFolder, Is.Null); + Assert.That(Program.StartupUserSettingsFolder, Is.Null); + AssertProviderIsOnTheUsualFolder(); Assert.That(remainingArgs, Is.Empty); } @@ -273,7 +296,8 @@ out var errorMessage errorMessage, Does.StartWith("Bloom cannot use \"\" as the --user-settings-folder") ); - Assert.That(BloomSettingsProvider.UserSettingsFolder, Is.Null); + Assert.That(Program.StartupUserSettingsFolder, Is.Null); + AssertProviderIsOnTheUsualFolder(); Assert.That(remainingArgs, Is.Empty); } @@ -289,28 +313,32 @@ out var errorMessage errorMessage, Is.EqualTo("Bloom only accepts one --user-settings-folder argument.") ); + Assert.That(Program.StartupUserSettingsFolder, Is.Null); + AssertProviderIsOnTheUsualFolder(); Assert.That(remainingArgs, Is.Empty); } [Test] - public void ParseStartupPortArguments_ClearsUserSettingsFolderWhenALaterArgumentIsBad() + public void ParseStartupPortArguments_GivesTheProviderNoFolderWhenALaterArgumentIsBad() { - // A valid folder set before a bad argument must not survive: the launch is rejected, and - // startup must not open that folder's user.config before reporting the error. + // A valid folder before a bad argument must not reach the settings provider: the launch + // is rejected, and startup must not open that folder's user.config before reporting the + // error. Program.ParseStartupPortArguments( new[] { @"--user-settings-folder=C:\valid", "--vite-port", "70000" }, out var errorMessage ); Assert.That(errorMessage, Is.Not.Null); - Assert.That(BloomSettingsProvider.UserSettingsFolder, Is.Null); + Assert.That(Program.StartupUserSettingsFolder, Is.Null); + AssertProviderIsOnTheUsualFolder(); } [Test] - public void ParseStartupPortArguments_ClearsUserSettingsFolderWhenExperimentalFeaturesLackE2e() + public void ParseStartupPortArguments_GivesTheProviderNoFolderWhenExperimentalFeaturesLackE2e() { - // The experimental-features check rejects the launch after the loop, so it has to clear - // the folder the loop already accepted, the same as a rejection inside the loop does. + // The experimental-features check rejects the launch after the loop, so a folder the + // loop already accepted must not reach the provider either. Program.ParseStartupPortArguments( new[] { @@ -325,7 +353,8 @@ out var errorMessage errorMessage, Is.EqualTo("Bloom only accepts --experimental-features together with --e2e.") ); - Assert.That(BloomSettingsProvider.UserSettingsFolder, Is.Null); + Assert.That(Program.StartupUserSettingsFolder, Is.Null); + AssertProviderIsOnTheUsualFolder(); } // --- IsBenignUnobservedTaskSocketNoise: the Sentry BeforeSend filter for From b6380d7bb286d0807474acfa78e1c9e4d451b493 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:58:32 +0000 Subject: [PATCH 3/4] Fix BloomStarted event name prefix Co-authored-by: andrew-polk <5847219+andrew-polk@users.noreply.github.com> --- src/BloomFreezeDoctor.Protocol/DoctorSignals.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/BloomFreezeDoctor.Protocol/DoctorSignals.cs b/src/BloomFreezeDoctor.Protocol/DoctorSignals.cs index 3f336c7bae0e..04da78eefb6c 100644 --- a/src/BloomFreezeDoctor.Protocol/DoctorSignals.cs +++ b/src/BloomFreezeDoctor.Protocol/DoctorSignals.cs @@ -59,7 +59,7 @@ public static string QuitRequestName(int processId) => /// Polling stays as the backstop, and must: a Bloom too old to know about any of this cannot announce /// itself, and those are the Blooms most worth watching. /// - public static string BloomStartedName() => @"LocalBloomFreezeDoctor.bloomstarted"; + public static string BloomStartedName() => @"Local\BloomFreezeDoctor.bloomstarted"; /// Set by Bloom as it dies, to ask for a dump while the process still exists. public static string DumpRequestName(int processId) => From 68ef70159e0bd3b412396d5ec41a906260f2442c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 10 Sep 2026 16:59:31 +0000 Subject: [PATCH 4/4] Replace nested ternary in age description Co-authored-by: andrew-polk <5847219+andrew-polk@users.noreply.github.com> --- .../Outbox/YouTrackSubmitter.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/BloomFreezeDoctor.Core/Outbox/YouTrackSubmitter.cs b/src/BloomFreezeDoctor.Core/Outbox/YouTrackSubmitter.cs index cfd6fa9ee01f..22f52d8a0277 100644 --- a/src/BloomFreezeDoctor.Core/Outbox/YouTrackSubmitter.cs +++ b/src/BloomFreezeDoctor.Core/Outbox/YouTrackSubmitter.cs @@ -916,10 +916,16 @@ private static string ReadReport(QueuedBundle bundle) } } - private static string Describe(TimeSpan age) => - age.TotalDays >= 1 ? $"{age.TotalDays:F0} day(s)" - : age.TotalHours >= 1 ? $"{age.TotalHours:F0} hour(s)" - : $"{age.TotalMinutes:F0} minute(s)"; + private static string Describe(TimeSpan age) + { + if (age.TotalDays >= 1) + return $"{age.TotalDays:F0} day(s)"; + + if (age.TotalHours >= 1) + return $"{age.TotalHours:F0} hour(s)"; + + return $"{age.TotalMinutes:F0} minute(s)"; + } private static string Trim(string value) => value.Length <= 300 ? value : value.Substring(0, 299) + "…";