Fix API not persisting pull request unit config when has_pull_requests is not set

models/repo/repo_unit.go

@@ -134,10 +134,25 @@ type PullRequestsConfig struct {
 	DefaultTargetBranch           string
 }
 
+func DefaultPullRequestsConfig() *PullRequestsConfig {
+	cfg := &PullRequestsConfig{
+		AllowMerge:                 true,
+		AllowRebase:                true,
+		AllowRebaseMerge:           true,
+		AllowSquash:                true,
+		AllowFastForwardOnly:       true,
+		AllowRebaseUpdate:          true,
+		DefaultAllowMaintainerEdit: true,
+	}
+	cfg.DefaultMergeStyle = MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle)
+	cfg.DefaultMergeStyle = util.IfZero(cfg.DefaultMergeStyle, MergeStyleMerge)
+	return cfg
+}
+
 // FromDB fills up a PullRequestsConfig from serialized format.
 func (cfg *PullRequestsConfig) FromDB(bs []byte) error {
-	// AllowRebaseUpdate = true as default for existing PullRequestConfig in DB
-	cfg.AllowRebaseUpdate = true
+	// set default values for existing PullRequestConfig in DB
+	*cfg = *DefaultPullRequestsConfig()
 	return json.UnmarshalHandleDoubleEncode(bs, &cfg)
 }
 
@@ -156,17 +171,8 @@ func (cfg *PullRequestsConfig) IsMergeStyleAllowed(mergeStyle MergeStyle) bool {
 		mergeStyle == MergeStyleManuallyMerged && cfg.AllowManualMerge
 }
 
-// GetDefaultMergeStyle returns the default merge style for this pull request
-func (cfg *PullRequestsConfig) GetDefaultMergeStyle() MergeStyle {
-	if len(cfg.DefaultMergeStyle) != 0 {
-		return cfg.DefaultMergeStyle
-	}
-
-	if setting.Repository.PullRequest.DefaultMergeStyle != "" {
-		return MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle)
-	}
-
-	return MergeStyleMerge
+func DefaultPullRequestsUnit(repoID int64) RepoUnit {
+	return RepoUnit{RepoID: repoID, Type: unit.TypePullRequests, Config: DefaultPullRequestsConfig()}
 }
 
 type ActionsConfig struct {

modules/optional/option.go

@@ -67,3 +67,17 @@ func ParseBool(s string) Option[bool] {
 	}
 	return Some(v)
 }
+
+func AssignPtrValue[T comparable](changed *bool, target, src *T) {
+	if src != nil && *src != *target {
+		*target = *src
+		*changed = true
+	}
+}
+
+func AssignPtrString[TO, FROM ~string](changed *bool, target *TO, src *FROM) {
+	if src != nil && string(*src) != string(*target) {
+		*target = TO(*src)
+		*changed = true
+	}
+}

routers/api/v1/repo/repo.go

@@ -884,77 +884,44 @@ func updateRepoUnits(ctx *context.APIContext, opts api.EditRepoOption) error {
 		}
 	}
 
