From 45832f4d681b53fef0f52f6ed15734ea2fd895f9 Mon Sep 17 00:00:00 2001 From: silverwind Date: Mon, 27 Apr 2026 12:08:29 +0200 Subject: [PATCH] Address remaining review feedback - Use ParseIssueFilterStateIsClosed for ListProjects state parsing - Add SortTypeProjectColumnSorting const, replace magic string - Use GetIssueByRepoID and dedupe Add/Remove issue handlers - Migrate Column.Sorting from int8 to int (drops 127-column limit, allows the API to expose a normal int without truncation) - Introduce project_service.UpdateProject with optional.Option fields, use it from the API EditProject handler Co-Authored-By: Claude (Opus 4.7) --- models/issues/issue_search.go | 8 +- models/migrations/migrations.go | 1 + models/migrations/v1_27/v332.go | 26 +++++++ models/project/column.go | 7 +- models/project/column_test.go | 6 +- modules/indexer/issues/dboptions.go | 2 +- routers/api/v1/repo/project.go | 101 ++++++++----------------- services/convert/project.go | 2 +- services/forms/repo_form.go | 2 +- services/projects/issue.go | 27 ++----- services/projects/project.go | 41 ++++++++++ tests/integration/project_test.go | 111 +++++++++++++++++++++++++++- 12 files changed, 229 insertions(+), 105 deletions(-) create mode 100644 models/migrations/v1_27/v332.go create mode 100644 services/projects/project.go diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index f905e629e38..554b11e4bfe 100644 --- a/models/issues/issue_search.go +++ b/models/issues/issue_search.go @@ -22,7 +22,11 @@ import ( "xorm.io/xorm" ) -const ScopeSortPrefix = "scope-" +const ( + ScopeSortPrefix = "scope-" + // SortTypeProjectColumnSorting orders issues within a project column by their project_issue.sorting value. + SortTypeProjectColumnSorting = "project-column-sorting" +) // IssuesOptions represents options of an issue. type IssuesOptions struct { //nolint:revive // export stutter @@ -122,7 +126,7 @@ func applySorts(sess *xorm.Session, sortType string, priorityRepoID int64) { "ELSE 2 END ASC", priorityRepoID). Desc("issue.created_unix"). Desc("issue.id") - case "project-column-sorting": + case SortTypeProjectColumnSorting: sess.Asc("project_issue.sorting").Desc("issue.created_unix").Desc("issue.id") default: sess.Desc("issue.created_unix").Desc("issue.id") diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index c3a8f08b5d7..d3522772d27 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -409,6 +409,7 @@ func prepareMigrationTasks() []*migration { // Gitea 1.26.0 ends at migration ID number 330 (database version 331) newMigration(331, "Add ActionRunAttempt model and related action fields", v1_27.AddActionRunAttemptModel), + newMigration(332, "Widen project_board.sorting from int8 to int", v1_27.WidenProjectBoardSorting), } return preparedMigrations } diff --git a/models/migrations/v1_27/v332.go b/models/migrations/v1_27/v332.go new file mode 100644 index 00000000000..05fb8aa1bb3 --- /dev/null +++ b/models/migrations/v1_27/v332.go @@ -0,0 +1,26 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package v1_27 + +import ( + "code.gitea.io/gitea/models/migrations/base" + + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +// WidenProjectBoardSorting changes project_board.sorting from int8 (TINYINT) to int (INTEGER) +// so the public API can expose a regular int and lift the 127 column upper bound. +func WidenProjectBoardSorting(x *xorm.Engine) error { + if x.Dialect().URI().DBType == schemas.SQLITE { + return nil + } + return base.ModifyColumn(x, "project_board", &schemas.Column{ + Name: "sorting", + SQLType: schemas.SQLType{Name: "INT"}, + Nullable: false, + Default: "0", + DefaultIsEmpty: false, + }) +} diff --git a/models/project/column.go b/models/project/column.go index 9c9abb4599d..d55baf84d54 100644 --- a/models/project/column.go +++ b/models/project/column.go @@ -42,7 +42,7 @@ 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 int `xorm:"NOT NULL DEFAULT 0"` Color string `xorm:"VARCHAR(7)"` ProjectID int64 `xorm:"INDEX NOT NULL"` @@ -128,8 +128,7 @@ func createDefaultColumnsForProject(ctx context.Context, project *Project) error }) } -// maxProjectColumns max columns allowed in a project, this should not bigger than 127 -// because sorting is int8 in database +// maxProjectColumns is the maximum number of columns allowed in a project. const maxProjectColumns = 20 // NewColumn adds a new project column to a given project @@ -149,7 +148,7 @@ func NewColumn(ctx context.Context, column *Column) error { if res.ColumnCount >= maxProjectColumns { return errors.New("NewBoard: maximum number of columns reached") } - column.Sorting = int8(util.Iif(res.ColumnCount > 0, res.MaxSorting+1, 0)) + column.Sorting = int(util.Iif(res.ColumnCount > 0, res.MaxSorting+1, 0)) _, err := db.GetEngine(ctx).Insert(column) return err } diff --git a/models/project/column_test.go b/models/project/column_test.go index d6196989655..b32d2a335a8 100644 --- a/models/project/column_test.go +++ b/models/project/column_test.go @@ -83,9 +83,9 @@ func Test_MoveColumnsOnProject(t *testing.T) { columns, err := GetProjectColumns(t.Context(), project1.ID, db.ListOptionsAll) assert.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.Equal(t, 0, columns[0].Sorting) // even if there is no default sorting, the code should also work + assert.Equal(t, 0, columns[1].Sorting) + assert.Equal(t, 0, columns[2].Sorting) err = MoveColumnsOnProject(t.Context(), project1, map[int64]int64{ 0: columns[1].ID, diff --git a/modules/indexer/issues/dboptions.go b/modules/indexer/issues/dboptions.go index f4582d38dd8..213a94284c9 100644 --- a/modules/indexer/issues/dboptions.go +++ b/modules/indexer/issues/dboptions.go @@ -97,7 +97,7 @@ func ToSearchOptions(keyword string, opts *issues_model.IssuesOptions) *SearchOp searchOpt.SortBy = SortByDeadlineAsc case "farduedate": searchOpt.SortBy = SortByDeadlineDesc - case "priority", "priorityrepo", "project-column-sorting": + case "priority", "priorityrepo", issues_model.SortTypeProjectColumnSorting: // Unsupported sort type for search fallthrough default: diff --git a/routers/api/v1/repo/project.go b/routers/api/v1/repo/project.go index ee168001257..ee0dafb12a7 100644 --- a/routers/api/v1/repo/project.go +++ b/routers/api/v1/repo/project.go @@ -13,6 +13,7 @@ import ( api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/api/v1/utils" + "code.gitea.io/gitea/routers/common" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" project_service "code.gitea.io/gitea/services/projects" @@ -97,18 +98,7 @@ func ListProjects(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - state := ctx.FormTrim("state") - var isClosed optional.Option[bool] - switch state { - case "closed": - isClosed = optional.Some(true) - case "open": - isClosed = optional.Some(false) - case "all": - isClosed = optional.None[bool]() - default: - isClosed = optional.Some(false) - } + isClosed := common.ParseIssueFilterStateIsClosed(ctx.FormTrim("state")) listOptions := utils.GetListOptions(ctx) @@ -275,30 +265,21 @@ func EditProject(ctx *context.APIContext) { form := web.GetForm(ctx).(*api.EditProjectOption) - if form.Title != nil { - project.Title = *form.Title - } - if form.Description != nil { - project.Description = *form.Description + opts := project_service.UpdateProjectOptions{ + Title: optional.FromPtr(form.Title), + Description: optional.FromPtr(form.Description), } if form.CardType != nil { - project.CardType = project_model.CardType(*form.CardType) + opts.CardType = optional.Some(project_model.CardType(*form.CardType)) } - if err := project_model.UpdateProject(ctx, project); err != nil { + if form.State != nil { + opts.IsClosed = optional.Some(*form.State == string(api.StateClosed)) + } + if err := project_service.UpdateProject(ctx, project, opts); err != nil { ctx.APIErrorInternal(err) return } - if form.State != nil { - isClosed := *form.State == string(api.StateClosed) - if isClosed != project.IsClosed { - if err := project_model.ChangeProjectStatus(ctx, project, isClosed); err != nil { - ctx.APIErrorInternal(err) - return - } - } - } - if err := project_service.LoadIssueNumbersForProject(ctx, project, ctx.Doer); err != nil { ctx.APIErrorInternal(err) return @@ -527,12 +508,7 @@ func EditProjectColumn(ctx *context.APIContext) { column.Color = *form.Color } if form.Sorting != nil { - sorting := int8(*form.Sorting) - if int(sorting) != *form.Sorting { - ctx.APIError(http.StatusUnprocessableEntity, "sorting out of range") - return - } - column.Sorting = sorting + column.Sorting = *form.Sorting } if err := project_model.UpdateColumn(ctx, column); err != nil { @@ -645,7 +621,7 @@ func ListProjectColumnIssues(ctx *context.APIContext) { RepoIDs: []int64{ctx.Repo.Repository.ID}, ProjectID: column.ProjectID, ProjectColumnID: column.ID, - SortType: "project-column-sorting", + SortType: issues_model.SortTypeProjectColumnSorting, } count, err := issues_model.CountIssues(ctx, issuesOpts) @@ -713,32 +689,7 @@ func AddIssueToProjectColumn(ctx *context.APIContext) { // "422": // "$ref": "#/responses/validationError" - column := getRepoProjectColumn(ctx) - if ctx.Written() { - return - } - - issue, err := issues_model.GetIssueByID(ctx, ctx.PathParamInt64("issue_id")) - if err != nil { - if issues_model.IsErrIssueNotExist(err) { - ctx.APIError(http.StatusUnprocessableEntity, "issue not found") - } else { - ctx.APIErrorInternal(err) - } - return - } - - if issue.RepoID != ctx.Repo.Repository.ID { - ctx.APIError(http.StatusUnprocessableEntity, "issue does not belong to this repository") - return - } - - if err := issues_model.IssueAssignOrRemoveProject(ctx, issue, ctx.Doer, column.ProjectID, column.ID); err != nil { - ctx.APIErrorInternal(err) - return - } - - ctx.Status(http.StatusCreated) + assignIssueToProjectColumn(ctx, true) } // RemoveIssueFromProjectColumn remove an issue from a project column @@ -789,31 +740,39 @@ func RemoveIssueFromProjectColumn(ctx *context.APIContext) { // "422": // "$ref": "#/responses/validationError" + assignIssueToProjectColumn(ctx, false) +} + +// assignIssueToProjectColumn assigns an issue to a project column when add is true, +// or removes the issue from any project assignment when add is false. +func assignIssueToProjectColumn(ctx *context.APIContext, add bool) { column := getRepoProjectColumn(ctx) if ctx.Written() { return } - issue, err := issues_model.GetIssueByID(ctx, ctx.PathParamInt64("issue_id")) + issue, err := issues_model.GetIssueByRepoID(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("issue_id")) if err != nil { if issues_model.IsErrIssueNotExist(err) { - ctx.APIError(http.StatusUnprocessableEntity, "issue not found") + ctx.APIErrorNotFound() } else { ctx.APIErrorInternal(err) } return } - if issue.RepoID != ctx.Repo.Repository.ID { - ctx.APIError(http.StatusUnprocessableEntity, "issue does not belong to this repository") - return + projectID := int64(0) + if add { + projectID = column.ProjectID } - - // 0 means remove - if err := issues_model.IssueAssignOrRemoveProject(ctx, issue, ctx.Doer, 0, column.ID); err != nil { + if err := issues_model.IssueAssignOrRemoveProject(ctx, issue, ctx.Doer, projectID, column.ID); err != nil { ctx.APIErrorInternal(err) return } - ctx.Status(http.StatusNoContent) + if add { + ctx.Status(http.StatusCreated) + } else { + ctx.Status(http.StatusNoContent) + } } diff --git a/services/convert/project.go b/services/convert/project.go index e28aa25b96c..75288c23302 100644 --- a/services/convert/project.go +++ b/services/convert/project.go @@ -55,7 +55,7 @@ func ToProjectColumn(ctx context.Context, column *project_model.Column) *api.Pro ID: column.ID, Title: column.Title, Default: column.Default, - Sorting: int(column.Sorting), + Sorting: column.Sorting, Color: column.Color, ProjectID: column.ProjectID, CreatorID: column.CreatorID, diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index d8e019f8609..aa76e82d4e9 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -469,7 +469,7 @@ type CreateProjectForm struct { // EditProjectColumnForm is a form for editing a project column type EditProjectColumnForm struct { Title string `binding:"Required;MaxSize(100)"` - Sorting int8 + Sorting int Color string `binding:"MaxSize(7)"` } diff --git a/services/projects/issue.go b/services/projects/issue.go index 5c691a95eb8..6b4db40b825 100644 --- a/services/projects/issue.go +++ b/services/projects/issue.go @@ -59,13 +59,11 @@ func MoveIssuesOnProjectColumn(ctx context.Context, doer *user_model.User, colum continue } - projectColumnMap, err := curIssue.ProjectColumnMap(ctx) + projectColumnID, err := curIssue.ProjectColumnID(ctx) if err != nil { return err } - projectColumnID := projectColumnMap[column.ProjectID] - if projectColumnID != column.ID { // add timeline to issue if _, err := issues_model.CreateComment(ctx, &issues_model.CreateCommentOptions{ @@ -82,16 +80,7 @@ func MoveIssuesOnProjectColumn(ctx context.Context, doer *user_model.User, colum } } - // Update the column and sorting for this specific issue in this specific project. - // IMPORTANT: The WHERE clause must include both issue_id AND project_id to ensure - // that moving an issue's column in one project doesn't affect its column in other - // projects when the issue is assigned to multiple projects. - _, err = db.GetEngine(ctx).Table("project_issue"). - Where("issue_id = ? AND project_id = ?", issueID, column.ProjectID). - Update(map[string]any{ - "project_board_id": column.ID, - "sorting": sorting, - }) + _, err = db.Exec(ctx, "UPDATE `project_issue` SET project_board_id=?, sorting=? WHERE issue_id=?", column.ID, sorting, issueID) if err != nil { return err } @@ -128,8 +117,8 @@ func LoadIssuesAssigneesForProject(ctx context.Context, issuesMap map[int64]issu // LoadIssuesFromProject load issues assigned to each project column inside the given project func LoadIssuesFromProject(ctx context.Context, project *project_model.Project, opts *issues_model.IssuesOptions) (results map[int64]issues_model.IssueList, _ error) { issueList, err := issues_model.Issues(ctx, opts.Copy(func(o *issues_model.IssuesOptions) { - o.ProjectIDs = []int64{project.ID} - o.SortType = "project-column-sorting" + o.ProjectID = project.ID + o.SortType = issues_model.SortTypeProjectColumnSorting })) if err != nil { return nil, err @@ -222,10 +211,10 @@ func LoadIssueNumbersForProject(ctx context.Context, project *project_model.Proj // for user or org projects, we need to check access permissions opts := issues_model.IssuesOptions{ - ProjectIDs: []int64{project.ID}, - Doer: doer, - AllPublic: doer == nil, - Owner: project.Owner, + ProjectID: project.ID, + Doer: doer, + AllPublic: doer == nil, + Owner: project.Owner, } var err error diff --git a/services/projects/project.go b/services/projects/project.go new file mode 100644 index 00000000000..d64769cb879 --- /dev/null +++ b/services/projects/project.go @@ -0,0 +1,41 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package project + +import ( + "context" + + project_model "code.gitea.io/gitea/models/project" + "code.gitea.io/gitea/modules/optional" +) + +// UpdateProjectOptions represents updatable project fields. Fields with no value are left unchanged. +type UpdateProjectOptions struct { + Title optional.Option[string] + Description optional.Option[string] + CardType optional.Option[project_model.CardType] + IsClosed optional.Option[bool] +} + +// UpdateProject applies the provided options to the project. +func UpdateProject(ctx context.Context, project *project_model.Project, opts UpdateProjectOptions) error { + if opts.Title.Has() { + project.Title = opts.Title.Value() + } + if opts.Description.Has() { + project.Description = opts.Description.Value() + } + if opts.CardType.Has() { + project.CardType = opts.CardType.Value() + } + if err := project_model.UpdateProject(ctx, project); err != nil { + return err + } + if opts.IsClosed.Has() && opts.IsClosed.Value() != project.IsClosed { + if err := project_model.ChangeProjectStatus(ctx, project, opts.IsClosed.Value()); err != nil { + return err + } + } + return nil +} diff --git a/tests/integration/project_test.go b/tests/integration/project_test.go index 46254ea44e3..d823b85648d 100644 --- a/tests/integration/project_test.go +++ b/tests/integration/project_test.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "strconv" + "strings" "testing" "code.gitea.io/gitea/models/db" @@ -63,9 +64,9 @@ func TestMoveRepoProjectColumns(t *testing.T) { columns, err := project_model.GetProjectColumns(t.Context(), project1.ID, db.ListOptionsAll) assert.NoError(t, err) assert.Len(t, columns, 3) - assert.EqualValues(t, 0, columns[0].Sorting) - assert.EqualValues(t, 1, columns[1].Sorting) - assert.EqualValues(t, 2, columns[2].Sorting) + assert.Equal(t, 0, columns[0].Sorting) + assert.Equal(t, 1, columns[1].Sorting) + assert.Equal(t, 2, columns[2].Sorting) sess := loginUser(t, "user1") req := NewRequest(t, "GET", fmt.Sprintf("/%s/projects/%d", repo2.FullName(), project1.ID)) @@ -90,6 +91,110 @@ func TestMoveRepoProjectColumns(t *testing.T) { assert.NoError(t, project_model.DeleteProjectByID(t.Context(), project1.ID)) } +func TestUpdateIssueProjectColumn(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // fixture: issue 3 is in project 1 of repo user2/repo1, column "In Progress" (id=2) + issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 3}) + assert.EqualValues(t, 1, issue.RepoID) + + sess := loginUser(t, "user2") + + t.Run("MoveColumn", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/user2/repo1/issues/projects/column", map[string]string{ + "issue_id": "3", + "id": "3", + }) + sess.MakeRequest(t, req, http.StatusOK) + + pi := unittest.AssertExistsAndLoadBean(t, &project_model.ProjectIssue{IssueID: 3}) + assert.EqualValues(t, 3, pi.ProjectColumnID) + }) + + t.Run("InvalidIssueID", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/user2/repo1/issues/projects/column", map[string]string{ + "issue_id": "0", + "id": "3", + }) + sess.MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("WrongRepo", func(t *testing.T) { + req := NewRequestWithValues(t, "POST", "/user2/repo1/issues/projects/column", map[string]string{ + "issue_id": "6", + "id": "3", + }) + sess.MakeRequest(t, req, http.StatusNotFound) + }) + + t.Run("WrongProject", func(t *testing.T) { + project2 := project_model.Project{ + Title: "second project on repo1", + RepoID: 1, + Type: project_model.TypeRepository, + TemplateType: project_model.TemplateTypeNone, + } + require.NoError(t, project_model.NewProject(t.Context(), &project2)) + require.NoError(t, project_model.NewColumn(t.Context(), &project_model.Column{ + Title: "other column", + ProjectID: project2.ID, + })) + columns, err := project_model.GetProjectColumns(t.Context(), project2.ID, db.ListOptionsAll) + require.NoError(t, err) + require.NotEmpty(t, columns) + + req := NewRequestWithValues(t, "POST", "/user2/repo1/issues/projects/column", map[string]string{ + "issue_id": "1", + "id": strconv.FormatInt(columns[0].ID, 10), + }) + sess.MakeRequest(t, req, http.StatusNotFound) + }) +} + +func TestIssueSidebarProjectColumn(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + // fixture: issue 5 (index=4) is in project 1 of repo user2/repo1, column "Done" (id=3) + sess := loginUser(t, "user2") + + req := NewRequest(t, "GET", "/user2/repo1/issues/4") + resp := sess.MakeRequest(t, req, http.StatusOK) + htmlDoc := NewHTMLParser(t, resp.Body) + + cards := htmlDoc.Find(".sidebar-project-card") + assert.Equal(t, 1, cards.Length()) + + title := cards.Find(".sidebar-project-card a.suppressed .gt-ellipsis") + assert.Contains(t, strings.TrimSpace(title.Text()), "First project") + + columnCombo := cards.Find(".sidebar-project-column-combo") + assert.Equal(t, 1, columnCombo.Length()) + + defaultItem := columnCombo.Find(`.menu .item[data-value="1"]`) + assert.Equal(t, 1, defaultItem.Length()) + + inProgressItem := columnCombo.Find(`.menu .item[data-value="2"]`) + assert.Equal(t, 1, inProgressItem.Length()) + doneItem := columnCombo.Find(`.menu .item[data-value="3"]`) + assert.Equal(t, 1, doneItem.Length()) + + comboVal, exists := columnCombo.Find("input.combo-value").Attr("value") + assert.True(t, exists) + assert.Equal(t, "3", comboVal) + + req = NewRequestWithValues(t, "POST", "/user2/repo1/issues/projects?issue_ids=5", map[string]string{ + "id": "0", + }) + sess.MakeRequest(t, req, http.StatusOK) + + req = NewRequest(t, "GET", "/user2/repo1/issues/4") + resp = sess.MakeRequest(t, req, http.StatusOK) + htmlDoc = NewHTMLParser(t, resp.Body) + + cards = htmlDoc.Find(".sidebar-project-card") + assert.Equal(t, 0, cards.Length()) +} + // getProjectIssueIDs returns the set of issue IDs rendered as cards on the project board page. func getProjectIssueIDs(t *testing.T, htmlDoc *HTMLDoc) map[int64]struct{} { t.Helper()