Get rules by id when editing branch protection rule (#22932)

When users rename an existing branch protection rule, a new rule with
the new name will be created and the old rule will still exist.

![image](https://user-images.githubusercontent.com/15528715/219276442-d3c001ad-e693-44ec-9ad2-b33f2666b49b.png)

---

![image](https://user-images.githubusercontent.com/15528715/219276478-547c3b93-b3f1-4292-a1ef-c1b7747fe1bb.png)

The reason is that the `SettingsProtectedBranchPost` function only get
branch protection rule by name before updating or creating a rule. When
the rule name changes, the function cannot find the existing rule so it
will create a new rule rather than update the existing rule. To fix the
bug, the function should get rule by id first.

---------

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 411a585c81d60131e30b3769aa6a0e6408beb334..92ca5be8d3584a696111a45d8e20fb59c82e4749 100644 (file)
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -2152,6 +2152,7 @@ settings.choose_branch = Choose a branch…
 settings.no_protected_branch = There are no protected branches.
 settings.edit_protected_branch = Edit
 settings.protected_branch_required_rule_name = Required rule name
+settings.protected_branch_duplicate_rule_name = Duplicate rule name
 settings.protected_branch_required_approvals_min = Required approvals cannot be negative.
 settings.tags = Tags
 settings.tags.protection = Tag Protection

diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go
index a54565c1f151286dd45017eac749bc8fd35ca500..0a8c39fef0735199d1942cb19b85844bfa78c5e2 100644 (file)
--- a/routers/web/repo/setting_protected_branch.go
+++ b/routers/web/repo/setting_protected_branch.go
@@ -166,10 +166,36 @@ func SettingsProtectedBranchPost(ctx *context.Context) {
        }
 
        var err error
-       protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName)
-       if err != nil {
-               ctx.ServerError("GetProtectBranchOfRepoByName", err)
-               return
+       if f.RuleID > 0 {
+               // If the RuleID isn't 0, it must be an edit operation. So we get rule by id.
+               protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, f.RuleID)
+               if err != nil {
+                       ctx.ServerError("GetProtectBranchOfRepoByID", err)
+                       return
+               }
+               if protectBranch != nil && protectBranch.RuleName != f.RuleName {
+                       // RuleName changed. We need to check if there is a rule with the same name.
+                       // If a rule with the same name exists, an error should be returned.
+                       sameNameProtectBranch, err := git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName)
+                       if err != nil {
+                               ctx.ServerError("GetProtectBranchOfRepoByName", err)
+                               return
+                       }
+                       if sameNameProtectBranch != nil {
+                               ctx.Flash.Error(ctx.Tr("repo.settings.protected_branch_duplicate_rule_name"))
+                               ctx.Redirect(fmt.Sprintf("%s/settings/branches/edit?rule_name=%s", ctx.Repo.RepoLink, protectBranch.RuleName))
+                               return
+                       }
+               }
+       } else {
+               // FIXME: If a new ProtectBranch has a duplicate RuleName, an error should be returned.
+               // Currently, if a new ProtectBranch with a duplicate RuleName is created, the existing ProtectBranch will be updated.
+               // But we cannot modify this logic now because many unit tests rely on it.
+               protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName)
+               if err != nil {
+                       ctx.ServerError("GetProtectBranchOfRepoByName", err)
+                       return
+               }
        }
        if protectBranch == nil {
                // No options found, create defaults.

diff --git a/services/forms/repo_form.go b/services/forms/repo_form.go
index ff0916f8e195cb4bb766b6ea20aeead32738043d..374ae0ac5cb598cd6c1c83ed36e69c8d8c766b9e 100644 (file)
--- a/services/forms/repo_form.go
+++ b/services/forms/repo_form.go
@@ -190,6 +190,7 @@ func (f *RepoSettingForm) Validate(req *http.Request, errs binding.Errors) bindi
 // ProtectBranchForm form for changing protected branch settings
 type ProtectBranchForm struct {
        RuleName                      string `binding:"Required"`
+       RuleID                        int64
        EnablePush                    string
        WhitelistUsers                string
        WhitelistTeams                string