mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
fix(models): deduplicate project sorting values and add unique constraints (#11334)
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>
This commit is contained in:
parent
a4821c2bf9
commit
2d4a3e5658
12 changed files with 574 additions and 25 deletions
|
|
@ -4,6 +4,7 @@
|
|||
title: Triage
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 0
|
||||
created_unix: 1738000000
|
||||
updated_unix: 1738000000
|
||||
|
||||
|
|
@ -13,5 +14,6 @@
|
|||
title: Triage
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 0
|
||||
created_unix: 1738000000
|
||||
updated_unix: 1738000000
|
||||
|
|
|
|||
|
|
@ -3,21 +3,25 @@
|
|||
issue_id: 6
|
||||
project_id: 1001
|
||||
project_board_id: 1001
|
||||
sorting: 1
|
||||
|
||||
-
|
||||
id: 1002
|
||||
issue_id: 7
|
||||
project_id: 1002
|
||||
project_board_id: 1002
|
||||
sorting: 0
|
||||
|
||||
-
|
||||
id: 1003
|
||||
issue_id: 16
|
||||
project_id: 1001
|
||||
project_board_id: 1001
|
||||
sorting: 0
|
||||
|
||||
-
|
||||
id: 1004
|
||||
issue_id: 1
|
||||
project_id: 1002
|
||||
project_board_id: 1002
|
||||
sorting: 1
|
||||
|
|
|
|||
|
|
@ -4,5 +4,6 @@
|
|||
title: Triage
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 0
|
||||
created_unix: 1738000000
|
||||
updated_unix: 1738000000
|
||||
|
|
|
|||
|
|
@ -3,9 +3,11 @@
|
|||
issue_id: 6
|
||||
project_id: 1001
|
||||
project_board_id: 1001
|
||||
sorting: 1
|
||||
|
||||
-
|
||||
id: 1002
|
||||
issue_id: 15
|
||||
project_id: 1001
|
||||
project_board_id: 1001
|
||||
sorting: 0
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@
|
|||
title: To Do
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 0
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -12,6 +13,7 @@
|
|||
project_id: 1
|
||||
title: In Progress
|
||||
creator_id: 2
|
||||
sorting: 1
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -20,6 +22,7 @@
|
|||
project_id: 1
|
||||
title: Done
|
||||
creator_id: 2
|
||||
sorting: 2
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -28,6 +31,7 @@
|
|||
project_id: 4
|
||||
title: Done
|
||||
creator_id: 2
|
||||
sorting: 0
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -37,6 +41,7 @@
|
|||
title: Backlog
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 0
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -46,6 +51,7 @@
|
|||
title: Backlog
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 1
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -55,6 +61,7 @@
|
|||
title: Done
|
||||
creator_id: 2
|
||||
default: false
|
||||
sorting: 0
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -64,6 +71,7 @@
|
|||
title: Backlog
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 0
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -73,5 +81,6 @@
|
|||
title: Uncategorized
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 1
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
|
|
|||
|
|
@ -3,21 +3,25 @@
|
|||
issue_id: 1
|
||||
project_id: 1
|
||||
project_board_id: 1
|
||||
sorting: 0
|
||||
|
||||
-
|
||||
id: 2
|
||||
issue_id: 2
|
||||
project_id: 1
|
||||
project_board_id: 0 # no board assigned
|
||||
sorting: 0
|
||||
|
||||
-
|
||||
id: 3
|
||||
issue_id: 3
|
||||
project_id: 1
|
||||
project_board_id: 2
|
||||
sorting: 0
|
||||
|
||||
-
|
||||
id: 4
|
||||
issue_id: 5
|
||||
project_id: 1
|
||||
project_board_id: 3
|
||||
sorting: 0
|
||||
|
|
|
|||
|
|
@ -0,0 +1,218 @@
|
|||
// Copyright 2025 The Forgejo Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: GPL-3.0-or-later
|
||||
|
||||
package forgejo_migrations
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
|
||||
"forgejo.org/models/gitea_migrations/base"
|
||||
"forgejo.org/modules/setting"
|
||||
|
||||
"xorm.io/xorm"
|
||||
)
|
||||
|
||||
func init() {
|
||||
registerMigration(&Migration{
|
||||
Description: "Fix duplicate project sorting values and add unique constraints",
|
||||
Upgrade: fixProjectSortingUniqueConstraints,
|
||||
})
|
||||
}
|
||||
|
||||
func fixProjectSortingUniqueConstraints(x *xorm.Engine) error {
|
||||
// Step 1: Fix existing duplicates in project_issue (cards)
|
||||
// Reassign sequential sorting values within each column
|
||||
if err := fixProjectIssueDuplicates(x); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Step 2: Fix existing duplicates in project_board (columns)
|
||||
// Reassign sequential sorting values within each project
|
||||
if err := fixProjectBoardDuplicates(x); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Step 3: Remove duplicate (project_id, issue_id) rows keeping the lowest id
|
||||
if err := fixProjectIssueDuplicateAssignments(x); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Step 4: Add unique constraints (idempotent — skip if already exists)
|
||||
if err := createUniqueIndexIfNotExists(x, "project_issue", "UQE_project_issue_column_sorting", "project_board_id, sorting"); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := createUniqueIndexIfNotExists(x, "project_issue", "UQE_project_issue_project_issue", "project_id, issue_id"); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := createUniqueIndexIfNotExists(x, "project_board", "UQE_project_board_project_sorting", "project_id, sorting"); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Step 5: Enforce NOT NULL on project_issue columns.
|
||||
// The struct tags declare NOT NULL but the DB may not enforce it.
|
||||
// On SQLite, RecreateTables rebuilds the table with NOT NULL and unique constraints.
|
||||
return enforceProjectIssueNotNull(x)
|
||||
}
|
||||
|
||||
func createUniqueIndexIfNotExists(x *xorm.Engine, tableName, indexName, columns string) error {
|
||||
exists, err := indexExists(x, tableName, indexName)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if exists {
|
||||
return nil
|
||||
}
|
||||
_, err = x.Exec(fmt.Sprintf("CREATE UNIQUE INDEX %s ON %s (%s)", indexName, tableName, columns))
|
||||
return err
|
||||
}
|
||||
|
||||
func fixProjectIssueDuplicates(x *xorm.Engine) error {
|
||||
switch {
|
||||
case setting.Database.Type.IsSQLite3():
|
||||
// SQLite: Use UPDATE with subquery
|
||||
_, err := x.Exec(`
|
||||
UPDATE project_issue SET sorting = (
|
||||
SELECT new_sort FROM (
|
||||
SELECT id, ROW_NUMBER() OVER (PARTITION BY project_board_id ORDER BY sorting, id) - 1 as new_sort
|
||||
FROM project_issue
|
||||
) ranked WHERE ranked.id = project_issue.id
|
||||
)
|
||||
`)
|
||||
return err
|
||||
|
||||
case setting.Database.Type.IsPostgreSQL():
|
||||
// PostgreSQL: Use UPDATE FROM with subquery
|
||||
_, err := x.Exec(`
|
||||
UPDATE project_issue pi SET sorting = ranked.new_sort
|
||||
FROM (
|
||||
SELECT id, ROW_NUMBER() OVER (PARTITION BY project_board_id ORDER BY sorting, id) - 1 as new_sort
|
||||
FROM project_issue
|
||||
) ranked
|
||||
WHERE pi.id = ranked.id
|
||||
`)
|
||||
return err
|
||||
|
||||
case setting.Database.Type.IsMySQL():
|
||||
// MySQL: Use UPDATE with JOIN
|
||||
_, err := x.Exec(`
|
||||
UPDATE project_issue pi
|
||||
INNER JOIN (
|
||||
SELECT id, ROW_NUMBER() OVER (PARTITION BY project_board_id ORDER BY sorting, id) - 1 as new_sort
|
||||
FROM project_issue
|
||||
) ranked ON pi.id = ranked.id
|
||||
SET pi.sorting = ranked.new_sort
|
||||
`)
|
||||
return err
|
||||
|
||||
default:
|
||||
return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType)
|
||||
}
|
||||
}
|
||||
|
||||
func fixProjectBoardDuplicates(x *xorm.Engine) error {
|
||||
switch {
|
||||
case setting.Database.Type.IsSQLite3():
|
||||
// SQLite: Use UPDATE with subquery
|
||||
_, err := x.Exec(`
|
||||
UPDATE project_board SET sorting = (
|
||||
SELECT new_sort FROM (
|
||||
SELECT id, ROW_NUMBER() OVER (PARTITION BY project_id ORDER BY sorting, id) - 1 as new_sort
|
||||
FROM project_board
|
||||
) ranked WHERE ranked.id = project_board.id
|
||||
)
|
||||
`)
|
||||
return err
|
||||
|
||||
case setting.Database.Type.IsPostgreSQL():
|
||||
// PostgreSQL: Use UPDATE FROM with subquery
|
||||
_, err := x.Exec(`
|
||||
UPDATE project_board pb SET sorting = ranked.new_sort
|
||||
FROM (
|
||||
SELECT id, ROW_NUMBER() OVER (PARTITION BY project_id ORDER BY sorting, id) - 1 as new_sort
|
||||
FROM project_board
|
||||
) ranked
|
||||
WHERE pb.id = ranked.id
|
||||
`)
|
||||
return err
|
||||
|
||||
case setting.Database.Type.IsMySQL():
|
||||
// MySQL: Use UPDATE with JOIN
|
||||
_, err := x.Exec(`
|
||||
UPDATE project_board pb
|
||||
INNER JOIN (
|
||||
SELECT id, ROW_NUMBER() OVER (PARTITION BY project_id ORDER BY sorting, id) - 1 as new_sort
|
||||
FROM project_board
|
||||
) ranked ON pb.id = ranked.id
|
||||
SET pb.sorting = ranked.new_sort
|
||||
`)
|
||||
return err
|
||||
|
||||
default:
|
||||
return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType)
|
||||
}
|
||||
}
|
||||
|
||||
func enforceProjectIssueNotNull(x *xorm.Engine) error {
|
||||
switch {
|
||||
case setting.Database.Type.IsSQLite3():
|
||||
type ProjectIssue struct {
|
||||
ID int64 `xorm:"pk autoincr"`
|
||||
IssueID int64 `xorm:"INDEX NOT NULL unique(project_issue)"`
|
||||
ProjectID int64 `xorm:"INDEX NOT NULL unique(project_issue)"`
|
||||
ProjectColumnID int64 `xorm:"'project_board_id' INDEX NOT NULL unique(column_sorting)"`
|
||||
Sorting int64 `xorm:"NOT NULL DEFAULT 0 unique(column_sorting)"`
|
||||
}
|
||||
return base.RecreateTables(new(ProjectIssue))(x)
|
||||
|
||||
case setting.Database.Type.IsPostgreSQL():
|
||||
for _, col := range []string{"issue_id", "project_id", "project_board_id"} {
|
||||
if _, err := x.Exec(fmt.Sprintf("ALTER TABLE project_issue ALTER COLUMN %s SET NOT NULL", col)); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
|
||||
case setting.Database.Type.IsMySQL():
|
||||
for _, col := range []string{"issue_id", "project_id", "project_board_id"} {
|
||||
if _, err := x.Exec(fmt.Sprintf("ALTER TABLE project_issue MODIFY COLUMN %s BIGINT NOT NULL", col)); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
|
||||
default:
|
||||
return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType)
|
||||
}
|
||||
}
|
||||
|
||||
// fixProjectIssueDuplicateAssignments removes duplicate (project_id, issue_id) rows,
|
||||
// keeping only the row with the lowest id for each pair.
|
||||
func fixProjectIssueDuplicateAssignments(x *xorm.Engine) error {
|
||||
switch {
|
||||
case setting.Database.Type.IsSQLite3():
|
||||
_, err := x.Exec(`
|
||||
DELETE FROM project_issue WHERE id NOT IN (
|
||||
SELECT MIN(id) FROM project_issue GROUP BY project_id, issue_id
|
||||
)
|
||||
`)
|
||||
return err
|
||||
|
||||
case setting.Database.Type.IsPostgreSQL():
|
||||
_, err := x.Exec(`
|
||||
DELETE FROM project_issue pi USING project_issue pi2
|
||||
WHERE pi.project_id = pi2.project_id AND pi.issue_id = pi2.issue_id AND pi.id > pi2.id
|
||||
`)
|
||||
return err
|
||||
|
||||
case setting.Database.Type.IsMySQL():
|
||||
_, err := x.Exec(`
|
||||
DELETE pi FROM project_issue pi
|
||||
INNER JOIN project_issue pi2
|
||||
ON pi.project_id = pi2.project_id AND pi.issue_id = pi2.issue_id AND pi.id > pi2.id
|
||||
`)
|
||||
return err
|
||||
|
||||
default:
|
||||
return fmt.Errorf("unsupported db dialect type %v", x.Dialect().URI().DBType)
|
||||
}
|
||||
}
|
||||
|
|
@ -4,6 +4,7 @@
|
|||
title: Done
|
||||
creator_id: 2
|
||||
default: false
|
||||
sorting: 1
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -13,6 +14,7 @@
|
|||
title: Backlog
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 0
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
||||
|
|
@ -22,5 +24,6 @@
|
|||
title: Uncategorized
|
||||
creator_id: 2
|
||||
default: true
|
||||
sorting: 1
|
||||
created_unix: 1588117528
|
||||
updated_unix: 1588117528
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ import (
|
|||
"regexp"
|
||||
|
||||
"forgejo.org/models/db"
|
||||
"forgejo.org/modules/container"
|
||||
"forgejo.org/modules/setting"
|
||||
"forgejo.org/modules/timeutil"
|
||||
"forgejo.org/modules/util"
|
||||
|
|
@ -42,10 +43,10 @@ type Column struct {
|
|||
ID int64 `xorm:"pk autoincr"`
|
||||
Title string
|
||||
Default bool `xorm:"NOT NULL DEFAULT false"` // issues not assigned to a specific column will be assigned to this column
|
||||
Sorting int8 `xorm:"NOT NULL DEFAULT 0"`
|
||||
Sorting int8 `xorm:"NOT NULL DEFAULT 0 unique(project_sorting)"`
|
||||
Color string `xorm:"VARCHAR(7)"`
|
||||
|
||||
ProjectID int64 `xorm:"INDEX NOT NULL"`
|
||||
ProjectID int64 `xorm:"INDEX NOT NULL unique(project_sorting)"`
|
||||
CreatorID int64 `xorm:"NOT NULL"`
|
||||
|
||||
CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"`
|
||||
|
|
@ -103,8 +104,9 @@ func createDefaultColumnsForProject(ctx context.Context, project *Project) error
|
|||
Title: "Backlog",
|
||||
ProjectID: project.ID,
|
||||
Default: true,
|
||||
Sorting: 0,
|
||||
}
|
||||
if err := db.Insert(ctx, column); err != nil {
|
||||
if err := db.Insert(ctx, &column); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
|
|
@ -113,12 +115,13 @@ func createDefaultColumnsForProject(ctx context.Context, project *Project) error
|
|||
}
|
||||
|
||||
columns := make([]Column, 0, len(items))
|
||||
for _, v := range items {
|
||||
for i, v := range items {
|
||||
columns = append(columns, Column{
|
||||
CreatedUnix: timeutil.TimeStampNow(),
|
||||
CreatorID: project.CreatorID,
|
||||
Title: v,
|
||||
ProjectID: project.ID,
|
||||
Sorting: int8(i + 1),
|
||||
})
|
||||
}
|
||||
|
||||
|
|
@ -215,9 +218,7 @@ func GetColumn(ctx context.Context, columnID int64) (*Column, error) {
|
|||
func UpdateColumn(ctx context.Context, column *Column) error {
|
||||
var fieldToUpdate []string
|
||||
|
||||
if column.Sorting != 0 {
|
||||
fieldToUpdate = append(fieldToUpdate, "sorting")
|
||||
}
|
||||
fieldToUpdate = append(fieldToUpdate, "sorting")
|
||||
|
||||
if column.Title != "" {
|
||||
fieldToUpdate = append(fieldToUpdate, "title")
|
||||
|
|
@ -257,12 +258,22 @@ func (p *Project) GetDefaultColumn(ctx context.Context) (*Column, error) {
|
|||
return &column, nil
|
||||
}
|
||||
|
||||
// create a default column if none is found
|
||||
// create a default column if none is found, using the next available sorting value
|
||||
res := struct {
|
||||
MaxSorting int64
|
||||
ColumnCount int64
|
||||
}{}
|
||||
if _, err := db.GetEngine(ctx).Select("max(sorting) as max_sorting, count(*) as column_count").
|
||||
Table("project_board").Where("project_id=?", p.ID).Get(&res); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
column = Column{
|
||||
ProjectID: p.ID,
|
||||
Default: true,
|
||||
Title: "Uncategorized",
|
||||
CreatorID: p.CreatorID,
|
||||
Sorting: int8(util.Iif(res.ColumnCount > 0, res.MaxSorting+1, 0)),
|
||||
}
|
||||
if _, err := db.GetEngine(ctx).Insert(&column); err != nil {
|
||||
return nil, err
|
||||
|
|
@ -305,27 +316,59 @@ func GetColumnsByIDs(ctx context.Context, projectID int64, columnsIDs []int64) (
|
|||
return columns, nil
|
||||
}
|
||||
|
||||
// MoveColumnsOnProject sorts columns in a project
|
||||
// MoveColumnsOnProject sorts columns in a project using a two-phase approach
|
||||
// to avoid unique constraint collisions during swap operations.
|
||||
// All columns in the project must be included in the sortedColumnIDs map.
|
||||
func MoveColumnsOnProject(ctx context.Context, project *Project, sortedColumnIDs map[int64]int64) error {
|
||||
return db.WithTx(ctx, func(ctx context.Context) error {
|
||||
sess := db.GetEngine(ctx)
|
||||
|
||||
// Validate no duplicate column IDs in map values
|
||||
columnIDSet := make(container.Set[int64], len(sortedColumnIDs))
|
||||
for _, columnID := range sortedColumnIDs {
|
||||
if !columnIDSet.Add(columnID) {
|
||||
return errors.New("duplicate column ID in reorder request")
|
||||
}
|
||||
}
|
||||
|
||||
// Validate all columns exist and belong to this project
|
||||
allColumns, err := project.GetColumns(ctx)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if len(allColumns) != len(sortedColumnIDs) {
|
||||
return errors.New("all columns in the project must be included in the reorder request")
|
||||
}
|
||||
|
||||
columnIDs := util.ValuesOfMap(sortedColumnIDs)
|
||||
movedColumns, err := GetColumnsByIDs(ctx, project.ID, columnIDs)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if len(movedColumns) != len(sortedColumnIDs) {
|
||||
return errors.New("some columns do not exist")
|
||||
return errors.New("some columns do not exist in this project")
|
||||
}
|
||||
|
||||
// Build reverse map: columnID → target sorting
|
||||
targetSortingByColumn := make(map[int64]int64, len(sortedColumnIDs))
|
||||
for sorting, columnID := range sortedColumnIDs {
|
||||
targetSortingByColumn[columnID] = sorting
|
||||
}
|
||||
|
||||
// Phase 1: negate using target sorting values (guaranteed unique since
|
||||
// they are map keys) to avoid unique constraint collisions during swap
|
||||
for _, column := range movedColumns {
|
||||
if column.ProjectID != project.ID {
|
||||
return fmt.Errorf("column[%d]'s projectID is not equal to project's ID [%d]", column.ProjectID, project.ID)
|
||||
targetSorting := targetSortingByColumn[column.ID]
|
||||
if _, err := sess.Exec("UPDATE `project_board` SET sorting=? WHERE id=?",
|
||||
-(targetSorting + 1), column.ID); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
// Phase 2: set final values
|
||||
for sorting, columnID := range sortedColumnIDs {
|
||||
if _, err := sess.Exec("UPDATE `project_board` SET sorting=? WHERE id=?", sorting, columnID); err != nil {
|
||||
if _, err := sess.Exec("UPDATE `project_board` SET sorting=? WHERE id=?",
|
||||
sorting, columnID); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -84,9 +84,9 @@ func Test_MoveColumnsOnProject(t *testing.T) {
|
|||
columns, err := project1.GetColumns(db.DefaultContext)
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, columns, 3)
|
||||
assert.EqualValues(t, 0, columns[0].Sorting) // even if there is no default sorting, the code should also work
|
||||
assert.EqualValues(t, 0, columns[1].Sorting)
|
||||
assert.EqualValues(t, 0, columns[2].Sorting)
|
||||
assert.EqualValues(t, 0, columns[0].Sorting)
|
||||
assert.EqualValues(t, 1, columns[1].Sorting)
|
||||
assert.EqualValues(t, 2, columns[2].Sorting)
|
||||
|
||||
err = MoveColumnsOnProject(db.DefaultContext, project1, map[int64]int64{
|
||||
0: columns[1].ID,
|
||||
|
|
@ -103,6 +103,59 @@ func Test_MoveColumnsOnProject(t *testing.T) {
|
|||
assert.Equal(t, columns[0].ID, columnsAfter[2].ID)
|
||||
}
|
||||
|
||||
func TestMoveColumnsOnProjectSwap(t *testing.T) {
|
||||
require.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
project1 := unittest.AssertExistsAndLoadBean(t, &Project{ID: 1})
|
||||
columns, err := project1.GetColumns(db.DefaultContext)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, columns, 3)
|
||||
|
||||
// First give them distinct positions
|
||||
err = MoveColumnsOnProject(db.DefaultContext, project1, map[int64]int64{
|
||||
0: columns[0].ID,
|
||||
1: columns[1].ID,
|
||||
2: columns[2].ID,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Now swap columns 0 and 1 (would collide under single-phase update)
|
||||
err = MoveColumnsOnProject(db.DefaultContext, project1, map[int64]int64{
|
||||
0: columns[1].ID,
|
||||
1: columns[0].ID,
|
||||
2: columns[2].ID,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
columnsAfter, err := project1.GetColumns(db.DefaultContext)
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, columnsAfter, 3)
|
||||
assert.Equal(t, columns[1].ID, columnsAfter[0].ID)
|
||||
assert.Equal(t, columns[0].ID, columnsAfter[1].ID)
|
||||
assert.Equal(t, columns[2].ID, columnsAfter[2].ID)
|
||||
}
|
||||
|
||||
func TestUpdateColumnSortingZero(t *testing.T) {
|
||||
require.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
column := unittest.AssertExistsAndLoadBean(t, &Column{ID: 1})
|
||||
column.Sorting = 5
|
||||
require.NoError(t, UpdateColumn(db.DefaultContext, column))
|
||||
|
||||
// Verify it was set to 5
|
||||
updated, err := GetColumn(db.DefaultContext, column.ID)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, int8(5), updated.Sorting)
|
||||
|
||||
// Now set it back to 0
|
||||
column.Sorting = 0
|
||||
require.NoError(t, UpdateColumn(db.DefaultContext, column))
|
||||
|
||||
updated, err = GetColumn(db.DefaultContext, column.ID)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, int8(0), updated.Sorting)
|
||||
}
|
||||
|
||||
func Test_NewColumn(t *testing.T) {
|
||||
require.NoError(t, unittest.PrepareTestDatabase())
|
||||
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ package project
|
|||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"slices"
|
||||
|
||||
"forgejo.org/models/db"
|
||||
"forgejo.org/modules/log"
|
||||
|
|
@ -15,14 +16,14 @@ import (
|
|||
// ProjectIssue saves relation from issue to a project
|
||||
type ProjectIssue struct { //revive:disable-line:exported
|
||||
ID int64 `xorm:"pk autoincr"`
|
||||
IssueID int64 `xorm:"INDEX"`
|
||||
ProjectID int64 `xorm:"INDEX"`
|
||||
IssueID int64 `xorm:"INDEX NOT NULL unique(project_issue)"`
|
||||
ProjectID int64 `xorm:"INDEX NOT NULL unique(project_issue)"`
|
||||
|
||||
// ProjectColumnID should not be zero since 1.22. If it's zero, the issue will not be displayed on UI and it might result in errors.
|
||||
ProjectColumnID int64 `xorm:"'project_board_id' INDEX"`
|
||||
ProjectColumnID int64 `xorm:"'project_board_id' INDEX NOT NULL unique(column_sorting)"`
|
||||
|
||||
// the sorting order on the column
|
||||
Sorting int64 `xorm:"NOT NULL DEFAULT 0"`
|
||||
Sorting int64 `xorm:"NOT NULL DEFAULT 0 unique(column_sorting)"`
|
||||
}
|
||||
|
||||
func init() {
|
||||
|
|
@ -62,26 +63,86 @@ func (p *Project) NumOpenIssues(ctx context.Context) int {
|
|||
return int(c)
|
||||
}
|
||||
|
||||
// MoveIssuesOnProjectColumn moves or keeps issues in a column and sorts them inside that column
|
||||
// MoveIssuesOnProjectColumn moves or keeps issues in a column and sorts them inside that column.
|
||||
// The sortedIssueIDs map keys are sorting positions and values are issue IDs.
|
||||
// Cards not in the map that already exist in the target column are shifted to
|
||||
// positions after the highest requested sorting value.
|
||||
func MoveIssuesOnProjectColumn(ctx context.Context, column *Column, sortedIssueIDs map[int64]int64) error {
|
||||
if len(sortedIssueIDs) == 0 {
|
||||
return nil
|
||||
}
|
||||
return db.WithTx(ctx, func(ctx context.Context) error {
|
||||
sess := db.GetEngine(ctx)
|
||||
issueIDs := util.ValuesOfMap(sortedIssueIDs)
|
||||
|
||||
count, err := sess.Table(new(ProjectIssue)).Where("project_id=?", column.ProjectID).In("issue_id", issueIDs).Count()
|
||||
// Build reverse map: issueID → sorting and validate no duplicate issue IDs
|
||||
sortingByIssue := make(map[int64]int64, len(sortedIssueIDs))
|
||||
for sorting, issueID := range sortedIssueIDs {
|
||||
sortingByIssue[issueID] = sorting
|
||||
}
|
||||
if len(sortingByIssue) != len(sortedIssueIDs) {
|
||||
return errors.New("duplicate issue IDs in reorder request")
|
||||
}
|
||||
|
||||
// Validate all issues exist and belong to this project
|
||||
count, err := sess.Table(new(ProjectIssue)).
|
||||
Where("project_id=?", column.ProjectID).
|
||||
In("issue_id", issueIDs).Count()
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if int(count) != len(sortedIssueIDs) {
|
||||
return errors.New("all issues have to be added to a project first")
|
||||
return errors.New("all issues must belong to the specified project")
|
||||
}
|
||||
|
||||
for sorting, issueID := range sortedIssueIDs {
|
||||
_, err = sess.Exec("UPDATE `project_issue` SET project_board_id=?, sorting=? WHERE issue_id=?", column.ID, sorting, issueID)
|
||||
// Sort issue IDs to ensure consistent lock ordering across concurrent transactions.
|
||||
// This prevents deadlocks when multiple transactions update overlapping rows.
|
||||
slices.Sort(issueIDs)
|
||||
|
||||
// Phase 1: Negate sorting for ALL cards currently in the target column
|
||||
// to free up all positive sorting positions. This prevents collisions
|
||||
// when moved cards are assigned their final positions.
|
||||
if _, err := sess.Exec("UPDATE `project_issue` SET sorting = -(sorting + 1) WHERE project_board_id=? AND sorting >= 0",
|
||||
column.ID); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Phase 2: Move the specified cards to the target column with their
|
||||
// final sorting values. Since all existing cards in the column now have
|
||||
// negative sorting, there are no collisions.
|
||||
for _, issueID := range issueIDs {
|
||||
_, err := sess.Exec("UPDATE `project_issue` SET project_board_id=?, sorting=? WHERE project_id=? AND issue_id=?",
|
||||
column.ID, sortingByIssue[issueID], column.ProjectID, issueID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
|
||||
// Phase 3: Re-pack any remaining cards in the column that still have
|
||||
// negative sorting (these are pre-existing cards NOT in the move set).
|
||||
// Assign them positions after the highest requested sorting value.
|
||||
var maxSorting int64
|
||||
for sorting := range sortedIssueIDs {
|
||||
if sorting > maxSorting {
|
||||
maxSorting = sorting
|
||||
}
|
||||
}
|
||||
|
||||
var remainingCards []ProjectIssue
|
||||
if err := sess.Where("project_board_id=? AND sorting < 0", column.ID).
|
||||
OrderBy("sorting DESC"). // original order was -(original+1), so DESC gives ascending original order
|
||||
Find(&remainingCards); err != nil {
|
||||
return err
|
||||
}
|
||||
nextSorting := maxSorting + 1
|
||||
for _, card := range remainingCards {
|
||||
if _, err := sess.Exec("UPDATE `project_issue` SET sorting=? WHERE id=?",
|
||||
nextSorting, card.ID); err != nil {
|
||||
return err
|
||||
}
|
||||
nextSorting++
|
||||
}
|
||||
|
||||
return nil
|
||||
})
|
||||
}
|
||||
|
|
|
|||
149
models/project/issue_test.go
Normal file
149
models/project/issue_test.go
Normal file
|
|
@ -0,0 +1,149 @@
|
|||
// 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)
|
||||
})
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue