Display template validation errors in issue template picker + Make markdown template parse errors be hard errors

Wazzaps · Wazzaps · commit 532fa06cd79f · 2022-08-14T01:53:16.000+03:00
diff --git a/modules/context/repo.go b/modules/context/repo.go
@@ -13,6 +13,7 @@ import (
 	"net/http"
 	"net/url"
 	"path"
+	"strconv"
 	"strings"
 
 	"code.gitea.io/gitea/models"
@@ -1033,11 +1034,23 @@ func UnitTypes() func(ctx *Context) {
 	}
 }
 
-func ExtractTemplateFromYaml(templateContent []byte, meta *api.IssueTemplate) (*api.IssueFormTemplate, error) {
+func ExtractTemplateFromYaml(templateContent []byte, meta *api.IssueTemplate) (*api.IssueFormTemplate, []string, error) {
 	var tmpl *api.IssueFormTemplate
 	err := yaml.Unmarshal(templateContent, &tmpl)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
+	}
+
+	// Make sure it's valid
+	if validationErrs := tmpl.Valid(); len(validationErrs) > 0 {
+		return nil, validationErrs, fmt.Errorf("invalid issue template: %v", validationErrs)
+	}
+
+	// Fill missing field IDs with the field index
+	for i, f := range tmpl.Fields {
+		if f.ID == "" {
+			tmpl.Fields[i].ID = strconv.FormatInt(int64(i+1), 10)
+		}
 	}
 
 	// Copy metadata
@@ -1050,22 +1063,23 @@ func ExtractTemplateFromYaml(templateContent []byte, meta *api.IssueTemplate) (*
 		meta.Ref = tmpl.Ref
 	}
 
-	return tmpl, nil
+	return tmpl, nil, nil
 }
 
 // IssueTemplatesFromDefaultBranch checks for issue templates in the repo's default branch
