Merge pull request #641 from exercism/download-uuid

Fix problem with downloading another person's solution using the --uuid flag

Author: Katrina Owen

cmd/download.go

@@ -132,20 +132,16 @@ func runDownload(cfg config.Configuration, flags *pflag.FlagSet, args []string)
 		IsRequester: payload.Solution.User.IsRequester,
 	}
 
-	dir := filepath.Join(usrCfg.GetString("workspace"), solution.Track)
-	os.MkdirAll(dir, os.FileMode(0755))
+	dir := usrCfg.GetString("workspace")
+	if !solution.IsRequester {
+		dir = filepath.Join(dir, "users", solution.Handle)
+	}
+	dir = filepath.Join(dir, solution.Track)
 
-	var ws workspace.Workspace
-	if solution.IsRequester {
-		ws, err = workspace.New(dir)
-		if err != nil {
-			return err
-		}
-	} else {
-		ws, err = workspace.New(filepath.Join(usrCfg.GetString("workspace"), "users", solution.Handle, solution.Track))
-		if err != nil {
-			return err
-		}
+	os.MkdirAll(dir, os.FileMode(0755))
+	ws, err := workspace.New(dir)
+	if err != nil {
+		return err
 	}
 
 	dir, err = ws.SolutionPath(solution.Exercise, solution.ID)

cmd/download_test.go

@@ -43,7 +43,7 @@ func TestDownloadWithoutFlags(t *testing.T) {
 	}
 }
 
-func TestDownload(t *testing.T) {
+func TestDownloadTHISONE(t *testing.T) {
 	oldOut := Out
 	oldErr := Err
 	Out = ioutil.Discard
@@ -53,46 +53,93 @@ func TestDownload(t *testing.T) {
 		Err = oldErr
 	}()
 
-	tmpDir, err := ioutil.TempDir("", "download-cmd")
-	assert.NoError(t, err)
+	testCases := []struct {
+		requestor       string
+		expectedDir     string
+		flag, flagValue string
+	}{
+		{requestorSelf, "", "exercise", "bogus-exercise"},
+		{requestorSelf, "", "uuid", "bogus-id"},
+		{requestorOther, filepath.Join("users", "alice"), "uuid", "bogus-id"},
+	}
 
-	ts := fakeDownloadServer()
-	defer ts.Close()
+	for _, tc := range testCases {
+		tmpDir, err := ioutil.TempDir("", "download-cmd")
+		assert.NoError(t, err)
 
-	v := viper.New()
-	v.Set("workspace", tmpDir)
-	v.Set("apibaseurl", ts.URL)
-	v.Set("token", "abc123")
+		ts := fakeDownloadServer(tc.requestor)
+		defer ts.Close()
 
-	cfg := config.Configuration{
-		UserViperConfig: v,
+		v := viper.New()
+		v.Set("workspace", tmpDir)
+		v.Set("apibaseurl", ts.URL)
+		v.Set("token", "abc123")
+
+		cfg := config.Configuration{
+			UserViperConfig: v,
+		}
+		flags := pflag.NewFlagSet("fake", pflag.PanicOnError)
+		setupDownloadFlags(flags)
+		flags.Set(tc.flag, tc.flagValue)
+
+		err = runDownload(cfg, flags, []string{})
+		assert.NoError(t, err)
+
+		assertDownloadedCorrectFiles(t, filepath.Join(tmpDir, tc.expectedDir), tc.requestor)
 	}
-	flags := pflag.NewFlagSet("fake", pflag.PanicOnError)
-	setupDownloadFlags(flags)
-	flags.Set("exercise", "bogus-exercise")
+}
+
+func fakeDownloadServer(requestor string) *httptest.Server {
+	mux := http.NewServeMux()
+	server := httptest.NewServer(mux)
 
-	err = runDownload(cfg, flags, []string{})
-	assert.NoError(t, err)
+	path1 := "file-1.txt"
+	mux.HandleFunc("/"+path1, func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "this is file 1")
+	})
+
+	path2 := "subdir/file-2.txt"
+	mux.HandleFunc("/"+path2, func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "this is file 2")
+	})
+
+	path3 := "file-3.txt"
+	mux.HandleFunc("/"+path3, func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, "")
+	})
 
+	payloadBody := fmt.Sprintf(payloadTemplate, requestor, server.URL+"/", path1, path2, path3)
+	mux.HandleFunc("/solutions/latest", func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, payloadBody)
+	})
+	mux.HandleFunc("/solutions/bogus-id", func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprint(w, payloadBody)
+	})
+
+	return server
+}
+
+func assertDownloadedCorrectFiles(t *testing.T, targetDir, requestor string) {
+	metadata := `{"track":"bogus-track","exercise":"bogus-exercise","id":"bogus-id","url":"","handle":"alice","is_requester":%s,"auto_approve":false}`
 	expectedFiles := []struct {
 		desc     string
 		path     string
 		contents string
 	}{
 		{
 			desc:     "a file in the exercise root directory",
-			path:     filepath.Join(tmpDir, "bogus-track", "bogus-exercise", "file-1.txt"),
+			path:     filepath.Join(targetDir, "bogus-track", "bogus-exercise", "file-1.txt"),
 			contents: "this is file 1",
 		},
 		{
 			desc:     "a file in a subdirectory",
-			path:     filepath.Join(tmpDir, "bogus-track", "bogus-exercise", "subdir", "file-2.txt"),
+			path:     filepath.Join(targetDir, "bogus-track", "bogus-exercise", "subdir", "file-2.txt"),
 			contents: "this is file 2",
 		},
 		{
 			desc:     "the solution metadata file",
-			path:     filepath.Join(tmpDir, "bogus-track", "bogus-exercise", ".solution.json"),
-			contents: `{"track":"bogus-track","exercise":"bogus-exercise","id":"bogus-id","url":"","handle":"alice","is_requester":true,"auto_approve":false}`,
+			path:     filepath.Join(targetDir, "bogus-track", "bogus-exercise", ".solution.json"),
+			contents: fmt.Sprintf(metadata, requestor),
 		},
 	}
 
@@ -104,45 +151,21 @@ func TestDownload(t *testing.T) {
 		})
 	}
 
-	path := filepath.Join(tmpDir, "bogus-track", "bogus-exercise", "file-3.txt")
-	_, err = os.Lstat(path)
+	path := filepath.Join(targetDir, "bogus-track", "bogus-exercise", "file-3.txt")
+	_, err := os.Lstat(path)
 	assert.True(t, os.IsNotExist(err), "It should not write the file if empty.")
 }
 
-func fakeDownloadServer() *httptest.Server {
-	mux := http.NewServeMux()
-	server := httptest.NewServer(mux)
-
-	path1 := "file-1.txt"
-	mux.HandleFunc("/"+path1, func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprint(w, "this is file 1")
-	})
-
-	path2 := "subdir/file-2.txt"
-	mux.HandleFunc("/"+path2, func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprint(w, "this is file 2")
-	})
-
-	path3 := "file-3.txt"
-	mux.HandleFunc("/"+path3, func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprint(w, "")
-	})
-
-	payloadBody := fmt.Sprintf(payloadTemplate, server.URL+"/", path1, path2, path3)
-	mux.HandleFunc("/solutions/latest", func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprint(w, payloadBody)
-	})
-
-	return server
-}
+const requestorSelf = "true"
+const requestorOther = "false"
 
 const payloadTemplate = `
 {
 	"solution": {
 		"id": "bogus-id",
 		"user": {
 			"handle": "alice",
-			"is_requester": true
+			"is_requester": %s
 		},
 		"exercise": {
 			"id": "bogus-exercise",