From d39c8fec8c00c7cd4f3a850f1b8303a24ad41437 Mon Sep 17 00:00:00 2001 From: Bram Hagens Date: Tue, 16 Jul 2024 14:38:46 +0000 Subject: [PATCH] ui: update pull request icons Added a new icon for closed PRs (similar to GitHub, GitLab, etc), Fixes https://codeberg.org/forgejo/forgejo/issues/4454. Before: - https://codeberg.org/attachments/b17c5846-506f-4b32-97c9-03f31c5ff758 - https://codeberg.org/attachments/babcd011-d340-4a9e-94db-ea17ef6d3c2b - https://codeberg.org/attachments/dbca009a-413e-48ab-84b1-55ad7f4fcd3d After: - https://codeberg.org/attachments/3e161f7b-4172-4a8c-a8eb-54bcf81c0cae - https://codeberg.org/attachments/0c308f7e-25a0-49a3-9c86-1b1f9ab39467 - https://codeberg.org/attachments/b982b6b8-c78a-4332-8269-50d01de834e0 Co-authored-by: 0ko <0ko@noreply.codeberg.org> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4455 Reviewed-by: Caesar Schinas Reviewed-by: 0ko <0ko@noreply.codeberg.org> Co-authored-by: Bram Hagens Co-committed-by: Bram Hagens --- tests/integration/pull_icon_test.go | 257 +++++++++++++++++++++ web_src/js/components/ContextPopup.test.js | 178 +++++++++++--- 2 files changed, 408 insertions(+), 27 deletions(-) create mode 100644 tests/integration/pull_icon_test.go diff --git a/tests/integration/pull_icon_test.go b/tests/integration/pull_icon_test.go new file mode 100644 index 0000000000..0602f49179 --- /dev/null +++ b/tests/integration/pull_icon_test.go @@ -0,0 +1,257 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package integration + +import ( + "context" + "fmt" + "net/http" + "net/url" + "strings" + "testing" + "time" + + "code.gitea.io/gitea/models/db" + issues_model "code.gitea.io/gitea/models/issues" + repo_model "code.gitea.io/gitea/models/repo" + unit_model "code.gitea.io/gitea/models/unit" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/git" + issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" + files_service "code.gitea.io/gitea/services/repository/files" + "code.gitea.io/gitea/tests" + + "github.com/PuerkitoBio/goquery" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPullRequestIcons(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + repo, _, f := CreateDeclarativeRepo(t, user, "pr-icons", []unit_model.Type{unit_model.TypeCode, unit_model.TypePullRequests}, nil, nil) + defer f() + + session := loginUser(t, user.LoginName) + + // Individual PRs + t.Run("Open", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + pull := createOpenPullRequest(db.DefaultContext, t, user, repo) + testPullRequestIcon(t, session, pull, "green", "octicon-git-pull-request") + }) + + t.Run("WIP (Open)", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + pull := createOpenWipPullRequest(db.DefaultContext, t, user, repo) + testPullRequestIcon(t, session, pull, "grey", "octicon-git-pull-request-draft") + }) + + t.Run("Closed", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + pull := createClosedPullRequest(db.DefaultContext, t, user, repo) + testPullRequestIcon(t, session, pull, "red", "octicon-git-pull-request-closed") + }) + + t.Run("WIP (Closed)", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + pull := createClosedWipPullRequest(db.DefaultContext, t, user, repo) + testPullRequestIcon(t, session, pull, "red", "octicon-git-pull-request-closed") + }) + + t.Run("Merged", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + pull := createMergedPullRequest(db.DefaultContext, t, user, repo) + testPullRequestIcon(t, session, pull, "purple", "octicon-git-merge") + }) + + // List + req := NewRequest(t, "GET", repo.HTMLURL()+"/pulls?state=all") + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + + t.Run("List Open", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + testPullRequestListIcon(t, doc, "open", "green", "octicon-git-pull-request") + }) + + t.Run("List WIP (Open)", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + testPullRequestListIcon(t, doc, "open-wip", "grey", "octicon-git-pull-request-draft") + }) + + t.Run("List Closed", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + testPullRequestListIcon(t, doc, "closed", "red", "octicon-git-pull-request-closed") + }) + + t.Run("List Closed (WIP)", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + testPullRequestListIcon(t, doc, "closed-wip", "red", "octicon-git-pull-request-closed") + }) + + t.Run("List Merged", func(t *testing.T) { + defer tests.PrintCurrentTest(t)() + + testPullRequestListIcon(t, doc, "merged", "purple", "octicon-git-merge") + }) + }) +} + +func testPullRequestIcon(t *testing.T, session *TestSession, pr *issues_model.PullRequest, expectedColor, expectedIcon string) { + req := NewRequest(t, "GET", pr.Issue.HTMLURL()) + resp := session.MakeRequest(t, req, http.StatusOK) + doc := NewHTMLParser(t, resp.Body) + doc.AssertElement(t, fmt.Sprintf("div.issue-state-label.%s > svg.%s", expectedColor, expectedIcon), true) + + req = NewRequest(t, "GET", pr.BaseRepo.HTMLURL()+"/branches") + resp = session.MakeRequest(t, req, http.StatusOK) + doc = NewHTMLParser(t, resp.Body) + doc.AssertElement(t, fmt.Sprintf(`a[href="/%s/pulls/%d"].%s > svg.%s`, pr.BaseRepo.FullName(), pr.Issue.Index, expectedColor, expectedIcon), true) +} + +func testPullRequestListIcon(t *testing.T, doc *HTMLDoc, name, expectedColor, expectedIcon string) { + sel := doc.doc.Find("div#issue-list > div.flex-item"). + FilterFunction(func(_ int, selection *goquery.Selection) bool { + return selection.Find(fmt.Sprintf(`div.flex-item-icon > svg.%s.%s`, expectedColor, expectedIcon)).Length() == 1 && + strings.HasSuffix(selection.Find("a.issue-title").Text(), name) + }) + + assert.Equal(t, 1, sel.Length()) +} + +func createOpenPullRequest(ctx context.Context, t *testing.T, user *user_model.User, repo *repo_model.Repository) *issues_model.PullRequest { + pull := createPullRequest(t, user, repo, "open") + + assert.False(t, pull.Issue.IsClosed) + assert.False(t, pull.HasMerged) + assert.False(t, pull.IsWorkInProgress(ctx)) + + return pull +} + +func createOpenWipPullRequest(ctx context.Context, t *testing.T, user *user_model.User, repo *repo_model.Repository) *issues_model.PullRequest { + pull := createPullRequest(t, user, repo, "open-wip") + + err := issue_service.ChangeTitle(ctx, pull.Issue, user, "WIP: "+pull.Issue.Title) + require.NoError(t, err) + + assert.False(t, pull.Issue.IsClosed) + assert.False(t, pull.HasMerged) + assert.True(t, pull.IsWorkInProgress(ctx)) + + return pull +} + +func createClosedPullRequest(ctx context.Context, t *testing.T, user *user_model.User, repo *repo_model.Repository) *issues_model.PullRequest { + pull := createPullRequest(t, user, repo, "closed") + + err := issue_service.ChangeStatus(ctx, pull.Issue, user, "", true) + require.NoError(t, err) + + assert.True(t, pull.Issue.IsClosed) + assert.False(t, pull.HasMerged) + assert.False(t, pull.IsWorkInProgress(ctx)) + + return pull +} + +func createClosedWipPullRequest(ctx context.Context, t *testing.T, user *user_model.User, repo *repo_model.Repository) *issues_model.PullRequest { + pull := createPullRequest(t, user, repo, "closed-wip") + + err := issue_service.ChangeTitle(ctx, pull.Issue, user, "WIP: "+pull.Issue.Title) + require.NoError(t, err) + + err = issue_service.ChangeStatus(ctx, pull.Issue, user, "", true) + require.NoError(t, err) + + assert.True(t, pull.Issue.IsClosed) + assert.False(t, pull.HasMerged) + assert.True(t, pull.IsWorkInProgress(ctx)) + + return pull +} + +func createMergedPullRequest(ctx context.Context, t *testing.T, user *user_model.User, repo *repo_model.Repository) *issues_model.PullRequest { + pull := createPullRequest(t, user, repo, "merged") + + gitRepo, err := git.OpenRepository(ctx, repo.RepoPath()) + defer gitRepo.Close() + + require.NoError(t, err) + + err = pull_service.Merge(ctx, pull, user, gitRepo, repo_model.MergeStyleMerge, pull.HeadCommitID, "merge", false) + require.NoError(t, err) + + assert.False(t, pull.Issue.IsClosed) + assert.True(t, pull.CanAutoMerge()) + assert.False(t, pull.IsWorkInProgress(ctx)) + + return pull +} + +func createPullRequest(t *testing.T, user *user_model.User, repo *repo_model.Repository, name string) *issues_model.PullRequest { + branch := "branch-" + name + title := "Testing " + name + + _, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, user, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{ + { + Operation: "update", + TreePath: "README.md", + ContentReader: strings.NewReader("Update README"), + }, + }, + Message: "Update README", + OldBranch: "main", + NewBranch: branch, + Author: &files_service.IdentityOptions{ + Name: user.Name, + Email: user.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: user.Name, + Email: user.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + }) + + require.NoError(t, err) + + pullIssue := &issues_model.Issue{ + RepoID: repo.ID, + Title: title, + PosterID: user.ID, + Poster: user, + IsPull: true, + } + + pullRequest := &issues_model.PullRequest{ + HeadRepoID: repo.ID, + BaseRepoID: repo.ID, + HeadBranch: branch, + BaseBranch: "main", + HeadRepo: repo, + BaseRepo: repo, + Type: issues_model.PullRequestGitea, + } + err = pull_service.NewPullRequest(git.DefaultContext, repo, pullIssue, nil, nil, pullRequest, nil) + require.NoError(t, err) + + return pullRequest +} diff --git a/web_src/js/components/ContextPopup.test.js b/web_src/js/components/ContextPopup.test.js index 1db6c38301..2726567b4a 100644 --- a/web_src/js/components/ContextPopup.test.js +++ b/web_src/js/components/ContextPopup.test.js @@ -1,39 +1,163 @@ -import {mount, flushPromises} from '@vue/test-utils'; +import {flushPromises, mount} from '@vue/test-utils'; import ContextPopup from './ContextPopup.vue'; -test('renders a issue info popup', async () => { - const owner = 'user2'; - const repo = 'repo1'; - const index = 1; +async function assertPopup(popupData, expectedIconColor, expectedIcon) { + const date = new Date('2024-07-13T22:00:00Z'); + vi.spyOn(global, 'fetch').mockResolvedValue({ json: vi.fn().mockResolvedValue({ ok: true, - created_at: '2023-09-30T19:00:00Z', - repository: {full_name: owner}, - pull_request: null, - state: 'open', - title: 'Normal issue', - body: 'Lorem ipsum...', - number: index, - labels: [{color: 'ee0701', name: "Bug :+1: "}], + created_at: date.toISOString(), + repository: {full_name: 'user2/repo1'}, + ...popupData, }), ok: true, }); - const wrapper = mount(ContextPopup); - wrapper.vm.$el.dispatchEvent(new CustomEvent('ce-load-context-popup', {detail: {owner, repo, index}})); + const popup = mount(ContextPopup); + popup.vm.$el.dispatchEvent(new CustomEvent('ce-load-context-popup', { + detail: {owner: 'user2', repo: 'repo1', index: popupData.number}, + })); await flushPromises(); - // Header - expect(wrapper.get('p:nth-of-type(1)').text()).toEqual('user2 on Sep 30, 2023'); - // Title - expect(wrapper.get('p:nth-of-type(2)').text()).toEqual('Normal issue #1'); - // Body - expect(wrapper.get('p:nth-of-type(3)').text()).toEqual('Lorem ipsum...'); - // Check that the state is correct. - expect(wrapper.get('svg').classes()).toContain('octicon-issue-opened'); - // Ensure that script is not an element. - expect(() => wrapper.get('.evil')).toThrowError(); - // Check content of label - expect(wrapper.get('.ui.label').text()).toContain("Bug 👍 "); + expect(popup.get('p:nth-of-type(1)').text()).toEqual(`user2/repo1 on ${date.toLocaleDateString(undefined, {year: 'numeric', month: 'short', day: 'numeric'})}`); + expect(popup.get('p:nth-of-type(2)').text()).toEqual(`${popupData.title} #${popupData.number}`); + expect(popup.get('p:nth-of-type(3)').text()).toEqual(popupData.body); + + expect(popup.get('svg').classes()).toContain(expectedIcon); + expect(popup.get('svg').classes()).toContain(expectedIconColor); + + for (const l of popupData.labels) { + expect(popup.findAll('.ui.label').map((x) => x.text())).toContain(l.name); + } +} + +test('renders an open issue popup', async () => { + await assertPopup({ + title: 'Open Issue', + body: 'Open Issue Body', + number: 1, + labels: [{color: 'd21b1fff', name: 'Bug'}, {color: 'aaff00', name: 'Confirmed'}], + state: 'open', + pull_request: null, + }, 'green', 'octicon-issue-opened'); +}); + +test('renders a closed issue popup', async () => { + await assertPopup({ + title: 'Closed Issue', + body: 'Closed Issue Body', + number: 1, + labels: [{color: 'd21b1fff', name: 'Bug'}, {color: 'aaff00', name: 'Confirmed'}], + state: 'closed', + pull_request: null, + }, 'red', 'octicon-issue-closed'); +}); + +test('renders an open PR popup', async () => { + await assertPopup({ + title: 'Open PR', + body: 'Open PR Body', + number: 1, + labels: [{color: 'd21b1fff', name: 'Bug'}, {color: 'aaff00', name: 'Confirmed'}], + state: 'open', + pull_request: {merged: false, draft: false}, + }, 'green', 'octicon-git-pull-request'); +}); + +test('renders an open WIP PR popup', async () => { + await assertPopup({ + title: 'WIP: Open PR', + body: 'WIP Open PR Body', + number: 1, + labels: [{color: 'd21b1fff', name: 'Bug'}, {color: 'aaff00', name: 'Confirmed'}], + state: 'open', + pull_request: {merged: false, draft: true}, + }, 'grey', 'octicon-git-pull-request-draft'); +}); + +test('renders a closed PR popup', async () => { + await assertPopup({ + title: 'Closed PR', + body: 'Closed PR Body', + number: 1, + labels: [{color: 'd21b1fff', name: 'Bug'}, {color: 'aaff00', name: 'Confirmed'}], + state: 'closed', + pull_request: {merged: false, draft: false}, + }, 'red', 'octicon-git-pull-request-closed'); +}); + +test('renders a closed WIP PR popup', async () => { + await assertPopup({ + title: 'WIP: Closed PR', + body: 'WIP Closed PR Body', + number: 1, + labels: [{color: 'd21b1fff', name: 'Bug'}, {color: 'aaff00', name: 'Confirmed'}], + state: 'closed', + pull_request: {merged: false, draft: true}, + }, 'red', 'octicon-git-pull-request-closed'); +}); + +test('renders a merged PR popup', async () => { + await assertPopup({ + title: 'Merged PR', + body: 'Merged PR Body', + number: 1, + labels: [{color: 'd21b1fff', name: 'Bug'}, {color: 'aaff00', name: 'Confirmed'}], + state: 'closed', + pull_request: {merged: true, draft: false}, + }, 'purple', 'octicon-git-merge'); +}); + +test('renders an issue popup with escaped HTML', async () => { + const evil = ''; + + vi.spyOn(global, 'fetch').mockResolvedValue({ + json: vi.fn().mockResolvedValue({ + ok: true, + created_at: '2024-07-13T22:00:00Z', + repository: {full_name: evil}, + title: evil, + body: evil, + labels: [{color: '000666', name: evil}], + state: 'open', + pull_request: null, + }), + ok: true, + }); + + const popup = mount(ContextPopup); + popup.vm.$el.dispatchEvent(new CustomEvent('ce-load-context-popup', { + detail: {owner: evil, repo: evil, index: 1}, + })); + await flushPromises(); + + expect(() => popup.get('.evil')).toThrowError(); + expect(popup.get('p:nth-of-type(1)').text()).toContain(evil); + expect(popup.get('p:nth-of-type(2)').text()).toContain(evil); + expect(popup.get('p:nth-of-type(3)').text()).toContain(evil); +}); + +test('renders an issue popup with emojis', async () => { + vi.spyOn(global, 'fetch').mockResolvedValue({ + json: vi.fn().mockResolvedValue({ + ok: true, + created_at: '2024-07-13T22:00:00Z', + repository: {full_name: 'user2/repo1'}, + title: 'Title', + body: 'Body', + labels: [{color: '000666', name: 'Tag :+1:'}], + state: 'open', + pull_request: null, + }), + ok: true, + }); + + const popup = mount(ContextPopup); + popup.vm.$el.dispatchEvent(new CustomEvent('ce-load-context-popup', { + detail: {owner: 'user2', repo: 'repo1', index: 1}, + })); + await flushPromises(); + + expect(popup.get('.ui.label').text()).toEqual('Tag 👍'); });