Skip to content

Commit 532fa06

Browse files
WazzapsWazzaps
committed
Display template validation errors in issue template picker + Make markdown template parse errors be hard errors
1 parent aa4c7a1 commit 532fa06

File tree

6 files changed

+147
-37
lines changed

6 files changed

+147
-37
lines changed

‎modules/context/repo.go

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"net/http"
1414
"net/url"
1515
"path"
16+
"strconv"
1617
"strings"
1718

1819
"code.gitea.io/gitea/models"
@@ -1033,11 +1034,23 @@ func UnitTypes() func(ctx *Context) {
10331034
}
10341035
}
10351036

1036-
func ExtractTemplateFromYaml(templateContent []byte, meta *api.IssueTemplate) (*api.IssueFormTemplate, error) {
1037+
func ExtractTemplateFromYaml(templateContent []byte, meta *api.IssueTemplate) (*api.IssueFormTemplate, []string, error) {
10371038
var tmpl *api.IssueFormTemplate
10381039
err := yaml.Unmarshal(templateContent, &tmpl)
10391040
if err != nil {
1040-
return nil, err
1041+
return nil, nil, err
1042+
}
1043+
1044+
// Make sure it's valid
1045+
if validationErrs := tmpl.Valid(); len(validationErrs) > 0 {
1046+
return nil, validationErrs, fmt.Errorf("invalid issue template: %v", validationErrs)
1047+
}
1048+
1049+
// Fill missing field IDs with the field index
1050+
for i, f := range tmpl.Fields {
1051+
if f.ID == "" {
1052+
tmpl.Fields[i].ID = strconv.FormatInt(int64(i+1), 10)
1053+
}
10411054
}
10421055

10431056
// Copy metadata
@@ -1050,22 +1063,23 @@ func ExtractTemplateFromYaml(templateContent []byte, meta *api.IssueTemplate) (*
10501063
meta.Ref = tmpl.Ref
10511064
}
10521065

1053-
return tmpl, nil
1066+
return tmpl, nil, nil
10541067
}
10551068

10561069
// IssueTemplatesFromDefaultBranch checks for issue templates in the repo's default branch
1057-
func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
1070+
func (ctx *Context) IssueTemplatesFromDefaultBranch() ([]api.IssueTemplate, map[string][]string) {
10581071
var issueTemplates []api.IssueTemplate
1072+
validationErrs := make(map[string][]string)
10591073

10601074
if ctx.Repo.Repository.IsEmpty {
1061-
return issueTemplates
1075+
return issueTemplates, nil
10621076
}
10631077

10641078
if ctx.Repo.Commit == nil {
10651079
var err error
10661080
ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetBranchCommit(ctx.Repo.Repository.DefaultBranch)
10671081
if err != nil {
1068-
return issueTemplates
1082+
return issueTemplates, nil
10691083
}
10701084
}
10711085

@@ -1076,7 +1090,7 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
10761090
}
10771091
entries, err := tree.ListEntries()
10781092
if err != nil {
1079-
return issueTemplates
1093+
return issueTemplates, nil
10801094
}
10811095
for _, entry := range entries {
10821096
if strings.HasSuffix(entry.Name(), ".md") {
@@ -1111,6 +1125,8 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
11111125
it.FileName = entry.Name()
11121126
if it.Valid() {
11131127
issueTemplates = append(issueTemplates, it)
1128+
} else {
1129+
fmt.Printf("%#v\n", it)
11141130
}
11151131
} else if strings.HasSuffix(entry.Name(), ".yaml") || strings.HasSuffix(entry.Name(), ".yml") {
11161132
if entry.Blob().Size() >= setting.UI.MaxDisplayFileSize {
@@ -1137,9 +1153,14 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
11371153

11381154
var it api.IssueTemplate
11391155
it.FileName = path.Base(entry.Name())
1140-
_, err = ExtractTemplateFromYaml(templateContent, &it)
1156+
1157+
var tmplValidationErrs []string
1158+
_, tmplValidationErrs, err = ExtractTemplateFromYaml(templateContent, &it)
11411159
if err != nil {
11421160
log.Debug("ExtractTemplateFromYaml: %v", err)
1161+
if tmplValidationErrs != nil {
1162+
validationErrs[path.Base(entry.Name())] = tmplValidationErrs
1163+
}
11431164
continue
11441165
}
11451166
if it.Valid() {
@@ -1148,8 +1169,8 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
11481169
}
11491170
}
11501171
if len(issueTemplates) > 0 {
1151-
return issueTemplates
1172+
return issueTemplates, validationErrs
11521173
}
11531174
}
1154-
return issueTemplates
1175+
return issueTemplates, validationErrs
11551176
}

‎modules/structs/issue_form.go

Lines changed: 67 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44

55
package structs
66

7-
import "strings"
7+
import (
8+
"fmt"
9+
"strings"
10+
)
811

912
type FormField struct {
1013
Type string `yaml:"type"`
@@ -26,18 +29,72 @@ type IssueFormTemplate struct {
2629
FileName string `yaml:"-"`
2730
}
2831

29-
// Valid checks whether an IssueFormTemplate is considered valid, e.g. at least name and about
30-
func (it IssueFormTemplate) Valid() bool {
31-
if strings.TrimSpace(it.Name) == "" || strings.TrimSpace(it.About) == "" {
32-
return false
32+
// Valid checks whether an IssueFormTemplate is considered valid, e.g. at least name and about, and labels for all fields
33+
func (it IssueFormTemplate) Valid() []string {
34+
// TODO: Localize error messages
35+
// TODO: Add a bunch more validations
36+
var errs []string
37+
38+
if strings.TrimSpace(it.Name) == "" {
39+
errs = append(errs, "the 'name' field of the issue template are required")
40+
}
41+
if strings.TrimSpace(it.About) == "" {
42+
errs = append(errs, "the 'about' field of the issue template are required")
3343
}
3444

35-
for _, field := range it.Fields {
36-
if strings.TrimSpace(field.ID) == "" {
37-
// TODO: add IDs should be optional, maybe generate slug from label? or use numberic id
38-
return false
45+
// Make sure all non-markdown fields have labels
46+
for fieldIdx, field := range it.Fields {
47+
// Make checker functions
48+
checkStringAttr := func(attrName string) {
49+
attr := field.Attributes[attrName]
50+
if attr == nil || strings.TrimSpace(attr.(string)) == "" {
51+
errs = append(errs, fmt.Sprintf(
52+
"(field #%d '%s'): the '%s' attribute is required for fields with type %s",
53+
fieldIdx+1, field.ID, attrName, field.Type,
54+
))
55+
}
56+
}
57+
checkOptionsStringAttr := func(optionIdx int, option map[interface{}]interface{}, attrName string) {
58+
attr := option[attrName]
59+
if attr == nil || strings.TrimSpace(attr.(string)) == "" {
60+
errs = append(errs, fmt.Sprintf(
61+
"(field #%d '%s', option #%d): the '%s' field is required for options",
62+
fieldIdx+1, field.ID, optionIdx, attrName,
63+
))
64+
}
65+
}
66+
checkListAttr := func(attrName string, itemChecker func(int, map[interface{}]interface{})) {
67+
attr := field.Attributes[attrName]
68+
if attr == nil {
69+
errs = append(errs, fmt.Sprintf(
70+
"(field #%d '%s'): the '%s' attribute is required for fields with type %s",
71+
fieldIdx+1, field.ID, attrName, field.Type,
72+
))
73+
} else {
74+
for i, item := range attr.([]interface{}) {
75+
itemChecker(i, item.(map[interface{}]interface{}))
76+
}
77+
}
78+
}
79+
80+
// Make sure each field has its attributes
81+
switch field.Type {
82+
case "markdown":
83+
checkStringAttr("value")
84+
case "textarea", "input", "dropdown":
85+
checkStringAttr("label")
86+
case "checkboxes":
87+
checkStringAttr("label")
88+
checkListAttr("options", func(i int, item map[interface{}]interface{}) {
89+
checkOptionsStringAttr(i, item, "label")
90+
})
91+
default:
92+
errs = append(errs, fmt.Sprintf(
93+
"(field #%d '%s'): unknown type '%s'",
94+
fieldIdx+1, field.ID, field.Type,
95+
))
3996
}
4097
}
4198

42-
return true
99+
return errs
43100
}

‎routers/api/v1/repo/repo.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1080,5 +1080,6 @@ func GetIssueTemplates(ctx *context.APIContext) {
10801080
// "200":
10811081
// "$ref": "#/responses/IssueTemplates"
10821082

1083-
ctx.JSON(http.StatusOK, ctx.IssueTemplatesFromDefaultBranch())
1083+
issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
1084+
ctx.JSON(http.StatusOK, issueTemplates)
10841085
}

‎routers/web/repo/issue.go

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ const (
6565

6666
issueTemplateKey = "IssueTemplate"
6767
issueFormTemplateKey = "IssueFormTemplate"
68+
issueFormErrorsKey = "IssueTemplateErrors"
6869
issueTemplateTitleKey = "IssueTemplateTitle"
6970
)
7071

@@ -408,7 +409,8 @@ func Issues(ctx *context.Context) {
408409
}
409410
ctx.Data["Title"] = ctx.Tr("repo.issues")
410411
ctx.Data["PageIsIssueList"] = true
411-
ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
412+
issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
413+
ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
412414
}
413415

414416
issues(ctx, ctx.FormInt64("milestone"), ctx.FormInt64("project"), util.OptionalBoolOf(isPullList))
@@ -751,7 +753,9 @@ func getFileContentFromDefaultBranch(repo *context.Repository, filename string)
751753
return string(bytes), true
752754
}
753755

754-
func getTemplate(repo *context.Repository, template string, possibleDirs, possibleFiles []string) (*api.IssueTemplate, string, *api.IssueFormTemplate, error) {
756+
func getTemplate(repo *context.Repository, template string, possibleDirs, possibleFiles []string) (*api.IssueTemplate, string, *api.IssueFormTemplate, map[string][]string, error) {
757+
validationErrs := make(map[string][]string)
758+
755759
// Add `possibleFiles` and each `{possibleDirs}/{template}` to `templateCandidates`
756760
templateCandidates := make([]string, 0, len(possibleFiles))
757761
if template != "" {
@@ -772,36 +776,42 @@ func getTemplate(repo *context.Repository, template string, possibleDirs, possib
772776

773777
if strings.HasSuffix(filename, ".md") {
774778
// Parse markdown template
775-
templateBody, err = markdown.ExtractMetadata(templateContent, meta)
779+
templateBody, err = markdown.ExtractMetadata(templateContent, &meta)
776780
} else if strings.HasSuffix(filename, ".yaml") || strings.HasSuffix(filename, ".yml") {
777781
// Parse yaml (form) template
778-
formTemplateBody, err = context.ExtractTemplateFromYaml([]byte(templateContent), &meta)
779-
formTemplateBody.FileName = path.Base(filename)
782+
var tmplValidationErrs []string
783+
formTemplateBody, tmplValidationErrs, err = context.ExtractTemplateFromYaml([]byte(templateContent), &meta)
784+
if err == nil {
785+
formTemplateBody.FileName = path.Base(filename)
786+
} else if tmplValidationErrs != nil {
787+
validationErrs[path.Base(filename)] = tmplValidationErrs
788+
}
780789
} else {
781790
err = errors.New("invalid template type")
782791
}
783792
if err != nil {
784793
log.Debug("could not extract metadata from %s [%s]: %v", filename, repo.Repository.FullName(), err)
785-
templateBody = templateContent
786-
err = nil
787794
}
788795

789-
return &meta, templateBody, formTemplateBody, err
796+
return &meta, templateBody, formTemplateBody, validationErrs, err
790797
}
791798
}
792799

793-
return nil, "", nil, errors.New("no template found")
800+
return nil, "", nil, validationErrs, errors.New("no template found")
794801
}
795802

796803
func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleDirs, possibleFiles []string) {
797-
templateMeta, templateBody, formTemplateBody, err := getTemplate(ctx.Repo, ctx.FormString("template"), possibleDirs, possibleFiles)
804+
templateMeta, templateBody, formTemplateBody, validationErrs, err := getTemplate(ctx.Repo, ctx.FormString("template"), possibleDirs, possibleFiles)
798805
if err != nil {
799806
return
800807
}
801808

802809
if formTemplateBody != nil {
803810
ctx.Data[issueFormTemplateKey] = formTemplateBody
804811
}
812+
if validationErrs != nil && len(validationErrs) > 0 {
813+
ctx.Data[issueFormErrorsKey] = validationErrs
814+
}
805815

806816
ctx.Data[issueTemplateTitleKey] = templateMeta.Title
807817
ctx.Data[ctxDataKey] = templateBody
@@ -836,7 +846,8 @@ func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleDirs,
836846
func NewIssue(ctx *context.Context) {
837847
ctx.Data["Title"] = ctx.Tr("repo.issues.new")
838848
ctx.Data["PageIsIssueList"] = true
839-
ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
849+
issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
850+
ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
840851
ctx.Data["RequireTribute"] = true
841852
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
842853
title := ctx.FormString("title")
@@ -893,7 +904,10 @@ func NewIssueChooseTemplate(ctx *context.Context) {
893904
ctx.Data["Title"] = ctx.Tr("repo.issues.new")
894905
ctx.Data["PageIsIssueList"] = true
895906

896-
issueTemplates := ctx.IssueTemplatesFromDefaultBranch()
907+
issueTemplates, validationErrs := ctx.IssueTemplatesFromDefaultBranch()
908+
if validationErrs != nil && len(validationErrs) > 0 {
909+
ctx.Data[issueFormErrorsKey] = validationErrs
910+
}
897911
ctx.Data["IssueTemplates"] = issueTemplates
898912

899913
if len(issueTemplates) == 0 {
@@ -1039,7 +1053,7 @@ func renderIssueFormValues(ctx *context.Context, form *url.Values) (string, erro
10391053
}
10401054

10411055
// Fetch template
1042-
_, _, formTemplateBody, err := getTemplate(
1056+
_, _, formTemplateBody, _, err := getTemplate(
10431057
ctx.Repo,
10441058
form.Get("form-type"),
10451059
context.IssueTemplateDirCandidates,
@@ -1094,7 +1108,8 @@ func NewIssuePost(ctx *context.Context) {
10941108
form := web.GetForm(ctx).(*forms.CreateIssueForm)
10951109
ctx.Data["Title"] = ctx.Tr("repo.issues.new")
10961110
ctx.Data["PageIsIssueList"] = true
1097-
ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
1111+
issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
1112+
ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
10981113
ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
10991114
ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
11001115
upload.AddUploadContext(ctx, "comment")
@@ -1287,7 +1302,8 @@ func ViewIssue(ctx *context.Context) {
12871302
return
12881303
}
12891304
ctx.Data["PageIsIssueList"] = true
1290-
ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
1305+
issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
1306+
ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
12911307
}
12921308

12931309
if issue.IsPull && !ctx.Repo.CanRead(unit.TypeIssues) {

‎routers/web/repo/milestone.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,8 @@ func MilestoneIssuesAndPulls(ctx *context.Context) {
290290
ctx.Data["Milestone"] = milestone
291291

292292
issues(ctx, milestoneID, 0, util.OptionalBoolNone)
293-
ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
293+
issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
294+
ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
294295

295296
ctx.Data["CanWriteIssues"] = ctx.Repo.CanWriteIssuesOrPulls(false)
296297
ctx.Data["CanWritePulls"] = ctx.Repo.CanWriteIssuesOrPulls(true)

‎templates/repo/issue/choose.tmpl

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,20 @@
3030
</div>
3131
</div>
3232
</div>
33+
{{- if .IssueTemplateErrors}}
34+
<div class="ui warning message">
35+
<div class="text left">
36+
<div>The following issue templates have errors:</div>
37+
<ul>
38+
{{range $filename, $errors := .IssueTemplateErrors}}
39+
{{range $errors}}
40+
<li>{{$filename}}: {{.}}</li>
41+
{{end}}
42+
{{end}}
43+
</ul>
44+
</div>
45+
</div>
46+
{{end}}
3347
</div>
3448
</div>
3549
{{template "base/footer" .}}

0 commit comments

Comments
 (0)