dashboard: refactor handleTestRequest errors

Instead of returning two values, return just one object.
Introduce several kinds of errors.

3 files changed, 50 insertions, 23 deletions

diff --git a/dashboard/app/jobs.go b/dashboard/app/jobs.go
index a1c95728c..49d7212c5 100644
--- a/dashboard/app/jobs.go
+++ b/dashboard/app/jobs.go
@@ -33,31 +33,31 @@ type testReqArgs struct {
 }
 
 // handleTestRequest added new job to db.
-// Returns empty string if job added successfully, or reason why it wasn't added.
-func handleTestRequest(c context.Context, args *testReqArgs) string {
+// Returns nil if job added successfully.
+// If the arguments are invalid, the error is of type *BadTestRequest.
+// If the request was denied, the error is of type *TestRequestDenied.
+// All other errors correspond to internal processing problems.
+func handleTestRequest(c context.Context, args *testReqArgs) error {
 	log.Infof(c, "test request: bug=%s user=%q extID=%q patch=%v, repo=%q branch=%q",
 		args.bug.Title, args.user, args.extID, len(args.patch), args.repo, args.branch)
 	for _, blocked := range config.EmailBlocklist {
 		if args.user == blocked {
-			log.Errorf(c, "test request from blocked user: %v", args.user)
-			return ""
+			return &TestRequestDeniedError{
+				fmt.Sprintf("test request from blocked user: %v", args.user),
+			}
 		}
 	}
 	now := timeNow(c)
 	crash, crashKey, err := findCrashForBug(c, args.bug)
 	if err != nil {
-		log.Errorf(c, "failed to find a crash: %v", err)
-		return ""
+		return fmt.Errorf("failed to find a crash: %v", err)
 	}
-	reply, err := addTestJob(c, &testJobArgs{
+	err = addTestJob(c, &testJobArgs{
 		testReqArgs: *args,
 		crash:       crash, crashKey: crashKey,
 	}, now)
 	if err != nil {
-		log.Errorf(c, "test request failed: %v", err)
-		if reply == "" {
-			reply = internalError
-		}
+		return err
 	}
 	// Update bug CC and last activity time.
 	tx := func(c context.Context) error {
@@ -80,7 +80,7 @@ func handleTestRequest(c context.Context, args *testReqArgs) string {
 		// We've already stored the job, so just log the error.
 		log.Errorf(c, "failed to update bug: %v", err)
 	}
-	return reply
+	return nil
 }
 
 type testJobArgs struct {
@@ -90,16 +90,16 @@ type testJobArgs struct {
 	testReqArgs
 }
 
-func addTestJob(c context.Context, args *testJobArgs, now time.Time) (string, error) {
+func addTestJob(c context.Context, args *testJobArgs, now time.Time) error {
 	if reason := checkTestJob(c, args.bug, args.bugReporting, args.crash,
 		args.repo, args.branch); reason != "" {
-		return reason, nil
+		return &BadTestRequestError{reason}
 	}
 	manager := args.crash.Manager
 	for _, ns := range config.Namespaces {
 		if mgr, ok := ns.Managers[manager]; ok {
 			if mgr.RestrictedTestingRepo != "" && args.repo != mgr.RestrictedTestingRepo {
-				return mgr.RestrictedTestingReason, nil
+				return &BadTestRequestError{mgr.RestrictedTestingReason}
 			}
 			if mgr.Decommissioned {
 				manager = mgr.DelegatedTo
@@ -109,7 +109,7 @@ func addTestJob(c context.Context, args *testJobArgs, now time.Time) (string, er
 	}
 	patchID, err := putText(c, args.bug.Namespace, textPatch, []byte(args.patch), false)
 	if err != nil {
-		return "", err
+		return err
 	}
 	reportingName := ""
 	if args.bugReporting != nil {
@@ -177,9 +177,9 @@ func addTestJob(c context.Context, args *testJobArgs, now time.Time) (string, er
 		}
 	}
 	if err != nil {
-		return "", fmt.Errorf("job tx failed: %v", err)
+		return fmt.Errorf("job tx failed: %v", err)
 	}
-	return "", nil
+	return nil
 }
 
 func checkTestJob(c context.Context, bug *Bug, bugReporting *BugReporting, crash *Crash,
@@ -204,6 +204,22 @@ func checkTestJob(c context.Context, bug *Bug, bugReporting *BugReporting, crash
 	return ""
 }
 
+type BadTestRequestError struct {
+	message string
+}
+
+func (e *BadTestRequestError) Error() string {
+	return e.message
+}
+
+type TestRequestDeniedError struct {
+	message string
+}
+
+func (e *TestRequestDeniedError) Error() string {
+	return e.message
+}
+
 // pollPendingJobs returns the next job to execute for the provided list of managers.
 func pollPendingJobs(c context.Context, managers map[string]dashapi.ManagerJobs) (
 	*dashapi.JobPollResp, error) {
@@ -303,7 +319,7 @@ func handleRetestForBug(c context.Context, now time.Time, bug *Bug, bugKey *db.K
 		if err != nil {
 			return err
 		}
-		reason, err := addTestJob(c, &testJobArgs{
+		err = addTestJob(c, &testJobArgs{
 			crash:     crash,
 			crashKey:  crashKeys[crashID],
 			configRef: build.KernelConfig,
@@ -314,9 +330,6 @@ func handleRetestForBug(c context.Context, now time.Time, bug *Bug, bugKey *db.K
 				branch: build.KernelBranch,
 			},
 		}, now)
-		if reason != "" {
-			return fmt.Errorf("job not added for reason %s", reason)
-		}
 		if err != nil {
 			return fmt.Errorf("failed to add job: %w", err)
 		}
diff --git a/dashboard/app/jobs_test.go b/dashboard/app/jobs_test.go
index 23481f2bf..034efe2f1 100644
--- a/dashboard/app/jobs_test.go
+++ b/dashboard/app/jobs_test.go
@@ -46,6 +46,7 @@ func TestJob(t *testing.T) {
 	c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+sampleGitPatch,
 		EmailOptFrom("test@requester.com"), EmailOptCC([]string{mailingList}))
 	body := c.pollEmailBug().Body
+	t.Logf("body: %s", body)
 	c.expectEQ(strings.Contains(body, "This crash does not have a reproducer"), true)
 
 	// Report crash with repro.
diff --git a/dashboard/app/reporting_email.go b/dashboard/app/reporting_email.go
index 47c6108d6..087f6185f 100644
--- a/dashboard/app/reporting_email.go
+++ b/dashboard/app/reporting_email.go
@@ -388,10 +388,23 @@ func handleTestCommand(c context.Context, info *bugInfoResult, msg *email.Email)
 		return replyTo(c, msg, info.bugReporting.ID,
 			fmt.Sprintf("want 2 args (repo, branch), got %v", len(args)))
 	}
-	reply := handleTestRequest(c, &testReqArgs{
+	reply := ""
+	err := handleTestRequest(c, &testReqArgs{
 		bug: info.bug, bugKey: info.bugKey, bugReporting: info.bugReporting,
 		user: msg.Author, extID: msg.MessageID, link: msg.Link,
 		patch: msg.Patch, repo: args[0], branch: args[1], jobCC: msg.Cc})
+	if err != nil {
+		log.Errorf(c, "failed to handle the test request: %v", err)
+		switch e := err.(type) {
+		case *TestRequestDeniedError:
+			// Don't send a reply in this case.
+		case *BadTestRequestError:
+			reply = e.Error()
+		default:
+			// Don't leak any details to the email.
+			reply = "Processing failed due to an internal error"
+		}
+	}
 	if reply != "" {
 		return replyTo(c, msg, info.bugReporting.ID, reply)
 	}