From 8cd3237a9ef6dfa438d97c7ff2ffbd5fb8db6247 Mon Sep 17 00:00:00 2001 From: John Olheiser Date: Tue, 27 Sep 2022 17:23:58 -0500 Subject: [PATCH] Better repo API unit checks (#21130) This PR would presumably Fix #20522 Fix #18773 Fix #19069 Fix #21077 Fix #13622 ----- 1. Check whether unit type is currently enabled 2. Check if it _will_ be enabled via opt 3. Allow modification as necessary Signed-off-by: jolheiser Co-authored-by: wxiaoguang Co-authored-by: 6543 <6543@obermui.de> --- modules/structs/repo.go | 24 ++++++++++++------------ routers/api/v1/repo/repo.go | 31 +++++++++++++++++++++++-------- templates/swagger/v1_json.tmpl | 18 +++++++++--------- tests/e2e/e2e_test.go | 3 +-- 4 files changed, 45 insertions(+), 31 deletions(-) diff --git a/modules/structs/repo.go b/modules/structs/repo.go index d3833105d7..27e7f4618c 100644 --- a/modules/structs/repo.go +++ b/modules/structs/repo.go @@ -151,13 +151,13 @@ type EditRepoOption struct { Template *bool `json:"template,omitempty"` // either `true` to enable issues for this repository or `false` to disable them. HasIssues *bool `json:"has_issues,omitempty"` - // set this structure to configure internal issue tracker (requires has_issues) + // set this structure to configure internal issue tracker InternalTracker *InternalTracker `json:"internal_tracker,omitempty"` - // set this structure to use external issue tracker (requires has_issues) + // set this structure to use external issue tracker ExternalTracker *ExternalTracker `json:"external_tracker,omitempty"` // either `true` to enable the wiki for this repository or `false` to disable it. HasWiki *bool `json:"has_wiki,omitempty"` - // set this structure to use external wiki instead of internal (requires has_wiki) + // set this structure to use external wiki instead of internal ExternalWiki *ExternalWiki `json:"external_wiki,omitempty"` // sets the default branch for this repository. DefaultBranch *string `json:"default_branch,omitempty"` @@ -165,25 +165,25 @@ type EditRepoOption struct { HasPullRequests *bool `json:"has_pull_requests,omitempty"` // either `true` to enable project unit, or `false` to disable them. HasProjects *bool `json:"has_projects,omitempty"` - // either `true` to ignore whitespace for conflicts, or `false` to not ignore whitespace. `has_pull_requests` must be `true`. + // either `true` to ignore whitespace for conflicts, or `false` to not ignore whitespace. IgnoreWhitespaceConflicts *bool `json:"ignore_whitespace_conflicts,omitempty"` - // either `true` to allow merging pull requests with a merge commit, or `false` to prevent merging pull requests with merge commits. `has_pull_requests` must be `true`. + // either `true` to allow merging pull requests with a merge commit, or `false` to prevent merging pull requests with merge commits. AllowMerge *bool `json:"allow_merge_commits,omitempty"` - // either `true` to allow rebase-merging pull requests, or `false` to prevent rebase-merging. `has_pull_requests` must be `true`. + // either `true` to allow rebase-merging pull requests, or `false` to prevent rebase-merging. AllowRebase *bool `json:"allow_rebase,omitempty"` - // either `true` to allow rebase with explicit merge commits (--no-ff), or `false` to prevent rebase with explicit merge commits. `has_pull_requests` must be `true`. + // either `true` to allow rebase with explicit merge commits (--no-ff), or `false` to prevent rebase with explicit merge commits. AllowRebaseMerge *bool `json:"allow_rebase_explicit,omitempty"` - // either `true` to allow squash-merging pull requests, or `false` to prevent squash-merging. `has_pull_requests` must be `true`. + // either `true` to allow squash-merging pull requests, or `false` to prevent squash-merging. AllowSquash *bool `json:"allow_squash_merge,omitempty"` - // either `true` to allow mark pr as merged manually, or `false` to prevent it. `has_pull_requests` must be `true`. + // either `true` to allow mark pr as merged manually, or `false` to prevent it. AllowManualMerge *bool `json:"allow_manual_merge,omitempty"` - // either `true` to enable AutodetectManualMerge, or `false` to prevent it. `has_pull_requests` must be `true`, Note: In some special cases, misjudgments can occur. + // either `true` to enable AutodetectManualMerge, or `false` to prevent it. Note: In some special cases, misjudgments can occur. AutodetectManualMerge *bool `json:"autodetect_manual_merge,omitempty"` - // either `true` to allow updating pull request branch by rebase, or `false` to prevent it. `has_pull_requests` must be `true`. + // either `true` to allow updating pull request branch by rebase, or `false` to prevent it. AllowRebaseUpdate *bool `json:"allow_rebase_update,omitempty"` // set to `true` to delete pr branch after merge by default DefaultDeleteBranchAfterMerge *bool `json:"default_delete_branch_after_merge,omitempty"` - // set to a merge style to be used by this repository: "merge", "rebase", "rebase-merge", or "squash". `has_pull_requests` must be `true`. + // set to a merge style to be used by this repository: "merge", "rebase", "rebase-merge", or "squash". DefaultMergeStyle *string `json:"default_merge_style,omitempty"` // set to `true` to archive this repository. Archived *bool `json:"archived,omitempty"` diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go index 6f40bb3e42..319c4c781a 100644 --- a/routers/api/v1/repo/repo.go +++ b/routers/api/v1/repo/repo.go @@ -732,8 +732,13 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { var units []repo_model.RepoUnit var deleteUnitTypes []unit_model.Type + currHasIssues := repo.UnitEnabledCtx(ctx, unit_model.TypeIssues) + newHasIssues := currHasIssues if opts.HasIssues != nil { - if *opts.HasIssues && opts.ExternalTracker != nil && !unit_model.TypeExternalTracker.UnitGlobalDisabled() { + newHasIssues = *opts.HasIssues + } + if currHasIssues || newHasIssues { + if newHasIssues && opts.ExternalTracker != nil && !unit_model.TypeExternalTracker.UnitGlobalDisabled() { // Check that values are valid if !validation.IsValidExternalURL(opts.ExternalTracker.ExternalTrackerURL) { err := fmt.Errorf("External tracker URL not valid") @@ -756,7 +761,7 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { }, }) deleteUnitTypes = append(deleteUnitTypes, unit_model.TypeIssues) - } else if *opts.HasIssues && opts.ExternalTracker == nil && !unit_model.TypeIssues.UnitGlobalDisabled() { + } else if newHasIssues && opts.ExternalTracker == nil && !unit_model.TypeIssues.UnitGlobalDisabled() { // Default to built-in tracker var config *repo_model.IssuesConfig @@ -783,7 +788,7 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { Config: config, }) deleteUnitTypes = append(deleteUnitTypes, unit_model.TypeExternalTracker) - } else if !*opts.HasIssues { + } else if !newHasIssues { if !unit_model.TypeExternalTracker.UnitGlobalDisabled() { deleteUnitTypes = append(deleteUnitTypes, unit_model.TypeExternalTracker) } @@ -793,8 +798,13 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { } } + currHasWiki := repo.UnitEnabledCtx(ctx, unit_model.TypeWiki) + newHasWiki := currHasWiki if opts.HasWiki != nil { - if *opts.HasWiki && opts.ExternalWiki != nil && !unit_model.TypeExternalWiki.UnitGlobalDisabled() { + newHasWiki = *opts.HasWiki + } + if currHasWiki || newHasWiki { + if newHasWiki && opts.ExternalWiki != nil && !unit_model.TypeExternalWiki.UnitGlobalDisabled() { // Check that values are valid if !validation.IsValidExternalURL(opts.ExternalWiki.ExternalWikiURL) { err := fmt.Errorf("External wiki URL not valid") @@ -810,7 +820,7 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { }, }) deleteUnitTypes = append(deleteUnitTypes, unit_model.TypeWiki) - } else if *opts.HasWiki && opts.ExternalWiki == nil && !unit_model.TypeWiki.UnitGlobalDisabled() { + } else if newHasWiki && opts.ExternalWiki == nil && !unit_model.TypeWiki.UnitGlobalDisabled() { config := &repo_model.UnitConfig{} units = append(units, repo_model.RepoUnit{ RepoID: repo.ID, @@ -818,7 +828,7 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { Config: config, }) deleteUnitTypes = append(deleteUnitTypes, unit_model.TypeExternalWiki) - } else if !*opts.HasWiki { + } else if !newHasWiki { if !unit_model.TypeExternalWiki.UnitGlobalDisabled() { deleteUnitTypes = append(deleteUnitTypes, unit_model.TypeExternalWiki) } @@ -828,8 +838,13 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { } } + currHasPullRequests := repo.UnitEnabledCtx(ctx, unit_model.TypePullRequests) + newHasPullRequests := currHasPullRequests if opts.HasPullRequests != nil { - if *opts.HasPullRequests && !unit_model.TypePullRequests.UnitGlobalDisabled() { + newHasPullRequests = *opts.HasPullRequests + } + if currHasPullRequests || newHasPullRequests { + if newHasPullRequests && !unit_model.TypePullRequests.UnitGlobalDisabled() { // We do allow setting individual PR settings through the API, so // we get the config settings and then set them // if those settings were provided in the opts. @@ -889,7 +904,7 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error { Type: unit_model.TypePullRequests, Config: config, }) - } else if !*opts.HasPullRequests && !unit_model.TypePullRequests.UnitGlobalDisabled() { + } else if !newHasPullRequests && !unit_model.TypePullRequests.UnitGlobalDisabled() { deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests) } } diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 15f71c244a..47100450f9 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -15569,32 +15569,32 @@ "type": "object", "properties": { "allow_manual_merge": { - "description": "either `true` to allow mark pr as merged manually, or `false` to prevent it. `has_pull_requests` must be `true`.", + "description": "either `true` to allow mark pr as merged manually, or `false` to prevent it.", "type": "boolean", "x-go-name": "AllowManualMerge" }, "allow_merge_commits": { - "description": "either `true` to allow merging pull requests with a merge commit, or `false` to prevent merging pull requests with merge commits. `has_pull_requests` must be `true`.", + "description": "either `true` to allow merging pull requests with a merge commit, or `false` to prevent merging pull requests with merge commits.", "type": "boolean", "x-go-name": "AllowMerge" }, "allow_rebase": { - "description": "either `true` to allow rebase-merging pull requests, or `false` to prevent rebase-merging. `has_pull_requests` must be `true`.", + "description": "either `true` to allow rebase-merging pull requests, or `false` to prevent rebase-merging.", "type": "boolean", "x-go-name": "AllowRebase" }, "allow_rebase_explicit": { - "description": "either `true` to allow rebase with explicit merge commits (--no-ff), or `false` to prevent rebase with explicit merge commits. `has_pull_requests` must be `true`.", + "description": "either `true` to allow rebase with explicit merge commits (--no-ff), or `false` to prevent rebase with explicit merge commits.", "type": "boolean", "x-go-name": "AllowRebaseMerge" }, "allow_rebase_update": { - "description": "either `true` to allow updating pull request branch by rebase, or `false` to prevent it. `has_pull_requests` must be `true`.", + "description": "either `true` to allow updating pull request branch by rebase, or `false` to prevent it.", "type": "boolean", "x-go-name": "AllowRebaseUpdate" }, "allow_squash_merge": { - "description": "either `true` to allow squash-merging pull requests, or `false` to prevent squash-merging. `has_pull_requests` must be `true`.", + "description": "either `true` to allow squash-merging pull requests, or `false` to prevent squash-merging.", "type": "boolean", "x-go-name": "AllowSquash" }, @@ -15604,7 +15604,7 @@ "x-go-name": "Archived" }, "autodetect_manual_merge": { - "description": "either `true` to enable AutodetectManualMerge, or `false` to prevent it. `has_pull_requests` must be `true`, Note: In some special cases, misjudgments can occur.", + "description": "either `true` to enable AutodetectManualMerge, or `false` to prevent it. Note: In some special cases, misjudgments can occur.", "type": "boolean", "x-go-name": "AutodetectManualMerge" }, @@ -15619,7 +15619,7 @@ "x-go-name": "DefaultDeleteBranchAfterMerge" }, "default_merge_style": { - "description": "set to a merge style to be used by this repository: \"merge\", \"rebase\", \"rebase-merge\", or \"squash\". `has_pull_requests` must be `true`.", + "description": "set to a merge style to be used by this repository: \"merge\", \"rebase\", \"rebase-merge\", or \"squash\".", "type": "string", "x-go-name": "DefaultMergeStyle" }, @@ -15660,7 +15660,7 @@ "x-go-name": "HasWiki" }, "ignore_whitespace_conflicts": { - "description": "either `true` to ignore whitespace for conflicts, or `false` to not ignore whitespace. `has_pull_requests` must be `true`.", + "description": "either `true` to ignore whitespace for conflicts, or `false` to not ignore whitespace.", "type": "boolean", "x-go-name": "IgnoreWhitespaceConflicts" }, diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index eb2b1d70f8..6028ff74fc 100644 --- a/tests/e2e/e2e_test.go +++ b/tests/e2e/e2e_test.go @@ -72,8 +72,7 @@ func TestMain(m *testing.M) { os.Exit(exitVal) } -// TestE2e should be the only test e2e necessary. It will collect all "*.test.e2e.js" -// files in this directory and build a test for each. +// TestE2e should be the only test e2e necessary. It will collect all "*.test.e2e.js" files in this directory and build a test for each. func TestE2e(t *testing.T) { // Find the paths of all e2e test files in test test directory. searchGlob := filepath.Join(filepath.Dir(setting.AppPath), "tests", "e2e", "*.test.e2e.js")