repo: fix redirect after opening/closing milestone (#5903)

* Fix milestone redirect

* gosimple

* Apply suggestions from code review

Co-Authored-By: ᴜɴᴋɴᴡᴏɴ <[email protected]>

* fix typo

* Update docstring of MakeURL

Co-authored-by: ᴜɴᴋɴᴡᴏɴ <[email protected]>

internal/context/repo.go

@@ -7,6 +7,7 @@ package context
 import (
 	"fmt"
 	"io/ioutil"
+	"net/url"
 	"strings"
 
 	"github.com/editorconfig/editorconfig-core-go/v2"
@@ -96,6 +97,22 @@ func (r *Repository) GetEditorconfig() (*editorconfig.Editorconfig, error) {
 	return editorconfig.ParseBytes(data)
 }
 
+// MakeURL accepts a string or url.URL as argument and returns escaped URL prepended with repository URL.
+func (r *Repository) MakeURL(location interface{}) string {
+	switch location := location.(type) {
+	case string:
+		tempURL := url.URL{
+			Path: r.RepoLink + "/" + location,
+		}
+		return tempURL.String()
+	case url.URL:
+		location.Path = r.RepoLink + "/" + location.Path
+		return location.String()
+	default:
+		panic("location type must be either string or url.URL")
+	}
+}
+
 // PullRequestURL returns URL for composing a pull request.
 // This function does not check if the repository can actually compose a pull request.
 func (r *Repository) PullRequestURL(baseBranch, headBranch string) string {

internal/route/repo/issue.go

@@ -23,7 +23,6 @@ import (
 	"gogs.io/gogs/internal/form"
 	"gogs.io/gogs/internal/markup"
 	"gogs.io/gogs/internal/setting"
-	"gogs.io/gogs/internal/template"
 	"gogs.io/gogs/internal/tool"
 )
 
@@ -449,7 +448,7 @@ func NewIssuePost(c *context.Context, f form.NewIssue) {
 	}
 
 	log.Trace("Issue created: %d/%d", c.Repo.Repository.ID, issue.ID)
-	c.Redirect(c.Repo.RepoLink + "/issues/" + com.ToStr(issue.Index))
+	c.RawRedirect(c.Repo.MakeURL(fmt.Sprintf("issues/%d", issue.Index)))
 }
 
 func uploadAttachment(c *context.Context, allowedTypes []string) {
@@ -522,10 +521,10 @@ func viewIssue(c *context.Context, isPullList bool) {
 
 	// Make sure type and URL matches.
 	if !isPullList && issue.IsPull {
-		c.Redirect(c.Repo.RepoLink + "/pulls/" + com.ToStr(issue.Index))
+		c.RawRedirect(c.Repo.MakeURL(fmt.Sprintf("pulls/%d", issue.Index)))
 		return
 	} else if isPullList && !issue.IsPull {
-		c.Redirect(c.Repo.RepoLink + "/issues/" + com.ToStr(issue.Index))
+		c.RawRedirect(c.Repo.MakeURL(fmt.Sprintf("issues/%d", issue.Index)))
 		return
 	}
 
@@ -660,8 +659,10 @@ func viewIssue(c *context.Context, isPullList bool) {
 			c.Repo.IsWriter() && c.Repo.GitRepo.IsBranchExist(pull.HeadBranch) &&
 			!branchProtected
 
-		deleteBranchUrl := template.EscapePound(c.Repo.RepoLink + "/branches/delete/" + pull.HeadBranch)
-		c.Data["DeleteBranchLink"] = fmt.Sprintf("%s?commit=%s&redirect_to=%s", deleteBranchUrl, pull.MergedCommitID, c.Data["Link"])
+		c.Data["DeleteBranchLink"] = c.Repo.MakeURL(url.URL{
+			Path:     "branches/delete/" + pull.HeadBranch,
+			RawQuery: fmt.Sprintf("commit=%s&redirect_to=%s", pull.MergedCommitID, c.Data["Link"]),
+		})
 	}
 
 	c.Data["Participants"] = participants
@@ -850,7 +851,7 @@ func NewComment(c *context.Context, f form.CreateComment) {
 
 	if c.HasError() {
 		c.Flash.Error(c.Data["ErrorMsg"].(string))
-		c.Redirect(fmt.Sprintf("%s/issues/%d", c.Repo.RepoLink, issue.Index))
+		c.RawRedirect(c.Repo.MakeURL(fmt.Sprintf("issues/%d", issue.Index)))
 		return
 	}
 
@@ -902,11 +903,16 @@ func NewComment(c *context.Context, f form.CreateComment) {
 		if issue.IsPull {
 			typeName = "pulls"
 		}
+
+		location := url.URL{
+			Path: fmt.Sprintf("%s/%d", typeName, issue.Index),
+		}
+
 		if comment != nil {
-			c.RawRedirect(fmt.Sprintf("%s/%s/%d#%s", c.Repo.RepoLink, typeName, issue.Index, comment.HashTag()))
-		} else {
-			c.Redirect(fmt.Sprintf("%s/%s/%d", c.Repo.RepoLink, typeName, issue.Index))
+			location.Fragment = comment.HashTag()
 		}
+
+		c.RawRedirect(c.Repo.MakeURL(location))
 	}()
 
 	// Fix #321: Allow empty comments, as long as we have attachments.
@@ -990,13 +996,13 @@ func Labels(c *context.Context) {
 
 func InitializeLabels(c *context.Context, f form.InitializeLabels) {
 	if c.HasError() {
-		c.Redirect(c.Repo.RepoLink + "/labels")
+		c.RawRedirect(c.Repo.MakeURL("labels"))
 		return
 	}
 	list, err := db.GetLabelTemplateFile(f.TemplateName)
 	if err != nil {
 		c.Flash.Error(c.Tr("repo.issues.label_templates.fail_to_load_file", f.TemplateName, err))
-		c.Redirect(c.Repo.RepoLink + "/labels")
+		c.RawRedirect(c.Repo.MakeURL("labels"))
 		return
 	}
 
@@ -1012,7 +1018,7 @@ func InitializeLabels(c *context.Context, f form.InitializeLabels) {
 		c.Handle(500, "NewLabels", err)
 		return
 	}
-	c.Redirect(c.Repo.RepoLink + "/labels")
+	c.RawRedirect(c.Repo.MakeURL("labels"))
 }
 
 func NewLabel(c *context.Context, f form.CreateLabel) {
@@ -1021,7 +1027,7 @@ func NewLabel(c *context.Context, f form.CreateLabel) {
 
 	if c.HasError() {
 		c.Flash.Error(c.Data["ErrorMsg"].(string))
-		c.Redirect(c.Repo.RepoLink + "/labels")
+		c.RawRedirect(c.Repo.MakeURL("labels"))
 		return
 	}
 
@@ -1034,7 +1040,7 @@ func NewLabel(c *context.Context, f form.CreateLabel) {
 		c.Handle(500, "NewLabel", err)
 		return
 	}
-	c.Redirect(c.Repo.RepoLink + "/labels")
+	c.RawRedirect(c.Repo.MakeURL("labels"))
 }
 
 func UpdateLabel(c *context.Context, f form.CreateLabel) {
@@ -1055,7 +1061,7 @@ func UpdateLabel(c *context.Context, f form.CreateLabel) {
 		c.Handle(500, "UpdateLabel", err)
 		return
 	}
-	c.Redirect(c.Repo.RepoLink + "/labels")
+	c.RawRedirect(c.Repo.MakeURL("labels"))
 }
 
 func DeleteLabel(c *context.Context) {
@@ -1066,7 +1072,7 @@ func DeleteLabel(c *context.Context) {
 	}
 
 	c.JSON(200, map[string]interface{}{
-		"redirect": c.Repo.RepoLink + "/labels",
+		"redirect": c.Repo.MakeURL("labels"),
 	})
 	return
 }
@@ -1161,7 +1167,7 @@ func NewMilestonePost(c *context.Context, f form.CreateMilestone) {
 	}
 
 	c.Flash.Success(c.Tr("repo.milestones.create_success", f.Title))
-	c.Redirect(c.Repo.RepoLink + "/milestones")
+	c.RawRedirect(c.Repo.MakeURL("milestones"))
 }
 
 func EditMilestone(c *context.Context) {
@@ -1228,7 +1234,7 @@ func EditMilestonePost(c *context.Context, f form.CreateMilestone) {
 	}
 
 	c.Flash.Success(c.Tr("repo.milestones.edit_success", m.Name))
-	c.Redirect(c.Repo.RepoLink + "/milestones")
+	c.RawRedirect(c.Repo.MakeURL("milestones"))
 }
 
 func ChangeMilestonStatus(c *context.Context) {
@@ -1242,6 +1248,10 @@ func ChangeMilestonStatus(c *context.Context) {
 		return
 	}
 
+	location := url.URL{
+		Path: "milestones",
+	}
+
 	switch c.Params(":action") {
 	case "open":
 		if m.IsClosed {
@@ -1250,7 +1260,7 @@ func ChangeMilestonStatus(c *context.Context) {
 				return
 			}
 		}
-		c.Redirect(c.Repo.RepoLink + "/milestones?state=open")
+		location.RawQuery = "state=open"
 	case "close":
 		if !m.IsClosed {
 			m.ClosedDate = time.Now()
@@ -1259,10 +1269,10 @@ func ChangeMilestonStatus(c *context.Context) {
 				return
 			}
 		}
-		c.Redirect(c.Repo.RepoLink + "/milestones?state=closed")
-	default:
-		c.Redirect(c.Repo.RepoLink + "/milestones")
+		location.RawQuery = "state=closed"
 	}
+
+	c.RawRedirect(c.Repo.MakeURL(location))
 }
 
 func DeleteMilestone(c *context.Context) {
@@ -1273,6 +1283,6 @@ func DeleteMilestone(c *context.Context) {
 	}
 
 	c.JSON(200, map[string]interface{}{
-		"redirect": c.Repo.RepoLink + "/milestones",
+		"redirect": c.Repo.MakeURL("milestones"),
 	})
 }