diff --git a/openmetadata-ui/src/main/resources/ui/eslint-suppressions.json b/openmetadata-ui/src/main/resources/ui/eslint-suppressions.json index eb6692be3c23..c15aac4d1217 100644 --- a/openmetadata-ui/src/main/resources/ui/eslint-suppressions.json +++ b/openmetadata-ui/src/main/resources/ui/eslint-suppressions.json @@ -460,7 +460,7 @@ }, "playwright/e2e/Features/Tasks/TaskComments.spec.ts": { "om-playwright/no-positional-locator": { - "count": 24 + "count": 2 } }, "playwright/e2e/Features/Tasks/TaskCreation.spec.ts": { diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/IncidentManager.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/IncidentManager.spec.ts index 6aff526f6469..a30bf8d2f6c7 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/IncidentManager.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/IncidentManager.spec.ts @@ -38,6 +38,7 @@ import { makeRetryRequest } from '../../utils/serviceIngestion'; import { sidebarClick } from '../../utils/sidebar'; import { waitForTaskResolveResponse } from '../../utils/task'; import { verifyTestCaseLastRunBanner } from '../../utils/testCases'; +import { clickAndWaitFor } from '../../utils/waitHelpers'; import { test } from '../fixtures/pages'; let user1: UserClass; @@ -1021,6 +1022,74 @@ test.describe('Incident Manager', PLAYWRIGHT_INGESTION_TAG_OBJ, () => { ).toBeVisible(); }); + /** + * Delete a comment from an incident's task tab + * @description #33112 was reported on the Incident Manager page, but the rest of + * the task-comment coverage exercises the activity-feed drawer only. This runs + * the same post-then-delete flow through TestCaseIncidentTab, which renders the + * task tab (and so CommentCard) rather than the drawer. + */ + test('Delete a task comment from the incident task tab', async ({ page }) => { + const testCase = table1.testCasesResponseData[0]; + const testCaseName = testCase?.['name'] as string; + + await visitProfilerTab(page, table1); + await waitForAllLoadersToDisappear(page); + + await page.getByTestId(testCaseName).getByText(testCaseName).click(); + await expect(page.getByTestId('entity-page-header')).toBeVisible(); + + await openIncidentTaskTab(page, true); + + const taskTab = page.getByTestId('task-tab'); + await expect(taskTab).toBeVisible(); + + // Post a comment to delete. Unique per run so the card can be matched by + // text rather than by position. + const message = `Incident tab comment ${Date.now()}`; + // The input is a trigger that opens the editor - it cannot be filled. + const commentInput = taskTab.getByTestId('comments-input-field'); + await expect(commentInput).toBeVisible(); + await commentInput.click(); + + const editor = taskTab.locator('[data-testid="editor-wrapper"] .ql-editor'); + await expect(editor).toBeVisible({ timeout: 15_000 }); + await editor.click(); + await editor.type(message); + + // Anchored so it cannot match the tab's own GET of the task with its comments. + const postResponse = await clickAndWaitFor( + page, + taskTab.getByTestId('send-button'), + /\/api\/v1\/tasks\/[^/]+\/comments$/ + ); + const postedTask = await postResponse.json(); + const comments = postedTask.comments ?? []; + const commentId = comments[comments.length - 1]?.id as string; + + const card = taskTab + .locator('[data-testid="feed-reply-card"]') + .filter({ hasText: message }); + await expect(card).toBeVisible(); + + // The affordance is revealed on hover but stays mounted, so it is present + // for the keyboard too - hovering here mirrors what a mouse user does. + await card.hover(); + + await card.getByTestId('delete-message').click(); + await clickAndWaitFor( + page, + page.getByTestId('save-button'), + new RegExp(`/comments/${commentId}$`) + ); + + await expect( + taskTab + .locator('[data-testid="feed-reply-card"]') + .filter({ hasText: message }) + ).toHaveCount(0); + }); + /** * Verify filters in Incident Manager page * @description Tests Assignee, Status, Test Case, and Date filters and confirms list updates accordingly. diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/TaskComments.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/TaskComments.spec.ts index 5ea5b922e3c2..8b60f12bcdaa 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/TaskComments.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/Tasks/TaskComments.spec.ts @@ -11,11 +11,18 @@ * limitations under the License. */ +import type { Locator, Page } from '@playwright/test'; import { TableClass } from '../../../support/entity/TableClass'; import { expect, test } from '../../../support/fixtures/base'; import { UserClass } from '../../../support/user/UserClass'; import { performAdminLogin } from '../../../utils/admin'; import { waitForPageLoaded } from '../../../utils/polling'; +import { + addCommentToTask, + CreatedTask, + openEntityTasksTab, + openTaskDetails, +} from '../../../utils/taskWorkflow'; /** * Task Comments Tests @@ -30,6 +37,7 @@ import { waitForPageLoaded } from '../../../utils/polling'; */ test.describe('Task Comments - Add Comment', () => { + let createdTask: CreatedTask; const adminUser = new UserClass(); const assigneeUser = new UserClass(); const commentingUser = new UserClass(); @@ -62,7 +70,8 @@ test.describe('Task Comments - Add Comment', () => { assignees: [assigneeUser.responseData.name], }, }); - const task = await taskResponse.json(); + createdTask = (await taskResponse.json()) as CreatedTask; + const task = createdTask; taskId = task.id; } finally { await afterAction(); @@ -86,141 +95,179 @@ test.describe('Task Comments - Add Comment', () => { await assigneeUser.login(page); await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); - - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + await openEntityTasksTab(page); // Click on task to open detail drawer - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); - - // Find comment input in drawer - const drawer = page.locator('.ant-drawer-content'); - - if (await drawer.isVisible()) { - const commentInput = drawer.locator( - '[data-testid="comment-input"], .ql-editor, [placeholder*="comment" i]' - ); - - if (await commentInput.isVisible()) { - await commentInput.fill('This is a test comment from assignee'); - - // Submit comment - const sendBtn = drawer.getByTestId('send-comment'); - if (await sendBtn.isVisible()) { - const commentResponse = page.waitForResponse( - (response) => - response.url().includes('/api/v1/tasks/') && - response.url().includes('/comments') - ); - await sendBtn.click(); - await commentResponse; - - // Verify comment appears - await expect( - drawer.getByText('This is a test comment from assignee') - ).toBeVisible(); - } - } - } - } + await openTaskDetails(page, createdTask); + + // Find comment input in drawer + const drawer = page.locator('#task-panel'); + + await expect(drawer).toBeVisible(); + await addCommentToTask(page, 'This is a test comment from assignee'); + + // Verify comment appears + await expect( + drawer.getByText('This is a test comment from assignee') + ).toBeVisible(); }); - test('non-assignee should be able to add comment', async ({ page }) => { - await commentingUser.login(page); + // Replaces a Jest assertion that could only check Tailwind class names: jsdom has + // no layout engine, so it could not have caught an actual reflow. Here the delete + // affordance is positioned out of flow, so revealing it on hover must not shift + // the comment body by a single pixel. + test('revealing the delete affordance on hover must not reflow the comment body', async ({ + page, + }) => { + await assigneeUser.login(page); await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); + await openEntityTasksTab(page); - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + // This describe seeds exactly one task against a fresh table, so the card can + // be addressed directly rather than by position - a positional locator would + // silently pick up a different task if the fixture ever grows. + await openTaskDetails(page, createdTask); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); + const drawer = page.locator('#task-panel'); + await expect(drawer).toBeVisible(); - const drawer = page.locator('.ant-drawer-content'); + const message = `Layout probe ${Date.now()}`; + await addCommentToTask(page, message); - if (await drawer.isVisible()) { - const commentInput = drawer.locator( - '[data-testid="comment-input"], .ql-editor, [placeholder*="comment" i]' - ); + const card = drawer + .locator('[data-testid="feed-reply-card"]') + .filter({ hasText: message }); + await expect(card).toBeVisible(); - if (await commentInput.isVisible()) { - await commentInput.fill('Comment from non-assignee user'); + const body = card.getByTestId('viewer-container'); - const sendBtn = drawer.getByTestId('send-comment'); - if (await sendBtn.isVisible()) { - await sendBtn.click(); - await waitForPageLoaded(page); + // The markdown preview measures itself and applies its own clamp one frame + // after mount, so sample until the box stops moving - otherwise this test + // measures that clamp rather than the hover it is meant to guard. + let previous = await body.boundingBox(); + await expect + .poll( + async () => { + const current = await body.boundingBox(); + const settled = JSON.stringify(current) === JSON.stringify(previous); + previous = current; - // Comment should be added or access denied - // (depends on permission model) - } - } - } - } + return settled; + }, + { timeout: 10_000 } + ) + .toBe(true); + + const before = previous; + + const actions = card.getByTestId('feed-actions'); + const deleteAction = card.getByTestId('delete-message'); + + // Posting the comment leaves the pointer over the card, which would hold + // the bar revealed - park it away first to sample the resting state. + await page.mouse.move(0, 0); + + // Mounted before any hover so it stays reachable by keyboard and screen + // readers - the reveal is opacity, which Playwright's visibility check + // deliberately ignores, so assert the computed value directly. + await expect(deleteAction).toBeAttached(); + await expect(actions).toHaveCSS('opacity', '0'); + + await card.hover(); + + await expect(actions).toHaveCSS('opacity', '1'); + await expect(deleteAction).toBeVisible(); + + const after = await body.boundingBox(); + + expect(after).toEqual(before); }); - test('admin should be able to add comment to any task', async ({ page }) => { - await adminUser.login(page); + test('the comment actions are reachable and operable by keyboard alone', async ({ + page, + }) => { + await assigneeUser.login(page); + await table.visitEntityPage(page); + + await openEntityTasksTab(page); + + await openTaskDetails(page, createdTask); + + const drawer = page.locator('#task-panel'); + await expect(drawer).toBeVisible(); + + const message = `Keyboard probe ${Date.now()}`; + await addCommentToTask(page, message); + + const card = drawer + .locator('[data-testid="feed-reply-card"]') + .filter({ hasText: message }); + await expect(card).toBeVisible(); + + const actions = card.getByTestId('feed-actions'); + const deleteAction = card.getByTestId('delete-message'); + + // Park the pointer away from the card first, so the reveal asserted below + // is attributable to focus-within and not to a leftover hover. + await page.mouse.move(0, 0); + + await expect(actions).toHaveCSS('opacity', '0'); + + // Regression coverage for the affordance being an `` span: + // it could not hold focus at all, so none of this was possible without a + // mouse. Focusing it must also bring the bar into view via focus-within. + await deleteAction.focus(); + + await expect(deleteAction).toBeFocused(); + await expect(actions).toHaveCSS('opacity', '1'); + + // Enter activates it, as it would any button. + await page.keyboard.press('Enter'); + + await expect(page.getByTestId('save-button')).toBeVisible(); + }); + + test('non-assignee should be able to add comment', async ({ page }) => { + await commentingUser.login(page); await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); + await openEntityTasksTab(page); + + await openTaskDetails(page, createdTask); + + const drawer = page.locator('#task-panel'); + + await expect(drawer).toBeVisible(); + await addCommentToTask(page, 'Comment from non-assignee user'); await waitForPageLoaded(page); - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + // Comment should be added or access denied + // (depends on permission model) + }); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); + test('admin should be able to add comment to any task', async ({ page }) => { + await adminUser.login(page); + await table.visitEntityPage(page); - const drawer = page.locator('.ant-drawer-content'); + await openEntityTasksTab(page); - if (await drawer.isVisible()) { - const commentInput = drawer.locator( - '[data-testid="comment-input"], .ql-editor, [placeholder*="comment" i]' - ); + await openTaskDetails(page, createdTask); - if (await commentInput.isVisible()) { - await commentInput.fill('Admin comment on task'); + const drawer = page.locator('#task-panel'); - const sendBtn = drawer.getByTestId('send-comment'); - if (await sendBtn.isVisible()) { - await sendBtn.click(); - await waitForPageLoaded(page); + await expect(drawer).toBeVisible(); + await addCommentToTask(page, 'Admin comment on task'); + await waitForPageLoaded(page); - await expect( - drawer.getByText('Admin comment on task') - ).toBeVisible(); - } - } - } - } + await expect(drawer.getByText('Admin comment on task')).toBeVisible(); }); }); test.describe('Task Comments - @Mention', () => { + let createdTask: CreatedTask; const adminUser = new UserClass(); const assigneeUser = new UserClass(); - const mentionedUser = new UserClass(); const table = new TableClass(); test.beforeAll('Setup test data', async ({ browser }) => { @@ -230,7 +277,6 @@ test.describe('Task Comments - @Mention', () => { await adminUser.create(apiContext); await adminUser.setAdminRole(apiContext); await assigneeUser.create(apiContext); - await mentionedUser.create(apiContext); await table.create(apiContext); await table.setOwner(apiContext, { @@ -238,7 +284,7 @@ test.describe('Task Comments - @Mention', () => { type: 'user', }); - await apiContext.post('/api/v1/tasks', { + const taskResponse = await apiContext.post('/api/v1/tasks', { data: { name: `Test Task - ${Date.now()}`, about: `<#E::table::${table.entityResponseData?.fullyQualifiedName}>`, @@ -247,6 +293,7 @@ test.describe('Task Comments - @Mention', () => { assignees: [assigneeUser.responseData.name], }, }); + createdTask = (await taskResponse.json()) as CreatedTask; } finally { await afterAction(); } @@ -257,7 +304,6 @@ test.describe('Task Comments - @Mention', () => { try { await table.delete(apiContext); - await mentionedUser.delete(apiContext); await assigneeUser.delete(apiContext); await adminUser.delete(apiContext); } finally { @@ -269,44 +315,37 @@ test.describe('Task Comments - @Mention', () => { await adminUser.login(page); await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); + await openEntityTasksTab(page); - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + await openTaskDetails(page, createdTask); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); + const drawer = page.locator('#task-panel'); - const drawer = page.locator('.ant-drawer-content'); + await expect(drawer).toBeVisible(); + const commentTrigger = drawer.getByTestId('comments-input-field'); - if (await drawer.isVisible()) { - const commentInput = drawer.locator( - '[data-testid="comment-input"], .ql-editor, [contenteditable="true"]' - ); + await expect(commentTrigger).toBeVisible(); + await commentTrigger.click(); - if (await commentInput.isVisible()) { - await commentInput.click(); - await page.keyboard.type('@'); - await waitForPageLoaded(page); + const commentInput = drawer.locator( + '[data-testid="editor-wrapper"] .ql-editor' + ); - // Should show mention dropdown - const mentionDropdown = page.locator( - '.mention-dropdown, .ql-mention-list-container, [data-testid="mention-suggestions"]' - ); + await expect(commentInput).toBeVisible({ timeout: 15_000 }); + await commentInput.click(); + await page.keyboard.type('@'); + await waitForPageLoaded(page); - await mentionDropdown - .first() - .waitFor({ state: 'visible', timeout: 2000 }) - .catch(() => undefined); - } - } - } + // Should show mention dropdown + const mentionDropdown = page.locator( + '.mention-dropdown, .ql-mention-list-container, [data-testid="mention-suggestions"]' + ); + + // The suggestion list is populated asynchronously, so this needs a wait - but + // it must actually arrive. A swallowed waitFor left this test asserting + // nothing, so it passed whether or not the dropdown ever rendered. + await expect(mentionDropdown).toHaveCount(1, { timeout: 10_000 }); + await expect(mentionDropdown).toBeVisible(); }); test('selecting user from @ dropdown should add mention', async ({ @@ -315,61 +354,53 @@ test.describe('Task Comments - @Mention', () => { await adminUser.login(page); await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); + await openEntityTasksTab(page); - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + await openTaskDetails(page, createdTask); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); - - const drawer = page.locator('.ant-drawer-content'); - - if (await drawer.isVisible()) { - const commentInput = drawer.locator( - '[data-testid="comment-input"], .ql-editor, [contenteditable="true"]' - ); - - if (await commentInput.isVisible()) { - await commentInput.click(); - - // Type @ and part of username - await page.keyboard.type(`@${mentionedUser.responseData.name}`); - - // Select from dropdown if visible - const mentionItem = page.locator( - `.mention-item, .ql-mention-list-item:has-text("${mentionedUser.responseData.displayName}")` - ); - await mentionItem - .first() - .waitFor({ state: 'visible', timeout: 2000 }) - .catch(() => undefined); - - if (await mentionItem.isVisible()) { - await mentionItem.click(); - - // Continue typing and submit - await page.keyboard.type(' please review this task'); - - const sendBtn = drawer.getByTestId('send-comment'); - if (await sendBtn.isVisible()) { - await sendBtn.click(); - await waitForPageLoaded(page); - } - } - } - } - } + const drawer = page.locator('#task-panel'); + + await expect(drawer).toBeVisible(); + const commentTrigger = drawer.getByTestId('comments-input-field'); + + await expect(commentTrigger).toBeVisible(); + await commentTrigger.click(); + + const commentInput = drawer.locator( + '[data-testid="editor-wrapper"] .ql-editor' + ); + + await expect(commentInput).toBeVisible({ timeout: 15_000 }); + await commentInput.click(); + + // Mention the seeded `admin` account rather than a user created in this + // describe's beforeAll: the suggestion list is search-index backed, and a + // seconds-old user is not reliably queryable yet. What this test guards is + // that picking from the dropdown inserts a mention, which any indexed user + // exercises identically. + const mentionTarget = 'admin'; + + // quill-mention only opens on a real keystroke - `fill()` sets the text in + // one shot and the module never sees the denotation char. + await commentInput.click(); + await page.keyboard.type(`@${mentionTarget}`); + + const mentionItem = page.locator(`[data-value="@${mentionTarget}"]`); + + await expect(mentionItem.first()).toBeVisible({ timeout: 15_000 }); + await mentionItem.first().click(); + + await page.keyboard.type(' please review this task'); + + const sendBtn = drawer.getByTestId('send-button'); + await expect(sendBtn).toBeEnabled(); + await sendBtn.click(); + await waitForPageLoaded(page); }); }); test.describe('Task Comments - Edit/Delete', () => { + let createdTask: CreatedTask; const adminUser = new UserClass(); const assigneeUser = new UserClass(); const table = new TableClass(); @@ -391,16 +422,14 @@ test.describe('Task Comments - Edit/Delete', () => { // Create task with comment const taskResponse = await apiContext.post('/api/v1/tasks', { data: { - about: { - type: 'table', - id: table.entityResponseData?.id, - fullyQualifiedName: table.entityResponseData?.fullyQualifiedName, - }, - type: 'RequestDescription', - assignees: [{ id: assigneeUser.responseData.id, type: 'user' }], + about: `<#E::table::${table.entityResponseData?.fullyQualifiedName}>`, + type: 'DescriptionUpdate', + category: 'MetadataUpdate', + assignees: [assigneeUser.responseData.name], }, }); - const task = await taskResponse.json(); + createdTask = (await taskResponse.json()) as CreatedTask; + const task = createdTask; // Add a comment await apiContext.post(`/api/v1/tasks/${task.id}/comments`, { @@ -429,187 +458,368 @@ test.describe('Task Comments - Edit/Delete', () => { await adminUser.login(page); await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); - - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + await openEntityTasksTab(page); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); + await openTaskDetails(page, createdTask); - const drawer = page.locator('.ant-drawer-content'); + const drawer = page.locator('#task-panel'); - if (await drawer.isVisible()) { - // Find comment - const comment = drawer.locator( - '[data-testid="comment-item"], .task-comment' - ); + await expect(drawer).toBeVisible(); - if (await comment.first().isVisible()) { - // Hover to show actions - await comment.first().hover(); + // Post a comment of our own so the card can be addressed by its text rather + // than by position - the drawer already holds comments from earlier tests in + // this serial describe. + const message = `Author actions ${Date.now()}`; + await addCommentToTask(page, message); - // Look for edit/delete buttons - const editBtn = comment.first().getByTestId('edit-comment'); - const deleteBtn = comment.first().getByTestId('delete-comment'); + const comment = drawer + .locator('[data-testid="feed-reply-card"]') + .filter({ hasText: message }); + await expect(comment).toHaveCount(1); + await comment.hover(); - // Author should see these buttons - // (depends on UI implementation) - } - } - } + // The author may both edit and delete their own comment. + await expect(comment.getByTestId('edit-message')).toBeVisible(); + await expect(comment.getByTestId('delete-message')).toBeVisible(); }); test('should be able to edit own comment', async ({ page }) => { await adminUser.login(page); await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); + await openEntityTasksTab(page); - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + await openTaskDetails(page, createdTask); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); + const drawer = page.locator('#task-panel'); - const drawer = page.locator('.ant-drawer-content'); + await expect(drawer).toBeVisible(); + // Only the author may edit, so this test has to own the comment it edits + // rather than reaching for whatever card happens to be first. + const original = `Original comment ${Date.now()}`; + await addCommentToTask(page, original); - if (await drawer.isVisible()) { - const comment = drawer.locator( - '[data-testid="comment-item"], .task-comment' - ); + const comment = drawer + .locator('[data-testid="feed-reply-card"]') + .filter({ hasText: original }); - if (await comment.first().isVisible()) { - await comment.first().hover(); + await expect(comment).toBeVisible(); + await comment.hover(); - const editBtn = comment.first().getByTestId('edit-comment'); + const editBtn = comment.getByTestId('edit-message'); - if (await editBtn.isVisible()) { - await editBtn.click(); + await expect(editBtn).toBeVisible(); + await editBtn.click(); - // Edit comment text - const editInput = drawer.locator( - '[data-testid="edit-comment-input"]' - ); - if (await editInput.isVisible()) { - await editInput.fill('Updated comment text'); + // Scoped to the editing card, not to `comment` - that locator filters on + // the original text, which stops matching the moment the editor is + // refilled. Qualifying with feed-reply-card keeps the panel's own comment + // composer, which is the same editor component, out of the match. + const editor = drawer.locator( + '[data-testid="feed-reply-card"] [data-testid="activity-feed-editor-new"]' + ); + const editInput = editor.locator('.ql-editor'); - const saveBtn = drawer.getByTestId('save-comment'); - await saveBtn.click(); - await waitForPageLoaded(page); + await expect(editInput).toBeVisible(); + await editInput.fill('Updated comment text'); - await expect( - drawer.getByText('Updated comment text') - ).toBeVisible(); - } - } - } - } - } + const saveBtn = editor.getByTestId('send-button'); + await saveBtn.click(); + await waitForPageLoaded(page); + + await expect(drawer.getByText('Updated comment text')).toBeVisible(); }); - test('should be able to delete own comment', async ({ page }) => { - await adminUser.login(page); + /** + * Shared by the two real delete tests below: opens the task's activity-feed + * drawer as `user` and posts one comment from there, returning the task's id + * (needed to match the DELETE response) and the comment's text (needed to + * find the right `feed-reply-card`). + */ + const postCommentAsUser = async (page: Page, message: string) => { await table.visitEntityPage(page); + await openEntityTasksTab(page); + + await openTaskDetails(page, createdTask); + + const drawer = page.locator('#task-panel'); + await expect(drawer).toBeVisible(); + + // The input is a trigger that opens the editor - it cannot be filled. + const commentInput = drawer.getByTestId('comments-input-field'); + await expect(commentInput).toBeVisible(); + await commentInput.click(); + + const editor = drawer.locator('[data-testid="editor-wrapper"] .ql-editor'); + await expect(editor).toBeVisible({ timeout: 15_000 }); + await editor.click(); + await editor.type(message); + + const sendBtn = drawer.getByTestId('send-button'); + const commentResponsePromise = page.waitForResponse( + (response) => + response.url().includes('/api/v1/tasks/') && + response.url().includes('/comments') && + response.request().method() === 'POST' + ); + await sendBtn.click(); + const commentResponse = await commentResponsePromise; + // POST /tasks/{id}/comments returns the updated Task, not the new comment, so + // the comment's own id has to come from the task's comments array. The server + // appends, so the new comment is the last entry. + const task = await commentResponse.json(); + const comments = task.comments ?? []; + const taskCommentId = comments[comments.length - 1]?.id as string; + + await expect(drawer.getByText(message)).toBeVisible(); + + return { drawer, taskCommentId }; + }; + + /** + * Deletes the comment identified by `message` from an already-open drawer, + * waiting for the real DELETE response and asserting the comment is gone + * from the DOM afterwards. The hover is required, not just realistic: the + * shared feed actions only mount while the card is hovered. + */ + const deleteCommentViaUi = async ( + page: Page, + drawer: Locator, + message: string, + taskCommentId: string + ) => { + const commentCard = drawer + .getByTestId('feed-reply-card') + .filter({ hasText: message }); + await expect(commentCard).toBeVisible(); + + await commentCard.hover(); + await commentCard.getByTestId('delete-message').click(); + + // Asserted on the confirm button rather than the modal container: antd's + // Modal does not forward `data-testid` to the rendered DOM. + const confirmButton = page.getByTestId('save-button'); + await expect(confirmButton).toBeVisible(); + + const deleteResponsePromise = page.waitForResponse( + (response) => + response.url().includes(`/comments/${taskCommentId}`) && + response.request().method() === 'DELETE' + ); + await confirmButton.click(); + const deleteResponse = await deleteResponsePromise; + + expect(deleteResponse.ok()).toBe(true); + await expect(commentCard).not.toBeVisible(); + await expect(drawer.getByText(message)).not.toBeVisible(); + }; - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); + test('should be able to delete own comment', async ({ page }) => { + // assigneeUser is a regular (non-admin) user, so a successful delete here + // exercises the author-match branch of canDelete, not the admin override. + await assigneeUser.login(page); - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + const message = `Author-deletable comment ${Date.now()}`; + const { drawer, taskCommentId } = await postCommentAsUser(page, message); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); + await deleteCommentViaUi(page, drawer, message, taskCommentId); + }); - const drawer = page.locator('.ant-drawer-content'); + test('admin should be able to delete a comment they did not author', async ({ + page, + browser, + }) => { + // Post as the non-admin assignee first, in a separate browser context so + // this test doesn't depend on execution order relative to the one above. + const authorContext = await browser.newContext(); + const authorPage = await authorContext.newPage(); + await assigneeUser.login(authorPage); - if (await drawer.isVisible()) { - const comments = drawer.locator( - '[data-testid="comment-item"], .task-comment' - ); - const initialCount = await comments.count(); + const message = `Admin-deletable comment ${Date.now()}`; + const { taskCommentId } = await postCommentAsUser(authorPage, message); + await authorContext.close(); - if (initialCount > 0) { - await comments.first().hover(); + await adminUser.login(page); + const { drawer } = await postCommentAsUser(page, `unused-${Date.now()}`); + // postCommentAsUser leaves an extra comment behind as a side effect of + // reusing it purely for its navigation-to-the-open-drawer behavior; that + // extra comment isn't asserted on and is left for afterAll to clean up + // along with the rest of the table. + + await deleteCommentViaUi(page, drawer, message, taskCommentId); + }); - const deleteBtn = comments.first().getByTestId('delete-comment'); + test('non-author, non-admin should not see the delete option', async ({ + page, + browser, + }) => { + const otherUser = new UserClass(); + const { apiContext, afterAction } = await performAdminLogin(browser); + try { + await otherUser.create(apiContext); + } finally { + await afterAction(); + } - if (await deleteBtn.isVisible()) { - await deleteBtn.click(); + try { + const authorContext = await browser.newContext(); + const authorPage = await authorContext.newPage(); + await assigneeUser.login(authorPage); + + const message = `Not-my-comment ${Date.now()}`; + await postCommentAsUser(authorPage, message); + await authorContext.close(); + + await otherUser.login(page); + const { drawer } = await postCommentAsUser( + page, + `viewer-comment-${Date.now()}` + ); - // Confirm deletion - const confirmBtn = page.getByRole('button', { - name: /confirm|yes|delete/i, - }); - if (await confirmBtn.isVisible()) { - await confirmBtn.click(); - await waitForPageLoaded(page); + const commentCard = drawer + .getByTestId('feed-reply-card') + .filter({ hasText: message }); + await expect(commentCard).toBeVisible(); + await commentCard.hover(); - // Comment count should decrease - const newCount = await comments.count(); - expect(newCount).toBeLessThan(initialCount); - } - } - } + await expect(commentCard.getByTestId('delete-message')).not.toBeVisible(); + } finally { + const { apiContext, afterAction } = await performAdminLogin(browser); + try { + await otherUser.delete(apiContext); + } finally { + await afterAction(); } } }); - test('non-author should not see edit/delete options', async ({ page }) => { + test('should be able to delete a comment from inside the activity-feed drawer', async ({ + page, + }) => { + // Regression coverage for the confirmation-modal/antd-Drawer z-index + // conflict: TaskTabNew (and so CommentCard's confirmation modal) is rendered + // inside an antd Drawer here, unlike the standalone task page used by the + // other delete tests above. If the confirmation dialog's overlay ever + // sits below the Drawer's own mask again, this click lands on the mask + // (which closes the drawer) instead of the dialog's confirm button, and + // this test will hang/time out waiting for the DELETE response instead + // of silently passing. await assigneeUser.login(page); - await table.visitEntityPage(page); - await page.getByTestId('activity_feed').click(); - await waitForPageLoaded(page); + const message = `Drawer-delete comment ${Date.now()}`; + const { drawer, taskCommentId } = await postCommentAsUser(page, message); - const tasksTab = page.getByRole('menuitem', { name: /tasks/i }); - if (await tasksTab.isVisible()) { - await tasksTab.click(); - await waitForPageLoaded(page); - } + await expect(page.locator('#task-panel')).toBeVisible(); - const taskCard = page.locator('[data-testid="task-feed-card"]').first(); - if (await taskCard.isVisible()) { - await taskCard.click(); - await waitForPageLoaded(page); + await deleteCommentViaUi(page, drawer, message, taskCommentId); + }); +}); - const drawer = page.locator('.ant-drawer-content'); +test.describe('Task Comments - Long Comment Overflow', () => { + let createdTask: CreatedTask; + const assigneeUser = new UserClass(); + const table = new TableClass(); - if (await drawer.isVisible()) { - // Find comment from admin (not assignee) - const comment = drawer.locator( - '[data-testid="comment-item"], .task-comment' - ); + test.beforeAll('Setup test data', async ({ browser }) => { + const { apiContext, afterAction } = await performAdminLogin(browser); - if (await comment.first().isVisible()) { - await comment.first().hover(); + try { + await assigneeUser.create(apiContext); - // Non-author should NOT see edit/delete buttons for others' comments - const editBtn = comment.first().getByTestId('edit-comment'); - const deleteBtn = comment.first().getByTestId('delete-comment'); + await table.create(apiContext); + await table.setOwner(apiContext, { + id: assigneeUser.responseData.id, + type: 'user', + }); - // These should not be visible (or should be for own comments only) - } - } + const taskResponse = await apiContext.post('/api/v1/tasks', { + data: { + about: `<#E::table::${table.entityResponseData?.fullyQualifiedName}>`, + type: 'DescriptionUpdate', + category: 'MetadataUpdate', + assignees: [assigneeUser.responseData.name], + }, + }); + createdTask = (await taskResponse.json()) as CreatedTask; + } finally { + await afterAction(); + } + }); + + test.afterAll('Cleanup test data', async ({ browser }) => { + const { apiContext, afterAction } = await performAdminLogin(browser); + + try { + await table.delete(apiContext); + await assigneeUser.delete(apiContext); + } finally { + await afterAction(); } }); + + test('a long comment shows a working View More / View Less toggle instead of being silently clamped', async ({ + page, + }) => { + // Regression coverage for the task tab's comment preview: comments longer + // than DESCRIPTION_MAX_PREVIEW_CHARACTERS are trimmed, so one that + // overflows needs the toggle rendered to stay readable. Covered end to end + // rather than in Jest because the value being guarded is that a real user + // can recover the full text, not that the previewer trims a string. + await assigneeUser.login(page); + await table.visitEntityPage(page); + + await openEntityTasksTab(page); + + await openTaskDetails(page, createdTask); + + const drawer = page.locator('#task-panel'); + await expect(drawer).toBeVisible(); + + const runId = Date.now(); + const headMarker = `overflow-head-${runId}`; + const tailMarker = `overflow-tail-${runId}`; + const longMessage = `${headMarker} ${'This comment is written to overflow the preview limit on the task comment body. '.repeat( + 8 + )}${tailMarker}`; + + // The input is a trigger that opens the editor - it cannot be filled. + const commentInput = drawer.getByTestId('comments-input-field'); + await expect(commentInput).toBeVisible(); + await commentInput.click(); + + const editor = drawer.locator('[data-testid="editor-wrapper"] .ql-editor'); + await expect(editor).toBeVisible({ timeout: 15_000 }); + await editor.click(); + await editor.type(longMessage); + + const sendBtn = drawer.getByTestId('send-button'); + const commentResponsePromise = page.waitForResponse( + (response) => + response.url().includes('/api/v1/tasks/') && + response.url().includes('/comments') && + response.request().method() === 'POST' + ); + await sendBtn.click(); + await commentResponsePromise; + + // Matched on the head marker, which survives the trim - the tail marker + // is cut out of the DOM entirely while the preview is collapsed. + const commentCard = drawer + .getByTestId('feed-reply-card') + .filter({ hasText: headMarker }); + await expect(commentCard).toBeVisible(); + + // The trim drops the end of the message rather than hiding it, so without + // a working toggle that text would be unreachable, not merely clipped. + await expect(commentCard.getByText(tailMarker)).toBeHidden(); + + const readMoreButton = commentCard.getByTestId('read-more-button'); + await expect(readMoreButton).toBeVisible(); + await readMoreButton.click(); + + await expect(commentCard.getByTestId('read-less-button')).toBeVisible(); + await expect(commentCard.getByText(tailMarker)).toBeVisible(); + }); }); test.describe('Task Comments - API Validation', () => { diff --git a/openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs b/openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs index a613c506e95c..cc95f71bcc2f 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs +++ b/openmetadata-ui/src/main/resources/ui/playwright/eslint-rules/tests/corpus.test.mjs @@ -42,7 +42,7 @@ test('the suppressions baseline matches its recorded state exactly', () => { const EXPECTED = { 'om-playwright/justified-rule-disable': 10, 'om-playwright/no-blanket-test-slow': 1, - 'om-playwright/no-positional-locator': 1239, + 'om-playwright/no-positional-locator': 1217, 'om-playwright/require-assertion-per-test': 1, 'playwright/no-skipped-test': 2, 'playwright/no-wait-for-selector': 35, diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.test.tsx index e790324ef864..6fc194c810ef 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.test.tsx @@ -11,8 +11,15 @@ * limitations under the License. */ -import { fireEvent, render, screen } from '@testing-library/react'; +import { + fireEvent, + render, + screen, + waitFor, + within, +} from '@testing-library/react'; import { MemoryRouter } from 'react-router-dom'; +import { ReactionOperation } from '../../../enums/reactions.enum'; import { ActivityEvent, ActivityEventType, @@ -22,6 +29,7 @@ import { ConversationReply, ConversationSource, } from '../../../generated/entity/feed/conversation'; +import { ReactionType } from '../../../generated/type/reaction'; import ActivityFeedCardNew from './ActivityFeedcardNew.component'; const mockProviderValue = { @@ -31,6 +39,8 @@ const mockProviderValue = { postFeed: jest.fn(), selectedThread: undefined, updateFeed: jest.fn(), + deleteFeed: jest.fn(), + updateReactions: jest.fn(), }; jest.mock('../../../hooks/useApplicationStore', () => ({ @@ -63,16 +73,52 @@ jest.mock('../ActivityFeedCardV2/FeedCardFooter/ActivityEventFooter', () => jest.fn(() =>
) ); -jest.mock('../Shared/ActivityFeedActions', () => - jest.fn(() =>
) +// CommentCard and ActivityFeedActions are deliberately NOT mocked: the point +// of these tests is that a user can actually edit, delete and react through +// the rendered reply. Only true boundaries are stubbed below - the Quill +// editor, the tiptap-backed previewer, the emoji element and the lazily +// imported confirmation modal. +jest.mock('../../common/RichTextEditor/RichTextEditorPreviewerV1', () => + jest.fn(({ markdown }) =>
{markdown}
) +); + +jest.mock('../ActivityFeedEditor/ActivityFeedEditorNew', () => + jest.fn(({ onSave, onTextChange }) => ( +
+ onTextChange?.(e.target.value)} + /> + +
+ )) ); -jest.mock('./CommentCard.component', () => - jest.fn(({ reply }) => ( -
{reply.message}
+jest.mock('../Reactions/Reactions', () => + jest.fn(({ onReactionSelect }) => ( + )) ); +jest.mock('../../Modals/ConfirmationModal/ConfirmationModal', () => + jest.fn(({ visible, onConfirm }) => + visible ? ( + + ) : null + ) +); + jest.mock('../../common/PopOverCard/EntityPopOverCard', () => jest.fn(({ children }) => <>{children}) ); @@ -137,6 +183,7 @@ const activityReply: ConversationReply = { describe('ActivityFeedCardNew', () => { beforeEach(() => { + jest.clearAllMocks(); mockProviderValue.activityReplies = []; }); @@ -148,11 +195,35 @@ describe('ActivityFeedCardNew', () => { ); expect(screen.getByTestId('conversation-reaction-footer')).toBeVisible(); - expect(screen.queryByTestId('conversation-root-actions')).toBeNull(); + + // Mounted before any pointer interaction: hiding these behind a hover + // state put them out of reach of the keyboard and of screen readers. The + // hover reveal is presentational, applied by CSS on the owning card. + expect(screen.getByTestId('feed-actions')).toBeVisible(); + expect(screen.getByTestId('edit-message')).toBeVisible(); fireEvent.mouseEnter(screen.getByTestId('feed-card-v2-sidebar')); - expect(screen.getByTestId('conversation-root-actions')).toBeVisible(); + expect(screen.getByTestId('feed-actions')).toBeVisible(); + }); + + it('keeps the root actions a direct child of the card body', () => { + render( + + + + ); + + // The hover reveal in activity-feed-actions.less is written as + // `.activity-feed-card-new:hover > .ant-card-body > .feed-actions`. antd's + // Card puts its children inside .ant-card-body, so that middle step is + // load-bearing: drop it and the selector stops matching, leaving the + // reply/resolve/edit/delete bar hidden from mouse users. + const actions = screen.getByTestId('feed-actions'); + const body = actions.parentElement; + + expect(body).toHaveClass('ant-card-body'); + expect(body?.parentElement).toHaveClass('activity-feed-card-new'); }); it('renders activity replies in the open side panel', () => { @@ -168,4 +239,79 @@ describe('ActivityFeedCardNew', () => { activityReply.message ); }); + + describe('reply actions', () => { + // Drives the real CommentCard / ActivityFeedActions the user sees, and + // asserts what the feed provider is asked to do as a result. + const renderReply = () => { + mockProviderValue.activityReplies = [activityReply]; + + render( + + + + ); + + return within(screen.getByTestId('feed-reply-card')); + }; + + it('offers edit and delete on the authors own reply', () => { + const reply = renderReply(); + + expect(reply.getByTestId('edit-message')).toBeInTheDocument(); + expect(reply.getByTestId('delete-message')).toBeInTheDocument(); + }); + + it('patches the reply through updateFeed when the user saves an edit', async () => { + const reply = renderReply(); + + fireEvent.click(reply.getByTestId('edit-message')); + + const editor = await reply.findByTestId('reply-editor'); + fireEvent.change(within(editor).getByTestId('reply-editor-input'), { + target: { value: 'edited' }, + }); + fireEvent.click(within(editor).getByTestId('reply-editor-save')); + + await waitFor(() => { + expect(mockProviderValue.updateFeed).toHaveBeenCalledWith( + activity.id, + activityReply.id, + false, + [{ op: 'replace', path: '/message', value: 'edited' }] + ); + }); + }); + + it('removes the reply through deleteFeed when the user confirms', async () => { + const reply = renderReply(); + + fireEvent.click(reply.getByTestId('delete-message')); + fireEvent.click(await screen.findByTestId('confirm-delete')); + + await waitFor(() => { + expect(mockProviderValue.deleteFeed).toHaveBeenCalledWith( + activity.id, + activityReply.id, + false + ); + }); + }); + + it('forwards a reaction on the reply to updateReactions', async () => { + const reply = renderReply(); + + fireEvent.click(reply.getByTestId('reply-reactions')); + + await waitFor(() => { + expect(mockProviderValue.updateReactions).toHaveBeenCalledWith( + activityReply, + activity.id, + false, + ReactionType.ThumbsUp, + ReactionOperation.ADD + ); + }); + }); + }); }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx index b4b5767c75ac..6d228841cb0d 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/ActivityFeedcardNew.component.tsx @@ -37,6 +37,7 @@ import { entityDisplayName, getEntityFQN, getEntityType, + isFeedPostAuthor, } from '../../../utils/FeedUtilsPure'; import { getUserPath } from '../../../utils/RouterUtils'; import searchClassBase from '../../../utils/SearchClassBase'; @@ -157,13 +158,14 @@ const ActivityFeedCardNew = ({ selectedThread, postFeed, updateFeed, + deleteFeed, + updateReactions, isPostsLoading, postActivityComment, activityReplies, } = useActivityFeedProvider(); const [showFeedEditor, setShowFeedEditor] = useState(false); const [isEditPost, setIsEditPost] = useState(false); - const [isHovered, setIsHovered] = useState(false); const [, , user] = useUserProfile({ permission: true, name: createdBy, @@ -289,9 +291,10 @@ const ActivityFeedCardNew = ({ setShowFeedEditor(false); }; - const canShowFeedActions = isHovered && !isActivityEvent && !isPost; + // Rendered unconditionally and revealed with CSS: gating the mount on hover + // put these permanently out of reach of the keyboard and screen readers. const feedActions = - canShowFeedActions && feed ? ( + !isActivityEvent && !isPost && feed ? ( - {orderedPosts.map((reply, index, arr) => ( - - ))} + {orderedPosts.map((reply, index, arr) => { + const conversationId = activity?.id ?? feed?.id ?? ''; + const canManage = + isFeedPostAuthor(currentUser, reply.author) || + Boolean(currentUser?.isAdmin); + + return ( + deleteFeed(conversationId, reply.id, false)} + onEdit={async (message) => { + await updateFeed( + conversationId, + reply.id, + false, + compare(reply, { ...reply, message }) + ); + }} + onReaction={(reaction, operation) => + updateReactions( + reply, + conversationId, + false, + reaction, + operation + ) + } + /> + ); + })} ); }, [ @@ -344,6 +372,10 @@ const ActivityFeedCardNew = ({ isActivityEvent, activityReplies, activity?.id, + currentUser, + deleteFeed, + updateFeed, + updateReactions, ]); const feedMessage = useMemo(() => { @@ -364,9 +396,7 @@ const ActivityFeedCardNew = ({ isActive )} data-conversation-id={feed?.id} - data-testid="feed-card-v2-sidebar" - onMouseEnter={() => setIsHovered(true)} - onMouseLeave={() => setIsHovered(false)}> + data-testid="feed-card-v2-sidebar">
@@ -531,9 +561,7 @@ const ActivityFeedCardNew = ({ isActive )} data-conversation-id={feed?.id} - data-testid="feed-card-v2-sidebar" - onMouseEnter={() => setIsHovered(true)} - onMouseLeave={() => setIsHovered(false)}> + data-testid="feed-card-v2-sidebar"> import('../ActivityFeedEditor/ActivityFeedEditorNew')) ); -interface CommentCardInterface { - conversation?: Conversation; - conversationId: string; - reply: ConversationReply; +/** + * The comment being rendered. Declared structurally rather than as + * ConversationReply or TaskComment so one card serves both: the activity feed + * passes a conversation reply, the task and incident tabs pass a task comment, + * and these four fields are all this component reads from either. There is no + * `conversation` counterpart because a task comment has none - anything that + * needs the surrounding thread is handled by the caller through the callbacks + * below. + */ +export interface CommentCardReply { + author: EntityReference; + createdAt: number; + message: string; + reactions?: Reaction[]; +} + +interface CommentCardProps { + reply: CommentCardReply; isLastReply: boolean; - closeFeedEditor: () => void; + canEdit: boolean; + canDelete: boolean; + onEdit: (message: string) => Promise; + onDelete: () => Promise; + /** Omitted by callers with no reactions support - hides the footer entirely. */ + onReaction?: ( + type: ReactionType, + operation: ReactionOperation + ) => Promise; + /** + * Lets the owning feed dismiss its own reply editor when this card opens + * one, so the two are never open at the same time. Optional because callers + * outside the activity feed have no second editor to close. + */ + closeFeedEditor?: () => void; } const CommentCard = ({ - conversation, - conversationId, reply, isLastReply, + canEdit, + canDelete, + onEdit, + onDelete, + onReaction, closeFeedEditor, -}: CommentCardInterface) => { - const { updateFeed } = useActivityFeedProvider(); - const [isHovered, setIsHovered] = useState(false); +}: CommentCardProps) => { + // Unpacked once here so the rest of the component reads the same field names + // regardless of which comment shape the caller handed over. + const { author, createdAt, message, reactions } = reply; const [isEditPost, setIsEditPost] = useState(false); const [postMessage, setPostMessage] = useState(''); + const [isSaving, setIsSaving] = useState(false); const seperator = '.'; const editorRef = useRef(null); - const authorName = reply.author.name ?? reply.author.fullyQualifiedName ?? ''; + const authorName = author.name ?? author.fullyQualifiedName ?? ''; useEffect(() => { const handleClickOutside = (event: MouseEvent) => { @@ -88,24 +119,32 @@ const CommentCard = ({ }); const onEditPost = () => { - closeFeedEditor(); + closeFeedEditor?.(); setIsEditPost(!isEditPost); }; - const onUpdate = async (message: string) => { - const updatedReply = { ...reply, message }; - const patch = compare(reply, updatedReply); - updateFeed(conversationId, reply.id, false, patch); - setIsEditPost(!isEditPost); - }; + const handleSave = useCallback(async () => { + // The editor stays open while the save is in flight, so guard against a + // second submit firing a concurrent edit. + if (isSaving) { + return; + } - const handleSave = useCallback(() => { - onUpdate?.(postMessage ?? ''); - }, [onUpdate, postMessage]); + setIsSaving(true); + try { + await onEdit(postMessage ?? ''); + setIsEditPost(false); + } catch { + // Keep the editor open and the draft intact so the edit can be retried. + // The caller owns reporting the failure. + } finally { + setIsSaving(false); + } + }, [isSaving, onEdit, postMessage]); const defaultValue = useMemo( - () => MarkdownToHTMLConverter.makeHtml(getFrontEndFormat(reply.message)), - [reply.message] + () => MarkdownToHTMLConverter.makeHtml(getFrontEndFormat(message)), + [message] ); const feedBodyRender = useMemo(() => { @@ -127,73 +166,78 @@ const CommentCard = ({ return ( ); }, [isEditPost, postMessage, handleSave]); return (
setIsHovered(true)} - onMouseLeave={() => setIsHovered(false)}> + className={classNames( + 'd-flex items-start justify-start relative reply-card gap-2', + { + 'reply-card-border-bottom': !isLastReply, + } + )} + data-testid="feed-reply-card">
- +
-
- - - - {getEntityName(user)} - - - - - {seperator} - - - - - {getRelativeTime(reply.createdAt)} - - - +
+
+ + + + {getEntityName(user)} + + + + + {seperator} + + + + + {getRelativeTime(createdAt)} + + + +
+
{feedBodyRender} - + {onReaction && ( +
+
+
+ +
+
+
+ )}
- - {isHovered && ( - - )}
); }; diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.test.tsx index 097ece317ae2..d4589b9b0824 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/CommentCard.test.tsx @@ -13,21 +13,10 @@ import { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { MemoryRouter } from 'react-router-dom'; -import { - Conversation, - ConversationReply, - ConversationSource, -} from '../../../generated/entity/feed/conversation'; +import { ReactionOperation } from '../../../enums/reactions.enum'; +import { ReactionType } from '../../../generated/type/reaction'; import CommentCard from './CommentCard.component'; -const mockUpdateFeed = jest.fn(); - -jest.mock('../ActivityFeedProvider/ActivityFeedProvider', () => ({ - useActivityFeedProvider: () => ({ - updateFeed: mockUpdateFeed, - }), -})); - jest.mock('../../../hooks/user-profile/useUserProfile', () => ({ useUserProfile: () => [ false, @@ -59,8 +48,17 @@ jest.mock('../../common/RichTextEditor/RichTextEditorPreviewerV1', () => { )); }); -jest.mock('../ActivityFeedCardV2/FeedCardFooter/FeedCardFooterNew', () => { - return jest.fn(() =>
); +jest.mock('../Reactions/Reactions', () => { + return jest.fn(({ reactions, onReactionSelect }) => ( + + )); }); jest.mock('../ActivityFeedEditor/ActivityFeedEditorNew', () => { @@ -71,24 +69,29 @@ jest.mock('../ActivityFeedEditor/ActivityFeedEditorNew', () => { data-testid="editor-input" onChange={(e) => onTextChange(e.target.value)} /> -
)); }); +const mockActivityFeedActions = jest.fn(); jest.mock('../Shared/ActivityFeedActions', () => { - return jest.fn(({ onEditPost }) => ( -
- - -
- )); + return jest.fn((props) => { + mockActivityFeedActions(props); + + return ( +
+ + +
+ ); + }); }); jest.mock('../../../utils/FeedUtilsPure', () => ({ @@ -98,49 +101,24 @@ jest.mock('../../../utils/FeedUtilsPure', () => ({ }, })); -const createMockReply = ( - author: string, - message: string -): ConversationReply => ({ - id: 'reply-123', - conversationId: 'conversation-123', - message, - createdAt: 1234567890, - updatedAt: 1234567890, - author: { id: 'user-1', type: 'user', name: author }, -}); - -const createMockConversation = (): Conversation => ({ - id: 'conversation-123', - href: 'http://test', - createdAt: 1234567890, - about: '<#E::table::test>', - createdBy: { id: 'user-1', type: 'user', name: 'testuser' }, - entityRef: { id: 'entity-1', type: 'table', name: 'test' }, - updatedAt: 1234567890, - updatedBy: 'testuser', - source: ConversationSource.User, - message: 'Test thread message', - replyCount: 1, - replies: [], - reactions: [], - resolved: false, -}); +const onEdit = jest.fn().mockResolvedValue(undefined); +const onDelete = jest.fn().mockResolvedValue(undefined); +const onReaction = jest.fn().mockResolvedValue(undefined); const renderCommentCard = ( - props?: Partial<{ - conversation: Conversation; - conversationId: string; - reply: ConversationReply; - isLastReply: boolean; - closeFeedEditor: () => void; - }> + props?: Partial> ) => { - const defaultProps = { - conversation: createMockConversation(), - conversationId: 'conversation-123', - reply: createMockReply('testuser', 'Test comment message'), + const defaultProps: React.ComponentProps = { + reply: { + author: { id: 'user-1', type: 'user', name: 'testuser' }, + createdAt: 1234567890, + message: 'Test comment message', + }, isLastReply: false, + canEdit: true, + canDelete: true, + onDelete, + onEdit, closeFeedEditor: jest.fn(), }; @@ -151,6 +129,14 @@ const renderCommentCard = ( ); }; +const hoverCard = async () => { + fireEvent.mouseEnter(screen.getByTestId('feed-reply-card')); + + await waitFor(() => { + expect(screen.getByTestId('feed-actions')).toBeInTheDocument(); + }); +}; + describe('CommentCard', () => { beforeEach(() => { jest.clearAllMocks(); @@ -181,45 +167,104 @@ describe('CommentCard', () => { expect(screen.getByTestId('user-popover-2')).toBeInTheDocument(); }); - it('should render feed card footer', () => { + it('should render timestamp', () => { renderCommentCard(); + expect(screen.getByTestId('timestamp')).toBeInTheDocument(); + }); + + it('should fall back to the fully qualified name when author has no name', () => { + renderCommentCard({ + reply: { + author: { + id: 'user-1', + type: 'user', + fullyQualifiedName: 'fqn-user', + }, + createdAt: 1234567890, + message: 'Test comment message', + }, + }); + + expect(screen.getByTestId('profile-fqn-user')).toBeInTheDocument(); + }); + }); + + describe('Reactions footer', () => { + it('should render the footer only when onReaction is provided', () => { + const { rerender } = renderCommentCard(); + + expect(screen.queryByTestId('feed-card-footer')).not.toBeInTheDocument(); + + rerender( + + + + ); + expect(screen.getByTestId('feed-card-footer')).toBeInTheDocument(); }); - it('should render timestamp', () => { - renderCommentCard(); + it('should forward the reaction selection to onReaction', () => { + renderCommentCard({ onReaction }); - expect(screen.getByTestId('timestamp')).toBeInTheDocument(); + fireEvent.click(screen.getByTestId('reactions')); + + expect(onReaction).toHaveBeenCalledWith( + ReactionType.ThumbsUp, + ReactionOperation.ADD + ); }); }); - describe('Hover Actions', () => { - it('should show feed actions on hover', async () => { + describe('Permissions', () => { + it('should forward canEdit and canDelete to the actions', async () => { + renderCommentCard({ canDelete: false, canEdit: true }); + + await hoverCard(); + + expect(mockActivityFeedActions).toHaveBeenCalledWith( + expect.objectContaining({ canDelete: false, canEdit: true }) + ); + }); + + it('should call onDelete when the delete action fires', async () => { renderCommentCard(); - const card = screen.getByTestId('feed-reply-card'); - fireEvent.mouseEnter(card); + await hoverCard(); - await waitFor(() => { - expect(screen.getByTestId('feed-actions')).toBeInTheDocument(); - }); + fireEvent.click(screen.getByTestId('delete-button')); + + expect(onDelete).toHaveBeenCalled(); }); + }); - it('should hide feed actions when not hovering', async () => { + describe('Hover Actions', () => { + it('should show feed actions on hover', async () => { renderCommentCard(); - const card = screen.getByTestId('feed-reply-card'); + await hoverCard(); + }); - fireEvent.mouseEnter(card); - await waitFor(() => { - expect(screen.getByTestId('feed-actions')).toBeInTheDocument(); - }); + it('should keep feed actions mounted when not hovering', () => { + renderCommentCard(); - fireEvent.mouseLeave(card); - await waitFor(() => { - expect(screen.queryByTestId('feed-actions')).not.toBeInTheDocument(); - }); + // Never unmounted on pointer state: doing that made the edit and delete + // controls unreachable by keyboard and invisible to screen readers. + // Hiding them until hover is CSS's job, and it leaves them focusable. + expect(screen.getByTestId('feed-actions')).toBeInTheDocument(); }); }); @@ -227,12 +272,7 @@ describe('CommentCard', () => { it('should show editor when edit button is clicked', async () => { renderCommentCard(); - const card = screen.getByTestId('feed-reply-card'); - fireEvent.mouseEnter(card); - - await waitFor(() => { - expect(screen.getByTestId('edit-button')).toBeInTheDocument(); - }); + await hoverCard(); fireEvent.click(screen.getByTestId('edit-button')); @@ -245,27 +285,17 @@ describe('CommentCard', () => { const closeFeedEditor = jest.fn(); renderCommentCard({ closeFeedEditor }); - const card = screen.getByTestId('feed-reply-card'); - fireEvent.mouseEnter(card); - - await waitFor(() => { - expect(screen.getByTestId('edit-button')).toBeInTheDocument(); - }); + await hoverCard(); fireEvent.click(screen.getByTestId('edit-button')); expect(closeFeedEditor).toHaveBeenCalled(); }); - it('should call updateFeed when saving edited message', async () => { + it('should call onEdit with the edited message when saving', async () => { renderCommentCard(); - const card = screen.getByTestId('feed-reply-card'); - fireEvent.mouseEnter(card); - - await waitFor(() => { - expect(screen.getByTestId('edit-button')).toBeInTheDocument(); - }); + await hoverCard(); fireEvent.click(screen.getByTestId('edit-button')); @@ -280,20 +310,64 @@ describe('CommentCard', () => { fireEvent.click(screen.getByTestId('send-button')); await waitFor(() => { - expect(mockUpdateFeed).toHaveBeenCalled(); + expect(onEdit).toHaveBeenCalledWith('updated message'); }); }); - it('should hide editor and show preview after update', async () => { + it('should not fire a second save while one is in flight', async () => { + let settle: () => void = () => undefined; + onEdit.mockReturnValueOnce( + new Promise((resolve) => { + settle = resolve; + }) + ); renderCommentCard(); - const card = screen.getByTestId('feed-reply-card'); - fireEvent.mouseEnter(card); + await hoverCard(); + + fireEvent.click(screen.getByTestId('edit-button')); + + await waitFor(() => { + expect(screen.getByTestId('feed-editor')).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('send-button')); + fireEvent.click(screen.getByTestId('send-button')); await waitFor(() => { - expect(screen.getByTestId('edit-button')).toBeInTheDocument(); + expect(onEdit).toHaveBeenCalledTimes(1); }); + settle(); + }); + + it('should keep the editor open when the save fails', async () => { + onEdit.mockRejectedValueOnce(new Error('boom')); + renderCommentCard(); + + await hoverCard(); + + fireEvent.click(screen.getByTestId('edit-button')); + + await waitFor(() => { + expect(screen.getByTestId('feed-editor')).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('send-button')); + + await waitFor(() => { + expect(onEdit).toHaveBeenCalled(); + }); + + // Dismissing here would look like the edit had been saved. + expect(screen.getByTestId('feed-editor')).toBeInTheDocument(); + }); + + it('should hide editor and show preview after update', async () => { + renderCommentCard(); + + await hoverCard(); + fireEvent.click(screen.getByTestId('edit-button')); await waitFor(() => { @@ -331,12 +405,7 @@ describe('CommentCard', () => { it('should close edit mode when clicking outside', async () => { renderCommentCard(); - const card = screen.getByTestId('feed-reply-card'); - fireEvent.mouseEnter(card); - - await waitFor(() => { - expect(screen.getByTestId('edit-button')).toBeInTheDocument(); - }); + await hoverCard(); fireEvent.click(screen.getByTestId('edit-button')); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/TaskCommentCard.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/TaskCommentCard.component.tsx deleted file mode 100644 index 77089f40d6f8..000000000000 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedCardNew/TaskCommentCard.component.tsx +++ /dev/null @@ -1,84 +0,0 @@ -/* - * Copyright 2024 Collate. - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0 - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { Space, Tooltip, Typography } from 'antd'; -import { FC, useMemo } from 'react'; -import { useUserProfile } from '../../../hooks/user-profile/useUserProfile'; -import { Task, TaskComment } from '../../../rest/tasksAPI'; -import { - formatDateTime, - getRelativeTime, -} from '../../../utils/date-time/DateTimeUtils'; -import { getEntityName } from '../../../utils/EntityNameUtils'; -import { getFrontEndFormat } from '../../../utils/FeedUtilsPure'; -import ProfilePicture from '../../common/ProfilePicture/ProfilePicture'; -import RichTextEditorPreviewNew from '../../common/RichTextEditor/RichTextEditorPreviewNew'; -interface TaskCommentCardProps { - comment: TaskComment; - task: Task; - isLastReply?: boolean; - closeFeedEditor?: () => void; -} - -const TaskCommentCard: FC = ({ - comment, - isLastReply = false, -}) => { - const [, , user] = useUserProfile({ - permission: true, - name: comment.author?.name ?? '', - }); - - const authorName = useMemo( - () => getEntityName(user) || comment.author?.name || 'Unknown', - [user, comment.author] - ); - - return ( -
- - -
- - - {authorName} - - {comment.createdAt && ( - - - {getRelativeTime(comment.createdAt)} - - - )} - -
- -
-
- -
- ); -}; - -export default TaskCommentCard; diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/activity-feed-tab.less b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/activity-feed-tab.less index 43f21195d6a8..0133224bc1dd 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/activity-feed-tab.less +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/activity-feed-tab.less @@ -486,13 +486,44 @@ } .reply-card { - padding-top: @padding-lg; + // Symmetric, so the divider below a reply sits midway between it and the + // next one rather than flush against the text it follows. Matches the + // padding of the comments container these sit in. + padding-top: var(--om-space-16); + padding-bottom: var(--om-space-16); } .reply-card-border-bottom { border-bottom: 0.5px solid var(--om-legacy-color-e4e4e4); } +// .feed-actions is absolutely positioned at a fixed top: 24px, which only +// lined up while this card's padding was also 24px. It now uses 16px, so pull +// the bar up to match - otherwise it hangs below the author row and covers the +// first line of the comment. Scoped to the reply card so the conversation +// card, which keeps its own spacing, is unaffected. +// A reply renders its actions inside the author row rather than floating them +// over the card, so undo the shared absolute positioning here. Keeping them in +// normal flow is what stops the bar overlapping the comment text - it can no +// longer be mispositioned relative to a padding or line-height it doesn't know +// about. +.reply-card .feed-actions.ant-space { + position: static; + padding-top: var(--om-space-2); + padding-bottom: var(--om-space-2); +} + +// Tooltips render into a portal on document.body, so this has to sit in a +// stylesheet that every component rendering `timestamp-tooltip` pulls in - +// both ActivityFeedcardNew and CommentCard import this file. Without it the +// tooltip keeps antd's default white text on the white background its `color` +// prop sets, i.e. an empty bubble. +.timestamp-tooltip { + .ant-tooltip-inner { + color: var(--om-color-text-primary); + } +} + .reply-message { color: var(--om-legacy-color-4a4a4a); white-space: normal; diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/ActivityFeedActions.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/ActivityFeedActions.test.tsx index 7de36e1c93f4..566497154054 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/ActivityFeedActions.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/ActivityFeedActions.test.tsx @@ -11,7 +11,7 @@ * limitations under the License. */ -import { fireEvent, render, screen } from '@testing-library/react'; +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { Conversation, ConversationReply, @@ -204,6 +204,132 @@ describe('ActivityFeedActions', () => { expect(mockUpdateEditorFocus).toHaveBeenCalledWith(true); }); + it('renders every control as a focusable button with an accessible name', () => { + render( + + ); + + // The regression this guards: these were ``, which Ant + // renders as `` - not focusable, not announced as a + // control, and unusable with Enter/Space. + for (const testId of [ + 'add-reply', + 'toggle-resolved', + 'edit-message', + 'delete-message', + ]) { + const control = screen.getByTestId(testId); + + expect(control.tagName).toBe('BUTTON'); + expect(control).toHaveAccessibleName(); + + control.focus(); + + expect(control).toHaveFocus(); + } + }); + + it('groups the actions and never unmounts them on pointer state', () => { + render( + + ); + + expect(screen.getByTestId('feed-actions')).toHaveAttribute('role', 'group'); + expect(screen.getByTestId('feed-actions')).toHaveAccessibleName(); + }); + + it('merges the reveal class handed down by the owning card', () => { + render( + + ); + + const actions = screen.getByTestId('feed-actions'); + + expect(actions).toHaveClass('feed-actions'); + expect(actions).toHaveClass('tw:opacity-0'); + }); + + it('honours canEdit and canDelete independently of the author rule', () => { + mockUseApplicationStore.mockReturnValue({ + currentUser: { id: 'other-id', name: 'bob', isAdmin: false }, + }); + + render( + + ); + + expect(screen.queryByTestId('edit-message')).not.toBeInTheDocument(); + expect(screen.getByTestId('delete-message')).toBeInTheDocument(); + }); + + it('calls onDelete instead of the feed provider when one is supplied', () => { + const onDelete = jest.fn(); + + render(); + fireEvent.click(screen.getByTestId('delete-message')); + fireEvent.click(screen.getByTestId('confirm-delete')); + + expect(onDelete).toHaveBeenCalled(); + expect(mockDeleteFeed).not.toHaveBeenCalled(); + }); + + it('does not fire a second delete while one is in flight', async () => { + let settle: () => void = () => undefined; + const onDelete = jest.fn().mockReturnValue( + new Promise((resolve) => { + settle = resolve; + }) + ); + + render(); + fireEvent.click(screen.getByTestId('delete-message')); + + // Double click while the request is still outstanding. Without the guard + // the second click deletes again and 404s against the missing comment. + fireEvent.click(screen.getByTestId('confirm-delete')); + fireEvent.click(screen.getByTestId('confirm-delete')); + + await waitFor(() => { + expect(onDelete).toHaveBeenCalledTimes(1); + }); + + settle(); + }); + + it('keeps the confirmation open when onDelete rejects', async () => { + const onDelete = jest.fn().mockRejectedValue(new Error('boom')); + + render(); + fireEvent.click(screen.getByTestId('delete-message')); + fireEvent.click(screen.getByTestId('confirm-delete')); + + await waitFor(() => { + expect(onDelete).toHaveBeenCalled(); + }); + + // Closing here would look like the delete had gone through. + expect(screen.getByTestId('confirmation-modal')).toBeInTheDocument(); + }); + it('closes the confirmation without deleting', () => { render( void; + /** + * Replaces the provider-backed delete. Required by callers outside the + * activity feed, which have no conversation to delete a post from. + */ + onDelete?: () => void | Promise; + /** + * Reveal styling from the owning card. These actions stay mounted so they + * remain reachable by Tab and by a screen reader; a consumer that wants them + * revealed on pointer hover does that with opacity on a `tw:group` ancestor, + * never by unmounting them. + */ + className?: string; } +/** + * Fall back to the feed's own author-or-admin rule for whichever action the + * caller did not state a permission for. + * + * This default is the *conversation* rule, not the task-comment one. A + * conversation reply is authorized through RBAC + * (ConversationRepository#patchReply / #deleteReply ask the authorizer for + * EDIT_ALL / DELETE), which an admin passes for both. A task comment is not: + * TaskRepository#editComment takes no `isAdmin` at all and permits the author + * only, while #deleteComment takes one and permits author-or-admin. The two + * rules are deliberately different - do not collapse them. Callers rendering + * task comments pass `canEdit`/`canDelete` explicitly, derived from + * `resolveCommentPermissions` in utils/TaskCommentUtils. + */ +const resolveActionVisibility = ( + isAuthor: boolean, + isAdmin: boolean, + canEdit?: boolean, + canDelete?: boolean +) => { + const canManage = isAuthor || isAdmin; + + return { + canManage, + showDelete: canDelete ?? canManage, + showEdit: canEdit ?? canManage, + }; +}; + const getIsAuthor = ( isReply: boolean, currentUser: { id?: string; name?: string } | undefined, conversation?: Conversation, reply?: ConversationReply -): boolean => { - const author = isReply - ? reply?.author.name ?? reply?.author.fullyQualifiedName - : conversation?.createdBy?.name ?? - conversation?.createdBy?.fullyQualifiedName; - const authorId = isReply ? reply?.author.id : conversation?.createdBy?.id; - - return authorId ? authorId === currentUser?.id : author === currentUser?.name; -}; +): boolean => + isFeedPostAuthor( + currentUser, + isReply ? reply?.author : conversation?.createdBy + ); const ActivityFeedActions = ({ conversation, conversationId, reply, isReply, + canEdit, + canDelete, onEditPost, + onDelete, + className, }: ActivityFeedActionsProps) => { const { t, i18n } = useTranslation(); const dir = i18n.dir(); const { currentUser } = useApplicationStore(); const isAuthor = getIsAuthor(isReply, currentUser, conversation, reply); const [showDeleteDialog, setShowDeleteDialog] = useState(false); + const [isDeleting, setIsDeleting] = useState(false); const { deleteFeed, showDrawer, hideDrawer, updateEditorFocus, updateFeed } = useActivityFeedProvider(); @@ -82,21 +135,56 @@ const ActivityFeedActions = ({ updateEditorFocus(true); }; - const handleDelete = () => { - const targetId = reply?.id ?? conversationId; - deleteFeed(conversationId, targetId, !isReply).catch(() => { - // ignore since error is displayed in toast in the parent promise. - }); + const handleDelete = async () => { + if (onDelete) { + // The confirmation stays open for the length of the request, so without + // this the button remains clickable and a second click fires a + // concurrent delete - the first succeeds, the second 404s, and the user + // is shown a failure for a delete that actually worked. + if (isDeleting) { + return; + } + + setIsDeleting(true); + try { + await onDelete(); + setShowDeleteDialog(false); + } catch { + // Leave the confirmation open so the delete can be retried. The caller + // owns reporting the failure. + } finally { + setIsDeleting(false); + } + + return; + } + setShowDeleteDialog(false); + + if (!conversationId) { + return; + } + + deleteFeed(conversationId, reply?.id ?? conversationId, !isReply).catch( + () => { + // ignore since error is displayed in toast in the parent promise. + } + ); + if (!isReply) { hideDrawer(); } }; - const canManage = isAuthor || Boolean(currentUser?.isAdmin); + const { canManage, showEdit, showDelete } = resolveActionVisibility( + isAuthor, + Boolean(currentUser?.isAdmin), + canEdit, + canDelete + ); const handleResolvedChange = () => { - if (!conversation || isReply) { + if (!conversation || isReply || !conversationId) { return; } updateFeed(conversationId, conversationId, true, [ @@ -111,49 +199,58 @@ const ActivityFeedActions = ({ return ( <> {!isReply && conversation && ( - )} {!isReply && conversation && canManage && ( - )} - {canManage && ( - )} - {canManage && ( - setShowDeleteDialog(true)} /> )} @@ -163,6 +260,7 @@ const ActivityFeedActions = ({ cancelText={t('label.cancel')} confirmText={t('label.delete')} header={t('message.delete-message-question-mark')} + isLoading={isDeleting} visible={showDeleteDialog} onCancel={() => setShowDeleteDialog(false)} onConfirm={handleDelete} diff --git a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/activity-feed-actions.less b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/activity-feed-actions.less index 64034d3c6066..08db382d455b 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/activity-feed-actions.less +++ b/openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/Shared/activity-feed-actions.less @@ -12,6 +12,36 @@ */ @import '../../../styles/variables.less'; +// Hidden until the card is hovered, or until focus lands inside. Declared once +// here against the actions' own class rather than handed down as utility +// classes: the behaviour is identical on every card, so only the hover trigger +// below has to know which card it belongs to. +.feed-actions.ant-space { + opacity: 0; + pointer-events: none; + transition: opacity var(--om-duration-300); + + &:focus-within { + opacity: 1; + pointer-events: auto; + } +} + +// The one part that cannot be shared: which card's hover reveals which bar. +// A conversation card contains reply cards, so each trigger is anchored to its +// own card rather than to any ancestor - a plain descendant selector on the +// conversation would also reveal every reply's bar. +// +// The .ant-card-body step is antd's own wrapper: Card renders its children +// inside it, so the conversation's bar is a grandchild, not a child. A test in +// ActivityFeedcardNew.component.test.tsx pins that shape so an antd upgrade +// that changes it fails loudly rather than silently leaving these hidden. +.activity-feed-card-new:hover > .ant-card-body > .feed-actions.ant-space, +.reply-card:hover .feed-actions.ant-space { + opacity: 1; + pointer-events: auto; +} + .feed-actions.ant-space { position: absolute; right: 10px; @@ -35,3 +65,17 @@ align-items: center; justify-content: center; } + +// ButtonUtility ships tw:p-1.5 (6px), which makes each control 28px and the +// bar noticeably taller than the 16px glyphs it replaced. Trim it back so the +// bar keeps its original height while the controls stay real, focusable +// buttons. Written here rather than passed as a class because a Tailwind +// utility would tie on specificity with p-1.5 and win or lose on stylesheet +// order. +.feed-actions.ant-space .toolbar-button { + padding: var(--om-space-2); + // ButtonUtility's `tertiary` colour paints the glyph tw:text-quaternary, a + // mid grey. Fall back to the bar's own text colour, which is what these + // icons inherited before they became buttons. + color: @text-color; +} diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.test.tsx index b283dd1866f6..f6d96a0660bb 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.test.tsx @@ -303,13 +303,14 @@ jest.mock('../../../../context/PermissionProvider/PermissionProvider', () => ({ })), })); +const mockFetchUpdatedThread = jest.fn().mockResolvedValue({}); jest.mock( '../../../ActivityFeed/ActivityFeedProvider/ActivityFeedProvider', () => ({ useActivityFeedProvider: jest.fn().mockImplementation(() => ({ postFeed: jest.fn().mockResolvedValue({}), updateTask: jest.fn(), - fetchUpdatedThread: jest.fn().mockResolvedValue({}), + fetchUpdatedThread: mockFetchUpdatedThread, updateTestCaseIncidentStatus: jest.fn(), testCaseResolutionStatus: [], isPostsLoading: false, @@ -317,11 +318,19 @@ jest.mock( }) ); +const mockShowErrorToast = jest.fn(); +jest.mock('../../../../utils/ToastUtils', () => ({ + ...jest.requireActual('../../../../utils/ToastUtils'), + showErrorToast: (...args: unknown[]) => mockShowErrorToast(...args), +})); + jest.mock('../../../../rest/tasksAPI', () => ({ ...jest.requireActual('../../../../rest/tasksAPI'), resolveTask: jest.fn().mockResolvedValue({}), closeTask: jest.fn().mockResolvedValue({}), patchTask: jest.fn().mockResolvedValue({}), + editTaskComment: jest.fn().mockResolvedValue({}), + deleteTaskComment: jest.fn().mockResolvedValue({}), })); jest.mock('../../../../rest/userAPI', () => ({ @@ -424,10 +433,15 @@ jest.mock( } ); +const mockCommentCardProps: Record[] = []; jest.mock( - '../../../ActivityFeed/ActivityFeedCardNew/TaskCommentCard.component', + '../../../ActivityFeed/ActivityFeedCardNew/CommentCard.component', () => { - return jest.fn().mockImplementation(() =>

TaskCommentCard

); + return jest.fn().mockImplementation((props) => { + mockCommentCardProps.push(props); + + return

CommentCard

; + }); } ); @@ -453,6 +467,7 @@ const mockProps = { describe('TaskTabNew Component', () => { beforeEach(() => { jest.clearAllMocks(); + mockCommentCardProps.length = 0; const { useAuth } = require('../../../../hooks/authHooks'); const { useApplicationStore, @@ -1270,4 +1285,150 @@ describe('TaskTabNew Component', () => { }); }); }); + + describe('task comments', () => { + // The card re-renders, so assert against the props it was last handed rather + // than a render count. + const lastCommentCardProps = () => + mockCommentCardProps[mockCommentCardProps.length - 1]; + + const MOCK_TASK_WITH_COMMENT: Task = { + ...MOCK_TASK, + comments: [ + { + id: 'comment-1', + message: 'A comment on the incident', + createdAt: 1735732800000, + author: { id: 'user-1', type: 'user', name: 'alice' }, + }, + ], + }; + + const renderWithComment = async () => { + await act(async () => { + render(, { + wrapper: MemoryRouter, + }); + }); + }; + + it('should pass the comment through to the card', async () => { + await renderWithComment(); + + expect(lastCommentCardProps()).toEqual( + expect.objectContaining({ + reply: expect.objectContaining({ + author: expect.objectContaining({ name: 'alice' }), + createdAt: 1735732800000, + message: 'A comment on the incident', + }), + }) + ); + }); + + it('should deny edit and delete to a non-author, non-admin user', async () => { + await renderWithComment(); + + expect(lastCommentCardProps()).toEqual( + expect.objectContaining({ canDelete: false, canEdit: false }) + ); + }); + + it('should let an admin delete but not edit someone elses comment', async () => { + const { + useApplicationStore, + } = require('../../../../hooks/useApplicationStore'); + useApplicationStore.mockReturnValue({ + currentUser: { + id: 'admin-id', + name: 'an-admin', + isAdmin: true, + teams: [], + }, + }); + + await renderWithComment(); + + expect(lastCommentCardProps()).toEqual( + expect.objectContaining({ canDelete: true, canEdit: false }) + ); + }); + + it('should omit onReaction so the card hides its reactions footer', async () => { + await renderWithComment(); + + expect(lastCommentCardProps().onReaction).toBeUndefined(); + }); + + it('should delete the comment and refetch the thread', async () => { + const { deleteTaskComment } = require('../../../../rest/tasksAPI'); + await renderWithComment(); + + mockFetchUpdatedThread.mockClear(); + + await act(async () => { + await (lastCommentCardProps().onDelete as () => Promise)(); + }); + + expect(deleteTaskComment).toHaveBeenCalledWith( + MOCK_TASK_WITH_COMMENT.id, + 'comment-1' + ); + expect(mockFetchUpdatedThread).toHaveBeenCalledWith( + MOCK_TASK_WITH_COMMENT.id, + true + ); + }); + + it('should toast and rethrow when deleting the comment fails', async () => { + const { deleteTaskComment } = require('../../../../rest/tasksAPI'); + const failure = new Error('nope'); + deleteTaskComment.mockRejectedValueOnce(failure); + await renderWithComment(); + + // Rethrown on purpose: the card keeps its confirmation open only if the + // callback it awaited actually rejects. + await expect( + (lastCommentCardProps().onDelete as () => Promise)() + ).rejects.toThrow('nope'); + + expect(mockShowErrorToast).toHaveBeenCalledWith(failure); + }); + + it('should toast and rethrow when editing the comment fails', async () => { + const { editTaskComment } = require('../../../../rest/tasksAPI'); + const failure = new Error('nope'); + editTaskComment.mockRejectedValueOnce(failure); + await renderWithComment(); + + await expect( + (lastCommentCardProps().onEdit as (m: string) => Promise)('x') + ).rejects.toThrow('nope'); + + expect(mockShowErrorToast).toHaveBeenCalledWith(failure); + }); + + it('should edit the comment and refetch the thread', async () => { + const { editTaskComment } = require('../../../../rest/tasksAPI'); + await renderWithComment(); + + mockFetchUpdatedThread.mockClear(); + + await act(async () => { + await ( + lastCommentCardProps().onEdit as (message: string) => Promise + )('an edited comment'); + }); + + expect(editTaskComment).toHaveBeenCalledWith( + MOCK_TASK_WITH_COMMENT.id, + 'comment-1', + 'an edited comment' + ); + expect(mockFetchUpdatedThread).toHaveBeenCalledWith( + MOCK_TASK_WITH_COMMENT.id, + true + ); + }); + }); }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.tsx index 3f9fe4e0e452..20102cb52275 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/TaskTabNew.component.tsx @@ -96,6 +96,8 @@ import { } from '../../../../rest/taskFormSchemasAPI'; import { closeTask as closeTaskAPI, + deleteTaskComment, + editTaskComment, patchTask, resolveTask as resolveTaskAPI, Task, @@ -129,6 +131,7 @@ import { fetchOptions, generateOptions, } from '../../../../utils/TaskAssigneeUtils'; +import { resolveCommentPermissions } from '../../../../utils/TaskCommentUtils'; import { applyTaskFormSchemaDefaults, getDefaultTaskFormSchema, @@ -149,7 +152,7 @@ import { } from '../../../../utils/TaskNavigationUtils'; import { getNormalizedTaskPayload } from '../../../../utils/TaskPayloadUtils'; import { showErrorToast, showSuccessToast } from '../../../../utils/ToastUtils'; -import TaskCommentCard from '../../../ActivityFeed/ActivityFeedCardNew/TaskCommentCard.component'; +import CommentCard from '../../../ActivityFeed/ActivityFeedCardNew/CommentCard.component'; import ActivityFeedEditorNew from '../../../ActivityFeed/ActivityFeedEditor/ActivityFeedEditorNew'; import { useActivityFeedProvider } from '../../../ActivityFeed/ActivityFeedProvider/ActivityFeedProvider'; import withSuspenseFallback from '../../../AppRouter/withSuspenseFallback'; @@ -1834,18 +1837,50 @@ export const TaskTabNew = ({ return ( - {sortedComments.map((comment, index, arr) => ( - - ))} + {sortedComments.map((comment, index, arr) => { + const { canEdit, canDelete } = resolveCommentPermissions( + currentUser, + comment + ); + + return ( + { + try { + await deleteTaskComment(task.id, comment.id); + await fetchUpdatedThread(task.id, true); + } catch (error) { + // The REST helpers throw without surfacing anything of their + // own. Rethrow after toasting so the card leaves the + // confirmation open for a retry instead of dismissing it as + // though the delete had succeeded. + showErrorToast(error as AxiosError); + + throw error; + } + }} + onEdit={async (message) => { + try { + await editTaskComment(task.id, comment.id, message); + await fetchUpdatedThread(task.id, true); + } catch (error) { + showErrorToast(error as AxiosError); + + throw error; + } + }} + /> + ); + })} ); - }, [task, closeFeedEditor, isPostsLoading]); + }, [task, closeFeedEditor, isPostsLoading, currentUser, fetchUpdatedThread]); useEffect(() => { closeFeedEditor(); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/task-tab-new.less b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/task-tab-new.less index 2ed22d19c8e8..c255a9f70801 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/task-tab-new.less +++ b/openmetadata-ui/src/main/resources/ui/src/components/Entity/Task/TaskTab/task-tab-new.less @@ -13,6 +13,22 @@ @import (reference) '../../../../styles/variables.less'; +// TaskTabNew renders .activity-feed-comments-container, but that class is only +// styled in activity-feed-tab.less and feed-panel-body-v1.less - neither of +// which loads on a surface that mounts the task tab on its own, such as the +// incident page's Issues tab. Without this the box lost its background, border +// and padding on a hard refresh there. Scoped to the task panel so it cannot +// cross-talk with the activity feed's own copies of the class. +.task-details-panel { + .activity-feed-comments-container { + border-radius: var(--om-radius-xl); + padding: var(--om-space-16); + margin: var(--om-space-20) 0 0 0; + border: 0.5px solid var(--om-color-gray-blue-100); + background: @grey-9; + } +} + .assignees-edit-input { .ant-space-item:first-child { width: 100%; diff --git a/openmetadata-ui/src/main/resources/ui/src/components/MyData/FeedWidget/feed-widget.less b/openmetadata-ui/src/main/resources/ui/src/components/MyData/FeedWidget/feed-widget.less index 19f653d773f6..b90397d5073d 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/MyData/FeedWidget/feed-widget.less +++ b/openmetadata-ui/src/main/resources/ui/src/components/MyData/FeedWidget/feed-widget.less @@ -57,9 +57,3 @@ color: @primary-7; } } - -.timestamp-tooltip { - .ant-tooltip-inner { - color: @grey-3; - } -} diff --git a/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.test.tsx index 10c13b99ac67..155eb5a2ac18 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.test.tsx @@ -12,6 +12,7 @@ */ import { act, fireEvent, render, screen, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { ComponentProps, ReactNode } from 'react'; const mockGetTaskById = jest.fn(); @@ -233,6 +234,25 @@ jest.mock('components/common/RichTextEditor/RichTextEditorPreviewerV1', () => ({ })); jest.mock('@openmetadata/ui-core-components', () => ({ + // Real + ), Badge: ({ children, color, @@ -986,11 +1006,10 @@ describe('TaskDetailPanel', () => { await act(async () => render()); - fireEvent.mouseEnter(screen.getByTestId('task-comment-card')); - await act(async () => { - fireEvent.click(screen.getByTestId('edit-task-comment')); - }); - const editBox = screen.getByTestId('edit-task-comment-editor'); + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + await user.click(screen.getByTestId('edit-task-comment')); + + const editBox = await screen.findByTestId('edit-task-comment-editor'); await act(async () => { fireEvent.click(within(editBox).getByTestId('comment-editor')); }); @@ -1005,16 +1024,14 @@ describe('TaskDetailPanel', () => { await act(async () => render()); - fireEvent.mouseEnter(screen.getByTestId('task-comment-card')); - await act(async () => { - fireEvent.click(screen.getByTestId('edit-task-comment')); - }); + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + await user.click(screen.getByTestId('edit-task-comment')); - expect(screen.getByTestId('edit-task-comment-editor')).toBeInTheDocument(); + expect( + await screen.findByTestId('edit-task-comment-editor') + ).toBeInTheDocument(); - await act(async () => { - fireEvent.click(screen.getByTestId('cancel-edit-task-comment')); - }); + await user.click(screen.getByTestId('cancel-edit-task-comment')); expect( screen.queryByTestId('edit-task-comment-editor') @@ -1028,13 +1045,9 @@ describe('TaskDetailPanel', () => { await act(async () => render()); - fireEvent.mouseEnter(screen.getByTestId('task-comment-card')); - await act(async () => { - fireEvent.click(screen.getByTestId('delete-task-comment')); - }); - await act(async () => { - fireEvent.click(screen.getByTestId('confirm-delete-task-comment')); - }); + const user = userEvent.setup({ advanceTimers: jest.advanceTimersByTime }); + await user.click(screen.getByTestId('delete-task-comment')); + await user.click(await screen.findByTestId('confirm-delete-task-comment')); expect(mockDeleteComment).toHaveBeenCalledWith('task-1', 'c1'); expect(mockGetTaskById).toHaveBeenCalledTimes(2); @@ -1046,8 +1059,8 @@ describe('TaskDetailPanel', () => { await act(async () => render()); - fireEvent.mouseEnter(screen.getByTestId('task-comment-card')); - + // No hover: the affordance a user is entitled to must be in the DOM (and so + // reachable by keyboard) regardless of pointer position. expect(screen.queryByTestId('edit-task-comment')).not.toBeInTheDocument(); expect(screen.getByTestId('delete-task-comment')).toBeInTheDocument(); }); @@ -1058,8 +1071,6 @@ describe('TaskDetailPanel', () => { await act(async () => render()); - fireEvent.mouseEnter(screen.getByTestId('task-comment-card')); - expect(screen.queryByTestId('edit-task-comment')).not.toBeInTheDocument(); expect(screen.queryByTestId('delete-task-comment')).not.toBeInTheDocument(); }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.tsx b/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.tsx index a290262f6631..b8649abe62bd 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/discovery/personal-space/InboxPage/components/TaskDetailPanel.tsx @@ -15,11 +15,16 @@ import { Badge, Box, Button, + ButtonUtility, EmptyPlaceholder, Tabs, Typography, } from '@openmetadata/ui-core-components'; -import { CheckCircle, Edit01, Trash01, XCircle } from '@untitledui/icons'; +import { + Delete as DeleteIcon, + Edit as EditIcon, +} from '@openmetadata/ui-core-components/icons'; +import { CheckCircle, XCircle } from '@untitledui/icons'; import { AxiosError } from 'axios'; import React, { ComponentProps, @@ -32,7 +37,6 @@ import React, { } from 'react'; import { useTranslation } from 'react-i18next'; import { Link } from 'react-router-dom'; -import ActivityFeedEditorNew from '../../../../../components/ActivityFeed/ActivityFeedEditor/ActivityFeedEditorNew'; import DeleteModal from '../../../../../components/common/DeleteModal/DeleteModal'; import ProfilePicture from '../../../../../components/common/ProfilePicture/ProfilePicture'; import RichTextEditorPreviewerV1 from '../../../../../components/common/RichTextEditor/RichTextEditorPreviewerV1'; @@ -69,9 +73,11 @@ import { } from '../../../../../utils/FeedUtilsPure'; import { getTestCaseDetailPagePath } from '../../../../../utils/RouterUtils'; import { getPermissionErrorText } from '../../../../../utils/StringUtils'; +import { resolveCommentPermissions } from '../../../../../utils/TaskCommentUtils'; import { getResolvedTaskFormSchema } from '../../../../../utils/TaskFormSchemaUtils'; import { getTaskDetailPathFromTask } from '../../../../../utils/TaskNavigationUtils'; import { showErrorToast } from '../../../../../utils/ToastUtils'; +import ActivityFeedEditorNew from '../../../../ActivityFeed/ActivityFeedEditor/ActivityFeedEditorNew'; import { getTaskStatusBadge } from '../taskResolution.utils'; import { buildResolveBody, @@ -255,110 +261,6 @@ interface TaskCommentRowProps { onChanged: () => void; } -interface CommentPermissions { - canDelete: boolean; - canEdit: boolean; - canModify: boolean; -} - -/** The comment's author may edit or delete it; an admin may also delete it. */ -const resolveCommentPermissions = ( - currentUser: { name?: string; isAdmin?: boolean } | undefined, - comment: TaskComment -): CommentPermissions => { - const isAuthor = - Boolean(currentUser?.name) && comment.author?.name === currentUser?.name; - const canEdit = isAuthor; - const canDelete = isAuthor || Boolean(currentUser?.isAdmin); - - return { canEdit, canDelete, canModify: canEdit || canDelete }; -}; - -interface TaskCommentActionsProps { - canDelete: boolean; - canEdit: boolean; - onDeleteRequest: () => void; - onEditRequest: () => void; -} - -/** Hover-only edit/delete affordances for a comment row. */ -const TaskCommentActions = ({ - canDelete, - canEdit, - onDeleteRequest, - onEditRequest, -}: TaskCommentActionsProps) => ( - - {canEdit && ( - - )} - {canDelete && ( - - )} - -); - -interface TaskCommentBodyProps { - comment: TaskComment; - isEditing: boolean; - onCancelEdit: () => void; - onSave: (message: string) => Promise; -} - -/** The comment's editor when editing, else its rendered markdown. */ -const TaskCommentBody = ({ - comment, - isEditing, - onCancelEdit, - onSave, -}: TaskCommentBodyProps) => { - const { t } = useTranslation(); - - if (isEditing) { - return ( - - - - - - - ); - } - - return ( - - - - ); -}; - /** * A single task comment with author, message and timestamp. The comment's author * can edit or delete it (admins can also delete); the actions surface on hover. @@ -379,7 +281,6 @@ const TaskCommentRow: React.FC = ({ canModify: canModifyComment, } = resolveCommentPermissions(currentUser, comment); - const [isHovered, setIsHovered] = useState(false); const [isEditing, setIsEditing] = useState(false); const [showDeleteDialog, setShowDeleteDialog] = useState(false); const [isDeleting, setIsDeleting] = useState(false); @@ -414,42 +315,86 @@ const TaskCommentRow: React.FC = ({ }, [taskId, comment.id, onChanged]); return ( + // align="start" keeps the avatar pinned to the top of the row, and the + // content column holds everything else so the message and timestamp line + // up under the author name instead of under the avatar. setIsHovered(true)} - onMouseLeave={() => setIsHovered(false)}> - - - + gap={2}> + + + {authorName} + {!isEditing && canModifyComment && ( + // Real buttons, kept mounted and revealed with opacity: unmounting + // them until hover puts them out of reach of the keyboard. +
+ {canEdit && ( + setIsEditing(true)} + /> + )} + {canDelete && ( + setShowDeleteDialog(true)} + /> + )} +
+ )} - {isHovered && !isEditing && canModifyComment && ( - setShowDeleteDialog(true)} - onEditRequest={() => setIsEditing(true)} - /> + {isEditing ? ( + + + + + + + ) : ( + + + )} + + {getRelativeTime(comment.createdAt)} + - setIsEditing(false)} - onSave={handleEditSave} - /> - - {getRelativeTime(comment.createdAt)} - { + const authorName = author?.name ?? author?.fullyQualifiedName; + + return author?.id + ? author.id === currentUser?.id + : authorName === currentUser?.name; +}; diff --git a/openmetadata-ui/src/main/resources/ui/src/utils/TaskCommentUtils.test.ts b/openmetadata-ui/src/main/resources/ui/src/utils/TaskCommentUtils.test.ts new file mode 100644 index 000000000000..2edc9aafb1ac --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/src/utils/TaskCommentUtils.test.ts @@ -0,0 +1,63 @@ +/* + * Copyright 2026 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { TaskComment } from '../generated/entity/tasks/task'; +import { resolveCommentPermissions } from './TaskCommentUtils'; + +const comment = { + id: 'c1', + message: 'hello', + createdAt: 0, + author: { id: 'u1', type: 'user', name: 'alice' }, +} as TaskComment; + +describe('resolveCommentPermissions', () => { + it('should let the author edit and delete their own comment', () => { + expect(resolveCommentPermissions({ name: 'alice' }, comment)).toEqual({ + canEdit: true, + canDelete: true, + canModify: true, + }); + }); + + it('should let an admin delete but not edit someone elses comment', () => { + expect( + resolveCommentPermissions({ name: 'bob', isAdmin: true }, comment) + ).toEqual({ canEdit: false, canDelete: true, canModify: true }); + }); + + it('should give a non-author non-admin nothing', () => { + expect( + resolveCommentPermissions({ name: 'bob', isAdmin: false }, comment) + ).toEqual({ canEdit: false, canDelete: false, canModify: false }); + }); + + it('should give an unknown current user nothing', () => { + expect(resolveCommentPermissions(undefined, comment)).toEqual({ + canEdit: false, + canDelete: false, + canModify: false, + }); + }); + + // Both sides being nameless must not read as "same person". + it('should not treat a nameless user as the author of a nameless comment', () => { + const anonymous = { ...comment, author: { id: 'u1', type: 'user' } }; + + expect(resolveCommentPermissions({}, anonymous as TaskComment)).toEqual({ + canEdit: false, + canDelete: false, + canModify: false, + }); + }); +}); diff --git a/openmetadata-ui/src/main/resources/ui/src/utils/TaskCommentUtils.ts b/openmetadata-ui/src/main/resources/ui/src/utils/TaskCommentUtils.ts new file mode 100644 index 000000000000..fb5b50b2420b --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/src/utils/TaskCommentUtils.ts @@ -0,0 +1,45 @@ +/* + * Copyright 2026 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { TaskComment } from '../generated/entity/tasks/task'; + +export interface CommentPermissions { + canDelete: boolean; + canEdit: boolean; + canModify: boolean; +} + +/** + * Who may act on a task comment. Mirrors the server's rules in + * TaskRepository#editComment / #deleteComment: the author may edit or delete + * their own comment, and an admin may additionally delete anyone's. + * + * Shared so the task tab and the Inbox task panel cannot drift apart from each + * other, or from the backend. + * + * Note the asymmetry is real and verified against the Java: #editComment has no + * `isAdmin` parameter, #deleteComment does. This is NOT the rule used for + * activity-feed conversation replies, which go through RBAC and let an admin do + * both - see resolveActionVisibility in ActivityFeedActions. + */ +export const resolveCommentPermissions = ( + currentUser: { name?: string; isAdmin?: boolean } | undefined, + comment: TaskComment +): CommentPermissions => { + const isAuthor = + Boolean(currentUser?.name) && comment.author?.name === currentUser?.name; + const canEdit = isAuthor; + const canDelete = isAuthor || Boolean(currentUser?.isAdmin); + + return { canEdit, canDelete, canModify: canEdit || canDelete }; +};