[SDESK-7811] Fix mark for desks in authoring-react (#5229)

* Fix mark for desks in authoring-react

The modal posted a plural marked_desks array, but the backend endpoint
only accepts a single marked_desk per request and treats each POST as a
toggle. So marking never worked.

The modal now toggles one desk per selection change instead of waiting
for a Save button, matching the toggle endpoint and the legacy Angular
behaviour. Both the modal and the popover patch authoring state in place
after the toggle resolves, since reloading the article would race with
the archive index refresh and read back stale data.

Also forwards data-test-value through Spacer and adds an e2e spec.

SDESK-7811

* Harden mark for desks: clear-all, deleted desks, request failures

Address review feedback on the mark-for-desks fix:
- The selection handler assumed TreeSelect only ever changes one desk at a
  time, but its clear-all button empties the whole selection in a single
  change. Diff the full previous and next id sets and toggle every change,
  then reconcile local state and authoring data from the toggles that
  actually persisted so a partial failure cannot desync the UI.
- Surface a notify.error when a toggle request fails (modal and popover);
  previously failures were silent unhandled rejections.
- A marked desk can be deleted while the article keeps the stale id. Add a
  shared resolveDesks helper that drops missing ids so the modal and popover
  no longer render undefined entries (the popover crashed on destructure).

Adds a unit spec for resolveDesks and an e2e case asserting clear-all
unmarks every desk.

* Address follow-up review on mark for desks

- Popover: compute the next marks inside the toggle's success handler so fast
  repeated removals do not start from stale props and reintroduce a removed desk.
- Modal: build the persisted list in the user's selection order, appending any
  desks that stayed marked because their unmark failed, so chips do not reorder.

Author: eos87

e2e/client/playwright/mark-for-desks.authoring-react.spec.ts

@@ -0,0 +1,88 @@
+import {test, expect} from '@playwright/test';
+import {Authoring} from './page-object-models/authoring';
+import {Monitoring} from './page-object-models/monitoring';
+import {restoreDatabaseSnapshot, s} from './utils';
+import {getStorageState} from './utils/storage-state';
+import {TreeSelectDriver} from './utils/tree-select-driver';
+
+test.use({
+    storageState: getStorageState({}, {authoringReact: true}),
+});
+
+test('mark for desks: per-action toggle via modal (authoring-react)', async ({page}) => {
+    await restoreDatabaseSnapshot();
+    const monitoring = new Monitoring(page);
+    const authoring = new Authoring(page);
+
+    await page.goto('/#/workspace/monitoring');
+    await monitoring.selectDeskOrWorkspace('Sports');
+
+    await monitoring.executeActionOnMonitoringItem(
+        page.locator(s('monitoring-group=Sports / Working Stage', 'article-item=test sports story')),
+        'Edit',
+    );
+
+    await authoring.waitForAuthoringReactToInitialize();
+
+    // No desks marked initially: the topbar bell widget should be absent.
+    const bell = page.locator('#marked-for-desks');
+
+    await expect(bell).toHaveCount(0);
+
+    await page.getByRole('button', {name: 'Actions menu'}).click();
+    await page.getByText('Marked for desks', {exact: true}).click();
+
+    const modal = page.locator(s('modal-mark-for-desks'));
+
+    await expect(modal).toBeVisible();
+
+    // Selecting a desk fires the toggle endpoint immediately. No Save button.
+    await new TreeSelectDriver(page, modal.locator(s('desks-select'))).addValues('Finance');
+
+    await expect(bell).toBeVisible();
+
+    // Clearing the chip fires the toggle again, unmarking Finance.
+    await new TreeSelectDriver(page, modal.locator(s('desks-select'))).setValues();
+
+    await expect(bell).toHaveCount(0);
+});
+
+test('mark for desks: clear-all unmarks every desk, not just one (authoring-react)', async ({page}) => {
+    await restoreDatabaseSnapshot();
+    const monitoring = new Monitoring(page);
+    const authoring = new Authoring(page);
+
+    await page.goto('/#/workspace/monitoring');
+    await monitoring.selectDeskOrWorkspace('Sports');
+
+    await monitoring.executeActionOnMonitoringItem(
+        page.locator(s('monitoring-group=Sports / Working Stage', 'article-item=test sports story')),
+        'Edit',
+    );
+
+    await authoring.waitForAuthoringReactToInitialize();
+
+    const bell = page.locator('#marked-for-desks');
+
+    await expect(bell).toHaveCount(0);
+
+    await page.getByRole('button', {name: 'Actions menu'}).click();
+    await page.getByText('Marked for desks', {exact: true}).click();
+
+    const modal = page.locator(s('modal-mark-for-desks'));
+
+    await expect(modal).toBeVisible();
+
+    const desksSelect = new TreeSelectDriver(page, modal.locator(s('desks-select')));
+
+    await desksSelect.addValues('Finance', 'Education');
+
+    await expect(bell).toBeVisible();
+
+    // Clear-all empties the whole selection in one change. The handler must toggle every marked
+    // desk, not just the first one, so the article ends up with no marked desks.
+    await desksSelect.setValues();
+
+    await expect(bell).toHaveCount(0);
+    expect(await desksSelect.getValues()).toEqual([]);
+});

scripts/apps/authoring-react/toolbar/mark-for-desks/helper.ts

@@ -1,12 +1,29 @@
+import {OrderedMap} from 'immutable';
+import {IDesk} from 'superdesk-api';
 import {httpRequestJsonLocal} from 'core/helpers/network';
 
-export function setMarkedDesks(deskIds: Array<string>, articleId: string) {
+/**
+ * The /marked_for_desks endpoint toggles a single desk per call: posting
+ * with an already-marked desk unmarks it. Callers must diff their selection
+ * and issue one request per change.
+ */
+export function toggleMarkedDesk(deskId: string, articleId: string) {
     return httpRequestJsonLocal({
         method: 'POST',
         path: '/marked_for_desks',
         payload: {
-            marked_desks: deskIds,
+            marked_desk: deskId,
             marked_item: articleId,
         },
     });
 }
+
+/**
+ * Turns marked desk ids into desk objects, dropping ids that no longer exist. A desk can be
+ * deleted while an article keeps the stale id, which would otherwise render as undefined.
+ */
+export function resolveDesks(deskIds: Array<string>, allDesks: OrderedMap<IDesk['_id'], IDesk>): Array<IDesk> {
+    return deskIds
+        .map((id) => allDesks.get(id))
+        .filter((desk): desk is IDesk => desk != null);
+}

scripts/apps/authoring-react/toolbar/mark-for-desks/mark-for-desks-modal.tsx

@@ -1,33 +1,109 @@
 import React from 'react';
 import {gettext} from 'core/utils';
 import {IArticle, IDesk} from 'superdesk-api';
-import {Alert, Button, Modal, TreeSelect} from 'superdesk-ui-framework/react';
+import {Modal, TreeSelect} from 'superdesk-ui-framework/react';
 import {sdApi} from 'api';
-import {nameof} from 'core/helpers/typescript-helpers';
-import {Spacer} from 'core/ui/components/Spacer';
-import {setMarkedDesks} from './helper';
+import {dispatchInternalEvent} from 'core/internal-events';
+import {notify} from 'core/notify/notify';
+import {resolveDesks, toggleMarkedDesk} from './helper';
+
+type IMarkedDesk = NonNullable<IArticle['marked_desks']>[number];
 
 interface IProps {
     article: IArticle;
     closeModal(): void;
 }
 
 interface IState {
-    selectedDesks: Array<string> | null;
+    selectedDesks: Array<IMarkedDesk>;
 }
 
 export class MarkForDesksModal extends React.PureComponent<IProps, IState> {
     constructor(props: IProps) {
         super(props);
 
         this.state = {
-            selectedDesks: this.props.article.marked_desks?.map((x) => x.desk_id),
+            selectedDesks: this.props.article.marked_desks ?? [],
+        };
+
+        this.handleSelectionChange = this.handleSelectionChange.bind(this);
+    }
+
+    private markFor(deskId: string): IMarkedDesk {
+        return {
+            desk_id: deskId,
+            user_marked: sdApi.user.getCurrentUserId(),
+            date_marked: new Date().toISOString(),
         };
     }
 
+    private handleSelectionChange(nextValue: Array<IDesk>): void {
+        const articleId = this.props.article._id;
+        const previousSelected = this.state.selectedDesks;
+        const previousIds = previousSelected.map((m) => m.desk_id);
+        const nextIds = nextValue.map((desk) => desk._id);
+
+        // TreeSelect gives us the whole next value (its clear-all empties everything at once), so
+        // toggle every added or removed desk, not just one.
+        const changedIds = [
+            ...nextIds.filter((id) => !previousIds.includes(id)),
+            ...previousIds.filter((id) => !nextIds.includes(id)),
+        ];
+
+        if (changedIds.length === 0) {
+            return;
+        }
+
+        const existingById = new Map(previousSelected.map((m) => [m.desk_id, m]));
+        const nextSelected = nextValue.map((desk) => existingById.get(desk._id) ?? this.markFor(desk._id));
+
+        this.setState({selectedDesks: nextSelected});
+
+        Promise.allSettled(
+            changedIds.map((deskId) => toggleMarkedDesk(deskId, articleId)),
+        ).then((results) => {
+            // Each toggle flips one desk. Apply only the ones that succeeded so the UI matches the server.
+            const persistedIds = new Set(previousIds);
+
+            changedIds.forEach((deskId, index) => {
+                if (results[index].status === 'rejected') {
+                    return;
+                }
+
+                if (persistedIds.has(deskId)) {
+                    persistedIds.delete(deskId);
+                    return;
+                }
+
+                persistedIds.add(deskId);
+            });
+
+            // Keep the user's order; put desks that stayed marked after a failed unmark at the end.
+            const persistedOrder = [
+                ...nextIds.filter((id) => persistedIds.has(id)),
+                ...Array.from(persistedIds).filter((id) => !nextIds.includes(id)),
+            ];
+            const persisted = persistedOrder.map((id) => existingById.get(id) ?? this.markFor(id));
+
+            // Patch state directly instead of reloading; a reload would race the index and read stale
+            // data. marked_desks has no editor input, so there are no unsaved edits to lose.
+            this.setState({selectedDesks: persisted});
+            dispatchInternalEvent('dangerouslyOverwriteAuthoringData', {
+                item: {
+                    _id: articleId,
+                    marked_desks: persisted,
+                },
+            });
+
+            if (results.some((result) => result.status === 'rejected')) {
+                notify.error(gettext('Some desks could not be updated.'));
+            }
+        });
+    }
+
     render(): JSX.Element {
         const allDesks = sdApi.desks.getAllDesks();
-        const selectedDesks = (this.state.selectedDesks ?? []).map((id) => allDesks.get(id));
+        const treeSelectValue = resolveDesks(this.state.selectedDesks.map((m) => m.desk_id), allDesks);
 
         return (
             <Modal
@@ -36,40 +112,19 @@ export class MarkForDesksModal extends React.PureComponent<IProps, IState> {
                 size="medium"
                 headerTemplate={gettext('Marked for desks')}
             >
-                <Spacer v gap="8">
+                <div data-test-id="modal-mark-for-desks">
                     <TreeSelect
                         kind="synchronous"
                         allowMultiple
                         label={gettext('Select desks')}
-                        value={selectedDesks}
-                        onChange={(value) => {
-                            this.setState({
-                                ...this.state,
-                                selectedDesks: value.map((desk) => desk._id),
-                            });
-                        }}
+                        value={treeSelectValue}
+                        onChange={this.handleSelectionChange}
                         getId={(desk) => desk.name}
                         getLabel={(desk) => desk.name}
                         getOptions={() => allDesks.toArray().map((item) => ({value: item}))}
+                        data-test-id="desks-select"
                     />
-
-                    <Spacer h gap="8" justifyContent="end" noWrap>
-                        <Button
-                            onClick={() => {
-                                setMarkedDesks(this.state.selectedDesks, this.props.article._id)
-                                    .then(() => this.props.closeModal());
-                            }}
-                            text={gettext('Save')}
-                            type="primary"
-                            style="filled"
-                        />
-                        <Button
-                            onClick={this.props.closeModal}
-                            text={gettext('Cancel')}
-                            style="hollow"
-                        />
-                    </Spacer>
-                </Spacer>
+                </div>
             </Modal>
         );
     }

scripts/apps/authoring-react/toolbar/mark-for-desks/mark-for-desks-popover.tsx

@@ -1,11 +1,12 @@
 import {sdApi} from 'api';
 import {dispatchInternalEvent} from 'core/internal-events';
+import {notify} from 'core/notify/notify';
 import {Spacer} from 'core/ui/components/Spacer';
 import {gettext} from 'core/utils';
 import React from 'react';
 import {IArticle} from 'superdesk-api';
 import {Button, Text} from 'superdesk-ui-framework/react';
-import {setMarkedDesks} from './helper';
+import {resolveDesks, toggleMarkedDesk} from './helper';
 
 interface IProps {
     article: IArticle;
@@ -19,27 +20,44 @@ export class MarkedDesks extends React.PureComponent<IProps> {
         this.getSelectedDeskIds = this.getSelectedDeskIds.bind(this);
     }
 
-    private getSelectedDeskIds(): Array<string> | null {
+    private getSelectedDeskIds(): Array<string> {
         return (this.props.article.marked_desks ?? []).map((x) => x.desk_id);
     }
 
     private unMarkDesks(deskId: string): void {
-        const deskIds = this.getSelectedDeskIds().filter((id) => id !== deskId);
+        const articleId = this.props.article._id;
 
-        setMarkedDesks(deskIds, this.props.article._id).then(() => {
-            this.setState({
-                selectedDeskIds: deskIds,
+        toggleMarkedDesk(deskId, articleId).then(() => {
+            // Read the marks here, not at call time, so fast repeated removals do not start from stale
+            // props and bring a removed desk back. Patch state directly instead of reloading; a reload
+            // would race the index and read stale data.
+            const nextMarks = (this.props.article.marked_desks ?? []).filter((m) => m.desk_id !== deskId);
+
+            dispatchInternalEvent('dangerouslyOverwriteAuthoringData', {
+                item: {
+                    _id: articleId,
+                    marked_desks: nextMarks,
+                },
             });
-            dispatchInternalEvent('dangerouslyForceReloadAuthoring', undefined);
+        }).catch(() => {
+            notify.error(gettext('Could not remove desk.'));
         });
     }
 
     render(): React.ReactNode {
         const allDesks = sdApi.desks.getAllDesks();
-        const selectedDesks = this.getSelectedDeskIds().map((id) => allDesks.get(id));
+        const selectedDesks = resolveDesks(this.getSelectedDeskIds(), allDesks);
 
         return selectedDesks.map(({name, _id}) => (
-            <Spacer gap="32" h key={_id} justifyContent="space-between" alignItems="stretch">
+            <Spacer
+                gap="32"
+                h
+                key={_id}
+                justifyContent="space-between"
+                alignItems="stretch"
+                data-test-id="marked-desk"
+                data-test-value={name}
+            >
                 <Text size="small">{name}</Text>
                 <Button
                     size="small"

scripts/apps/authoring-react/toolbar/mark-for-desks/tests/resolve-desks.spec.ts

@@ -0,0 +1,30 @@
+import {OrderedMap} from 'immutable';
+import {IDesk} from 'superdesk-api';
+import {resolveDesks} from '../helper';
+
+function desk(id: string, name: string): IDesk {
+    return {_id: id, name} as IDesk;
+}
+
+describe('resolveDesks', () => {
+    const allDesks = OrderedMap<IDesk['_id'], IDesk>([
+        ['desk-1', desk('desk-1', 'Sports')],
+        ['desk-2', desk('desk-2', 'Finance')],
+    ]);
+
+    it('resolves ids to desk objects, preserving order', () => {
+        expect(resolveDesks(['desk-2', 'desk-1'], allDesks).map((d) => d.name)).toEqual(['Finance', 'Sports']);
+    });
+
+    it('drops ids that no longer exist instead of returning undefined entries', () => {
+        const result = resolveDesks(['desk-1', 'deleted-desk', 'desk-2'], allDesks);
+
+        expect(result.map((d) => d.name)).toEqual(['Sports', 'Finance']);
+        expect(result.every((d) => d != null)).toBe(true);
+    });
+
+    it('returns an empty array when nothing matches', () => {
+        expect(resolveDesks(['gone'], allDesks)).toEqual([]);
+        expect(resolveDesks([], allDesks)).toEqual([]);
+    });
+});

scripts/core/superdesk-api.d.ts

@@ -2555,6 +2555,7 @@ declare module 'superdesk-api' {
         children: Array<React.ReactNode>;
 
         'data-test-id'?: string;
+        'data-test-value'?: string;
     }
 
     type ILiveQueryChildrenOptions<T> = {loading: true} | {loading: false; data: IRestApiResponse<T>};

scripts/core/ui/components/Spacer.tsx

@@ -22,6 +22,7 @@ export class Spacer extends React.PureComponent<IPropsSpacer> {
                     ...(this.props.style ?? {}),
                 }}
                 data-test-id={this.props['data-test-id']}
+                data-test-value={this.props['data-test-value']}
             >
                 {this.props.children.map((el, i) => noWrap ? el : (
                     <div