From 15f1fdf58ce717e60171b7084066a7d5ec50c9d6 Mon Sep 17 00:00:00 2001 From: rgee0 Date: Tue, 16 Jan 2018 22:05:29 +0000 Subject: [PATCH] Moved out handleComment code into seperate functions. Added tests for new helper functions. Added edit title as an alias for set title & updated README Signed-off-by: rgee0 --- README.md | 4 + commentHandler.go | 318 ++++++++++++++++++++++------------------- commentHandler_test.go | 187 +++++++++++++++++++++--- main.go | 2 +- 4 files changed, 350 insertions(+), 161 deletions(-) diff --git a/README.md b/README.md index 3d51827..eaa18f3 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,10 @@ Let's say a user raised an issue with the title `I can't get it to work on my co ``` Derek set title: Question - does this work on Windows 10? ``` +or +``` +Derek edit title: Question - does this work on Windows 10? +``` * Triage and organise work through labels diff --git a/commentHandler.go b/commentHandler.go index 6eae9b9..4160026 100644 --- a/commentHandler.go +++ b/commentHandler.go @@ -1,6 +1,7 @@ package main import ( + "bytes" "context" "fmt" "os" @@ -12,8 +13,17 @@ import ( "github.com/google/go-github/github" ) -const open = "open" -const closed = "closed" +const openConstant string = "open" +const closedConstant string = "closed" +const closeConstant string = "close" +const reopenConstant string = "reopen" +const lockConstant string = "Lock" +const unlockConstant string = "Unlock" +const setTitleConstant string = "SetTitle" +const assignConstant string = "Assign" +const unassignConstant string = "Unassign" +const removeLabelConstant string = "RemoveLabel" +const addLabelConstant string = "AddLabel" func makeClient(installation int) (*github.Client, context.Context) { ctx := context.Background() @@ -39,183 +49,201 @@ func makeClient(installation int) (*github.Client, context.Context) { func handleComment(req types.IssueCommentOuter) { + var feedback string + var err error + command := parse(req.Comment.Body) switch command.Type { - case "AddLabel": - fmt.Printf("%s wants to %s of %s to issue %d\n", req.Comment.User.Login, command.Type, command.Value, req.Issue.Number) + case addLabelConstant, removeLabelConstant: - found := false - for _, label := range req.Issue.Labels { - if label.Name == command.Value { - found = true - break - } - } + feedback, err = manageLabel(req, command.Type, command.Value) + break - if found == true { - fmt.Println("Label already exists.") - return - } + case assignConstant, unassignConstant: - client, ctx := makeClient(req.Installation.ID) - _, res, err := client.Issues.AddLabelsToIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, []string{command.Value}) - if err != nil { - log.Fatalf("%s, limit: %d, remaining: %d", err, res.Limit, res.Remaining) - } + feedback, err = manageAssignment(req, command.Type, command.Value) + break - fmt.Println("Label added successfully or already existed.") + case closeConstant, reopenConstant: + feedback, err = manageState(req, command.Type) break - case "RemoveLabel": - - fmt.Printf("%s wants to %s of %s to issue %d \n", req.Comment.User.Login, command.Type, command.Value, req.Issue.Number) + case setTitleConstant: - found := false - for _, label := range req.Issue.Labels { - if label.Name == command.Value { - found = true - break - } - } + feedback, err = manageTitle(req, command.Type, command.Value) + break - if found == false { - fmt.Println("Label didn't exist on issue.") - return - } + case lockConstant, unlockConstant: - client, ctx := makeClient(req.Installation.ID) - _, err := client.Issues.RemoveLabelForIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, command.Value) - if err != nil { - log.Fatalln(err) - } - fmt.Println("Label removed successfully or already removed.") + feedback, err = manageLocking(req, command.Type) + break + default: + feedback = "Unable to work with comment: " + req.Comment.Body + err = nil break + } - case "Assign": + fmt.Print(feedback) - fmt.Printf("%s wants to %s user %s to issue %d\n", req.Comment.User.Login, command.Type, command.Value, req.Issue.Number) + if err != nil { + fmt.Println(err) + } +} - client, ctx := makeClient(req.Installation.ID) - assignee := command.Value - if assignee == "me" { - assignee = req.Comment.User.Login - } +func findLabel(currentLabels []types.IssueLabel, cmdLabel string) bool { - _, _, err := client.Issues.AddAssignees(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, []string{assignee}) - if err != nil { - log.Fatalln(err) + for _, label := range currentLabels { + if strings.EqualFold(label.Name, cmdLabel) { + return true } - fmt.Printf("%s assigned successfully or already assigned.\n", command.Value) + } + return false +} - break +func manageLabel(req types.IssueCommentOuter, cmdType string, labelValue string) (string, error) { - case "Unassign": + var buffer bytes.Buffer + labelAction := strings.Replace(strings.ToLower(cmdType), "label", "", 1) - fmt.Printf("%s wants to %s user %s from issue %d\n", req.Comment.User.Login, command.Type, command.Value, req.Issue.Number) + buffer.WriteString(fmt.Sprintf("%s wants to %s label of '%s' on issue #%d \n", req.Comment.User.Login, labelAction, labelValue, req.Issue.Number)) - client, ctx := makeClient(req.Installation.ID) + found := findLabel(req.Issue.Labels, labelValue) - assignee := command.Value - if assignee == "me" { - assignee = req.Comment.User.Login - } - _, _, err := client.Issues.RemoveAssignees(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, []string{assignee}) - if err != nil { - log.Fatalln(err) - } - fmt.Printf("%s unassigned successfully or already unassigned.\n", command.Value) + if !validAction(found, cmdType, addLabelConstant, removeLabelConstant) { + buffer.WriteString(fmt.Sprintf("Request to %s label of '%s' on issue #%d was unnecessary.", labelAction, labelValue, req.Issue.Number)) + return buffer.String(), nil + } - break + client, ctx := makeClient(req.Installation.ID) - case "close", "reopen": - fmt.Printf("%s wants to %s issue #%d\n", req.Comment.User.Login, command.Type, req.Issue.Number) + var err error - newState, validTransition := checkTransition(command.Type, req.Issue.State) + if cmdType == addLabelConstant { + _, _, err = client.Issues.AddLabelsToIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, []string{labelValue}) + } else { + _, err = client.Issues.RemoveLabelForIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, labelValue) + } - if !validTransition { - fmt.Printf("Request to %s issue #%d by %s was invalid.\n", command.Type, req.Issue.Number, req.Comment.User.Login) - return - } + if err != nil { + return buffer.String(), err + } - client, ctx := makeClient(req.Installation.ID) - input := &github.IssueRequest{State: &newState} + buffer.WriteString(fmt.Sprintf("Request to %s label of '%s' on issue #%d was successfully completed.", labelAction, labelValue, req.Issue.Number)) + return buffer.String(), nil +} - _, _, err := client.Issues.Edit(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, input) - if err != nil { - log.Fatalln(err) - } - fmt.Printf("Request to %s issue #%d by %s was successful.\n", command.Type, req.Issue.Number, req.Comment.User.Login) +func manageTitle(req types.IssueCommentOuter, cmdType string, cmdValue string) (string, error) { - break + var buffer bytes.Buffer + buffer.WriteString(fmt.Sprintf("%s wants to set the title of issue #%d\n", req.Comment.User.Login, req.Issue.Number)) - case "SetTitle": + newTitle := cmdValue - fmt.Printf("%s wants to set the title of issue #%d\n", req.Comment.User.Login, req.Issue.Number) + if newTitle == req.Issue.Title || len(newTitle) == 0 { + buffer.WriteString(fmt.Sprintf("Setting the title of #%d by %s was unsuccessful as the new title was empty or unchanged.\n", req.Issue.Number, req.Comment.User.Login)) + return buffer.String(), nil + } - newTitle := command.Value + client, ctx := makeClient(req.Installation.ID) - if newTitle == req.Issue.Title { - fmt.Printf("Setting the title of #%d by %s was unsuccessful as the new title was empty or unchanged.\n", req.Issue.Number, req.Comment.User.Login) - return - } + input := &github.IssueRequest{Title: &newTitle} - client, ctx := makeClient(req.Installation.ID) + _, _, err := client.Issues.Edit(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, input) + if err != nil { + return buffer.String(), err + } - input := &github.IssueRequest{Title: &newTitle} + buffer.WriteString(fmt.Sprintf("Request to set the title of issue #%d by %s was successful.\n", req.Issue.Number, req.Comment.User.Login)) + return buffer.String(), nil +} - _, _, err := client.Issues.Edit(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, input) - if err != nil { - log.Fatalln(err) - } +func manageAssignment(req types.IssueCommentOuter, cmdType string, cmdValue string) (string, error) { - fmt.Printf("Request to set the title of issue #%d by %s was successful.\n", req.Issue.Number, req.Comment.User.Login) + var buffer bytes.Buffer - break + buffer.WriteString(fmt.Sprintf("%s wants to %s user '%s' from issue #%d\n", req.Comment.User.Login, strings.ToLower(cmdType), cmdValue, req.Issue.Number)) - case "Lock": - fmt.Printf("%s wants to lock issue #%d\n", req.Comment.User.Login, req.Issue.Number) + client, ctx := makeClient(req.Installation.ID) - if req.Issue.Locked { - fmt.Printf("Issue #%d is already locked.\n", req.Issue.Number) - return - } + if cmdValue == "me" { + cmdValue = req.Comment.User.Login + } - client, ctx := makeClient(req.Installation.ID) + var err error - _, err := client.Issues.Lock(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number) - if err != nil { - log.Fatalln(err) - } - fmt.Printf("Request to lock issue #%d by %s was successful.\n", req.Issue.Number, req.Comment.User.Login) + if cmdType == unassignConstant { + _, _, err = client.Issues.RemoveAssignees(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, []string{cmdValue}) + } else { + _, _, err = client.Issues.AddAssignees(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, []string{cmdValue}) + } - break + if err != nil { + return buffer.String(), err + } - case "Unlock": - fmt.Printf("%s wants to unlock issue #%d\n", req.Comment.User.Login, req.Issue.Number) + buffer.WriteString(fmt.Sprintf("%s %sed successfully or already %sed.\n", cmdValue, strings.ToLower(cmdType), strings.ToLower(cmdType))) + return buffer.String(), nil +} - if !req.Issue.Locked { - fmt.Printf("Issue #%d is already unlocked\n", req.Issue.Number) - return - } +func manageState(req types.IssueCommentOuter, cmdType string) (string, error) { - client, ctx := makeClient(req.Installation.ID) + var buffer bytes.Buffer - _, err := client.Issues.Unlock(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number) - if err != nil { - log.Fatalln(err) - } - fmt.Printf("Request to unlock issue #%d by %s was successful.\n", req.Issue.Number, req.Comment.User.Login) + buffer.WriteString(fmt.Sprintf("%s wants to %s issue #%d\n", req.Comment.User.Login, cmdType, req.Issue.Number)) - break + newState, validTransition := checkTransition(cmdType, req.Issue.State) - default: - log.Fatalln("Unable to work with comment: " + req.Comment.Body) - break + if !validTransition { + buffer.WriteString(fmt.Sprintf("Request to %s issue #%d by %s was invalid.\n", cmdType, req.Issue.Number, req.Comment.User.Login)) + return buffer.String(), nil + } + + client, ctx := makeClient(req.Installation.ID) + input := &github.IssueRequest{State: &newState} + + _, _, err := client.Issues.Edit(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number, input) + if err != nil { + return buffer.String(), err + } + + buffer.WriteString(fmt.Sprintf("Request to %s issue #%d by %s was successful.\n", cmdType, req.Issue.Number, req.Comment.User.Login)) + return buffer.String(), nil + +} + +func manageLocking(req types.IssueCommentOuter, cmdType string) (string, error) { + + var buffer bytes.Buffer + + buffer.WriteString(fmt.Sprintf("%s wants to %s issue #%d\n", req.Comment.User.Login, strings.ToLower(cmdType), req.Issue.Number)) + + if !validAction(req.Issue.Locked, cmdType, lockConstant, unlockConstant) { + + buffer.WriteString(fmt.Sprintf("Issue #%d is already %sed\n", req.Issue.Number, strings.ToLower(cmdType))) + + return buffer.String(), nil + } + + client, ctx := makeClient(req.Installation.ID) + + var err error + + if cmdType == lockConstant { + _, err = client.Issues.Lock(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number) + } else { + _, err = client.Issues.Unlock(ctx, req.Repository.Owner.Login, req.Repository.Name, req.Issue.Number) + } + + if err != nil { + return buffer.String(), err } + + buffer.WriteString(fmt.Sprintf("Request to %s issue #%d by %s was successful.\n", strings.ToLower(cmdType), req.Issue.Number, req.Comment.User.Login)) + return buffer.String(), nil } func parse(body string) *types.CommentAction { @@ -223,15 +251,16 @@ func parse(body string) *types.CommentAction { commentAction := types.CommentAction{} commands := map[string]string{ - "Derek add label: ": "AddLabel", - "Derek remove label: ": "RemoveLabel", - "Derek assign: ": "Assign", - "Derek unassign: ": "Unassign", - "Derek close": "close", - "Derek reopen": "reopen", - "Derek set title: ": "SetTitle", - "Derek lock": "Lock", - "Derek unlock": "Unlock", + "Derek add label: ": addLabelConstant, + "Derek remove label: ": removeLabelConstant, + "Derek assign: ": assignConstant, + "Derek unassign: ": unassignConstant, + "Derek close": closeConstant, + "Derek reopen": reopenConstant, + "Derek set title: ": setTitleConstant, + "Derek edit title: ": setTitleConstant, + "Derek lock": lockConstant, + "Derek unlock": unlockConstant, } for trigger, commandType := range commands { @@ -253,18 +282,19 @@ func isValidCommand(body string, trigger string) bool { (body == trigger && !strings.HasSuffix(trigger, ": ")) } -func checkTransition(requestedAction string, currentState string) (string, bool) { +func validAction(running bool, requestedAction string, start string, stop string) bool { - desiredState := "" - validTransition := false + return !running && requestedAction == start || running && requestedAction == stop + +} + +func checkTransition(requestedAction string, currentState string) (string, bool) { - if requestedAction == "close" && currentState != closed { - desiredState = closed - validTransition = true - } else if requestedAction == "reopen" && currentState != open { - desiredState = open - validTransition = true + if requestedAction == closeConstant && currentState != closedConstant { + return closedConstant, true + } else if requestedAction == reopenConstant && currentState != openConstant { + return openConstant, true } - return desiredState, validTransition + return "", false } diff --git a/commentHandler_test.go b/commentHandler_test.go index ed3634a..667f364 100644 --- a/commentHandler_test.go +++ b/commentHandler_test.go @@ -2,6 +2,8 @@ package main import ( "testing" + + "github.com/alexellis/derek/types" ) var actionOptions = []struct { @@ -101,25 +103,25 @@ func Test_Parsing_Assignments(t *testing.T) { { title: "Assign to burt", body: "Derek assign: burt", - expectedType: "Assign", + expectedType: assignConstant, expectedVal: "burt", }, { title: "Unassign burt", body: "Derek unassign: burt", - expectedType: "Unassign", + expectedType: unassignConstant, expectedVal: "burt", }, { title: "Assign to me", body: "Derek assign: me", - expectedType: "Assign", + expectedType: assignConstant, expectedVal: "me", }, { title: "Unassign me", body: "Derek unassign: me", - expectedType: "Unassign", + expectedType: unassignConstant, expectedVal: "me", }, { @@ -158,7 +160,7 @@ func Test_Parsing_Titles(t *testing.T) { { title: "Set Title", body: "Derek set title: This is a really great Title!", - expectedType: "SetTitle", + expectedType: setTitleConstant, expectedVal: "This is a really great Title!", }, { @@ -176,7 +178,7 @@ func Test_Parsing_Titles(t *testing.T) { { title: "Empty Title (Double Space)", body: "Derek set title: ", - expectedType: "SetTitle", + expectedType: setTitleConstant, expectedVal: "", }, } @@ -203,30 +205,30 @@ func Test_assessState(t *testing.T) { }{ { title: "Currently Closed and trying to close", - requestedAction: "close", - currentState: "closed", + requestedAction: closeConstant, + currentState: closedConstant, expectedNewState: "", expectedBool: false, }, { title: "Currently Open and trying to reopen", - requestedAction: "reopen", - currentState: "open", + requestedAction: reopenConstant, + currentState: openConstant, expectedNewState: "", expectedBool: false, }, { title: "Currently Closed and trying to open", - requestedAction: "reopen", - currentState: "closed", - expectedNewState: "open", + requestedAction: reopenConstant, + currentState: closedConstant, + expectedNewState: openConstant, expectedBool: true, }, { title: "Currently Open and trying to close", - requestedAction: "close", - currentState: "open", - expectedNewState: "closed", + requestedAction: closeConstant, + currentState: openConstant, + expectedNewState: closedConstant, expectedBool: true, }, } @@ -242,3 +244,156 @@ func Test_assessState(t *testing.T) { }) } } + +func Test_validAction(t *testing.T) { + + var stateOptions = []struct { + title string + running bool + requestedAction string + start string + stop string + expectedBool bool + }{ + { + title: "Currently unlocked and trying to lock", + running: false, + requestedAction: lockConstant, + start: lockConstant, + stop: unlockConstant, + expectedBool: true, + }, + { + title: "Currently unlocked and trying to unlock", + running: false, + requestedAction: unlockConstant, + start: lockConstant, + stop: unlockConstant, + expectedBool: false, + }, + { + title: "Currently locked and trying to lock", + running: true, + requestedAction: lockConstant, + start: lockConstant, + stop: unlockConstant, + expectedBool: false, + }, + { + title: "Currently locked and trying to unlock", + running: true, + requestedAction: unlockConstant, + start: lockConstant, + stop: unlockConstant, + expectedBool: true, + }, + } + + for _, test := range stateOptions { + t.Run(test.title, func(t *testing.T) { + + isValid := validAction(test.running, test.requestedAction, test.start, test.stop) + + if isValid != test.expectedBool { + t.Errorf("\nActions - wanted: %t, got %t\n", test.expectedBool, isValid) + } + }) + } +} + +func Test_findLabel(t *testing.T) { + + var stateOptions = []struct { + title string + currentLabels []types.IssueLabel + cmdLabel string + expectedFound bool + }{ + { + title: "Label exists lowercase", + currentLabels: []types.IssueLabel{ + types.IssueLabel{ + Name: "rod", + }, + types.IssueLabel{ + Name: "jane", + }, + types.IssueLabel{ + Name: "freddie", + }, + }, + cmdLabel: "rod", + expectedFound: true, + }, + { + title: "Label exists case insensitive", + currentLabels: []types.IssueLabel{ + types.IssueLabel{ + Name: "rod", + }, + types.IssueLabel{ + Name: "jane", + }, + types.IssueLabel{ + Name: "freddie", + }, + }, + cmdLabel: "Rod", + expectedFound: true, + }, + { + title: "Label doesnt exist lowercase", + currentLabels: []types.IssueLabel{ + types.IssueLabel{ + Name: "rod", + }, + types.IssueLabel{ + Name: "jane", + }, + types.IssueLabel{ + Name: "freddie", + }, + }, + cmdLabel: "derek", + expectedFound: false, + }, + { + title: "Label doesnt exist case insensitive", + currentLabels: []types.IssueLabel{ + types.IssueLabel{ + Name: "rod", + }, + types.IssueLabel{ + Name: "jane", + }, + types.IssueLabel{ + Name: "freddie", + }, + }, + cmdLabel: "Derek", + expectedFound: false, + }, + { + title: "no existing labels lowercase", + currentLabels: nil, + cmdLabel: "derek", + expectedFound: false, + }, + {title: "Label doesnt exist case insensitive", + currentLabels: nil, + cmdLabel: "Derek", + expectedFound: false, + }, + } + + for _, test := range stateOptions { + t.Run(test.title, func(t *testing.T) { + + labelFound := findLabel(test.currentLabels, test.cmdLabel) + + if labelFound != test.expectedFound { + t.Errorf("Find Labels(%s) - wanted: %t, got %t\n", test.title, test.expectedFound, labelFound) + } + }) + } +} diff --git a/main.go b/main.go index 0e3978e..fe23550 100644 --- a/main.go +++ b/main.go @@ -67,7 +67,7 @@ func handleEvent(eventType string, bytesIn []byte) error { if err != nil { return fmt.Errorf("Unable to access maintainers file at: %s/%s", req.Repository.Owner.Login, req.Repository.Name) } - if req.Action != closed { + if req.Action != closedConstant { if enabledFeature(dcoCheck, derekConfig) { handlePullRequest(req) }