-func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
+func (ctx *Context) IssueTemplatesFromDefaultBranch() ([]api.IssueTemplate, map[string][]string) {
 	var issueTemplates []api.IssueTemplate
+	validationErrs := make(map[string][]string)
 
 	if ctx.Repo.Repository.IsEmpty {
-		return issueTemplates
+		return issueTemplates, nil
 	}
 
 	if ctx.Repo.Commit == nil {
 		var err error
 		ctx.Repo.Commit, err = ctx.Repo.GitRepo.GetBranchCommit(ctx.Repo.Repository.DefaultBranch)
 		if err != nil {
-			return issueTemplates
+			return issueTemplates, nil
 		}
 	}
 
@@ -1076,7 +1090,7 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
 		}
 		entries, err := tree.ListEntries()
 		if err != nil {
-			return issueTemplates
+			return issueTemplates, nil
 		}
 		for _, entry := range entries {
 			if strings.HasSuffix(entry.Name(), ".md") {
@@ -1111,6 +1125,8 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
 				it.FileName = entry.Name()
 				if it.Valid() {
 					issueTemplates = append(issueTemplates, it)
+				} else {
+					fmt.Printf("%#v\n", it)
 				}
 			} else if strings.HasSuffix(entry.Name(), ".yaml") || strings.HasSuffix(entry.Name(), ".yml") {
 				if entry.Blob().Size() >= setting.UI.MaxDisplayFileSize {
@@ -1137,9 +1153,14 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
 
 				var it api.IssueTemplate
 				it.FileName = path.Base(entry.Name())
-				_, err = ExtractTemplateFromYaml(templateContent, &it)
+
+				var tmplValidationErrs []string
+				_, tmplValidationErrs, err = ExtractTemplateFromYaml(templateContent, &it)
 				if err != nil {
 					log.Debug("ExtractTemplateFromYaml: %v", err)
+					if tmplValidationErrs != nil {
+						validationErrs[path.Base(entry.Name())] = tmplValidationErrs
+					}
 					continue
 				}
 				if it.Valid() {
@@ -1148,8 +1169,8 @@ func (ctx *Context) IssueTemplatesFromDefaultBranch() []api.IssueTemplate {
 			}
 		}
 		if len(issueTemplates) > 0 {
-			return issueTemplates
+			return issueTemplates, validationErrs
 		}
 	}
-	return issueTemplates
+	return issueTemplates, validationErrs
 }
diff --git a/modules/structs/issue_form.go b/modules/structs/issue_form.go
@@ -4,7 +4,10 @@
 
 package structs
 
-import "strings"
+import (
+	"fmt"
+	"strings"
+)
 
 type FormField struct {
 	Type        string                 `yaml:"type"`
@@ -26,18 +29,72 @@ type IssueFormTemplate struct {
 	FileName  string      `yaml:"-"`
 }
 
-// Valid checks whether an IssueFormTemplate is considered valid, e.g. at least name and about
-func (it IssueFormTemplate) Valid() bool {
-	if strings.TrimSpace(it.Name) == "" || strings.TrimSpace(it.About) == "" {
-		return false
+// Valid checks whether an IssueFormTemplate is considered valid, e.g. at least name and about, and labels for all fields
+func (it IssueFormTemplate) Valid() []string {
+	// TODO: Localize error messages
+	// TODO: Add a bunch more validations
+	var errs []string
+
+	if strings.TrimSpace(it.Name) == "" {
+		errs = append(errs, "the 'name' field of the issue template are required")
+	}
+	if strings.TrimSpace(it.About) == "" {
+		errs = append(errs, "the 'about' field of the issue template are required")
 	}
 
-	for _, field := range it.Fields {
-		if strings.TrimSpace(field.ID) == "" {
-			// TODO: add IDs should be optional, maybe generate slug from label? or use numberic id
-			return false
+	// Make sure all non-markdown fields have labels
+	for fieldIdx, field := range it.Fields {
+		// Make checker functions
+		checkStringAttr := func(attrName string) {
+			attr := field.Attributes[attrName]
+			if attr == nil || strings.TrimSpace(attr.(string)) == "" {
+				errs = append(errs, fmt.Sprintf(
+					"(field #%d '%s'): the '%s' attribute is required for fields with type %s",
+					fieldIdx+1, field.ID, attrName, field.Type,
+				))
+			}
+		}
+		checkOptionsStringAttr := func(optionIdx int, option map[interface{}]interface{}, attrName string) {
+			attr := option[attrName]
+			if attr == nil || strings.TrimSpace(attr.(string)) == "" {
+				errs = append(errs, fmt.Sprintf(
+					"(field #%d '%s', option #%d): the '%s' field is required for options",
+					fieldIdx+1, field.ID, optionIdx, attrName,
+				))
+			}
+		}
+		checkListAttr := func(attrName string, itemChecker func(int, map[interface{}]interface{})) {
+			attr := field.Attributes[attrName]
+			if attr == nil {
+				errs = append(errs, fmt.Sprintf(
+					"(field #%d '%s'): the '%s' attribute is required for fields with type %s",
+					fieldIdx+1, field.ID, attrName, field.Type,
+				))
+			} else {
+				for i, item := range attr.([]interface{}) {
+					itemChecker(i, item.(map[interface{}]interface{}))
+				}
+			}
+		}
+
+		// Make sure each field has its attributes
+		switch field.Type {
+		case "markdown":
+			checkStringAttr("value")
+		case "textarea", "input", "dropdown":
+			checkStringAttr("label")
+		case "checkboxes":
+			checkStringAttr("label")
+			checkListAttr("options", func(i int, item map[interface{}]interface{}) {
+				checkOptionsStringAttr(i, item, "label")
+			})
+		default:
+			errs = append(errs, fmt.Sprintf(
+				"(field #%d '%s'): unknown type '%s'",
+				fieldIdx+1, field.ID, field.Type,
+			))
 		}
 	}
 
-	return true
+	return errs
 }
diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go
@@ -1080,5 +1080,6 @@ func GetIssueTemplates(ctx *context.APIContext) {
 	//   "200":
 	//     "$ref": "#/responses/IssueTemplates"
 
-	ctx.JSON(http.StatusOK, ctx.IssueTemplatesFromDefaultBranch())
+	issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
+	ctx.JSON(http.StatusOK, issueTemplates)
 }
diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go
@@ -65,6 +65,7 @@ const (
 
 	issueTemplateKey      = "IssueTemplate"
 	issueFormTemplateKey  = "IssueFormTemplate"
+	issueFormErrorsKey    = "IssueTemplateErrors"
 	issueTemplateTitleKey = "IssueTemplateTitle"
 )
 
@@ -408,7 +409,8 @@ func Issues(ctx *context.Context) {
 		}
 		ctx.Data["Title"] = ctx.Tr("repo.issues")
 		ctx.Data["PageIsIssueList"] = true
-		ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
+		issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
+		ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
 	}
 
 	issues(ctx, ctx.FormInt64("milestone"), ctx.FormInt64("project"), util.OptionalBoolOf(isPullList))
@@ -751,7 +753,9 @@ func getFileContentFromDefaultBranch(repo *context.Repository, filename string)
 	return string(bytes), true
 }
 
-func getTemplate(repo *context.Repository, template string, possibleDirs, possibleFiles []string) (*api.IssueTemplate, string, *api.IssueFormTemplate, error) {
+func getTemplate(repo *context.Repository, template string, possibleDirs, possibleFiles []string) (*api.IssueTemplate, string, *api.IssueFormTemplate, map[string][]string, error) {
+	validationErrs := make(map[string][]string)
+
 	// Add `possibleFiles` and each `{possibleDirs}/{template}` to `templateCandidates`
 	templateCandidates := make([]string, 0, len(possibleFiles))
 	if template != "" {
@@ -772,36 +776,42 @@ func getTemplate(repo *context.Repository, template string, possibleDirs, possib
 
 			if strings.HasSuffix(filename, ".md") {
 				// Parse markdown template
-				templateBody, err = markdown.ExtractMetadata(templateContent, meta)
+				templateBody, err = markdown.ExtractMetadata(templateContent, &meta)
 			} else if strings.HasSuffix(filename, ".yaml") || strings.HasSuffix(filename, ".yml") {
 				// Parse yaml (form) template
-				formTemplateBody, err = context.ExtractTemplateFromYaml([]byte(templateContent), &meta)
-				formTemplateBody.FileName = path.Base(filename)
+				var tmplValidationErrs []string
+				formTemplateBody, tmplValidationErrs, err = context.ExtractTemplateFromYaml([]byte(templateContent), &meta)
+				if err == nil {
+					formTemplateBody.FileName = path.Base(filename)
+				} else if tmplValidationErrs != nil {
+					validationErrs[path.Base(filename)] = tmplValidationErrs
+				}
 			} else {
 				err = errors.New("invalid template type")
 			}
 			if err != nil {
 				log.Debug("could not extract metadata from %s [%s]: %v", filename, repo.Repository.FullName(), err)
-				templateBody = templateContent
-				err = nil
 			}
 
-			return &meta, templateBody, formTemplateBody, err
+			return &meta, templateBody, formTemplateBody, validationErrs, err
 		}
 	}
 
-	return nil, "", nil, errors.New("no template found")
+	return nil, "", nil, validationErrs, errors.New("no template found")
 }
 
 func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleDirs, possibleFiles []string) {
-	templateMeta, templateBody, formTemplateBody, err := getTemplate(ctx.Repo, ctx.FormString("template"), possibleDirs, possibleFiles)
+	templateMeta, templateBody, formTemplateBody, validationErrs, err := getTemplate(ctx.Repo, ctx.FormString("template"), possibleDirs, possibleFiles)
 	if err != nil {
 		return
 	}
 
 	if formTemplateBody != nil {
 		ctx.Data[issueFormTemplateKey] = formTemplateBody
 	}
+	if validationErrs != nil && len(validationErrs) > 0 {
+		ctx.Data[issueFormErrorsKey] = validationErrs
+	}
 
 	ctx.Data[issueTemplateTitleKey] = templateMeta.Title
 	ctx.Data[ctxDataKey] = templateBody
@@ -836,7 +846,8 @@ func setTemplateIfExists(ctx *context.Context, ctxDataKey string, possibleDirs,
 func NewIssue(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("repo.issues.new")
 	ctx.Data["PageIsIssueList"] = true
-	ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
+	issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
+	ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
 	ctx.Data["RequireTribute"] = true
 	ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
 	title := ctx.FormString("title")
@@ -893,7 +904,10 @@ func NewIssueChooseTemplate(ctx *context.Context) {
 	ctx.Data["Title"] = ctx.Tr("repo.issues.new")
 	ctx.Data["PageIsIssueList"] = true
 
-	issueTemplates := ctx.IssueTemplatesFromDefaultBranch()
+	issueTemplates, validationErrs := ctx.IssueTemplatesFromDefaultBranch()
+	if validationErrs != nil && len(validationErrs) > 0 {
+		ctx.Data[issueFormErrorsKey] = validationErrs
+	}
 	ctx.Data["IssueTemplates"] = issueTemplates
 
 	if len(issueTemplates) == 0 {
@@ -1039,7 +1053,7 @@ func renderIssueFormValues(ctx *context.Context, form *url.Values) (string, erro
 	}
 
 	// Fetch template
-	_, _, formTemplateBody, err := getTemplate(
+	_, _, formTemplateBody, _, err := getTemplate(
 		ctx.Repo,
 		form.Get("form-type"),
 		context.IssueTemplateDirCandidates,
@@ -1094,7 +1108,8 @@ func NewIssuePost(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.CreateIssueForm)
 	ctx.Data["Title"] = ctx.Tr("repo.issues.new")
 	ctx.Data["PageIsIssueList"] = true
-	ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
+	issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
+	ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
 	ctx.Data["PullRequestWorkInProgressPrefixes"] = setting.Repository.PullRequest.WorkInProgressPrefixes
 	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled
 	upload.AddUploadContext(ctx, "comment")
@@ -1287,7 +1302,8 @@ func ViewIssue(ctx *context.Context) {
 			return
 		}
 		ctx.Data["PageIsIssueList"] = true
-		ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
+		issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
+		ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
 	}
 
 	if issue.IsPull && !ctx.Repo.CanRead(unit.TypeIssues) {
diff --git a/routers/web/repo/milestone.go b/routers/web/repo/milestone.go
@@ -290,7 +290,8 @@ func MilestoneIssuesAndPulls(ctx *context.Context) {
 	ctx.Data["Milestone"] = milestone
 
 	issues(ctx, milestoneID, 0, util.OptionalBoolNone)
-	ctx.Data["NewIssueChooseTemplate"] = len(ctx.IssueTemplatesFromDefaultBranch()) > 0
+	issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()
+	ctx.Data["NewIssueChooseTemplate"] = len(issueTemplates) > 0
 
 	ctx.Data["CanWriteIssues"] = ctx.Repo.CanWriteIssuesOrPulls(false)
 	ctx.Data["CanWritePulls"] = ctx.Repo.CanWriteIssuesOrPulls(true)
diff --git a/templates/repo/issue/choose.tmpl b/templates/repo/issue/choose.tmpl
@@ -30,6 +30,20 @@
 				</div>
 			</div>
 		</div>
+		{{- if .IssueTemplateErrors}}
+			<div class="ui warning message">
+				<div class="text left">
+					<div>The following issue templates have errors:</div>
+					<ul>
+            {{range $filename, $errors := .IssueTemplateErrors}}
+                {{range $errors}}
+									<li>{{$filename}}: {{.}}</li>
+                {{end}}
+            {{end}}
+					</ul>
+				</div>
+			</div>
+		{{end}}
 	</div>
 </div>
 {{template "base/footer" .}}

Original file line number	Diff line number	Diff line change
`@@ -1080,5 +1080,6 @@ func GetIssueTemplates(ctx *context.APIContext) {`
`1080`	`1080`	`// "200":`
`1081`	`1081`	`// "$ref": "#/responses/IssueTemplates"`
`1082`	`1082`
`1083`		`- ctx.JSON(http.StatusOK, ctx.IssueTemplatesFromDefaultBranch())`
	`1083`	`+ issueTemplates, _ := ctx.IssueTemplatesFromDefaultBranch()`
	`1084`	`+ ctx.JSON(http.StatusOK, issueTemplates)`
`1084`	`1085`	`}`