mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-03-26 02:13:05 -04:00
Project columns and cards use a sorting field for ordering, but nothing prevents duplicate values from being inserted. This causes unpredictable ordering and makes swap-based reordering unsafe. Additionally, the same issue can be added to a project multiple times, which shouldn't be possible. This PR adds a migration to clean up existing data and enforce three unique constraints: - (project_id, sorting) on project_board — one sorting value per column per project - (project_id, issue_id) on project_issue — one card per issue per project - (project_board_id, sorting) on project_issue — one sorting value per card per column The migration deduplicates existing rows and reassigns sequential sorting values before adding the constraints. Changes - Migration: fix duplicate sorting values in project_board and project_issue, remove duplicate (project_id, issue_id) rows, add three unique constraints - MoveColumnsOnProject: two-phase swap (negate then set) to avoid constraint collisions - MoveIssuesOnProjectColumn: three-phase approach with duplicate validation and sorted lock ordering - UpdateColumn: always persist sorting field (allows setting to 0) - GetDefaultColumn: query max sorting before auto-creating - createDefaultColumnsForProject: explicit sequential sorting - changeProjectStatus: only set ClosedDateUnix when closing ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. *The decision if the pull request will be shown in the release notes is up to the mergers / release team.* The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11334 Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org> Co-authored-by: Myers Carpenter <myers@maski.org> Co-committed-by: Myers Carpenter <myers@maski.org>
149 lines
4.8 KiB
Go
149 lines
4.8 KiB
Go
// Copyright 2020 The Gitea Authors. All rights reserved.
|
|
// SPDX-License-Identifier: MIT
|
|
|
|
package project
|
|
|
|
import (
|
|
"testing"
|
|
|
|
"forgejo.org/models/db"
|
|
"forgejo.org/models/unittest"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestMoveIssuesOnProjectColumn(t *testing.T) {
|
|
require.NoError(t, unittest.PrepareTestDatabase())
|
|
|
|
// Get column 1 which belongs to project 1 and has issue 1
|
|
column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1})
|
|
require.Equal(t, int64(1), column.ProjectID)
|
|
|
|
t.Run("Success", func(t *testing.T) {
|
|
// Issue 1 is in column 1 (from fixtures)
|
|
sortedIssueIDs := map[int64]int64{
|
|
0: 1, // sorting position 0 -> issue_id 1
|
|
}
|
|
err := MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs)
|
|
require.NoError(t, err)
|
|
|
|
// Verify the sorting was updated using direct DB query
|
|
var card ProjectIssue
|
|
has, err := db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", column.ProjectID, 1).Get(&card)
|
|
require.NoError(t, err)
|
|
require.True(t, has)
|
|
assert.Equal(t, int64(0), card.Sorting)
|
|
})
|
|
|
|
t.Run("MoveIssueFromDifferentColumn", func(t *testing.T) {
|
|
// Issue 3 is in column 2, not column 1 — but same project, so cross-column move should succeed
|
|
sortedIssueIDs := map[int64]int64{
|
|
0: 3,
|
|
}
|
|
err := MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs)
|
|
require.NoError(t, err)
|
|
|
|
// Verify the card was moved to column 1 and sorting updated
|
|
var card ProjectIssue
|
|
has, err := db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", column.ProjectID, 3).Get(&card)
|
|
require.NoError(t, err)
|
|
require.True(t, has)
|
|
assert.Equal(t, column.ID, card.ProjectColumnID)
|
|
assert.Equal(t, int64(0), card.Sorting)
|
|
})
|
|
|
|
t.Run("ErrorIssueNotInProject", func(t *testing.T) {
|
|
// Issue 999 doesn't exist
|
|
sortedIssueIDs := map[int64]int64{
|
|
0: 999,
|
|
}
|
|
err := MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs)
|
|
require.Error(t, err)
|
|
assert.Contains(t, err.Error(), "all issues must belong to the specified project")
|
|
})
|
|
}
|
|
|
|
func TestMoveIssuesOnProjectColumnSwap(t *testing.T) {
|
|
require.NoError(t, unittest.PrepareTestDatabase())
|
|
|
|
column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1})
|
|
|
|
// Setup: insert two cards at distinct positions using direct DB inserts
|
|
card1 := &ProjectIssue{
|
|
IssueID: 14,
|
|
ProjectID: 1,
|
|
ProjectColumnID: column.ID,
|
|
Sorting: 10,
|
|
}
|
|
card2 := &ProjectIssue{
|
|
IssueID: 15,
|
|
ProjectID: 1,
|
|
ProjectColumnID: column.ID,
|
|
Sorting: 11,
|
|
}
|
|
_, err := db.GetEngine(db.DefaultContext).Insert(card1)
|
|
require.NoError(t, err)
|
|
_, err = db.GetEngine(db.DefaultContext).Insert(card2)
|
|
require.NoError(t, err)
|
|
|
|
// Swap them: card at 10→11, card at 11→10
|
|
sortedIssueIDs := map[int64]int64{
|
|
11: 14, // issue 14 goes to position 11
|
|
10: 15, // issue 15 goes to position 10
|
|
}
|
|
err = MoveIssuesOnProjectColumn(db.DefaultContext, column, sortedIssueIDs)
|
|
require.NoError(t, err)
|
|
|
|
var resultCard14 ProjectIssue
|
|
has, err := db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", 1, 14).Get(&resultCard14)
|
|
require.NoError(t, err)
|
|
require.True(t, has)
|
|
assert.Equal(t, int64(11), resultCard14.Sorting)
|
|
|
|
var resultCard15 ProjectIssue
|
|
has, err = db.GetEngine(db.DefaultContext).Where("project_id=? AND issue_id=?", 1, 15).Get(&resultCard15)
|
|
require.NoError(t, err)
|
|
require.True(t, has)
|
|
assert.Equal(t, int64(10), resultCard15.Sorting)
|
|
}
|
|
|
|
func TestMoveIssuesOnProjectColumnEmptyMap(t *testing.T) {
|
|
require.NoError(t, unittest.PrepareTestDatabase())
|
|
|
|
column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1})
|
|
err := MoveIssuesOnProjectColumn(db.DefaultContext, column, map[int64]int64{})
|
|
require.NoError(t, err) // empty map should be a no-op
|
|
}
|
|
|
|
func TestMoveIssuesOnProjectColumnDuplicateIssueIDs(t *testing.T) {
|
|
require.NoError(t, unittest.PrepareTestDatabase())
|
|
|
|
column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1})
|
|
err := MoveIssuesOnProjectColumn(db.DefaultContext, column, map[int64]int64{
|
|
0: 1,
|
|
1: 1, // duplicate issue ID
|
|
})
|
|
require.Error(t, err)
|
|
assert.Contains(t, err.Error(), "duplicate issue IDs")
|
|
}
|
|
|
|
func TestMoveIssuesToAnotherColumnErrorPaths(t *testing.T) {
|
|
require.NoError(t, unittest.PrepareTestDatabase())
|
|
|
|
t.Run("DifferentProject", func(t *testing.T) {
|
|
col1 := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1, ProjectID: 1})
|
|
col5 := unittest.AssertExistsAndLoadBean(t, &Column{ID: 5, ProjectID: 2})
|
|
|
|
err := col1.moveIssuesToAnotherColumn(db.DefaultContext, col5)
|
|
require.Error(t, err)
|
|
assert.Contains(t, err.Error(), "columns have to be in the same project")
|
|
})
|
|
|
|
t.Run("SameColumnIsNoOp", func(t *testing.T) {
|
|
col1 := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1, ProjectID: 1})
|
|
|
|
err := col1.moveIssuesToAnotherColumn(db.DefaultContext, col1)
|
|
require.NoError(t, err)
|
|
})
|
|
}
|