diff --git a/core-web/apps/dotcms-ui-e2e/src/pages/contentDrive.page.ts b/core-web/apps/dotcms-ui-e2e/src/pages/contentDrive.page.ts index 121d7e5bb0cd..71fcfb31b679 100644 --- a/core-web/apps/dotcms-ui-e2e/src/pages/contentDrive.page.ts +++ b/core-web/apps/dotcms-ui-e2e/src/pages/contentDrive.page.ts @@ -28,9 +28,15 @@ export class ContentDrivePage { readonly currentSiteHostname: Locator; readonly listTitles: Locator; readonly treeNodeLabels: Locator; + readonly allSiteContentRow: Locator; + readonly systemHostRow: Locator; readonly searchField: Locator; - readonly uploadIndicator: Locator; - readonly uploadProgress: Locator; + readonly statusToast: Locator; + readonly statusToastSummary: Locator; + readonly scopeBar: Locator; + readonly scopeBarSlot: Locator; + readonly scopeBarSummary: Locator; + readonly scopeBarToggle: Locator; readonly toasts: Locator; constructor(private page: Page) { @@ -45,9 +51,25 @@ export class ContentDrivePage { this.currentSiteHostname = this.sidebar.getByTestId('tree-node-label').first(); this.listTitles = page.getByTestId('item-title-text'); this.treeNodeLabels = this.sidebar.getByTestId('tree-node-label'); - // The toolbar's in-flight indicator, and the position it shows when a run reports one. - this.uploadIndicator = page.getByTestId('action-execution-indicator'); - this.uploadProgress = page.getByTestId('action-execution-progress'); + // The two entries that are not part of the hierarchy. They carry their own testids and no + // `tree-node-label`, which is why `currentSiteHostname` above still finds the site row. + this.allSiteContentRow = this.sidebar.getByTestId('all-site-content'); + this.systemHostRow = this.sidebar.getByTestId('system-host'); + // A run in flight is reported by the status toast, not by the toolbar. The toolbar used to + // draw an indicator at the end of the filter row (`action-execution-indicator`); that markup + // is gone, so anything still looking for it is asserting on a testid that cannot appear. + this.statusToast = page.getByTestId('dot-status-toast'); + this.statusToastSummary = page.getByTestId('status-toast-summary'); + // The bar above the listing: what is being shown, and the one control that changes it. + // Its slot is always in the DOM and opens by height, so visibility is the question to ask + // rather than presence. + this.scopeBar = page.getByTestId('scope-bar'); + // The wrapper that opens and closes by height. Assert against this rather than the bar + // inside it: Playwright's visibility ignores clipping by an ancestor, so the bar itself + // still measures as visible while this has squeezed it to nothing. + this.scopeBarSlot = page.getByTestId('scope-bar-slot'); + this.scopeBarSummary = page.getByTestId('scope-bar-summary'); + this.scopeBarToggle = page.getByTestId('scope-bar-toggle'); this.toasts = page.locator('.p-toast-message'); } @@ -291,10 +313,88 @@ export class ContentDrivePage { * learns the rules changed; without the indicator the batch looks finished when it is not. */ async expectHandedToBackground() { - await expect(this.toasts.filter({ hasText: 'in the background' }).first()).toBeVisible({ + // One surface says both halves now. The status toast carries the in-flight wording, which + // for a backgrounded batch is the sentence that tells the author the batch is theirs to + // leave — so its presence is the indicator, and there is no second element to check. + // It used to be two: a wide advisory toast plus the toolbar's indicator, which meant a + // backgrounded upload announced itself twice. + await expect(this.statusToastSummary.filter({ hasText: 'in the background' })).toBeVisible({ timeout: OUTCOME_TIMEOUT }); - await expect(this.uploadIndicator).toBeVisible(); + } + + /** What the status toast is saying right now, if anything. */ + async expectStatusToastContaining(text: string) { + await expect(this.statusToastSummary.filter({ hasText: text }).first()).toBeVisible({ + timeout: OUTCOME_TIMEOUT + }); + } + + /** + * The status toast has stopped reporting. + * + * A run that never clears its toast leaves the portlet claiming work is in flight forever, and + * the toast is sticky precisely so it cannot time itself out -- which makes it the store's job + * to end it, and therefore worth asserting. + */ + async expectStatusToastGone() { + await expect(this.statusToastSummary).toHaveCount(0, { timeout: OUTCOME_TIMEOUT }); + } + + /** + * Whether the status toast is what a click would land on at its own centre. + * + * Asked of the browser's hit-testing rather than by clicking a control underneath. The first + * version of this clicked the listing's rows-per-page box on the claim that it is "always + * enabled"; it is not -- an empty folder disables it, and a test that seeds its own folder + * always starts empty, so the check failed on the control rather than on the toast. + * + * `elementFromPoint` asks the question directly and needs nothing beneath the toast at all. + */ + async statusToastTakesClicksAtItsCentre(): Promise { + const box = await this.statusToast.boundingBox(); + + if (!box) { + throw new Error('no status toast on screen to test'); + } + + return this.page.evaluate( + ([x, y]) => + !!document.elementFromPoint(x, y)?.closest('[data-testid="dot-status-toast"]'), + [box.x + box.width / 2, box.y + box.height / 2] + ); + } + + /** Flips the System Host toggle and waits for the listing it re-requests. */ + async toggleSystemHostInScopeBar() { + const listing = this.page.waitForResponse( + (response) => response.url().includes('/v1/drive/search') && response.ok() + ); + await this.scopeBarToggle.click(); + await listing; + } + + /** Opens the New menu and returns the labels it offers. */ + async openNewMenu(): Promise { + await this.toolbar.getByTestId('add-new-button').click(); + const items = this.page.getByRole('menuitem'); + await expect(items.first()).toBeVisible({ timeout: 10000 }); + + return items.allInnerTexts(); + } + + /** + * The path the folder dialog says a new folder will land on. + * + * Read rather than asserted here because the wrong value is not a missing element: the builder + * used to paste a location that is not a folder path straight after the hostname, so the field + * was populated and confidently wrong. + */ + async folderDialogPath(): Promise { + const path = this.page.getByTestId('folder-path-preview'); + await expect(path).toBeVisible({ timeout: 10000 }); + + return (await path.innerText()).trim(); } /** A message the author can read, whatever severity it arrived with. */ @@ -374,6 +474,58 @@ export class ContentDrivePage { async expectNoSingleFileWarning() { await expect(this.page.locator('.p-toast-message-warn')).toHaveCount(0); } + + /** Selects the All Site Content entry and waits for the listing it triggers. */ + async selectAllSiteContent() { + await this.selectSidebarEntry(this.allSiteContentRow); + } + + /** Selects the System Host entry and waits for the listing it triggers. */ + async selectSystemHost() { + await this.selectSidebarEntry(this.systemHostRow); + } + + /** + * Clicks a sidebar entry and waits for the listing request the click sets off. + * + * Armed before the click, not after: the response can land first, and then a wait registered + * afterwards never resolves. + */ + private async selectSidebarEntry(row: Locator) { + // Clicking the entry you are already on changes no location, so the store re-requests + // nothing and a wait for the listing never resolves -- the test then dies on its own + // timeout with "Page closed", which says nothing about the entry. The drive lands on all + // site content, so this is the ordinary case for a test that starts there. + if ((await row.getAttribute('aria-current')) === 'true') { + return; + } + + const listing = this.page.waitForResponse( + (response) => response.url().includes('/v1/drive/search') && response.ok() + ); + await row.click(); + await listing; + } + + /** + * Asserts which sidebar entry reads as the current one. + * + * `aria-current` rather than a class: the rows announce selection to assistive tech through + * it, so asserting on it checks the thing that actually has to be right. + */ + async expectSelectedEntry(entry: 'all' | 'system-host' | 'neither') { + const row = entry === 'system-host' ? this.systemHostRow : this.allSiteContentRow; + await expect(row).toHaveAttribute('aria-current', entry === 'neither' ? /^$/ : 'true', { + timeout: entry === 'neither' ? 2000 : undefined + }); + } + + /** Whether an entry currently announces itself as the selected one. */ + async isEntrySelected(entry: 'all' | 'system-host') { + const row = entry === 'system-host' ? this.systemHostRow : this.allSiteContentRow; + + return (await row.getAttribute('aria-current')) === 'true'; + } } /** A tiny in-memory PNG, so the tests carry no fixture files. */ diff --git a/core-web/apps/dotcms-ui-e2e/src/tests/content-drive/content-drive-browse-scopes.spec.ts b/core-web/apps/dotcms-ui-e2e/src/tests/content-drive/content-drive-browse-scopes.spec.ts new file mode 100644 index 000000000000..067c0224007d --- /dev/null +++ b/core-web/apps/dotcms-ui-e2e/src/tests/content-drive/content-drive-browse-scopes.spec.ts @@ -0,0 +1,339 @@ +import { ContentDrivePage } from '@pages'; +import { expect } from '@playwright/test'; + +import { ContentDriveTree } from './helpers/content-drive-tree'; + +import { test } from '../../fixtures/content-drive.fixture'; + +/** + * Journey: Content Drive browse scopes (#37426). + * + * The sidebar offers three selections rather than one: All Site Content at the top, the site + * hierarchy in the middle, and System Host at the bottom. These are the two independent tests the + * spec defines for user stories 1 and 2, plus the selection rule that binds them — exactly one + * entry is ever current. + * + * Kept to what only a browser can answer. Which items each scope returns is pinned by the + * integration tests against the endpoint; what these cover is that the entries exist, that + * choosing one changes what is listed, and that the selection cannot land in two places at once. + */ +test.describe('Content Drive Browse Scopes', () => { + test('offers all site content and System Host around the hierarchy @critical', async ({ + adminPage, + apiHelpers + }) => { + const site = await apiHelpers.getDefaultSite(); + + const drive = new ContentDrivePage(adminPage); + const tree = new ContentDriveTree(adminPage); + + await drive.goTo(); + + // The site row is still the tree's first node: the two new entries are buttons outside the + // hierarchy and carry no `tree-node-label`, which is what keeps this assertion working. + await drive.expectSiteHostname(site.hostname); + await tree.expectVisible(); + + await expect(drive.allSiteContentRow).toBeVisible(); + await expect(drive.systemHostRow).toBeVisible(); + }); + + test('puts the entries above and below the hierarchy, not inside it @critical', async ({ + adminPage + }) => { + // Structure, not decoration: System Host stays reachable however many folders are + // expanded, because the hierarchy between them is the only part that scrolls. + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + + const all = await drive.allSiteContentRow.boundingBox(); + const systemHost = await drive.systemHostRow.boundingBox(); + + expect(all).toBeTruthy(); + expect(systemHost).toBeTruthy(); + expect(all?.y ?? 0).toBeLessThan(systemHost?.y ?? 0); + expect(systemHost?.y ?? 0).toBeGreaterThan((all?.y ?? 0) + (all?.height ?? 0)); + }); + + test('keeps exactly one entry current as the user moves between them @critical', async ({ + adminPage + }) => { + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + + await drive.selectSystemHost(); + await drive.expectSelectedEntry('system-host'); + expect(await drive.isEntrySelected('all')).toBe(false); + + await drive.selectAllSiteContent(); + await drive.expectSelectedEntry('all'); + expect(await drive.isEntrySelected('system-host')).toBe(false); + }); + + test('lists content from inside folders under all site content @critical', async ({ + adminPage, + apiHelpers, + testSuffix + }) => { + // The distinction the feature exists for: a file inside a folder is absent from the site + // root and present in the flat view. + const site = await apiHelpers.getDefaultSite(); + const folderName = `cd-scope-${testSuffix}`; + await apiHelpers.createFolders(site.hostname, [`/${folderName}`]); + + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + await drive.openFolder(folderName); + await drive.dropFilesOnList([`scoped-${testSuffix}.png`]); + await drive.expectUploadedTitle(folderName, `scoped-${testSuffix}.png`); + + await drive.selectAllSiteContent(); + await drive.expectListContainsTitle(`scoped-${testSuffix}.png`); + }); + + test('restores the tree selection when the user goes back to a folder @critical', async ({ + adminPage, + apiHelpers, + testSuffix + }) => { + // Reported from the browser: Back restored the URL and the listing, while the tree showed + // nothing selected — so the sidebar stopped agreeing with what it was displaying. The two + // standalone entries were never affected, because they derive their state from the + // location; the tree's is stored, and nothing brought it back in line. + const site = await apiHelpers.getDefaultSite(); + const folderName = `cd-back-${testSuffix}`; + await apiHelpers.createFolders(site.hostname, [`/${folderName}`]); + + const drive = new ContentDrivePage(adminPage); + const tree = new ContentDriveTree(adminPage); + + await drive.goTo(); + await drive.openFolder(folderName); + await tree.expectFolderSelected(folderName); + + await drive.selectSystemHost(); + await drive.expectSelectedEntry('system-host'); + + await adminPage.goBack(); + + await tree.expectFolderSelected(folderName); + expect(await drive.isEntrySelected('system-host')).toBe(false); + }); + + test('carries the selection in the URL so a reload reopens it @critical', async ({ + adminPage + }) => { + // One value says where the drive is browsing, so a shared link cannot disagree with + // itself about which entry was open. + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + + await drive.selectSystemHost(); + expect(adminPage.url()).toContain('SYSTEM_HOST'); + + await adminPage.reload(); + await drive.expectSelectedEntry('system-host'); + }); + + test('leaves the hierarchy unselected on System Host, and stays there @critical', async ({ + adminPage + }) => { + // Reported from the browser as "the root of the site selected and the system host + // selected". Two faults met here. The hierarchy load resolved the reserved location + // `SYSTEM_HOST` as the folder path `/SYSTEM_HOST/`, found nothing, and fell back to + // selecting the site row -- so two entries read as current. Then the shell, which derives + // the location *from* the selected node, took that site row and rewrote the location back + // to the site root, pushing the author out of the scope they had just chosen. + // + // A reload is the case that made it reliable: on a cold start the location is already + // System Host when the tree is built, so the fallback ran every time. + const drive = new ContentDrivePage(adminPage); + const tree = new ContentDriveTree(adminPage); + + await drive.goTo(); + await drive.selectSystemHost(); + await adminPage.reload(); + + await drive.expectSelectedEntry('system-host'); + await tree.expectNothingSelected(); + expect(adminPage.url()).toContain('SYSTEM_HOST'); + }); + + test('offers no new folder on System Host @critical', async ({ adminPage }) => { + // System Host lists no folders, and its entry opens no tree, so one created there could + // never be shown again by this portlet. The dialog could not even name where it would + // land: the location is a reserved word, and pasting it after the hostname produced + // `//demo.dotcms.comSYSTEM_HOST/`. + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + + await drive.selectSystemHost(); + const onSystemHost = await drive.openNewMenu(); + + expect(onSystemHost.join(' ')).not.toContain('Folder'); + // The other half of the rule: System Host holds content, and adding some is the whole + // reason the scope accepts new items at all. + expect(onSystemHost.join(' ')).toContain('Content'); + }); + + test('names a path that resolves when creating a folder @critical', async ({ + adminPage, + apiHelpers + }) => { + // The preview is built from the location the drive is open on, and only one of the three + // kinds of location is a folder path. Asserted as "starts with the site and carries no + // reserved word" rather than as an exact string, so it holds for whichever site the run + // lands on. + const site = await apiHelpers.getDefaultSite(); + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + + await drive.selectAllSiteContent(); + await drive.openNewMenu(); + await adminPage.getByRole('menuitem', { name: 'Folder' }).click(); + + const path = await drive.folderDialogPath(); + + expect(path).not.toContain('SYSTEM_HOST'); + expect(path).toBe(`//${site.hostname}/`); + }); + + test('refreshes the listing after an upload lands on System Host @critical', async ({ + adminPage, + testSuffix + }) => { + // The grid reloads only when the finished run names the folder on screen, and both sides + // of that comparison were built from the switcher's site glued to the location. On System + // Host that gave `//demo.dotcms.comSYSTEM_HOST` for the listing and `//demo.dotcms.com` + // for the batch -- neither naming where the files actually went, and never equal. The + // files arrived; the listing they arrived in sat stale. + const drive = new ContentDrivePage(adminPage); + const title = `sys-upload-${testSuffix}.png`; + + await drive.goTo(); + await drive.selectSystemHost(); + await drive.chooseFilesForUpload([title]); + + // No reload of our own: the refresh arriving by itself is the whole assertion. + await drive.expectListContainsTitle(title); + }); + + test('reports a run in flight and stops when it settles @critical', async ({ + adminPage, + apiHelpers, + testSuffix + }) => { + // The toast replaced the toolbar indicator, and it is raised `sticky` so it cannot time + // itself out. That makes ending it the store's job rather than PrimeNG's, which is the + // half worth pinning: a run that never clears leaves the portlet claiming work is in + // flight forever. + const site = await apiHelpers.getDefaultSite(); + const folderName = `cd-toast-${testSuffix}`; + await apiHelpers.createFolders(site.hostname, [`/${folderName}`]); + + const drive = new ContentDrivePage(adminPage); + + try { + await drive.goTo(); + await drive.openFolder(folderName); + await drive.chooseFilesForUpload([`toast-${testSuffix}.png`]); + + // Its own words, not the workflow sentence: an upload puts files INTO a place rather + // than applying an action TO content, which is what "Applying Upload to ..." claimed. + await drive.expectStatusToastContaining('Uploading'); + await drive.expectStatusToastGone(); + } finally { + await apiHelpers.deleteFolders(site.hostname, [`/${folderName}`]); + } + }); + + test('leaves the page controls clickable while a run is reported @critical', async ({ + adminPage, + apiHelpers, + testSuffix + }) => { + // The status toast is a fixed box at the bottom centre of the viewport, which is where the + // paginator lives. It reported an upload from directly on top of the page controls and + // swallowed the click, so the page never changed and the listing sat on one page while the + // paginator read as another. + // + // Nothing in the toast is clickable, so nothing in it should take a click. Asked of the + // browser's own hit-testing at the toast's centre, which needs nothing enabled underneath + // -- a freshly seeded folder is empty, and an empty listing disables its page controls. + const site = await apiHelpers.getDefaultSite(); + const folderName = `cd-click-${testSuffix}`; + await apiHelpers.createFolders(site.hostname, [`/${folderName}`]); + + const drive = new ContentDrivePage(adminPage); + + try { + await drive.goTo(); + await drive.openFolder(folderName); + await drive.chooseFilesForUpload([`click-${testSuffix}.png`]); + await drive.expectStatusToastContaining('Uploading'); + + expect(await drive.statusToastTakesClicksAtItsCentre()).toBe(false); + } finally { + await apiHelpers.deleteFolders(site.hostname, [`/${folderName}`]); + } + }); + + test('says what all site content is showing, and only there @critical', async ({ + adminPage + }) => { + // The bar carries the sentence and the System Host toggle that used to be a chip in the + // filter row. Only all site content gets one: the site root is this site's root and + // nothing else, and System Host is shared content and nothing else, so neither leaves a + // sentence anything to qualify. + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + + await drive.selectAllSiteContent(); + await expect(drive.scopeBar).toBeVisible(); + await expect(drive.scopeBarToggle).toBeVisible(); + + await drive.selectSystemHost(); + // Retried rather than sampled once: the bar closes over 300ms and `selectSystemHost` + // resolves on the listing response, which lands while it is still shrinking. A one-shot + // read of its height sees it mid-animation and calls it open — which is what this test + // did, and it is the exact thing the repo's own conventions warn against. + await expect(drive.scopeBarSlot).toBeHidden(); + }); + + test('flips the sentence with the System Host toggle @critical', async ({ adminPage }) => { + // The sentence and the switch are one statement: if the toggle can say "included" while + // the words say "excluded", the bar is worse than no bar. + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + await drive.selectAllSiteContent(); + + const before = (await drive.scopeBarSummary.innerText()).trim(); + await drive.toggleSystemHostInScopeBar(); + const after = (await drive.scopeBarSummary.innerText()).trim(); + + expect(after).not.toBe(before); + // Whichever way round the run starts, the pair must be the two halves of the same choice. + expect([before, after].sort()).toEqual( + [ + 'All Files in site (System Host shared files excluded)', + 'All Files in site (System Host shared files included)' + ].sort() + ); + }); + + test('carries the toggle into the URL so a reload keeps it @critical', async ({ + adminPage + }) => { + // The filter is written either way rather than cleared, so the applied state is spelled + // out rather than implied by an absent key that happens to read as on. + const drive = new ContentDrivePage(adminPage); + await drive.goTo(); + await drive.selectAllSiteContent(); + + await drive.toggleSystemHostInScopeBar(); + const summary = (await drive.scopeBarSummary.innerText()).trim(); + + await adminPage.reload(); + await expect(drive.scopeBarSummary).toHaveText(summary); + }); +}); diff --git a/core-web/apps/dotcms-ui-e2e/src/tests/content-drive/helpers/content-drive-tree.ts b/core-web/apps/dotcms-ui-e2e/src/tests/content-drive/helpers/content-drive-tree.ts index b20f7b0a0550..dd67de68a809 100644 --- a/core-web/apps/dotcms-ui-e2e/src/tests/content-drive/helpers/content-drive-tree.ts +++ b/core-web/apps/dotcms-ui-e2e/src/tests/content-drive/helpers/content-drive-tree.ts @@ -87,6 +87,27 @@ export class ContentDriveTree { await expect(node).toBeVisible({ timeout: 10000 }); } + /** + * How many rows inside the hierarchy read as selected. + * + * The two standalone entries reuse `p-tree-node-content` and `p-tree-node-selected` so they are + * styled like the tree they sit around, which means a document-wide count of the class answers + * the wrong question. Scoped to the tree, this is the assertion that the sidebar is not + * claiming the user is in two places at once. + */ + async expectNothingSelected() { + // `hierarchy-scroll`, not `sidebar`: this class's root is the whole panel, and the two + // standalone entries deliberately reuse `p-tree-node-content` / `p-tree-node-selected` so + // they match the rows they sit around. Counting from the panel would therefore count the + // selected System Host button as a selected tree row, and the assertion would pass or fail + // for the wrong reason. That wrapper holds the hierarchy and nothing else. + await expect( + this.root + .getByTestId('hierarchy-scroll') + .locator('.p-tree-node-content.p-tree-node-selected') + ).toHaveCount(0, { timeout: 10000 }); + } + /** * The state-aware folder icon the shared tree renders on a folder row (#37362). * diff --git a/core-web/libs/dotcms-models/src/lib/dot-content-drive.model.ts b/core-web/libs/dotcms-models/src/lib/dot-content-drive.model.ts index bc8f5b9740e4..dacdd3a2c425 100644 --- a/core-web/libs/dotcms-models/src/lib/dot-content-drive.model.ts +++ b/core-web/libs/dotcms-models/src/lib/dot-content-drive.model.ts @@ -191,6 +191,12 @@ export interface DotContentDriveQueryFilters { text: string; } +/** + * Which slice of content a Content Drive listing is asked for: the whole current site at any + * depth, only what sits at the site root, or System Host alone. + */ +export type DotContentDriveBrowseScope = 'ALL' | 'ROOT' | 'SYSTEM_HOST'; + /** * Request body for the /api/v1/drive/search endpoint. * @@ -227,6 +233,15 @@ export interface DotContentDriveSearchRequest { */ includeSystemHost?: boolean; + /** + * Which slice of content to list. Omitting it means today's behavior, and it carries no + * default for that reason: at the site root an omitted scope and `ALL` agree, but inside a + * folder they do not, so defaulting it would turn folder requests into listings of every + * descendant. Only valid with a site-root `assetPath`; naming one alongside a folder path is + * refused by the endpoint. + */ + browseScope?: DotContentDriveBrowseScope; + /** * List of language identifiers to include in the search. * Supports both language codes (e.g., "en", "es") and language IDs. diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.spec.ts index 838a5bd8d292..89cab996098b 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.spec.ts @@ -8,6 +8,8 @@ import { import { MockComponent } from 'ng-mocks'; import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { signal } from '@angular/core'; + import { DotMessageService } from '@dotcms/data-access'; import { DOT_PALETTE_PERSIST_PREFERENCES, @@ -23,6 +25,9 @@ import { DotContentDriveStore } from '../../../store/dot-content-drive.store'; const SELECTED_VARIABLE = 'Blog'; +// Real signal: the component reads it in a computed, and a vi.fn cannot invalidate one. +const systemHostSelected = signal(false); + describe('DotContentDriveDialogContentTypeSelectorComponent', () => { let spectator: Spectator; let store: SpyObject>; @@ -48,7 +53,8 @@ describe('DotContentDriveDialogContentTypeSelectorComponent', () => { path: vi.fn().mockReturnValue('/about-us/'), selectedNode: vi .fn() - .mockReturnValue({ data: { type: 'folder', inode: 'inode-1' } }) + .mockReturnValue({ data: { type: 'folder', inode: 'inode-1' } }), + $systemHostSelected: systemHostSelected }), mockProvider(DotContentDriveNavigationService, { createContent: vi.fn() diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.ts index a6dc1c098cf7..7bc743aa69a5 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component.ts @@ -12,6 +12,7 @@ import { } from '@dotcms/portlets/dot-ema/ui'; import { DotMessagePipe } from '@dotcms/ui'; +import { SYSTEM_HOST } from '../../../shared/constants'; import { DotContentDriveNavigationService } from '../../../shared/services/dot-content-drive-navigation.service'; import { DotContentDriveStore } from '../../../store/dot-content-drive.store'; @@ -70,6 +71,13 @@ export class DotContentDriveDialogContentTypeSelectorComponent { * At the site root both fall back to the current site (empty path / no inode). */ #getCurrentFolder(): { folderPath?: string; folderInode?: string } { + // System Host is a destination in its own right, and the site in the switcher is only + // context while it is selected. Pasting the location onto the hostname would also produce + // `demo.dotcms.comSYSTEM_HOST` for the reserved word, which resolves to nothing. + if (this.#store.$systemHostSelected()) { + return { folderInode: SYSTEM_HOST.identifier }; + } + const hostname = this.#store.currentSite()?.hostname; const path = this.#store.path(); const data = this.#store.selectedNode()?.data; diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.spec.ts index 2c4635b5019a..3fe4916c361d 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.spec.ts @@ -16,7 +16,7 @@ import { createFakeSite, MockDotMessageService } from '@dotcms/utils-testing'; import { DotContentDriveDialogFolderComponent } from './dot-content-drive-dialog-folder.component'; -import { DEFAULT_FILE_ASSET_TYPES } from '../../../shared/constants'; +import { DEFAULT_FILE_ASSET_TYPES, SYSTEM_HOST_PATH } from '../../../shared/constants'; import { DotContentDriveStore } from '../../../store/dot-content-drive.store'; const mockSite = createFakeSite({ @@ -230,6 +230,17 @@ describe('DotContentDriveDialogFolderComponent', () => { expect(component.$finalPath()).toBe('//demo.dotcms.com/'); }); + it('should not paste a location that is not a folder path into the preview', () => { + // System Host reaches the dialog as the location `SYSTEM_HOST`, which is a reserved + // word rather than a path — that is what tells it apart from a folder. Concatenated + // onto the hostname it produced `//demo.dotcms.comSYSTEM_HOST/`, a path that resolves + // to nothing, and the dialog showed it to the user as where their folder would land. + store.path.mockReturnValue(SYSTEM_HOST_PATH); + component.folderForm.get('name')?.setValue('new-folder'); + + expect(component.$finalPath()).toBe('//demo.dotcms.com/new-folder/'); + }); + it('should handle path with trailing slash', () => { store.path.mockReturnValue('/documents/'); component.folderForm.patchValue({ name: 'new-folder' }); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.ts index e8acfbca4bf9..39028cb35a7d 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component.ts @@ -36,7 +36,8 @@ import { DotFieldRequiredDirective, DotMessagePipe } from '@dotcms/ui'; import { SUGGESTED_ALLOWED_FILE_EXTENSIONS, DEFAULT_FILE_ASSET_TYPES, - FOLDER_UPLOAD_BEHAVIOR_OPTIONS + FOLDER_UPLOAD_BEHAVIOR_OPTIONS, + ROOT_PATH } from '../../../shared/constants'; import { DotContentDriveStore } from '../../../store/dot-content-drive.store'; interface FolderForm { @@ -436,13 +437,21 @@ export class DotContentDriveDialogFolderComponent { * different folder entirely — saving would 404, or silently overwrite a same-named folder under * the open one. * + * Only a location that *is* a folder path anchors anything. The sidebar can select two things + * that are not: all site content, which is the absence of a location, and System Host, a + * reserved word that can never be mistaken for a path precisely because it does not start with + * `/`. Pasted onto the hostname the reserved word produced `//demo.dotcms.comSYSTEM_HOST/`, a + * path resolving to nothing, and the dialog showed it as where the folder would land. + * * @returns {string} The parent path, e.g. `/application/blog` or `''` at the site root */ #getParentPath(): string { const folder = this.$folder(); if (!folder) { - return this.#store.path()?.replace(/\/$/, '') ?? ''; + const location = this.#store.path() ?? ''; + + return location.startsWith(ROOT_PATH) ? location.replace(/\/$/, '') : ''; } const withoutTrailingSlash = folder.path.replace(/\/$/, ''); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.html b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.html new file mode 100644 index 000000000000..3251070218be --- /dev/null +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.html @@ -0,0 +1,19 @@ +
+ + + {{ $summaryKey() | dm }} + + + + +
diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.spec.ts new file mode 100644 index 000000000000..f2e1e0c330db --- /dev/null +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.spec.ts @@ -0,0 +1,75 @@ +import { Spectator, createComponentFactory, mockProvider } from '@openng/spectator/vitest'; +import { vi } from 'vitest'; + +import { signal } from '@angular/core'; + +import { DotMessageService } from '@dotcms/data-access'; +import { SHARED_ASSETS_DISABLED_VALUE, SHARED_ASSETS_FILTER_KEY } from '@dotcms/ui'; +import { MockDotMessageService } from '@dotcms/utils-testing'; + +import { DotContentDriveScopeBarComponent } from './dot-content-drive-scope-bar.component'; + +import { DotContentDriveStore } from '../../store/dot-content-drive.store'; + +describe('DotContentDriveScopeBarComponent', () => { + let spectator: Spectator; + + const filtersSignal = signal>({}); + + const createComponent = createComponentFactory({ + component: DotContentDriveScopeBarComponent, + componentProviders: [ + mockProvider(DotContentDriveStore, { + filters: filtersSignal, + getFilterValue: vi.fn((key: string) => filtersSignal()[key]), + patchFilters: vi.fn() + }) + ], + providers: [ + { + provide: DotMessageService, + useValue: new MockDotMessageService({ + 'content-drive.scope-bar.all-site-content.excluded': + 'All Files in site (System Host shared files excluded)', + 'content-drive.scope-bar.all-site-content.included': + 'All Files in site (System Host shared files included)', + 'content-drive.scope-bar.include-system-host': 'Include System Host:', + 'content-drive.scope-bar.off': 'Off' + }) + } + ] + }); + + beforeEach(() => { + filtersSignal.set({}); + spectator = createComponent(); + }); + + const summary = () => spectator.query('[data-testid="scope-bar-summary"]')?.textContent ?? ''; + + it('should say shared files are included while the toggle is on', () => { + // On is the default everywhere: the endpoint's form defaults the flag to true and an + // absent key reads as on, so the sentence has to agree with that rather than with silence. + expect(summary()).toContain('included'); + }); + + it('should say shared files are excluded once the toggle is off', () => { + filtersSignal.set({ [SHARED_ASSETS_FILTER_KEY]: SHARED_ASSETS_DISABLED_VALUE }); + spectator.detectChanges(); + + expect(summary()).toContain('excluded'); + }); + + it('should write the filter when the toggle is flipped', () => { + // The control the toolbar chip used to be. It writes the state either way rather than + // clearing the key, so the applied filter is spelled out in the URL instead of implied by + // an absence that reads as on. + const store = spectator.inject(DotContentDriveStore, true); + + spectator.click('[data-testid="scope-bar-toggle"] input'); + + expect(store.patchFilters).toHaveBeenCalledWith({ + [SHARED_ASSETS_FILTER_KEY]: SHARED_ASSETS_DISABLED_VALUE + }); + }); +}); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.ts new file mode 100644 index 000000000000..fac052e6ae1f --- /dev/null +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component.ts @@ -0,0 +1,80 @@ +import { ChangeDetectionStrategy, Component, computed, inject } from '@angular/core'; +import { FormsModule } from '@angular/forms'; + +import { ToggleSwitchModule } from 'primeng/toggleswitch'; + +import { + DotMessagePipe, + SHARED_ASSETS_DISABLED_VALUE, + SHARED_ASSETS_ENABLED_VALUE, + SHARED_ASSETS_FILTER_KEY +} from '@dotcms/ui'; + +import { DotContentDriveStore } from '../../store/dot-content-drive.store'; + +/** + * Says what the listing is showing, and carries the one control that changes it. + * + * Only all site content gets a bar. The other two scopes answer the question it asks by being + * chosen: the site root is this site's root and nothing else, and System Host is shared content and + * nothing else — so there is nothing left for a sentence to qualify or a toggle to decide. + * + * The toggle replaced the `Show System Host` chip that used to sit in the filter row. Same filter, + * same values; what changes is that the control now sits beside the sentence describing its effect, + * rather than in a row of chips that answer a different kind of question. + */ +@Component({ + selector: 'dot-content-drive-scope-bar', + imports: [DotMessagePipe, ToggleSwitchModule, FormsModule], + templateUrl: './dot-content-drive-scope-bar.component.html', + changeDetection: ChangeDetectionStrategy.OnPush, + host: { class: 'block' }, + styles: [ + ` + /* A "small" size input on p-toggleswitch does nothing here: the dotCMS Lara preset + defines no small variant, so no p-toggleswitch-sm class is emitted and no + --p-toggleswitch-sm-* token exists to pick up. The size is set through the tokens the + theme DOES define, scoped to this bar so no other switch in the app changes. + + Two thirds of the default (3rem / 1.75rem / 1.25rem): this control annotates a + sentence rather than sitting in a form, so it should read as part of the line. */ + :host { + --p-toggleswitch-width: 2rem; + --p-toggleswitch-height: 1.125rem; + --p-toggleswitch-handle-size: 0.75rem; + } + ` + ] +}) +export class DotContentDriveScopeBarComponent { + readonly #store = inject(DotContentDriveStore); + + /** + * Whether shared content is in the listing. + * + * "Off only when explicitly off": the endpoint's own form defaults the flag to true and an + * absent key reads as on, so anything other than the disabled value means included. + */ + protected readonly $includesSystemHost = computed( + () => this.#store.getFilterValue(SHARED_ASSETS_FILTER_KEY) !== SHARED_ASSETS_DISABLED_VALUE + ); + + /** The sentence, which has to agree with the toggle rather than describe a fixed scope. */ + protected readonly $summaryKey = computed(() => + this.$includesSystemHost() + ? 'content-drive.scope-bar.all-site-content.included' + : 'content-drive.scope-bar.all-site-content.excluded' + ); + + /** + * Writes the state either way rather than clearing the key, so the applied filter is spelled + * out in the URL instead of being implied by an absence that happens to read as on. + */ + protected onToggle(): void { + this.#store.patchFilters({ + [SHARED_ASSETS_FILTER_KEY]: this.$includesSystemHost() + ? SHARED_ASSETS_DISABLED_VALUE + : SHARED_ASSETS_ENABLED_VALUE + }); + } +} diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.html b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.html index f75390c816b2..a5897d648936 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.html +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.html @@ -1,11 +1,68 @@ - + + + + +
+ +
+ + +@if ($systemHostVisible()) { + + +} diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.spec.ts index 3eb5de0d2554..8ed7fb7d86e2 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.spec.ts @@ -1,7 +1,13 @@ -import { createComponentFactory, mockProvider, Spectator } from '@openng/spectator/vitest'; +import { + byTestId, + createComponentFactory, + mockProvider, + Spectator +} from '@openng/spectator/vitest'; import { of } from 'rxjs'; import { Mock, Mocked, vi } from 'vitest'; +import { signal } from '@angular/core'; import { fakeAsync, tick } from '@angular/core/testing'; import { TreeNodeExpandEvent, TreeNodeSelectEvent } from 'primeng/tree'; @@ -23,6 +29,7 @@ import { createFakeSite } from '@dotcms/utils-testing'; import { DotContentDriveSidebarComponent } from './dot-content-drive-sidebar.component'; +import { SYSTEM_HOST } from '../../shared/constants'; import { DotContentDriveStore } from '../../store/dot-content-drive.store'; import { createSiteNode } from '../../utils/tree-folder.utils'; @@ -99,6 +106,15 @@ describe('DotContentDriveSidebarComponent', () => { } ]; + // A real signal, not a vi.fn: the row's selected state is read in an OnPush template, so it + // only re-renders when a signal it reads is invalidated. Same pattern the toolbar spec uses. + const allSiteContentSelected = signal(false); + const systemHostSelected = signal(false); + // Drives the System Host row's drop gate the way the store's own lookup would. + const systemHostCanAddChildren = signal(true); + // And whether the row is offered at all. True unless the server refused the lookup outright. + const systemHostCanRead = signal(true); + const createComponent = createComponentFactory({ component: DotContentDriveSidebarComponent, imports: [DotTreeFolderComponent], @@ -108,6 +124,8 @@ describe('DotContentDriveSidebarComponent', () => { }), mockProvider(DotContentDriveStore, { initContentDrive: vi.fn(), + systemHostCanAddChildren: systemHostCanAddChildren, + systemHostCanRead: systemHostCanRead, currentSite: vi.fn().mockReturnValue(mockSiteDetails), isTreeExpanded: vi.fn().mockReturnValue(true), removeFilter: vi.fn(), @@ -128,7 +146,11 @@ describe('DotContentDriveSidebarComponent', () => { loadChildFolders: vi.fn(), patchContextMenu: vi.fn(), updateFolders: vi.fn(), - setSelectedNode: vi.fn() + setSelectedNode: vi.fn(), + selectAllSiteContent: vi.fn(), + selectSystemHost: vi.fn(), + $allSiteContentSelected: allSiteContentSelected, + $systemHostSelected: systemHostSelected }), mockProvider(DotMessageService, { get: vi.fn().mockImplementation((key: string) => key) @@ -137,6 +159,9 @@ describe('DotContentDriveSidebarComponent', () => { }); beforeEach(() => { + allSiteContentSelected.set(false); + systemHostSelected.set(false); + spectator = createComponent({ providers: [ mockProvider(DotFolderService, { @@ -150,6 +175,213 @@ describe('DotContentDriveSidebarComponent', () => { spectator.detectChanges(); }); + describe('all site content', () => { + const row = () => spectator.query(byTestId('all-site-content')); + + it('should offer a row above the hierarchy', () => { + expect(row()).toBeTruthy(); + }); + + it('should ask the store for all site content when the row is chosen', () => { + spectator.click(byTestId('all-site-content')); + + expect(contentDriveStore.selectAllSiteContent).toHaveBeenCalled(); + }); + + it('should be reachable by keyboard, not only by pointer', () => { + // The hierarchy beside it is a tree with its own arrow-key handling, so this row has to + // carry its own semantics rather than inheriting the tree's. + expect(row()?.tagName.toLowerCase()).toBe('button'); + }); + + it('should not read as current while a folder is being browsed', () => { + // The default mock browses '/test/path'. + expect(row()?.getAttribute('aria-current')).toBeNull(); + }); + + it('should read as current when the drive carries no location', () => { + // `aria-current`, not `aria-selected`: the latter is only meaningful on roles like + // option, tab or treeitem, and on a button it is dropped from the accessibility tree + // outright — which is how this was caught, as a row that announced nothing and looked + // identical whether or not it was the view you were on. + allSiteContentSelected.set(true); + + spectator.detectChanges(); + + expect(row()?.getAttribute('aria-current')).toBe('true'); + }); + }); + + describe('reading System Host', () => { + afterEach(() => systemHostCanRead.set(true)); + + it('should offer the entry to a user who may read System Host', () => { + expect(spectator.query(byTestId('system-host'))).toBeTruthy(); + }); + + it('should offer no entry at all to a user who may not', () => { + // Hidden rather than disabled: a control with nothing to decide should not be sitting + // there, which is the call this feature already made about the toggle. The URL is + // gated in the store, because hiding a button stops nobody who has a link. + systemHostCanRead.set(false); + spectator.detectChanges(); + + expect(spectator.query(byTestId('system-host'))).toBeNull(); + }); + }); + + describe('drag and drop onto the sidebar entries', () => { + const dragWith = (row: Element | null, files: File[]) => { + // This environment neither populates `files` from `items.add` nor carries a + // `dataTransfer` through the DragEvent constructor, and the component forks on + // `files.length` -- so it is attached to the event itself. + const fileList = { + ...files, + length: files.length, + item: (i: number) => files[i] ?? null + } as unknown as FileList; + const fire = (type: string) => { + const event = new DragEvent(type, { bubbles: true, cancelable: true }); + Object.defineProperty(event, 'dataTransfer', { + value: { files: fileList }, + configurable: true + }); + + return row?.dispatchEvent(event); + }; + fire('dragenter'); + // A row that accepts a drop cancels dragover; anything else declines it. + const offered = fire('dragover') === false; + fire('drop'); + spectator.detectChanges(); + + return offered; + }; + const png = () => new File(['x'], 'a.png', { type: 'image/png' }); + + beforeEach(() => systemHostCanAddChildren.set(true)); + + describe('the System Host entry', () => { + it('should upload files dropped on it, targeting System Host', () => { + const uploads: DotContentDriveUploadFiles[] = []; + spectator + .output('uploadFiles') + .subscribe((e) => uploads.push(e)); + + dragWith(spectator.query(byTestId('system-host')), [png()]); + + expect(uploads.length).toBe(1); + expect(uploads[0].targetFolder?.id).toBe(SYSTEM_HOST.identifier); + }); + + it('should move content dropped on it, targeting System Host', () => { + const moves: DotContentDriveMoveItems[] = []; + spectator + .output('moveItems') + .subscribe((e) => moves.push(e)); + + dragWith(spectator.query(byTestId('system-host')), []); + + expect(moves.length).toBe(1); + expect(moves[0].targetFolder?.id).toBe(SYSTEM_HOST.identifier); + }); + + it('should offer itself as a drop target while the user may add to System Host', () => { + expect(dragWith(spectator.query(byTestId('system-host')), [png()])).toBe(true); + }); + + it('should not offer itself when the user may not add to System Host', () => { + // The spec refuses the target outright rather than accepting and then failing: + // a drop that is going to be rejected should never look available. + systemHostCanAddChildren.set(false); + spectator.detectChanges(); + + const moves: DotContentDriveMoveItems[] = []; + spectator + .output('moveItems') + .subscribe((e) => moves.push(e)); + + expect(dragWith(spectator.query(byTestId('system-host')), [])).toBe(false); + expect(moves.length).toBe(0); + }); + }); + + describe('the All Site Content entry', () => { + it('should never be a drop target, for files or for content', () => { + // Files dropped on the LISTING in this scope are accepted; the entry itself is + // not a destination, because the site row beneath it already means the site root. + const uploads: DotContentDriveUploadFiles[] = []; + const moves: DotContentDriveMoveItems[] = []; + spectator + .output('uploadFiles') + .subscribe((e) => uploads.push(e)); + spectator + .output('moveItems') + .subscribe((e) => moves.push(e)); + + expect(dragWith(spectator.query(byTestId('all-site-content')), [png()])).toBe( + false + ); + expect(dragWith(spectator.query(byTestId('all-site-content')), [])).toBe(false); + expect(uploads.length).toBe(0); + expect(moves.length).toBe(0); + }); + + it('should look refused while a drag is over it, rather than inert', () => { + // A gesture that simply does nothing reads as a broken UI. + const row = spectator.query(byTestId('all-site-content')); + const dt = new DataTransfer(); + row?.dispatchEvent( + new DragEvent('dragenter', { + bubbles: true, + cancelable: true, + dataTransfer: dt + }) + ); + spectator.detectChanges(); + + expect(row?.getAttribute('aria-disabled')).toBe('true'); + }); + }); + }); + + describe('System Host', () => { + const row = () => spectator.query(byTestId('system-host')); + + it('should offer a row below the hierarchy', () => { + expect(row()).toBeTruthy(); + }); + + it('should sit after the hierarchy in document order', () => { + // Below the tree, not above it: System Host belongs to no site, so it reads as the + // other place you can be rather than as part of this site's structure. + const hierarchy = spectator.query(byTestId('hierarchy-scroll')); + + expect( + hierarchy?.compareDocumentPosition(row() as Node) & Node.DOCUMENT_POSITION_FOLLOWING + ).toBeTruthy(); + }); + + it('should ask the store for System Host when the row is chosen', () => { + spectator.click(byTestId('system-host')); + + expect(contentDriveStore.selectSystemHost).toHaveBeenCalled(); + }); + + it('should be reachable by keyboard, not only by pointer', () => { + expect(row()?.tagName.toLowerCase()).toBe('button'); + }); + + it('should read as current only while System Host is what is being shown', () => { + expect(row()?.getAttribute('aria-current')).toBeNull(); + + systemHostSelected.set(true); + spectator.detectChanges(); + + expect(row()?.getAttribute('aria-current')).toBe('true'); + }); + }); + describe('HTML Rendering', () => { it('should render dot-tree-folder component', () => { const treeComponent = spectator.query(DotTreeFolderComponent); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.ts index 14bbc6f27552..358cb4a05336 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-sidebar/dot-content-drive-sidebar.component.ts @@ -8,13 +8,18 @@ import { inject, Injector, output, + signal, untracked, viewChild } from '@angular/core'; import type { TreeNodeExpandEvent, TreeNodeSelectEvent } from 'primeng/types/tree'; -import { DotContentDriveActionableFolder, TreeNodeLoadMoreData } from '@dotcms/dotcms-models'; +import { + DotContentDriveActionableFolder, + PERMISSIONS_TYPE, + TreeNodeLoadMoreData +} from '@dotcms/dotcms-models'; import { DotContentDriveMoveItems, DotContentDriveTreeRightClick, @@ -24,7 +29,9 @@ import { DotTreeFolderComponent, LOAD_MORE_NODE_TYPE } from '@dotcms/portlets/content-drive/ui'; +import { DotMessagePipe } from '@dotcms/ui'; +import { SYSTEM_HOST } from '../../shared/constants'; import { DotContentDriveStore } from '../../store/dot-content-drive.store'; import { appendLoadMoreNodes, mergeFolderNodePage } from '../../utils/functions'; /** @@ -37,14 +44,25 @@ import { appendLoadMoreNodes, mergeFolderNodePage } from '../../utils/functions' selector: 'dot-content-drive-sidebar', templateUrl: './dot-content-drive-sidebar.component.html', changeDetection: ChangeDetectionStrategy.OnPush, - imports: [DotTreeFolderComponent], - host: { class: 'block w-full h-full' }, + imports: [DotTreeFolderComponent, DotMessagePipe], + host: { class: 'flex h-full w-full flex-col' }, styles: ` - /* The top inset used to come from the site-name header that sat above the tree. With the - site now named by the tree's own root row, the tree owns that spacing — and the amount is - what centers that first row on the toolbar's search box and tree toggler beside it. */ + /* The top inset used to come from the site-name header that sat above the tree, then from + the tree itself once the site was named by its own root row. It now belongs to whatever + is first in the column, which is the all-site-content row — the amount is what centers + that first row on the toolbar's search box and tree toggler beside it, so it has to + travel with the row rather than stay on the tree. */ :host ::ng-deep .p-tree { - padding: 1.25rem 0.75rem 0.75rem; + /* Almost no left inset, so the tree's chevron sits in the same column as the icons on + the rows above and below it. + + What has to match is the middle of each mark, not the left of its box. The chevron is + a 10.5px glyph centred in a 24.5px button, while those rows carry a 16px icon, so + lining the boxes up leaves the chevron looking 4px to the right of everything else. + Working back from the icon centre through the row's own 0.625rem leaves this much for + the tree, and it belongs to the sidebar layout rather than to the shared tree, which + knows nothing about the rows it happens to sit between. */ + padding: 0 0.75rem 0.75rem 1px; } ` }) @@ -53,6 +71,12 @@ export class DotContentDriveSidebarComponent { readonly #injector = inject(Injector); readonly $loading = this.#store.sidebarLoading; + + /** Whether the sidebar's first entry, all site content, is the selected one. */ + readonly $allSiteContentSelected = this.#store.$allSiteContentSelected; + + /** Whether the sidebar's last entry, System Host, is the selected one. */ + readonly $systemHostSelected = this.#store.$systemHostSelected; readonly $folders = this.#store.folders; readonly $selectedNode = this.#store.selectedNode; readonly $currentSite = this.#store.currentSite; @@ -60,6 +84,95 @@ export class DotContentDriveSidebarComponent { readonly uploadFiles = output(); readonly moveItems = output(); + /** Whether the user may add content to System Host; unknown reads as allowed. */ + readonly $systemHostCanAddChildren = this.#store.systemHostCanAddChildren; + + /** + * Whether to offer the System Host entry at all. + * + * A user who cannot read it gets no entry rather than a disabled one. The scope is still + * gated in the store for anyone arriving by URL — this only stops the drive advertising a + * door that opens onto nothing. + */ + readonly $systemHostVisible = this.#store.systemHostCanRead; + + /** + * Whether a drag is currently over the all-site-content entry. + * + * Held so the row can look refused rather than inert. A gesture that simply does nothing + * reads as a broken UI, and this row is the one place in the sidebar where a drop is + * declined by what the row *means* rather than by a permission. + */ + protected readonly $allSiteContentDragOver = signal(false); + + /** + * The drop target that stands for System Host. + * + * An empty `path` is what marks it as the host itself rather than a folder on it — the same + * distinction the upload contract draws, where a folder id with no path is a site. The + * permission travels with the target so the shell's existing gate answers about System Host + * instead of about whichever site the switcher happens to show. + */ + private systemHostTarget(): DotFolderTreeNodeContentData { + return { + type: 'folder', + id: SYSTEM_HOST.identifier, + path: '', + hostname: SYSTEM_HOST.hostname, + permissions: [PERMISSIONS_TYPE.CAN_ADD_CHILDREN] + } as DotFolderTreeNodeContentData; + } + + /** + * Offers the System Host entry as a drop target, but only while the user may add to it. + * + * Cancelling the event is what makes a drop possible at all, so declining to cancel is how + * the row declines the drop — the browser then shows the "no drop" cursor on its own, which + * is the refusal the spec asks for without inventing a second way to say it. + */ + protected onSystemHostDragOver(event: DragEvent): void { + if (this.$systemHostCanAddChildren() === false) { + return; + } + + event.preventDefault(); + event.stopPropagation(); + } + + /** Files land as an upload, anything else as a move — the same fork the tree makes. */ + protected onSystemHostDrop(event: DragEvent): void { + if (this.$systemHostCanAddChildren() === false) { + return; + } + + event.preventDefault(); + event.stopPropagation(); + + const targetFolder = this.systemHostTarget(); + const files = event.dataTransfer?.files ?? undefined; + + if (files?.length) { + this.uploadFiles.emit({ files, targetFolder }); + + return; + } + + this.moveItems.emit({ targetFolder }); + } + + /** + * All site content is never a destination: it spans every folder, and the site row directly + * beneath it already means the site root. The event is deliberately left uncancelled so the + * drop cannot happen; all this does is let the row say so while the drag is over it. + */ + protected onAllSiteContentDragOver(): void { + this.$allSiteContentDragOver.set(true); + } + + protected onAllSiteContentDragLeave(): void { + this.$allSiteContentDragOver.set(false); + } + readonly treeFolder = viewChild('treeFolder'); readonly getSiteFoldersEffect = effect(() => { const currentSite = this.$currentSite(); @@ -151,6 +264,21 @@ export class DotContentDriveSidebarComponent { { injector: this.#injector } ); } + /** + * Chooses the whole site. The store clears the tree's selection as it does so, because exactly + * one entry in the sidebar is ever selected and the tree cannot represent this one. + */ + protected onSelectAllSiteContent(): void { + this.#store.selectAllSiteContent(); + } + + /** + * Chooses System Host, which belongs to no site and so clears the tree's selection too. + */ + protected onSelectSystemHost(): void { + this.#store.selectSystemHost(); + } + /** * Handles node selection events * diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.spec.ts index c4cd5134b188..30511701083e 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.spec.ts @@ -28,7 +28,8 @@ describe('DotContentDriveSearchInputComponent', () => { mockProvider(DotContentDriveStore, { getFilterValue: vi.fn().mockReturnValue(undefined), setGlobalSearch: vi.fn(), - selectRootNode: vi.fn() + selectRootNode: vi.fn(), + selectAllSiteContent: vi.fn() }), { provide: DotMessageService, @@ -75,7 +76,10 @@ describe('DotContentDriveSearchInputComponent', () => { spectator.triggerEventHandler(searchInput(), 'search', 'blog'); expect(store.setGlobalSearch).toHaveBeenCalledWith('blog'); - expect(store.selectRootNode).toHaveBeenCalled(); + // All site content, not the site row: the results span the whole site at any depth, and + // the site row now means the root alone. Selecting it would have the sidebar naming a + // narrower place than the list is showing. + expect(store.selectAllSiteContent).toHaveBeenCalled(); }); it('should clear the search in the store when an empty term is emitted', () => { @@ -85,7 +89,7 @@ describe('DotContentDriveSearchInputComponent', () => { spectator.triggerEventHandler(searchInput(), 'search', ''); expect(store.setGlobalSearch).toHaveBeenCalledWith(''); - expect(store.selectRootNode).toHaveBeenCalled(); + expect(store.selectAllSiteContent).toHaveBeenCalled(); }); // The claim lives here rather than in the shell because this component is the one holding the diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.ts index 2aff99ad324c..b77fdc1f0c02 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-search-input/dot-content-drive-search-input.component.ts @@ -109,11 +109,13 @@ export class DotContentDriveSearchInputComponent implements OnDestroy { * A new search resets the folder scope: results are drive-wide, so leaving the tree pinned to * the previously selected folder would contradict what the list shows. * - * `selectRootNode()` rather than pinning a synthetic node: the tree's root is the real site - * row now (see `createSiteNode`), so there is no "All folders" node left to select. + * All site content is exactly that scope, and selecting it is now the honest way to say so. + * This used to select the tree's site row, with a comment regretting that there was no "All + * folders" node left to choose; there is one again, and the site row has since come to mean + * the root alone, which is narrower than what a search returns. */ protected onSearch(term: string): void { this.#store.setGlobalSearch(term); - this.#store.selectRootNode(); + this.#store.selectAllSiteContent(); } } diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-workflow-filter/dot-content-drive-workflow-filter.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-workflow-filter/dot-content-drive-workflow-filter.component.ts index cce2f6156468..925394a43a18 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-workflow-filter/dot-content-drive-workflow-filter.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/components/dot-content-drive-workflow-filter/dot-content-drive-workflow-filter.component.ts @@ -34,11 +34,8 @@ import { } from '@dotcms/ui'; import { PANEL_SCROLL_HEIGHT } from '../../../../shared/constants'; -import { - parseWorkflowToken, - workflowEntryToToken, - WorkflowFilterEntry -} from '../../../../utils/functions'; +import { WorkflowFilterEntry } from '../../../../shared/models'; +import { parseWorkflowToken, workflowEntryToToken } from '../../../../utils/functions'; /** * One selected scheme, optionally pinned to a single step. `step` omitted means diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.html b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.html index 4a03031d4a8c..8100be3dc1f8 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.html +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.html @@ -78,9 +78,6 @@ than named by a registry, which is what lets the two portlet-local ones — Workflow and the field filters — sit in the same row as the shared ones. --> - - - @if ($hasRunInFlight()) { -
- - - - - @if ($actionExecutionPercent() !== undefined) { - - {{ $actionExecutionPercent() }}% - - } -
- } diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.spec.ts index 7768cd250a1e..05fd34ad9a6a 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.spec.ts @@ -67,6 +67,11 @@ describe('DotContentDriveToolbarComponent', () => { // Real signals so the component's computeds re-run when they change const isTreeExpandedSignal = signal(false); + const allSiteContentSelectedSignal = signal(false); + const systemHostSelectedSignal = signal(false); + // The toolbar reports a run by raising a status message rather than drawing it, so the spec owns + // the service it pushes through. + const messageServiceSpy = { add: vi.fn(), clear: vi.fn() }; const filtersSignal = signal({}); const selectedItemsSignal = signal([]); const selectedNodeSignal = signal< @@ -139,9 +144,17 @@ describe('DotContentDriveToolbarComponent', () => { toolbarRun: actionExecutionSignal, toolbarRunCount: activeRunCountSignal, siteCanAddChildren: siteCanAddChildrenSignal, + $allSiteContentSelected: allSiteContentSelectedSignal, + $systemHostSelected: systemHostSelectedSignal, // Mirrors the store's own computed so the toolbar tests still drive the gate - // through the two signals it derives from, not through a hardcoded answer. + // through the signals it derives from, not through a hardcoded answer. $canAddChildren: computed(() => { + // All site content lands on the site root, so the site answers for it — and + // ahead of any node left over from before the tree selection was cleared. + if (allSiteContentSelectedSignal()) { + return siteCanAddChildrenSignal() !== false; + } + const permissions = selectedNodeSignal()?.data?.permissions; if (!permissions?.length) { @@ -194,7 +207,7 @@ describe('DotContentDriveToolbarComponent', () => { }, // Needed once a selection exists: that mounts the workflow-actions child, which injects // both of these. - mockProvider(MessageService, { add: vi.fn() }), + mockProvider(MessageService, messageServiceSpy), mockProvider(DotContentDriveNavigationService, { editContent: vi.fn(), editPage: vi.fn() @@ -210,6 +223,8 @@ describe('DotContentDriveToolbarComponent', () => { }); beforeEach(() => { + messageServiceSpy.add.mockClear(); + messageServiceSpy.clear.mockClear(); spectator = createComponent(); store = spectator.inject(DotContentDriveStore, true); spectator.detectChanges(); @@ -292,13 +307,23 @@ describe('DotContentDriveToolbarComponent', () => { }); describe('chip row', () => { + // The full row exists in all site content, which is the only scope where the Show System + // Host chip has anything to decide. Asserted there rather than lowered to five chips, + // because what these guard is that every chip is present and correctly ordered when it + // applies, not how many happen to apply in the default state. + beforeEach(async () => { + allSiteContentSelectedSignal.set(true); + await settleToolbarAnimation(spectator); + }); + + afterEach(() => allSiteContentSelectedSignal.set(false)); + it('should render all six chips', () => { const chips = Array.from(spectator.element.querySelectorAll('[data-filter-chip]')).map( (element) => element.getAttribute('data-filter-chip') ); expect(chips).toEqual([ - 'sharedAssets', 'contentType', 'workflow', 'status', @@ -324,7 +349,9 @@ describe('DotContentDriveToolbarComponent', () => { spectator.element.querySelectorAll('[data-filter-chip]') ); - expect(chips.length).toBe(6); + // Five since the System Host toggle moved out of this row and into the scope bar, + // where it sits beside the sentence describing what it does. + expect(chips.length).toBe(5); chips.forEach((chip) => { const focusable = chip.matches('[tabindex]') ? chip @@ -683,7 +710,12 @@ describe('DotContentDriveToolbarComponent', () => { // when the run settles. Refusing to open it is the honest version of that state. selectedItemsSignal.set([MOCK_ITEMS[0]]); activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 3 }); + actionExecutionSignal.set({ + actionName: 'Upload', + total: 3, + labelKey: 'content-drive.upload.indicator', + targetLabel: 'demo.dotcms.com' + }); await settleToolbarAnimation(spectator); const button = spectator @@ -727,7 +759,12 @@ describe('DotContentDriveToolbarComponent', () => { it('should explain why it is disabled while an action is running', async () => { selectedItemsSignal.set([MOCK_ITEMS[0]]); activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 3 }); + actionExecutionSignal.set({ + actionName: 'Upload', + total: 3, + labelKey: 'content-drive.upload.indicator', + targetLabel: 'demo.dotcms.com' + }); await settleToolbarAnimation(spectator); expect(spectator.component.$actionCenterTooltip()).toBe( @@ -745,7 +782,12 @@ describe('DotContentDriveToolbarComponent', () => { it('should not open the dialog while an action is running', async () => { selectedItemsSignal.set([MOCK_ITEMS[0]]); activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 3 }); + actionExecutionSignal.set({ + actionName: 'Upload', + total: 3, + labelKey: 'content-drive.upload.indicator', + targetLabel: 'demo.dotcms.com' + }); await settleToolbarAnimation(spectator); // Guards the handler too: a disabled attribute alone would leave the store reachable. @@ -757,7 +799,12 @@ describe('DotContentDriveToolbarComponent', () => { it('should become available again once the run settles', async () => { selectedItemsSignal.set([MOCK_ITEMS[0]]); activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 3 }); + actionExecutionSignal.set({ + actionName: 'Upload', + total: 3, + labelKey: 'content-drive.upload.indicator', + targetLabel: 'demo.dotcms.com' + }); await settleToolbarAnimation(spectator); // A settled run leaves neither a name nor a count. Clearing only the name described a @@ -774,203 +821,6 @@ describe('DotContentDriveToolbarComponent', () => { }); }); - describe('running-action indicator', () => { - it('should stay hidden when nothing is running', () => { - spectator.detectChanges(); - - expect(spectator.query(byTestId('action-execution-indicator'))).toBeNull(); - }); - - it('should report the action and the number of items once a run starts', () => { - // The toolbar is the only place still reporting the run after the Action Center dialog is - // closed, which is the whole reason the indicator lives out here. - activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 3 }); - spectator.detectChanges(); - - const indicator = spectator.query(byTestId('action-execution-indicator')); - - expect(indicator).toBeTruthy(); - expect(spectator.component.$actionExecutionLabel()).toBe( - 'content-drive.action-center.applying' - ); - }); - - it('should not render markup carried by the action name', () => { - // Workflow action names come from the backend verbatim (`$selectedAction()?.name`), and - // the label is placed in the DOM as HTML so the message's own `` renders. A name - // carrying markup must not become live DOM — an event-handler attribute least of all. - // - // The shared mock returns the bare key, which would make this pass without rendering - // anything; the real message has to be in play for the assertion to mean something. - const messageService = spectator.inject(DotMessageService); - - vi.spyOn(messageService, 'get').mockImplementation((key: string, ...args: string[]) => - key === 'content-drive.action-center.applying' - ? `Applying ${args[0]} to ${args[1]} item(s)…` - : key - ); - - activeRunCountSignal.set(1); - actionExecutionSignal.set({ - actionName: '', - total: 3 - }); - spectator.detectChanges(); - - const indicator = spectator.query(byTestId('action-execution-indicator')); - - // Asserted structurally rather than by searching the markup for "onerror": once the name - // is escaped it renders as visible text that legitimately still contains that word. - expect(indicator?.querySelector('img')).toBeNull(); - expect(indicator?.querySelector('[onerror]')).toBeNull(); - // …and the name is still shown to the user, just as text. - expect(indicator?.textContent).toContain(' { - activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 3 }); - spectator.detectChanges(); - - actionExecutionSignal.set(undefined); - // Settling clears the count too; the indicator now keys visibility off it so that - // several runs (where no single one is named) still show something. - activeRunCountSignal.set(0); - spectator.detectChanges(); - - expect(spectator.query(byTestId('action-execution-indicator'))).toBeNull(); - }); - - // ---- FR-017: several runs at once ---- - - it('should collapse to a count when several runs are in flight', () => { - // `actionExecution` is undefined with more than one run: naming one of several - // arbitrarily is worse than naming none. - activeRunCountSignal.set(2); - actionExecutionSignal.set(undefined); - spectator.detectChanges(); - - expect(spectator.query(byTestId('action-execution-indicator'))).toBeTruthy(); - expect(spectator.component.$actionExecutionLabel()).toBe( - 'content-drive.action-center.applying-many' - ); - }); - - it('should still hide the indicator when nothing at all is running', () => { - activeRunCountSignal.set(0); - actionExecutionSignal.set(undefined); - spectator.detectChanges(); - - expect(spectator.query(byTestId('action-execution-indicator'))).toBeNull(); - }); - - // ---- FR-010: name the item when there is one ---- - - it('should name the item, not a count, when the run is over a single thing', () => { - // "Applying Publish to 1 item(s)" is worse than useless on a context-menu action: the - // author knows it is one item, what they cannot see is *which*. - activeRunCountSignal.set(1); - actionExecutionSignal.set({ - actionName: 'Publish', - total: 1, - targetLabel: 'My Page' - }); - spectator.detectChanges(); - - expect(spectator.component.$actionExecutionLabel()).toBe( - 'content-drive.action-center.applying-item' - ); - }); - - it('should keep the count form when several items are in play', () => { - activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 12 }); - spectator.detectChanges(); - - expect(spectator.component.$actionExecutionLabel()).toBe( - 'content-drive.action-center.applying' - ); - }); - - // ---- FR-011: the item name is author-supplied content ---- - - it('should not render markup carried by the item name', () => { - // `actionName` comes from the backend; a title is typed by an author, so this is the - // likelier of the two to carry markup and the one that must not become live DOM. - const messageService = spectator.inject(DotMessageService); - - vi.spyOn(messageService, 'get').mockImplementation((key: string, ...args: string[]) => - key === 'content-drive.action-center.applying-item' - ? `Applying ${args[0]} to ${args[1]}` - : key - ); - - activeRunCountSignal.set(1); - actionExecutionSignal.set({ - actionName: 'Publish', - total: 1, - targetLabel: '' - }); - spectator.detectChanges(); - - const indicator = spectator.query(byTestId('action-execution-indicator')); - - expect(indicator?.querySelector('img')).toBeNull(); - expect(indicator?.querySelector('[onerror]')).toBeNull(); - expect(indicator?.textContent).toContain(' { - // Progress readback is the backend's largest piece of hidden work. Until it lands every - // run is indeterminate, and the indicator must not imply a position it does not have. - activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 50 }); - spectator.detectChanges(); - - expect(spectator.query(byTestId('action-execution-indicator'))).toBeTruthy(); - expect(spectator.query(byTestId('action-execution-progress'))).toBeNull(); - }); - - it('should show the position once the run reports one', () => { - activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 50, processed: 20 }); - spectator.detectChanges(); - - const progress = spectator.query(byTestId('action-execution-progress')); - - expect(progress).toBeTruthy(); - expect(spectator.component.$actionExecutionPercent()).toBe(40); - }); - - it('should treat a reported zero as a position, not as absence', () => { - // `processed: 0` is a run that has genuinely done nothing yet, which is different from a - // run that cannot say. A truthiness check would collapse the two. - activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 50, processed: 0 }); - spectator.detectChanges(); - - expect(spectator.query(byTestId('action-execution-progress'))).toBeTruthy(); - expect(spectator.component.$actionExecutionPercent()).toBe(0); - }); - - // ---- FR-035: announce state changes, not every tick ---- - - it('should keep progress updates out of the live region', () => { - // The indicator is a polite live region. A fifty-file upload that re-announced on every - // tick would speak fifty times; the value stays queryable instead of being pushed. - activeRunCountSignal.set(1); - actionExecutionSignal.set({ actionName: 'Publish', total: 50, processed: 20 }); - spectator.detectChanges(); - - const progress = spectator.query(byTestId('action-execution-progress')); - - expect(progress?.getAttribute('aria-live')).toBe('off'); - }); - }); - describe('field-filter chips', () => { it('should render a chip only for active variables resolved against loaded fields', () => { store.userSearchableFields.set([ @@ -1011,6 +861,42 @@ describe('DotContentDriveToolbarComponent', () => { expect(spectator.component.$canAddChildren()).toBe(true); }); + // All site content spans every folder in the site, which for a while was read as "there is + // nowhere to put anything" and closed the affordances. It now behaves as the site root + // does: content added here lands on the site, and the indicator names the site so the + // author can see where it went. + describe('in all site content', () => { + afterEach(() => allSiteContentSelectedSignal.set(false)); + + it('should allow creation where the site accepts children', async () => { + siteCanAddChildrenSignal.set(true); + allSiteContentSelectedSignal.set(true); + await settleToolbarAnimation(spectator); + + expect(spectator.component.$canAddChildren()).toBe(true); + }); + + it('should refuse creation where the site refuses children', async () => { + // A permission answer again, and about the site the content would land on. The + // scope stopped being a reason of its own. + siteCanAddChildrenSignal.set(false); + allSiteContentSelectedSignal.set(true); + await settleToolbarAnimation(spectator); + + expect(spectator.component.$canAddChildren()).toBe(false); + }); + + it('should blame permissions, since the scope is no longer a reason', async () => { + siteCanAddChildrenSignal.set(false); + allSiteContentSelectedSignal.set(true); + await settleToolbarAnimation(spectator); + + expect(spectator.component.$addChildrenTooltip()).toBe( + 'content-drive.add-new.no-add-children' + ); + }); + }); + // The site root: the parent is the host, not a folder, so the tree's site node carries no // permissions and the answer comes from the store's own lookup on the site instead. describe('at the site root', () => { @@ -1076,16 +962,42 @@ describe('DotContentDriveToolbarComponent', () => { }); }); - describe('progress the run measures itself', () => { - it('should fall back to the item ratio when the run reports no percent', () => { - actionExecutionSignal.set({ - actionName: 'Publish', - total: 4, - processed: 1 - } as DotContentDriveActionExecution); + describe('the New menu on System Host', () => { + const folderEntry = () => + spectator.component + .$items() + .find((item) => item.label === 'content-drive.add-new.context-menu.folder'); + + it('should not offer a new folder there', () => { + // System Host lists no folders: `listsFolders` returns false for that scope and the + // sidebar row opens no tree beneath it. A folder created there could never be shown + // again by this portlet, and the dialog could not even name where it would land -- the + // location is a reserved word, so pasting it after the hostname gave + // `//demo.dotcms.comSYSTEM_HOST/`. + systemHostSelectedSignal.set(true); + spectator.detectChanges(); + + expect(folderEntry()).toBeUndefined(); + }); + + it('should still offer content types there', () => { + // The other half of the rule: System Host holds content, and adding some is the whole + // reason the scope accepts new items at all. Only the folder entry goes. + systemHostSelectedSignal.set(true); + spectator.detectChanges(); + + expect( + spectator.component + .$items() + .some((item) => item.label === 'content-drive.add-new.all-content-types') + ).toBe(true); + }); + + it('should offer a new folder everywhere else', () => { + systemHostSelectedSignal.set(false); spectator.detectChanges(); - expect(spectator.component.$actionExecutionPercent()).toBe(25); + expect(folderEntry()).toBeDefined(); }); }); }); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.ts index 3ffa954d713f..25315f7cbf25 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-content-drive-toolbar/dot-content-drive-toolbar.component.ts @@ -32,7 +32,6 @@ import { DotFilterChipError, DotLanguageFilterChipComponent, DotMessagePipe, - DotSharedAssetsFilterComponent, DotStatusFilterComponent, DotUploadButtonComponent } from '@dotcms/ui'; @@ -50,25 +49,6 @@ import { DotContentDriveStore } from '../../store/dot-content-drive.store'; */ const ANIMATION_DELAY = 135; -/** Characters that would let an interpolated value become markup. */ -const HTML_ESCAPES: Record = { - '&': '&', - '<': '<', - '>': '>', - '"': '"', - "'": ''' -}; - -/** - * Escapes a value that will be interpolated into a message rendered as HTML. - * - * Needed only because `content-drive.action-center.applying` carries its own ``, which forces the - * label to be bound with `[innerHTML]` rather than interpolated. Everything substituted into such a - * message has to be escaped, or the message stops being the only source of markup in it. - */ -const escapeHtml = (value: string): string => - value.replace(/[&<>"']/g, (char) => HTML_ESCAPES[char]); - /** * Base-type options in the "New" menu (all base types except FORM, which is deprecated). * Each maps to a precise palette list type and a Material Symbols icon (rendered via the @@ -145,7 +125,6 @@ interface ToolbarAnimationState { DotContentDriveWorkflowFilterComponent, DotFieldFilterComponent, DotFieldFilterMenuComponent, - DotSharedAssetsFilterComponent, DotFilterBarComponent, DotStatusFilterComponent, TooltipModule @@ -226,10 +205,27 @@ export class DotContentDriveToolbarComponent { */ protected readonly $canAddChildren = this.#store.$canAddChildren; + /** Gates the Show System Host chip: it has something to decide in one scope only. */ + protected readonly $allSiteContentSelected = this.#store.$allSiteContentSelected; + /** Empty when creation is allowed, so the buttons carry no tooltip in the normal case. */ - protected readonly $addChildrenTooltip = computed(() => - this.$canAddChildren() ? '' : 'content-drive.add-new.no-add-children' - ); + /** + * Why creating and uploading are unavailable, when they are. + * + * Two different refusals reach the same disabled buttons, and they must not say the same + * thing. In all site content nothing is wrong with the user's permissions: the view simply + * spans the whole site and names no place for new content to land. Telling them they lack + * permission there would send them to an administrator for a problem they do not have. + */ + protected readonly $addChildrenTooltip = computed(() => { + if (this.$canAddChildren()) { + return ''; + } + + // One reason left, and it is always a permission one: every scope that reaches here now + // has a place for content to land. + return 'content-drive.add-new.no-add-children'; + }); protected readonly $uploadBaseType = computed(() => { const data = this.#store.selectedNode()?.data; @@ -239,7 +235,19 @@ export class DotContentDriveToolbarComponent { : null; }); - readonly $items = signal([ + /** + * What the "New" menu offers, which depends on where the user is. + * + * A computed rather than a fixed list because System Host takes one entry away. It lists no + * folders -- `listsFolders` answers false for that scope, and its sidebar row opens no tree -- + * so a folder created there could never be shown again by this portlet. The dialog could not + * even name where it would land: the location is a reserved word, so the path preview read + * `//demo.dotcms.comSYSTEM_HOST/`. + * + * Content types stay. System Host holds content, and adding some is the whole reason the scope + * accepts new items at all. + */ + readonly $items = computed(() => [ { label: this.#dotMessageService.get('content-drive.add-new.all-content-types'), icon: 'grid_view', @@ -251,17 +259,25 @@ export class DotContentDriveToolbarComponent { icon: option.icon, command: () => this.#openContentTypeSelector(option.listType) })), - { separator: true }, - { - label: this.#dotMessageService.get('content-drive.add-new.context-menu.folder'), - icon: 'folder', - command: () => { - this.#store.setDialog({ - type: DIALOG_TYPE.FOLDER, - header: this.#dotMessageService.get('content-drive.dialog.folder.header') - }); - } - } + ...(this.#store.$systemHostSelected() + ? [] + : [ + { separator: true }, + { + label: this.#dotMessageService.get( + 'content-drive.add-new.context-menu.folder' + ), + icon: 'folder', + command: () => { + this.#store.setDialog({ + type: DIALOG_TYPE.FOLDER, + header: this.#dotMessageService.get( + 'content-drive.dialog.folder.header' + ) + }); + } + } + ]) ]); /** @@ -291,96 +307,14 @@ export class DotContentDriveToolbarComponent { readonly $defaultLanguageId = computed(() => this.#store.defaultLanguageId() ?? null); /** - * The action currently being applied, surfaced here because the run outlives the Action Center - * dialog. Once the user closes that dialog the toolbar is the only place still reporting the run, - * so without this the work would continue with no indication until the completion toast fired. - */ - readonly $actionExecution = this.#store.toolbarRun; - - /** - * How many runs are in flight. Drives whether the indicator is shown at all: with several runs - * `$actionExecution` is deliberately undefined, so keying visibility off it alone would hide the - * indicator exactly when the most is happening. + * Whether anything is in flight, which is all the toolbar needs to know: it disables the Action + * Center while a run is going and says so in the tooltip. Reporting the run is the shell's job, + * because the shell owns the outlet it is reported through and outlives every dialog. */ readonly $activeRunCount = this.#store.toolbarRunCount; readonly $hasRunInFlight = computed(() => this.$activeRunCount() > 0); - /** - * Resolved indicator label. Built here rather than in the template because `DotMessagePipe` takes - * `string[]` arguments and the item count is a number. - * - * The action name is escaped because this label is bound with `[innerHTML]` — the message itself - * carries a ``, which is the only reason it is not plain interpolation. For a workflow action - * that name is `WorkflowAction.name` straight from the backend, so without this a name containing - * markup becomes real DOM. Angular's sanitizer already drops event-handler attributes, so this is - * not an XSS fix; what it stops is structural injection that survives sanitizing — an `` - * pointing at an arbitrary URL, a link, or markup that simply breaks the toolbar's layout. - */ - readonly $actionExecutionLabel = computed(() => { - const execution = this.$actionExecution(); - - // Several at once: name none of them and report the number instead (FR-017). - if (!execution) { - return this.$activeRunCount() > 1 - ? this.#dotMessageService.get( - 'content-drive.action-center.applying-many', - String(this.$activeRunCount()) - ) - : ''; - } - - // Both halves are escaped, and the item name matters more: `actionName` is a - // `WorkflowAction.name` from the backend, but a target label is content an author typed. - const actionName = escapeHtml(execution.actionName); - - // A run carrying its own copy uses it: the "Applying X to Y" form describes an operation - // being performed on something, which is not what every run is. - if (execution.labelKey) { - return this.#dotMessageService.get( - execution.labelKey, - escapeHtml(execution.targetLabel ?? ''), - String(execution.total) - ); - } - - // "Applying Publish to 1 item(s)" tells an author nothing they did not already know. When - // the run is over one nameable thing, name it. - return execution.targetLabel - ? this.#dotMessageService.get( - 'content-drive.action-center.applying-item', - actionName, - escapeHtml(execution.targetLabel) - ) - : this.#dotMessageService.get( - 'content-drive.action-center.applying', - actionName, - String(execution.total) - ); - }); - - /** - * How far the run has got, as a percentage, or `undefined` when it does not report progress. - * - * `undefined` and `0` are deliberately different answers: a run that has done nothing yet is not - * the same as a run that cannot say. A truthiness check would collapse the two and show an empty - * bar for a run that has no bar to show. - * - * Counts items, and only items. An upload measures itself in bytes, which this cannot express — - * but it has no position to report here anyway: the app runs on Angular's fetch backend, which - * never emits upload progress, and the XHR backend that would is deprecated. An upload shows the - * indeterminate spinner instead, which is the honest rendering of a wait with no denominator. - */ - readonly $actionExecutionPercent = computed(() => { - const execution = this.$actionExecution(); - - if (!execution || execution.processed === undefined || !execution.total) { - return undefined; - } - - return Math.round((execution.processed / execution.total) * 100); - }); - /** * Active field-filter chips, in the order the user added them (the store keeps `userSearchableActive` * in add order). Each variable is resolved to its field metadata, so chips render only once the diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.spec.ts index c09a54c6e452..37dd51a5099d 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.spec.ts @@ -1064,32 +1064,6 @@ describe('DotFolderListViewContextMenuComponent', () => { expect(store.reloadContentDrive).toHaveBeenCalled(); }); - it('should report the delete on the toolbar while it runs', async () => { - // A recursive subtree delete is the slowest thing in the portlet and reported - // nothing at all until its toast. The confirm dialog closes on accept, so the - // run outlives its trigger and belongs on the indicator (FR-007). - folderService.deleteFolder = vi.fn().mockReturnValue(NEVER); - const startExternalRun = vi.spyOn(store, 'startExternalRun'); - - await component.getMenuItems(folderContextMenuWithEdit); - deleteItem()?.command?.({} as unknown as MenuItemCommandEvent); - (alertConfirmService.confirm as Mock).mock.lastCall[0].accept(); - - expect(startExternalRun).toHaveBeenCalledWith( - expect.objectContaining({ - total: 1, - targetLabel: folderWithEdit.name, - // Both keys, because neither alone is reliably the one the row carries: - // the search service backfills `inode` from `identifier` only when the - // API returned none, so a folder that arrives with a distinct inode - // would never be marked if this targeted the identifier alone. - targets: [folderWithEdit.identifier, folderWithEdit.inode].filter( - Boolean - ) - }) - ); - }); - it('should clear the indicator when the delete succeeds', async () => { const endExternalRun = vi.spyOn(store, 'endExternalRun'); @@ -1780,24 +1754,6 @@ describe('DotFolderListViewContextMenuComponent', () => { vi.useRealTimers(); }); - it('should report the run on the toolbar indicator, naming the item', async () => { - vi.useFakeTimers(); - const startExternalRun = vi.spyOn(store, 'startExternalRun'); - - await fireSaveAction(); - - expect(startExternalRun).toHaveBeenCalledWith( - expect.objectContaining({ - actionName: 'Save', - total: 1, - targetLabel: mockContentlet.title, - targets: [mockContentlet.inode] - }) - ); - - vi.useRealTimers(); - }); - it('should clear the indicator once the run settles', async () => { vi.useFakeTimers(); const endExternalRun = vi.spyOn(store, 'endExternalRun'); @@ -1847,26 +1803,6 @@ describe('DotFolderListViewContextMenuComponent', () => { vi.advanceTimersByTime(0); }; - it('should report a lock on the toolbar, naming the item', async () => { - // The same Lock fired from the Workflow Center registers a run and reports; from the - // row menu it registered nothing, so one action read two ways depending on the surface. - vi.useFakeTimers(); - dotContentletService.lockContent.mockReturnValue(NEVER); - const startExternalRun = vi.spyOn(store, 'startExternalRun'); - - await fireLock(false); - - expect(startExternalRun).toHaveBeenCalledWith( - expect.objectContaining({ - total: 1, - targetLabel: mockContentlet.title, - targets: [mockContentlet.inode] - }) - ); - - vi.useRealTimers(); - }); - it('should report an unlock the same way', async () => { vi.useFakeTimers(); dotContentletService.unlockContent.mockReturnValue(NEVER); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.ts index 3de2cfa26c1b..b020aa424aa7 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dot-folder-list-context-menu/dot-folder-list-context-menu.component.ts @@ -476,9 +476,7 @@ export class DotFolderListViewContextMenuComponent { // the listing" and nothing else (FR-007, FR-009). const runId = this.#store.startExternalRun({ operation: actionId, - actionName, total: 1, - targetLabel: itemTitle, targets: [contentletInode] }); this.#workflowActionsFireService @@ -518,13 +516,7 @@ export class DotFolderListViewContextMenuComponent { // workflow action, this locks through the contentlet service. Only the *reporting* is shared. const runId = this.#store.startExternalRun({ operation: canLockData.locked ? 'UNLOCK' : 'LOCK', - actionName: this.#dotMessageService.get( - canLockData.locked - ? 'content-drive.context-menu.unlock' - : 'content-drive.context-menu.lock' - ), total: 1, - targetLabel: contentlet.title, targets: [contentlet.inode] }); @@ -761,9 +753,7 @@ export class DotFolderListViewContextMenuComponent { // shows nothing — the delete still working, but looking like nothing is happening. const runId = this.#store.startExternalRun({ operation: 'DELETE_FOLDER', - actionName: this.#dotMessageService.get('content-drive.context-menu.delete-folder'), total: 1, - targetLabel: folder.name, targets: [folder.identifier, folder.inode].filter(Boolean) }); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.html b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.html index 957e4812ddee..8df8aa4b7541 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.html +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.html @@ -20,7 +20,7 @@
+ +
+ +
+ +
+
+ + class="col-start-2 row-start-4 overflow-auto"> @@ -64,6 +95,11 @@ + + + @if ($contextMenuData()?.showAddToBundle) {
+ class="col-start-3 row-span-3 row-start-2 flex w-0 flex-col items-center justify-center font-semibold"> = signal(true); // The site-level answer the drop guard falls back to at the root. Module scope for the same reason // as the one above: two store mocks in this file read it from describes with no shared `beforeEach`. const siteCanAddChildrenSignal: WritableSignal = signal(undefined); +const systemHostCanReadSignal = signal(true); +const allSiteContentSelectedSignal = signal(false); describe('DotContentDriveShellComponent', () => { let spectator: Spectator; @@ -127,6 +136,11 @@ describe('DotContentDriveShellComponent', () => { // Reactive so the shell's $extraColumns computed recomputes when the fields change. let showInListFieldsSignal: WritableSignal; + // The run the toolbar surface speaks for, driven from here so the shell's status-toast effect + // can be exercised. Only unmarked runs reach it; the store's own computed does that filtering. + const toolbarRunSignal = signal(undefined); + const toolbarRunCountSignal = signal(0); + const createComponent = createComponentFactory({ component: DotContentDriveShellComponent, providers: [ @@ -214,6 +228,8 @@ describe('DotContentDriveShellComponent', () => { beforeEach(() => { canAddChildrenSignal.set(true); siteCanAddChildrenSignal.set(undefined); + systemHostCanReadSignal.set(true); + allSiteContentSelectedSignal.set(false); filtersSignal = signal({}); statusSignal = signal(DotContentDriveStatus.LOADING); dialogSignal = signal(undefined); @@ -226,6 +242,9 @@ describe('DotContentDriveShellComponent', () => { showInListFieldsSignal = signal([]); editPanelRequestSignal.set(null); + const currentSiteMock = vi.fn().mockReturnValue(MOCK_SITES[0]); + const systemHostSelectedMock = vi.fn().mockReturnValue(false); + spectator = createComponent({ providers: [ mockProvider(DotContentDriveStore, { @@ -237,7 +256,10 @@ describe('DotContentDriveShellComponent', () => { // their creation affordances on it. $canAddChildren: canAddChildrenSignal, siteCanAddChildren: siteCanAddChildrenSignal, - currentSite: vi.fn().mockReturnValue(MOCK_SITES[0]), + // The sidebar this shell renders reads it to decide whether to offer the + // System Host entry at all. + systemHostCanRead: systemHostCanReadSignal, + currentSite: currentSiteMock, // Tree collapsed at start to render the toggle button on toolbar isTreeExpanded: vi.fn().mockReturnValue(false), removeFilter: vi.fn(), @@ -263,8 +285,8 @@ describe('DotContentDriveShellComponent', () => { trackUploadJob: vi.fn(), updateExternalRun: vi.fn(), activeRunCount: signal(0), - toolbarRun: signal(undefined), - toolbarRunCount: signal(0), + toolbarRun: toolbarRunSignal, + toolbarRunCount: toolbarRunCountSignal, busyRows: signal([]), endExternalRun: vi.fn(), setPagination: vi.fn(), @@ -297,6 +319,17 @@ describe('DotContentDriveShellComponent', () => { folders: vi.fn(), selectedNode: vi.fn(), setSelectedNode: vi.fn(), + // The shell renders the sidebar, which asks the store which entry is selected. + $allSiteContentSelected: allSiteContentSelectedSignal, + $systemHostSelected: systemHostSelectedMock, + // Mirrors the store's own computed rather than hardcoding an answer, so these + // tests keep driving the destination through the signals they already control: + // System Host when that is selected, the current site otherwise. + $newContentHostId: vi.fn(() => + systemHostSelectedMock() ? 'SYSTEM_HOST' : currentSiteMock()?.identifier + ), + selectAllSiteContent: vi.fn(), + selectSystemHost: vi.fn(), sidebarLoading: vi.fn(), closeDialog: vi.fn(), patchContextMenu: vi.fn(), @@ -383,6 +416,12 @@ describe('DotContentDriveShellComponent', () => { router = spectator.inject(Router); location = spectator.inject(Location); messageService = spectator.inject(MessageService); + + // Module-level, so whatever the last test left here is what the next one starts with. The + // shell raises the status toast off these, which made an unrelated test see a message it + // never asked for. + toolbarRunSignal.set(undefined); + toolbarRunCountSignal.set(0); uploadService = spectator.inject(DotUploadFileService); routerService = spectator.inject(DotRouterService); dotMessageService = spectator.inject(DotMessageService); @@ -2020,6 +2059,125 @@ describe('DotContentDriveShellComponent', () => { }); }); + describe('the scope bar slot', () => { + it('should take the bar out of reach while it is closed', () => { + // Closed is zero-height and transparent, which hides it from the eye and from nothing + // else: the toggle inside stayed tabbable and stayed in the accessibility tree in + // every scope that does not show the bar. + allSiteContentSelectedSignal.set(false); + spectator.detectChanges(); + + const slot = spectator.query(byTestId('scope-bar-slot')); + + expect(slot?.hasAttribute('inert')).toBe(true); + expect(slot?.getAttribute('aria-hidden')).toBe('true'); + }); + + it('should put it back in reach when all site content is selected', () => { + allSiteContentSelectedSignal.set(true); + spectator.detectChanges(); + + const slot = spectator.query(byTestId('scope-bar-slot')); + + expect(slot?.hasAttribute('inert')).toBe(false); + expect(slot?.hasAttribute('aria-hidden')).toBe(false); + }); + }); + + describe('reporting a run in flight', () => { + // Moved here with the effect itself. The toolbar raised this while it still drew the + // indicator in its filter row; now that the status is a toast rendered by the shell, the + // shell owns raising it -- it holds the outlet and outlives every dialog the run may have + // started from. + const statusMessages = () => + (messageService.add as unknown as { mock: { calls: unknown[][] } }).mock.calls + .map(([message]) => message as { key?: string; summary?: string }) + .filter((message) => message.key === STATUS_TOAST_KEY); + + it('should raise nothing while nothing is running', () => { + spectator.detectChanges(); + spectator.flushEffects(); + + expect(statusMessages()).toHaveLength(0); + }); + + it('should report a run that brought its own wording', () => { + toolbarRunCountSignal.set(1); + toolbarRunSignal.set({ + actionName: 'Upload', + total: 3, + labelKey: 'content-drive.upload.indicator' + } as DotContentDriveActionExecution); + spectator.detectChanges(); + spectator.flushEffects(); + + expect(statusMessages()).toHaveLength(1); + }); + + it('should stay silent for a run that brought none', () => { + // Only unmarked runs arrive here, and every one of those is an upload, which names + // itself. Anything else is a run nobody wrote words for, and inventing "Applying X to + // Y" for it is what made an upload read "Applying Upload to demo.dotcms.com". + toolbarRunCountSignal.set(1); + toolbarRunSignal.set({ + actionName: 'Publish', + total: 3 + } as DotContentDriveActionExecution); + spectator.detectChanges(); + spectator.flushEffects(); + + expect(statusMessages()).toHaveLength(0); + }); + + it('should clear the status once the run settles', () => { + toolbarRunCountSignal.set(1); + toolbarRunSignal.set({ + actionName: 'Upload', + total: 3, + labelKey: 'content-drive.upload.indicator' + } as DotContentDriveActionExecution); + spectator.detectChanges(); + spectator.flushEffects(); + + toolbarRunCountSignal.set(0); + toolbarRunSignal.set(undefined); + spectator.detectChanges(); + spectator.flushEffects(); + + // Sticky on purpose, so nothing times it out mid-upload. That makes ending it the + // store's job, which is the half worth pinning. + expect(messageService.clear).toHaveBeenCalledWith(STATUS_TOAST_KEY); + }); + + it('should raise it as something the reader cannot dismiss', () => { + toolbarRunCountSignal.set(1); + toolbarRunSignal.set({ + actionName: 'Upload', + total: 3, + labelKey: 'content-drive.upload.indicator' + } as DotContentDriveActionExecution); + spectator.detectChanges(); + spectator.flushEffects(); + + // PrimeNG reads `closable` off the message, not the outlet, so this is the only place + // that can turn the close button off for real. + expect(statusMessages()[0]).toEqual( + expect.objectContaining({ sticky: true, closable: false }) + ); + }); + + it('should collapse to a count when several runs are in flight', () => { + // With several at once the store leaves the run undefined on purpose: name none of + // them and report the number instead. + toolbarRunCountSignal.set(3); + toolbarRunSignal.set(undefined); + spectator.detectChanges(); + spectator.flushEffects(); + + expect(statusMessages()).toHaveLength(1); + }); + }); + describe('upload — a batch of files', () => { beforeEach(() => { spectator.detectChanges(); @@ -2082,6 +2240,86 @@ describe('DotContentDriveShellComponent', () => { ); }); + it('should count a single file in the singular', () => { + // "Uploading 1 files" is the price of dropping the "(s)" hedge, so the caller picks + // the wording -- it is the only place that knows how many were chosen. + selectUploadType({ + targetFolder: TARGET_FOLDER_DATA, + files: createFileList([createFile('a.png')]), + baseType: 'DOTASSET' + }); + + expect(store.startExternalRun).toHaveBeenCalledWith( + expect.objectContaining({ labelKey: 'content-drive.upload.indicator.one' }) + ); + }); + + it('should describe itself as an upload, not as an action applied to a site', () => { + // Without a label of its own the run falls to the workflow sentence, which reads + // "Applying Upload to demo.dotcms.com" — phrased for an action applied TO content, + // not for files going INTO a place. + selectUploadType({ + targetFolder: TARGET_FOLDER_DATA, + files: createFileList([createFile('a.png')]), + baseType: 'DOTASSET' + }); + + expect(store.startExternalRun).toHaveBeenCalledWith( + expect.objectContaining({ + labelKey: expect.stringContaining('content-drive.upload.indicator') + }) + ); + }); + + it('should target System Host when that is where the batch lands, not the switcher site', () => { + // The switcher still names a site while System Host is browsed, and that site is + // context rather than the destination. Uploading here with the site's identifier + // silently lands the files somewhere the author did not choose. + store.currentSite.mockReturnValue(MOCK_SITES[0]); + store.$systemHostSelected.mockReturnValue(true); + + selectUploadType({ + targetFolder: undefined, + files: createFileList([createFile('a.png'), createFile('b.png')]), + baseType: 'DOTASSET' + }); + + expect(uploadService.uploadFilesByBaseType).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ siteId: SYSTEM_HOST.identifier }) + ); + }); + + it('should remember a System Host batch as landing on System Host', () => { + // The listing reloads only when the run's folders include the one on screen, and both + // sides of that comparison were computed from the switcher's site plus the location. + // On System Host that gave `//demo.dotcms.com` for the batch and + // `//demo.dotcms.comsystem_host` for the listing — two references to nothing alike, so + // an upload finished and the grid it landed in never refreshed. + store.currentSite.mockReturnValue(MOCK_SITES[0]); + store.$systemHostSelected.mockReturnValue(true); + store.path.mockReturnValue(SYSTEM_HOST_PATH); + uploadService.uploadFilesByBaseType.mockReturnValue( + of({ + kind: 'accepted', + handle: { jobId: 'job-sh', statusUrl: '/api/v1/jobs/job-sh/status' } + }) + ); + + selectUploadType({ + targetFolder: undefined, + files: createFileList([createFile('a.png')]), + baseType: 'DOTASSET' + }); + + expect(store.trackUploadJob).toHaveBeenCalledWith( + 'job-sh', + [`//${SYSTEM_HOST.identifier}`.toLowerCase()], + expect.any(String), + 'DOTASSET' + ); + }); + it('should submit a single file down the same path, as a batch of one', () => { // Not a special case. One path, one set of gates, one method: a lone file is a batch // whose length is one, so nothing forks on count. @@ -2182,10 +2420,15 @@ describe('DotContentDriveShellComponent', () => { baseType: 'DOTASSET' }); - expect(messageService.add).toHaveBeenCalledWith( + // The advisory toast this used to assert is gone: the status it sat beside says + // "in the background" itself, and both on screen announced one upload twice. + // Matched as a prefix: what this protects is that one file is still announced as + // backgrounded, not which noun the sentence uses. The wording does vary by count -- + // "1 file" against "3 files" -- and pinning the exact key here would fail for the + // grammar while the behaviour under test was perfectly intact. + expect(store.startExternalRun).toHaveBeenCalledWith( expect.objectContaining({ - severity: 'info', - detail: 'content-drive.upload.toast.backgrounded-detail' + labelKey: expect.stringContaining('content-drive.upload.indicator.background') }) ); }); @@ -2218,13 +2461,17 @@ describe('DotContentDriveShellComponent', () => { baseType: 'DOTASSET' }); - expect(dotMessageService.get).toHaveBeenCalledWith( - 'content-drive.upload.toast.backgrounded-detail', - '2' + expect(store.startExternalRun).toHaveBeenCalledWith( + expect.objectContaining({ + labelKey: 'content-drive.upload.indicator.background', + total: 2 + }) ); - expect(dotMessageService.get).not.toHaveBeenCalledWith( - 'content-drive.upload.toast.backgrounded-detail', - '3' + expect(store.startExternalRun).not.toHaveBeenCalledWith( + expect.objectContaining({ + labelKey: 'content-drive.upload.indicator.background', + total: 3 + }) ); }); @@ -2277,9 +2524,11 @@ describe('DotContentDriveShellComponent', () => { baseType: 'DOTASSET' }); - expect(dotMessageService.get).toHaveBeenCalledWith( - 'content-drive.upload.toast.backgrounded-detail', - '2' + expect(store.startExternalRun).toHaveBeenCalledWith( + expect.objectContaining({ + labelKey: 'content-drive.upload.indicator.background', + total: 2 + }) ); }); @@ -2768,10 +3017,9 @@ describe('DotContentDriveShellComponent', () => { baseType: 'DOTASSET' }); - expect(messageService.add).toHaveBeenCalledWith( + expect(store.startExternalRun).toHaveBeenCalledWith( expect.objectContaining({ - severity: 'info', - detail: 'content-drive.upload.toast.backgrounded-detail' + labelKey: expect.stringContaining('content-drive.upload.indicator.background') }) ); // Still nothing that names it a success: the files do not exist yet. @@ -2892,13 +3140,17 @@ describe('DotContentDriveShellComponent', () => { baseType: 'DOTASSET' }); - expect(addSpy).toHaveBeenCalledTimes(1); - expect(addSpy).toHaveBeenCalledWith( - expect.objectContaining({ - severity: 'info', - summary: 'content-drive.upload.toast.backgrounded' - }) - ); + // Nothing on the wide outlet. The handoff advisory this used to assert was removed + // because the status toast beside it already said the upload was in the background, + // and one upload announcing itself twice is the noise this replaced. + // + // Keyed rather than absolute: the status toast is raised by this same service, and it + // is the one thing that SHOULD appear while a run is in flight. + const wideOutletMessages = addSpy.mock.calls + .map(([message]) => message as { key?: string }) + .filter((message) => message.key !== STATUS_TOAST_KEY); + + expect(wideOutletMessages).toEqual([]); }); it('should not announce an upload the listing now shows', () => { @@ -3283,6 +3535,24 @@ describe('DotContentDriveShellComponent', () => { expect(workflowService.bulkFire).toHaveBeenCalled(); }); + // In System Host and all site content there is no selected folder at all, so the + // target arrives undefined and the folder-level check has nothing to answer about. + // The store's gate already knows which scope is open and whose permission applies; + // without deferring to it, a drop here is waved through on the site's answer while + // the Upload button beside it is correctly disabled. + it('should refuse a drop with no target when the scope refuses content', () => { + canAddChildrenSignal.set(false); + + store.dragItems.mockReturnValue({ + folders: [], + contentlets: [MOCK_ITEMS[0] as DotCMSContentlet] + }); + const sidebar = spectator.debugElement.query(By.css('[data-testid="sidebar"]')); + spectator.triggerEventHandler(sidebar, 'moveItems', { targetFolder: undefined }); + + expect(workflowService.bulkFire).not.toHaveBeenCalled(); + }); + // The site root carries no permissions of its own, so the store's site-level answer is // what decides there. it('should refuse a move onto the site root when the site refuses content', () => { @@ -4011,6 +4281,33 @@ describe('DotContentDriveShellComponent', () => { expect(store.setPath).toHaveBeenCalledWith('/documents/'); }); + it('should read the site row as the site root, not as no location at all', () => { + // The tree tells its site row apart from a folder by carrying an empty path. As a + // *location* that means the site root, which is a different thing from all site + // content — and all site content is what an absent location means. Without this + // translation the two collapse into each other and choosing the site row silently + // lands on the flat whole-site view. + const siteRow: DotFolderTreeNodeItem = { + key: 'site', + label: 'demo.dotcms.com', + data: { + id: 'site-123', + hostname: 'demo.dotcms.com', + path: '', + type: 'site' + }, + leaf: false + }; + + store.selectedNode.mockReturnValue(siteRow); + store.setPath.mockClear(); + + spectator.detectChanges(); + spectator.detectChanges(); + + expect(store.setPath).toHaveBeenCalledWith('/'); + }); + it('should not set path when selectedNode is null', () => { store.selectedNode.mockReturnValue(null); store.setPath.mockClear(); @@ -4614,6 +4911,9 @@ describe('DotContentDriveShellComponent — editContent deep link', () => { // their creation affordances on it. $canAddChildren: canAddChildrenSignal, siteCanAddChildren: siteCanAddChildrenSignal, + // The sidebar this shell renders reads it to decide whether to offer the + // System Host entry at all. + systemHostCanRead: systemHostCanReadSignal, currentSite: vi.fn().mockReturnValue(MOCK_SITES[0]), isTreeExpanded: vi.fn().mockReturnValue(false), items: vi.fn().mockReturnValue(MOCK_ITEMS), diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.ts index 7a1e7a954489..30def424a4c7 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/dot-content-drive-shell/dot-content-drive-shell.component.ts @@ -66,14 +66,17 @@ import { DotKeyboardShortcutUnregister, hasOverlayAbove, DotMessagePipe, + DotStatusToastComponent, DotToastComponent, DotUploadDropzoneComponent, - DotUploadTypeSelectorComponent + DotUploadTypeSelectorComponent, + STATUS_TOAST_KEY } from '@dotcms/ui'; import { DotContentDriveActionCenterComponent } from '../components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component'; import { DotContentDriveDialogContentTypeSelectorComponent } from '../components/dialogs/dot-content-drive-dialog-content-type-selector/dot-content-drive-dialog-content-type-selector.component'; import { DotContentDriveDialogFolderComponent } from '../components/dialogs/dot-content-drive-dialog-folder/dot-content-drive-dialog-folder.component'; +import { DotContentDriveScopeBarComponent } from '../components/dot-content-drive-scope-bar/dot-content-drive-scope-bar.component'; import { DotContentDriveSidebarComponent } from '../components/dot-content-drive-sidebar/dot-content-drive-sidebar.component'; import { DotContentDriveToolbarComponent } from '../components/dot-content-drive-toolbar/dot-content-drive-toolbar.component'; import { DotFolderListViewContextMenuComponent } from '../components/dot-folder-list-context-menu/dot-folder-list-context-menu.component'; @@ -87,7 +90,8 @@ import { ERROR_MESSAGE_LIFE, MOVE_TO_FOLDER_WORKFLOW_ACTION_ID, UPLOAD_BATCH_OPERATION, - NEW_CONTENT_MARKER + NEW_CONTENT_MARKER, + ROOT_PATH } from '../shared/constants'; import { DotContentDriveContentTypeSelectorPayload, @@ -107,12 +111,13 @@ import { canAddChildrenTo, encodeFilters, isFolder, + browsedFolderRef, normalizeFolderRef, - toFolderRef + toFolderRef, + uploadIndicatorKey } from '../utils/functions'; import { refuseOverCeiling } from '../utils/upload-ceilings'; import { describeUploadFailures } from '../utils/upload-failures'; - @Component({ selector: 'dot-content-drive-shell', imports: [ @@ -130,10 +135,12 @@ import { describeUploadFailures } from '../utils/upload-failures'; MessageModule, DotMessagePipe, DotUploadDropzoneComponent, + DotStatusToastComponent, DotToastComponent, DotEditContentSidePanelComponent, ProgressSpinnerModule, - DotContentDriveActionCenterComponent + DotContentDriveActionCenterComponent, + DotContentDriveScopeBarComponent ], providers: [ DotContentDriveStore, @@ -164,7 +171,7 @@ import { describeUploadFailures } from '../utils/upload-failures'; templateUrl: './dot-content-drive-shell.component.html', changeDetection: ChangeDetectionStrategy.OnPush, host: { - class: 'grid relative h-full grid-cols-[min-content_1fr_min-content] grid-rows-[min-content_min-content_1fr]', + class: 'grid relative h-full grid-cols-[min-content_1fr_min-content] grid-rows-[min-content_min-content_min-content_1fr]', // Bound here rather than with addEventListener: Angular unbinds it when the shell is // destroyed. A hand-rolled window listener outlives the portlet unless every teardown path // remembers to remove it, and then a stale closure keeps guarding the page on a count that @@ -230,6 +237,15 @@ export class DotContentDriveShellComponent implements OnDestroy { */ readonly $treeExpanded = this.#store.isTreeVisuallyExpanded; + /** + * Whether the scope bar has anything to say. + * + * Only all site content gets one: the site root and System Host each answer the question the + * bar asks simply by being chosen, leaving nothing for its sentence to qualify or its toggle to + * decide. + */ + readonly $allSiteContentSelected = this.#store.$allSiteContentSelected; + /** * Folder a dropped file lands in. The shared dropzone is presentational, so the target comes * from here rather than the dropzone reaching into the store itself. @@ -664,7 +680,133 @@ export class DotContentDriveShellComponent implements OnDestroy { !affectedFolders?.length || affectedFolders .map(normalizeFolderRef) - .includes(toFolderRef(this.#store.currentSite()?.hostname, this.#store.path())); + .includes(browsedFolderRef(this.#store.currentSite()?.hostname, this.#store.path())); + + /** + * The action currently being applied, surfaced here because the run outlives the Action Center + * dialog. Once the user closes that dialog the toolbar is the only place still reporting the run, + * so without this the work would continue with no indication until the completion toast fired. + */ + readonly $actionExecution = this.#store.toolbarRun; + + /** + * How many runs are in flight. With several at once the store leaves the run undefined on + * purpose, so keying anything off the run alone would go quiet exactly when the most is + * happening. + */ + readonly $activeRunCount = this.#store.toolbarRunCount; + + readonly $hasRunInFlight = computed(() => this.$activeRunCount() > 0); + + /** + * Resolved indicator label. Built here rather than in the template because `DotMessagePipe` takes + * `string[]` arguments and the item count is a number. + * + * The action name is escaped because this label is bound with `[innerHTML]` — the message itself + * carries a ``, which is the only reason it is not plain interpolation. For a workflow action + * that name is `WorkflowAction.name` straight from the backend, so without this a name containing + * markup becomes real DOM. Angular's sanitizer already drops event-handler attributes, so this is + * not an XSS fix; what it stops is structural injection that survives sanitizing — an `` + * pointing at an arbitrary URL, a link, or markup that simply breaks the toolbar's layout. + */ + readonly $actionExecutionLabel = computed(() => { + const execution = this.$actionExecution(); + + // Several at once: name none of them and report the number instead (FR-017). + if (!execution) { + return this.$activeRunCount() > 1 + ? this.#dotMessageService.get( + 'content-drive.action-center.applying-many', + String(this.$activeRunCount()) + ) + : ''; + } + + // A run says whatever it brought, and nothing otherwise. + // + // There used to be an "Applying X to Y" fallback here for runs with no copy of their own. + // Nothing could reach it: only an *unmarked* run arrives here (`toolbarRun` filters to + // `targets.length === 0`, because a run whose rows are marked in the grid is already + // telling the author where it is), and every unmarked run is an upload, which names + // itself. Its one real effect was on uploads before they had their own words, where it + // produced "Applying Upload to demo.dotcms.com" -- a sentence for an action performed ON + // content rather than for files going INTO a place. + // + // So a run arriving with no `labelKey` is one nobody has written words for, and inventing + // some is what caused that. Silence is the honest answer, and the effect above raises + // nothing for an empty label. + // + // The count is the only thing interpolated. It is a number this code produced, so nothing + // here needs escaping even though the message carries its own `` and is therefore bound + // with `[innerHTML]`. Anything author-written that is ever added to these messages does. + return execution.labelKey + ? this.#dotMessageService.get(execution.labelKey, String(execution.total)) + : ''; + }); + + /** What the status toast is currently saying, so an unchanged run is not re-raised. */ + #shownRunLabel: string | undefined; + + /** + * Mirrors the run in flight into the status toast. + * + * The toolbar used to draw this itself, at the end of the filter row. It moved because the row + * is where the user works — filter chips come and go beside it — and a status that appears and + * disappears there shifts the controls under the pointer. A toast says the same thing without + * competing for that space, and gives the in-flight state and its outcome one surface instead + * of an indicator here and a toast elsewhere. + * + * Sticky while the run lasts and cleared when it settles: the outcome toast that follows is + * raised by the shell, which is where results are turned into copy. + * + * The percentage the old indicator could show is deliberately not carried over. Nothing ever + * sets a run's `processed` — `updateExternalRun` has no callers — so it could not render, and + * the app's HTTP backend does not report upload progress either. + */ + readonly runToastSync = effect(() => { + const running = this.$hasRunInFlight(); + const label = this.$actionExecutionLabel(); + + untracked(() => { + if (!running) { + this.#shownRunLabel = undefined; + this.#messageService.clear(STATUS_TOAST_KEY); + + return; + } + + // A run with nothing to say raises nothing rather than an empty pill. + if (!label) { + this.#shownRunLabel = undefined; + this.#messageService.clear(STATUS_TOAST_KEY); + + return; + } + + // Only when the wording actually changes. PrimeNG has no update, so re-reporting means + // clearing and raising again — and the old inline indicator simply changed its text, + // so re-animating on every store touch would be a behaviour this replaced, not kept. + // The label does change while runs are in flight: a second run starting collapses it to + // the count form, and finishing brings the named form back. + if (label === this.#shownRunLabel) { + return; + } + + this.#shownRunLabel = label; + this.#messageService.clear(STATUS_TOAST_KEY); + this.#messageService.add({ + key: STATUS_TOAST_KEY, + severity: 'info', + summary: label, + icon: 'pi pi-spin pi-spinner', + sticky: true, + // PrimeNG reads this off the message, not the outlet, and defaults to closable. + // A status is not the reader's to dismiss: it reports work already under way and + // clears itself when that work settles. + closable: false + }); + }); + }); /** * Reports a finished workflow action as a toast, refreshes the grid, and closes the dialog if it @@ -785,7 +927,7 @@ export class DotContentDriveShellComponent implements OnDestroy { const refusingFolderIsOnScreen = affectedRefs.length === 1 && affectedRefs[0] === - toFolderRef(this.#store.currentSite()?.hostname, this.#store.path()); + browsedFolderRef(this.#store.currentSite()?.hostname, this.#store.path()); // Narrowed the same way the upload itself narrows the selection: the tree's load-more row // is a node without a folder behind it, so it carries no filter to name. @@ -1021,8 +1163,14 @@ export class DotContentDriveShellComponent implements OnDestroy { return; } - if (data.path != currentPath) { - this.#store.setPath(data.path); + // The tree tells its site row apart from a folder by giving it an empty path. As a + // *location* that means the site root, `/`, which is a different thing from all site + // content — and all site content is what an absent location means. Translating here keeps + // the tree's own representation untouched while stopping the two collapsing into one. + const location = data.path === '' ? ROOT_PATH : data.path; + + if (location != currentPath) { + this.#store.setPath(location); } }); @@ -1257,7 +1405,16 @@ export class DotContentDriveShellComponent implements OnDestroy { * @returns {boolean} Whether the drop may proceed */ #canDropInto(targetFolder?: DotFolderTreeNodeData): boolean { - if (canAddChildrenTo(targetFolder, this.#store.siteCanAddChildren())) { + // No target means no folder is selected, which is every scope that is not a folder: all + // site content and System Host. `canAddChildrenTo` answers `true` for an absent target + // because it has nothing to judge, so asking it there would wave the drop through on the + // site's answer while the Upload button beside it is correctly disabled. The store's gate + // already knows which scope is open and whose permission applies. + const allowed = targetFolder + ? canAddChildrenTo(targetFolder, this.#store.siteCanAddChildren()) + : this.#store.$canAddChildren(); + + if (allowed) { return true; } @@ -1496,11 +1653,15 @@ export class DotContentDriveShellComponent implements OnDestroy { // action there is nothing to guard against here: each submission carries its own // freshly chosen files, so two uploads at once is legitimate rather than a double-fire. operation: `${UPLOAD_BATCH_OPERATION}:${(this.#uploadSequence += 1)}`, - actionName: this.#dotMessageService.get('content-drive.upload'), + // Its own wording rather than the workflow sentence. Without this the run reads + // "Applying Upload to demo.dotcms.com" — a phrasing for an action applied TO content, + // which is not what putting files INTO a place is. + // + // The caller picks singular or plural because it is the only place that knows how + // many files were chosen. The messages spell the noun out rather than hedging with + // "file(s)", which is what the count is for. + labelKey: uploadIndicatorKey(files.length), total: files.length, - // `||`, not `??`: the site root's node carries an *empty* path, which is present but - // names nothing, so the indicator would read "Applying Upload to " with a blank target. - targetLabel: hostFolder?.path || this.#store.currentSite()?.hostname, // Empty on purpose. The indicator speaks only for runs with nothing to mark, since a // run over rows is already reported by those rows dimming. An upload's content does not // exist until the run creates it, so the indicator is its only surface — naming the @@ -1535,10 +1696,14 @@ export class DotContentDriveShellComponent implements OnDestroy { // id" reads as "is a folder" and sends a site id as `folderId`, which the server // answers 404 to, correctly: that folder does not exist. An empty `path` is what // marks the row as the site itself. + // + // The last fallback is the *browsed* host, not the site in the switcher: with + // System Host selected the switcher still shows a site, and that site is context + // rather than the destination. ...(hostFolder?.id && hostFolder.path ? { folderId: hostFolder.id } : { - siteId: hostFolder?.id ?? this.#store.currentSite()?.identifier ?? '' + siteId: hostFolder?.id ?? this.#store.$newContentHostId() ?? '' }) }) .subscribe({ @@ -1577,10 +1742,8 @@ export class DotContentDriveShellComponent implements OnDestroy { const backgroundRunId = this.#store.startExternalRun({ operation: `${UPLOAD_BATCH_OPERATION}:${event.handle.jobId}`, - actionName: this.#dotMessageService.get('content-drive.upload'), - labelKey: 'content-drive.upload.indicator.background', + labelKey: uploadIndicatorKey(submitted, { backgrounded: true }), total: submitted, - targetLabel: hostFolder?.path || this.#store.currentSite()?.hostname, targets: [] }); @@ -1592,12 +1755,23 @@ export class DotContentDriveShellComponent implements OnDestroy { this.#store.trackUploadJob( event.handle.jobId, [ - toFolderRef( - hostFolder?.hostname ?? this.#store.currentSite()?.hostname, - // Same reason: an empty path is the site root, which normalises to - // `//hostname` — the ref the listing computes when browsing it. - hostFolder?.path || '/' - ) + hostFolder?.hostname + ? toFolderRef( + hostFolder.hostname, + // An empty path is the site root, which normalises to + // `//hostname` — the ref the listing computes when browsing it. + hostFolder.path || ROOT_PATH + ) + : // No folder chosen means the batch lands wherever the sidebar is + // pointing, which is exactly what the browsed reference describes. + // Rebuilding it from the switcher's site instead named the site + // root while the files were going to System Host, so the run and + // the listing disagreed about where they had landed and the grid + // was never refreshed. + browsedFolderRef( + this.#store.currentSite()?.hostname, + this.#store.path() + ) ], backgroundRunId, // Carried to the outcome because a resubmission means opposite things by @@ -1606,22 +1780,14 @@ export class DotContentDriveShellComponent implements OnDestroy { baseType ); - // The one notification this flow raises, and the only in-flight fact worth - // one: until the handle existed, leaving lost the batch and the page guard - // said so; now leaving costs nothing. That rule changed with no visible - // cause, and the indicator cannot report it — it says work is happening, not - // that the author is released from it. - this.#messageService.add({ - severity: 'info', - summary: this.#dotMessageService.get( - 'content-drive.upload.toast.backgrounded' - ), - detail: this.#dotMessageService.get( - 'content-drive.upload.toast.backgrounded-detail', - String(submitted) - ), - life: SUCCESS_MESSAGE_LIFE - }); + // No notification here any more. This used to raise one, because the + // indicator could say work was happening but not that the page guard had + // released the author. The status toast that replaced the indicator says + // "in the background" itself, and having both on screen meant a backgrounded + // upload announced itself twice, once wide and once compact. + // + // What is genuinely lost is the sentence spelling out that the author may + // leave the page. The wording carries the fact; it no longer argues for it. // Nothing else to do, and deliberately nothing. A `202` means the batch is queued, // not that any file exists, so reloading here refetches a folder whose files @@ -1719,7 +1885,7 @@ export class DotContentDriveShellComponent implements OnDestroy { return; } - const { folderName, pathToMove, dragItems } = this.getMoveMetadata(event); + const { pathToMove, dragItems } = this.getMoveMetadata(event); const dragItemsInodes = dragItems.contentlets.map((item) => item.inode); const assetContentletsCount = dragItems.contentlets.length; @@ -1729,9 +1895,7 @@ export class DotContentDriveShellComponent implements OnDestroy { // which the indicator says better and without stacking up over the outcome that follows. const runId = this.#store.startExternalRun({ operation: MOVE_TO_FOLDER_WORKFLOW_ACTION_ID, - actionName: this.#dotMessageService.get('content-drive.context-menu.move'), total: assetContentletsCount, - targetLabel: folderName, targets: dragItemsInodes }); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/constants.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/constants.ts index 98675b8436ef..b714cc12e3e2 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/constants.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/constants.ts @@ -74,6 +74,16 @@ export const DEFAULT_PATH = undefined; */ export const ROOT_PATH = '/'; +/** + * The location value that means System Host rather than a place inside the current site. + * + * A reserved word can never be mistaken for a folder, because every real path begins with `/` and + * this does not. That is what lets one value in the URL say all four things the sidebar can + * select — absent for all site content, `/` for the site root, a path for a folder, and this — + * without a second value beside it that could disagree. + */ +export const SYSTEM_HOST_PATH = 'SYSTEM_HOST'; + export const DEFAULT_PAGE: DotContentDrivePage = { hasMoreContent: true, hasMoreFolders: true, diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/models.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/models.ts index e1dc8fe3ed9f..1c4c5b43fd9d 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/models.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/shared/models.ts @@ -142,18 +142,8 @@ export interface DotContentDriveDialog { * progress instead of offering to fire it again. */ export interface DotContentDriveActionExecution { - /** Already-resolved action label, not an i18n key — it goes straight into the indicator. */ - actionName: string; /** Number of contentlets the run was fired over. */ total: number; - /** - * What the run is being applied to, when that is one nameable thing. - * - * Lets a single-item run read "Applying **Publish** to *My Page*" instead of "to 1 item(s)". - * Like `actionName` it reaches the indicator's `[innerHTML]`, and unlike `actionName` it is - * content the author typed, so escaping it is not optional. - */ - targetLabel?: string; /** * Copy this run names itself with, instead of the indicator's "Applying X to Y" form. * @@ -163,15 +153,6 @@ export interface DotContentDriveActionExecution { * operation. Resolved with the target label and the total as arguments, in that order. */ labelKey?: string; - /** - * How many items are done, when the run reports it. - * - * **Optional on purpose.** Absent means the run does not report progress, which the indicator - * shows as activity without a position — never as zero. Progress readback is the backend's - * largest piece of hidden work, so the indicator must degrade to indeterminate rather than - * assume a number exists. - */ - processed?: number; } /** @@ -464,3 +445,28 @@ export type DotContentDriveFilters = Partial & { * @interface DotContentDriveDecodeFunction */ export type DotContentDriveDecodeFunction = (value: string) => string | string[]; + +/** A single workflow filter entry: one scheme, optionally pinned to a step. */ +export interface WorkflowFilterEntry { + scheme: string; + step?: string; +} + +/** + * One level of the folder hierarchy returned by {@link getFolderHierarchyByPath}. + * `path` is the parent path that was queried; `folders` are its direct children (first page). + */ +export type FolderTreeHierarchyLevel = { + path: string; + folders: DotFolder[]; + totalEntries: number; + /** + * The 1-based page "Load more" should request next for this level, expressed in + * {@link FOLDER_TREE_PAGE_SIZE} units because that is what load-more pages by. + * + * Derived from the folders actually fetched, never from the rendered node count: a level can + * carry one extra folder that {@link resolveHierarchyAncestor} appended out of sort order, and + * counting that as paged-through would make load-more skip a page of real folders. + */ + nextPage: number; +}; diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.spec.ts index 06e8279308c0..cb7d0b66a8ab 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.spec.ts @@ -47,6 +47,7 @@ import { DEFAULT_PAGINATION, DEFAULT_PATH, DEFAULT_SORT, + ROOT_PATH, DEFAULT_TREE_EXPANDED, SHARED_ASSETS_DISABLED_VALUE, SHARED_ASSETS_ENABLED_VALUE, @@ -295,10 +296,12 @@ describe('DotContentDriveStore', () => { it('should still show folders when a language is selected', () => { // Folders have no language, so a locale filter — which selects a *version* of content — - // must not tear down the structure being navigated. + // must not tear down the structure being navigated. Asserted at the site root, which is + // where structure exists: all site content asks for no folders by design, so testing it + // there would prove nothing about the language filter. store.initContentDrive({ currentSite: SYSTEM_HOST, - path: DEFAULT_PATH, + path: ROOT_PATH, filters: { languageId: ['1', '2'] }, isTreeExpanded: false }); @@ -339,7 +342,10 @@ describe('DotContentDriveStore', () => { // Likewise `status`: omitted entirely when nothing is selected, so an unfiltered // request stays byte-identical to one that never knew about the filter (FR-002). expect(request.status).toBeUndefined(); - expect(request.showFolders).toBe(true); + // No location means all site content, which spans every folder in the site and so + // lists none of them; the tree is still there to navigate. Browsing the site root + // instead is what asks for the top-level folders. + expect(request.showFolders).toBe(false); }); describe('includeSystemHost', () => { @@ -425,6 +431,146 @@ describe('DotContentDriveStore', () => { expect(request.assetPath).toBe(`//${customSite.hostname}/`); }); + describe('browse scope', () => { + // The wire values are written out rather than imported: this is the one place the + // client's idea of a location turns into what the endpoint is asked for, so the + // assertions should fail if that mapping drifts, not follow it. + + it('should ask for all site content when the URL carries no location', () => { + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: DEFAULT_PATH, + filters: {}, + isTreeExpanded: false + }); + + const request = store.$request(); + + expect(request.browseScope).toBe('ALL'); + expect(request.assetPath).toBe(`//${SYSTEM_HOST.hostname}/`); + }); + + it('should ask for the site root when the location is the root itself', () => { + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: '/', + filters: {}, + isTreeExpanded: false + }); + + const request = store.$request(); + + expect(request.browseScope).toBe('ROOT'); + expect(request.assetPath).toBe(`//${SYSTEM_HOST.hostname}/`); + }); + + it('should name no scope when the location is a folder', () => { + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: '/documents/', + filters: {}, + isTreeExpanded: false + }); + + const request = store.$request(); + + // A folder is addressed by its path alone. Naming a scope as well would be + // refused by the endpoint, and it is what would turn this into a listing of + // every descendant. + expect(request.browseScope).toBeUndefined(); + expect(request.assetPath).toBe(`//${SYSTEM_HOST.hostname}/documents/`); + }); + + it('should ask for System Host without pasting the reserved word onto the site', () => { + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: 'SYSTEM_HOST', + filters: {}, + isTreeExpanded: false + }); + + const request = store.$request(); + + expect(request.browseScope).toBe('SYSTEM_HOST'); + // The bug this exists to catch: interpolating the location straight into the + // path produces `//demo.dotcms.comSYSTEM_HOST`, which resolves to nothing. + expect(request.assetPath).toBe(`//${SYSTEM_HOST.hostname}/`); + }); + + it('should keep System Host selected when the site is switched', () => { + // **Characterization test: green the day it is written**, like the host-clause + // guard on the backend. System Host belongs to no site, so switching sites does + // not change what it lists, and the selection already survives because it is + // derived from the location while a switch changes the site. + // + // Written precisely because nothing would notice if that stopped being true. A + // later change that reset the path on a site switch would drift the highlight + // onto the new site's root, and the sidebar would claim the user is in two + // places at once — with every other test still passing. + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: 'SYSTEM_HOST', + filters: {}, + isTreeExpanded: false + }); + expect(store.$systemHostSelected()).toBe(true); + + // A site switch reaches the store as a re-init carrying the new site and the + // location the route still holds — which is how the switch can change the + // site without disturbing where the drive is browsing. + store.initContentDrive({ + currentSite: MOCK_SITES[0], + path: 'SYSTEM_HOST', + filters: {}, + isTreeExpanded: false + }); + + expect(store.$systemHostSelected()).toBe(true); + expect(store.$allSiteContentSelected()).toBe(false); + // The hierarchy below re-renders for the newly chosen site, so the switch + // visibly does something rather than appearing to fail. + expect(store.currentSite()).toEqual(MOCK_SITES[0]); + // And the request still asks for System Host, not for the new site's content. + expect(store.$request().browseScope).toBe('SYSTEM_HOST'); + }); + + it('should not ask for folders in all site content', () => { + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: DEFAULT_PATH, + filters: {}, + isTreeExpanded: false + }); + + // Folders are not results in a flat listing that spans the whole site, and the + // tree is still there to navigate them. + expect(store.$request().showFolders).toBe(false); + }); + + it('should not ask for folders in System Host, which has none', () => { + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: 'SYSTEM_HOST', + filters: {}, + isTreeExpanded: false + }); + + expect(store.$request().showFolders).toBe(false); + }); + + it('should still ask for folders at the site root', () => { + store.initContentDrive({ + currentSite: SYSTEM_HOST, + path: '/', + filters: {}, + isTreeExpanded: false + }); + + // The top-level folders sit at the root, so they are part of what is there. + expect(store.$request().showFolders).toBe(true); + }); + }); + it('should include title filter in request when provided', () => { const filters = { title: 'Blog Post' @@ -589,14 +735,15 @@ describe('DotContentDriveStore', () => { it('should KEEP showFolders true when a languageId filter is provided', () => { // Folders have no language, so a locale filter — which picks a *version* of content - // — must not tear down the structure being navigated. + // — must not tear down the structure being navigated. At the site root for the same + // reason as the sibling test above. const filters = { languageId: ['en'] }; store.initContentDrive({ currentSite: SYSTEM_HOST, - path: DEFAULT_PATH, + path: ROOT_PATH, filters, isTreeExpanded: false }); @@ -641,7 +788,7 @@ describe('DotContentDriveStore', () => { it('should drop the status filter when filters are cleared', () => { store.initContentDrive({ currentSite: SYSTEM_HOST, - path: DEFAULT_PATH, + path: ROOT_PATH, filters: { status: ['ARCHIVED'] }, isTreeExpanded: false }); @@ -688,9 +835,11 @@ describe('DotContentDriveStore', () => { }); it('should set showFolders to true when no filters are provided', () => { + // At the site root: with no location at all this is now all site content, which + // asks for no folders whatever the filters say. store.initContentDrive({ currentSite: SYSTEM_HOST, - path: DEFAULT_PATH, + path: ROOT_PATH, filters: {}, isTreeExpanded: false }); @@ -1960,7 +2109,7 @@ describe('DotContentDriveStore - withActionExecution', () => { store.executeQuickAction('LOCK', 'Lock', ['inode-2']); const lockInFlight = store.actionExecution(); - expect(lockInFlight).toEqual(expect.objectContaining({ actionName: 'Lock', total: 1 })); + expect(lockInFlight).toEqual(expect.objectContaining({ operation: 'LOCK', total: 1 })); store.reportRefreshCompleted('Refresh', { jobId: 'job-1', @@ -2021,7 +2170,7 @@ describe('DotContentDriveStore - withActionExecution', () => { store.executeQuickAction('LOCK', 'Lock', ['inode-2']); const lockInFlight = store.actionExecution(); - expect(lockInFlight).toEqual(expect.objectContaining({ actionName: 'Lock', total: 1 })); + expect(lockInFlight).toEqual(expect.objectContaining({ operation: 'LOCK', total: 1 })); store.reportRefreshCompleted('Refresh', { jobId: 'job-1', @@ -2141,7 +2290,7 @@ describe('DotContentDriveStore - withActionExecution', () => { store.executeQuickAction('LOCK', 'Lock', ['inode-1', 'inode-2']); expect(store.actionExecution()).toEqual( - expect.objectContaining({ actionName: 'Lock', total: 2 }) + expect.objectContaining({ operation: 'LOCK', total: 2 }) ); }); @@ -2427,7 +2576,7 @@ describe('DotContentDriveStore - withActionExecution', () => { store.executeAddToBundle('Add to Bundle', BUNDLE, ['id-1', 'id-2']); expect(store.actionExecution()).toEqual( - expect.objectContaining({ actionName: 'Add to Bundle', total: 2 }) + expect.objectContaining({ operation: 'Add to Bundle', total: 2 }) ); }); @@ -2601,7 +2750,7 @@ describe('DotContentDriveStore - withActionExecution', () => { store.executePushPublish('Push Publish', ['id-1', 'id-2'], SETTINGS); expect(store.actionExecution()).toEqual( - expect.objectContaining({ actionName: 'Push Publish', total: 2 }) + expect.objectContaining({ operation: 'Push Publish', total: 2 }) ); }); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.ts index 2a60a0bf50cf..251627267903 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/dot-content-drive.store.ts @@ -46,6 +46,7 @@ import { SHARED_ASSETS_DISABLED_VALUE, SHARED_ASSETS_FILTER_KEY, SYSTEM_HOST, + SYSTEM_HOST_PATH, USER_SEARCHABLE_PREFIX } from '../shared/constants'; import { @@ -60,8 +61,10 @@ import { buildUserSearchablePayload, decodeFilters, getUserSearchableActive, + listsFolders, parseWorkflowFilter, sortedEncodedFilters, + toRequestLocation, withFilterDefaults } from '../utils/functions'; @@ -125,8 +128,14 @@ export const DotContentDriveStore = signalStore( userSearchableFields() ); + // One value says where the user is; this is where it becomes the two the + // endpoint expects. Mapped rather than interpolated: pasting the location + // into the path yields `//demo.dotcms.comSYSTEM_HOST` for a reserved word. + const location = toRequestLocation(currentSite()?.hostname, path()); + return { - assetPath: `//${currentSite()?.hostname}${path() || '/'}`, + assetPath: location.assetPath, + browseScope: location.browseScope, // Off only when explicitly turned off. The key is seeded on every path // that builds filters (see `withFilterDefaults`), so a missing one means // state that predates the seeding, not a deliberate opt-out. @@ -162,6 +171,9 @@ export const DotContentDriveStore = signalStore( // and pinning it would contradict an Archived selection. status: filters()?.status?.length ? filters()?.status : undefined, showFolders: + // Folders are not results in a listing that spans the whole site, + // and System Host has none. + listsFolders(location.browseScope) && page.hasMoreFolders && !filters()?.baseType?.length && !filters()?.contentType?.length && @@ -680,7 +692,9 @@ export const DotContentDriveStore = signalStore( withActionExecution(), withPushPublishEnvironments(), withSitePermissions(), - withComputed(() => { + // Sharing one `withComputed` with the sidebar selection below, rather than standing alone: + // `signalStore` takes at most sixteen features, and this store is at that ceiling. + withComputed(({ path }) => { const globalStore = inject(GlobalStore); return { @@ -693,45 +707,134 @@ export const DotContentDriveStore = signalStore( * an instance older than the field, which callers must treat alike — no readable * ceiling, so the refusing is left to the server. */ - uploadCeilings: computed(() => globalStore.systemBulkUpload()) + uploadCeilings: computed(() => globalStore.systemBulkUpload()), + + /** + * Whether the sidebar's first entry, all site content, is the selected one. + * + * Derived from the location rather than stored beside it: an absent location *is* what + * all site content means, so a second piece of state saying so could only ever + * disagree. + */ + $allSiteContentSelected: computed(() => !path()), + + /** Whether the sidebar's last entry, System Host, is the selected one. */ + $systemHostSelected: computed(() => path() === SYSTEM_HOST_PATH) }; }), - withComputed(({ selectedNode, siteCanAddChildren }) => ({ - /** - * Whether the browsed folder accepts new children. - * - * A new folder needs CAN_ADD_CHILDREN on the parent (`FolderAPIImpl:673`) and moving a - * contentlet needs it on the destination (`ESContentletAPIImpl:607`). An **upload does not**: - * the contentlet checkin path never checks it, so that one is gated here for consistency - * rather than as a preview of a refusal — otherwise uploading would quietly allow what - * creating a folder in the same place forbids. - * - * Computed here rather than in each consumer because three surfaces gate on it — the New - * menu, the Upload button and the drop zone — and three copies of the folder-then-site - * fallback would be three chances to disagree. - * - * A node with no permissions is the site root, whose parent is the host rather than a - * folder; `siteCanAddChildren` answers that case. Both unknowns read as allowed: a lookup - * in flight, and an instance too old to report the field. Starting disabled would flicker - * the affordances off and on for the common case, and the server refuses the write anyway. - */ - $canAddChildren: computed(() => { - const permissions = (selectedNode()?.data as { permissions?: string[] } | undefined) - ?.permissions; - - if (!permissions?.length) { - return siteCanAddChildren() !== false; - } + // Both destinations in one feature, for the sixteen-feature ceiling noted above. They read + // the same selection signals and neither depends on the other. + withComputed( + ({ + currentSite, + selectedNode, + siteCanAddChildren, + systemHostCanAddChildren, + $allSiteContentSelected, + $systemHostSelected + }) => ({ + /** + * The host that would receive new content here. + * + * Three paths ask this and used to answer it separately: the upload button, a drag and + * drop, and the New menu. Each fell back to the current site when no folder was + * selected, which is right everywhere except System Host, where the current site is + * context rather than the destination. The New menu was worse than wrong — it built + * its target by pasting the location onto the hostname, which with a reserved word + * yields `demo.dotcms.comSYSTEM_HOST` and resolves to nothing. + * + * A folder, when one is selected, is still more specific than this and wins. + */ + $newContentHostId: computed(() => + $systemHostSelected() ? SYSTEM_HOST.identifier : currentSite()?.identifier + ), + + /** + * Whether the browsed folder accepts new children. + * + * A new folder needs CAN_ADD_CHILDREN on the parent (`FolderAPIImpl:673`) and moving a + * contentlet needs it on the destination (`ESContentletAPIImpl:607`). An **upload does not**: + * the contentlet checkin path never checks it, so that one is gated here for consistency + * rather than as a preview of a refusal — otherwise uploading would quietly allow what + * creating a folder in the same place forbids. + * + * Computed here rather than in each consumer because three surfaces gate on it — the New + * menu, the Upload button and the drop zone — and three copies of the folder-then-site + * fallback would be three chances to disagree. + * + * A node with no permissions is the site root, whose parent is the host rather than a + * folder; `siteCanAddChildren` answers that case. Both unknowns read as allowed: a lookup + * in flight, and an instance too old to report the field. Starting disabled would flicker + * the affordances off and on for the common case, and the server refuses the write anyway. + */ + $canAddChildren: computed(() => { + // System Host is a real destination, so this is a permission answer — but about + // System Host, not about whichever site the switcher happens to show. + if ($systemHostSelected()) { + return systemHostCanAddChildren() !== false; + } - return permissions.includes(PERMISSIONS_TYPE.CAN_ADD_CHILDREN); + // All site content spans every folder, so it names no single place — but content + // added here lands on the site root, and that is whose permission decides. Asked + // before the node below on purpose: selecting all site content clears the tree + // selection, and a node left over from before it was cleared would be answering + // about a folder that is not the destination. + if ($allSiteContentSelected()) { + return siteCanAddChildren() !== false; + } + + const permissions = (selectedNode()?.data as { permissions?: string[] } | undefined) + ?.permissions; + + if (!permissions?.length) { + return siteCanAddChildren() !== false; + } + + return permissions.includes(PERMISSIONS_TYPE.CAN_ADD_CHILDREN); + }) }) - })), - withHooks((store) => ({ - onInit() { - // Fed the signal rather than called on each site change: `rxMethod` re-runs on every - // emission and `switchMap` drops the previous site's in-flight answer, so switching - // sites quickly can never settle the gate with the wrong site's result. - store.loadSitePermissions(store.currentSite); - } - })) + ), + withHooks((store) => { + let systemHostGate: EffectRef | undefined; + + return { + onInit() { + // Fed the signal rather than called on each site change: `rxMethod` re-runs on every + // emission and `switchMap` drops the previous site's in-flight answer, so switching + // sites quickly can never settle the gate with the wrong site's result. + store.loadSitePermissions(store.currentSite); + // Once, not per site: System Host belongs to none of them. + store.loadSystemHostPermissions(); + + /** + * Sends a user who cannot read System Host back to all site content. + * + * The sidebar hides the entry, but hiding a button is not a gate: the location is + * carried in the URL, so a link, a reload or a typed address reaches the scope + * without ever touching the sidebar. + * + * Silently, and to all site content rather than an error: the user did nothing + * wrong — usually they followed a colleague's link — and the drive has somewhere + * sensible to put them. This is an affordance, not a defence; the listing enforces + * read permissions on its own, so nothing here is what stops content leaking. + * + * Lives in the store's own hooks rather than in `withSidebar`, which composes + * earlier and cannot see this answer. + */ + systemHostGate = effect(() => { + const onSystemHost = store.$systemHostSelected(); + const canRead = store.systemHostCanRead(); + + untracked(() => { + if (onSystemHost && !canRead) { + store.selectAllSiteContent(); + } + }); + }); + }, + onDestroy() { + systemHostGate?.destroy(); + } + }; + }) ); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.spec.ts index 6e287ae357db..34aed983d694 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.spec.ts @@ -142,7 +142,7 @@ describe('withActionExecution', () => { // `objectContaining`: a run now also carries its id, operation and targets. The two // fields the indicator reads are what this pins. expect(store.actionExecution()).toEqual( - expect.objectContaining({ actionName: 'Lock', total: 2 }) + expect.objectContaining({ operation: 'lock-id', total: 2 }) ); }); @@ -181,7 +181,7 @@ describe('withActionExecution', () => { expect(fireDefaultAction).toHaveBeenCalledTimes(1); expect(store.actionExecution()).toEqual( - expect.objectContaining({ actionName: 'Lock', total: 1 }) + expect.objectContaining({ operation: 'lock-id', total: 1 }) ); }); @@ -626,7 +626,6 @@ describe('withActionExecution', () => { build(); const runId = store.startExternalRun({ operation: 'upload:1', - actionName: 'Upload', total: 1, targets: [] as string[] }); @@ -728,7 +727,7 @@ describe('withActionExecution', () => { // Was a real hazard when there was one slot to wipe. Keying runs by id removes it by // construction, so this now guards the property rather than the workaround. expect(store.actionExecution()).toEqual( - expect.objectContaining({ actionName: 'Lock', total: 1 }) + expect.objectContaining({ operation: 'lock-id', total: 1 }) ); }); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.ts index bf8c2b29e140..ad40092ccc71 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/action-execution/withActionExecution.ts @@ -41,7 +41,7 @@ import { DotContentDriveUploadJob, DotContentDriveState } from '../../../shared/models'; -import { normalizeFolderRef, toFolderRef } from '../../../utils/functions'; +import { browsedFolderRef, normalizeFolderRef } from '../../../utils/functions'; interface WithActionExecutionState { /** @@ -329,7 +329,6 @@ export function withActionExecution() { const runId = startRun({ operation: actionName, - actionName, // Counted in identifiers, because that is what the server queues: language // versions of one contentlet are one asset. total: identifiers.length, @@ -408,7 +407,6 @@ export function withActionExecution() { const runId = startRun({ operation: actionId, - actionName, total: inodes.length, targets: inodes }); @@ -619,7 +617,7 @@ export function withActionExecution() { // A move changes two folders: the one the rows leave and the one they // arrive in. Every other workflow action changes rows where they already // are, so the browsed folder is the only one affected. - const browsedFolder = toFolderRef( + const browsedFolder = browsedFolderRef( store.currentSite()?.hostname, store.path() ); @@ -629,7 +627,6 @@ export function withActionExecution() { const runId = startRun({ operation: workflowActionId, - actionName, total: contentletIds.length, targets: contentletIds }); @@ -752,28 +749,6 @@ export function withActionExecution() { /** Settles a run registered with {@link startExternalRun}. */ endExternalRun: (runId: string): void => endRun(runId), - /** - * Updates a run in flight, for the fields it reports as it goes. - * - * Ignores a run that is already gone rather than resurrecting it: progress can - * arrive a tick after the run settled, and re-adding it would leave the - * indicator reporting something finished. - */ - updateExternalRun: ( - runId: string, - patch: Partial> - ): void => { - const run = store.runs()[runId]; - - if (!run) { - return; - } - - patchState(store, { - runs: { ...store.runs(), [runId]: { ...run, ...patch } } - }); - }, - /** * Remembers a batch this store submitted, so its completion can be told from * another tab's. diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.spec.ts index 33bf99965a3e..427231e741c8 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.spec.ts @@ -10,7 +10,7 @@ import { createFakeFolderSearchView, createFakeSite } from '@dotcms/utils-testin import { withSidebar } from './withSidebar'; -import { SYSTEM_HOST } from '../../../shared/constants'; +import { ROOT_PATH, SYSTEM_HOST, SYSTEM_HOST_PATH } from '../../../shared/constants'; import { DotContentDriveSortOrder, DotContentDriveState, @@ -548,3 +548,89 @@ describe('withSidebar - undefined path scenarios', () => { })); }); }); + +describe('withSidebar - a location that is not a folder', () => { + let spectator: SpectatorService>; + let store: InstanceType; + let folderService: Mocked; + + // System Host belongs to no site, so it is nowhere in this site's hierarchy. It reaches the + // sidebar as a location like any other, which is what made it look like a folder path. + const systemHostLocationStoreMock = signalStore( + withState({ + ...initialState, + path: SYSTEM_HOST_PATH + }), + withSidebar() + ); + + const createService = createServiceFactory({ + service: systemHostLocationStoreMock, + providers: [ + mockProvider(DotFolderService, { + searchFolders: vi.fn().mockReturnValue(searchResult([])) + }) + ] + }); + + beforeEach(() => { + spectator = createService(); + store = spectator.service; + folderService = spectator.inject(DotFolderService); + }); + + it('should not go looking for it among the site folders', () => { + // The site's own root level is still fetched — the tree shows this site's folders whatever + // location is open. What must not happen is resolving the location itself as a folder: it + // was turned into `/SYSTEM_HOST/` and queried, a folder nobody has, so the request could + // only ever come back empty. + expect(folderService.searchFolders).not.toHaveBeenCalledWith( + expect.objectContaining({ path: '/SYSTEM_HOST/' }) + ); + }); + + it('should leave the tree with nothing selected', () => { + // The failing behaviour, and the one the user sees: the hierarchy load fell back to the + // site row, so the site root and System Host both looked selected at once. Worse, the + // shell syncs the location from the selected node, so that row then rewrote the location + // to the site root and bounced the user straight back out of System Host. + expect(store.selectedNode()).toBeUndefined(); + }); +}); + +describe('withSidebar - the site root as a location', () => { + let spectator: SpectatorService>; + let store: InstanceType; + + const rootPathStoreMock = signalStore( + withState({ + ...initialState, + path: ROOT_PATH + }), + withSidebar() + ); + + const createService = createServiceFactory({ + service: rootPathStoreMock, + providers: [ + mockProvider(DotFolderService, { + searchFolders: vi.fn().mockReturnValue(searchResult([])) + }) + ] + }); + + beforeEach(() => { + spectator = createService(); + store = spectator.service; + }); + + it('should keep the site row selected once the location settles', () => { + // The tree marks its site row with an empty path, while the site root as a *location* is + // `/`. Looking the location up literally finds no node, so the sync that keeps the tree in + // step with the location read that as "nothing here" and cleared the selection every time + // the user was at the site root. + spectator.flushEffects(); + + expect(store.selectedNode()?.data?.id).toBe(mockSite.identifier); + }); +}); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.ts index c1ab51cd0169..d58fa0bea279 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/sidebar/withSidebar.ts @@ -9,22 +9,31 @@ import { import { rxMethod } from '@ngrx/signals/rxjs-interop'; import { Observable, of, pipe, switchMap, tap } from 'rxjs'; -import { inject } from '@angular/core'; +import { effect, EffectRef, inject, untracked } from '@angular/core'; import { catchError } from 'rxjs/operators'; import { DotFolderService } from '@dotcms/data-access'; import { DotFolderTreeNodeItem } from '@dotcms/portlets/content-drive/ui'; -import { SYSTEM_HOST } from '../../../shared/constants'; -import { DotContentDriveState } from '../../../shared/models'; +import { + DEFAULT_PAGE, + DEFAULT_PATH, + ROOT_PATH, + SYSTEM_HOST, + SYSTEM_HOST_PATH +} from '../../../shared/constants'; +import { DotContentDriveState, FolderTreeHierarchyLevel } from '../../../shared/models'; import { applyLoadMoreToHierarchy, - FolderTreeHierarchyLevel, getFolderHierarchyByPath, getFolderNodesByPath } from '../../../utils/functions'; -import { buildTreeFolderNodes, createSiteNode } from '../../../utils/tree-folder.utils'; +import { + buildTreeFolderNodes, + createSiteNode, + findNodeByPath +} from '../../../utils/tree-folder.utils'; interface WithSidebarState { sidebarLoading: boolean; @@ -68,7 +77,17 @@ export function withSidebar() { } const siteNode = createSiteNode(currentSite); - const urlFolderPath = store.path() || ''; + + // Only a folder path names a place inside this site's hierarchy. The other + // two locations do not: all site content is the absence of one, and System + // Host is a host rather than a folder. Both were resolved as folder paths + // anyway, so System Host was queried as `/SYSTEM_HOST/` — a folder nobody + // has — and the empty result fell back to selecting the site row. That left + // the site root and System Host both looking selected, and since the shell + // syncs the location *from* the selected node, the site row then rewrote the + // location back to the site root and bounced the user out of System Host. + const location = store.path() || ''; + const urlFolderPath = location.startsWith(ROOT_PATH) ? location : ''; // Only the initial state used to set this, so every later cold load (a site // change) left the previous site's tree on screen while its replacement was @@ -117,7 +136,11 @@ export function withSidebar() { // nothing, and expanding it fetched them a second time — the // tree showed every root folder twice. folders: [{ ...siteNode, children: rootsWithLoadMore }], - selectedNode: selectedNode + // No location means all site content, which is not a place in + // the hierarchy. Preselecting the site row there would have the + // sidebar claiming the root is what you are looking at, and the + // root and the flat whole-site view are different things. + selectedNode: urlFolderPath ? selectedNode : undefined }); }) ); @@ -159,15 +182,36 @@ export function withSidebar() { }, /** - * Selects the tree's root row, the one that stands for the site rather than a folder. + * Selects all site content: the whole current site at any depth, which is the one + * sidebar entry that names no place inside the hierarchy. + * + * Clearing the selected node is half the job. Exactly one thing in the sidebar is ever + * selected, and the tree cannot represent this entry, so leaving a node selected would + * have the sidebar claiming the user is in two places at once. * - * Used when a search spans the whole site, where no single folder is the selected one. - * A tree of plain folders has no such row, and then nothing is selected, which says the - * same thing. + * The location is cleared rather than set to the root: absent is what all site content + * looks like in the URL, which is also what links made before this feature carry. */ - selectRootNode: () => { + selectAllSiteContent: () => { patchState(store, { - selectedNode: store.folders().find((folder) => !folder.data?.path) + path: DEFAULT_PATH, + selectedNode: undefined, + pagination: { ...store.pagination(), page: 1, offset: 0 }, + pages: [DEFAULT_PAGE] + }); + }, + + /** + * Selects System Host: shared content on its own, which belongs to no site and so has + * no place in the hierarchy either. Same shape as choosing all site content — one + * entry selected, the tree's own selection cleared. + */ + selectSystemHost: () => { + patchState(store, { + path: SYSTEM_HOST_PATH, + selectedNode: undefined, + pagination: { ...store.pagination(), page: 1, offset: 0 }, + pages: [DEFAULT_PAGE] }); }, @@ -183,9 +227,47 @@ export function withSidebar() { } })), withHooks((store) => { + let selectionSync: EffectRef | undefined; + return { onInit() { store.loadFolders(); + + // Keeps the tree's selection honest as the location moves. + // + // Folders reload on a site change, not on a Back, so returning to a folder + // restored the URL and the listing while the tree showed nothing selected. + // Everything else in the sidebar derives its selected state from the location + // and was therefore already right; this is the one stored piece, so it has to + // be pushed back in line rather than left holding whatever the previous + // location put there. + // + // Reads `folders()` as well as `path()` on purpose: on a cold start the tree + // is empty when the location is already known, and this has to run again once + // the folders arrive. + selectionSync = effect(() => { + const path = store.path(); + const folders = store.folders(); + // The tree marks its site row with an empty path, while the site root as a + // *location* is `/` — the same translation the shell makes in the other + // direction. Looking `/` up literally matches no node, so without this the + // sync read the site root as "nowhere" and cleared the selection every time + // the user was standing on it. + const match = path?.startsWith(ROOT_PATH) + ? findNodeByPath(folders, path === ROOT_PATH ? '' : path) + : undefined; + + // Only when it actually differs: a folder click already sets the node, and + // rewriting the same one on every location change churns the tree. + untracked(() => { + if (store.selectedNode()?.data?.path !== match?.data?.path) { + patchState(store, { selectedNode: match }); + } + }); + }); + }, + onDestroy() { + selectionSync?.destroy(); } }; }) diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.spec.ts index 1b02c54afcbb..4bf0d38361a2 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.spec.ts @@ -3,6 +3,8 @@ import { createServiceFactory, mockProvider, SpectatorService } from '@openng/sp import { NEVER, of, throwError } from 'rxjs'; import { describe, expect, it, vi } from 'vitest'; +import { HttpErrorResponse } from '@angular/common/http'; + import { DotPermissionsService } from '@dotcms/data-access'; import { DotSite } from '@dotcms/dotcms-models'; @@ -139,3 +141,57 @@ describe('withSitePermissions', () => { }); }); }); + +describe('withSitePermissions — reading System Host', () => { + let spectator: SpectatorService>; + let store: InstanceType; + + const canAddChildren = vi.fn(); + + const createService = createServiceFactory({ + service: sitePermissionsStoreMock, + providers: [mockProvider(DotPermissionsService, { canAddChildren })] + }); + + beforeEach(() => { + canAddChildren.mockReset(); + }); + + it('should treat a refusal as not readable', () => { + // The permissions resource checks READ before it answers and refuses outright when the + // caller does not hold it, so a 403 from this one call is the server saying the user + // cannot see the asset at all -- not merely that they cannot add to it. + canAddChildren.mockReturnValue(throwError(() => new HttpErrorResponse({ status: 403 }))); + spectator = createService(); + store = spectator.service; + + store.loadSystemHostPermissions(); + + expect(store.systemHostCanRead()).toBe(false); + }); + + it('should keep System Host readable when the lookup fails for any other reason', () => { + // A timeout or a 500 says nothing about permissions. Locking someone out of a scope + // because the network hiccuped is worse than showing them an entry the server will + // police anyway -- the listing enforces read permissions on its own. + canAddChildren.mockReturnValue(throwError(() => new HttpErrorResponse({ status: 500 }))); + spectator = createService(); + store = spectator.service; + + store.loadSystemHostPermissions(); + + expect(store.systemHostCanRead()).toBe(true); + }); + + it('should keep System Host readable when the lookup succeeds', () => { + canAddChildren.mockReturnValue(of(false)); + spectator = createService(); + store = spectator.service; + + store.loadSystemHostPermissions(); + + // Answering the add question at all means the read check upstream of it passed. + expect(store.systemHostCanRead()).toBe(true); + expect(store.systemHostCanAddChildren()).toBe(false); + }); +}); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.ts index 2eae6f2115c9..6b7cfefd94a8 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/store/features/site-permissions/withSitePermissions.ts @@ -3,6 +3,7 @@ import { patchState, signalStoreFeature, withMethods, withState } from '@ngrx/si import { rxMethod } from '@ngrx/signals/rxjs-interop'; import { filter, pipe, switchMap, tap } from 'rxjs'; +import { HttpErrorResponse } from '@angular/common/http'; import { inject } from '@angular/core'; import { DotPermissionsService } from '@dotcms/data-access'; @@ -24,6 +25,29 @@ interface WithSitePermissionsState { * inherits from SYSTEM_HOST and so answers identically no matter which site is open. */ siteCanAddChildren: boolean | undefined; + + /** + * CAN_ADD_CHILDREN on System Host itself. + * + * Kept apart from {@link siteCanAddChildren} rather than folded into it. That one is reset and + * re-fetched every time the site changes, and System Host belongs to no site, so sharing the + * slot would have each switch throw away an answer that had not changed and briefly ungate + * the affordances for a destination nobody navigated away from. + */ + systemHostCanAddChildren: boolean | undefined; + + /** + * Whether the user may see System Host at all. + * + * Read from the same lookup rather than a second call: the permissions resource checks READ + * before it answers anything and refuses outright without it, so a refusal IS the read answer. + * + * `true` until told otherwise, including when the lookup fails for any other reason. The + * listing enforces read permissions server-side regardless, so the worst a wrong `true` does + * is offer an entry that lists nothing — where a wrong `false` would lock someone out of a + * scope they are entitled to because the network hiccuped. + */ + systemHostCanRead: boolean; } /** @@ -37,7 +61,9 @@ export function withSitePermissions() { // the only slice it touches, so nothing here needs the host store's shape. return signalStoreFeature( withState({ - siteCanAddChildren: undefined + siteCanAddChildren: undefined, + systemHostCanAddChildren: undefined, + systemHostCanRead: true }), withMethods((store, dotPermissionsService = inject(DotPermissionsService)) => ({ /** @@ -49,6 +75,42 @@ export function withSitePermissions() { * the permission. Push Publish disables on failure because offering a push with nowhere * to send it fails later and less legibly. */ + /** + * Looks up CAN_ADD_CHILDREN on System Host. + * + * Separate from the site lookup, which filters System Host out deliberately: there it + * arrives as the seed the drive holds before a real site resolves, and answering for + * it would answer about the wrong asset. Here it is the destination the user chose, so + * the same identifier means the opposite thing and needs its own way in. + * + * Same failure posture as the site lookup: a transient error settles on allowed, since + * this only softens an affordance the server still guards. + */ + loadSystemHostPermissions: rxMethod( + pipe( + switchMap(() => + dotPermissionsService.canAddChildren(SYSTEM_HOST.identifier).pipe( + tapResponse({ + next: (canAddChildren) => + patchState(store, { + systemHostCanAddChildren: canAddChildren, + // An answer at all means the resource's own READ check + // passed upstream of it. + systemHostCanRead: true + }), + // A refusal is the read answer; anything else says nothing about + // permissions and must not lock the user out of the scope. + error: (error: HttpErrorResponse) => + patchState(store, { + systemHostCanAddChildren: true, + systemHostCanRead: error?.status !== 403 + }) + }) + ) + ) + ) + ), + loadSitePermissions: rxMethod( pipe( // SYSTEM_HOST is the seed the drive holds before a real site resolves, and it diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/functions.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/functions.ts index 2a6067da4919..d22612255703 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/functions.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/functions.ts @@ -8,6 +8,7 @@ import { PERMISSIONS_TYPE, DotContentDriveActionableFolder, DotContentDriveActionableItem, + DotContentDriveBrowseScope, DotFolder, DotSite, FolderSearchView, @@ -22,14 +23,19 @@ import { FOLDER_NAME_FILTER_MIN_LENGTH, FOLDER_TREE_HIERARCHY_PAGE_SIZE, FOLDER_TREE_PAGE_SIZE, + ROOT_PATH, SHARED_ASSETS_ENABLED_VALUE, SHARED_ASSETS_FILTER_KEY, + SYSTEM_HOST, + SYSTEM_HOST_PATH, USER_SEARCHABLE_PREFIX } from '../shared/constants'; import { DotContentDriveDecodeFunction, DotContentDriveFilters, - DotKnownContentDriveFilters + DotKnownContentDriveFilters, + FolderTreeHierarchyLevel, + WorkflowFilterEntry } from '../shared/models'; /** @@ -55,12 +61,6 @@ const multiSelector = (value = ''): string[] => */ const singleSelector: DotContentDriveDecodeFunction = (value = ''): string => value.trim(); -/** A single workflow filter entry: one scheme, optionally pinned to a step. */ -export interface WorkflowFilterEntry { - scheme: string; - step?: string; -} - /** Separator for the `schemeId[:stepId]` workflow token encoding. */ export const WORKFLOW_TOKEN_SEPARATOR = ':'; @@ -434,25 +434,6 @@ export function folderSearchViewToDotFolder(view: FolderSearchView, hostName: st }; } -/** - * One level of the folder hierarchy returned by {@link getFolderHierarchyByPath}. - * `path` is the parent path that was queried; `folders` are its direct children (first page). - */ -export type FolderTreeHierarchyLevel = { - path: string; - folders: DotFolder[]; - totalEntries: number; - /** - * The 1-based page "Load more" should request next for this level, expressed in - * {@link FOLDER_TREE_PAGE_SIZE} units because that is what load-more pages by. - * - * Derived from the folders actually fetched, never from the rendered node count: a level can - * carry one extra folder that {@link resolveHierarchyAncestor} appended out of sort order, and - * counting that as paged-through would make load-more skip a page of real folders. - */ - nextPage: number; -}; - /** * The last segment of a folder path: `/a/b/` → `b`, `/a/` → `a`. * @@ -891,6 +872,52 @@ export function canAddChildrenTo( return permissions.includes(PERMISSIONS_TYPE.CAN_ADD_CHILDREN); } +/** + * Turns the one value that says where the user is browsing into the two the endpoint expects. + * + * The sidebar can select four things and the URL carries one value for all of them: absent means + * all site content, `/` means the site root, a deeper path means that folder, and a reserved word + * means System Host. Keeping it to one value is what stops a location and a scope disagreeing, + * and this is the single place the two representations meet. + * + * Written as a mapping rather than interpolation on purpose. Pasting the location into the path + * produces `//demo.dotcms.comSYSTEM_HOST` for the reserved word, which resolves to nothing. + * + * A folder deliberately gets **no** scope: it is addressed by its path, the endpoint refuses a + * scope alongside a folder path, and a scope there is what would turn the listing into every + * descendant. + */ +export function toRequestLocation( + hostname: string | undefined, + path: string | undefined +): { assetPath: string; browseScope?: DotContentDriveBrowseScope } { + const siteRoot = `//${hostname}/`; + + if (!path?.length) { + return { assetPath: siteRoot, browseScope: 'ALL' }; + } + + if (path === SYSTEM_HOST_PATH) { + return { assetPath: siteRoot, browseScope: 'SYSTEM_HOST' }; + } + + if (path === ROOT_PATH) { + return { assetPath: siteRoot, browseScope: 'ROOT' }; + } + + return { assetPath: `//${hostname}${path}` }; +} + +/** + * Whether the listing should ask for folders at all. + * + * Folders are not results in a listing that spans the whole site, and System Host has none, so + * both of those scopes ask for none. The tree is still there to navigate them. + */ +export function listsFolders(browseScope: DotContentDriveBrowseScope | undefined): boolean { + return browseScope !== 'ALL' && browseScope !== 'SYSTEM_HOST'; +} + /** * Canonical form for comparing two folder references: `//hostname/path`, lower-cased and without a * trailing slash. @@ -907,3 +934,44 @@ export const toFolderRef = (hostname: string | null | undefined, path: string | /** Normalises an already-formed `//hostname/path` reference. See {@link toFolderRef}. */ export const normalizeFolderRef = (ref: string | null | undefined): string => (ref ?? '').toLowerCase().replace(/\/+$/, ''); + +/** + * The folder reference for the location the drive is on, in the form runs are compared against. + * + * Not {@link toFolderRef} applied to the site and the location directly, because the location is + * not always a path on the browsed site. System Host belongs to no site, so pairing its reserved + * location value with whatever hostname the switcher happens to show produced + * `//demo.dotcms.comsystem_host` — a reference to nothing, which matched no run's affected folders. + * The listing therefore never reloaded after an upload landed there. + * + * @param {string | null | undefined} hostname - The browsed site's hostname + * @param {string | null | undefined} path - The location, which may not be a folder path at all + * @returns {string} the canonical reference for what is on screen + */ +export const browsedFolderRef = ( + hostname: string | null | undefined, + path: string | null | undefined +): string => + path === SYSTEM_HOST_PATH + ? toFolderRef(SYSTEM_HOST.hostname, ROOT_PATH) + : toFolderRef(hostname, path); + +/** + * Which wording an upload run names itself with. + * + * The messages spell "file" or "files" out instead of hedging with "file(s)", so the count has to + * choose between them, and only the caller knows the count. Nothing here names a destination: the + * drive already shows where the author is, and the sentence was long enough that the part they + * could read at a glance was getting lost behind the part they could not. + * + * @param {number} count - How many files the batch carries + * @param {{ backgrounded?: boolean }} [options] - Whether the batch is the server's now + * @returns {string} the message key for that run + */ +export const uploadIndicatorKey = (count: number, options?: { backgrounded?: boolean }): string => { + const base = options?.backgrounded + ? 'content-drive.upload.indicator.background' + : 'content-drive.upload.indicator'; + + return count === 1 ? `${base}.one` : base; +}; diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.spec.ts index 8a2cc8e9fec9..ac36ade407ac 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.spec.ts @@ -6,6 +6,7 @@ import { buildTreeFolderNodes, createSiteNode, createTreeNode, + findNodeByPath, generateAllParentPaths } from './tree-folder.utils'; @@ -753,3 +754,59 @@ describe('Sidebar Utils', () => { }); }); }); + +describe('findNodeByPath', () => { + // Shaped like the real tree: the site row on top, its folders as children, nesting below. + const tree = [ + { + key: 'site', + label: 'demo.dotcms.com', + data: { id: 'site-1', path: '', type: 'folder' }, + children: [ + { + key: '/blog/', + label: 'blog', + data: { id: 'f1', path: '/blog/', type: 'folder' }, + children: [ + { + key: '/blog/2026/', + label: '2026', + data: { id: 'f2', path: '/blog/2026/', type: 'folder' }, + children: [] + } + ] + }, + { + key: '/images/', + label: 'images', + data: { id: 'f3', path: '/images/', type: 'folder' }, + children: [] + } + ] + } + ] as never; + + it('should find a folder at the top level', () => { + expect(findNodeByPath(tree, '/images/')?.data?.id).toBe('f3'); + }); + + it('should find a folder nested below another', () => { + // The case the bug turned on: Back can land on any depth, not just a root folder. + expect(findNodeByPath(tree, '/blog/2026/')?.data?.id).toBe('f2'); + }); + + it('should return undefined for a path no folder has', () => { + expect(findNodeByPath(tree, '/nope/')).toBeUndefined(); + }); + + it('should return undefined rather than throwing when the tree is not loaded yet', () => { + // A cold start knows the location before it has any folders, so this is reached on the + // common path rather than as an edge case. + expect(findNodeByPath(undefined, '/blog/')).toBeUndefined(); + }); + + it('should match on the path the URL carries, not the node key', () => { + // Keys encode tree position; the URL carries the path, and the two are not the same thing. + expect(findNodeByPath(tree, 'site')).toBeUndefined(); + }); +}); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.ts index f9a3c7c9aeb9..242a1b7f170d 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/tree-folder.utils.ts @@ -169,3 +169,33 @@ export const buildTreeFolderNodes = ({ return { rootNodes, selectedNode }; }; + +/** + * Finds the node for a folder path in an already-built tree. + * + * The sidebar's two standalone entries derive their selected state from the location, so they are + * always right. The tree's does not: it is stored, and until this existed it was only ever + * recomputed when folders loaded — so a Back that changed the location without reloading folders + * left the tree showing nothing selected while the listing showed that folder's contents. + * + * Matches on the node's own path rather than its key, because the key encodes tree position while + * the path is what the URL carries. + */ +export const findNodeByPath = ( + nodes: DotFolderTreeNodeItem[] | undefined, + path: string +): DotFolderTreeNodeItem | undefined => { + for (const node of nodes ?? []) { + if (node.data?.path === path) { + return node; + } + + const found = findNodeByPath(node.children as DotFolderTreeNodeItem[] | undefined, path); + + if (found) { + return found; + } + } + + return undefined; +}; diff --git a/core-web/libs/ui/src/index.ts b/core-web/libs/ui/src/index.ts index 5b0a3a58c4b7..a5b6eb905a9a 100644 --- a/core-web/libs/ui/src/index.ts +++ b/core-web/libs/ui/src/index.ts @@ -82,6 +82,7 @@ export * from './lib/components/dot-folder-list-view/constants'; export { DotSiteComponent } from './lib/components/dot-site/dot-site.component'; export * from './lib/components/dot-theme/dot-theme.component'; +export * from './lib/components/dot-status-toast/dot-status-toast.component'; export * from './lib/components/dot-toast/dot-toast.component'; export * from './lib/components/dot-upload-button/dot-upload-button.component'; export * from './lib/components/dot-upload-dropzone/dot-upload-dropzone.component'; diff --git a/core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.html b/core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.html index 39668b59ef4d..c672ea740f92 100644 --- a/core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.html +++ b/core-web/libs/ui/src/lib/components/dot-folder-list-view/dot-folder-list-view.component.html @@ -227,11 +227,12 @@ column, and only shared rows render it. The wording went through the full sentence (crowded the row), "Shared" alone (reads as shared with other USERS) and "All Sites" (unambiguous - about scope, but never says what it is describing). "Shared Asset" is the - term the toolbar's own filter uses -- "Show Shared Assets" sits directly - above this table -- so the label is defined by the control the user just - operated, which disambiguates it better than any rewording can. The full - sentence stays on hover. --> + about scope, but never says what it is describing). What settled it is + that the label should name whatever the toolbar's own filter names, since + that control sits directly above this table and is what the user just + operated. That filter is now "Show System Host", and the sidebar carries + a System Host entry beside it, so this follows both rather than drifting + into a third word for one thing. The full sentence stays on hover. --> + + + + @if (message.icon) { + + + } @else { + + } + + +
+ +
+
+ + diff --git a/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.scss b/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.scss new file mode 100644 index 000000000000..f9d152fbb28b --- /dev/null +++ b/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.scss @@ -0,0 +1,76 @@ +:host { + // PrimeNG ships unlayered, so its own rules outrank Tailwind utilities. These have to be + // written as CSS against its class names rather than as classes in the template. + // + // Values are set directly rather than as `var(--p-toast-*, fallback)`: the theme DEFINES those + // variables, so a fallback never applies and the rule silently has no effect. That is why the + // summary kept rendering at the theme's 14px while this file asked for something smaller. + ::ng-deep { + // Nothing here is clickable, so nothing here may take a click. + // + // The outlet is a fixed-position box at the bottom centre of the viewport, which is exactly + // where the listing's paginator is. Without this it sits over the page controls and + // swallows the click: the page never changes, and the listing is left showing one page + // while the paginator believes it is on another. + // + // Safe to blank the whole subtree rather than re-enabling it on the message, because this + // outlet has no controls at all -- the close button was removed, and a status is not + // something the reader acts on. It also means no hover-to-pause, which this toast does not + // want either: it is sticky and ends when the run it reports ends. + .p-toast, + .p-toast-message { + pointer-events: none; + } + + // Sized to its content, not to a fixed 350px — a two-word outcome should not sit in a slab. + // It grows with the label instead of clipping it, up to a ceiling where it wraps, because + // what a run has to say is not the same length every time. + .p-toast-message { + width: fit-content; + max-width: min(28rem, calc(100vw - 2rem)); + margin-left: auto; + margin-right: auto; + } + + .p-toast-message-content { + align-items: center; + gap: 0.625rem; + padding: 0.625rem 0.75rem; + } + + // `min-width: 0` is what lets the text shrink inside the flex row. Without it the row + // refuses to give ground and the label overflows its own box — which read as the first + // characters being cut off rather than as the box being too small. + .p-toast-message-text { + flex: 0 1 auto; + min-width: 0; + margin: 0; + } + + .p-toast-summary { + font-size: 0.8125rem; + font-weight: 500; + line-height: 1.3; + // Wraps rather than truncates. The line is short by design, but a caller can still + // hand it more than fits, and a status cut off mid-word says less than no status. + overflow-wrap: anywhere; + } + + // PrimeNG renders its own close button unless the MESSAGE carries `closable: false` + // (its template reads `message?.closable !== false`; there is no input on the component to + // set it from here). Callers are told to pass it, and this makes the outlet safe when one + // forgets: a status is never something the reader has to dismiss, so the control must not + // appear whatever was raised. `display: none` also takes it out of the tab order and the + // accessibility tree, rather than leaving a control that can be reached but not seen. + .p-toast-close-button { + display: none; + } + + .p-toast-message-icon { + display: inline-flex; + align-items: center; + font-size: 1rem; + flex: 0 0 auto; + } + } +} diff --git a/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.spec.ts b/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.spec.ts new file mode 100644 index 000000000000..fadef9cfdc7c --- /dev/null +++ b/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.spec.ts @@ -0,0 +1,100 @@ +import { Spectator, createComponentFactory } from '@openng/spectator/vitest'; + +import { MessageService } from 'primeng/api'; + +import { DotStatusToastComponent, STATUS_TOAST_KEY } from './dot-status-toast.component'; + +describe('DotStatusToastComponent', () => { + let spectator: Spectator; + let messageService: MessageService; + + const createComponent = createComponentFactory({ + component: DotStatusToastComponent, + providers: [MessageService] + }); + + const raise = (message: Parameters[0]) => { + messageService.add({ key: STATUS_TOAST_KEY, ...message }); + spectator.detectChanges(); + }; + + const box = () => spectator.query('.p-toast-message'); + + beforeEach(() => { + spectator = createComponent(); + messageService = spectator.inject(MessageService); + }); + + it('should show the outcome and nothing else', () => { + // The whole point of this outlet: one short line. An outcome that needs a paragraph is a + // report, and reports belong in dot-toast, which is sized for them. + raise({ severity: 'success', summary: 'Uploaded' }); + + expect(spectator.query('[data-testid="status-toast-summary"]')?.textContent?.trim()).toBe( + 'Uploaded' + ); + }); + + it('should carry the severity so the theme can colour it', () => { + // Colour comes from the severity through the dotCMS PrimeNG preset — never hardcoded, and + // never the dark pill from the prototype, which is not a pattern in this system. + raise({ severity: 'success', summary: 'Uploaded' }); + + expect(box()?.getAttribute('data-pc-severity') ?? box()?.className).toContain('success'); + }); + + it('should render nothing a caller can close by hand', () => { + // A status is not a message the reader has to deal with. It reports something already + // under way and goes when that finishes, so a close button asks the reader to tidy up + // after a thing they did not start and cannot affect -- and the row of controls it sat in + // was most of what made this feel like a panel rather than a status. + raise({ severity: 'success', summary: 'Uploaded' }); + + expect(spectator.query('[data-testid="status-toast-close"]')).toBeNull(); + }); + + it('should ignore a detail line rather than growing to fit it', () => { + // A caller that passes one is using the wrong outlet. Dropping it keeps this toast the + // size it promises to be instead of quietly turning into the wide one. + raise({ severity: 'success', summary: 'Uploaded', detail: 'a paragraph nobody asked for' }); + + expect(spectator.query('[data-testid="status-toast-summary"]')?.textContent?.trim()).toBe( + 'Uploaded' + ); + expect(spectator.fixture.nativeElement.textContent).not.toContain('a paragraph'); + }); + + it('should show a spinner while a run is still going', () => { + // The in-flight half of the same story, so "Uploading…" and "Uploaded" are one surface + // rather than an indicator in the toolbar and a toast somewhere else. + raise({ severity: 'secondary', summary: 'Uploading…', icon: 'pi pi-spin pi-spinner' }); + + expect(spectator.query('.pi-spinner')).toBeTruthy(); + }); + + it('should size itself to its text rather than to a fixed width', () => { + // The complaint this exists to answer: one short word sat in a 350px slab. Asserted as a + // rule rather than a pixel count, which would pin the font metrics of whatever runs it. + raise({ severity: 'success', summary: 'Uploaded' }); + + expect(getComputedStyle(box() as Element).width).not.toBe('350px'); + }); + + it('should ignore messages that are not addressed to it', () => { + // A portlet provides ONE MessageService, and an outlet with no key renders every message + // on it — which showed each status twice, wide at the top and compact at the bottom. + messageService.add({ severity: 'success', summary: 'for the other outlet' }); + spectator.detectChanges(); + + expect(spectator.query('[data-testid="status-toast-summary"]')).toBeNull(); + }); + + it('should keep the markup the run label carries', () => { + // The toolbar's label bolds the action and its target. Interpolating would print the tags. + raise({ severity: 'info', summary: 'Applying Upload to demo.dotcms.com' }); + + expect(spectator.query('[data-testid="status-toast-summary"] b')?.textContent).toBe( + 'Upload' + ); + }); +}); diff --git a/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.ts b/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.ts new file mode 100644 index 000000000000..53bed60ac183 --- /dev/null +++ b/core-web/libs/ui/src/lib/components/dot-status-toast/dot-status-toast.component.ts @@ -0,0 +1,48 @@ +import { ChangeDetectionStrategy, Component, input } from '@angular/core'; + +import { ToastModule } from 'primeng/toast'; +import type { ToastPositionType } from 'primeng/types/toast'; + +import { DotSeverityIconComponent } from '../dot-severity-icon/dot-severity-icon.component'; + +/** The key this outlet claims by default, so callers and the template cannot drift apart. */ +export const STATUS_TOAST_KEY = 'dot-status'; + +/** + * Toast outlet for a run's status: an icon, one short line, and a dismiss. + * + * Separate from {@link DotToastComponent} because the two carry different things, not because the + * styling differs. That one is a report — a title plus a detail paragraph, often several lines, + * frequently carrying markup — and it is 350px wide because it has to be. This one states an + * outcome in a couple of words, so it is sized to its text and drops `detail` entirely rather than + * growing to fit something a caller should not have sent here. + * + * It offers no way to dismiss it either. A status reports something already under way and clears + * when whoever raised it says the work is done, so a close button would ask the reader to tidy up + * after a thing they did not start and cannot affect. + * + * Colour comes from Lara through the dotCMS preset, keyed on the message severity. The prototype + * this follows used a dark pill; that is deliberately not reproduced, because a black surface is + * not a pattern in this design system. + */ +@Component({ + selector: 'dot-status-toast', + imports: [ToastModule, DotSeverityIconComponent], + templateUrl: './dot-status-toast.component.html', + styleUrl: './dot-status-toast.component.scss', + changeDetection: ChangeDetectionStrategy.OnPush +}) +export class DotStatusToastComponent { + /** Where the stack renders, mirroring `p-toast`'s own positions. */ + $position = input('bottom-center', { alias: 'position' }); + + /** + * Which messages this outlet claims. + * + * Not optional in practice: a portlet provides ONE `MessageService`, and an outlet with no key + * renders every message on it — so sharing a shell with `dot-toast` showed each status twice, + * once wide at the top and once compact at the bottom. Callers add with this key to reach this + * outlet and no other. + */ + $key = input(STATUS_TOAST_KEY, { alias: 'key' }); +} diff --git a/core-web/libs/ui/src/lib/theme/theme.config.ts b/core-web/libs/ui/src/lib/theme/theme.config.ts index f481aa375178..080114dc29a8 100644 --- a/core-web/libs/ui/src/lib/theme/theme.config.ts +++ b/core-web/libs/ui/src/lib/theme/theme.config.ts @@ -226,6 +226,16 @@ export const CustomLaraPreset = definePreset(Lara, { } } }, + toast: { + root: { + // Lara draws a 6px severity-coloured stripe down the left edge + // (its own default is '0 0 0 6px'). Every toast in the app is a plain + // rectangle instead: severity is already carried by the background, the + // icon and the text colour, and the stripe only adds a second, louder + // way to say the same thing. + borderWidth: '0' + } + }, toolbar: { root: { borderRadius: '0', diff --git a/docs/frontend/KEYBOARD_SHORTCUTS.md b/docs/frontend/KEYBOARD_SHORTCUTS.md index f841d6f75a16..476b90cb70d3 100644 --- a/docs/frontend/KEYBOARD_SHORTCUTS.md +++ b/docs/frontend/KEYBOARD_SHORTCUTS.md @@ -12,7 +12,7 @@ which cannot arbitrate between two surfaces that want the same key. |---|---|---| | `/` | Focus the search field | `dot-content-drive-search-input`, `dot-asset-picker-toolbar` | | `Mod + K` | Focus the search field (alias for `/`) | as above | -| `Mod + B` | Show or hide the folder tree | `dot-content-drive-shell` | +| `Mod + B` | Show or hide the sidebar (the folder tree and the entries around it) | `dot-content-drive-shell` | | `Escape` | Clear the selection | `dot-content-drive-shell` | | `Escape` | Close the content side panel (wins while open) | `dot-edit-content-side-panel` | | `↑` `↓` | Move focus between rows | the shared listing | diff --git a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java index 18eddfa91d46..8832199f5f53 100644 --- a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java @@ -729,7 +729,16 @@ private String buildPureESQuery(final BrowserQuery browserQuery) { ? browserQuery.site.getIdentifier() : browserQuery.folder.getHostId(); - if (browserQuery.forceSystemHost || browserQuery.folder.isSystemFolder()) { + // The caller's request decides this, and nothing else. This used to widen the clause + // whenever the folder happened to be the system folder — that is, at every site root — + // which made this builder answer differently from the SQL one about a structural + // criterion. ADR-0018 makes the database authoritative for exactly those criteria and + // forbids re-routing them to the index, so the two must name the same hosts for the same + // request. The divergence only ever surfaced under the non-default PURE_ES heuristic, + // which is why it went unnoticed rather than why it was acceptable. + if (SystemHostMode.ONLY == browserQuery.systemHostMode) { + query.append("+conhost:SYSTEM_HOST "); + } else if (SystemHostMode.INCLUDE == browserQuery.systemHostMode) { query.append("+(conhost:").append(hostId).append(" OR conhost:SYSTEM_HOST) "); } else { query.append("+conhost:").append(hostId).append(" "); @@ -2206,8 +2215,8 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) { if (shouldApplySiteFiltering) { if (browserQuery.site != null) { appendSiteQuery(candidatesPredicates, browserQuery.site.getIdentifier(), - browserQuery.forceSystemHost, parameters); - } else if (browserQuery.forceSystemHost) { + browserQuery.systemHostMode, parameters); + } else if (SystemHostMode.EXCLUDE != browserQuery.systemHostMode) { appendSystemHostQuery(candidatesPredicates); } } @@ -2243,9 +2252,11 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) { if (shouldApplySiteFiltering) { if (browserQuery.site != null) { appendSiteQuery(selectQuery, browserQuery.site.getIdentifier(), - browserQuery.forceSystemHost, parameters); + browserQuery.systemHostMode, parameters); } else { - if (browserQuery.forceSystemHost) { + // No site to narrow to, so the only host clause worth emitting is the + // System Host one, which both INCLUDE and ONLY want here. + if (SystemHostMode.EXCLUDE != browserQuery.systemHostMode) { appendSystemHostQuery(selectQuery); } } @@ -2416,9 +2427,14 @@ private void appendLanguageQuery(StringBuilder sqlQuery, Set languageIds, * @param siteIdentifier The site identifier to filter by. * @param parameters The list of parameters to add the site identifier to. */ - private void appendSiteQuery(StringBuilder sqlQuery, String siteIdentifier, boolean forceSystemHost, - List parameters) { - if(forceSystemHost){ + private void appendSiteQuery(StringBuilder sqlQuery, String siteIdentifier, + SystemHostMode systemHostMode, List parameters) { + if (SystemHostMode.ONLY == systemHostMode) { + // The site is context rather than a filter here, so nothing is bound. + appendSystemHostQuery(sqlQuery); + return; + } + if (SystemHostMode.INCLUDE == systemHostMode) { sqlQuery.append(" and (id.host_inode = ? or id.host_inode = 'SYSTEM_HOST') "); } else { sqlQuery.append(" and (id.host_inode = ?) "); diff --git a/dotCMS/src/main/java/com/dotcms/browser/BrowserQuery.java b/dotCMS/src/main/java/com/dotcms/browser/BrowserQuery.java index eaf909a60ede..217dca3f269e 100644 --- a/dotCMS/src/main/java/com/dotcms/browser/BrowserQuery.java +++ b/dotCMS/src/main/java/com/dotcms/browser/BrowserQuery.java @@ -70,7 +70,7 @@ public class BrowserQuery { final Set contentTypeIds; final Set excludedContentTypeIds; final Host site; - final boolean forceSystemHost; + final SystemHostMode systemHostMode; final boolean skipFolder; final boolean ignoreSiteForFolders; final Folder folder; @@ -130,7 +130,7 @@ public String toString() { ", contentCursor=" + contentCursor + ", folderCursor=" + folderCursor + ", linkCursor=" + linkCursor + " ,site:" + site + ", folder:" + folder + ", filter:" - + filter + ", sortBy:" + sortBy + ", forceSystemHost:" + forceSystemHost + + filter + ", sortBy:" + sortBy + ", systemHostMode:" + systemHostMode + ", skipFolder:" + skipFolder + ", ignoreSiteForFolders:" + ignoreSiteForFolders + ", offset:" + offset + ", maxResults:" + maxResults + ", showWorking:" + showWorking + ", showArchived:" @@ -200,8 +200,9 @@ private BrowserQuery(final Builder builder) { this.showMenuItemsOnly = builder.showMenuItemsOnly; this.site = siteAndFolder._1; this.folder = siteAndFolder._2; - //Despite the site and folder passed, forceSystemHost makes the inclusion of SYSTEM_HOME in the query - this.forceSystemHost = builder.forceSystemHost; + //Despite the site and folder passed, this decides whether SYSTEM_HOST content joins the + //results, is kept out of them, or is the only thing in them. + this.systemHostMode = builder.systemHostMode; this.directParent = this.folder.isSystemFolder() ? site : folder; this.roles= Try.of(()->APILocator.getRoleAPI().loadRolesForUser(user.getUserId()).toArray(new Role[0])).getOrElse(new Role[0]); } @@ -312,7 +313,7 @@ public static final class Builder { private final StringBuilder luceneQuery = new StringBuilder(); private final Set baseTypes = new HashSet<>(); private String hostFolderId = FolderAPI.SYSTEM_FOLDER; - private boolean forceSystemHost = false; + private SystemHostMode systemHostMode = SystemHostMode.EXCLUDE; private boolean skipFolder = false; private boolean ignoreSiteForFolders = false; private String hostIdSystemFolder = null; @@ -359,7 +360,7 @@ private Builder(BrowserQuery browserQuery) { ? browserQuery.site.getIdentifier() : browserQuery.folder.getInode(); this.useElasticsearchFiltering = browserQuery.useElasticsearchFiltering; - this.forceSystemHost = browserQuery.forceSystemHost; + this.systemHostMode = browserQuery.systemHostMode; this.skipFolder = browserQuery.skipFolder; this.ignoreSiteForFolders = browserQuery.ignoreSiteForFolders; this.filter = browserQuery.filter; @@ -440,12 +441,18 @@ public Builder withHostOrFolderId(@Nonnull String hostFolderId) { } /** - * When set, search includes items that belong to system-host - * @param forceSystemHost - * @return + * What the search does about System Host content: keeps it out, admits it alongside the + * named site, or returns nothing else. + *

+ * Replaces a boolean that could only say the first two. Left unset it is + * {@link SystemHostMode#EXCLUDE}, which is what the boolean {@code false} meant, so a + * caller that never mentions System Host is unaffected. + * + * @param systemHostMode how System Host content is treated, never null + * @return this builder */ - public Builder forceSystemHost(boolean forceSystemHost) { - this.forceSystemHost = forceSystemHost; + public Builder systemHostMode(@Nonnull SystemHostMode systemHostMode) { + this.systemHostMode = systemHostMode; return this; } diff --git a/dotCMS/src/main/java/com/dotcms/browser/SystemHostMode.java b/dotCMS/src/main/java/com/dotcms/browser/SystemHostMode.java new file mode 100644 index 000000000000..96dba3f34fc6 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/browser/SystemHostMode.java @@ -0,0 +1,26 @@ +package com.dotcms.browser; + +/** + * What a listing does about System Host content. + * + *

Three states rather than a flag, because the host predicate has three shapes and a boolean + * can only name two of them. {@link #ONLY} is the one it could never say, and it is what lets + * Content Drive browse shared content on its own.

+ * + *

{@link #EXCLUDE} is the default, and that is load-bearing. It reproduces exactly what + * the boolean {@code false} produced before this existed. Several callers reach this builder + * without ever mentioning System Host — the assets API, the older file browser and its deprecated + * tree endpoint, the legacy admin browser, a Velocity viewtool, and two internal callers — so any + * other default would silently change what all of them return.

+ */ +public enum SystemHostMode { + + /** The named site only. What a caller that says nothing about System Host gets. */ + EXCLUDE, + + /** The named site, plus System Host alongside it. */ + INCLUDE, + + /** System Host alone. The named site becomes context rather than a filter. */ + ONLY +} diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/AbstractDriveRequestForm.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/AbstractDriveRequestForm.java index 8cd8e4004f4e..bca0cca0ec25 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/AbstractDriveRequestForm.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/AbstractDriveRequestForm.java @@ -1,6 +1,10 @@ package com.dotcms.rest.api.v1.drive; +import com.dotcms.rest.exception.BadRequestException; import com.dotmarketing.business.APILocator; +import com.dotmarketing.util.HostUtil; +import com.liferay.util.StringPool; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; @@ -72,6 +76,7 @@ @Value.Immutable @JsonSerialize(as = DriveRequestForm.class) @JsonDeserialize(as = DriveRequestForm.class) +@JsonIgnoreProperties(ignoreUnknown = true) public interface AbstractDriveRequestForm { /** @@ -112,6 +117,71 @@ public interface AbstractDriveRequestForm { @Value.Default default boolean includeSystemHost(){return true;} + /** + * Which slice of content to list: the whole site, the site root alone, or System Host. + *

+ * Omitting it means today's behavior, exactly. It carries no default on purpose: at the + * site root an omitted scope and {@link BrowseScope#ALL} agree, but inside a folder they do + * not, and defaulting the field would silently turn every existing folder request recursive. + * That guarantee is what leaves the Asset Picker, which calls this same endpoint, untouched. + *

+ *

+ * Only valid with a site-root {@link #assetPath()}. The three scopes are things you can only + * be in at the top of a site; a folder is addressed by its path, so a scope named alongside a + * folder path is two contradictory statements and is refused rather than resolved. + *

+ *

+ * {@link #includeSystemHost()} is read only when this is {@code ALL} or omitted — the other + * two scopes already answer the System Host question themselves. + *

+ * + * @return the requested browse scope, or null for today's behavior + */ + @Nullable + @JsonProperty("browseScope") + BrowseScope browseScope(); + + /** + * A browse scope is only meaningful at the site root, so one named alongside a folder path is + * refused. + *

+ * Refused rather than resolved by precedence. A caller that names both has said two + * contradictory things, and honouring either one would quietly list somewhere they did not + * ask for. This follows {@code BulkUploadForm}, which refuses a submission naming both a + * folder and a site for the same reason. Ignoring the scope instead would be kinder to a + * sloppy caller and would hide the caller's bug. + *

+ *

+ * The path is split here rather than resolved: this asks only whether anything follows the + * site, which needs no site lookup and no database. Resolving the path is + * {@code AssetPathResolver}'s job and happens later, once the request is known to be coherent. + *

+ */ + @Value.Check + default void checkBrowseScopeIsAtTheSiteRoot() { + if (null == browseScope()) { + return; + } + final String pathWithinSite = pathWithinSite(assetPath()); + if (!pathWithinSite.isEmpty() && !StringPool.FORWARD_SLASH.equals(pathWithinSite)) { + throw new BadRequestException(String.format( + "browseScope is only valid at the site root; got '%s' with path '%s'", + browseScope().name(), pathWithinSite)); + } + } + + /** + * The portion of {@code //site/some/path/} that follows the site, or an empty string when the + * path names a site and nothing else. + */ + static String pathWithinSite(final String assetPath) { + final String withoutHostIndicator = assetPath.startsWith(HostUtil.HOST_INDICATOR) + ? assetPath.substring(HostUtil.HOST_INDICATOR.length()) + : assetPath; + final int firstSlash = withoutHostIndicator.indexOf(StringPool.FORWARD_SLASH); + return firstSlash < 0 ? StringPool.BLANK : withoutHostIndicator.substring(firstSlash); + } + /** * List of language identifiers to include in the search. *

diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/BrowseScope.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/BrowseScope.java new file mode 100644 index 000000000000..03d8662759c9 --- /dev/null +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/BrowseScope.java @@ -0,0 +1,26 @@ +package com.dotcms.rest.api.v1.drive; + +/** + * Which slice of content a Content Drive listing is asked for. + * + *

Named in full — browse scope — because Content Drive separately carries a + * search scope that says which fields a text search reads. The two are independent: this + * one says where you are, the other says how a search reads what is there.

+ * + *

Absent is not a value here, and that is deliberate. A request that omits the browse + * scope keeps meaning exactly what it means today, which is what leaves the Asset Picker and every + * other caller of this endpoint untouched. In particular {@link #ALL} is not the default: it is + * meaningful only at the site root, and an omitted scope inside a folder is not the same thing as + * {@code ALL} inside a folder.

+ */ +public enum BrowseScope { + + /** The whole current site, at any depth. What the site root returns today. */ + ALL, + + /** Only the items that sit at the site root, not inside any folder. */ + ROOT, + + /** System Host content only. The site the request names is context, not a filter. */ + SYSTEM_HOST +} diff --git a/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/ContentDriveHelper.java b/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/ContentDriveHelper.java index e4518dfc03a1..aa34ad78dbfa 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/ContentDriveHelper.java +++ b/dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/ContentDriveHelper.java @@ -5,6 +5,7 @@ import com.dotcms.browser.BrowserQuery; import com.dotcms.browser.BrowserQuery.Builder; import com.dotcms.browser.ContentStatus; +import com.dotcms.browser.SystemHostMode; import com.dotcms.browser.FieldSearchCriteria; import com.dotcms.rest.exception.BadRequestException; import com.dotcms.contenttype.business.ContentTypeAPI; @@ -162,11 +163,12 @@ public PaginatedContents driveSearch(final DriveRequestForm requestForm, final U .sortByDesc(sortDesc); // Determine if we're requesting from a specific folder or host root - if (folder.isSystemFolder()) { + final boolean atSiteRoot = folder.isSystemFolder(); + if (atSiteRoot) { builder.withHostOrFolderId(host.getIdentifier()) - /// if we're setting a site-name directly, we care fore all subfolders - /// Therefore, we should skip setting a folder path - .skipFolder(true); + /// Whether the folder path is applied is what separates "the whole site" from + /// "what sits at its root"; the system folder's path is already "/". + .skipFolder(skipsFolderConstraint(requestForm.browseScope(), atSiteRoot)); } else { builder.withHostOrFolderId(folder.getInode()) // When a specific folder is selected, enable ignoreSiteForFolders to allow @@ -174,7 +176,8 @@ public PaginatedContents driveSearch(final DriveRequestForm requestForm, final U .ignoreSiteForFolders(true); } //This ensures that despite the site passed systemHost will be included too - builder.forceSystemHost(requestForm.includeSystemHost()); + builder.systemHostMode( + systemHostModeFor(requestForm.browseScope(), requestForm.includeSystemHost())); // Enable Elasticsearch filtering for text search when filter is provided if (null != requestForm.filters() && UtilMethods.isSet(requestForm.filters().text())) { @@ -260,6 +263,52 @@ public PaginatedContents driveSearch(final DriveRequestForm requestForm, final U return browserAPI.getPaginatedContents(browserQuery); } + /** + * Whether the folder constraint is dropped, so the listing spans every depth of the site + * instead of the level it names. + *

+ * A request that names a folder is never recursive, whatever else it says. That is the + * first line of this method and the reason it is written as a decision rather than inlined: + * the Asset Picker addresses folders by path and never names a scope, so making the + * all-site-content scope mean "always skip the folder" would silently turn every one of its + * folder requests into a listing of every descendant. + *

+ * At the site root the folder to constrain by is the system folder, whose path is already + * {@code /}. So dropping the constraint gives the whole site, and keeping it gives what sits + * at the root — which is the entire difference between the two scopes. + * + * @param browseScope the requested scope, or null for today's behavior + * @param atSiteRoot whether the resolved folder is the site's root + */ + static boolean skipsFolderConstraint(final BrowseScope browseScope, final boolean atSiteRoot) { + if (!atSiteRoot) { + return false; + } + return null == browseScope || BrowseScope.ALL == browseScope; + } + + /** + * Which host clause the listing emits. + *

+ * The chip decides only where it has something to decide. The site-root scope excludes System + * Host and the System Host scope returns nothing else, so in both the scope already answers + * the question the chip asks, and a stale chip value carried on the request cannot override + * it. That is also why the chip is offered in one scope and disabled in the rest. + * + * @param browseScope the requested scope, or null for today's behavior + * @param includeSystemHost what the "Show System Host" chip sent + */ + static SystemHostMode systemHostModeFor(final BrowseScope browseScope, + final boolean includeSystemHost) { + if (BrowseScope.SYSTEM_HOST == browseScope) { + return SystemHostMode.ONLY; + } + if (BrowseScope.ROOT == browseScope) { + return SystemHostMode.EXCLUDE; + } + return includeSystemHost ? SystemHostMode.INCLUDE : SystemHostMode.EXCLUDE; + } + /** * if base types include FILEASSET then we pass the respective parameter as true * @param baseTypes base types diff --git a/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties b/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties index 4835a1bb6f99..bca04fa7b700 100644 --- a/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties +++ b/dotCMS/src/main/webapp/WEB-INF/messages/Language.properties @@ -7368,11 +7368,12 @@ content-drive.upload.toast.already-uploaded=These {0} files were already content-drive.upload.toast.already-uploaded-again=This batch was uploaded before, and {0} file(s) have now been uploaded again. This folder holds two copies of each, so delete the ones you do not want. # The handle: the batch is now the server's, and the author is released from it. The only # in-flight fact this flow announces, because it is a rule changing rather than work happening. -content-drive.upload.toast.backgrounded=Uploading in the background -content-drive.upload.toast.backgrounded-detail={0} file(s) are on their way. You can keep working, or leave this page. We will let you know when it is done. # The indicator's words for the server phase. "Applying X to Y" would describe an operation the # author has to wait for, which is the opposite of what they were just told. -content-drive.upload.indicator.background=Uploading {1} file(s) to {0} in the background… +content-drive.upload.indicator=Uploading {0} files… +content-drive.upload.indicator.one=Uploading {0} file… +content-drive.upload.indicator.background=Uploading {0} files in the background… +content-drive.upload.indicator.background.one=Uploading {0} file in the background… # Refused submissions. The endpoint keeps its two ceilings distinguishable by status, so these # stay two sentences: "fewer files" and "smaller files" are different instructions. content-drive.upload.refused.too-large=That batch is over the upload size limit. Nothing was uploaded, so try again with fewer or smaller files. @@ -7384,7 +7385,7 @@ com.dotcms.repackage.javax.portlet.title.content-drive=Content Drive com.dotcms.repackage.javax.portlet.title.usage=Usage com.dotcms.repackage.javax.portlet.title.experiments=Experiments content-drive.feature.state=This feature is currently in -content-drive-dropzone.message.drag-and-drop-header=Drop your files here to upload to the selected folder +content-drive-dropzone.message.drag-and-drop-header=Drop your files here to upload development=development content-drive.add-dotasset-success=Upload Complete content-drive.add-dotasset-success-detail={0} was uploaded as {1} @@ -7419,7 +7420,12 @@ content-drive.toast.download-success-detail=The download has started. content-drive.chip-filter.overflow-label={0} and {1} more content-drive.filters.clear-all=Clear all -content-drive.shared-assets-filter.title=Show Shared Assets +content-drive.shared-assets-filter.title=Show System Host +content-drive.scope-bar.all-site-content.excluded=All Files in site (System Host shared files excluded) +content-drive.scope-bar.all-site-content.included=All Files in site (System Host shared files included) +content-drive.scope-bar.include-system-host=Include System Host: +content-drive.sidebar.all-site-content=All Site Content +content-drive.sidebar.system-host=System Host ## Content Drive - Keyboard shortcuts (labels are the source for the author-facing docs) content-drive.shortcut.search=Focus the search field @@ -7467,8 +7473,6 @@ content-drive.action-center.section.pushPublish=Push publish content-drive.action-center.section.bundle=Bundle content-drive.action-center.push-publish.no-environment=Choose at least one environment to continue. content-drive.action-center.approximate-count=This action has a condition that is evaluated per item, so it may apply to fewer items than shown. -content-drive.action-center.applying=Applying {0} to {1} item(s)… -content-drive.action-center.applying-item=Applying {0} to {1}… content-drive.action-center.applying-many={0} operations running… content-drive.list-view.row-busy=An action is running on this item… content-drive.action-center.busy=Wait for the running action to finish before starting another. @@ -7506,8 +7510,8 @@ notification.bulkupload.duplicate=This batch was uploaded before. {1} file(s) we notification.bulkupload.duplicate.dotasset=This batch was uploaded before, and {0} file(s) have now been uploaded again. The folder holds two copies of each, so delete the ones you do not want. content-drive.action-center.unlock.locked-by-others={0} of these are locked by another user, which may require administrator permission to unlock. Any that can't be unlocked will be reported. content-drive.list-view.locked-by-another-user=Locked by another user -content-drive.list-view.shared-asset=Shared across all sites -content-drive.list-view.shared-asset.label=Shared Asset +content-drive.list-view.shared-asset=Lives on System Host, shared across every site +content-drive.list-view.shared-asset.label=System Host content-drive.action-center.toast.error=Action failed content-drive.action-center.toast.error-detail=Something went wrong. No changes were applied. diff --git a/dotCMS/src/test/java/com/dotcms/browser/BrowserQueryHostClauseTest.java b/dotCMS/src/test/java/com/dotcms/browser/BrowserQueryHostClauseTest.java new file mode 100644 index 000000000000..245e36f3f2ff --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/browser/BrowserQueryHostClauseTest.java @@ -0,0 +1,171 @@ +package com.dotcms.browser; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.dotcms.contenttype.model.type.BaseContentType; +import com.dotmarketing.beans.Host; +import com.dotmarketing.portlets.folders.model.Folder; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.List; +import java.util.Set; +import org.junit.Test; + +/** + * The host clause a listing emits when the caller says nothing about System Host. + * + *

This is a characterization test, and it passes the day it is written. That is the + * point of it. Content Drive is gaining a browse scope that needs the System-Host-only clause, + * which means replacing {@code forceSystemHost} with a three-state. Seven callers reach this same + * builder and none of them sets that flag: the assets API, the older file browser and its + * deprecated tree endpoint, the legacy admin browser, a Velocity viewtool, and two internal + * callers in the file-asset and folder factories. If the three-state's default emits anything + * other than the clause asserted below, every one of them silently changes what it returns.

+ * + *

A regression guard that failed first would be guarding something else. This one is committed + * before the refactor and must stay green through it; the assertions below are deliberately about + * the emitted SQL rather than about how the builder is invoked, so the refactor should not need + * to touch this file at all. If it does, that is the signal to look harder.

+ * + *

Exercised through the real SQL-building method by reflection, with no {@code APILocator} + * bootstrap and no database, following {@link BrowserAPIMimeTypeQueryTest}.

+ * + *

Amended once, and only in how it asks. The folder predicate moved into a materialized + * {@code candidates} CTE, which binds ahead of the host clause, so the site identifier is no + * longer the first bound value. The statement and its parameter list still line up + * ({@code and id.parent_path=? and (id.host_inode = ?)} against {@code [/, ]}), so the + * guarantee is unchanged and is now asserted without an index. Nothing about which rows a caller + * gets moved.

+ */ +public class BrowserQueryHostClauseTest { + + private static final String SITE_ID = "48190c8c-42c4-46af-8d1a-0cd5db894797"; + + /** + * Given a query that names a site and says nothing about System Host — the shape every caller + * outside Content Drive builds — When the statement is built, Then it is scoped to that site + * alone: one bound identifier, and no System Host literal anywhere in the text. + */ + @Test + public void testAQueryThatSaysNothingAboutSystemHostIsScopedToItsSiteAlone() throws Exception { + final SelectResult result = buildSelect(); + + assertTrue("the statement must filter on the site's identifier", + result.sql.contains("id.host_inode = ?")); + assertFalse("a caller that asked for nothing must not be given System Host content", + result.sql.contains("SYSTEM_HOST")); + assertTrue("the site identifier is bound", + result.params.stream().map(String::valueOf).anyMatch(SITE_ID::equals)); + assertFalse("the site identifier is bound, never concatenated into the text", + result.sql.contains(SITE_ID)); + } + + /** + * The same guarantee stated the other way round, because the failure this protects against is + * a widened clause rather than a missing one: the site predicate must not be OR'd with + * anything. An {@code OR} in this fragment is how "only this site" becomes "this site plus + * everything shared", which is exactly the silent change being guarded. + */ + @Test + public void testTheSitePredicateIsNotWidenedByAnOr() throws Exception { + final SelectResult result = buildSelect(); + + final int hostClauseStart = result.sql.indexOf("id.host_inode"); + assertTrue("the host predicate must be present to be checked", hostClauseStart >= 0); + + final String hostClause = result.sql.substring(hostClauseStart, + result.sql.indexOf(')', hostClauseStart) + 1); + assertFalse("the host predicate must stand alone: " + hostClause, + hostClause.toUpperCase().contains(" OR ")); + } + + /** + * The other half of the same guarantee: when the caller asks for shared content — which is + * what the "Show System Host" chip sends, on by default — the site predicate widens to admit + * System Host as well. + *

+ * Untested anywhere before this. The refactor that gives this flag a third state passes + * through here, so without this a broken "chip on" would ship in silence: shared assets would + * simply stop appearing, with every existing test still green. + */ + @Test + public void testAskingForSharedContentWidensThePredicateToAdmitSystemHost() throws Exception { + final SelectResult result = buildSelect(true); + + assertTrue("the site is still matched", result.sql.contains("id.host_inode = ?")); + assertTrue("and System Host is admitted alongside it", + result.sql.contains("id.host_inode = 'SYSTEM_HOST'")); + assertTrue("the site identifier is still bound rather than widened away", + result.params.stream().map(String::valueOf).anyMatch(SITE_ID::equals)); + assertFalse("and System Host is a literal in the text, never a second binding", + result.sql.contains(SITE_ID)); + } + + /** + * Builds the statement for a query that names a site and a folder and nothing else, which is + * the default shape of every non-Content-Drive caller. + */ + private static SelectResult buildSelect() throws Exception { + return buildSelect(false); + } + + /** + * @param askForSharedContent what the caller says about System Host. This is the one line the + * three-state refactor changes in this file; the assertions above it must not move. + */ + private static SelectResult buildSelect(final boolean askForSharedContent) throws Exception { + final BrowserAPIImpl api = mock(BrowserAPIImpl.class, CALLS_REAL_METHODS); + final BrowserQuery query = mock(BrowserQuery.class, CALLS_REAL_METHODS); + + for (final String emptyCollection : List.of("languageIds", "contentTypeIds", + "excludedContentTypeIds", "workflowSchemeIds", "workflowStepIds", + "contentStatuses")) { + setField(query, emptyCollection, Set.of()); + } + setField(query, "baseTypes", Set.of(BaseContentType.ANY)); + setField(query, "fieldCriteria", List.of()); + setField(query, "mimeTypes", List.of()); + setField(query, "systemHostMode", + askForSharedContent ? SystemHostMode.INCLUDE : SystemHostMode.EXCLUDE); + + // Both are mocked rather than constructed: `new Host()` resolves its content type through + // the legacy cache, which calls APILocator.systemUser() and reaches for a database + // connection. Only two accessors are read while the statement is built. + final Host site = mock(Host.class); + when(site.getIdentifier()).thenReturn(SITE_ID); + setField(query, "site", site); + + final Folder folder = mock(Folder.class); + when(folder.getPath()).thenReturn("/"); + setField(query, "folder", folder); + + final Method selectQuery = + BrowserAPIImpl.class.getDeclaredMethod("selectQuery", BrowserQuery.class); + selectQuery.setAccessible(true); + final BrowserAPIImpl.SelectQuery built = + (BrowserAPIImpl.SelectQuery) selectQuery.invoke(api, query); + + return new SelectResult(built.selectQuery, built.params); + } + + private static void setField(final BrowserQuery query, final String name, final Object value) + throws Exception { + final Field field = BrowserQuery.class.getDeclaredField(name); + field.setAccessible(true); + field.set(query, value); + } + + private static class SelectResult { + private final String sql; + private final List params; + + private SelectResult(final String sql, final List params) { + this.sql = sql; + this.params = params; + } + } +} diff --git a/dotCMS/src/test/java/com/dotcms/browser/BrowserQueryIndexHostClauseTest.java b/dotCMS/src/test/java/com/dotcms/browser/BrowserQueryIndexHostClauseTest.java new file mode 100644 index 000000000000..c7413bb0717f --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/browser/BrowserQueryIndexHostClauseTest.java @@ -0,0 +1,179 @@ +package com.dotcms.browser; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.CALLS_REAL_METHODS; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.dotmarketing.beans.Host; +import com.dotmarketing.portlets.folders.model.Folder; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.util.List; +import java.util.Set; +import org.junit.Test; + +/** + * The host clause the index query carries, and that it agrees with the one the SQL + * carries. + * + *

Two builders answer the same question about the same request. ADR-0018 makes the database + * authoritative for the structural criteria — parent folder and site, System Host included — and + * says such criteria "must never be silently re-routed to the index". Two builders disagreeing + * about one of them is the failure that decision exists to prevent, so this is a correctness fix + * rather than tidying.

+ * + *

The disagreement was narrow and easy to miss: the index builder widened the clause to admit + * System Host whenever the folder happened to be the system folder, no matter what the caller + * asked for. It only surfaces under the non-default {@code PURE_ES} heuristic, which is why it + * has gone unnoticed, and why it is pinned here rather than left to an integration run.

+ * + *

Exercised through the real query-building method by reflection, with no {@code APILocator} + * bootstrap and no database, following {@link BrowserAPIMimeTypeQueryTest}.

+ */ +public class BrowserQueryIndexHostClauseTest { + + private static final String SITE_ID = "48190c8c-42c4-46af-8d1a-0cd5db894797"; + + /** + * Given a caller that says nothing about System Host, When the index query is built at the + * site root, Then it matches that site alone. + *

+ * This is the case that was wrong. The site root resolves to the system folder, and the + * builder used to read that as a reason to admit System Host regardless of the request. + */ + @Test + public void testAtTheSiteRootSayingNothingStillMatchesThatSiteAlone() throws Exception { + final String query = buildIndexQuery(SystemHostMode.EXCLUDE, true); + + assertTrue("the site must be matched", query.contains("+conhost:" + SITE_ID)); + assertFalse("being at the root is not a request for shared content: " + query, + query.contains("SYSTEM_HOST")); + } + + /** + * Given a caller asking for shared content, When the index query is built, Then the clause + * widens to admit System Host alongside the site. + */ + @Test + public void testAskingForSharedContentAdmitsSystemHostAlongsideTheSite() throws Exception { + final String query = buildIndexQuery(SystemHostMode.INCLUDE, true); + + assertTrue("both must be admitted: " + query, + query.contains("+(conhost:" + SITE_ID + " OR conhost:SYSTEM_HOST)")); + } + + /** + * Given a caller asking for System Host alone, When the index query is built, Then the site is + * not matched at all — it is context for the request, not a filter on it. + */ + @Test + public void testAskingForSystemHostAloneDoesNotMatchTheSite() throws Exception { + final String query = buildIndexQuery(SystemHostMode.ONLY, true); + + assertTrue("System Host must be matched", query.contains("+conhost:SYSTEM_HOST")); + assertFalse("the site must not be matched as well: " + query, + query.contains(SITE_ID)); + } + + /** + * Inside a folder the two builders already agreed, and must keep agreeing: the site is matched + * and nothing else is admitted. + */ + @Test + public void testInsideAFolderTheSiteAloneIsMatched() throws Exception { + final String query = buildIndexQuery(SystemHostMode.EXCLUDE, false); + + assertTrue("the site must be matched", query.contains("+conhost:" + SITE_ID)); + assertFalse("nothing else may be admitted: " + query, query.contains("SYSTEM_HOST")); + } + + /** + * The point of the whole exercise, stated directly: for one request, both builders name the + * same hosts. Asserted on the clause each emits rather than on results, because the index + * builder runs under a heuristic that gives up read-your-writes (ADR-0018), so comparing + * returned content would be comparing two different moments. + */ + @Test + public void testBothBuildersNameTheSameHostsForTheSameRequest() throws Exception { + for (final SystemHostMode mode : SystemHostMode.values()) { + final String indexQuery = buildIndexQuery(mode, true); + final String sql = buildSelect(mode); + + final boolean indexAdmitsSystemHost = indexQuery.contains("SYSTEM_HOST"); + final boolean sqlAdmitsSystemHost = sql.contains("SYSTEM_HOST"); + assertEquals(mode + ": the two builders must agree about System Host", + sqlAdmitsSystemHost, indexAdmitsSystemHost); + + final boolean indexMatchesSite = indexQuery.contains(SITE_ID); + // The SQL binds the site as a parameter rather than inlining it, so its equivalent of + // "matches the site" is the presence of the bound predicate. + final boolean sqlMatchesSite = sql.contains("id.host_inode = ?"); + assertEquals(mode + ": the two builders must agree about the site", + sqlMatchesSite, indexMatchesSite); + } + } + + private static String buildIndexQuery(final SystemHostMode mode, final boolean atSiteRoot) + throws Exception { + final BrowserAPIImpl api = mock(BrowserAPIImpl.class, CALLS_REAL_METHODS); + final BrowserQuery query = baseQuery(mode, atSiteRoot); + + final Method method = + BrowserAPIImpl.class.getDeclaredMethod("buildPureESQuery", BrowserQuery.class); + method.setAccessible(true); + + return (String) method.invoke(api, query); + } + + private static String buildSelect(final SystemHostMode mode) throws Exception { + final BrowserAPIImpl api = mock(BrowserAPIImpl.class, CALLS_REAL_METHODS); + final BrowserQuery query = baseQuery(mode, true); + + final Method method = + BrowserAPIImpl.class.getDeclaredMethod("selectQuery", BrowserQuery.class); + method.setAccessible(true); + final BrowserAPIImpl.SelectQuery built = + (BrowserAPIImpl.SelectQuery) method.invoke(api, query); + + return built.selectQuery; + } + + private static BrowserQuery baseQuery(final SystemHostMode mode, final boolean atSiteRoot) + throws Exception { + final BrowserQuery query = mock(BrowserQuery.class, CALLS_REAL_METHODS); + + for (final String emptyCollection : List.of("languageIds", "contentTypeIds", + "excludedContentTypeIds", "workflowSchemeIds", "workflowStepIds", + "contentStatuses")) { + setField(query, emptyCollection, Set.of()); + } + setField(query, "baseTypes", Set.of()); + setField(query, "fieldCriteria", List.of()); + setField(query, "mimeTypes", List.of()); + setField(query, "systemHostMode", mode); + + // Mocked rather than constructed: `new Host()` resolves its content type through the + // legacy cache, which calls APILocator.systemUser() and reaches for a database connection. + final Host site = mock(Host.class); + when(site.getIdentifier()).thenReturn(SITE_ID); + setField(query, "site", site); + + final Folder folder = mock(Folder.class); + when(folder.isSystemFolder()).thenReturn(atSiteRoot); + when(folder.getPath()).thenReturn(atSiteRoot ? "/" : "/application/"); + when(folder.getHostId()).thenReturn(SITE_ID); + setField(query, "folder", folder); + + return query; + } + + private static void setField(final BrowserQuery query, final String name, final Object value) + throws Exception { + final Field field = BrowserQuery.class.getDeclaredField(name); + field.setAccessible(true); + field.set(query, value); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveHelperBrowseScopeTest.java b/dotCMS/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveHelperBrowseScopeTest.java new file mode 100644 index 000000000000..ad382ecbaad2 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveHelperBrowseScopeTest.java @@ -0,0 +1,130 @@ +package com.dotcms.rest.api.v1.drive; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import com.dotcms.browser.SystemHostMode; +import org.junit.Test; + +/** + * What a browse scope decides: whether the folder constraint is applied, and which host clause is + * emitted. + * + *

Both are expressed as pure functions so they can be pinned without resolving a path, which + * needs a site lookup and a database. The listing itself is exercised end to end by the + * integration tests; what is asserted here is the decision, case by case.

+ * + *

The case that matters most is the one about no scope at all inside a folder. The Asset + * Picker calls this same endpoint and addresses folders by path without ever naming a scope. If + * "all" were ever implemented as unconditional folder-skipping, those requests would silently + * start returning every descendant, and nothing else in the suite would notice.

+ */ +public class ContentDriveHelperBrowseScopeTest { + + private static final boolean AT_THE_SITE_ROOT = true; + private static final boolean INSIDE_A_FOLDER = false; + private static final boolean CHIP_ON = true; + private static final boolean CHIP_OFF = false; + + // ---- which folder constraint applies ------------------------------------------------- + + /** + * Given no browse scope, When the request is at the site root, Then the folder constraint is + * dropped — the whole site at any depth, which is what the site root returns today. + */ + @Test + public void testNoScopeAtTheSiteRootStillListsTheWholeSite() { + assertTrue("today's site-root behavior must survive untouched", + ContentDriveHelper.skipsFolderConstraint(null, AT_THE_SITE_ROOT)); + } + + /** + * Given no browse scope, When the request names a folder, Then the folder constraint applies. + * This is the Asset Picker's shape, and the regression this whole design is arranged around. + */ + @Test + public void testNoScopeInsideAFolderStaysInThatFolder() { + assertFalse("a folder request must never become recursive", + ContentDriveHelper.skipsFolderConstraint(null, INSIDE_A_FOLDER)); + } + + /** + * Given the all-site-content scope at the root, Then it decides exactly what no scope decides + * there. Saying it explicitly and leaving it unsaid must not differ. + */ + @Test + public void testAllSiteContentAtTheRootMatchesSayingNothing() { + assertEquals("the explicit spelling must agree with the implicit one", + ContentDriveHelper.skipsFolderConstraint(null, AT_THE_SITE_ROOT), + ContentDriveHelper.skipsFolderConstraint(BrowseScope.ALL, AT_THE_SITE_ROOT)); + } + + /** + * Given the site-root scope, Then the folder constraint applies, which is the whole point of + * it: the system folder's path is already {@code /}, so applying it lists what sits at the + * root rather than everything beneath it. + */ + @Test + public void testTheSiteRootScopeAppliesTheFolderConstraint() { + assertFalse("the root scope must constrain to the root", + ContentDriveHelper.skipsFolderConstraint(BrowseScope.ROOT, AT_THE_SITE_ROOT)); + } + + /** + * Given the System Host scope, Then the folder constraint applies too. System Host has no + * folders, so everything in it sits at its root. + */ + @Test + public void testTheSystemHostScopeAppliesTheFolderConstraint() { + assertFalse("System Host content all sits at its root", + ContentDriveHelper.skipsFolderConstraint(BrowseScope.SYSTEM_HOST, AT_THE_SITE_ROOT)); + } + + // ---- which host clause is emitted ---------------------------------------------------- + + /** + * Given no browse scope, Then the chip decides, exactly as it does today. + */ + @Test + public void testWithNoScopeTheChipDecides() { + assertEquals(SystemHostMode.INCLUDE, ContentDriveHelper.systemHostModeFor(null, CHIP_ON)); + assertEquals(SystemHostMode.EXCLUDE, ContentDriveHelper.systemHostModeFor(null, CHIP_OFF)); + } + + /** + * Given the all-site-content scope, Then the chip still decides. This is the only scope where + * the chip has anything to say, which is why it is the only one where it is offered. + */ + @Test + public void testInAllSiteContentTheChipStillDecides() { + assertEquals(SystemHostMode.INCLUDE, + ContentDriveHelper.systemHostModeFor(BrowseScope.ALL, CHIP_ON)); + assertEquals(SystemHostMode.EXCLUDE, + ContentDriveHelper.systemHostModeFor(BrowseScope.ALL, CHIP_OFF)); + } + + /** + * Given the site-root scope, Then System Host is excluded whatever the chip says. The chip is + * disabled there, but a request can still carry a stale value, and the scope must win. + */ + @Test + public void testTheSiteRootScopeExcludesSystemHostWhateverTheChipSays() { + assertEquals(SystemHostMode.EXCLUDE, + ContentDriveHelper.systemHostModeFor(BrowseScope.ROOT, CHIP_ON)); + assertEquals(SystemHostMode.EXCLUDE, + ContentDriveHelper.systemHostModeFor(BrowseScope.ROOT, CHIP_OFF)); + } + + /** + * Given the System Host scope, Then System Host is the only thing listed, whatever the chip + * says. Same reasoning as the root scope: the scope already answers the question the chip asks. + */ + @Test + public void testTheSystemHostScopeReturnsSystemHostAloneWhateverTheChipSays() { + assertEquals(SystemHostMode.ONLY, + ContentDriveHelper.systemHostModeFor(BrowseScope.SYSTEM_HOST, CHIP_ON)); + assertEquals(SystemHostMode.ONLY, + ContentDriveHelper.systemHostModeFor(BrowseScope.SYSTEM_HOST, CHIP_OFF)); + } +} diff --git a/dotCMS/src/test/java/com/dotcms/rest/api/v1/drive/DriveRequestFormBrowseScopeTest.java b/dotCMS/src/test/java/com/dotcms/rest/api/v1/drive/DriveRequestFormBrowseScopeTest.java new file mode 100644 index 000000000000..db88b901de56 --- /dev/null +++ b/dotCMS/src/test/java/com/dotcms/rest/api/v1/drive/DriveRequestFormBrowseScopeTest.java @@ -0,0 +1,147 @@ +package com.dotcms.rest.api.v1.drive; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertThrows; + +import com.dotcms.rest.api.v1.DotObjectMapperProvider; +import java.util.List; +import javax.ws.rs.WebApplicationException; +import org.junit.Test; + +/** + * The invariant that keeps a browse scope and a folder path from contradicting each other. + * + *

The three browse scopes are things you can only be in at the top of a site: the whole site, + * the site root itself, and System Host. A folder is addressed by its path. So there is no + * combination of a folder path and a scope that means anything, and the request is refused rather + * than resolved by precedence.

+ * + *

Why refused and not resolved. The precedent is {@code BulkUploadForm}, which refuses a + * submission naming both a folder and a site with the same reasoning: a caller that says two + * contradictory things has not expressed a preference, and picking one would silently put content + * somewhere they did not choose. Ignoring the scope instead would be friendlier to a sloppy caller + * and would hide the caller's bug, which is the trade this test pins.

+ * + *

On Red. This test names a field the form does not have yet, so it cannot compile until + * the field is declared. The declaration is not the behavior under test: the behavior is the + * refusal. The honest Red state for this test is therefore "the field exists, the validation does + * not, and the assertion fails" — not a compile error. Confirm it in that state before writing the + * validation.

+ * + *

Needs no database.

+ */ +public class DriveRequestFormBrowseScopeTest { + + private static final String SITE_ROOT = "//demo.dotcms.com/"; + private static final String A_FOLDER = "//demo.dotcms.com/application/"; + + /** + * Every builder here pins {@code language}. Its declared default asks the language API for the + * default language, which reaches the database, so leaving it unset makes the build fail with + * "No Company!" before any validation runs — a failure that looks like a refusal but is only + * the bootstrap. Pinning it keeps the assertions about the browse scope. + */ + private static DriveRequestForm.Builder formFor(final String assetPath) { + return DriveRequestForm.builder().assetPath(assetPath).language(List.of("1")); + } + + /** + * Given a browse scope named together with a folder path, When the form is built, Then it is + * refused, and the message names both halves of the contradiction so the caller can see which + * two statements they made. + */ + @Test + public void testAnExplicitBrowseScopeWithAFolderPathIsRefused() { + for (final BrowseScope scope : BrowseScope.values()) { + // Deliberately not pinned to an exception type: where the refusal is raised is an + // implementation choice, and a test that names the type would decide it here. What is + // asserted is that the request does not succeed and that the reason is legible. + final RuntimeException refused = assertThrows( + "a folder path with " + scope + " must be refused, not resolved", + RuntimeException.class, + () -> formFor(A_FOLDER).browseScope(scope).build()); + + // The message assertions are what keep the broad type honest: an incidental + // NullPointerException carries no message and fails here rather than passing as a + // refusal that never happened. + final String message = reasonGivenToTheCaller(refused); + assertTrue("the refusal must name the scope, got: " + message, + message.contains(scope.name())); + assertTrue("the refusal must name the path, got: " + message, + message.contains("/application/")); + } + } + + /** + * The reason a refusal gives the caller, read from wherever the refusal carries it. A JAX-RS + * exception puts it in the response the caller receives rather than in {@code getMessage()}, + * which returns only the status line; anything else is read the ordinary way. Written this way + * so the assertion is about the caller being told what went wrong, not about which exception + * the validation happens to raise. + */ + private static String reasonGivenToTheCaller(final RuntimeException refused) { + return refused instanceof WebApplicationException + ? String.valueOf(((WebApplicationException) refused).getResponse().getEntity()) + : String.valueOf(refused.getMessage()); + } + + /** + * Given each browse scope at the site root, When the form is built, Then it is accepted. This + * is the other half of the invariant: the refusal must be about the contradiction, not about + * the scope being present at all. + */ + @Test + public void testEveryBrowseScopeIsAcceptedAtTheSiteRoot() { + for (final BrowseScope scope : BrowseScope.values()) { + formFor(SITE_ROOT).browseScope(scope).build(); + } + } + + /** + * Given a folder path and no browse scope, When the form is built, Then it is accepted + * unchanged. This is the shape the Asset Picker sends and the reason the field has no default: + * a request that never mentions a scope must keep meaning exactly what it means today. + */ + @Test + public void testAFolderPathWithNoBrowseScopeIsUntouched() { + formFor(A_FOLDER).build(); + } + + /** + * Given a body carrying a field this binary does not know, When it is deserialized, Then it is + * accepted and the unknown field ignored. + * + *

This is the rollback direction of the same compatibility the rest of this class pins going + * forward. A browser holding a cached bundle keeps sending {@code browseScope} after the server + * it talks to has been rolled back to a build that predates the field, and that is not an edge + * case: the field is sent for all site content, the site root and System Host, so it rides on + * the view the drive opens on. Without this the older binary answers every one of those with a + * deserialization failure, and browsing stays broken until the user hard-refreshes -- which + * they have no way of knowing to do.

+ * + *

The trade being made is real: unknown fields are now ignored rather than refused, so a + * caller who misspells one gets silence instead of an error naming their typo. The peer forms + * ({@code A11yAgentStopForm}, {@code PageScanCheckForm}, {@code FileUploadDetail}) make the same + * trade, for the same reason.

+ * + *

Deserialized through the mapper the resource actually uses, not a plain one. A plain + * {@code ObjectMapper} was tried first and fails before reaching the assertion: this form holds + * Guava collections, and the modules that read them are registered by + * {@code createDefaultMapper}. Standing the real one up is also what makes the test mean + * something -- it is that mapper's settings, not Jackson's defaults, that decide whether an + * unknown field is refused, and it leaves {@code FAIL_ON_UNKNOWN_PROPERTIES} at Jackson's + * enabled default.

+ */ + @Test + public void testAFieldThisBinaryDoesNotKnowIsIgnoredRatherThanRefused() throws Exception { + final String bodyFromANewerFrontend = "{\"assetPath\":\"" + SITE_ROOT + + "\",\"language\":[\"1\"],\"aFieldFromTheFuture\":\"ROOT\"}"; + + final DriveRequestForm form = DotObjectMapperProvider.createDefaultMapper() + .readValue(bodyFromANewerFrontend, DriveRequestForm.class); + + assertEquals("the fields this binary does know must still be read", + SITE_ROOT, form.assetPath()); + } +} diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java index 35f58f427945..3b2810a92d39 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java @@ -8,6 +8,7 @@ import com.dotcms.jitsu.validators.AnalyticsValidatorUtilTest; import com.dotcms.junit.MainBaseSuite; import com.dotcms.publisher.business.PublisherQueueJobTest; +import com.dotcms.rest.api.v1.drive.ContentDriveBrowseScopeTest; import com.dotcms.rest.api.v1.drive.ContentDriveFieldFilterTest; import com.dotcms.rest.api.v1.drive.ContentDriveHelperContentletAPIComparisonTest; import com.dotcms.rest.api.v1.drive.ContentDriveKeywordSearchTest; @@ -85,6 +86,7 @@ SecondaryCategoryPermissionTest.class, RequestCostReportTest.class, OpenAIVisionAPIImplTest.class, + ContentDriveBrowseScopeTest.class, ContentDriveFieldFilterTest.class, ContentDriveHelperContentletAPIComparisonTest.class, ContentDriveKeywordSearchTest.class, diff --git a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java index 48d17a3a8f87..cff2d75bf6f2 100644 --- a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java @@ -1980,7 +1980,7 @@ public void test_getContent_Using_LimitedUser_WithRead_Permissions() throws Exce .ignoreSiteForFolders(true) .respectFrontEndRoles(false) // <-- This is key for this test! .withUser(limitedUser) - .forceSystemHost(false) + .systemHostMode(SystemHostMode.EXCLUDE) .showContent(true) .showFiles(false) .showFolders(false) @@ -2080,7 +2080,7 @@ public void test_exhaustive_pagination_with_permission_filtering() throws Except .ignoreSiteForFolders(true) .respectFrontEndRoles(false) .withUser(limitedUser) - .forceSystemHost(false) + .systemHostMode(SystemHostMode.EXCLUDE) .showContent(true) .contentCursor(0) .showFiles(false) @@ -3978,7 +3978,7 @@ public void test_getPaginatedContents_folderScopedCte_permissionScopingUnchanged .ignoreSiteForFolders(true) .respectFrontEndRoles(false) .withUser(limitedUser) - .forceSystemHost(false) + .systemHostMode(SystemHostMode.EXCLUDE) .showFiles(true) .showWorking(true) .build(); diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveBrowseScopeTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveBrowseScopeTest.java new file mode 100644 index 000000000000..86a9b4b56a1e --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveBrowseScopeTest.java @@ -0,0 +1,474 @@ +package com.dotcms.rest.api.v1.drive; + +import com.dotcms.DataProviderWeldRunner; +import com.dotcms.IntegrationTestBase; +import com.dotcms.browser.BrowserAPIImpl; +import com.dotcms.browser.BrowserAPIImpl.PaginatedContents; +import com.dotcms.contenttype.model.type.BaseContentType; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.datagen.FolderDataGen; +import com.dotcms.datagen.SiteDataGen; +import com.dotcms.util.IntegrationTestInitService; +import com.dotcms.rest.exception.BadRequestException; +import com.dotmarketing.beans.Host; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.exception.DotDataException; +import com.dotmarketing.exception.DotSecurityException; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.folders.model.Folder; +import com.dotmarketing.util.Config; +import com.liferay.portal.model.User; +import java.util.Set; +import java.util.function.BiConsumer; +import java.util.stream.Collectors; +import javax.enterprise.context.ApplicationScoped; +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +/** + * Integration tests for the Content Drive browse scopes (issue #37426) on + * {@code POST /api/v1/drive/search}. + * + *

Three scopes replace the one the drive could express: {@code ALL} is the whole site at any + * depth, {@code ROOT} is what sits at the site root, and {@code SYSTEM_HOST} is the shared content + * that belongs to no site. A request that names no scope must keep meaning what it means today, + * because the Asset Picker and six other callers reach this same listing and none of them will + * ever send one.

+ * + *

Isolation follows {@link ContentDriveStatusFilterTest}: a dedicated site, a purpose-built + * content type, and a unique id per run, so nothing here depends on shared demo data or on what + * another test left behind. The site is removed afterwards.

+ * + *

Why a System Host fixture needs care. Content published to System Host outlives this + * test's site and is visible to every other test in the suite. The assertions below therefore + * check that System Host content this test created is present or absent, never that System Host + * contains only that content — another suite's fixture may legitimately be sitting there.

+ */ +@ApplicationScoped +@RunWith(DataProviderWeldRunner.class) +public class ContentDriveBrowseScopeTest extends IntegrationTestBase { + + private static final ContentDriveHelper contentDriveHelper = new ContentDriveHelper(); + private static User systemUser; + + private static Host testSite; + private static Host systemHost; + private static Folder childFolder; + + private static String siteRootPath; + private static String childFolderPath; + + private static ContentType type; + + /** Sits at the site root, with no folder above it. */ + private static Contentlet rootItem; + /** Sits inside {@link #childFolder}, one level below the root. */ + private static Contentlet nestedItem; + /** Belongs to System Host, so to no site at all. */ + private static Contentlet systemHostItem; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + + systemUser = APILocator.getUserAPI().getSystemUser(); + systemHost = APILocator.getHostAPI().findSystemHost(); + + final String uniqueId = System.currentTimeMillis() + ""; + + testSite = new SiteDataGen().name("drive-scope-" + uniqueId + ".local").nextPersisted(); + childFolder = new FolderDataGen().name("driveScopeFolder_" + uniqueId) + .site(testSite).nextPersisted(); + + siteRootPath = "//" + testSite.getHostname() + "/"; + childFolderPath = "//" + testSite.getHostname() + childFolder.getPath(); + + // Built on System Host rather than on the test site, so content of this type is allowed to + // live on either. A type scoped to the test site could not hold the System Host fixture. + type = new ContentTypeDataGen() + .baseContentType(BaseContentType.CONTENT) + .name("DriveScopeType_" + uniqueId) + .velocityVarName("driveScopeType" + uniqueId) + .host(systemHost) + .nextPersisted(); + + rootItem = new ContentletDataGen(type.id()) + .host(testSite) + .setProperty("title", "scope-root-" + uniqueId) + .nextPersisted(); + + nestedItem = new ContentletDataGen(type.id()) + .host(testSite) + .folder(childFolder) + .setProperty("title", "scope-nested-" + uniqueId) + .nextPersisted(); + + systemHostItem = new ContentletDataGen(type.id()) + .host(systemHost) + .setProperty("title", "scope-shared-" + uniqueId) + .nextPersisted(); + } + + @AfterClass + public static void cleanup() throws Exception { + // The site takes its folder and both site-bound contentlets with it. The System Host item + // has no site to be removed with, so it is deleted on its own -- left behind it would show + // up in every later test that lists shared content. + if (null != systemHostItem) { + APILocator.getContentletAPI().destroy(systemHostItem, systemUser, false); + } + if (null != testSite) { + APILocator.getHostAPI().archive(testSite, systemUser, false); + APILocator.getHostAPI().delete(testSite, systemUser, false); + } + } + + /** + * The inodes the drive returned. + * + *

Inodes rather than identifiers, for the reason spelled out in + * {@link ContentDriveStatusFilterTest}: the query selects an inode, so the inode is what proves + * it joined the right version. Read at assertion time, never captured at fixture time.

+ */ + private Set driveInodes(final DriveRequestForm request) + throws DotDataException, DotSecurityException { + return inodesFrom(contentDriveHelper.driveSearch(request, systemUser)); + } + + private static Set inodesFrom(final PaginatedContents results) { + return results.list.stream() + .map(item -> (String) item.get("inode")) + .collect(Collectors.toSet()); + } + + private static String workingInode(final Contentlet contentlet) throws DotDataException { + return APILocator.getVersionableAPI() + .getContentletVersionInfo(contentlet.getIdentifier(), contentlet.getLanguageId()) + .orElseThrow(() -> new AssertionError( + "No version info for " + contentlet.getIdentifier())) + .getWorkingInode(); + } + + private DriveRequestForm.Builder requestAt(final String assetPath) { + return DriveRequestForm.builder() + .assetPath(assetPath) + .live(false) + .offset(0) + .maxResults(100); + } + + // ------------------------------------------------------------- FR-007: the site root + + /** + * Selecting the site row lists what sits at the root and nothing from inside a folder (FR-007). + * + *

This is the view that does not exist today: before the scopes, asking for the site and + * asking for the whole site were the same request.

+ */ + @Test + public void testSiteRootListsRootItemsAndNotFolderContents() + throws DotDataException, DotSecurityException { + final Set inodes = driveInodes( + requestAt(siteRootPath).browseScope(BrowseScope.ROOT).build()); + + assertTrue("Content at the site root must be listed", + inodes.contains(workingInode(rootItem))); + assertFalse("Content inside a folder must not be listed at the site root", + inodes.contains(workingInode(nestedItem))); + } + + /** + * The site root never admits System Host content, whatever the toggle says (FR-008). + * + *

Asserted with {@code includeSystemHost(true)} deliberately — the value the toggle sends + * when it is on. The scope has to win over it, or "the site root" would quietly mean "the site + * root plus everything shared".

+ */ + @Test + public void testSiteRootExcludesSystemHostEvenWithTheToggleOn() + throws DotDataException, DotSecurityException { + final Set inodes = driveInodes(requestAt(siteRootPath) + .browseScope(BrowseScope.ROOT) + .includeSystemHost(true) + .build()); + + assertFalse("System Host content must never appear in the site root scope", + inodes.contains(workingInode(systemHostItem))); + } + + // ------------------------------------------------------------- FR-006: all site content + + /** + * All Site Content spans every depth of the site (FR-006), which is what the site row used to + * do and what this scope now carries. + */ + @Test + public void testAllSiteContentListsEveryDepth() + throws DotDataException, DotSecurityException { + final Set inodes = driveInodes( + requestAt(siteRootPath).browseScope(BrowseScope.ALL).build()); + + assertTrue("Content at the site root must be listed", + inodes.contains(workingInode(rootItem))); + assertTrue("Content inside a folder must also be listed", + inodes.contains(workingInode(nestedItem))); + } + + /** + * The toggle decides whether All Site Content carries shared content alongside the site's + * (FR-020), and it is the only scope where the question means anything. + */ + @Test + public void testAllSiteContentHonoursTheSystemHostToggle() + throws DotDataException, DotSecurityException { + final String sharedInode = workingInode(systemHostItem); + + assertTrue("With the toggle on, shared content joins the site's", + driveInodes(requestAt(siteRootPath) + .browseScope(BrowseScope.ALL) + .includeSystemHost(true) + .build()).contains(sharedInode)); + + assertFalse("With the toggle off, shared content is excluded", + driveInodes(requestAt(siteRootPath) + .browseScope(BrowseScope.ALL) + .includeSystemHost(false) + .build()).contains(sharedInode)); + } + + // ------------------------------------------------------------- FR-010: System Host + + /** + * System Host lists shared content and admits nothing belonging to a site (FR-010). + * + *

Both halves matter. Listing the shared item proves the scope reaches the clause that was + * unreachable before this feature; excluding the site's items proves it did not simply widen.

+ */ + @Test + public void testSystemHostListsSharedContentAndNoSiteContent() + throws DotDataException, DotSecurityException { + final Set inodes = driveInodes( + requestAt(siteRootPath).browseScope(BrowseScope.SYSTEM_HOST).build()); + + assertTrue("Shared content must be listed", + inodes.contains(workingInode(systemHostItem))); + assertFalse("Content at the site root must not appear under System Host", + inodes.contains(workingInode(rootItem))); + assertFalse("Content inside a site folder must not appear under System Host", + inodes.contains(workingInode(nestedItem))); + } + + // ------------------------------------------------------------- folders, per scope + + /** + * The site root reports the site's top-level folders alongside its root content (FR-007). + * + *

They sit at the root, so they are part of what is "at" the root. This is the half that + * distinguishes the site-root scope from all-site-content by more than depth: the two differ in + * their content and in their folders.

+ */ + @Test + public void testSiteRootReportsTheSitesTopLevelFolders() + throws DotDataException, DotSecurityException { + final PaginatedContents results = contentDriveHelper.driveSearch( + requestAt(siteRootPath).browseScope(BrowseScope.ROOT).build(), systemUser); + + assertTrue("The site's top-level folders must be listed at the site root", + results.folderCount > 0); + // `title` rather than a folder-shaped guess: DotFolderTransformerImpl sets both "name" and + // "title" to the folder's name, and "title" is the key content rows carry too. + assertTrue("The test's own folder must be among them", + results.list.stream() + .anyMatch(item -> childFolder.getName().equals(item.get("title")))); + } + + /** + * System Host reports no folders because it has none (FR-010) — asserted with folders + * explicitly requested, so this is about the place and not about the request. + * + *

Distinct from the all-site-content case below. There, a caller could ask for folders and + * get them, and the drive simply does not ask. Here there is nothing to return however the + * request is phrased, which is why System Host can never grow a folder column by accident.

+ */ + @Test + public void testSystemHostHasNoFoldersEvenWhenAskedFor() + throws DotDataException, DotSecurityException { + final PaginatedContents results = contentDriveHelper.driveSearch( + requestAt(siteRootPath) + .browseScope(BrowseScope.SYSTEM_HOST) + .showFolders(true) + .build(), + systemUser); + + assertEquals("System Host holds no folders, so none can be listed", + 0, results.folderCount); + } + + /** + * All Site Content carries no folders (FR-006) — and the decision is the caller's. + * + *

Folder policy deliberately lives with the caller so the response always matches the + * request, which is why this asserts the two halves separately: asking for no folders returns + * none, and asking for them still returns them. A scope that silently suppressed folders would + * make the response stop matching the request, and would take the Asset Picker with it.

+ */ + @Test + public void testAllSiteContentReturnsNoFoldersWhenItDoesNotAskForThem() + throws DotDataException, DotSecurityException { + assertEquals("Asking for no folders must return none", + 0, + contentDriveHelper.driveSearch(requestAt(siteRootPath) + .browseScope(BrowseScope.ALL) + .showFolders(false) + .build(), systemUser).folderCount); + + assertTrue("The scope must not decide this on the caller's behalf", + contentDriveHelper.driveSearch(requestAt(siteRootPath) + .browseScope(BrowseScope.ALL) + .showFolders(true) + .build(), systemUser).folderCount > 0); + } + + // ------------------------------------------------------------- FR-026: the scope-less request + + /** + * A request naming no scope behaves as it does today (FR-026). The most important test in + * this file. + * + *

Seven callers reach this listing without ever sending a scope. Asserted at a folder path + * rather than at the root because that is the Asset Picker's shape, and because it is the one + * that would break if {@code ALL} had been made the default: the folder constraint would be + * discarded and the picker would start listing every descendant.

+ */ + @Test + public void testNoScopeAtAFolderPathStillListsThatFolderOnly() + throws DotDataException, DotSecurityException { + final Set inodes = driveInodes(requestAt(childFolderPath).build()); + + assertTrue("The folder's own content must be listed", + inodes.contains(workingInode(nestedItem))); + assertFalse("Content outside the folder must not be listed", + inodes.contains(workingInode(rootItem))); + } + + /** + * At the site root, no scope and {@code ALL} are the same request (FR-026) — the whole-site + * view the drive produced before this feature, now reachable by name. + */ + @Test + public void testNoScopeAtTheSiteRootMatchesAllSiteContent() + throws DotDataException, DotSecurityException { + assertEquals("Omitting the scope at the site root must equal asking for ALL", + driveInodes(requestAt(siteRootPath).build()), + driveInodes(requestAt(siteRootPath).browseScope(BrowseScope.ALL).build())); + } + + /** + * A scope paired with a folder path is refused rather than resolved (FR-022's invariant). + * + *

Refusing is the point. Picking one of two contradictory statements would list content from + * somewhere the caller did not ask for, and the caller would have no way to tell.

+ */ + @Test + public void testAScopeWithAFolderPathIsRefused() { + // `com.dotcms.rest.exception.BadRequestException`, not the JAX-RS class of the same name. + // They are siblings rather than subtypes -- this one extends `HttpStatusCodeException` -- + // so naming the wrong one compiles and then fails the moment the test first runs. The + // unit-level twin of this assertion deliberately catches `RuntimeException` and checks the + // message, which is why it never noticed. + assertThrows("A scope is only meaningful at the site root", + BadRequestException.class, + () -> requestAt(childFolderPath).browseScope(BrowseScope.ROOT).build()); + } + + // ------------------------------------------------------------- FR-012 / SC-004: both paths + + /** The two internal search paths, only the first of which runs unless configured otherwise. */ + private static final String[] SEARCH_HEURISTICS = + {"HYBRID_SINGLE_CHUNKED_QUERY_ES", "PURE_ES"}; + + /** + * Runs one request under both search paths and hands each result set to {@code assertions}. + * + *

The heuristic is memoised per {@link BrowserAPIImpl} instance, so setting the config is + * not enough on its own — each run gets a fresh instance through the helper's injectable + * constructor, whose lazy read then picks the new value up.

+ * + *

A text filter is what routes a request through the index at all. Without one neither + * heuristic is consulted and a test using this would quietly assert nothing, so it is applied + * here rather than left to each caller to remember.

+ */ + private void underBothSearchPaths(final BrowseScope scope, + final BiConsumer> assertions) + throws DotDataException, DotSecurityException { + final String original = Config.getStringProperty("BROWSE_API_HEURISTIC_TYPE", + SEARCH_HEURISTICS[0]); + try { + for (final String heuristic : SEARCH_HEURISTICS) { + Config.setProperty("BROWSE_API_HEURISTIC_TYPE", heuristic); + + assertions.accept(heuristic, inodesFrom( + new ContentDriveHelper(new BrowserAPIImpl()).driveSearch( + requestAt(siteRootPath) + .browseScope(scope) + .filters(QueryFilters.builder().text("scope").build()) + .build(), + systemUser))); + } + } finally { + // Read with the default rather than null: Config hands the value straight to the + // properties store, so restoring a null would be worse than the state it replaced. + Config.setProperty("BROWSE_API_HEURISTIC_TYPE", original); + } + } + + /** + * System Host admits nothing belonging to a site, whichever path serves the request (FR-012). + * + *

What is asserted, and what deliberately is not. The index-only path gives up + * read-your-writes by design, so content written moments ago may legitimately not be indexed + * yet. Asserting the fixture is present would produce a flake that reads exactly like a scope + * bug. What must hold under both paths is what is excluded — and an unindexed fixture + * cannot make an exclusion true by accident.

+ */ + @Test + public void testSystemHostScopeAdmitsNoSiteContentUnderEitherSearchPath() + throws DotDataException, DotSecurityException { + final String rootInode = workingInode(rootItem); + final String nestedInode = workingInode(nestedItem); + + underBothSearchPaths(BrowseScope.SYSTEM_HOST, (heuristic, inodes) -> { + assertFalse("Site root content leaked into System Host under " + heuristic, + inodes.contains(rootInode)); + assertFalse("Folder content leaked into System Host under " + heuristic, + inodes.contains(nestedInode)); + }); + } + + /** + * The same guarantee for the site root: searching within it never starts returning content + * from inside a folder, or shared content (FR-012, and the spec's search edge case). + */ + @Test + public void testSiteRootAdmitsNoFolderOrSharedContentUnderEitherSearchPath() + throws DotDataException, DotSecurityException { + final String nestedInode = workingInode(nestedItem); + final String sharedInode = workingInode(systemHostItem); + + underBothSearchPaths(BrowseScope.ROOT, (heuristic, inodes) -> { + assertFalse("Folder content leaked into the site root under " + heuristic, + inodes.contains(nestedInode)); + assertFalse("Shared content leaked into the site root under " + heuristic, + inodes.contains(sharedInode)); + }); + } +} diff --git a/dotcms-postman/src/main/resources/postman/ContentDriveResource.postman_collection.json b/dotcms-postman/src/main/resources/postman/ContentDriveResource.postman_collection.json index 5b18894027cb..8ad5abcb243b 100644 --- a/dotcms-postman/src/main/resources/postman/ContentDriveResource.postman_collection.json +++ b/dotcms-postman/src/main/resources/postman/ContentDriveResource.postman_collection.json @@ -1448,6 +1448,216 @@ ], "description": "Tests search and filtering functionality using the filters.text parameter." }, + { + "name": "Browse Scope Tests", + "item": [ + { + "name": "No Scope - Behaves As It Does Today", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "// The shape every caller outside Content Drive sends. Whatever it returns today it", + "// must keep returning: the Asset Picker and six other callers reach this same listing", + "// and none of them will ever send a scope.", + "pm.test(\"Status code should be 200\", function () {", + " pm.response.to.have.status(200);", + "});", + "", + "pm.test(\"Returns a listing, with no scope sent\", function () {", + " var body = pm.response.json();", + " pm.expect(body.entity).to.have.property('list');", + " pm.collectionVariables.set('noScopeCount', body.entity.list.length);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\n \"assetPath\": \"//{{testSiteName}}/\",\n \"maxResults\": 50\n}" + }, + "url": { + "raw": "{{serverURL}}/api/v1/drive/search", + "host": [ + "{{serverURL}}" + ], + "path": [ + "api", + "v1", + "drive", + "search" + ] + } + }, + "response": [] + }, + { + "name": "ALL At The Site Root - Same As Sending Nothing", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "// At the site root these are the same request. ALL is what the drive's own flat view", + "// asks for, and it must not mean anything different from the historic default.", + "pm.test(\"Status code should be 200\", function () {", + " pm.response.to.have.status(200);", + "});", + "", + "pm.test(\"Returns the same number of items as sending no scope\", function () {", + " var body = pm.response.json();", + " var noScope = Number(pm.collectionVariables.get('noScopeCount'));", + " pm.expect(body.entity.list.length).to.eql(noScope);", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\n \"assetPath\": \"//{{testSiteName}}/\",\n \"browseScope\": \"ALL\",\n \"maxResults\": 50\n}" + }, + "url": { + "raw": "{{serverURL}}/api/v1/drive/search", + "host": [ + "{{serverURL}}" + ], + "path": [ + "api", + "v1", + "drive", + "search" + ] + } + }, + "response": [] + }, + { + "name": "ROOT - Lists The Site Root Only", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "// The view that did not exist before this feature: what sits AT the root, without the", + "// contents of the folders below it.", + "pm.test(\"Status code should be 200\", function () {", + " pm.response.to.have.status(200);", + "});", + "", + "pm.test(\"Admits nothing that lives inside a folder\", function () {", + " var body = pm.response.json();", + " body.entity.list.forEach(function (item) {", + " // Folders themselves belong at the root; their CONTENT does not.", + " if (item.baseType) {", + " pm.expect(item.folder, JSON.stringify(item.title)).to.eql('SYSTEM_FOLDER');", + " }", + " });", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\n \"assetPath\": \"//{{testSiteName}}/\",\n \"browseScope\": \"ROOT\",\n \"maxResults\": 50\n}" + }, + "url": { + "raw": "{{serverURL}}/api/v1/drive/search", + "host": [ + "{{serverURL}}" + ], + "path": [ + "api", + "v1", + "drive", + "search" + ] + } + }, + "response": [] + }, + { + "name": "Scope With A Folder Path - Refused", + "event": [ + { + "listen": "test", + "script": { + "exec": [ + "// Refused rather than resolved by precedence. Picking one of two contradictory", + "// statements would list content from somewhere the caller did not ask for, and the", + "// caller would have no way to tell it happened.", + "pm.test(\"Status code should be 400\", function () {", + " pm.response.to.have.status(400);", + "});", + "", + "pm.test(\"Says which scope and which path it refused\", function () {", + " var text = pm.response.text();", + " pm.expect(text).to.include('browseScope');", + "});" + ], + "type": "text/javascript" + } + } + ], + "request": { + "method": "POST", + "header": [ + { + "key": "Content-Type", + "value": "application/json" + } + ], + "body": { + "mode": "raw", + "raw": "{\n \"assetPath\": \"//{{testSiteName}}/alpha-folder/\",\n \"browseScope\": \"ROOT\",\n \"maxResults\": 50\n}" + }, + "url": { + "raw": "{{serverURL}}/api/v1/drive/search", + "host": [ + "{{serverURL}}" + ], + "path": [ + "api", + "v1", + "drive", + "search" + ] + } + }, + "response": [] + } + ] + }, { "name": "Combined Tests", "item": [ diff --git a/specs/37426-content-drive-browse-scopes/contracts/drive-search-browse-scope.md b/specs/37426-content-drive-browse-scopes/contracts/drive-search-browse-scope.md new file mode 100644 index 000000000000..ed0e59b78a08 --- /dev/null +++ b/specs/37426-content-drive-browse-scopes/contracts/drive-search-browse-scope.md @@ -0,0 +1,72 @@ +# Contract delta: `browseScope` on `POST /api/v1/drive/search` + +**Feature**: [#37426](https://github.com/dotCMS/core/issues/37426) · **Spec**: [spec.md](../spec.md) · **Date**: 2026-09-11 + +This endpoint has two callers: Content Drive and the Asset Picker (`DotContentDriveService.search`). Both are in this repo, so the change is coordinated rather than published, but the Asset Picker is owned elsewhere and must not have to change. Everything below is written from that constraint. + +--- + +## The addition + +One optional field on the request body. + +```jsonc +{ + "assetPath": "//demo.dotcms.com/", + "browseScope": "ALL" | "ROOT" | "SYSTEM_HOST" // optional + // …every other field unchanged +} +``` + +There is no change to the response body, to any other request field, or to any status code other than the new 400 described below. + +--- + +## What each value means + +| `browseScope` | `assetPath` | Returns | +|---|---|---| +| *(omitted)* | `//site/` | Everything on the site, at any depth, System Host included when `includeSystemHost` is true. **Exactly today's behavior.** | +| *(omitted)* | `//site/folder/` | That folder's contents. **Exactly today's behavior.** | +| `ALL` | `//site/` | The same as omitting it at the site root. The explicit spelling. | +| `ROOT` | `//site/` | Only what sits at the site root. System Host is never included. | +| `SYSTEM_HOST` | `//site/` | System Host content only. The site portion of `assetPath` is context, not a filter, and is not read. | +| any value | `//site/folder/` | **400.** See below. | + +--- + +## The compatibility guarantee + +**A request that does not carry `browseScope` returns exactly what it returns today, byte for byte.** This is the contract's load-bearing promise, and it is what lets the Asset Picker stay untouched. Two consequences worth stating so they are not traded away later: + +The field is **not** defaulted to `ALL` in a way that changes behavior. `ALL` is meaningful only at the site root, and an omitted scope inside a folder is not the same as `ALL` inside a folder. Defaulting the field so that it reads more tidily in the schema would silently turn the Asset Picker's folder requests recursive. + +`includeSystemHost` keeps its current meaning and default. It is read when the scope is `ALL` or omitted, and ignored otherwise, because the other two scopes already answer the System Host question. + +--- + +## The refusal + +An explicit `browseScope` with a path that is not the site root is rejected with **400**, naming both values. + +The three scopes are things you can only be in at the root: the whole site, the root itself, and System Host. A folder is addressed by its path. There is therefore no combination of a folder path and a scope that means anything, and rather than resolve one by precedence the request is refused, following `BulkUploadForm.isExactlyOneTargetGiven`: "Both is refused rather than resolved by precedence. A caller that sends a folder and a site has said two contradictory things, and picking one would silently put an author's files somewhere they did not choose." + +```json +{ "message": "browseScope is only valid at the site root; got 'SYSTEM_HOST' with path '/application/'" } +``` + +--- + +## OpenAPI + +`openapi.yaml` is generated at compile time, so the description lives in the Java annotation and the regenerated file is committed alongside it. The `@Schema` must state three things a generator cannot infer: that omitting the field means today's behavior, that it is only valid at the site root, and that `includeSystemHost` is read only for `ALL`. + +Regenerate with `./mvnw compile -pl :dotcms-core --am -DskipTests`. + +--- + +## What is not in this contract + +`showFolders` stays the caller's decision. The endpoint honours whatever it is sent so the response always matches the request and the folder cursors never describe a query the caller did not make. The client suppresses folders in `ALL` and `SYSTEM_HOST`; the server does not do it for them. + +The frontend's URL encoding of the same choice (absent, `/`, a path, or `SYSTEM_HOST`, all in one value) is a Content Drive concern and is not part of this contract. The Asset Picker has no URL and no browse scope. diff --git a/specs/37426-content-drive-browse-scopes/data-model.md b/specs/37426-content-drive-browse-scopes/data-model.md new file mode 100644 index 000000000000..a76dfc1c2865 --- /dev/null +++ b/specs/37426-content-drive-browse-scopes/data-model.md @@ -0,0 +1,68 @@ +# Phase 1 Data Model: Content Drive browse scopes + +**Feature**: [spec.md](./spec.md) · **Plan**: [plan.md](./plan.md) · **Date**: 2026-09-11 + +No persisted data changes: no table, no column, no index mapping. Everything here is request-time state. + +--- + +## BrowseScope (new, request-level) + +Which slice of content the listing is asked for. + +| Value | Folder constraint | Host clause | Folders listed | +|---|---|---|---| +| *(absent)* | today's behavior: dropped at the site root, applied inside a folder | today's behavior | caller's choice | +| `ALL` | dropped, so every depth of the site | site, plus System Host when the toggle is on | no (client asks for none) | +| `ROOT` | applied at `/`, so only what sits at the root | site only | yes, the top-level folders | +| `SYSTEM_HOST` | applied at `/` | System Host only | no (System Host has none) | + +**Validation**: a value other than absent is accepted only when the request's path is the site root. With a folder path, any explicit value is a 400. Refused, never resolved by precedence. + +**Relationship to the existing `includeSystemHost`**: read only when the scope is `ALL` or absent. `ROOT` excludes System Host regardless; `SYSTEM_HOST` is System Host regardless. + +--- + +## SystemHostMode (new, internal to `BrowserQuery`) + +Replaces the `forceSystemHost` boolean. Not part of the REST contract. + +| Value | SQL emitted | Lucene emitted | +|---|---|---| +| `EXCLUDE` *(default)* | `and (id.host_inode = ?)` | `+conhost:` | +| `INCLUDE` | `and (id.host_inode = ? or id.host_inode = 'SYSTEM_HOST')` | `+(conhost: OR conhost:SYSTEM_HOST)` | +| `ONLY` | `and (id.host_inode = 'SYSTEM_HOST')` | `+conhost:SYSTEM_HOST` | + +`EXCLUDE` as the default is the whole backward-compatibility story: it reproduces exactly what the boolean `false` produces today, and no caller outside Content Drive ever set the flag. `ONLY` reaches `appendSystemHostQuery`, which exists today and is unreachable because it fires only when the query carries no site. + +**State transitions**: none. The mode is derived per request from the browse scope and the System Host toggle, never mutated after the query is built. + +--- + +## Location (frontend, one value) + +The store's existing `path` becomes the single statement of where the user is. It is the only thing the URL carries about location. + +| Value | Selection | Sent as | +|---|---|---| +| *(absent)* | All Site Content | `assetPath: ///`, `browseScope: ALL` | +| `/` | the site root | `assetPath: ///`, `browseScope: ROOT` | +| `/folder/…` | that folder | `assetPath: ///folder/…`, no scope | +| `SYSTEM_HOST` | System Host | `assetPath: ///`, `browseScope: SYSTEM_HOST` | + +Two rules protect this table. Reserved words can never collide with a folder, because every real path begins with `/` and no reserved word does. And the mapping to `assetPath` must be explicit rather than template interpolation: the current expression at `dot-content-drive.store.ts:129` would produce `//demo.dotcms.comSYSTEM_HOST`. + +**Why absent rather than an explicit `ALL` token**: the URL writer already removes the `path` parameter when the path is empty (`DEFAULT_PATH = undefined`), so links already in circulation carry no path and must keep meaning "the whole site". Absent is not a gap in the model, it is the back-compatible spelling of All Site Content. + +--- + +## Sidebar selection (frontend) + +Exactly one of four things is selected: the All Site Content row, the site row, a folder node, or the System Host row. The tree's existing `selectedNode` continues to represent the middle two; the two new rows live outside the tree and must clear it when chosen, and be cleared by it. The System Host row sits below the hierarchy, which scrolls within its own area, so a growing tree can never push it out of reach. + +| Selection | Drop target | Add content | +|---|---|---| +| All Site Content | no | no | +| site row | yes, as today | yes, as today | +| folder | yes, as today | yes, as today | +| System Host | yes, moves content there | yes, gated against System Host itself | diff --git a/specs/37426-content-drive-browse-scopes/spec.md b/specs/37426-content-drive-browse-scopes/spec.md new file mode 100644 index 000000000000..702780320631 --- /dev/null +++ b/specs/37426-content-drive-browse-scopes/spec.md @@ -0,0 +1,176 @@ +# Feature Specification: Content Drive browse scopes + +**Feature Branch**: `37426-content-drive-browse-scopes` + +**Created**: 2026-09-09 + +**Status**: Draft + +**Type**: New Feature + +**Input**: GitHub issue [#37426](https://github.com/dotCMS/core/issues/37426) — "[TASK] Content Drive: browse scopes for All, site root and System Host" + +## User Scenarios & Testing *(mandatory)* + +Content Drive can express only one browse scope today. Selecting a site lists everything on it, at every depth, with System Host content mixed in. There is no way to ask for the items that sit **at** the site root, and no way to reach System Host content on its own. This feature splits that single browse scope into three the user picks from the sidebar. + +### User Story 1 - Browse the site root, and browse the whole site, as separate things (Priority: P1) + +An editor opens Content Drive and wants to see what actually lives at the top of the site, not a flat list of every asset in every folder. Selecting the site in the hierarchy shows the site root: the items sitting there plus the site's top-level folders. When they do want the flat everything-on-this-site view, they select **All Site Content** at the top of the sidebar. + +**Why this priority**: This is the defect at the heart of the request. "Browse the site" and "browse the site root" are the same request today, so one of the two views simply does not exist. Everything else in this feature builds on the sidebar having distinct, selectable browse scopes. + +**Independent Test**: On a site with content at the root and content nested in folders, select the site row and confirm only root-level items and top-level folders are listed; select All Site Content and confirm the nested content appears and no folders do. Delivers the missing root view without any System Host work. + +**Acceptance Scenarios**: + +1. **Given** a site with an asset at its root and another asset inside a folder, **When** the user selects the site row in the hierarchy, **Then** the listing shows the root asset and the site's top-level folders, and does not show the asset that lives inside a folder. +2. **Given** the same site, **When** the user selects **All Site Content**, **Then** the listing shows both assets and shows no folders at all. +3. **Given** the user has selected **All Site Content**, **When** they select a folder in the hierarchy, **Then** the listing shows that folder's contents exactly as it does today. +4. **Given** the user has selected the site row, **When** they look at the listing, **Then** no System Host content appears in it regardless of any other setting. +5. **Given** any of the three sidebar entries is selected, **When** the user selects a different one, **Then** the previous selection is cleared, so exactly one entry is ever active. +6. **Given** the user has selected **All Site Content**, **When** they add content by any route — uploading, creating, or dropping files onto the listing — **Then** it is accepted and lands at the site root. +7. **Given** the user is on a later page of **All Site Content**, **When** they select a different sidebar entry, **Then** the listing starts again at its first page with no items still selected. +8. **Given** the site row is selected and a search is running, **When** the search is served by either of the product's two internal search paths, **Then** both return the same items, and neither admits content from inside a folder or from System Host. + +--- + +### User Story 2 - See System Host content on its own (Priority: P2) + +A user needs to find or manage assets shared across every site. Today those assets can only be seen mixed into a site's listing. Selecting **System Host** at the bottom of the sidebar lists System Host content and nothing else. + +**Why this priority**: It is the second capability that does not exist today, and it is what makes the "Show System Host" toggle honest: shared content becomes reachable on its own instead of only ever appearing as an overlay on a site. + +**Independent Test**: With shared content published to System Host and other content on a regular site, select System Host and confirm only the shared content is listed and no folders appear. + +**Acceptance Scenarios**: + +1. **Given** content exists on System Host and on the current site, **When** the user selects **System Host**, **Then** only the System Host content is listed. +2. **Given** the user has selected **System Host**, **When** they look at the listing, **Then** no folders are offered, because System Host has none. +3. **Given** the user has selected **All Site Content** with "Show System Host" on, **When** they look at the listing, **Then** System Host content appears alongside the current site's content. +4. **Given** the user has selected **All Site Content** with "Show System Host" off, **When** they look at the listing, **Then** no System Host content appears. +5. **Given** the user has selected the site row or a folder, **When** they look at the filter bar, **Then** the "Show System Host" control is not offered: it has nothing to decide outside **All Site Content**, because System Host content can only ever sit at the System Host root. +6. **Given** **System Host** is selected and a search is running, **When** the search is served by either of the product's two internal search paths, **Then** both return the same items, and neither admits content belonging to a site. + +--- + +### User Story 3 - Move content to System Host by dropping it there (Priority: P3) + +Having selected some content, a user drags it onto the **System Host** entry to share it across every site, the same gesture they already use to move content into a folder. + +**Why this priority**: System Host is a real destination, so making it a drop target completes the interaction. It is separable from browsing: the browse scopes are useful before drag and drop is wired up. + +**Independent Test**: Select an item on a site, drag it onto the System Host entry, and confirm it afterwards appears under System Host and no longer under the site. + +**Acceptance Scenarios**: + +1. **Given** the user has selected content on a site, **When** they drop it onto the **System Host** entry, **Then** the content is moved to System Host and the listing reflects the move. +2. **Given** the user is dragging content, **When** they drag it over the **All Site Content** entry, **Then** it is not offered as a drop target and nothing is moved. Dropping *files* onto the listing while this scope is selected is a different gesture and is accepted (FR-013); what is refused here is the **entry** as a move destination, because the site row directly beneath it already means the site root. Two adjacent rows that move content to the same place is a worse offer than one. +3. **Given** the user is dragging content, **When** they drag it over the site row or a folder, **Then** it behaves exactly as it does today. +4. **Given** the user lacks permission to add content to System Host, **When** they drag content over the **System Host** entry, **Then** it is not offered as a drop target. + +--- + +### Edge Cases + +- **A site with nothing at its root.** Selecting the site row shows the top-level folders and no content, or the empty state if the site has no folders either. It must not silently fall back to the everything view. +- **A search combined with a browse scope.** A text search narrows within the selected browse scope; it never widens it. Searching while the site row is selected must not start returning content from inside folders, and searching while System Host is selected must not start returning site content. +- **Restoring a shared link.** A link that names a browse scope reopens on it. A link saved before this feature names none and reopens on the view it produced before, so old links do not silently change meaning. +- **Switching sites while System Host is selected.** The selection survives, because System Host belongs to no site and its content does not change. The **hierarchy below re-renders for the newly chosen site**, so the switch visibly does something rather than appearing to fail, and the selected state MUST stay on the System Host entry: if the highlight drifts onto the new site's root, the sidebar claims the user is in two places at once. +- **A URL whose location is a reserved word rather than a path.** It selects that browse scope. The two cannot be confused, because every real folder path begins with `/` and no reserved word does, so no site can ever own a folder that collides with one. +- **A user without read access to System Host content.** Permission filtering applies to every browse scope, so the System Host entry can legitimately produce an empty listing for such a user. +- **A user who may browse System Host but not add to it.** The System Host entry lists content but refuses uploads, creation and drops, the same way a folder the user cannot add to already behaves. +- **Switching browse scope mid-page.** A user on page 4 of All Site Content who selects System Host lands on the first page of System Host, with nothing carried over from the previous selection. + +## Requirements *(mandatory)* + +### Functional Requirements + +#### Sidebar structure + +- **FR-001**: The sidebar MUST offer three kinds of selection: an **All Site Content** entry at the top, the **site hierarchy** (the site row and its folders), and a **System Host** entry at the bottom. +- **FR-001a**: The hierarchy MUST scroll within its own area rather than growing the sidebar, so that the **System Host** entry below it stays visible without hunting for it. The hierarchy loads lazily and grows as folders are expanded, so an entry that merely followed it in the page flow would drift further out of reach with every expansion. The entry above the hierarchy needs nothing special: scrolling back to the top is a cheap, known gesture. +- **FR-002**: **All Site Content** and **System Host** MUST be presented as plain sidebar sections, not as nodes of the site hierarchy: no expansion control, no children, and nothing beneath them to navigate into. This is about structure only. It does not stop System Host accepting content, which the rules below require of it. +- **FR-003**: Exactly one entry MUST be selected at any time; selecting one clears the previous selection. +- **FR-004**: The sidebar MUST NOT show item counts next to any entry. +- **FR-005**: Both sections MUST be reachable and selectable by keyboard, alongside the hierarchy they sit around. + +#### What each browse scope lists + +- **FR-006**: Selecting **All Site Content** MUST list the current site's content at any depth, and MUST NOT list folders. +- **FR-007**: Selecting the **site row** MUST list only the items that sit at the site root, including the site's top-level folders. +- **FR-008**: Selecting the **site row** MUST NOT include System Host content, under any setting. +- **FR-009**: Selecting a **folder** MUST list its contents exactly as it does today. +- **FR-010**: Selecting **System Host** MUST list System Host content only, and MUST NOT list folders. +- **FR-011**: Every browse scope MUST continue to respect the requesting user's read permissions. +- **FR-012**: A text search or a field filter MUST only ever remove items from the selected browse scope, never add items from outside it. Filtering narrows a browse scope; it never changes which browse scope was asked for. The answer MUST NOT depend on which of the product's internal search paths served the request. + +#### Creating, uploading and moving + +- **FR-013**: **All Site Content** MUST accept new content, which lands at the **site root**. Uploading, creating and dropping files onto the listing all behave as they do on the site row. The in-flight status deliberately does NOT name the destination: it was tried and removed on 2026-09-16 because the sentence had grown long enough that the part an author reads at a glance was lost behind the part they do not. The cost is knowingly accepted — in the System Host scope the site switcher still names a different site, so an upload there says only how many files are going, not where. + + This reverses an earlier version of this requirement, which made the view read-only on the grounds that a view spanning the whole site "names no single place to put anything". That is true of the view and false of the product: the site root is the obvious destination, and **this is the content search view, where uploading has always been possible and has always landed on the current site**. Refusing the upload prevented no mistake — it sent people to the site row to do the same thing one click later. + + The cost is accepted knowingly: because the listing carries no path column (see Assumptions), a file added here appears among everything else with nothing saying it sits at the root. That gap is inherited rather than introduced, the indicator naming the site is today's mitigation, and the location column that closes it properly is being handled separately. +- **FR-014**: The **site row**, a **folder**, and **System Host** MUST accept new content: uploads, creation, and content dropped onto them. For the site row and folders this is exactly today's behavior. +- **FR-015**: Dropping content onto the **System Host** entry MUST move it to System Host. +- **FR-016**: While **System Host** is selected, the permission check that gates creating and uploading MUST be evaluated against System Host itself, never against whichever site is selected in the site switcher. + +#### The System Host toggle + +- **FR-016a**: A user who may not read System Host MUST NOT be offered it. The sidebar entry is absent rather than disabled — a control with nothing to decide should not be sitting there — and a location naming System Host, however it was arrived at, MUST return the user to All Site Content silently. Silently because they did nothing wrong: usually they followed somebody else's link, and the drive has somewhere sensible to put them. + This is an affordance, not a defence. The listing already enforces read permissions server-side (FR-011), so what this removes is a door that opens onto an empty room, not an exposure. Accordingly a lookup that fails for any reason other than an outright refusal MUST leave the scope available: a timeout says nothing about permissions, and locking someone out of a scope they hold is the worse error. +- **FR-017**: The control that decides whether System Host content appears MUST live in a bar above the listing rather than among the filter chips, beside a sentence naming what the listing is currently showing. The sentence MUST agree with the control: with System Host included it reads "All Files in site (System Host shared files included)", and with it excluded, "…excluded". Links already in circulation that carry the control's value MUST keep restoring correctly. The Asset Picker keeps the chip it has always had, labelled "Show System Host"; this requirement governs Content Drive only. +- **FR-018**: The bar MUST be offered only while **All Site Content** is selected — the site root and System Host each answer the question it asks simply by being chosen, leaving its sentence nothing to qualify and its toggle nothing to decide. It MUST open and close by height rather than appearing and vanishing, so the listing is pushed down instead of jumping under the pointer. It MUST NOT show an item count: the listing endpoint pages by cursor and returns no total, so any number shown there would be invented. +- **FR-019**: The control MUST retain its value while another browse scope is selected, so returning to **All Site Content** restores the user's previous choice rather than resetting it. +- **FR-020**: With the control on, **All Site Content** MUST include System Host content alongside the site's; with it off, **All Site Content** MUST exclude it. + +#### Persistence + +- **FR-021**: The selection MUST be carried by the single URL value that already says where the drive is browsing, not by a second value beside it. Two values could disagree with each other, and then neither would be the answer. +- **FR-022**: That one value MUST be able to express all four selections: **absent** means All Site Content, `/` means the site root, a deeper path means that folder, and a reserved word means System Host. A reserved word MUST NOT be mistakable for a folder, which the leading `/` on every real path already guarantees. +- **FR-023**: Links made before this feature MUST keep meaning what they meant. A link carrying no location still lists the whole site, and a link to a folder still opens that folder. +- **FR-024**: A reload, a browser back or forward, and a shared link MUST all reopen the selection the sender was viewing. +- **FR-025**: Changing the selection MUST return the listing to its first page and clear the current item selection, since neither carries any meaning across selections. + +#### Not breaking what exists + +- **FR-026**: A content listing requested without a browse scope MUST behave as it does today, so other surfaces that share this listing (notably the Asset Picker) are unaffected by this feature. +- **FR-027**: No consumer of the shared content-listing service other than Content Drive may change behavior. Only Content Drive's own requests carry a browse scope; every other caller MUST keep producing exactly the results it produces today. +- **FR-028**: That claim MUST be demonstrated rather than assumed. Every one of those consumers reaches the listing through a single shared seam, so the seam itself MUST be pinned by a test, and each named consumer MUST be recorded as either covered by that test or checked by inspection. None may be left unaccounted for. + +### Key Entities + +- **Browse scope**: Which slice of content the listing is being asked for. One of: the whole current site at any depth, the current site's root only, or System Host only. Named in full throughout, because Content Drive is separately gaining a *search* scope that says which fields a search looks at. The two are independent: a browse scope says where you are, a search scope says how a search reads what is there. +- **Site root**: The level of a site that is not inside any folder. Home to both root-level content and the site's top-level folders. +- **System Host**: The site-independent container for content shared across every site. It holds no folders, and site content cannot live inside it. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: A user can list the items sitting at a site's root without any content from inside that site's folders appearing, which is not possible today. +- **SC-002**: A user can list System Host content on its own without any site's content appearing, which is not possible today. +- **SC-003**: In 100% of listings produced by the site-root and System Host browse scopes, every returned item belongs to the browse scope that was asked for. +- **SC-004**: A given browse scope returns the same set of items with a search active as without one, minus only the items the search legitimately excludes. +- **SC-005**: Opening a shared Content Drive link reproduces the browse scope the sender was viewing, every time. +- **SC-006**: Content is moved to System Host in a single drag, with no dialog and no intermediate step. +- **SC-007**: Surfaces other than Content Drive that browse content list exactly what they listed before this change, a listing asked for without a browse scope returns exactly what it returns today, and every consumer of that listing is accounted for by name rather than covered by a blanket claim. +- **SC-008**: Content added from the All Site Content view — by any of uploading, creating, or dropping files onto the listing — arrives at the site root. + +## Legacy Considerations *(dotCMS-specific — mandatory)* + +- **Existing behavior touched**: The Content Drive listing and the underlying content-browsing service it shares with the Asset Picker, which is long-standing code that predates Content Drive. The meaning of "the site is selected" changes deliberately: it becomes the site root rather than the whole site, with the whole-site view moving to its own **All Site Content** entry. That is a visible behavior change for existing Content Drive users and is the point of the feature. +- **Backward-compatibility expectations**: **Preserving existing behavior is a hard constraint, not a preference.** The content-listing service Content Drive uses is shared with the assets API, the older file browser and its deprecated tree endpoint, the legacy admin browser, a Velocity viewtool, and two internal callers. None of them will ask for a scope, so whatever a scope-less request means today it must mean afterwards, byte for byte, including how System Host content is treated. Shared Content Drive links created before this change must keep restoring, including the value of the toggle being renamed. No deprecation of existing admin workflows is intended. +- **A rename that now lands on one surface**: the "Show Shared Assets" label is a single shared translation that once served both the Content Drive toolbar and the Asset Picker toolbar. Content Drive no longer uses it — its control moved into the scope bar with wording of its own (FR-017) — so renaming it to "Show System Host" changes the Asset Picker alone. That is a visible change on a surface this feature does not otherwise touch, and it should be called out in review rather than discovered there. +- **Known related decisions**: The listing has two internal query paths that can disagree about whether System Host content is included; they must agree before any browse scope can be trusted, which is why the spec requires a scope to return the same items with or without a search rather than leaving it as an implementation concern. Only one of the two runs by default, and the other is reachable only by configuration, so proving they agree means deliberately exercising the site-root and System Host scopes under each rather than waiting for the non-default one to show up on its own. Moving content and browsing content also address System Host by different means, so support for one does not imply support for the other. Issue #37166 is related: it touches how Content Drive reports operations and surfaced this while examining what a move actually changes in the listing. The plan phase will formally consult `dotCMS/platform-adrs`. + +## Assumptions + +- **System Host accepts everything a folder accepts**: moved content, uploaded files, and newly created content. It is a real place to put things. **All Site Content** names no place of its own, but content added there lands at the site root, which is the destination the site row names — so the two agree rather than one of them refusing. +- **Selecting System Host survives a site switch**, because System Host belongs to no site and the listing would not change. +- **The site root browse scope shows the site's top-level folders.** They sit at the root, so they are part of what is "at" the root. This means the site-root and All Site Content browse scopes differ in their content, not in their folders, since All Site Content shows no folders at all. +- **Where an item lives is not shown in All Site Content, and that is accepted for now.** The listing carries no path column, so a flat view spanning the whole site cannot tell two files of the same name in different folders apart. The gap is inherited rather than introduced: today's site view is already this flat view. Naming the view and making it the default does raise the cost of it, and a location column shown when the listing spans more than one folder is the fix, but it is being handled separately and is not a defect in this work. +- **The Move dialog is not being changed to match the drop.** Its destination picker filters System Host out of the site list whenever a folder destination is required, which is the mode the dialog runs in, so dragging onto System Host will be the only route to that destination. Whether the dialog should offer it too is a product decision, deliberately left outside this feature. +- **"Children of the site root" is deliberately not a browse scope.** System Host has no folders and site content cannot live under System Host, so the site hierarchy already covers browsing below the root. +- **Item counts beside the sidebar entries are prototype-only** and are not part of this feature.