Skip to content

Commit cce4922

Browse files
jodyheavenerjodyheavener
committed
Updates from review feedback
1 parent 119051e commit cce4922

File tree

5 files changed

+107
-86
lines changed

5 files changed

+107
-86
lines changed

‎script/application.go

+75-46
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,10 @@ type Project struct {
1414
Name string `json:"name"`
1515
Description string `json:"description"`
1616
Contributors int `json:"contributors"`
17-
HomeUrl string `json:"home_url"`
18-
RepoUrl string `json:"repo_url,omitempty"`
17+
HomeURL string `json:"home_url"`
18+
RepoURL string `json:"repo_url,omitempty"`
1919
LicenseType string `json:"license_type,omitempty"`
20-
LicenseUrl string `json:"license_url,omitempty"`
20+
LicenseURL string `json:"license_url,omitempty"`
2121
IsEvent bool `json:"is_event"`
2222
IsTeam bool `json:"is_team"`
2323
}
@@ -26,7 +26,7 @@ type Applicant struct {
2626
Name string `json:"name"`
2727
Email string `json:"email"`
2828
Role string `json:"role"`
29-
Id int64 `json:"id"`
29+
ID int64 `json:"id"`
3030
}
3131

3232
type Application struct {
@@ -43,7 +43,7 @@ type Application struct {
4343
CreatedAt time.Time `json:"created_at"`
4444
}
4545

46-
func (a *Application) Parse(issue github.Issue) {
46+
func (a *Application) Parse(issue *github.Issue) {
4747
a.validator = Validator{}
4848

4949
if strings.Contains(*issue.Title, "[project name]") {
@@ -63,32 +63,32 @@ func (a *Application) Parse(issue github.Issue) {
6363

6464
a.CreatedAt = issue.CreatedAt.Time
6565
a.IssueNumber = *issue.Number
66-
a.Account = a.stringSection("Account URL", IsPresent, ParseAccountUrl)
67-
a.boolSection("Non-commercial confirmation", IsPresent, ParseCheckbox, IsChecked)
66+
a.Account = a.stringSection("Account URL", true, ParseAccountURL)
67+
a.boolSection("Non-commercial confirmation", true, ParseCheckbox, IsChecked)
6868

69-
a.Project.IsTeam = a.boolSection("Team application", ParseCheckbox)
70-
a.Project.IsEvent = a.boolSection("Event application", ParseCheckbox)
69+
a.Project.IsTeam = a.boolSection("Team application", false, ParseCheckbox)
70+
a.Project.IsEvent = a.boolSection("Event application", false, ParseCheckbox)
7171

7272
isProject := !a.Project.IsTeam && !a.Project.IsEvent
7373

74-
a.Project.Name = a.stringSection("Project name", IsPresent, ParsePlainString)
75-
a.Project.Description = a.stringSection("Short description", IsPresent, ParsePlainString)
76-
a.Project.Contributors = a.intSection("Number of team members/core contributors", IsPresent, ParsePlainString)
77-
a.Project.HomeUrl = a.stringSection("Homepage URL", IsPresent, IsUrl)
78-
a.Project.RepoUrl = a.stringSection("Repository URL", IsUrl)
79-
a.Project.LicenseType = a.stringSection("License type", When(isProject, IsPresent), ParsePlainString)
80-
a.Project.LicenseUrl = a.stringSection("License URL", When(isProject, IsPresent), IsUrl)
81-
a.boolSection("Age confirmation", When(isProject, IsPresent), ParseCheckbox, When(isProject, IsChecked))
74+
a.Project.Name = a.stringSection("Project name", true, ParsePlainString)
75+
a.Project.Description = a.stringSection("Short description", true, ParsePlainString)
76+
a.Project.Contributors = a.intSection("Number of team members/core contributors", true, ParsePlainString)
77+
a.Project.HomeURL = a.stringSection("Homepage URL", true, IsURL)
78+
a.Project.RepoURL = a.stringSection("Repository URL", false, IsURL)
79+
a.Project.LicenseType = a.stringSection("License type", isProject, ParsePlainString)
80+
a.Project.LicenseURL = a.stringSection("License URL", isProject, IsURL)
81+
a.boolSection("Age confirmation", isProject, ParseCheckbox, When(isProject, IsChecked))
8282

83-
a.Applicant.Name = a.stringSection("Name", IsPresent, ParsePlainString)
84-
a.Applicant.Email = a.stringSection("Email", IsPresent, IsEmail)
85-
a.Applicant.Role = a.stringSection("Project role", IsPresent)
86-
a.Applicant.Id = *issue.User.ID
83+
a.Applicant.Name = a.stringSection("Name", true, ParsePlainString)
84+
a.Applicant.Email = a.stringSection("Email", true, IsEmail)
85+
a.Applicant.Role = a.stringSection("Project role", true)
86+
a.Applicant.ID = *issue.User.ID
8787

88-
a.stringSection("Profile or website", IsUrl)
89-
a.stringSection("Additional comments")
88+
a.stringSection("Profile or website", false, IsURL)
89+
a.stringSection("Additional comments", false)
9090

91-
a.CanContact = a.boolSection("Can we contact you?", ParseCheckbox)
91+
a.CanContact = a.boolSection("Can we contact you?", false, ParseCheckbox)
9292

9393
if isTestingIssue() {
9494
debugMessage("Application data:", a.GetData())
@@ -112,44 +112,73 @@ func (a *Application) GetData() string {
112112
return string(data)
113113
}
114114

115+
// Take the Markdown-format body of an issue and break it down by section header
116+
// and the content directly below it. We can reasonably expect the correct format
117+
// here if someone files an issue using the application template, but it will also
118+
// gracefully handle when this format is not present. Note that this will only
119+
// create an entry when there is content to be added; in other words, a section
120+
// header without any content will not be added.
115121
func (a *Application) extractSections(body string) map[string]string {
116122
sections := make(map[string]string)
117123

118124
lines := strings.Split(body, "\n")
119125
var currentHeader string
120126
contentBuilder := strings.Builder{}
121127

128+
// For each line of the body content, it can either be a section's
129+
// header or the content associated with that section's header.
122130
for _, line := range lines {
123131
trimmedLine := strings.TrimSpace(line)
124-
if strings.HasPrefix(trimmedLine, "### ") {
125-
if currentHeader != "" {
126-
sections[currentHeader] = strings.TrimSpace(contentBuilder.String())
127-
contentBuilder.Reset()
132+
133+
// If we're in a section and the content doesn't start with
134+
// a header marker, append it to our content builder
135+
if !strings.HasPrefix(trimmedLine, "### ") {
136+
if currentHeader == "" {
137+
continue
128138
}
129-
currentHeader = strings.TrimSpace(trimmedLine[4:])
130-
} else if currentHeader != "" {
139+
131140
contentBuilder.WriteString(line + "\n")
141+
continue
142+
}
143+
144+
// The content has a header marker, so create a new
145+
// section entry and prepare the content builder
146+
if currentHeader != "" && contentBuilder.Len() > 0 {
147+
sections[currentHeader] = strings.TrimSpace(contentBuilder.String())
148+
contentBuilder.Reset()
132149
}
150+
151+
currentHeader = strings.TrimSpace(trimmedLine[4:])
133152
}
134153

135-
if currentHeader != "" {
154+
// Once the loop has completed check if there's a
155+
// trailing section needing to be closed
156+
if currentHeader != "" && contentBuilder.Len() > 0 {
136157
sections[currentHeader] = strings.TrimSpace(contentBuilder.String())
137158
}
138159

139160
return sections
140161
}
141162

142-
func (a *Application) stringSection(sectionName string, callbacks ...ValidatorCallback) string {
163+
func (a *Application) stringSection(sectionName string, required bool, callbacks ...ValidatorCallback) string {
143164
value, exists := a.sections[sectionName]
144-
145-
if !exists {
146-
a.validator.AddError(sectionName, value, "was not completed for application")
165+
_, value, _ = ParseInput(value)
166+
167+
// If the section is required, apply the presence validator if the entry
168+
// exists, early fail validation if it doesn't exist. If the section is
169+
// not required and there is no content to work with, don't try to run
170+
// additional validations.
171+
if required {
172+
if exists {
173+
callbacks = append([]ValidatorCallback{IsPresent}, callbacks...)
174+
} else {
175+
a.validator.AddError(sectionName, value, "was not completed for application")
176+
return value
177+
}
178+
} else if !exists || value == "" {
147179
return value
148180
}
149181

150-
// everything gets passed through ParseInput first
151-
callbacks = append([]ValidatorCallback{ParseInput}, callbacks...)
152-
153182
for _, callback := range callbacks {
154183
pass, newValue, message := callback(value)
155184
value = newValue
@@ -163,11 +192,11 @@ func (a *Application) stringSection(sectionName string, callbacks ...ValidatorCa
163192
return value
164193
}
165194

166-
func (a *Application) intSection(sectionName string, callbacks ...ValidatorCallback) int {
167-
value := a.stringSection(sectionName, callbacks...)
195+
func (a *Application) intSection(sectionName string, required bool, callbacks ...ValidatorCallback) int {
196+
value := a.stringSection(sectionName, required, callbacks...)
168197

169-
// don't bother proceeding if there's already an error parsing the string
170-
if a.validator.HasError(sectionName) {
198+
// Don't bother proceeding if there's already an error parsing the string
199+
if a.validator.HasError(sectionName) || value == "" {
171200
return 0
172201
}
173202

@@ -180,11 +209,11 @@ func (a *Application) intSection(sectionName string, callbacks ...ValidatorCallb
180209
return number
181210
}
182211

183-
func (a *Application) boolSection(sectionName string, callbacks ...ValidatorCallback) bool {
184-
value := a.stringSection(sectionName, callbacks...)
212+
func (a *Application) boolSection(sectionName string, required bool, callbacks ...ValidatorCallback) bool {
213+
value := a.stringSection(sectionName, required, callbacks...)
185214

186-
// don't bother proceeding if there's already an error parsing the string
187-
if a.validator.HasError(sectionName) {
215+
// Don't bother proceeding if there's already an error parsing the string
216+
if a.validator.HasError(sectionName) || value == "" {
188217
return false
189218
}
190219

‎script/application_test.go

+5-11
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ func errMustBeChecked(sectionTitle string) error {
2222
return fmt.Errorf("%s: must be checked", sectionTitle)
2323
}
2424

25-
func errInvalidAccountUrl(sectionTitle string) error {
25+
func errInvalidAccountURL(sectionTitle string) error {
2626
return fmt.Errorf("%s: is invalid 1Password account URL", sectionTitle)
2727
}
2828

@@ -34,7 +34,7 @@ func errParsingNumber(sectionTitle string) error {
3434
return fmt.Errorf("%s: could not be parsed into a number", sectionTitle)
3535
}
3636

37-
func errInvalidUrl(sectionTitle string) error {
37+
func errInvalidURL(sectionTitle string) error {
3838
return fmt.Errorf("%s: is an invalid URL", sectionTitle)
3939
}
4040

@@ -77,22 +77,16 @@ func TestApplication(t *testing.T) {
7777
expectedProblems: []error{
7878
errIncomplete("Account URL"),
7979
errIncomplete("Non-commercial confirmation"),
80-
errIncomplete("Team application"),
81-
errIncomplete("Event application"),
8280
errIncomplete("Project name"),
8381
errIncomplete("Short description"),
8482
errIncomplete("Number of team members/core contributors"),
8583
errIncomplete("Homepage URL"),
86-
errIncomplete("Repository URL"),
8784
errIncomplete("License type"),
8885
errIncomplete("License URL"),
8986
errIncomplete("Age confirmation"),
9087
errIncomplete("Name"),
9188
errIncomplete("Email"),
9289
errIncomplete("Project role"),
93-
errIncomplete("Profile or website"),
94-
errIncomplete("Additional comments"),
95-
errIncomplete("Can we contact you?"),
9690
},
9791
},
9892
{
@@ -119,11 +113,11 @@ func TestApplication(t *testing.T) {
119113
expectedValid: false,
120114
expectedProblems: []error{
121115
errNoProjectName("Application title"),
122-
errInvalidAccountUrl("Account URL"),
116+
errInvalidAccountURL("Account URL"),
123117
errMustBeChecked("Non-commercial confirmation"),
124118
errContainsEmoji("Project name"),
125119
errParsingNumber("Number of team members/core contributors"),
126-
errInvalidUrl("Homepage URL"),
120+
errInvalidURL("Homepage URL"),
127121
},
128122
},
129123
}
@@ -138,7 +132,7 @@ func TestApplication(t *testing.T) {
138132
testIssueName = fmt.Sprintf("invalid-%s", tt.name)
139133
}
140134

141-
application.Parse(*getTestIssue())
135+
application.Parse(getTestIssue())
142136

143137
if application.IsValid() != tt.expectedValid {
144138
if tt.expectedValid {

‎script/reviewer.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ func (r *Reviewer) Review() {
1919
r.printErrorAndExit(err)
2020
}
2121

22-
r.application.Parse(*r.gitHub.Issue)
22+
r.application.Parse(r.gitHub.Issue)
2323

2424
if isTestingIssue() {
2525
if r.application.IsValid() {

‎script/validator.go

+16-24
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,14 @@ import (
88
"regexp"
99
"strconv"
1010
"strings"
11+
"unicode"
1112

1213
"github.com/PuerkitoBio/goquery"
1314
"github.com/russross/blackfriday/v2"
1415
)
1516

1617
var (
17-
accountUrlRegex = regexp.MustCompile(`^(https?:\/\/)?[\w.-]+\.1password\.(com|ca|eu)\/?$`)
18+
accountURLRegex = regexp.MustCompile(`^(https?:\/\/)?[\w.-]+\.1password\.(com|ca|eu)\/?$`)
1819
urlRegex = regexp.MustCompile(`https?://[^\s]+`)
1920
emailRegex = regexp.MustCompile(`[a-zA-Z0-9._%+\-]+@[a-zA-Z0-9.\-]+\.[a-zA-Z]{2,}`)
2021
emojiRegex = regexp.MustCompile(`[\x{1F600}-\x{1F64F}\x{1F300}-\x{1F5FF}\x{1F680}-\x{1F6FF}\x{1F700}-\x{1F77F}\x{1F780}-\x{1F7FF}\x{1F800}-\x{1F8FF}\x{1F900}-\x{1F9FF}\x{1FA00}-\x{1FA6F}\x{1FA70}-\x{1FAFF}\x{1FB00}-\x{1FBFF}]+`)
@@ -95,21 +96,20 @@ func ParsePlainString(value string) (bool, string, string) {
9596
return true, value, ""
9697
}
9798

98-
func ParseAccountUrl(value string) (bool, string, string) {
99-
if accountUrlRegex.Match([]byte(value)) {
100-
if !strings.HasPrefix(value, "http://") && !strings.HasPrefix(value, "https://") {
101-
value = "https://" + value
102-
}
103-
104-
u, err := url.Parse(value)
105-
if err != nil {
106-
return false, value, err.Error()
107-
}
108-
109-
return true, u.Hostname(), ""
110-
} else {
99+
func ParseAccountURL(value string) (bool, string, string) {
100+
if !accountURLRegex.Match([]byte(value)) {
111101
return false, value, "is invalid 1Password account URL"
112102
}
103+
if !strings.HasPrefix(value, "http://") && !strings.HasPrefix(value, "https://") {
104+
value = "https://" + value
105+
}
106+
107+
u, err := url.Parse(value)
108+
if err != nil {
109+
return false, value, err.Error()
110+
}
111+
112+
return true, u.Hostname(), ""
113113
}
114114

115115
func ParseCheckbox(value string) (bool, string, string) {
@@ -128,7 +128,7 @@ func ParseNumber(value string) (bool, int, string) {
128128
cleanedString := ""
129129

130130
for _, char := range value {
131-
if char >= '0' && char <= '9' {
131+
if unicode.IsDigit(char) {
132132
cleanedString += string(char)
133133
}
134134
}
@@ -161,22 +161,14 @@ func IsPresent(value string) (bool, string, string) {
161161
}
162162

163163
func IsEmail(value string) (bool, string, string) {
164-
if value == "" {
165-
return true, value, ""
166-
}
167-
168164
if _, err := mail.ParseAddress(value); err == nil {
169165
return true, value, ""
170166
}
171167

172168
return false, value, "is an invalid email"
173169
}
174170

175-
func IsUrl(value string) (bool, string, string) {
176-
if value == "" {
177-
return true, value, ""
178-
}
179-
171+
func IsURL(value string) (bool, string, string) {
180172
parsedURL, err := url.ParseRequestURI(value)
181173
if err != nil {
182174
return false, value, "is an invalid URL"

‎script/validator_test.go

+10-4
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ func TestParseAccountUrl(t *testing.T) {
4848
testCases := []validationTestCase{
4949
{"myteam.1password.com", true, "myteam.1password.com", ""},
5050
}
51-
runValidationTests(t, testCases, ParseAccountUrl, "ParseAccountUrl")
51+
runValidationTests(t, testCases, ParseAccountURL, "ParseAccountUrl")
5252
}
5353

5454
func TestParseCheckbox(t *testing.T) {
@@ -123,16 +123,22 @@ func TestIsPresent(t *testing.T) {
123123

124124
func TestIsEmail(t *testing.T) {
125125
testCases := []validationTestCase{
126-
{"", true, "", ""},
126+
{"[email protected]", true, "[email protected]", ""},
127+
{"@example.com", false, "@example.com", ""},
128+
{"", false, "", ""},
127129
}
128130
runValidationTests(t, testCases, IsEmail, "IsEmail")
129131
}
130132

131133
func TestIsUrl(t *testing.T) {
132134
testCases := []validationTestCase{
133-
{"", true, "", ""},
135+
{"https://www.com", true, "https://www.com", ""},
136+
{"ftp://www.com", false, "ftp://www.com", ""},
137+
{"example.com", false, "example.com", ""},
138+
{"foo", false, "foo", ""},
139+
{"", false, "", ""},
134140
}
135-
runValidationTests(t, testCases, IsUrl, "IsUrl")
141+
runValidationTests(t, testCases, IsURL, "IsUrl")
136142
}
137143

138144
func TestIsChecked(t *testing.T) {

0 commit comments

Comments
 (0)