-	if opts.HasPullRequests != nil && !unit_model.TypePullRequests.UnitGlobalDisabled() {
-		if *opts.HasPullRequests {
-			// 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.
+	if !unit_model.TypePullRequests.UnitGlobalDisabled() {
+		mustDeletePullRequestUnit := opts.HasPullRequests != nil && !*opts.HasPullRequests
+		mustInsertPullRequestUnit := opts.HasPullRequests != nil && *opts.HasPullRequests
+		if mustDeletePullRequestUnit {
+			deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests)
+		} else {
+			// 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.
 			unit, err := repo.GetUnit(ctx, unit_model.TypePullRequests)
-			var config *repo_model.PullRequestsConfig
-			if err != nil {
-				// Unit type doesn't exist so we make a new config file with default values
-				config = &repo_model.PullRequestsConfig{
-					IgnoreWhitespaceConflicts:     false,
-					AllowMerge:                    true,
-					AllowRebase:                   true,
-					AllowRebaseMerge:              true,
-					AllowSquash:                   true,
-					AllowFastForwardOnly:          true,
-					AllowManualMerge:              true,
-					AutodetectManualMerge:         false,
-					AllowRebaseUpdate:             true,
-					DefaultDeleteBranchAfterMerge: false,
-					DefaultMergeStyle:             repo_model.MergeStyleMerge,
-					DefaultAllowMaintainerEdit:    false,
-				}
-			} else {
-				config = unit.PullRequestsConfig()
-			}
-
-			if opts.IgnoreWhitespaceConflicts != nil {
-				config.IgnoreWhitespaceConflicts = *opts.IgnoreWhitespaceConflicts
-			}
-			if opts.AllowMerge != nil {
-				config.AllowMerge = *opts.AllowMerge
-			}
-			if opts.AllowRebase != nil {
-				config.AllowRebase = *opts.AllowRebase
-			}
-			if opts.AllowRebaseMerge != nil {
-				config.AllowRebaseMerge = *opts.AllowRebaseMerge
-			}
-			if opts.AllowSquash != nil {
-				config.AllowSquash = *opts.AllowSquash
-			}
-			if opts.AllowFastForwardOnly != nil {
-				config.AllowFastForwardOnly = *opts.AllowFastForwardOnly
-			}
-			if opts.AllowManualMerge != nil {
-				config.AllowManualMerge = *opts.AllowManualMerge
-			}
-			if opts.AutodetectManualMerge != nil {
-				config.AutodetectManualMerge = *opts.AutodetectManualMerge
-			}
-			if opts.AllowRebaseUpdate != nil {
-				config.AllowRebaseUpdate = *opts.AllowRebaseUpdate
-			}
-			if opts.DefaultDeleteBranchAfterMerge != nil {
-				config.DefaultDeleteBranchAfterMerge = *opts.DefaultDeleteBranchAfterMerge
-			}
-			if opts.DefaultMergeStyle != nil {
-				config.DefaultMergeStyle = repo_model.MergeStyle(*opts.DefaultMergeStyle)
+			if err != nil && !errors.Is(err, util.ErrNotExist) {
+				return err
 			}
-			if opts.DefaultAllowMaintainerEdit != nil {
-				config.DefaultAllowMaintainerEdit = *opts.DefaultAllowMaintainerEdit
+			if unit == nil {
+				// Unit doesn't exist yet but is being enabled, create with defaults
+				unit = new(repo_model.DefaultPullRequestsUnit(repo.ID))
 			}
 
-			units = append(units, repo_model.RepoUnit{
-				RepoID: repo.ID,
-				Type:   unit_model.TypePullRequests,
-				Config: config,
-			})
-		} else {
-			deleteUnitTypes = append(deleteUnitTypes, unit_model.TypePullRequests)
+			changed := new(false)
+			config := unit.PullRequestsConfig()
+			optional.AssignPtrValue(changed, &config.IgnoreWhitespaceConflicts, opts.IgnoreWhitespaceConflicts)
+			optional.AssignPtrValue(changed, &config.AllowMerge, opts.AllowMerge)
+			optional.AssignPtrValue(changed, &config.AllowRebase, opts.AllowRebase)
+			optional.AssignPtrValue(changed, &config.AllowRebaseMerge, opts.AllowRebaseMerge)
+			optional.AssignPtrValue(changed, &config.AllowSquash, opts.AllowSquash)
+			optional.AssignPtrValue(changed, &config.AllowFastForwardOnly, opts.AllowFastForwardOnly)
+			optional.AssignPtrValue(changed, &config.AllowManualMerge, opts.AllowManualMerge)
+			optional.AssignPtrValue(changed, &config.AutodetectManualMerge, opts.AutodetectManualMerge)
+			optional.AssignPtrValue(changed, &config.AllowRebaseUpdate, opts.AllowRebaseUpdate)
+			optional.AssignPtrValue(changed, &config.DefaultDeleteBranchAfterMerge, opts.DefaultDeleteBranchAfterMerge)
+			optional.AssignPtrValue(changed, &config.DefaultAllowMaintainerEdit, opts.DefaultAllowMaintainerEdit)
+			optional.AssignPtrString(changed, &config.DefaultMergeStyle, opts.DefaultMergeStyle)
+			if *changed || mustInsertPullRequestUnit {
+				units = append(units, repo_model.RepoUnit{
+					RepoID: repo.ID,
+					Type:   unit_model.TypePullRequests,
+					Config: config,
+				})
+			}
 		}
 	}
 

routers/web/repo/issue_view.go

@@ -905,9 +905,8 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss
 	// Check correct values and select default
 	if ms, ok := ctx.Data["MergeStyle"].(repo_model.MergeStyle); !ok ||
 		!prConfig.IsMergeStyleAllowed(ms) {
-		defaultMergeStyle := prConfig.GetDefaultMergeStyle()
-		if prConfig.IsMergeStyleAllowed(defaultMergeStyle) && !ok {
-			mergeStyle = defaultMergeStyle
+		if prConfig.IsMergeStyleAllowed(prConfig.DefaultMergeStyle) && !ok {
+			mergeStyle = prConfig.DefaultMergeStyle
 		} else if prConfig.AllowMerge {
 			mergeStyle = repo_model.MergeStyleMerge
 		} else if prConfig.AllowRebase {

services/convert/repository.go

@@ -118,7 +118,7 @@ func innerToRepo(ctx context.Context, repo *repo_model.Repository, permissionInR
 		allowManualMerge = config.AllowManualMerge
 		autodetectManualMerge = config.AutodetectManualMerge
 		defaultDeleteBranchAfterMerge = config.DefaultDeleteBranchAfterMerge
-		defaultMergeStyle = config.GetDefaultMergeStyle()
+		defaultMergeStyle = config.DefaultMergeStyle
 		defaultAllowMaintainerEdit = config.DefaultAllowMaintainerEdit
 		defaultTargetBranch = config.DefaultTargetBranch
 	}

services/repository/create.go

@@ -386,15 +386,7 @@ func createRepositoryInDB(ctx context.Context, doer, u *user_model.User, repo *r
 				},
 			})
 		case unit.TypePullRequests:
-			units = append(units, repo_model.RepoUnit{
-				RepoID: repo.ID,
-				Type:   tp,
-				Config: &repo_model.PullRequestsConfig{
-					AllowMerge: true, AllowRebase: true, AllowRebaseMerge: true, AllowSquash: true, AllowFastForwardOnly: true,
-					DefaultMergeStyle: repo_model.MergeStyle(setting.Repository.PullRequest.DefaultMergeStyle),
-					AllowRebaseUpdate: true,
-				},
-			})
+			units = append(units, repo_model.DefaultPullRequestsUnit(repo.ID))
 		case unit.TypeProjects:
 			units = append(units, repo_model.RepoUnit{
 				RepoID: repo.ID,

tests/integration/api_pull_test.go

@@ -279,10 +279,10 @@ func TestAPICreatePullSuccess(t *testing.T) {
 	}).AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusCreated)
 
-	// Also test that AllowMaintainerEdit is false by default
+	// Also test that AllowMaintainerEdit is true by default, the "false" case is covered by TestAPICreatePullBasePermission
 	prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle})
 	pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID})
