From 62d47e025280cefdef75027d4d4c0a3eac4631e4 Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Sun, 12 Jan 2025 23:37:47 -0800 Subject: [PATCH 01/15] Validate that the tag doesn't exist when creating a tag via the web --- routers/web/repo/release.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 53655703fcb9f..585c0417416d8 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -489,7 +489,7 @@ func NewReleasePost(ctx *context.Context) { return } } else { - if !rel.IsTag { + if len(form.TagOnly) > 0 && rel.IsTag { ctx.Data["Err_TagName"] = true ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) return From 69984e501cacf258c273c16df77e96e5420f4bbd Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Mon, 13 Jan 2025 00:21:31 -0800 Subject: [PATCH 02/15] PR feedback --- routers/web/repo/release.go | 2 +- routers/web/repo/release_test.go | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 585c0417416d8..21d8726f761c3 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -489,7 +489,7 @@ func NewReleasePost(ctx *context.Context) { return } } else { - if len(form.TagOnly) > 0 && rel.IsTag { + if form.TagOnly != "" && rel.IsTag { ctx.Data["Err_TagName"] = true ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) return diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index 7ebea4c3fbe30..9f79c6f501b7e 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -22,12 +22,13 @@ func TestNewReleasePost(t *testing.T) { RepoID int64 UserID int64 TagName string + IsTag bool Form forms.NewReleaseForm }{ - { + { // pre-existing tag RepoID: 1, UserID: 2, - TagName: "v1.1", // pre-existing tag + TagName: "v1.1", Form: forms.NewReleaseForm{ TagName: "newtag", Target: "master", @@ -35,6 +36,18 @@ func TestNewReleasePost(t *testing.T) { Content: "content", }, }, + { // creating a new tag when there's already a pre-existing tag + RepoID: 1, + UserID: 2, + TagName: "delete-tag", + IsTag: true, + Form: forms.NewReleaseForm{ + TagName: "delete-tag", + Target: "master", + Title: "delete-tag", + TagOnly: "1", + }, + }, { RepoID: 1, UserID: 2, @@ -62,6 +75,7 @@ func TestNewReleasePost(t *testing.T) { Target: testCase.Form.Target, Title: testCase.Form.Title, Note: testCase.Form.Content, + IsTag: true, }, unittest.Cond("is_draft=?", len(testCase.Form.Draft) > 0)) ctx.Repo.GitRepo.Close() } From a3a6493cac83a92f696e912c996d29bc3eac28cc Mon Sep 17 00:00:00 2001 From: Kemal Zebari Date: Mon, 13 Jan 2025 00:24:29 -0800 Subject: [PATCH 03/15] Fix typo in test --- routers/web/repo/release_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index 9f79c6f501b7e..97dec10c809cd 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -75,7 +75,7 @@ func TestNewReleasePost(t *testing.T) { Target: testCase.Form.Target, Title: testCase.Form.Title, Note: testCase.Form.Content, - IsTag: true, + IsTag: testCase.IsTag, }, unittest.Cond("is_draft=?", len(testCase.Form.Draft) > 0)) ctx.Repo.GitRepo.Close() } From 7a5595cbfa55a84450a86f01602f0dde60ff3e3a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 13 Jan 2025 16:33:32 +0800 Subject: [PATCH 04/15] add comments for the "string" type --- services/forms/repo_form.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index b14171787e466..0bb6b49a5b735 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -647,12 +647,16 @@ type UpdateAllowEditsForm struct { // NewReleaseForm form for creating release type NewReleaseForm struct { - TagName string `binding:"Required;GitRefName;MaxSize(255)"` - Target string `form:"tag_target" binding:"Required;MaxSize(255)"` - Title string `binding:"MaxSize(255)"` - Content string - Draft string - TagOnly string + TagName string `binding:"Required;GitRefName;MaxSize(255)"` + Target string `form:"tag_target" binding:"Required;MaxSize(255)"` + Title string `binding:"MaxSize(255)"` + Content string + Draft string + + // TODO: ideally it should be a bool. The "string" type here was used to accept the value of ``. + // As now, the tmpl code had been refactor to ``, so the type could be safely refactored to "bool" in the future. + TagOnly string + Prerelease bool AddTagMsg bool Files []string From 5cf894706b13b95dbe2805ca3a6393ace7e9fb3b Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 13 Jan 2025 22:53:56 +0800 Subject: [PATCH 05/15] fix legacy bugs --- routers/web/repo/release.go | 136 ++++++++++++++----------------- routers/web/repo/release_test.go | 2 +- services/forms/repo_form.go | 16 ++-- templates/repo/release/new.tmpl | 18 ++-- 4 files changed, 72 insertions(+), 100 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 21d8726f761c3..f603c8e1bbca4 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -17,7 +17,6 @@ import ( "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/git" - "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/markup/markdown" "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" @@ -408,58 +407,56 @@ func NewReleasePost(ctx *context.Context) { } // Title of release cannot be empty - if len(form.TagOnly) == 0 && len(form.Title) == 0 { + if !form.TagOnly && form.Title == "" { ctx.RenderWithErr(ctx.Tr("repo.release.title_empty"), tplReleaseNew, &form) return } - var attachmentUUIDs []string - if setting.Attachment.Enabled { - attachmentUUIDs = form.Files + handleTagReleaseError := func(err error) { + ctx.Data["Err_TagName"] = true + switch { + case release_service.IsErrTagAlreadyExists(err): + ctx.RenderWithErr(ctx.Tr("repo.branch.tag_collision", form.TagName), tplReleaseNew, &form) + case repo_model.IsErrReleaseAlreadyExist(err): + ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) + case release_service.IsErrInvalidTagName(err): + ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_invalid"), tplReleaseNew, &form) + case release_service.IsErrProtectedTagName(err): + ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_protected"), tplReleaseNew, &form) + default: + ctx.ServerError("handleTagReleaseError", err) + } } rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName) - if err != nil { - if !repo_model.IsErrReleaseNotExist(err) { - ctx.ServerError("GetRelease", err) - return - } - - msg := "" - if len(form.Title) > 0 && form.AddTagMsg { - msg = form.Title + "\n\n" + form.Content - } - - if len(form.TagOnly) > 0 { - if err = release_service.CreateNewTag(ctx, ctx.Doer, ctx.Repo.Repository, form.Target, form.TagName, msg); err != nil { - if release_service.IsErrTagAlreadyExists(err) { - e := err.(release_service.ErrTagAlreadyExists) - ctx.Flash.Error(ctx.Tr("repo.branch.tag_collision", e.TagName)) - ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.RefTypeNameSubURL()) - return - } - - if release_service.IsErrInvalidTagName(err) { - ctx.Flash.Error(ctx.Tr("repo.release.tag_name_invalid")) - ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.RefTypeNameSubURL()) - return - } - - if release_service.IsErrProtectedTagName(err) { - ctx.Flash.Error(ctx.Tr("repo.release.tag_name_protected")) - ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.RefTypeNameSubURL()) - return - } + if err != nil && !repo_model.IsErrReleaseNotExist(err) { + ctx.ServerError("GetRelease", err) + return + } - ctx.ServerError("release_service.CreateNewTag", err) - return - } + newTagMsg := "" + if form.Title != "" && form.AddTagMsg { + newTagMsg = form.Title + "\n\n" + form.Content + } - ctx.Flash.Success(ctx.Tr("repo.tag.create_success", form.TagName)) - ctx.Redirect(ctx.Repo.RepoLink + "/src/tag/" + util.PathEscapeSegments(form.TagName)) + // no release, and tag only + if rel == nil && form.TagOnly { + if err = release_service.CreateNewTag(ctx, ctx.Doer, ctx.Repo.Repository, form.Target, form.TagName, newTagMsg); err != nil { + handleTagReleaseError(err) return } + ctx.Flash.Success(ctx.Tr("repo.tag.create_success", form.TagName)) + ctx.Redirect(ctx.Repo.RepoLink + "/src/tag/" + util.PathEscapeSegments(form.TagName)) + return + } + + var attachmentUUIDs []string + if setting.Attachment.Enabled { + attachmentUUIDs = form.Files + } + // no release, create a new release + if rel == nil { rel = &repo_model.Release{ RepoID: ctx.Repo.Repository.ID, Repo: ctx.Repo.Repository, @@ -469,48 +466,35 @@ func NewReleasePost(ctx *context.Context) { TagName: form.TagName, Target: form.Target, Note: form.Content, - IsDraft: len(form.Draft) > 0, + IsDraft: form.Draft, IsPrerelease: form.Prerelease, IsTag: false, } - - if err = release_service.CreateRelease(ctx.Repo.GitRepo, rel, attachmentUUIDs, msg); err != nil { - ctx.Data["Err_TagName"] = true - switch { - case repo_model.IsErrReleaseAlreadyExist(err): - ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) - case release_service.IsErrInvalidTagName(err): - ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_invalid"), tplReleaseNew, &form) - case release_service.IsErrProtectedTagName(err): - ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_protected"), tplReleaseNew, &form) - default: - ctx.ServerError("CreateRelease", err) - } - return - } - } else { - if form.TagOnly != "" && rel.IsTag { - ctx.Data["Err_TagName"] = true - ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) - return - } - - rel.Title = form.Title - rel.Note = form.Content - rel.Target = form.Target - rel.IsDraft = len(form.Draft) > 0 - rel.IsPrerelease = form.Prerelease - rel.PublisherID = ctx.Doer.ID - rel.IsTag = false - - if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, attachmentUUIDs, nil, nil); err != nil { - ctx.Data["Err_TagName"] = true - ctx.ServerError("UpdateRelease", err) + if err = release_service.CreateRelease(ctx.Repo.GitRepo, rel, attachmentUUIDs, newTagMsg); err != nil { + handleTagReleaseError(err) return } + ctx.Redirect(ctx.Repo.RepoLink + "/releases") + return } - log.Trace("Release created: %s/%s:%s", ctx.Doer.LowerName, ctx.Repo.Repository.Name, form.TagName) + // release exists, try to update it (it can't do tag-only if the release is just a tag) + if form.TagOnly && rel.IsTag { + ctx.Data["Err_TagName"] = true + ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) + return + } + rel.Title = form.Title + rel.Note = form.Content + rel.Target = form.Target + rel.IsDraft = form.Draft + rel.IsPrerelease = form.Prerelease + rel.PublisherID = ctx.Doer.ID + rel.IsTag = false + if err = release_service.UpdateRelease(ctx, ctx.Doer, ctx.Repo.GitRepo, rel, attachmentUUIDs, nil, nil); err != nil { + handleTagReleaseError(err) + return + } ctx.Redirect(ctx.Repo.RepoLink + "/releases") } diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index 97dec10c809cd..ebad151642cd5 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -45,7 +45,7 @@ func TestNewReleasePost(t *testing.T) { TagName: "delete-tag", Target: "master", Title: "delete-tag", - TagOnly: "1", + TagOnly: true, }, }, { diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go index 0bb6b49a5b735..35ea5378d3a67 100644 --- a/services/forms/repo_form.go +++ b/services/forms/repo_form.go @@ -647,16 +647,12 @@ type UpdateAllowEditsForm struct { // NewReleaseForm form for creating release type NewReleaseForm struct { - TagName string `binding:"Required;GitRefName;MaxSize(255)"` - Target string `form:"tag_target" binding:"Required;MaxSize(255)"` - Title string `binding:"MaxSize(255)"` - Content string - Draft string - - // TODO: ideally it should be a bool. The "string" type here was used to accept the value of ``. - // As now, the tmpl code had been refactor to ``, so the type could be safely refactored to "bool" in the future. - TagOnly string - + TagName string `binding:"Required;GitRefName;MaxSize(255)"` + Target string `form:"tag_target" binding:"Required;MaxSize(255)"` + Title string `binding:"MaxSize(255)"` + Content string + Draft bool + TagOnly bool Prerelease bool AddTagMsg bool Files []string diff --git a/templates/repo/release/new.tmpl b/templates/repo/release/new.tmpl index 574b0d0311e05..23e9ba29ebad2 100644 --- a/templates/repo/release/new.tmpl +++ b/templates/repo/release/new.tmpl @@ -109,23 +109,15 @@ {{ctx.Locale.Tr "repo.release.delete_release"}} {{if .IsDraft}} - - + + {{else}} - + {{end}} {{else}} - {{if not .tag_name}} - - {{end}} + - + {{end}} From 7d77978e35f95646c454099aedce037fea666d22 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 13 Jan 2025 23:04:12 +0800 Subject: [PATCH 06/15] fix legacy bugs --- routers/web/repo/release.go | 84 +++++++++++++++++---------------- templates/repo/release/new.tmpl | 4 +- 2 files changed, 46 insertions(+), 42 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index f603c8e1bbca4..40acbaf14a81d 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -329,12 +329,40 @@ func LatestRelease(ctx *context.Context) { ctx.Redirect(release.Link()) } -// NewRelease render creating or edit release page -func NewRelease(ctx *context.Context) { +func newReleaseCommon(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.release.new_release") ctx.Data["PageIsReleaseList"] = true + + tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID) + if err != nil { + ctx.ServerError("GetTagNamesByRepoID", err) + return + } + ctx.Data["Tags"] = tags + + ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled + assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository) + if err != nil { + ctx.ServerError("GetRepoAssignees", err) + return + } + ctx.Data["Assignees"] = shared_user.MakeSelfOnTop(ctx.Doer, assigneeUsers) + + upload.AddUploadContext(ctx, "release") + + PrepareBranchList(ctx) // for New Release page +} + +// NewRelease render creating or edit release page +func NewRelease(ctx *context.Context) { + newReleaseCommon(ctx) + if ctx.Written() { + return + } + + // pre-fill the form with the tag name, target branch and the existing release (if exists) ctx.Data["tag_target"] = ctx.Repo.Repository.DefaultBranch - if tagName := ctx.FormString("tag"); len(tagName) > 0 { + if tagName := ctx.FormString("tag"); tagName != "" { rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, tagName) if err != nil && !repo_model.IsErrReleaseNotExist(err) { ctx.ServerError("GetRelease", err) @@ -343,58 +371,38 @@ func NewRelease(ctx *context.Context) { if rel != nil { rel.Repo = ctx.Repo.Repository - if err := rel.LoadAttributes(ctx); err != nil { + if err = rel.LoadAttributes(ctx); err != nil { ctx.ServerError("LoadAttributes", err) return } + ctx.Data["TagNameReleaseExists"] = true ctx.Data["tag_name"] = rel.TagName - if rel.Target != "" { - ctx.Data["tag_target"] = rel.Target - } + ctx.Data["tag_target"] = rel.Target ctx.Data["title"] = rel.Title ctx.Data["content"] = rel.Note ctx.Data["attachments"] = rel.Attachments } } - ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled - assigneeUsers, err := repo_model.GetRepoAssignees(ctx, ctx.Repo.Repository) - if err != nil { - ctx.ServerError("GetRepoAssignees", err) - return - } - ctx.Data["Assignees"] = shared_user.MakeSelfOnTop(ctx.Doer, assigneeUsers) - - upload.AddUploadContext(ctx, "release") - - // For New Release page - PrepareBranchList(ctx) - if ctx.Written() { - return - } - - tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID) - if err != nil { - ctx.ServerError("GetTagNamesByRepoID", err) - return - } - ctx.Data["Tags"] = tags ctx.HTML(http.StatusOK, tplReleaseNew) } // NewReleasePost response for creating a release func NewReleasePost(ctx *context.Context) { + newReleaseCommon(ctx) + if ctx.Written() { + return + } + form := web.GetForm(ctx).(*forms.NewReleaseForm) - ctx.Data["Title"] = ctx.Tr("repo.release.new_release") - ctx.Data["PageIsReleaseList"] = true - tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID) - if err != nil { - ctx.ServerError("GetTagNamesByRepoID", err) + rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName) + if err != nil && !repo_model.IsErrReleaseNotExist(err) { + ctx.ServerError("GetRelease", err) return } - ctx.Data["Tags"] = tags + ctx.Data["TagNameReleaseExists"] = rel != nil if ctx.HasError() { ctx.HTML(http.StatusOK, tplReleaseNew) @@ -428,12 +436,6 @@ func NewReleasePost(ctx *context.Context) { } } - rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName) - if err != nil && !repo_model.IsErrReleaseNotExist(err) { - ctx.ServerError("GetRelease", err) - return - } - newTagMsg := "" if form.Title != "" && form.AddTagMsg { newTagMsg = form.Title + "\n\n" + form.Content diff --git a/templates/repo/release/new.tmpl b/templates/repo/release/new.tmpl index 23e9ba29ebad2..7bba17e0aa2c3 100644 --- a/templates/repo/release/new.tmpl +++ b/templates/repo/release/new.tmpl @@ -115,7 +115,9 @@ {{end}} {{else}} - + {{if not .TagNameReleaseExists}} + + {{end}} {{end}} From d190fb660c67ca0c6e4c98772441bc852dd93386 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 13 Jan 2025 23:13:53 +0800 Subject: [PATCH 07/15] add comments --- routers/web/repo/release.go | 13 +++++++------ routers/web/repo/release_test.go | 2 +- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 40acbaf14a81d..ae612db2a3f29 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -397,6 +397,8 @@ func NewReleasePost(ctx *context.Context) { form := web.GetForm(ctx).(*forms.NewReleaseForm) + // first, check whether the release exists, + // it should be done before the form error check, because the tmpl needs "TagNameReleaseExists" to show/hide the "tag only" button rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName) if err != nil && !repo_model.IsErrReleaseNotExist(err) { ctx.ServerError("GetRelease", err) @@ -404,6 +406,7 @@ func NewReleasePost(ctx *context.Context) { } ctx.Data["TagNameReleaseExists"] = rel != nil + // do some form checks if ctx.HasError() { ctx.HTML(http.StatusOK, tplReleaseNew) return @@ -414,8 +417,8 @@ func NewReleasePost(ctx *context.Context) { return } - // Title of release cannot be empty if !form.TagOnly && form.Title == "" { + // if not "tag only", then the title of the release cannot be empty ctx.RenderWithErr(ctx.Tr("repo.release.title_empty"), tplReleaseNew, &form) return } @@ -436,6 +439,7 @@ func NewReleasePost(ctx *context.Context) { } } + // prepare the git message for creating a new tag newTagMsg := "" if form.Title != "" && form.AddTagMsg { newTagMsg = form.Title + "\n\n" + form.Content @@ -452,12 +456,9 @@ func NewReleasePost(ctx *context.Context) { return } - var attachmentUUIDs []string - if setting.Attachment.Enabled { - attachmentUUIDs = form.Files - } + attachmentUUIDs := util.Iif(setting.Attachment.Enabled, form.Files, nil) - // no release, create a new release + // no existing release, create a new release if rel == nil { rel = &repo_model.Release{ RepoID: ctx.Repo.Repository.ID, diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index ebad151642cd5..54b85aa16ad8b 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -76,7 +76,7 @@ func TestNewReleasePost(t *testing.T) { Title: testCase.Form.Title, Note: testCase.Form.Content, IsTag: testCase.IsTag, - }, unittest.Cond("is_draft=?", len(testCase.Form.Draft) > 0)) + }, unittest.Cond("is_draft=?", testCase.Form.Draft)) ctx.Repo.GitRepo.Close() } } From 982cb1fb6f1e5de224f1c2876488c9b54598b34a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 13 Jan 2025 23:23:05 +0800 Subject: [PATCH 08/15] fix test --- routers/web/repo/release_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index 54b85aa16ad8b..28e98fc859bcd 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -75,9 +75,8 @@ func TestNewReleasePost(t *testing.T) { Target: testCase.Form.Target, Title: testCase.Form.Title, Note: testCase.Form.Content, - IsTag: testCase.IsTag, - }, unittest.Cond("is_draft=?", testCase.Form.Draft)) - ctx.Repo.GitRepo.Close() + }, unittest.Cond("is_tag=? AND is_draft=?", testCase.IsTag, testCase.Form.Draft)) + _ = ctx.Repo.GitRepo.Close() } } From 3ca836c42127dbfb590b018777bc510ef45ef2cd Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 13 Jan 2025 23:53:08 +0800 Subject: [PATCH 09/15] rewrite tests --- routers/web/repo/release.go | 2 +- routers/web/repo/release_test.go | 137 +++++++++++++++++++------------ 2 files changed, 85 insertions(+), 54 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index ae612db2a3f29..942d45b3d8f6b 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -482,7 +482,7 @@ func NewReleasePost(ctx *context.Context) { } // release exists, try to update it (it can't do tag-only if the release is just a tag) - if form.TagOnly && rel.IsTag { + if form.TagOnly { ctx.Data["Err_TagName"] = true ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) return diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index 28e98fc859bcd..7f5866a3098eb 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -11,73 +11,104 @@ import ( "code.gitea.io/gitea/models/unit" "code.gitea.io/gitea/models/unittest" "code.gitea.io/gitea/modules/web" + "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/contexttest" "code.gitea.io/gitea/services/forms" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewReleasePost(t *testing.T) { - for _, testCase := range []struct { - RepoID int64 - UserID int64 - TagName string - IsTag bool - Form forms.NewReleaseForm - }{ - { // pre-existing tag - RepoID: 1, - UserID: 2, - TagName: "v1.1", - Form: forms.NewReleaseForm{ - TagName: "newtag", - Target: "master", - Title: "title", - Content: "content", - }, - }, - { // creating a new tag when there's already a pre-existing tag - RepoID: 1, - UserID: 2, - TagName: "delete-tag", - IsTag: true, - Form: forms.NewReleaseForm{ - TagName: "delete-tag", - Target: "master", - Title: "delete-tag", - TagOnly: true, - }, - }, - { - RepoID: 1, - UserID: 2, - TagName: "newtag", - Form: forms.NewReleaseForm{ - TagName: "newtag", - Target: "master", - Title: "title", - Content: "content", - }, - }, - } { - unittest.PrepareTestEnv(t) + unittest.PrepareTestEnv(t) + + get := func(t *testing.T, tagName string) *context.Context { + ctx, _ := contexttest.MockContext(t, "user2/repo1/releases/new?tag="+tagName) + contexttest.LoadUser(t, ctx, 2) + contexttest.LoadRepo(t, ctx, 1) + contexttest.LoadGitRepo(t, ctx) + defer ctx.Repo.GitRepo.Close() + NewRelease(ctx) + return ctx + } + + t.Run("NewReleasePage", func(t *testing.T) { + ctx := get(t, "v1.1") + assert.NotEmpty(t, ctx.Data["TagNameReleaseExists"]) + ctx = get(t, "new-tag-name") + assert.Empty(t, ctx.Data["TagNameReleaseExists"]) + }) + post := func(t *testing.T, form forms.NewReleaseForm) *context.Context { ctx, _ := contexttest.MockContext(t, "user2/repo1/releases/new") contexttest.LoadUser(t, ctx, 2) contexttest.LoadRepo(t, ctx, 1) contexttest.LoadGitRepo(t, ctx) - web.SetForm(ctx, &testCase.Form) + defer ctx.Repo.GitRepo.Close() + web.SetForm(ctx, &form) NewReleasePost(ctx) - unittest.AssertExistsAndLoadBean(t, &repo_model.Release{ - RepoID: 1, - PublisherID: 2, - TagName: testCase.Form.TagName, - Target: testCase.Form.Target, - Title: testCase.Form.Title, - Note: testCase.Form.Content, - }, unittest.Cond("is_tag=? AND is_draft=?", testCase.IsTag, testCase.Form.Draft)) - _ = ctx.Repo.GitRepo.Close() + return ctx } + + loadRelease := func(t *testing.T, tagName string) *repo_model.Release { + return unittest.GetBean(t, &repo_model.Release{}, unittest.Cond("repo_id=1 AND tag_name=?", tagName)) + } + + t.Run("NewTagRelease", func(t *testing.T) { + post(t, forms.NewReleaseForm{ + TagName: "newtag", + Target: "master", + Title: "title", + Content: "content", + }) + rel := loadRelease(t, "newtag") + require.NotNil(t, rel) + assert.False(t, rel.IsTag) + assert.Equal(t, "master", rel.Target) + assert.Equal(t, "title", rel.Title) + assert.Equal(t, "content", rel.Note) + }) + + t.Run("ReleaseExistsDoUpdate", func(t *testing.T) { + post(t, forms.NewReleaseForm{ + TagName: "v1.1", + Target: "master", + Title: "updated-title", + Content: "updated-content", + }) + rel := loadRelease(t, "v1.1") + require.NotNil(t, rel) + assert.Equal(t, "updated-title", rel.Title) + assert.Equal(t, "updated-content", rel.Note) + }) + + t.Run("TagOnly", func(t *testing.T) { + ctx := post(t, forms.NewReleaseForm{ + TagName: "new-tag-only", + Target: "master", + Title: "title", + Content: "content", + TagOnly: true, + }) + rel := loadRelease(t, "new-tag-only") + require.NotNil(t, rel) + assert.True(t, rel.IsTag) + assert.Empty(t, ctx.Flash.ErrorMsg) + }) + + t.Run("TagOnlyConflict", func(t *testing.T) { + ctx := post(t, forms.NewReleaseForm{ + TagName: "v1.1", + Target: "master", + Title: "title", + Content: "content", + TagOnly: true, + }) + rel := loadRelease(t, "v1.1") + require.NotNil(t, rel) + assert.False(t, rel.IsTag) + assert.NotEmpty(t, ctx.Flash.ErrorMsg) + }) } func TestCalReleaseNumCommitsBehind(t *testing.T) { From 1f8952f71b58983e5601c800f8245e2dbab3c8bd Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 13 Jan 2025 23:55:10 +0800 Subject: [PATCH 10/15] fix test --- tests/integration/release_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/release_test.go b/tests/integration/release_test.go index 40bd798d16159..d3c4ed6a83eda 100644 --- a/tests/integration/release_test.go +++ b/tests/integration/release_test.go @@ -39,7 +39,7 @@ func createNewRelease(t *testing.T, session *TestSession, repoURL, tag, title st postData["prerelease"] = "on" } if draft { - postData["draft"] = "Save Draft" + postData["draft"] = "1" } req = NewRequestWithValues(t, "POST", link, postData) From f68bee55b23ce4642d87a60aede40d87f27b2e27 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Jan 2025 00:21:35 +0800 Subject: [PATCH 11/15] add more tests --- routers/web/repo/release.go | 8 +++++-- routers/web/repo/release_test.go | 36 +++++++++++++++++++++++++++++--- 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 942d45b3d8f6b..ee33f8dfdf366 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -481,12 +481,16 @@ func NewReleasePost(ctx *context.Context) { return } - // release exists, try to update it (it can't do tag-only if the release is just a tag) - if form.TagOnly { + // tag exists, try to convert it to a real release + // old logic: if the release is not a tag (it is a real release), do not update it on the "new release" page + // add new logic: if tag-only, do not convert the tag to a release + if form.TagOnly || !rel.IsTag { ctx.Data["Err_TagName"] = true ctx.RenderWithErr(ctx.Tr("repo.release.tag_name_already_exist"), tplReleaseNew, &form) return } + + // convert a tag to a real release (set is_tag=false) rel.Title = form.Title rel.Note = form.Content rel.Target = form.Target diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index 7f5866a3098eb..fab3a6da640af 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -69,8 +69,8 @@ func TestNewReleasePost(t *testing.T) { assert.Equal(t, "content", rel.Note) }) - t.Run("ReleaseExistsDoUpdate", func(t *testing.T) { - post(t, forms.NewReleaseForm{ + t.Run("ReleaseExistsDoUpdate(non-tag)", func(t *testing.T) { + ctx := post(t, forms.NewReleaseForm{ TagName: "v1.1", Target: "master", Title: "updated-title", @@ -78,8 +78,38 @@ func TestNewReleasePost(t *testing.T) { }) rel := loadRelease(t, "v1.1") require.NotNil(t, rel) + assert.False(t, rel.IsTag) + assert.Equal(t, "testing-release", rel.Title) + assert.NotEmpty(t, ctx.Flash.ErrorMsg) + }) + + t.Run("ReleaseExistsDoUpdate(tag-only)", func(t *testing.T) { + ctx := post(t, forms.NewReleaseForm{ + TagName: "delete-tag", // a strange name, but it is the only "is_tag=true" fixture + Target: "master", + Title: "updated-title", + Content: "updated-content", + TagOnly: true, + }) + rel := loadRelease(t, "delete-tag") + require.NotNil(t, rel) + assert.True(t, rel.IsTag) // the record should not be updated because the request is "tag-only". TODO: need to improve the logic? + assert.Equal(t, "delete-tag", rel.Title) + assert.NotEmpty(t, ctx.Flash.ErrorMsg) + }) + + t.Run("ReleaseExistsDoUpdate(tag-release)", func(t *testing.T) { + ctx := post(t, forms.NewReleaseForm{ + TagName: "delete-tag", // a strange name, but it is the only "is_tag=true" fixture + Target: "master", + Title: "updated-title", + Content: "updated-content", + }) + rel := loadRelease(t, "delete-tag") + require.NotNil(t, rel) + assert.False(t, rel.IsTag) // the tag has been "updated" to be a real "release" assert.Equal(t, "updated-title", rel.Title) - assert.Equal(t, "updated-content", rel.Note) + assert.Empty(t, ctx.Flash.ErrorMsg) }) t.Run("TagOnly", func(t *testing.T) { From 57aef6ddf1c0eff7a891ac8eb470a04786c00d36 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Jan 2025 07:22:14 +0800 Subject: [PATCH 12/15] fix tag-only button logic --- routers/web/repo/release.go | 13 +++++++++---- routers/web/repo/release_test.go | 4 ++-- templates/repo/release/new.tmpl | 2 +- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index ee33f8dfdf366..02b90989363bd 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -360,6 +360,8 @@ func NewRelease(ctx *context.Context) { return } + ctx.Data["ShowCreateTagOnlyButton"] = true + // pre-fill the form with the tag name, target branch and the existing release (if exists) ctx.Data["tag_target"] = ctx.Repo.Repository.DefaultBranch if tagName := ctx.FormString("tag"); tagName != "" { @@ -376,7 +378,7 @@ func NewRelease(ctx *context.Context) { return } - ctx.Data["TagNameReleaseExists"] = true + ctx.Data["ShowCreateTagOnlyButton"] = false ctx.Data["tag_name"] = rel.TagName ctx.Data["tag_target"] = rel.Target ctx.Data["title"] = rel.Title @@ -397,14 +399,17 @@ func NewReleasePost(ctx *context.Context) { form := web.GetForm(ctx).(*forms.NewReleaseForm) - // first, check whether the release exists, - // it should be done before the form error check, because the tmpl needs "TagNameReleaseExists" to show/hide the "tag only" button + // first, check whether the release exists, and prepare "ShowCreateTagOnlyButton" + // the logic should be done before the form error check to make the tmpl has correct variables rel, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, form.TagName) if err != nil && !repo_model.IsErrReleaseNotExist(err) { ctx.ServerError("GetRelease", err) return } - ctx.Data["TagNameReleaseExists"] = rel != nil + + // we should still show the "tag only" button if the user clicks it, no matter the release exists or not. + // because if error occurs, end users need to have the chance to edit the name and submit the form with "tag-only" again. + ctx.Data["ShowCreateTagOnlyButton"] = form.TagOnly || rel == nil // do some form checks if ctx.HasError() { diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index fab3a6da640af..bc72752da1dda 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -34,9 +34,9 @@ func TestNewReleasePost(t *testing.T) { t.Run("NewReleasePage", func(t *testing.T) { ctx := get(t, "v1.1") - assert.NotEmpty(t, ctx.Data["TagNameReleaseExists"]) + assert.Empty(t, ctx.Data["ShowCreateTagOnlyButton"]) ctx = get(t, "new-tag-name") - assert.Empty(t, ctx.Data["TagNameReleaseExists"]) + assert.NotEmpty(t, ctx.Data["ShowCreateTagOnlyButton"]) }) post := func(t *testing.T, form forms.NewReleaseForm) *context.Context { diff --git a/templates/repo/release/new.tmpl b/templates/repo/release/new.tmpl index 7bba17e0aa2c3..8b6aa252affda 100644 --- a/templates/repo/release/new.tmpl +++ b/templates/repo/release/new.tmpl @@ -115,7 +115,7 @@ {{end}} {{else}} - {{if not .TagNameReleaseExists}} + {{if .ShowCreateTagOnlyButton}} {{end}} From be79b34c01febb31e39daee3b1244de07bf82380 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Jan 2025 07:26:39 +0800 Subject: [PATCH 13/15] add more comments --- routers/web/repo/release.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 02b90989363bd..4a95ee1e66833 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -407,8 +407,13 @@ func NewReleasePost(ctx *context.Context) { return } - // we should still show the "tag only" button if the user clicks it, no matter the release exists or not. - // because if error occurs, end users need to have the chance to edit the name and submit the form with "tag-only" again. + // We should still show the "tag only" button if the user clicks it, no matter the release exists or not. + // Because if error occurs, end users need to have the chance to edit the name and submit the form with "tag-only" again. + // It is still not completely right, because there could still be cases like this: + // * user visit "new release" page, see the "tag only" button + // * input something, click other buttons but not "tag only" + // * error occurs, the "new release" page is rendered again, but the "tag only" button is gone + // Such cases are not able to be handled by current code, it needs frontend code to toggle the "tag-only" button if the input changes. ctx.Data["ShowCreateTagOnlyButton"] = form.TagOnly || rel == nil // do some form checks From 43c5100e3e22882b58b93aecfe3f7b53760ad9fc Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Jan 2025 07:28:40 +0800 Subject: [PATCH 14/15] improve comment --- routers/web/repo/release.go | 1 + 1 file changed, 1 insertion(+) diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go index 4a95ee1e66833..8909dedbb16a9 100644 --- a/routers/web/repo/release.go +++ b/routers/web/repo/release.go @@ -414,6 +414,7 @@ func NewReleasePost(ctx *context.Context) { // * input something, click other buttons but not "tag only" // * error occurs, the "new release" page is rendered again, but the "tag only" button is gone // Such cases are not able to be handled by current code, it needs frontend code to toggle the "tag-only" button if the input changes. + // Or another choice is "always show the tag-only button" if error occurs. ctx.Data["ShowCreateTagOnlyButton"] = form.TagOnly || rel == nil // do some form checks From 7cd7d601790e7f8d5c0649f4a72a5f08a6c693e8 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 14 Jan 2025 09:00:30 +0800 Subject: [PATCH 15/15] improve test --- routers/web/repo/release_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/routers/web/repo/release_test.go b/routers/web/repo/release_test.go index bc72752da1dda..9f49fc750070c 100644 --- a/routers/web/repo/release_test.go +++ b/routers/web/repo/release_test.go @@ -96,6 +96,7 @@ func TestNewReleasePost(t *testing.T) { assert.True(t, rel.IsTag) // the record should not be updated because the request is "tag-only". TODO: need to improve the logic? assert.Equal(t, "delete-tag", rel.Title) assert.NotEmpty(t, ctx.Flash.ErrorMsg) + assert.NotEmpty(t, ctx.Data["ShowCreateTagOnlyButton"]) // still show the "tag-only" button }) t.Run("ReleaseExistsDoUpdate(tag-release)", func(t *testing.T) {