diff --git a/eslint.config.ts b/eslint.config.ts index 95ae056d152..7e78fd34cfe 100644 --- a/eslint.config.ts +++ b/eslint.config.ts @@ -211,7 +211,7 @@ export default defineConfig([ '@typescript-eslint/no-magic-numbers': [0], '@typescript-eslint/no-meaningless-void-operator': [0], '@typescript-eslint/no-misused-new': [2], - '@typescript-eslint/no-misused-promises': [2, {checksVoidReturn: {attributes: false, arguments: false}}], + '@typescript-eslint/no-misused-promises': [2, {checksVoidReturn: {attributes: false, arguments: false, properties: false}}], '@typescript-eslint/no-misused-spread': [2], '@typescript-eslint/no-mixed-enums': [0], '@typescript-eslint/no-namespace': [2], diff --git a/models/project/issue.go b/models/project/issue.go index 84a8675f2ee..8f09a2557c0 100644 --- a/models/project/issue.go +++ b/models/project/issue.go @@ -43,6 +43,41 @@ func columnIssueIDs(column *Column) []int64 { return []int64{column.ID} } +// LockProject serializes concurrent card moves of a project, avoiding row-lock deadlocks between them. +func LockProject(ctx context.Context, projectID int64) error { + _, err := db.GetEngine(ctx).Exec("UPDATE `project` SET title=title WHERE id=?", projectID) + return err +} + +// ColumnIssueSorting is an issue's position in a column. +type ColumnIssueSorting struct { + IssueID int64 `xorm:"issue_id"` + Sorting int64 `xorm:"sorting"` +} + +// GetColumnIssueSortings returns the column's issues except excludeIssueID in board order. +func GetColumnIssueSortings(ctx context.Context, column *Column, excludeIssueID int64) (rows []ColumnIssueSorting, _ error) { + err := db.GetEngine(ctx).Table("project_issue"). + Select("project_issue.issue_id, project_issue.sorting"). + Join("INNER", "issue", "issue.id = project_issue.issue_id"). + Where("project_issue.project_id=?", column.ProjectID). + In("project_issue.project_board_id", columnIssueIDs(column)). + And("project_issue.issue_id<>?", excludeIssueID). + OrderBy("project_issue.sorting ASC, issue.created_unix DESC, issue.id DESC"). + Find(&rows) + return rows, err +} + +// SetIssueSorting sets the issue's sorting if it is still in the column. +func SetIssueSorting(ctx context.Context, column *Column, issueID, sorting int64) error { + _, err := db.GetEngine(ctx). + Where("project_id=? AND issue_id=?", column.ProjectID, issueID). + In("project_board_id", columnIssueIDs(column)). + Cols("sorting"). + Update(&ProjectIssue{Sorting: sorting}) + return err +} + // IsIssueInColumn reports whether the issue is placed in the column. func IsIssueInColumn(ctx context.Context, issueID int64, column *Column) (bool, error) { return db.GetEngine(ctx). diff --git a/routers/web/shared/project/project.go b/routers/web/shared/project/project.go index 599544f427e..29d6d75b2f4 100644 --- a/routers/web/shared/project/project.go +++ b/routers/web/shared/project/project.go @@ -154,50 +154,30 @@ func MoveIssues(ctx *context.Context) { return } - type movedIssuesForm struct { - Issues []struct { - IssueID int64 `json:"issueID"` - Sorting int64 `json:"sorting"` - } `json:"issues"` - } - - form := &movedIssuesForm{} - if err := json.NewDecoder(ctx.Req.Body).Decode(&form); err != nil { - ctx.ServerError("DecodeMovedIssuesForm", err) + form := &struct { + IssueID int64 `json:"issueID"` + PrevIssueID int64 `json:"prevIssueID"` + }{} + if err := json.NewDecoder(ctx.Req.Body).Decode(form); err != nil { + ctx.ServerError("DecodeMovedIssueForm", err) return } - issueIDs := make([]int64, 0, len(form.Issues)) - sortedIssueIDs := make(map[int64]int64) - for _, issue := range form.Issues { - issueIDs = append(issueIDs, issue.IssueID) - sortedIssueIDs[issue.Sorting] = issue.IssueID - } - movedIssues, err := issues_model.GetIssuesByIDs(ctx, issueIDs) + issue, err := issues_model.GetIssueByID(ctx, form.IssueID) if err != nil { ctx.ServerError("GetIssueByID", err) return } - - if len(movedIssues) != len(form.Issues) { - ctx.ServerError("some issues do not exist", errors.New("some issues do not exist")) + if err = issue.LoadRepo(ctx); err != nil { + ctx.ServerError("LoadRepo", err) return } - - if _, err = movedIssues.LoadRepositories(ctx); err != nil { - ctx.ServerError("LoadRepositories", err) + if !project.CanBeAccessedByOwnerRepo(issue.Repo.OwnerID, issue.Repo) { + ctx.ServerError("Some issue's repoID is not equal to project's repoID", errors.New("Some issue's repoID is not equal to project's repoID")) return } - - for _, issue := range movedIssues { - if !project.CanBeAccessedByOwnerRepo(issue.Repo.OwnerID, issue.Repo) { - ctx.ServerError("Some issue's repoID is not equal to project's repoID", errors.New("Some issue's repoID is not equal to project's repoID")) - return - } - } - - if err = project_service.MoveIssuesOnProjectColumn(ctx, ctx.Doer, column, sortedIssueIDs); err != nil { - ctx.ServerError("MoveIssuesOnProjectColumn", err) + if err = project_service.MoveIssueAfter(ctx, ctx.Doer, column, issue, form.PrevIssueID); err != nil { + ctx.ServerError("MoveIssueAfter", err) return } diff --git a/routers/web/shared/project/project_test.go b/routers/web/shared/project/project_test.go index 33c5eb0bc8b..5035810c18f 100644 --- a/routers/web/shared/project/project_test.go +++ b/routers/web/shared/project/project_test.go @@ -84,7 +84,7 @@ func TestMoveIssuesNeedsOnlyProjectWrite(t *testing.T) { require.NoError(t, err) ctx.SetPathParam("id", strconv.FormatInt(project.ID, 10)) ctx.SetPathParam("columnID", strconv.FormatInt(column.ID, 10)) - ctx.Req.Body = io.NopCloser(strings.NewReader(fmt.Sprintf(`{"issues":[{"issueID":%d,"sorting":0}]}`, issue.ID))) + ctx.Req.Body = io.NopCloser(strings.NewReader(fmt.Sprintf(`{"issueID":%d,"prevIssueID":0}`, issue.ID))) MoveIssues(ctx) assert.Equal(t, http.StatusOK, ctx.Resp.WrittenStatus()) diff --git a/services/projects/issue.go b/services/projects/issue.go index f17950f871f..d707626e19f 100644 --- a/services/projects/issue.go +++ b/services/projects/issue.go @@ -99,10 +99,35 @@ func RemoveIssueFromColumn(ctx context.Context, doer *user_model.User, issue *is return nil } -// MoveIssuesOnProjectColumn moves or keeps issues in a column and sorts them inside that column -func MoveIssuesOnProjectColumn(ctx context.Context, doer *user_model.User, column *project_model.Column, sortedIssueIDs map[int64]int64) error { +// MoveIssueAfter places the issue after prevIssueID, first for 0, last when it left the column. +func MoveIssueAfter(ctx context.Context, doer *user_model.User, column *project_model.Column, issue *issues_model.Issue, prevIssueID int64) error { moves, err := db.WithTx2(ctx, func(ctx context.Context) ([]columnMove, error) { - return moveIssuesOnProjectColumn(ctx, doer, column, sortedIssueIDs) + if err := project_model.LockProject(ctx, column.ProjectID); err != nil { + return nil, err + } + others, err := project_model.GetColumnIssueSortings(ctx, column, issue.ID) + if err != nil { + return nil, err + } + pos := util.Iif(prevIssueID == 0, 0, len(others)) + if i := slices.IndexFunc(others, func(row project_model.ColumnIssueSorting) bool { return row.IssueID == prevIssueID }); i >= 0 { + pos = i + 1 + } + + moves, err := moveIssuesOnProjectColumn(ctx, doer, column, map[int64]int64{int64(pos): issue.ID}) + if err != nil { + return nil, err + } + for i, other := range others { + newSorting := int64(i + util.Iif(i >= pos, 1, 0)) + if other.Sorting == newSorting { + continue + } + if err := project_model.SetIssueSorting(ctx, column, other.IssueID, newSorting); err != nil { + return nil, err + } + } + return moves, nil }) if err != nil { return err diff --git a/services/projects/issue_test.go b/services/projects/issue_test.go index 6360445466a..1f490d0e26a 100644 --- a/services/projects/issue_test.go +++ b/services/projects/issue_test.go @@ -156,7 +156,7 @@ func Test_Projects(t *testing.T) { // move the issue into a non-default column of the first project only targetColumn := &project_model.Column{Title: "target", ProjectID: projects[0].ID} assert.NoError(t, project_model.NewColumn(t.Context(), targetColumn)) - assert.NoError(t, MoveIssuesOnProjectColumn(t.Context(), user2, targetColumn, map[int64]int64{0: issue11.ID})) + assert.NoError(t, MoveIssueAfter(t.Context(), user2, targetColumn, issue11, 0)) unittest.AssertExistsAndLoadBean(t, &project_model.ProjectIssue{ IssueID: issue11.ID, ProjectID: projects[0].ID, ProjectColumnID: targetColumn.ID, @@ -166,6 +166,44 @@ func Test_Projects(t *testing.T) { }) }) + t.Run("Moving an issue never moves other cards", func(t *testing.T) { + project := &project_model.Project{Title: "incremental move", RepoID: 1, Type: project_model.TypeRepository} + assert.NoError(t, project_model.NewProject(t.Context(), project)) + defer func() { + assert.NoError(t, project_model.DeleteProjectByID(t.Context(), project.ID)) + }() + colA := &project_model.Column{Title: "A", ProjectID: project.ID} + colB := &project_model.Column{Title: "B", ProjectID: project.ID} + assert.NoError(t, project_model.NewColumn(t.Context(), colA)) + assert.NoError(t, project_model.NewColumn(t.Context(), colB)) + for i, id := range []int64{1, 2, 3, 5} { + assert.NoError(t, db.Insert(t.Context(), &project_model.ProjectIssue{IssueID: id, ProjectID: project.ID, ProjectColumnID: colA.ID, Sorting: int64(i)})) + } + issue := func(id int64) *issues_model.Issue { + return unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: id}) + } + order := func(col *project_model.Column) (ids []int64) { + rows, err := project_model.GetColumnIssueSortings(t.Context(), col, 0) + assert.NoError(t, err) + for i, row := range rows { + assert.EqualValues(t, i, row.Sorting) + ids = append(ids, row.IssueID) + } + return ids + } + + assert.NoError(t, MoveIssueAfter(t.Context(), user2, colB, issue(1), 0)) + assert.NoError(t, MoveIssueAfter(t.Context(), user2, colA, issue(2), 1)) + assert.Equal(t, []int64{3, 5, 2}, order(colA)) + assert.NoError(t, MoveIssueAfter(t.Context(), user2, colA, issue(5), 0)) + assert.Equal(t, []int64{5, 3, 2}, order(colA)) + assert.NoError(t, MoveIssueAfter(t.Context(), user2, colA, issue(5), 3)) + assert.Equal(t, []int64{3, 5, 2}, order(colA)) + assert.Equal(t, []int64{1}, order(colB)) + assert.NoError(t, project_model.SetIssueSorting(t.Context(), colA, 1, 99)) + assert.Equal(t, []int64{1}, order(colB)) + }) + t.Run("Repository projects", func(t *testing.T) { repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) diff --git a/tests/e2e/issue-project.test.ts b/tests/e2e/issue-project.test.ts index 10466cd822f..0e714016651 100644 --- a/tests/e2e/issue-project.test.ts +++ b/tests/e2e/issue-project.test.ts @@ -1,6 +1,25 @@ import {env} from 'node:process'; import {test, expect} from '@playwright/test'; -import {login, apiCreateRepo, apiCreateIssue, apiDeleteRepo, createProject, createProjectColumn, randomString} from './utils.ts'; +import { + login, apiCreateRepo, apiCreateIssue, apiCreateProject, apiDeleteRepo, createProject, createProjectColumn, randomString, +} from './utils.ts'; +import type {Page} from '@playwright/test'; + +const projectCards = (page: Page, column: string) => page.locator('.project-column', {hasText: column}).locator('.cards'); + +async function dragProjectCardToTop(page: Page, title: string, column: string) { + const card = page.locator('.issue-card', {hasText: title}); + const cards = projectCards(page, column); + const moved = page.waitForResponse((resp) => resp.url().endsWith('/move')); + await card.hover(); + await page.mouse.down(); + await card.hover({position: {x: 10, y: 10}}); + await expect(card).toHaveClass(/card-ghost/); // sortable ignores dragover until its deferred drag start ran + await cards.hover({position: {x: 10, y: 10}}); + await expect(cards.locator('.issue-card').first()).toContainText(title); + await page.mouse.up(); + await moved; +} test('assign issue to project and change column', async ({page}) => { const repoName = `e2e-issue-project-${randomString(8)}`; @@ -441,3 +460,25 @@ test('select projects on new issue page shows in sidebar', async ({page}) => { await apiDeleteRepo(page.request, env.GITEA_TEST_E2E_USER, repoName); } }); + +test('project board drag keeps a card moved in another tab', async ({page}) => { + const owner = env.GITEA_TEST_E2E_USER; + const repo = `e2e-project-board-${randomString(8)}`; + const [otherPage] = await Promise.all([ + page.context().newPage(), login(page), apiCreateRepo(page.request, {name: repo, autoInit: false}), + ]); + try { + const projectID = await apiCreateProject(page.request, owner, repo, 'Board', {templateType: 'basic_kanban'}); + for (const title of ['first', 'second']) await apiCreateIssue(page.request, {owner, repo, title, projects: [projectID]}); + await Promise.all([page, otherPage].map((board) => board.goto(`/${owner}/${repo}/projects/${projectID}`))); + + await dragProjectCardToTop(otherPage, 'first', 'To Do'); + await dragProjectCardToTop(page, 'second', 'Backlog'); + + await page.reload(); + await expect(projectCards(page, 'Backlog').locator('.issue-card')).toHaveText([/second/]); + await expect(projectCards(page, 'To Do').locator('.issue-card')).toHaveText([/first/]); + } finally { + await apiDeleteRepo(page.request, owner, repo); + } +}); diff --git a/tests/e2e/utils.ts b/tests/e2e/utils.ts index a0bb1a4babb..9cde276f009 100644 --- a/tests/e2e/utils.ts +++ b/tests/e2e/utils.ts @@ -130,10 +130,10 @@ export async function createProjectColumn(requestContext: APIRequestContext, own }), 'createProjectColumn'); } -export async function apiCreateProject(requestContext: APIRequestContext, owner: string, repo: string, title: string): Promise { +export async function apiCreateProject(requestContext: APIRequestContext, owner: string, repo: string, title: string, {templateType}: {templateType?: string} = {}): Promise { const response = await apiRetry(() => requestContext.post(`${baseUrl()}/api/v1/repos/${owner}/${repo}/projects`, { headers: apiHeaders(), - data: {title}, + data: {title, template_type: templateType}, }), 'apiCreateProject'); return (await response.json()).id; } diff --git a/tests/integration/project_workflow_test.go b/tests/integration/project_workflow_test.go index 57d40278e3a..61331ea3a2d 100644 --- a/tests/integration/project_workflow_test.go +++ b/tests/integration/project_workflow_test.go @@ -84,7 +84,7 @@ func TestProjectWorkflowsWeb(t *testing.T) { assert.Contains(t, NewHTMLParser(t, resp.Body).Find(".timeline-item.event .comment-text-line").Text(), "Project workflow Item added to project") req = NewRequestWithJSON(t, "POST", link+"/3/move", map[string]any{ - "issues": []map[string]any{{"issueID": created.ID, "sorting": 0}}, + "issueID": created.ID, "prevIssueID": 0, }) session.MakeRequest(t, req, http.StatusOK) diff --git a/web_src/js/features/repo-projects.ts b/web_src/js/features/repo-projects.ts index ddcca73c11a..be6d175b063 100644 --- a/web_src/js/features/repo-projects.ts +++ b/web_src/js/features/repo-projects.ts @@ -16,27 +16,19 @@ function updateIssueCount(card: HTMLElement): void { } async function moveIssue({item, from, to, oldIndex}: SortableEvent): Promise { - const columnCards = to.querySelectorAll('.issue-card'); updateIssueCount(from); updateIssueCount(to); - const columnSorting = { - issues: Array.from(columnCards, (card, i) => ({ - issueID: parseInt(card.getAttribute('data-issue')!), - sorting: i, - })), - }; - - try { - await POST(`${to.getAttribute('data-url')}/move`, { - data: columnSorting, - }); - } catch (error) { - console.error(error); - if (oldIndex !== undefined) { - from.insertBefore(item, from.children[oldIndex]); - } - } + const prevCard = item.previousElementSibling; + const resp = await performFetchActionRequest(item, { + method: 'POST', + url: `${to.getAttribute('data-url')}/move`, + data: { + issueID: parseInt(item.getAttribute('data-issue')!), + prevIssueID: prevCard ? parseInt(prevCard.getAttribute('data-issue')!) : 0, + }, + }); + if (!resp && oldIndex !== undefined) from.insertBefore(item, from.children[oldIndex]); } function initRepoProjectSortable(): void { @@ -49,7 +41,7 @@ function initRepoProjectSortable(): void { handle: '.project-column-header', delayOnTouchOnly: true, delay: 500, - onSort: async () => { // eslint-disable-line @typescript-eslint/no-misused-promises -- Sortable ignores the returned promise, the body catches its own errors + onSort: async () => { boardColumns = mainBoard.querySelectorAll('.project-column'); const columnSorting = { @@ -73,8 +65,8 @@ function initRepoProjectSortable(): void { const boardCardList = boardColumn.querySelector('.cards')!; createSortable(boardCardList, { group: 'shared', - onAdd: moveIssue, // eslint-disable-line @typescript-eslint/no-misused-promises -- Sortable ignores the returned promise, moveIssue catches its own errors - onUpdate: moveIssue, // eslint-disable-line @typescript-eslint/no-misused-promises -- Sortable ignores the returned promise, moveIssue catches its own errors + onAdd: moveIssue, + onUpdate: moveIssue, delayOnTouchOnly: true, delay: 500, }); diff --git a/web_src/js/features/tribute.ts b/web_src/js/features/tribute.ts index e6192408691..682636e5eca 100644 --- a/web_src/js/features/tribute.ts +++ b/web_src/js/features/tribute.ts @@ -31,7 +31,7 @@ export function attachTribute(element: HTMLElement) { }; const mentionCollection: TributeCollection = { - values: async (_query: string, cb: (matches: Mention[]) => void) => { // eslint-disable-line @typescript-eslint/no-misused-promises -- tributejs ignores the returned promise, results arrive via the callback + values: async (_query: string, cb: (matches: Mention[]) => void) => { cb(mentionsUrl ? await fetchMentions(mentionsUrl) : []); }, requireLeadingSpace: true,