From ad020dfdc650b71c629b2e6bab9764ab574f2bbd Mon Sep 17 00:00:00 2001 From: zetta Date: Thu, 16 Feb 2023 11:01:03 +0800 Subject: [PATCH 01/13] get protected branch rule by id instead of name --- routers/web/repo/setting_protected_branch.go | 10 ++++++---- services/forms/repo_form.go | 1 + templates/swagger/v1_json.tmpl | 5 +++++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index a54565c1f1512..b57c251332636 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -166,10 +166,12 @@ 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 { + protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, f.RuleID) + if err != nil { + ctx.ServerError("GetProtectedBranchRuleByID", 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 c1b58009688b4..3091e777a666c 100644 --- 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 diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index e096faf3f3b8f..0e25424ae1c35 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14502,6 +14502,11 @@ "type": "string", "x-go-name": "RuleName" }, + "rule_id": { + "type": "integer", + "format": "int64", + "x-go-name": "RuleID" + }, "status_check_contexts": { "type": "array", "items": { From 4097e3eb0074191b12def259279f56411d5237e3 Mon Sep 17 00:00:00 2001 From: zetta Date: Thu, 16 Feb 2023 13:50:17 +0800 Subject: [PATCH 02/13] add the rule_id field --- modules/structs/repo_branch.go | 1 + templates/swagger/v1_json.tmpl | 9 ++++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index e9927aec2797a..9af6902710b23 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -25,6 +25,7 @@ type BranchProtection struct { // Deprecated: true BranchName string `json:"branch_name"` RuleName string `json:"rule_name"` + RuleID string `json:"rule_id"` EnablePush bool `json:"enable_push"` EnablePushWhitelist bool `json:"enable_push_whitelist"` PushWhitelistUsernames []string `json:"push_whitelist_usernames"` diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 0e25424ae1c35..5d7c192165a65 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14498,15 +14498,14 @@ "format": "int64", "x-go-name": "RequiredApprovals" }, + "rule_id": { + "type": "string", + "x-go-name": "RuleID" + }, "rule_name": { "type": "string", "x-go-name": "RuleName" }, - "rule_id": { - "type": "integer", - "format": "int64", - "x-go-name": "RuleID" - }, "status_check_contexts": { "type": "array", "items": { From 8d1f1c9868a044148d6c853603fd5f12d0436f0e Mon Sep 17 00:00:00 2001 From: zetta Date: Thu, 16 Feb 2023 14:35:21 +0800 Subject: [PATCH 03/13] fix err declaration --- routers/web/repo/setting_protected_branch.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index b57c251332636..ed43d6208044b 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -165,8 +165,8 @@ func SettingsProtectedBranchPost(ctx *context.Context) { return } - var err error if f.RuleID > 0 { + var err error protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, f.RuleID) if err != nil { ctx.ServerError("GetProtectedBranchRuleByID", err) @@ -245,7 +245,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch - err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ + err := git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ UserIDs: whitelistUsers, TeamIDs: whitelistTeams, MergeUserIDs: mergeWhitelistUsers, From fc0c63cb4bad3042bdf6f562d234114e6eb9f868 Mon Sep 17 00:00:00 2001 From: zetta Date: Thu, 16 Feb 2023 15:43:56 +0800 Subject: [PATCH 04/13] get rule by id after getting rule by name failed --- routers/web/repo/setting_protected_branch.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index ed43d6208044b..6d85e6b6bde44 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -165,8 +165,13 @@ func SettingsProtectedBranchPost(ctx *context.Context) { return } - if f.RuleID > 0 { - var err error + var err error + protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName) + if err != nil { + ctx.ServerError("GetProtectedBranchRuleByName", err) + return + } + if protectBranch == nil && f.RuleID > 0 { protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, f.RuleID) if err != nil { ctx.ServerError("GetProtectedBranchRuleByID", err) @@ -245,7 +250,7 @@ func SettingsProtectedBranchPost(ctx *context.Context) { protectBranch.UnprotectedFilePatterns = f.UnprotectedFilePatterns protectBranch.BlockOnOutdatedBranch = f.BlockOnOutdatedBranch - err := git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ + err = git_model.UpdateProtectBranch(ctx, ctx.Repo.Repository, protectBranch, git_model.WhitelistOptions{ UserIDs: whitelistUsers, TeamIDs: whitelistTeams, MergeUserIDs: mergeWhitelistUsers, From 96e3dd5fbd950de4e0311bce9c4268cd458f579b Mon Sep 17 00:00:00 2001 From: zetta Date: Thu, 16 Feb 2023 15:45:16 +0800 Subject: [PATCH 05/13] fix error msg --- routers/web/repo/setting_protected_branch.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 6d85e6b6bde44..e068a6b3d0b09 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -168,13 +168,13 @@ 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("GetProtectedBranchRuleByName", err) + ctx.ServerError("GetProtectBranchOfRepoByName", err) return } if protectBranch == nil && f.RuleID > 0 { protectBranch, err = git_model.GetProtectedBranchRuleByID(ctx, ctx.Repo.Repository.ID, f.RuleID) if err != nil { - ctx.ServerError("GetProtectedBranchRuleByID", err) + ctx.ServerError("GetProtectBranchOfRepoByID", err) return } } From c8fcf2f2b13751594e10619d1fcb711192a6d11f Mon Sep 17 00:00:00 2001 From: zetta Date: Thu, 16 Feb 2023 19:06:58 +0800 Subject: [PATCH 06/13] update comment --- routers/web/repo/setting_protected_branch.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index e068a6b3d0b09..8eedd3905edc6 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -165,6 +165,11 @@ func SettingsProtectedBranchPost(ctx *context.Context) { return } + // FIXME: If a new ProtectBranch has a duplicate RuleName, an error should be returned. + + // Currently an existing ProtectBranch will be updated according to the new one has the same name. + // But we cannot modify this logic now because many unit tests rely on it. + var err error protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName) if err != nil { From 59f2fb1da6aec74fe1f5bc02c7b934aeceb8c98d Mon Sep 17 00:00:00 2001 From: zetta Date: Thu, 16 Feb 2023 19:11:32 +0800 Subject: [PATCH 07/13] update comment --- routers/web/repo/setting_protected_branch.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 8eedd3905edc6..ec1faaf11f82a 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -167,7 +167,8 @@ func SettingsProtectedBranchPost(ctx *context.Context) { // FIXME: If a new ProtectBranch has a duplicate RuleName, an error should be returned. - // Currently an existing ProtectBranch will be updated according to the new one has the same name. + // Currently, if a new ProtectBranch with the same name is created, + // the existing ProtectBranch will be updated. // But we cannot modify this logic now because many unit tests rely on it. var err error From f03b608aa92c7c019c5c2c157ea02e3b97c72ad0 Mon Sep 17 00:00:00 2001 From: zetta Date: Fri, 17 Feb 2023 10:05:12 +0800 Subject: [PATCH 08/13] get rule by id first --- routers/web/repo/setting_protected_branch.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index ec1faaf11f82a..8cf90085b9d66 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -166,23 +166,25 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } // FIXME: If a new ProtectBranch has a duplicate RuleName, an error should be returned. - // Currently, if a new ProtectBranch with the same name is created, // the existing ProtectBranch will be updated. // But we cannot modify this logic now because many unit tests rely on it. var err error - protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName) - if err != nil { - ctx.ServerError("GetProtectBranchOfRepoByName", err) - return - } - if protectBranch == nil && f.RuleID > 0 { + 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 } + } else { + // The RuleID is 0, it should be a create operation. We can only get rule by name. + 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. From 2dad832b64acaa20cb3004034cf6933503d36d86 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 20 Feb 2023 11:36:08 +0800 Subject: [PATCH 09/13] check rule name --- routers/web/repo/setting_protected_branch.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 8cf90085b9d66..13772fae14d70 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -178,6 +178,20 @@ func SettingsProtectedBranchPost(ctx *context.Context) { 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 { // The RuleID is 0, it should be a create operation. We can only get rule by name. protectBranch, err = git_model.GetProtectedBranchRuleByName(ctx, ctx.Repo.Repository.ID, f.RuleName) From 4b9823e68ad65d6aeafc8819449c8633bdcdfca3 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 20 Feb 2023 12:04:04 +0800 Subject: [PATCH 10/13] add error message --- options/locale/locale_en-US.ini | 1 + 1 file changed, 1 insertion(+) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 411a585c81d60..92ca5be8d3584 100644 --- 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 From 1ec3b6ba94a6cca97354a50c84f53f781ed22477 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 20 Feb 2023 13:44:04 +0800 Subject: [PATCH 11/13] update fixme message --- routers/web/repo/setting_protected_branch.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 13772fae14d70..4d7003d702da0 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -165,11 +165,6 @@ func SettingsProtectedBranchPost(ctx *context.Context) { return } - // FIXME: If a new ProtectBranch has a duplicate RuleName, an error should be returned. - // Currently, if a new ProtectBranch with the same name is created, - // the existing ProtectBranch will be updated. - // But we cannot modify this logic now because many unit tests rely on it. - var err error if f.RuleID > 0 { // If the RuleID isn't 0, it must be an edit operation. So we get rule by id. @@ -193,7 +188,10 @@ func SettingsProtectedBranchPost(ctx *context.Context) { } } } else { - // The RuleID is 0, it should be a create operation. We can only get rule by name. + // 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) From cd5688fa73790218f5609316203c0ef4f992fdc1 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 20 Feb 2023 13:52:45 +0800 Subject: [PATCH 12/13] remove unnecessary code --- modules/structs/repo_branch.go | 1 - templates/swagger/v1_json.tmpl | 4 ---- 2 files changed, 5 deletions(-) diff --git a/modules/structs/repo_branch.go b/modules/structs/repo_branch.go index 9af6902710b23..e9927aec2797a 100644 --- a/modules/structs/repo_branch.go +++ b/modules/structs/repo_branch.go @@ -25,7 +25,6 @@ type BranchProtection struct { // Deprecated: true BranchName string `json:"branch_name"` RuleName string `json:"rule_name"` - RuleID string `json:"rule_id"` EnablePush bool `json:"enable_push"` EnablePushWhitelist bool `json:"enable_push_whitelist"` PushWhitelistUsernames []string `json:"push_whitelist_usernames"` diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 588f2fd91e219..2a675766abc92 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -14498,10 +14498,6 @@ "format": "int64", "x-go-name": "RequiredApprovals" }, - "rule_id": { - "type": "string", - "x-go-name": "RuleID" - }, "rule_name": { "type": "string", "x-go-name": "RuleName" From 9be8b2dfbadae94fb0455bba771de7924e894476 Mon Sep 17 00:00:00 2001 From: Zettat123 Date: Mon, 20 Feb 2023 14:05:10 +0800 Subject: [PATCH 13/13] remove empty line --- routers/web/repo/setting_protected_branch.go | 1 - 1 file changed, 1 deletion(-) diff --git a/routers/web/repo/setting_protected_branch.go b/routers/web/repo/setting_protected_branch.go index 4d7003d702da0..0a8c39fef0735 100644 --- a/routers/web/repo/setting_protected_branch.go +++ b/routers/web/repo/setting_protected_branch.go @@ -191,7 +191,6 @@ func SettingsProtectedBranchPost(ctx *context.Context) { // 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)