Skip to content

Commit

Permalink
Updates from review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@jodyheavener
jodyheavener committed Mar 26, 2024
1 parent 119051e commit cce4922
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 86 deletions.
121 changes: 75 additions & 46 deletions script/application.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ type Project struct {
Name string `json:"name"`
Description string `json:"description"`
Contributors int `json:"contributors"`
HomeUrl string `json:"home_url"`
RepoUrl string `json:"repo_url,omitempty"`
HomeURL string `json:"home_url"`
RepoURL string `json:"repo_url,omitempty"`
LicenseType string `json:"license_type,omitempty"`
LicenseUrl string `json:"license_url,omitempty"`
LicenseURL string `json:"license_url,omitempty"`
IsEvent bool `json:"is_event"`
IsTeam bool `json:"is_team"`
}
Expand All @@ -26,7 +26,7 @@ type Applicant struct {
Name string `json:"name"`
Email string `json:"email"`
Role string `json:"role"`
Id int64 `json:"id"`
ID int64 `json:"id"`
}

type Application struct {
Expand All @@ -43,7 +43,7 @@ type Application struct {
CreatedAt time.Time `json:"created_at"`
}

func (a *Application) Parse(issue github.Issue) {
func (a *Application) Parse(issue *github.Issue) {
a.validator = Validator{}

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

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

a.Project.IsTeam = a.boolSection("Team application", ParseCheckbox)
a.Project.IsEvent = a.boolSection("Event application", ParseCheckbox)
a.Project.IsTeam = a.boolSection("Team application", false, ParseCheckbox)
a.Project.IsEvent = a.boolSection("Event application", false, ParseCheckbox)

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

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

a.Applicant.Name = a.stringSection("Name", IsPresent, ParsePlainString)
a.Applicant.Email = a.stringSection("Email", IsPresent, IsEmail)
a.Applicant.Role = a.stringSection("Project role", IsPresent)
a.Applicant.Id = *issue.User.ID
a.Applicant.Name = a.stringSection("Name", true, ParsePlainString)
a.Applicant.Email = a.stringSection("Email", true, IsEmail)
a.Applicant.Role = a.stringSection("Project role", true)
a.Applicant.ID = *issue.User.ID

a.stringSection("Profile or website", IsUrl)
a.stringSection("Additional comments")
a.stringSection("Profile or website", false, IsURL)
a.stringSection("Additional comments", false)

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

if isTestingIssue() {
debugMessage("Application data:", a.GetData())
Expand All @@ -112,44 +112,73 @@ func (a *Application) GetData() string {
return string(data)
}

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

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

// For each line of the body content, it can either be a section's
// header or the content associated with that section's header.
for _, line := range lines {
trimmedLine := strings.TrimSpace(line)
if strings.HasPrefix(trimmedLine, "### ") {
if currentHeader != "" {
sections[currentHeader] = strings.TrimSpace(contentBuilder.String())
contentBuilder.Reset()

// If we're in a section and the content doesn't start with
// a header marker, append it to our content builder
if !strings.HasPrefix(trimmedLine, "### ") {
if currentHeader == "" {
continue
}
currentHeader = strings.TrimSpace(trimmedLine[4:])
} else if currentHeader != "" {

contentBuilder.WriteString(line + "\n")
continue
}

// The content has a header marker, so create a new
// section entry and prepare the content builder
if currentHeader != "" && contentBuilder.Len() > 0 {
sections[currentHeader] = strings.TrimSpace(contentBuilder.String())
contentBuilder.Reset()
}

currentHeader = strings.TrimSpace(trimmedLine[4:])
}

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

return sections
}

func (a *Application) stringSection(sectionName string, callbacks ...ValidatorCallback) string {
func (a *Application) stringSection(sectionName string, required bool, callbacks ...ValidatorCallback) string {
value, exists := a.sections[sectionName]

if !exists {
a.validator.AddError(sectionName, value, "was not completed for application")
_, value, _ = ParseInput(value)

// If the section is required, apply the presence validator if the entry
// exists, early fail validation if it doesn't exist. If the section is
// not required and there is no content to work with, don't try to run
// additional validations.
if required {
if exists {
callbacks = append([]ValidatorCallback{IsPresent}, callbacks...)
} else {
a.validator.AddError(sectionName, value, "was not completed for application")
return value
}
} else if !exists || value == "" {
return value
}

// everything gets passed through ParseInput first
callbacks = append([]ValidatorCallback{ParseInput}, callbacks...)

for _, callback := range callbacks {
pass, newValue, message := callback(value)
value = newValue
Expand All @@ -163,11 +192,11 @@ func (a *Application) stringSection(sectionName string, callbacks ...ValidatorCa
return value
}

func (a *Application) intSection(sectionName string, callbacks ...ValidatorCallback) int {
value := a.stringSection(sectionName, callbacks...)
func (a *Application) intSection(sectionName string, required bool, callbacks ...ValidatorCallback) int {
value := a.stringSection(sectionName, required, callbacks...)

// don't bother proceeding if there's already an error parsing the string
if a.validator.HasError(sectionName) {
// Don't bother proceeding if there's already an error parsing the string
if a.validator.HasError(sectionName) || value == "" {
return 0
}

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

func (a *Application) boolSection(sectionName string, callbacks ...ValidatorCallback) bool {
value := a.stringSection(sectionName, callbacks...)
func (a *Application) boolSection(sectionName string, required bool, callbacks ...ValidatorCallback) bool {
value := a.stringSection(sectionName, required, callbacks...)

// don't bother proceeding if there's already an error parsing the string
if a.validator.HasError(sectionName) {
// Don't bother proceeding if there's already an error parsing the string
if a.validator.HasError(sectionName) || value == "" {
return false
}

Expand Down
16 changes: 5 additions & 11 deletions script/application_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func errMustBeChecked(sectionTitle string) error {
return fmt.Errorf("%s: must be checked", sectionTitle)
}

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

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

func errInvalidUrl(sectionTitle string) error {
func errInvalidURL(sectionTitle string) error {
return fmt.Errorf("%s: is an invalid URL", sectionTitle)
}

Expand Down Expand Up @@ -77,22 +77,16 @@ func TestApplication(t *testing.T) {
expectedProblems: []error{
errIncomplete("Account URL"),
errIncomplete("Non-commercial confirmation"),
errIncomplete("Team application"),
errIncomplete("Event application"),
errIncomplete("Project name"),
errIncomplete("Short description"),
errIncomplete("Number of team members/core contributors"),
errIncomplete("Homepage URL"),
errIncomplete("Repository URL"),
errIncomplete("License type"),
errIncomplete("License URL"),
errIncomplete("Age confirmation"),
errIncomplete("Name"),
errIncomplete("Email"),
errIncomplete("Project role"),
errIncomplete("Profile or website"),
errIncomplete("Additional comments"),
errIncomplete("Can we contact you?"),
},
},
{
Expand All @@ -119,11 +113,11 @@ func TestApplication(t *testing.T) {
expectedValid: false,
expectedProblems: []error{
errNoProjectName("Application title"),
errInvalidAccountUrl("Account URL"),
errInvalidAccountURL("Account URL"),
errMustBeChecked("Non-commercial confirmation"),
errContainsEmoji("Project name"),
errParsingNumber("Number of team members/core contributors"),
errInvalidUrl("Homepage URL"),
errInvalidURL("Homepage URL"),
},
},
}
Expand All @@ -138,7 +132,7 @@ func TestApplication(t *testing.T) {
testIssueName = fmt.Sprintf("invalid-%s", tt.name)
}

application.Parse(*getTestIssue())
application.Parse(getTestIssue())

if application.IsValid() != tt.expectedValid {
if tt.expectedValid {
Expand Down
2 changes: 1 addition & 1 deletion script/reviewer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func (r *Reviewer) Review() {
r.printErrorAndExit(err)
}

r.application.Parse(*r.gitHub.Issue)
r.application.Parse(r.gitHub.Issue)

if isTestingIssue() {
if r.application.IsValid() {
Expand Down
40 changes: 16 additions & 24 deletions script/validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import (
"regexp"
"strconv"
"strings"
"unicode"

"github.com/PuerkitoBio/goquery"
"github.com/russross/blackfriday/v2"
)

var (
accountUrlRegex = regexp.MustCompile(`^(https?:\/\/)?[\w.-]+\.1password\.(com|ca|eu)\/?$`)
accountURLRegex = regexp.MustCompile(`^(https?:\/\/)?[\w.-]+\.1password\.(com|ca|eu)\/?$`)
urlRegex = regexp.MustCompile(`https?://[^\s]+`)
emailRegex = regexp.MustCompile(`[a-zA-Z0-9._%+\-]+@[a-zA-Z0-9.\-]+\.[a-zA-Z]{2,}`)
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}]+`)
Expand Down Expand Up @@ -95,21 +96,20 @@ func ParsePlainString(value string) (bool, string, string) {
return true, value, ""
}

func ParseAccountUrl(value string) (bool, string, string) {
if accountUrlRegex.Match([]byte(value)) {
if !strings.HasPrefix(value, "http://") && !strings.HasPrefix(value, "https://") {
value = "https://" + value
}

u, err := url.Parse(value)
if err != nil {
return false, value, err.Error()
}

return true, u.Hostname(), ""
} else {
func ParseAccountURL(value string) (bool, string, string) {
if !accountURLRegex.Match([]byte(value)) {
return false, value, "is invalid 1Password account URL"
}
if !strings.HasPrefix(value, "http://") && !strings.HasPrefix(value, "https://") {
value = "https://" + value
}

u, err := url.Parse(value)
if err != nil {
return false, value, err.Error()
}

return true, u.Hostname(), ""
}

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

for _, char := range value {
if char >= '0' && char <= '9' {
if unicode.IsDigit(char) {
cleanedString += string(char)
}
}
Expand Down Expand Up @@ -161,22 +161,14 @@ func IsPresent(value string) (bool, string, string) {
}

func IsEmail(value string) (bool, string, string) {
if value == "" {
return true, value, ""
}

if _, err := mail.ParseAddress(value); err == nil {
return true, value, ""
}

return false, value, "is an invalid email"
}

func IsUrl(value string) (bool, string, string) {
if value == "" {
return true, value, ""
}

func IsURL(value string) (bool, string, string) {
parsedURL, err := url.ParseRequestURI(value)
if err != nil {
return false, value, "is an invalid URL"
Expand Down
14 changes: 10 additions & 4 deletions script/validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestParseAccountUrl(t *testing.T) {
testCases := []validationTestCase{
{"myteam.1password.com", true, "myteam.1password.com", ""},
}
runValidationTests(t, testCases, ParseAccountUrl, "ParseAccountUrl")
runValidationTests(t, testCases, ParseAccountURL, "ParseAccountUrl")
}

func TestParseCheckbox(t *testing.T) {
Expand Down Expand Up @@ -123,16 +123,22 @@ func TestIsPresent(t *testing.T) {

func TestIsEmail(t *testing.T) {
testCases := []validationTestCase{
{"", true, "", ""},
{"test@example.com", true, "test@example.com", ""},
{"@example.com", false, "@example.com", ""},
{"", false, "", ""},
}
runValidationTests(t, testCases, IsEmail, "IsEmail")
}

func TestIsUrl(t *testing.T) {
testCases := []validationTestCase{
{"", true, "", ""},
{"https://www.com", true, "https://www.com", ""},
{"ftp://www.com", false, "ftp://www.com", ""},
{"example.com", false, "example.com", ""},
{"foo", false, "foo", ""},
{"", false, "", ""},
}
runValidationTests(t, testCases, IsUrl, "IsUrl")
runValidationTests(t, testCases, IsURL, "IsUrl")
}

func TestIsChecked(t *testing.T) {
Expand Down

0 comments on commit cce4922

Please sign in to comment.