-	assert.False(t, pr.AllowMaintainerEdit)
+	assert.True(t, pr.AllowMaintainerEdit)
 
 	MakeRequest(t, req, http.StatusUnprocessableEntity) // second request should fail
 }
@@ -304,7 +304,7 @@ func TestAPICreatePullBasePermission(t *testing.T) {
 		Base:  "master",
 		Title: prTitle,
 
-		AllowMaintainerEdit: new(true),
+		AllowMaintainerEdit: new(false),
 	}
 	req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusForbidden)
@@ -317,10 +317,10 @@ func TestAPICreatePullBasePermission(t *testing.T) {
 	req = NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls", owner10.Name, repo10.Name), &opts).AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusCreated)
 
-	// Also test that AllowMaintainerEdit is set to true
+	// Also test that AllowMaintainerEdit is set to false, the default "true" case is covered by TestAPICreatePullSuccess
 	prIssue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: prTitle})
 	pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: prIssue.ID})
-	assert.True(t, pr.AllowMaintainerEdit)
+	assert.False(t, pr.AllowMaintainerEdit)
 }
 
 func TestAPICreatePullHeadPermission(t *testing.T) {

tests/integration/api_repo_edit_test.go

@@ -418,5 +418,20 @@ func TestAPIRepoEdit(t *testing.T) {
 		req = NewRequestWithJSON(t, "PATCH", fmt.Sprintf("/api/v1/repos/%s/%s", user2.Name, repo1.Name), &repoEditOption).
 			AddTokenAuth(token4)
 		MakeRequest(t, req, http.StatusForbidden)
+
+		// Test updating pull request settings without setting has_pull_requests
+		repo1 = unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+		url = fmt.Sprintf("/api/v1/repos/%s/%s", user2.Name, repo1.Name)
+		req = NewRequestWithJSON(t, "PATCH", url, &api.EditRepoOption{
+			DefaultDeleteBranchAfterMerge: &bTrue,
+		}).AddTokenAuth(token2)
+		resp = MakeRequest(t, req, http.StatusOK)
+		DecodeJSON(t, resp, &repo)
+		assert.True(t, repo.DefaultDeleteBranchAfterMerge)
+		// reset
+		req = NewRequestWithJSON(t, "PATCH", url, &api.EditRepoOption{
+			DefaultDeleteBranchAfterMerge: &bFalse,
+		}).AddTokenAuth(token2)
+		_ = MakeRequest(t, req, http.StatusOK)
 	})
 }