From df2e85f667b96811317566b3c1d2c122a6d68878 Mon Sep 17 00:00:00 2001 From: Solomon Victorino Date: Wed, 7 Aug 2024 16:24:49 -0600 Subject: [PATCH] fix(ui): allow unreacting from comment popover - fix selectors for hasReacted - don't send empty HTML on reaction errors - add E2E test (cherry picked from commit b8a5ca2c402f3f4c0a9df627035ef6a74574365b) --- routers/web/repo/issue.go | 40 +++++------- tests/e2e/reaction-selectors.test.e2e.js | 65 ++++++++++++++++++++ web_src/js/features/comp/ReactionSelector.js | 2 +- 3 files changed, 80 insertions(+), 27 deletions(-) create mode 100644 tests/e2e/reaction-selectors.test.e2e.js diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index bf9fc5f993..869fccbfb8 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -3299,12 +3299,6 @@ func ChangeIssueReaction(ctx *context.Context) { log.Info("CreateIssueReaction: %s", err) break } - // Reload new reactions - issue.Reactions = nil - if err = issue.LoadAttributes(ctx); err != nil { - log.Info("issue.LoadAttributes: %s", err) - break - } log.Trace("Reaction for issue created: %d/%d/%d", ctx.Repo.Repository.ID, issue.ID, reaction.ID) case "unreact": @@ -3313,19 +3307,19 @@ func ChangeIssueReaction(ctx *context.Context) { return } - // Reload new reactions - issue.Reactions = nil - if err := issue.LoadAttributes(ctx); err != nil { - log.Info("issue.LoadAttributes: %s", err) - break - } - log.Trace("Reaction for issue removed: %d/%d", ctx.Repo.Repository.ID, issue.ID) default: ctx.NotFound(fmt.Sprintf("Unknown action %s", ctx.Params(":action")), nil) return } + // Reload new reactions + issue.Reactions = nil + if err := issue.LoadAttributes(ctx); err != nil { + ctx.ServerError("ChangeIssueReaction.LoadAttributes", err) + return + } + if len(issue.Reactions) == 0 { ctx.JSON(http.StatusOK, map[string]any{ "empty": true, @@ -3406,12 +3400,6 @@ func ChangeCommentReaction(ctx *context.Context) { log.Info("CreateCommentReaction: %s", err) break } - // Reload new reactions - comment.Reactions = nil - if err = comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil { - log.Info("comment.LoadReactions: %s", err) - break - } log.Trace("Reaction for comment created: %d/%d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID, reaction.ID) case "unreact": @@ -3420,19 +3408,19 @@ func ChangeCommentReaction(ctx *context.Context) { return } - // Reload new reactions - comment.Reactions = nil - if err = comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil { - log.Info("comment.LoadReactions: %s", err) - break - } - log.Trace("Reaction for comment removed: %d/%d/%d", ctx.Repo.Repository.ID, comment.Issue.ID, comment.ID) default: ctx.NotFound(fmt.Sprintf("Unknown action %s", ctx.Params(":action")), nil) return } + // Reload new reactions + comment.Reactions = nil + if err = comment.LoadReactions(ctx, ctx.Repo.Repository); err != nil { + ctx.ServerError("ChangeCommentReaction.LoadReactions", err) + return + } + if len(comment.Reactions) == 0 { ctx.JSON(http.StatusOK, map[string]any{ "empty": true, diff --git a/tests/e2e/reaction-selectors.test.e2e.js b/tests/e2e/reaction-selectors.test.e2e.js new file mode 100644 index 0000000000..91754b0931 --- /dev/null +++ b/tests/e2e/reaction-selectors.test.e2e.js @@ -0,0 +1,65 @@ +// @ts-check +import {test, expect} from '@playwright/test'; +import {login_user, load_logged_in_context} from './utils_e2e.js'; + +test.beforeAll(async ({browser}, workerInfo) => { + await login_user(browser, workerInfo, 'user2'); +}); + +const assertReactionCounts = (comment, counts) => + expect(async () => { + await expect(comment.locator('.reactions')).toBeVisible(); + + const reactions = Object.fromEntries( + await Promise.all( + ( + await comment + .locator(`.reactions [role=button][data-reaction-content]`) + .all() + ).map(async (button) => [ + await button.getAttribute('data-reaction-content'), + parseInt(await button.locator('.reaction-count').textContent()), + ]), + ), + ); + return expect(reactions).toStrictEqual(counts); + }).toPass(); + +async function toggleReaction(menu, reaction) { + await menu.evaluateAll((menus) => menus[0].focus()); + await menu.locator('.add-reaction').click(); + await menu.locator(`[role=menuitem][data-reaction-content="${reaction}"]`).click(); +} + +test('Reaction Selectors', async ({browser}, workerInfo) => { + const context = await load_logged_in_context(browser, workerInfo, 'user2'); + const page = await context.newPage(); + + const response = await page.goto('/user2/repo1/issues/1'); + await expect(response?.status()).toBe(200); + + const comment = page.locator('.comment#issuecomment-2').first(); + + const topPicker = comment.locator('.actions [role=menu].select-reaction'); + const bottomPicker = comment.locator('.reactions').getByRole('menu'); + + await assertReactionCounts(comment, {'laugh': 2}); + + await toggleReaction(topPicker, '+1'); + await assertReactionCounts(comment, {'laugh': 2, '+1': 1}); + + await toggleReaction(bottomPicker, '+1'); + await assertReactionCounts(comment, {'laugh': 2}); + + await toggleReaction(bottomPicker, '-1'); + await assertReactionCounts(comment, {'laugh': 2, '-1': 1}); + + await toggleReaction(topPicker, '-1'); + await assertReactionCounts(comment, {'laugh': 2}); + + await comment.locator('.reactions [role=button][data-reaction-content=laugh]').click(); + await assertReactionCounts(comment, {'laugh': 1}); + + await toggleReaction(topPicker, 'laugh'); + await assertReactionCounts(comment, {'laugh': 2}); +}); diff --git a/web_src/js/features/comp/ReactionSelector.js b/web_src/js/features/comp/ReactionSelector.js index 2def3db51a..fd4601fb91 100644 --- a/web_src/js/features/comp/ReactionSelector.js +++ b/web_src/js/features/comp/ReactionSelector.js @@ -9,7 +9,7 @@ export function initCompReactionSelector($parent) { const actionUrl = this.closest('[data-action-url]')?.getAttribute('data-action-url'); const reactionContent = this.getAttribute('data-reaction-content'); - const hasReacted = this.closest('.ui.segment.reactions')?.querySelector(`a[data-reaction-content="${reactionContent}"]`)?.getAttribute('data-has-reacted') === 'true'; + const hasReacted = this.closest('.comment')?.querySelector(`.ui.segment.reactions a[data-reaction-content="${reactionContent}"]`)?.getAttribute('data-has-reacted') === 'true'; const res = await POST(`${actionUrl}/${hasReacted ? 'unreact' : 'react'}`, { data: new URLSearchParams({content: reactionContent}),