diff --git a/core-web/apps/dotcms-ui-e2e/src/tests/edit-content/sidebar/history-refresh.spec.ts b/core-web/apps/dotcms-ui-e2e/src/tests/edit-content/sidebar/history-refresh.spec.ts new file mode 100644 index 000000000000..91ebe1effba4 --- /dev/null +++ b/core-web/apps/dotcms-ui-e2e/src/tests/edit-content/sidebar/history-refresh.spec.ts @@ -0,0 +1,244 @@ +import { APIRequestContext, expect, Locator, Page, test } from '@playwright/test'; +import { ContentType, createFakeContentType, deleteContentType } from '@requests/contentType'; +import { admin1 } from '@utils/credentials'; +import { generateBase64Credentials } from '@utils/generateBase64Credential'; + +/** + * Regression spec for issue #36617: the sidebar's History and Comments sections must + * reflect the latest state after a save/publish, with no manual page reload. + * + * The bug had two faces, one per host, so both are covered here: + * - Full-screen (routed): the save navigates, which re-initializes the store and empties + * the version list. Without the fix the list stayed empty. + * - Dialog (overlay): the save does NOT navigate, so nothing clears or re-fetches. + * Without the fix the list stayed stale until the dialog was closed and reopened. + * + * Notes: + * - Sidebar tab headers carry no data-testid; they are identified by role/tab index + * (0 = Actions, 1 = History, 2 = Comments). The panels themselves DO have testids + * (`history`, `activities`), so the assertions anchor on those. + * - The dialog is only reachable in production from UVE or the relationship field's + * "New Content" item. The latter needs no page/template setup, so it is used here. + */ + +const HISTORY_TAB_INDEX = 1; +const COMMENTS_TAB_INDEX = 2; + +function authHeaders() { + return { Authorization: generateBase64Credentials(admin1.username, admin1.password) }; +} + +/** Version rows currently rendered in the History panel of the given root. */ +function historyItems(root: Page | Locator): Locator { + return root.getByTestId('history').getByTestId('history-item'); +} + +async function openSidebarTab(root: Page | Locator, index: number): Promise { + // Must be scoped to `sidebar-tabs`: the form area has its own role="tab" elements + // (Content / SEO / Social / Advanced Properties) that would otherwise match first. + await root.getByTestId('sidebar-tabs').getByRole('tab').nth(index).click(); +} + +async function fireSidebarAction(root: Page | Locator, actionName: string): Promise { + await openSidebarTab(root, 0); + const button = root.getByTestId('sidebar-workflow-actions').getByRole('button', { + name: actionName + }); + await button.waitFor({ state: 'visible', timeout: 10000 }); + await button.click(); +} + +test.describe('Sidebar History/Comments refresh after save (#36617)', () => { + test.describe.configure({ mode: 'serial' }); + + let contentType: ContentType; + let contentletInode: string; + + test.beforeAll(async ({ request }) => { + const suffix = Date.now(); + + contentType = await createFakeContentType(request, { + name: `HistoryRefresh${suffix}`, + variable: `historyRefreshCT${suffix}`, + fields: [ + { + clazz: 'com.dotcms.contenttype.model.field.ImmutableTextField', + name: 'Title', + variable: 'title', + sortOrder: 1 + } + ] + }); + + const response = await request.put( + '/api/v1/workflow/actions/default/fire/PUBLISH?indexPolicy=WAIT_FOR', + { + data: { + contentlet: { + contentType: contentType.variable, + title: `History Refresh ${suffix}` + } + }, + headers: authHeaders() + } + ); + + expect(response.status()).toBe(200); + contentletInode = (await response.json()).entity.inode; + }); + + test.afterAll(async ({ request }) => { + // deleteContentType cascades contentlets + if (contentType?.id) { + await deleteContentType(request, contentType.id); + } + }); + + test.describe('full-screen host', () => { + test.beforeEach(async ({ page }) => { + await page.goto(`/dotAdmin/#/content/${contentletInode}`); + await page.waitForLoadState('domcontentloaded'); + await page + .locator('dot-edit-content-sidebar') + .waitFor({ state: 'visible', timeout: 15000 }); + await page.getByTestId('title').waitFor({ state: 'visible', timeout: 15000 }); + }); + + test('adds the new version to History after publishing, with no reload', async ({ + page + }) => { + await openSidebarTab(page, HISTORY_TAB_INDEX); + await expect(historyItems(page).first()).toBeVisible({ timeout: 15000 }); + const before = await historyItems(page).count(); + + await fireSidebarAction(page, 'Publish'); + + // Back to History: the list must grow on its own. The bug rendered it empty + // here (the routed save empties the list and nothing re-fetched it). + await openSidebarTab(page, HISTORY_TAB_INDEX); + await expect(historyItems(page)).toHaveCount(before + 1, { timeout: 20000 }); + }); + + test('adds the new comment to Comments after posting, with no reload', async ({ page }) => { + await openSidebarTab(page, COMMENTS_TAB_INDEX); + + const comment = `e2e comment ${Date.now()}`; + await page.getByTestId('activities-input').fill(comment); + await page.getByTestId('activities-submit').click(); + + await expect( + page.getByTestId('activities').getByText(comment, { exact: false }) + ).toBeVisible({ timeout: 15000 }); + }); + }); + + test.describe('dialog host', () => { + /** + * Opens the editor in a dialog through the relationship field's "New Content" + * item, saves once so the contentlet gains a version, and returns the dialog root. + */ + async function openDialogWithSavedContent( + page: Page, + request: APIRequestContext + ): Promise { + const suffix = Date.now(); + + const targetType = await createFakeContentType(request, { + name: `DialogTarget${suffix}`, + variable: `dialogTargetCT${suffix}`, + fields: [ + { + clazz: 'com.dotcms.contenttype.model.field.ImmutableTextField', + name: 'Title', + variable: 'title', + sortOrder: 1 + } + ] + }); + + const parentType = await createFakeContentType(request, { + name: `DialogParent${suffix}`, + variable: `dialogParentCT${suffix}`, + fields: [ + { + clazz: 'com.dotcms.contenttype.model.field.ImmutableTextField', + name: 'Title', + variable: 'title', + sortOrder: 1 + }, + { + clazz: 'com.dotcms.contenttype.model.field.ImmutableRelationshipField', + name: 'Rel', + variable: 'rel', + sortOrder: 2, + relationships: { + cardinality: 0, + velocityVar: targetType.variable + } + } + ] + }); + + const created = await request.put( + '/api/v1/workflow/actions/default/fire/PUBLISH?indexPolicy=WAIT_FOR', + { + data: { + contentlet: { + contentType: parentType.variable, + title: `Dialog Parent ${suffix}` + } + }, + headers: authHeaders() + } + ); + expect(created.status()).toBe(200); + const parentInode = (await created.json()).entity.inode; + + await page.goto(`/dotAdmin/#/content/${parentInode}`); + await page.waitForLoadState('domcontentloaded'); + await page.getByTestId('relationship-add-button').click(); + await page.getByRole('menuitem', { name: 'New Content' }).click(); + + const dialog = page.getByTestId('edit-content-dialog'); + await dialog.waitFor({ state: 'visible', timeout: 15000 }); + + // Save once inside the dialog so there is a version to compare against. + await dialog.getByTestId('title').fill(`Dialog Child ${suffix}`); + await fireSidebarAction(dialog, 'Publish'); + await openSidebarTab(dialog, HISTORY_TAB_INDEX); + await expect(historyItems(dialog).first()).toBeVisible({ timeout: 20000 }); + + return dialog; + } + + test('adds the new version to History after publishing inside the dialog', async ({ + page, + request + }) => { + const dialog = await openDialogWithSavedContent(page, request); + const before = await historyItems(dialog).count(); + + await fireSidebarAction(dialog, 'Publish'); + + // The dialog never navigates, so nothing clears or re-fetches on its own. + // The bug left this list stale until the dialog was closed and reopened. + await openSidebarTab(dialog, HISTORY_TAB_INDEX); + await expect(historyItems(dialog)).toHaveCount(before + 1, { timeout: 20000 }); + }); + + /** + * Comments are deliberately NOT covered in the dialog host. The comment form is + * hidden whenever the editor was opened for new content + * (`$hideForm = $initialContentletState() === 'new'` in + * dot-edit-content-sidebar-activities.component.ts), and that flag keeps its + * value after the first save. The relationship field — the only dialog entry + * point that needs no page/template setup — always opens with `mode: 'new'`, + * so the form is never available there. + * + * Covering it requires the UVE pencil flow (`mode: 'edit'`), which needs a page + * with a contentlet on it. Worth adding when a page fixture exists; the + * full-screen comment test above already covers the refresh logic itself, and + * the dialog History test covers the dialog-specific half of the bug. + */ + }); +}); diff --git a/core-web/libs/edit-content/src/lib/components/dot-edit-content-layout/dot-edit-content.layout.component.spec.ts b/core-web/libs/edit-content/src/lib/components/dot-edit-content-layout/dot-edit-content.layout.component.spec.ts index 99c1ab5fbe77..9cb4ab4161b3 100644 --- a/core-web/libs/edit-content/src/lib/components/dot-edit-content-layout/dot-edit-content.layout.component.spec.ts +++ b/core-web/libs/edit-content/src/lib/components/dot-edit-content-layout/dot-edit-content.layout.component.spec.ts @@ -1,3 +1,4 @@ +import { patchState } from '@ngrx/signals'; import { byTestId, createComponentFactory, @@ -34,7 +35,7 @@ import { DotWorkflowsActionsService, DotWorkflowService } from '@dotcms/data-access'; -import { DotCMSWorkflowAction, DotLanguage } from '@dotcms/dotcms-models'; +import { ComponentStatus, DotCMSWorkflowAction, DotLanguage } from '@dotcms/dotcms-models'; import { GlobalStore } from '@dotcms/store'; import { DotMessagePipe } from '@dotcms/ui'; import { @@ -55,7 +56,7 @@ import { DotRelatedContentNavigationStore } from '../../store/dot-related-content-navigation.store'; import { DotEditContentStore } from '../../store/edit-content.store'; -import { MOCK_CONTENTLET_1_TAB } from '../../utils/edit-content.mock'; +import { MOCK_CONTENTLET_1_TAB, MOCK_WORKFLOW_STATUS } from '../../utils/edit-content.mock'; import * as utils from '../../utils/functions.util'; import { CONTENT_TYPE_MOCK } from '../../utils/mocks'; import { DotEditContentFormComponent } from '../dot-edit-content-form/dot-edit-content-form.component'; @@ -1042,3 +1043,219 @@ describe('EditContentLayoutComponent - Dialog Dirty-Close Guard', () => { }); }); }); + +/** + * Integration coverage for the sidebar-refresh invariant (issue #36617). + * + * The feature specs (`activities.feature.spec.ts`, `information.feature.spec.ts`, + * `history.feature.spec.ts`) verify the store effects against a synthetic store with + * NO component mounted. That cannot catch the regression this fix addresses: those + * effects used to live on `DotEditContentSidebarComponent`, which the layout's + * `@if ($store.isLoaded() || $store.isSaving() || $store.isReloading())` destroys and + * recreates — taking the effects with it. + * + * Here the store is real and mounted inside the real layout, while the sidebar is a + * `MockComponent` (no effects of its own). So if the fetching ever moves back into + * that component, these tests fail where the feature specs would still pass. + * + * Both hosts are covered because they differ in what happens after a save: the + * full-screen host navigates (re-initializing and clearing the lists), while the + * dialog host does not — and the refresh must not depend on that difference. + */ +describe.each([ + ['full-screen', false], + ['dialog', true] +])( + 'EditContentLayoutComponent - sidebar refresh is store-owned (%s host)', + (_hostName, inPlaceNavigation) => { + let spectator: Spectator; + let store: SpyObject>; + let dotEditContentService: SpyObject; + + const IDENTIFIER = MOCK_CONTENTLET_1_TAB.identifier; + const hostTrail = signal([]); + const host = { + inPlaceNavigation, + inPlaceNavigation$: undefined, + trail: hostTrail, + setTrail: jest.fn(), + resolveIdentity: jest.fn().mockReturnValue({}), + reportSaved: jest.fn(), + reloadContent: jest.fn(), + setContentTitle: jest.fn(), + addBreadcrumb: jest.fn(), + goToSavedContent: jest.fn(), + goToRestoredVersion: jest.fn(), + goToRelatedContent: jest.fn(), + goToCrumb: jest.fn() + }; + + const emptyPage = { + entity: [], + pagination: null, + errors: [], + i18nMessagesMap: {}, + messages: [], + permissions: [] + }; + + const createComponent = createComponentFactory({ + component: DotEditContentLayoutComponent, + imports: [ + MessageModule, + ButtonModule, + MockComponent(DotEditContentFormComponent), + MockComponent(DotEditContentSidebarComponent), + DotMessagePipe + ], + componentProviders: [ + DotEditContentStore, + mockProvider(DotWorkflowsActionsService), + mockProvider(DotWorkflowActionsFireService), + mockProvider(DotEditContentService), + mockProvider(DotContentTypeService), + mockProvider(DotWorkflowService), + mockProvider(DotContentletService), + mockProvider(DotVersionableService), + ConfirmationService, + { provide: EDIT_CONTENT_HOST, useValue: host } + ], + providers: [ + mockProvider(DotHttpErrorManagerService), + mockProvider(MessageService), + mockProvider(DialogService), + mockProvider(DotLanguagesService), + mockProvider(DotSiteService, { + getCurrentSite: jest + .fn() + .mockReturnValue(of({ identifier: 'default', hostname: 'demo.dotcms.com' })) + }), + mockProvider(DotSystemConfigService, { + getSystemConfig: jest.fn().mockReturnValue(of({})) + }), + GlobalStore, + { + provide: DotCurrentUserService, + useValue: { getCurrentUser: () => of({ userId: '123', userName: 'John Doe' }) } + }, + { + provide: ActivatedRoute, + useValue: { + get snapshot() { + return { params: { id: '', contentType: '' } }; + } + } + }, + mockProvider(Router, { + navigate: jest.fn().mockReturnValue(Promise.resolve(true)), + url: '/test-url', + events: of() + }), + provideHttpClient(), + provideHttpClientTesting(), + mockProvider(DotMessageService, { get: jest.fn((key: string) => key) }), + mockProvider(DotRelatedContentNavigationStore, { + trail: hostTrail, + registerTitle: jest.fn(), + buildTrailForSavedInode: jest.fn().mockReturnValue(null) + }) + ] + }); + + /** Puts the store in the state a loaded contentlet produces, so the sidebar renders. */ + const loadContentlet = (contentlet = MOCK_CONTENTLET_1_TAB) => { + patchState(store, { + contentlet, + contentType: CONTENT_TYPE_MOCK, + state: ComponentStatus.LOADED + }); + spectator.detectChanges(); + }; + + beforeEach(() => { + spectator = createComponent({ detectChanges: false }); + store = spectator.inject(DotEditContentStore, true); + dotEditContentService = spectator.inject(DotEditContentService, true); + + dotEditContentService.getActivities.mockReturnValue(of([])); + dotEditContentService.getReferencePages.mockReturnValue(of(0)); + dotEditContentService.getVersions.mockReturnValue(of(emptyPage)); + dotEditContentService.getPushPublishHistory.mockReturnValue(of(emptyPage)); + + // The lock and workflow features also react to `contentlet`, so their services + // need observables or their effects blow up before the ones under test run. + spectator + .inject(DotContentletService, true) + .canLock.mockReturnValue(of({ entity: { canLock: true } })); + spectator + .inject(DotWorkflowsActionsService, true) + .getByInode.mockReturnValue(of(MOCK_SINGLE_WORKFLOW_ACTIONS)); + spectator + .inject(DotWorkflowService, true) + .getWorkflowStatus.mockReturnValue(of(MOCK_WORKFLOW_STATUS)); + }); + + it('should fetch sidebar data even while the sidebar component is not rendered', fakeAsync(() => { + // `@if` is false here (not loaded, not saving, not reloading), so the sidebar is + // never mounted — yet the data must still load, because the store owns it. + patchState(store, { + contentlet: MOCK_CONTENTLET_1_TAB, + state: ComponentStatus.LOADING + }); + spectator.detectChanges(); + tick(); + + expect(spectator.query('dot-edit-content-sidebar')).toBeNull(); + expect(dotEditContentService.getActivities).toHaveBeenCalledWith(IDENTIFIER); + expect(dotEditContentService.getReferencePages).toHaveBeenCalledWith(IDENTIFIER); + expect(dotEditContentService.getVersions).toHaveBeenCalled(); + })); + + it('should refresh after the sidebar component is destroyed and recreated', fakeAsync(() => { + loadContentlet(); + tick(); + expect(spectator.query('dot-edit-content-sidebar')).not.toBeNull(); + + dotEditContentService.getActivities.mockClear(); + dotEditContentService.getReferencePages.mockClear(); + dotEditContentService.getVersions.mockClear(); + + // Flip the `@if` off — Angular destroys the sidebar and, before this fix, its effects. + patchState(store, { contentType: null, state: ComponentStatus.LOADING }); + spectator.detectChanges(); + tick(); + expect(spectator.query('dot-edit-content-sidebar')).toBeNull(); + + // Come back with a new inode, as a save does. + loadContentlet({ ...MOCK_CONTENTLET_1_TAB, inode: 'inode-after-save' }); + tick(); + + expect(spectator.query('dot-edit-content-sidebar')).not.toBeNull(); + expect(dotEditContentService.getActivities).toHaveBeenCalledWith(IDENTIFIER); + expect(dotEditContentService.getReferencePages).toHaveBeenCalledWith(IDENTIFIER); + expect(dotEditContentService.getVersions).toHaveBeenCalled(); + })); + + it('should refresh on a save that mints a new inode without any re-initialization', fakeAsync(() => { + // This is the dialog host's path: no navigation, so nothing clears the lists and + // the identifier/locale never change. Asserted for both hosts because the refresh + // must no longer depend on which one is mounted. + loadContentlet(); + tick(); + + dotEditContentService.getActivities.mockClear(); + dotEditContentService.getReferencePages.mockClear(); + dotEditContentService.getVersions.mockClear(); + + patchState(store, { + contentlet: { ...MOCK_CONTENTLET_1_TAB, inode: 'inode-after-publish' } + }); + spectator.detectChanges(); + tick(); + + expect(dotEditContentService.getActivities).toHaveBeenCalledWith(IDENTIFIER); + expect(dotEditContentService.getReferencePages).toHaveBeenCalledWith(IDENTIFIER); + expect(dotEditContentService.getVersions).toHaveBeenCalled(); + })); + } +); diff --git a/core-web/libs/edit-content/src/lib/components/dot-edit-content-sidebar/dot-edit-content-sidebar.component.ts b/core-web/libs/edit-content/src/lib/components/dot-edit-content-sidebar/dot-edit-content-sidebar.component.ts index c7a63be974fa..ad05f3aa187d 100644 --- a/core-web/libs/edit-content/src/lib/components/dot-edit-content-sidebar/dot-edit-content-sidebar.component.ts +++ b/core-web/libs/edit-content/src/lib/components/dot-edit-content-sidebar/dot-edit-content-sidebar.component.ts @@ -1,13 +1,4 @@ -import { - ChangeDetectionStrategy, - Component, - computed, - effect, - inject, - model, - output, - untracked -} from '@angular/core'; +import { ChangeDetectionStrategy, Component, computed, inject, model, output } from '@angular/core'; import { toSignal } from '@angular/core/rxjs-interop'; import { ConfirmationService } from 'primeng/api'; @@ -134,31 +125,6 @@ export class DotEditContentSidebarComponent { */ readonly workflowActionFired = output(); - /** - * Effect that loads sidebar data (reference pages and activities) when the - * sidebar is open and the contentlet identifier is available. - * Gating on `isSidebarOpen` avoids firing these API calls on every edit-content - * page load when the user never actually opens the sidebar. - * - * Depends on the whole `contentlet` (not just the identifier) so it also refreshes - * after a save/publish, which mints a NEW inode under the SAME identifier: the - * reload resets these statuses to LOADING, and without a re-fetch here they would - * stay LOADING forever and hang the unified loading overlay. - */ - // eslint-disable-next-line no-unused-private-class-members -- effect() runs for its side effects; the field only holds the EffectRef - #informationEffect = effect(() => { - const contentlet = this.$store.contentlet(); - const identifier = contentlet?.identifier; - const isSidebarOpen = this.$store.isSidebarOpen(); - - untracked(() => { - if (identifier && isSidebarOpen) { - this.$store.getReferencePages(identifier); - this.$store.loadActivities(identifier); - } - }); - }); - /** * Fires the reset-workflow action directly against the store. * diff --git a/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.spec.ts b/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.spec.ts index 21c3617d4f23..b8209a783a97 100644 --- a/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.spec.ts +++ b/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.spec.ts @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/no-explicit-any */ -import { signalStore, withState } from '@ngrx/signals'; +import { patchState, signalStore, withState } from '@ngrx/signals'; import { createServiceFactory, SpectatorService, SpyObject } from '@openng/spectator/jest'; import { of, throwError } from 'rxjs'; @@ -12,7 +12,7 @@ import { delay } from 'rxjs/operators'; import { DotHttpErrorManagerService, DotMessageService } from '@dotcms/data-access'; import { HttpCode } from '@dotcms/dotcms-js'; -import { ComponentStatus } from '@dotcms/dotcms-models'; +import { ComponentStatus, DotCMSContentlet } from '@dotcms/dotcms-models'; import { withActivities } from './activities.feature'; @@ -264,4 +264,74 @@ describe('Activities Feature Store', () => { expect(store.activities()).toEqual([...mockActivities, mockNewActivity]); })); }); + + describe('Automatic Activities Loading Effect', () => { + const mockContentlet = { + identifier: 'test-identifier', + inode: 'test-inode', + languageId: 1 + } as DotCMSContentlet; + + beforeEach(() => { + dotEditContentService.getActivities.mockReturnValue(of(mockActivities)); + }); + + it('should automatically load activities when the contentlet is set', fakeAsync(() => { + patchState(store, { contentlet: mockContentlet }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getActivities).toHaveBeenCalledWith('test-identifier'); + expect(store.activities()).toEqual(mockActivities); + })); + + it('should automatically reload activities when a save mints a new inode under the same identifier', fakeAsync(() => { + patchState(store, { contentlet: mockContentlet }); + spectator.flushEffects(); + tick(); + dotEditContentService.getActivities.mockClear(); + + patchState(store, { contentlet: { ...mockContentlet, inode: 'new-inode-after-save' } }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getActivities).toHaveBeenCalledWith('test-identifier'); + })); + + it('should not load activities when the sidebar is closed', fakeAsync(() => { + patchState(store, { + contentlet: mockContentlet, + uiState: { ...store.uiState(), isSidebarOpen: false } + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getActivities).not.toHaveBeenCalled(); + })); + + it('should not load activities when there is no contentlet identifier', fakeAsync(() => { + patchState(store, { contentlet: null }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getActivities).not.toHaveBeenCalled(); + })); + + it('should not reload activities on unrelated uiState changes', fakeAsync(() => { + patchState(store, { contentlet: mockContentlet }); + spectator.flushEffects(); + tick(); + dotEditContentService.getActivities.mockClear(); + + // Every uiState writer replaces the slice wholesale, so the effect must + // depend on the isSidebarOpen leaf rather than the object. + patchState(store, { + uiState: { ...store.uiState(), activeSidebarTab: 2, view: 'form' } + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getActivities).not.toHaveBeenCalled(); + })); + }); }); diff --git a/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.ts b/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.ts index d07b9b4233fd..090d325c0e85 100644 --- a/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.ts +++ b/core-web/libs/edit-content/src/lib/store/features/activities/activities.feature.ts @@ -1,10 +1,10 @@ import { tapResponse } from '@ngrx/operators'; -import { patchState, signalStoreFeature, type, withMethods } from '@ngrx/signals'; +import { patchState, signalStoreFeature, type, withHooks, withMethods } from '@ngrx/signals'; import { rxMethod } from '@ngrx/signals/rxjs-interop'; import { pipe } from 'rxjs'; import { HttpErrorResponse } from '@angular/common/http'; -import { inject } from '@angular/core'; +import { effect, inject, untracked } from '@angular/core'; import { MessageService } from 'primeng/api'; @@ -136,6 +136,35 @@ export function withActivities() { ) ) }) - ) + ), + withHooks({ + onInit(store) { + /** + * Reloads activities whenever the contentlet changes, store-owned so + * it works regardless of which component (or host — dialog vs + * full-screen route) happens to be mounted at the time. Depends on the + * whole contentlet (not just the identifier) so it also refreshes after + * a save/publish, which mints a NEW inode under the SAME identifier. + * Gated on `isSidebarOpen` to avoid firing on every edit-content load + * when the user never opens the sidebar. + * + * Reads the `isSidebarOpen` leaf rather than `uiState()`: every writer + * replaces that slice wholesale, so depending on the object would refetch + * on unrelated UI changes — including the `view` flip that `loadVersions` + * itself performs. + */ + effect(() => { + const contentlet = store.contentlet(); + const identifier = contentlet?.identifier; + const isSidebarOpen = store.uiState.isSidebarOpen(); + + untracked(() => { + if (identifier && isSidebarOpen) { + store.loadActivities(identifier); + } + }); + }); + } + }) ); } diff --git a/core-web/libs/edit-content/src/lib/store/features/content/content.feature.ts b/core-web/libs/edit-content/src/lib/store/features/content/content.feature.ts index b20a45f30e37..401a38bd5dda 100644 --- a/core-web/libs/edit-content/src/lib/store/features/content/content.feature.ts +++ b/core-web/libs/edit-content/src/lib/store/features/content/content.feature.ts @@ -286,11 +286,11 @@ export function withContent() { // // Reference pages + activities are reset to LOADING so that, // during an in-place reload, `isFullyLoaded` stays false until - // the sidebar re-fetches them for the new content (the sidebar's - // identifier effect refires when the contentlet swaps). Without - // this, their stale LOADED status from the previous content would - // make `isFullyLoaded` briefly true and drop the reload overlay - // before the sidebar actually reloaded. + // they are re-fetched for the new content (the store-level + // effects in `withInformation`/`withActivities` refire when the + // contentlet swaps). Without this, their stale LOADED status + // from the previous content would make `isFullyLoaded` briefly + // true and drop the reload overlay before they actually reloaded. information: { status: ComponentStatus.LOADING, error: null, diff --git a/core-web/libs/edit-content/src/lib/store/features/history/history.feature.spec.ts b/core-web/libs/edit-content/src/lib/store/features/history/history.feature.spec.ts index 5b74acc6bf16..ae36842090e9 100644 --- a/core-web/libs/edit-content/src/lib/store/features/history/history.feature.spec.ts +++ b/core-web/libs/edit-content/src/lib/store/features/history/history.feature.spec.ts @@ -1016,17 +1016,29 @@ describe('HistoryFeature', () => { expect(store.pushPublishHistory()).toEqual(expectedSortedAfterClear); })); - it('should not reload anything when only the version inode changes', fakeAsync(() => { + it('should reload versions but not push publish history when the live inode moves', fakeAsync(() => { + // A bare inode move with no historical flags set is a newly minted version + // (save/publish) — browsing a version always goes through `loadVersionContent`, + // which sets isViewingHistoricalVersion. This is also the dialog-host path: + // it never navigates, so nothing clears the list and the identity keys stay + // equal; only the live inode moves. spectator.flushEffects(); tick(); + expect(store.versionsStatus().status).toBe(ComponentStatus.LOADED); dotEditContentService.getVersions.mockClear(); dotEditContentService.getPushPublishHistory.mockClear(); - store.updateContentlet({ ...mockContentlet, inode: 'another-version-inode' }); + store.updateContentlet({ ...mockContentlet, inode: 'new-inode-after-publish' }); spectator.flushEffects(); tick(); - expect(dotEditContentService.getVersions).not.toHaveBeenCalled(); + expect(dotEditContentService.getVersions).toHaveBeenCalledTimes(1); + expect(dotEditContentService.getVersions).toHaveBeenCalledWith( + mockContentlet.identifier, + { offset: 1, limit: DEFAULT_VERSIONS_PER_PAGE }, + mockContentlet.languageId + ); + // Push publish history is per identifier, which did not change. expect(dotEditContentService.getPushPublishHistory).not.toHaveBeenCalled(); })); @@ -1155,6 +1167,241 @@ describe('HistoryFeature', () => { expect(dotEditContentService.getVersions).not.toHaveBeenCalled(); expect(dotEditContentService.getPushPublishHistory).not.toHaveBeenCalled(); })); + + it('should reload versions when an in-place reload empties the list under the same identity', fakeAsync(() => { + spectator.flushEffects(); + tick(); + expect(store.versions()).toEqual(mockVersionsResponse.entity); + dotEditContentService.getVersions.mockClear(); + + // What `initializeExistingContent` does after a save/publish: the lists are + // emptied back to INIT while identifier and locale stay the same. + patchState(store, { + versions: [], + versionsPagination: null, + versionsStatus: { status: ComponentStatus.INIT, error: null } + }); + store.updateContentlet({ ...mockContentlet, inode: 'new-inode-after-publish' }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).toHaveBeenCalledWith( + mockContentlet.identifier, + { offset: 1, limit: DEFAULT_VERSIONS_PER_PAGE }, + mockContentlet.languageId + ); + expect(store.versions()).toEqual(mockVersionsResponse.entity); + })); + + it('should reload push publish history when an in-place reload empties it under the same identifier', fakeAsync(() => { + spectator.flushEffects(); + tick(); + dotEditContentService.getPushPublishHistory.mockClear(); + + patchState(store, { + pushPublishHistory: [], + pushPublishHistoryPagination: null, + pushPublishHistoryStatus: { status: ComponentStatus.INIT, error: null } + }); + store.updateContentlet({ ...mockContentlet, inode: 'new-inode-after-publish' }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getPushPublishHistory).toHaveBeenCalledWith( + mockContentlet.identifier, + { offset: 1, limit: DEFAULT_PUSH_PUBLISH_HISTORY_PER_PAGE } + ); + expect(store.pushPublishHistory()).toHaveLength( + mockPushPublishHistoryResponse.entity.length + ); + })); + + it('should reload versions when publishing while in compare view', fakeAsync(() => { + spectator.flushEffects(); + tick(); + + // What `loadCompareVersionContent` does: `contentlet` stays LIVE, only + // compareContentlet is set, and isViewingHistoricalVersion stays false. + const compareContent = { ...mockContentlet, inode: 'compare-inode' }; + patchState(store, { + compareContentlet: compareContent, + historicalVersionInode: 'compare-inode', + originalContentlet: mockContentlet, + isViewingHistoricalVersion: false, + uiState: { ...store.uiState(), view: 'compare' } + }); + spectator.flushEffects(); + tick(); + dotEditContentService.getVersions.mockClear(); + + // Publishing from compare view still mints a new live version, so the list + // must refresh — compare must not be mistaken for browsing a version. + store.updateContentlet({ ...mockContentlet, inode: 'new-inode-after-publish' }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).toHaveBeenCalledTimes(1); + })); + + it('should not fetch with the outgoing identifier while a reload is in flight', fakeAsync(() => { + spectator.flushEffects(); + tick(); + dotEditContentService.getVersions.mockClear(); + dotEditContentService.getPushPublishHistory.mockClear(); + + // What `initializeExistingContent` does: empties the lists to INIT and flips + // state to LOADING while KEEPING the outgoing contentlet on screen. + patchState(store, { + state: ComponentStatus.LOADING, + versions: [], + versionsPagination: null, + versionsStatus: { status: ComponentStatus.INIT, error: null }, + pushPublishHistory: [], + pushPublishHistoryPagination: null, + pushPublishHistoryStatus: { status: ComponentStatus.INIT, error: null } + }); + spectator.flushEffects(); + tick(); + + // Nothing yet: firing here would request the content we are leaving. + expect(dotEditContentService.getVersions).not.toHaveBeenCalled(); + expect(dotEditContentService.getPushPublishHistory).not.toHaveBeenCalled(); + + // The reload lands: new contentlet and LOADED arrive in the same patch. + const incoming = { + ...mockContentlet, + identifier: 'incoming-identifier', + inode: 'incoming-inode' + }; + patchState(store, { contentlet: incoming, state: ComponentStatus.LOADED }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).toHaveBeenCalledWith( + 'incoming-identifier', + { offset: 1, limit: DEFAULT_VERSIONS_PER_PAGE }, + incoming.languageId + ); + expect(dotEditContentService.getPushPublishHistory).toHaveBeenCalledWith( + 'incoming-identifier', + { offset: 1, limit: DEFAULT_PUSH_PUBLISH_HISTORY_PER_PAGE } + ); + })); + + it('should not reload versions when entering a historical version', fakeAsync(() => { + spectator.flushEffects(); + tick(); + dotEditContentService.getVersions.mockClear(); + + // What `loadVersionContent` does: swaps the contentlet for the old version. + patchState(store, { + contentlet: { ...mockContentlet, inode: 'historical-inode' }, + originalContentlet: mockContentlet, + isViewingHistoricalVersion: true, + historicalVersionInode: 'historical-inode' + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).not.toHaveBeenCalled(); + })); + + it('should not reload versions when returning from a historical version', fakeAsync(() => { + spectator.flushEffects(); + tick(); + + patchState(store, { + contentlet: { ...mockContentlet, inode: 'historical-inode' }, + originalContentlet: mockContentlet, + isViewingHistoricalVersion: true, + historicalVersionInode: 'historical-inode' + }); + spectator.flushEffects(); + tick(); + dotEditContentService.getVersions.mockClear(); + + // What `exitHistoricalView` does: restores the original live contentlet. + patchState(store, { + contentlet: mockContentlet, + originalContentlet: null, + isViewingHistoricalVersion: false, + historicalVersionInode: null + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).not.toHaveBeenCalled(); + })); + + it('should not reload versions when entering compare view', fakeAsync(() => { + spectator.flushEffects(); + tick(); + dotEditContentService.getVersions.mockClear(); + + // Compare keeps `contentlet` live but sets historicalVersionInode. + patchState(store, { + compareContentlet: { ...mockContentlet, inode: 'compare-inode' }, + historicalVersionInode: 'compare-inode', + originalContentlet: mockContentlet, + uiState: { ...store.uiState(), view: 'compare' } + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).not.toHaveBeenCalled(); + })); + + it('should reload versions after a publish that follows a historical round trip', fakeAsync(() => { + spectator.flushEffects(); + tick(); + + // Browse a historical version and come back. + patchState(store, { + contentlet: { ...mockContentlet, inode: 'historical-inode' }, + originalContentlet: mockContentlet, + isViewingHistoricalVersion: true, + historicalVersionInode: 'historical-inode' + }); + spectator.flushEffects(); + tick(); + patchState(store, { + contentlet: mockContentlet, + originalContentlet: null, + isViewingHistoricalVersion: false, + historicalVersionInode: null + }); + spectator.flushEffects(); + tick(); + dotEditContentService.getVersions.mockClear(); + + // Now publish: the baseline must still be the live inode, so this counts. + store.updateContentlet({ ...mockContentlet, inode: 'new-inode-after-publish' }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).toHaveBeenCalledTimes(1); + })); + + it('should not refetch repeatedly once a reload-triggered load settles', fakeAsync(() => { + spectator.flushEffects(); + tick(); + dotEditContentService.getVersions.mockClear(); + + patchState(store, { + versions: [], + versionsStatus: { status: ComponentStatus.INIT, error: null } + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).toHaveBeenCalledTimes(1); + + // Flushing again must not re-enter the loader: status is LOADED, not INIT. + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getVersions).toHaveBeenCalledTimes(1); + })); }); describe('restoreVersion', () => { diff --git a/core-web/libs/edit-content/src/lib/store/features/history/history.feature.ts b/core-web/libs/edit-content/src/lib/store/features/history/history.feature.ts index e74a337d42f4..94cf7be64b0b 100644 --- a/core-web/libs/edit-content/src/lib/store/features/history/history.feature.ts +++ b/core-web/libs/edit-content/src/lib/store/features/history/history.feature.ts @@ -763,12 +763,42 @@ export function withHistory() { * Reloads never clear the current items first: the previous list stays * visible while loading (page 1 replaces it on response), so the sidebar * doesn't collapse into skeletons. + * + * The identity keys alone are not enough: a save/publish mints a NEW inode + * under the SAME identifier and locale, so the keys stay equal while the + * version list has genuinely gone stale. Two extra signals cover that: + * - A list emptied back to INIT by `initializeExistingContent` (the + * full-screen in-place reload), which would otherwise render empty. + * - The live inode moving while NOT browsing history (the dialog host never + * re-initializes, so nothing clears or refetches on its own). + * `loadedLiveInode` only tracks the inode of the live version, so entering + * a historical/compare version — and returning from it — does not refetch. */ let loadedVersionsKey: string | null = null; + let loadedLiveInode: string | null = null; let loadedPushPublishIdentifier: string | null = null; effect(() => { const contentlet = store.contentlet(); + // An in-place reload empties the lists back to INIT while deliberately + // keeping the OUTGOING contentlet on screen (stale-while-revalidate), + // so acting on the cleared flag mid-reload would fetch the identifier + // we are leaving. Waiting for LOADING to clear is safe: the reload + // patches `contentlet` and `state: LOADED` together while the statuses + // are still INIT, so this fires on that same pass. + // The loaders themselves only ever move LOADING -> LOADED/ERROR, so + // reading these cannot re-trigger them. + const isReloading = store.state() === ComponentStatus.LOADING; + const versionsCleared = + !isReloading && store.versionsStatus().status === ComponentStatus.INIT; + const pushPublishCleared = + !isReloading && + store.pushPublishHistoryStatus().status === ComponentStatus.INIT; + // Only historical view swaps `contentlet` for an older version. Compare + // view leaves `contentlet` live (it only sets `compareContentlet`), so it + // must NOT suppress the new-version check — otherwise publishing while + // comparing would leave the list stale for the rest of the session. + const isViewingHistoricalVersion = store.isViewingHistoricalVersion(); untracked(() => { // Only load data if we have a contentlet with an identifier @@ -777,16 +807,26 @@ export function withHistory() { } const versionsKey = `${contentlet.identifier}:${contentlet.languageId}`; - if (versionsKey !== loadedVersionsKey) { + const identityChanged = versionsKey !== loadedVersionsKey; + // A workflow action (save/publish/restore) moved the live version + // forward. Skipped on the first pass, where there is no baseline yet. + const newLiveVersion = + !isViewingHistoricalVersion && + loadedLiveInode !== null && + contentlet.inode !== loadedLiveInode; + + if (identityChanged || versionsCleared || newLiveVersion) { const isInitialLoad = loadedVersionsKey === null; loadedVersionsKey = versionsKey; - if (!isInitialLoad) { + if (identityChanged && !isInitialLoad) { // The content identity (locale or identifier) changed under an // active compare/historical session — that state belongs to the // previous context, so discard it. Otherwise a stale // originalContentlet could later restore the previous locale's // content when exiting compare or historical view. + // Skipped on a same-identity refetch: there the compare session + // is still valid and must survive the reload. patchState(store, { compareContentlet: null, historicalVersionInode: null, @@ -801,13 +841,23 @@ export function withHistory() { }); } - if (contentlet.identifier !== loadedPushPublishIdentifier) { + if ( + contentlet.identifier !== loadedPushPublishIdentifier || + pushPublishCleared + ) { loadedPushPublishIdentifier = contentlet.identifier; store.loadPushPublishHistory({ identifier: contentlet.identifier, page: 1 }); } + + // Only the live version updates the baseline, so returning from a + // historical version lands back on a known inode instead of looking + // like a brand-new one. + if (!isViewingHistoricalVersion) { + loadedLiveInode = contentlet.inode; + } }); }); diff --git a/core-web/libs/edit-content/src/lib/store/features/information/information.feature.spec.ts b/core-web/libs/edit-content/src/lib/store/features/information/information.feature.spec.ts new file mode 100644 index 000000000000..27db6cce7b4c --- /dev/null +++ b/core-web/libs/edit-content/src/lib/store/features/information/information.feature.spec.ts @@ -0,0 +1,158 @@ +/* eslint-disable @typescript-eslint/no-explicit-any */ +import { patchState, signalStore, withState } from '@ngrx/signals'; +import { createServiceFactory, SpectatorService, SpyObject } from '@openng/spectator/jest'; +import { of, throwError } from 'rxjs'; + +import { HttpErrorResponse } from '@angular/common/http'; +import { fakeAsync, tick } from '@angular/core/testing'; + +import { delay } from 'rxjs/operators'; + +import { DotHttpErrorManagerService } from '@dotcms/data-access'; +import { HttpCode } from '@dotcms/dotcms-js'; +import { ComponentStatus, DotCMSContentlet } from '@dotcms/dotcms-models'; + +import { withInformation } from './information.feature'; + +import { DotEditContentService } from '../../../services/dot-edit-content.service'; +import { initialRootState } from '../../edit-content.store'; + +describe('Information Feature Store', () => { + let spectator: SpectatorService; + let store: any; + let dotHttpErrorManagerService: SpyObject; + let dotEditContentService: SpyObject; + + const mockContentlet = { + identifier: 'test-identifier', + inode: 'test-inode', + languageId: 1 + } as DotCMSContentlet; + + const createStore = createServiceFactory({ + service: signalStore(withState(initialRootState), withInformation()), + mocks: [DotEditContentService, DotHttpErrorManagerService] + }); + + beforeEach(() => { + spectator = createStore(); + store = spectator.service; + dotHttpErrorManagerService = spectator.inject(DotHttpErrorManagerService); + dotEditContentService = spectator.inject(DotEditContentService); + }); + + describe('isLoadingInformation', () => { + it('should be true only while the reference pages request is in flight', fakeAsync(() => { + dotEditContentService.getReferencePages.mockReturnValue(of(3).pipe(delay(100))); + + expect(store.isLoadingInformation()).toBe(false); + + store.getReferencePages('test-identifier'); + expect(store.isLoadingInformation()).toBe(true); + + tick(100); + expect(store.isLoadingInformation()).toBe(false); + })); + }); + + describe('getReferencePages', () => { + it('should set loading state and update relatedContent on success', fakeAsync(() => { + dotEditContentService.getReferencePages.mockReturnValue(of(3).pipe(delay(100))); + + store.getReferencePages('test-identifier'); + + expect(store.information()).toEqual({ + status: ComponentStatus.LOADING, + error: null, + relatedContent: '0' + }); + + tick(100); + + expect(store.information()).toEqual({ + status: ComponentStatus.LOADED, + error: null, + relatedContent: '3' + }); + })); + + it('should handle errors', fakeAsync(() => { + const httpError = new HttpErrorResponse({ + status: HttpCode.SERVER_ERROR, + statusText: 'Server Error', + error: { message: 'Backend error message' } + }); + dotEditContentService.getReferencePages.mockReturnValue(throwError(() => httpError)); + + store.getReferencePages('test-identifier'); + tick(); + + expect(store.information().status).toBe(ComponentStatus.ERROR); + expect(dotHttpErrorManagerService.handle).toHaveBeenCalledTimes(1); + })); + }); + + describe('Automatic Reference Pages Loading Effect', () => { + beforeEach(() => { + dotEditContentService.getReferencePages.mockReturnValue(of(2)); + }); + + it('should automatically load reference pages when the contentlet is set', fakeAsync(() => { + patchState(store, { contentlet: mockContentlet }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getReferencePages).toHaveBeenCalledWith('test-identifier'); + expect(store.information().relatedContent).toBe('2'); + })); + + it('should automatically reload when a save mints a new inode under the same identifier', fakeAsync(() => { + patchState(store, { contentlet: mockContentlet }); + spectator.flushEffects(); + tick(); + dotEditContentService.getReferencePages.mockClear(); + + patchState(store, { contentlet: { ...mockContentlet, inode: 'new-inode-after-save' } }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getReferencePages).toHaveBeenCalledWith('test-identifier'); + })); + + it('should not load reference pages when the sidebar is closed', fakeAsync(() => { + patchState(store, { + contentlet: mockContentlet, + uiState: { ...store.uiState(), isSidebarOpen: false } + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getReferencePages).not.toHaveBeenCalled(); + })); + + it('should not load reference pages when there is no contentlet identifier', fakeAsync(() => { + patchState(store, { contentlet: null }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getReferencePages).not.toHaveBeenCalled(); + })); + + it('should not reload reference pages on unrelated uiState changes', fakeAsync(() => { + patchState(store, { contentlet: mockContentlet }); + spectator.flushEffects(); + tick(); + dotEditContentService.getReferencePages.mockClear(); + + // Every uiState writer replaces the slice wholesale, so the effect must + // depend on the isSidebarOpen leaf rather than the object. + patchState(store, { + uiState: { ...store.uiState(), activeSidebarTab: 2, view: 'form' } + }); + spectator.flushEffects(); + tick(); + + expect(dotEditContentService.getReferencePages).not.toHaveBeenCalled(); + })); + }); +}); diff --git a/core-web/libs/edit-content/src/lib/store/features/information/information.feature.ts b/core-web/libs/edit-content/src/lib/store/features/information/information.feature.ts index 5ab911e07ccd..d8be5f72c5c4 100644 --- a/core-web/libs/edit-content/src/lib/store/features/information/information.feature.ts +++ b/core-web/libs/edit-content/src/lib/store/features/information/information.feature.ts @@ -1,10 +1,17 @@ import { tapResponse } from '@ngrx/operators'; -import { patchState, signalStoreFeature, type, withComputed, withMethods } from '@ngrx/signals'; +import { + patchState, + signalStoreFeature, + type, + withComputed, + withHooks, + withMethods +} from '@ngrx/signals'; import { rxMethod } from '@ngrx/signals/rxjs-interop'; import { pipe } from 'rxjs'; import { HttpErrorResponse } from '@angular/common/http'; -import { computed, inject } from '@angular/core'; +import { computed, effect, inject, untracked } from '@angular/core'; import { switchMap, tap } from 'rxjs/operators'; @@ -73,6 +80,35 @@ export function withInformation() { ) ) }) - ) + ), + withHooks({ + onInit(store) { + /** + * Reloads reference pages whenever the contentlet changes, store-owned + * so it works regardless of which component (or host — dialog vs + * full-screen route) happens to be mounted at the time. Depends on the + * whole contentlet (not just the identifier) so it also refreshes after + * a save/publish, which mints a NEW inode under the SAME identifier. + * Gated on `isSidebarOpen` to avoid firing on every edit-content load + * when the user never opens the sidebar. + * + * Reads the `isSidebarOpen` leaf rather than `uiState()`: every writer + * replaces that slice wholesale, so depending on the object would refetch + * on unrelated UI changes — including the `view` flip that `loadVersions` + * itself performs. + */ + effect(() => { + const contentlet = store.contentlet(); + const identifier = contentlet?.identifier; + const isSidebarOpen = store.uiState.isSidebarOpen(); + + untracked(() => { + if (identifier && isSidebarOpen) { + store.getReferencePages(identifier); + } + }); + }); + } + }) ); }