From 13892121e63e401b4af5807c8e836733d01ad6a8 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Wed, 24 Apr 2024 23:33:08 +0200 Subject: [PATCH 1/5] tests: Refactor CreateDeclarativeRepo Lets introduce a new helper function, `CreateDeclarativeRepoWithOptions`! This is almost the same as the existing `CreateDeclarativeRepo` helper, but instead of taking a list of random parameters the author thought of at the time of its introduction, it takes a `DeclarativeRepoOptions` struct, with optional members. This makes it easier to extend the function, as new members can be added without breaking or having to update existing callsites, as long as the newly added members default to compatible values. `CreateDeclarativeRepo` is then reimplemented on top of the new function. Callsites aren't updated yet, we can do that organically, whenever touching code that uses the older function. No new functionality is introduced just yet, this is merely a refactor. Signed-off-by: Gergely Nagy --- tests/integration/integration_test.go | 65 ++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 10 deletions(-) diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index 8fd55d7fa8..e50338d5fa 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -36,6 +36,7 @@ import ( "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/testlogger" "code.gitea.io/gitea/modules/util" @@ -652,15 +653,26 @@ func GetHTMLTitle(t testing.TB, session *TestSession, urlStr string) string { return doc.Find("head title").Text() } -func CreateDeclarativeRepo(t *testing.T, owner *user_model.User, name string, enabledUnits, disabledUnits []unit_model.Type, files []*files_service.ChangeRepoFile) (*repo_model.Repository, string, func()) { +type DeclarativeRepoOptions struct { + Name optional.Option[string] + EnabledUnits optional.Option[[]unit_model.Type] + DisabledUnits optional.Option[[]unit_model.Type] + Files optional.Option[[]*files_service.ChangeRepoFile] +} + +func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts DeclarativeRepoOptions) (*repo_model.Repository, string, func()) { t.Helper() - repoName := name - if repoName == "" { + // Not using opts.Name.ValueOrDefault() here to avoid unnecessarily + // generating an UUID when a name is specified. + var repoName string + if opts.Name.Has() { + repoName = opts.Name.Value() + } else { repoName = gouuid.NewString() } - // Create a new repository + // Create the repository repo, err := repo_service.CreateRepository(db.DefaultContext, owner, owner, repo_service.CreateRepoOptions{ Name: repoName, Description: "Temporary Repo", @@ -673,21 +685,31 @@ func CreateDeclarativeRepo(t *testing.T, owner *user_model.User, name string, en assert.NoError(t, err) assert.NotEmpty(t, repo) - if enabledUnits != nil || disabledUnits != nil { - units := make([]repo_model.RepoUnit, len(enabledUnits)) - for i, unitType := range enabledUnits { - units[i] = repo_model.RepoUnit{ + // Populate `enabledUnits` if we have any enabled. + var enabledUnits []repo_model.RepoUnit + if opts.EnabledUnits.Has() { + units := opts.EnabledUnits.Value() + enabledUnits = make([]repo_model.RepoUnit, len(units)) + + for i, unitType := range units { + enabledUnits[i] = repo_model.RepoUnit{ RepoID: repo.ID, Type: unitType, } } + } - err := repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, units, disabledUnits) + // Adjust the repo units according to our parameters. + if opts.EnabledUnits.Has() || opts.DisabledUnits.Has() { + err := repo_service.UpdateRepositoryUnits(db.DefaultContext, repo, enabledUnits, opts.DisabledUnits.ValueOrDefault(nil)) assert.NoError(t, err) } + // Add files, if any. var sha string - if len(files) > 0 { + if opts.Files.Has() { + files := opts.Files.Value() + resp, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, owner, &files_service.ChangeRepoFilesOptions{ Files: files, Message: "add files", @@ -712,7 +734,30 @@ func CreateDeclarativeRepo(t *testing.T, owner *user_model.User, name string, en sha = resp.Commit.SHA } + // Return the repo, the top commit, and a defer-able function to delete the + // repo. return repo, sha, func() { repo_service.DeleteRepository(db.DefaultContext, owner, repo, false) } } + +func CreateDeclarativeRepo(t *testing.T, owner *user_model.User, name string, enabledUnits, disabledUnits []unit_model.Type, files []*files_service.ChangeRepoFile) (*repo_model.Repository, string, func()) { + t.Helper() + + var opts DeclarativeRepoOptions + + if name != "" { + opts.Name = optional.Some(name) + } + if enabledUnits != nil { + opts.EnabledUnits = optional.Some(enabledUnits) + } + if disabledUnits != nil { + opts.DisabledUnits = optional.Some(disabledUnits) + } + if files != nil { + opts.Files = optional.Some(files) + } + + return CreateDeclarativeRepoWithOptions(t, owner, opts) +} From 0da02b92136f7c254fd0075e6612007108960fda Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Thu, 25 Apr 2024 00:08:53 +0200 Subject: [PATCH 2/5] tests: Let CreateDeclarativeRepoWithOptions create a Wiki too Add a new member to `DeclarativeRepoOptions`: `WikiBranch`. If specified, create a Wiki with the given branch, and a single "Home" page. This will be used by an upcoming test. Signed-off-by: Gergely Nagy --- tests/integration/integration_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index e50338d5fa..c92dca472a 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -46,6 +46,7 @@ import ( repo_service "code.gitea.io/gitea/services/repository" files_service "code.gitea.io/gitea/services/repository/files" user_service "code.gitea.io/gitea/services/user" + wiki_service "code.gitea.io/gitea/services/wiki" "code.gitea.io/gitea/tests" "github.com/PuerkitoBio/goquery" @@ -658,6 +659,7 @@ type DeclarativeRepoOptions struct { EnabledUnits optional.Option[[]unit_model.Type] DisabledUnits optional.Option[[]unit_model.Type] Files optional.Option[[]*files_service.ChangeRepoFile] + WikiBranch optional.Option[string] } func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts DeclarativeRepoOptions) (*repo_model.Repository, string, func()) { @@ -734,6 +736,22 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts sha = resp.Commit.SHA } + // If there's a Wiki branch specified, create a wiki, and a default wiki page. + if opts.WikiBranch.Has() { + // Set the wiki branch in the database first + repo.WikiBranch = opts.WikiBranch.Value() + err := repo_model.UpdateRepositoryCols(db.DefaultContext, repo, "wiki_branch") + assert.NoError(t, err) + + // Initialize the wiki + err = wiki_service.InitWiki(db.DefaultContext, repo) + assert.NoError(t, err) + + // Add a new wiki page + err = wiki_service.AddWikiPage(db.DefaultContext, owner, repo, "Home", "Welcome to the wiki!", "Add a Home page") + assert.NoError(t, err) + } + // Return the repo, the top commit, and a defer-able function to delete the // repo. return repo, sha, func() { From 6f35a5ab904325027be24c98bcf6fdd9f9ddce54 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 30 Apr 2024 11:21:41 +0200 Subject: [PATCH 3/5] Fix Issue watching / unwatching on the web ui When subscribing or unsubscribing to/from an issue on the web ui, the request was posted to a route handled by `repo.IssueWatch`. This function used `ctx.Req.PostForm.Get()`, erroneously. `request.PostForm` is *only* available if `request.ParseForm()` has been called before it. The function in question did not do that. Under some circumstances, something, somewhere did end up calling `ParseForm()`, but not in every scenario. Since we do not need to check for multiple values, the easiest fix here is to use `ctx.Req.PostFormValue`, which will call `ParseForm()` if necessary. Fixes #3516. Signed-off-by: Gergely Nagy --- release-notes/8.0.0/fix/3562.md | 1 + routers/web/repo/issue_watch.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 release-notes/8.0.0/fix/3562.md diff --git a/release-notes/8.0.0/fix/3562.md b/release-notes/8.0.0/fix/3562.md new file mode 100644 index 0000000000..8099056205 --- /dev/null +++ b/release-notes/8.0.0/fix/3562.md @@ -0,0 +1 @@ +Fixed a bug where subscribing to or unsubscribing from an issue in a repository with no code produced an internal server error. diff --git a/routers/web/repo/issue_watch.go b/routers/web/repo/issue_watch.go index 8b033f3b17..6799bf8eb2 100644 --- a/routers/web/repo/issue_watch.go +++ b/routers/web/repo/issue_watch.go @@ -46,7 +46,7 @@ func IssueWatch(ctx *context.Context) { return } - watch, err := strconv.ParseBool(ctx.Req.PostForm.Get("watch")) + watch, err := strconv.ParseBool(ctx.Req.PostFormValue("watch")) if err != nil { ctx.ServerError("watch is not bool", err) return From f83ae0fad232a69ea30243390bba80e266ee2e41 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 30 Apr 2024 01:35:40 +0200 Subject: [PATCH 4/5] tests: Support creating a declarative repo without AutoInit To be able to easily test cases where the repository does not have any code, where the git repo itself is completely uninitialized, lets support a case where the `AutoInit` property is false. For the sake of backwards compatibility, if the option is not set either way, it will default to `true`. Signed-off-by: Gergely Nagy --- tests/integration/integration_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/integration/integration_test.go b/tests/integration/integration_test.go index c92dca472a..f1c6b9f135 100644 --- a/tests/integration/integration_test.go +++ b/tests/integration/integration_test.go @@ -660,6 +660,7 @@ type DeclarativeRepoOptions struct { DisabledUnits optional.Option[[]unit_model.Type] Files optional.Option[[]*files_service.ChangeRepoFile] WikiBranch optional.Option[string] + AutoInit optional.Option[bool] } func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts DeclarativeRepoOptions) (*repo_model.Repository, string, func()) { @@ -674,11 +675,18 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts repoName = gouuid.NewString() } + var autoInit bool + if opts.AutoInit.Has() { + autoInit = opts.AutoInit.Value() + } else { + autoInit = true + } + // Create the repository repo, err := repo_service.CreateRepository(db.DefaultContext, owner, owner, repo_service.CreateRepoOptions{ Name: repoName, Description: "Temporary Repo", - AutoInit: true, + AutoInit: autoInit, Gitignores: "", License: "WTFPL", Readme: "Default", @@ -710,6 +718,7 @@ func CreateDeclarativeRepoWithOptions(t *testing.T, owner *user_model.User, opts // Add files, if any. var sha string if opts.Files.Has() { + assert.True(t, autoInit, "Files cannot be specified if AutoInit is disabled") files := opts.Files.Value() resp, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, owner, &files_service.ChangeRepoFilesOptions{ From 36b8e68eeeca5f29705b8c00278f8a00fc1fc739 Mon Sep 17 00:00:00 2001 From: Gergely Nagy Date: Tue, 30 Apr 2024 13:06:31 +0200 Subject: [PATCH 5/5] Add a test case for unsubscribing from an issue Signed-off-by: Gergely Nagy --- tests/integration/issue_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/integration/issue_test.go b/tests/integration/issue_test.go index 84487a847a..94ba670b71 100644 --- a/tests/integration/issue_test.go +++ b/tests/integration/issue_test.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/indexer/issues" + "code.gitea.io/gitea/modules/optional" "code.gitea.io/gitea/modules/references" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" @@ -866,3 +867,23 @@ body: }) }) } + +func TestIssueUnsubscription(t *testing.T) { + onGiteaRun(t, func(t *testing.T, u *url.URL) { + defer tests.PrepareTestEnv(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + repo, _, f := CreateDeclarativeRepoWithOptions(t, user, DeclarativeRepoOptions{ + AutoInit: optional.Some(false), + }) + defer f() + session := loginUser(t, user.Name) + + issueURL := testNewIssue(t, session, user.Name, repo.Name, "Issue title", "Description") + req := NewRequestWithValues(t, "POST", fmt.Sprintf("%s/watch", issueURL), map[string]string{ + "_csrf": GetCSRF(t, session, issueURL), + "watch": "0", + }) + session.MakeRequest(t, req, http.StatusOK) + }) +}