Merge pull request #275 from macb/bugfix/github_slack_urls

Correct url building for branch query.

pkg/plugin/notify/slack.go

@@ -3,7 +3,7 @@ package notify
 import (
 	"encoding/json"
 	"fmt"
-	"path"
+	"net/url"
 )
 
 const (
@@ -37,7 +37,12 @@ func (s *Slack) Send(context *Context) error {
 }
 
 func getBuildUrl(context *Context) string {
-	return context.Host + path.Join("/", context.Repo.Slug, "commit", context.Commit.Hash)
+	branchQuery := url.Values{}
+	if context.Commit.Branch != "" {
+		branchQuery.Set("branch", context.Commit.Branch)
+	}
+
+	return fmt.Sprintf("%s/%s/commit/%s?%s", context.Host, context.Repo.Slug, context.Commit.Hash, branchQuery.Encode())
 }
 
 func getMessage(context *Context, message string) string {

pkg/plugin/notify/slack_test.go

@@ -0,0 +1,41 @@
+package notify
+
+import (
+	"github.com/drone/drone/pkg/model"
+	"testing"
+)
+
+func Test_getBuildUrl(t *testing.T) {
+	c := &Context{
+		Host: "http://examplehost.com",
+		Repo: &model.Repo{
+			Slug: "examplegit.com/owner/repo",
+		},
+		Commit: &model.Commit{
+			Hash:   "abc",
+			Branch: "example",
+		},
+	}
+	expected := "http://examplehost.com/examplegit.com/owner/repo/commit/abc?branch=example"
+	output := getBuildUrl(c)
+
+	if output != expected {
+		t.Errorf("Failed to build url. Expected: %s, got %s", expected, output)
+	}
+
+	c.Commit.Branch = "url/unsafe/branch"
+	expected = "http://examplehost.com/examplegit.com/owner/repo/commit/abc?branch=url%2Funsafe%2Fbranch"
+	output = getBuildUrl(c)
+
+	if output != expected {
+		t.Errorf("Failed to build url. Expected: %s, got %s", expected, output)
+	}
+
+	c.Commit.Branch = ""
+	expected = "http://examplehost.com/examplegit.com/owner/repo/commit/abc?"
+	output = getBuildUrl(c)
+
+	if output != expected {
+		t.Errorf("Failed to build url. Expected: %s, got %s", expected, output)
+	}
+}

pkg/queue/worker.go

@@ -12,6 +12,7 @@ import (
 	"github.com/drone/go-github/github"
 	"io"
 	"log"
+	"net/url"
 	"path/filepath"
 	"time"
 )
@@ -222,11 +223,16 @@ func updateGitHubStatus(repo *Repo, commit *Commit) error {
 
 	client := github.New(user.GithubToken)
 	client.ApiUrl = settings.GitHubApiUrl
+	buildUrl := getBuildUrl(settings.URL().String(), repo, commit)
 
-	var url string
+	return client.Repos.CreateStatus(repo.Owner, repo.Name, status, buildUrl, message, commit.Hash)
-	url = settings.URL().String() + "/" + repo.Slug + "/commit/" + commit.Branch + "/" + commit.Hash
+}
 
-	return client.Repos.CreateStatus(repo.Owner, repo.Name, status, url, message, commit.Hash)
+func getBuildUrl(host string, repo *Repo, commit *Commit) string {
+	branchQuery := url.Values{}
+	branchQuery.Set("branch", commit.Branch)
+	buildUrl := fmt.Sprintf("%s/%s/commit/%s?%s", host, repo.Slug, commit.Hash, branchQuery.Encode())
+	return buildUrl
 }
 
 type bufferWrapper struct {

pkg/queue/worker_test.go

@@ -0,0 +1,31 @@
+package queue
+
+import (
+	. "github.com/drone/drone/pkg/model"
+	"testing"
+)
+
+func Test_getBuildUrl(t *testing.T) {
+	repo := &Repo{
+		Slug: "examplegit.com/owner/repo",
+	}
+	commit := &Commit{
+		Hash:   "abc",
+		Branch: "example",
+	}
+
+	expected := "http://examplehost.com/examplegit.com/owner/repo/commit/abc?branch=example"
+	output := getBuildUrl("http://examplehost.com", repo, commit)
+
+	if output != expected {
+		t.Errorf("Failed to build url. Expected: %s, got %s", expected, output)
+	}
+
+	commit.Branch = "url/unsafe/branch"
+	expected = "http://examplehost.com/examplegit.com/owner/repo/commit/abc?branch=url%2Funsafe%2Fbranch"
+	output = getBuildUrl("http://examplehost.com", repo, commit)
+
+	if output != expected {
+		t.Errorf("Failed to build url. Expected: %s, got %s", expected, output)
+	}
+}