fix for (1) "whole site" inconsistent with form (2) user URL not usin…

config/config.go

@@ -99,6 +99,10 @@ func IsSiteDomainURL(urlString, siteDomain string) bool {
 	if siteDomain == "" {
 		siteDomain = cfg.SiteDomain
 	}
+	if !strings.HasPrefix(urlString, "http") {
+		// user may enter a URL without a scheme (e.g. `host/path`), so add it before parsing
+		urlString = "https://" + urlString
+	}
 	urlObject, err := url.ParseRequestURI(urlString)
 	if err != nil {
 		return false

config/config_test.go

@@ -58,7 +58,7 @@ func TestConfig(t *testing.T) {
 				So(isAllowedURL, ShouldBeTrue)
 			})
 			Convey("Then a sub-domain/host of the config's site domain is recognised", func() {
-				isAllowedURL := IsSiteDomainURL("https://anything.localhost", "")
+				isAllowedURL := IsSiteDomainURL("anything.localhost", "")
 				So(isAllowedURL, ShouldBeTrue)
 			})
 			Convey("Then a non-site domain URL is not recognised for config's site domain", func() {

handlers/feedback.go

@@ -96,7 +96,7 @@ func addFeedback(w http.ResponseWriter, req *http.Request, rend interfaces.Rende
 	}
 
 	if ff.URL == "" {
-		ff.URL = "Whole site"
+		ff.URL = mapper.WholeSite
 	}
 
 	if err := emailSender.Send(
@@ -111,7 +111,7 @@ func addFeedback(w http.ResponseWriter, req *http.Request, rend interfaces.Rende
 
 	returnTo := ff.URL
 
-	if returnTo == "Whole site" || returnTo == "" {
+	if returnTo == mapper.WholeSite || returnTo == "" {
 		returnTo = "https://www.ons.gov.uk"
 	}
 
@@ -133,7 +133,7 @@ func validateForm(ff *model.FeedbackForm, siteDomain string) (validationErrors [
 	}
 
 	ff.URL = strings.TrimSpace(ff.URL)
-	if ff.Type == "A specific page" {
+	if ff.Type == mapper.ASpecificPage {
 		if ff.URL == "" {
 			validationErrors = append(validationErrors, core.ErrorItem{
 				Description: core.Localisation{
@@ -156,9 +156,7 @@ func validateForm(ff *model.FeedbackForm, siteDomain string) (validationErrors [
 				ff.IsURLErr = true
 			}
 		}
-	}
-
-	if ff.Type != "A specific page" && ff.URL != "" {
+	} else if ff.Type != mapper.ASpecificPage && ff.URL != "" {
 		ff.URL = ""
 	}
 

handlers/feedback_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/ONSdigital/dp-frontend-feedback-controller/config"
 	"github.com/ONSdigital/dp-frontend-feedback-controller/email/emailtest"
 	"github.com/ONSdigital/dp-frontend-feedback-controller/interfaces/interfacestest"
+	"github.com/ONSdigital/dp-frontend-feedback-controller/mapper"
 	"github.com/ONSdigital/dp-frontend-feedback-controller/mocks"
 	"github.com/ONSdigital/dp-frontend-feedback-controller/model"
 	"github.com/ONSdigital/dp-renderer/v2/helper"
@@ -345,7 +346,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the form is valid",
 				given: &model.FeedbackForm{
-					Type:        "Whole site",
+					Type:        mapper.WholeSite,
 					Description: "Some text",
 				},
 				expectedDescription: "no validation errors are returned",
@@ -370,7 +371,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the a specific page/url type is chosen but the child input field is empty",
 				given: &model.FeedbackForm{
-					Type:        "A specific page",
+					Type:        mapper.ASpecificPage,
 					Description: "Some text",
 				},
 				expectedDescription: "a page/url validation error is returned",
@@ -387,7 +388,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the a specific page/url type is chosen and the url is invalid",
 				given: &model.FeedbackForm{
-					Type:        "A specific page",
+					Type:        mapper.ASpecificPage,
 					Description: "Some text",
 					URL:         "not a url",
 				},
@@ -405,7 +406,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the a specific page/url type is chosen and the url is valid but not allowed",
 				given: &model.FeedbackForm{
-					Type:        "A specific page",
+					Type:        mapper.ASpecificPage,
 					Description: "Some text",
 					URL:         "https://not-site-domain.com",
 				},
@@ -422,7 +423,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the a specific page/url type is chosen and the url is valid without a path",
 				given: &model.FeedbackForm{
-					Type:        "A specific page",
+					Type:        mapper.ASpecificPage,
 					Description: "Some text",
 					URL:         "https://cy.ons.gov.uk",
 				},
@@ -432,17 +433,17 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the a specific page/url type is chosen and the url is valid and has a path",
 				given: &model.FeedbackForm{
-					Type:        "A specific page",
+					Type:        mapper.ASpecificPage,
 					Description: "Some text",
 					URL:         "https://cy.ons.gov.uk/path",
 				},
 				expectedDescription: "no validation errors are returned",
 				expected:            []coreModel.ErrorItem(nil),
 			},
 			{
-				givenDescription: "the a whole site type is chosen but the child input for a specific page is not empty",
+				givenDescription: "the whole-site type is chosen but the child input for a specific page is not empty",
 				given: &model.FeedbackForm{
-					Type:        "Whole site",
+					Type:        mapper.WholeSite,
 					Description: "Some text",
 					URL:         "http://somewhere.com",
 				},
@@ -461,7 +462,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the form does not have any feedback",
 				given: &model.FeedbackForm{
-					Type: "Whole site",
+					Type: mapper.WholeSite,
 				},
 				expectedDescription: "a description validation error is returned",
 				expected: []coreModel.ErrorItem{
@@ -477,7 +478,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the feedback provided is whitespace",
 				given: &model.FeedbackForm{
-					Type:        "Whole site",
+					Type:        mapper.WholeSite,
 					Description: " ",
 				},
 				expectedDescription: "a description validation error is returned",
@@ -494,7 +495,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the email field has an invalid email address",
 				given: &model.FeedbackForm{
-					Type:        "Whole site",
+					Type:        mapper.WholeSite,
 					Description: "A description",
 					Email:       "a.string",
 				},
@@ -512,7 +513,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "the email field has a valid email address",
 				given: &model.FeedbackForm{
-					Type:        "Whole site",
+					Type:        mapper.WholeSite,
 					Description: "A description",
 					Email:       "[email protected]",
 				},
@@ -522,7 +523,7 @@ func TestValidateForm(t *testing.T) {
 			{
 				givenDescription: "multiple form validation errors",
 				given: &model.FeedbackForm{
-					Type:        "A specific page",
+					Type:        mapper.ASpecificPage,
 					URL:         "",
 					Description: "",
 					Email:       "not an email address",

mapper/mapper.go

@@ -10,6 +10,11 @@ import (
 	core "github.com/ONSdigital/dp-renderer/v2/model"
 )
 
+const (
+	WholeSite     = "The whole website"
+	ASpecificPage = "A specific page"
+)
+
 // CreateGetFeedback returns a mapped feedback page to the feedback model
 func CreateGetFeedback(req *http.Request, basePage core.Page, validationErrors []core.ErrorItem, ff model.FeedbackForm, lang string) model.Feedback {
 	p := model.Feedback{
@@ -59,25 +64,25 @@ func CreateGetFeedback(req *http.Request, basePage core.Page, validationErrors [
 			{
 				Input: core.Input{
 					ID:        "whole-site",
-					IsChecked: ff.Type == "The whole website",
+					IsChecked: ff.Type == WholeSite,
 					Label: core.Localisation{
 						LocaleKey: "FeedbackWholeWebsite",
 						Plural:    1,
 					},
 					Name:  "type",
-					Value: "The whole website",
+					Value: WholeSite,
 				},
 			},
 			{
 				Input: core.Input{
 					ID:        "specific-page",
-					IsChecked: ff.Type == "A specific page" || (ff.URL != "" && serviceDescription == ""),
+					IsChecked: ff.Type == ASpecificPage || (ff.URL != "" && serviceDescription == ""),
 					Label: core.Localisation{
 						LocaleKey: "FeedbackASpecificPage",
 						Plural:    1,
 					},
 					Name:  "type",
-					Value: "A specific page",
+					Value: ASpecificPage,
 				},
 				OtherInput: core.Input{
 					Autocomplete: "url",
@@ -196,24 +201,27 @@ func CreateGetFeedback(req *http.Request, basePage core.Page, validationErrors [
 	return p
 }
 
-func CreateGetFeedbackThanks(req *http.Request, basePage core.Page, lang, referrer, wholeSite string) model.Feedback {
-	p := model.Feedback{
-		Page: basePage,
+func CreateGetFeedbackThanks(req *http.Request, basePage core.Page, lang, referrer, wholeSiteURL string) model.Feedback {
+	if wholeSiteURL == "" {
+		wholeSiteURL = "https://www.ons.gov.uk"
 	}
 	if referrer == "" {
-		referrer = wholeSite
+		referrer = wholeSiteURL
 	}
 
+	p := model.Feedback{
+		Page:        basePage,
+		PreviousURL: referrer,
+	}
 	p.Language = lang
 	p.Type = "feedback"
 	p.URI = req.URL.Path
 	p.Metadata.Title = helper.Localise("FeedbackThanks", lang, 1)
-	p.PreviousURL = referrer
 
 	// returnTo is rendered on page so needs XSS protection
 	returnTo := html.EscapeString(req.URL.Query().Get("returnTo"))
-	if returnTo == "Whole site" {
-		returnTo = wholeSite
+	if returnTo == WholeSite {
+		returnTo = wholeSiteURL
 	} else if returnTo == "" {
 		returnTo = referrer
 	} else if !config.IsSiteDomainURL(returnTo, "") {

mapper/mapper_test.go

@@ -3,6 +3,7 @@ package mapper
 import (
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/ONSdigital/dp-frontend-feedback-controller/mocks"
@@ -105,8 +106,8 @@ func TestCreateGetFeedbackThanks(t *testing.T) {
 			bp := core.Page{}
 			url := "https://localhost/a/page/somewhere"
 			lang := "en"
-			wholeSite := "https://ons.gov.uk"
-			sut := CreateGetFeedbackThanks(req, bp, lang, url, wholeSite)
+			wholeSiteURL := "https://ons.gov.uk"
+			sut := CreateGetFeedbackThanks(req, bp, lang, url, wholeSiteURL)
 
 			Convey("Then it sets the page metadata", func() {
 				So(sut.Metadata.Title, ShouldEqual, "Thank you")
@@ -123,16 +124,28 @@ func TestCreateGetFeedbackThanks(t *testing.T) {
 			})
 		})
 
-		Convey("When the return to parameter is set", func() {
-			req := httptest.NewRequest(http.MethodGet, "/?returnTo=Whole%20site", nil)
-			bp := core.Page{}
-			url := "https://localhost/a/page/somewhere"
-			lang := "en"
-			wholeSite := "https://ons.gov.uk"
-			sut := CreateGetFeedbackThanks(req, bp, lang, url, wholeSite)
+		var (
+			referrer     = "https://any.localhost/a/page/somewhere"
+			lang         = "en"
+			wholeSiteURL = "https://cy.localhost"
+			encWholeSite = url.QueryEscape(WholeSite)
+			bp           = core.Page{}
+		)
 
-			Convey("Then it sets the returnTo property", func() {
-				So(sut.ReturnTo, ShouldEqual, wholeSite)
+		Convey("When the returnTo parameter is set to whole-site and whole-site is explicit", func() {
+			req := httptest.NewRequest(http.MethodGet, "/?returnTo="+encWholeSite, nil)
+			sut := CreateGetFeedbackThanks(req, bp, lang, referrer, wholeSiteURL)
+
+			Convey("Then it sets the returnTo property to the whole-site", func() {
+				So(sut.ReturnTo, ShouldEqual, wholeSiteURL)
+			})
+		})
+		Convey("When the returnTo parameter is set to whole-site but whole-site is not explicit", func() {
+			req := httptest.NewRequest(http.MethodGet, "/?returnTo="+encWholeSite, nil)
+			sut := CreateGetFeedbackThanks(req, bp, lang, referrer, "")
+
+			Convey("Then it sets the returnTo property to the default whole-site", func() {
+				So(sut.ReturnTo, ShouldEqual, "https://www.ons.gov.uk")
 			})
 		})
 	})