internal/relui: stop relying on approval task name

Add a column to track whether approval is required for a task. This
relieves us from having to rely on a magic task name for showing the
button in the UI.

Unfortunately, we still need to capture the task name at definition time
in order to query it later on. It may make sense to relay the task's
name inside the TaskContext.

Adds documentation and minor refactoring to Approve tasks.

Updates approval polling interval to 5 seconds.

Fixes golang/go#53295

Change-Id: I46953ed3166944726a34f3f62dc8c4622201922e
Reviewed-on: https://go-review.googlesource.com/c/build/+/413575
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Alex Rakoczy <alex@golang.org>
Auto-Submit: Alex Rakoczy <alex@golang.org>
diff --git a/internal/relui/db/models.go b/internal/relui/db/models.go
index 8377f14..d9dac15 100644
--- a/internal/relui/db/models.go
+++ b/internal/relui/db/models.go
@@ -12,14 +12,15 @@
 )
 
 type Task struct {
-	WorkflowID uuid.UUID
-	Name       string
-	Finished   bool
-	Result     sql.NullString
-	Error      sql.NullString
-	CreatedAt  time.Time
-	UpdatedAt  time.Time
-	ApprovedAt sql.NullTime
+	WorkflowID       uuid.UUID
+	Name             string
+	Finished         bool
+	Result           sql.NullString
+	Error            sql.NullString
+	CreatedAt        time.Time
+	UpdatedAt        time.Time
+	ApprovedAt       sql.NullTime
+	ReadyForApproval bool
 }
 
 type TaskLog struct {
diff --git a/internal/relui/db/workflows.sql.go b/internal/relui/db/workflows.sql.go
index 9ea56e8..ad56026 100644
--- a/internal/relui/db/workflows.sql.go
+++ b/internal/relui/db/workflows.sql.go
@@ -16,10 +16,10 @@
 const approveTask = `-- name: ApproveTask :one
 UPDATE tasks
 SET approved_at = $3,
-    updated_at = $3
+    updated_at  = $3
 WHERE workflow_id = $1
   AND name = $2
-RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at
+RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at, ready_for_approval
 `
 
 type ApproveTaskParams struct {
@@ -40,25 +40,27 @@
 		&i.CreatedAt,
 		&i.UpdatedAt,
 		&i.ApprovedAt,
+		&i.ReadyForApproval,
 	)
 	return i, err
 }
 
 const createTask = `-- name: CreateTask :one
-INSERT INTO tasks (workflow_id, name, finished, result, error, created_at, updated_at, approved_at)
-VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
-RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at
+INSERT INTO tasks (workflow_id, name, finished, result, error, created_at, updated_at, approved_at, ready_for_approval)
+VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)
+RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at, ready_for_approval
 `
 
 type CreateTaskParams struct {
-	WorkflowID uuid.UUID
-	Name       string
-	Finished   bool
-	Result     sql.NullString
-	Error      sql.NullString
-	CreatedAt  time.Time
-	UpdatedAt  time.Time
-	ApprovedAt sql.NullTime
+	WorkflowID       uuid.UUID
+	Name             string
+	Finished         bool
+	Result           sql.NullString
+	Error            sql.NullString
+	CreatedAt        time.Time
+	UpdatedAt        time.Time
+	ApprovedAt       sql.NullTime
+	ReadyForApproval bool
 }
 
 func (q *Queries) CreateTask(ctx context.Context, arg CreateTaskParams) (Task, error) {
@@ -71,6 +73,7 @@
 		arg.CreatedAt,
 		arg.UpdatedAt,
 		arg.ApprovedAt,
+		arg.ReadyForApproval,
 	)
 	var i Task
 	err := row.Scan(
@@ -82,6 +85,7 @@
 		&i.CreatedAt,
 		&i.UpdatedAt,
 		&i.ApprovedAt,
+		&i.ReadyForApproval,
 	)
 	return i, err
 }
@@ -150,13 +154,13 @@
 
 const resetTask = `-- name: ResetTask :one
 UPDATE tasks
-SET finished   = false,
+SET finished   = FALSE,
     result     = DEFAULT,
     error      = DEFAULT,
     updated_at = $3
 WHERE workflow_id = $1
   AND name = $2
-RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at
+RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at, ready_for_approval
 `
 
 type ResetTaskParams struct {
@@ -177,13 +181,14 @@
 		&i.CreatedAt,
 		&i.UpdatedAt,
 		&i.ApprovedAt,
+		&i.ReadyForApproval,
 	)
 	return i, err
 }
 
 const resetWorkflow = `-- name: ResetWorkflow :one
 UPDATE workflows
-SET finished   = false,
+SET finished   = FALSE,
     output     = DEFAULT,
     error      = DEFAULT,
     updated_at = $2
@@ -213,7 +218,7 @@
 }
 
 const task = `-- name: Task :one
-SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at
+SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at, tasks.ready_for_approval
 FROM tasks
 WHERE workflow_id = $1
   AND name = $2
@@ -237,6 +242,7 @@
 		&i.CreatedAt,
 		&i.UpdatedAt,
 		&i.ApprovedAt,
+		&i.ReadyForApproval,
 	)
 	return i, err
 }
@@ -315,7 +321,7 @@
 }
 
 const tasks = `-- name: Tasks :many
-SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at
+SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at, tasks.ready_for_approval
 FROM tasks
 ORDER BY updated_at
 `
@@ -338,6 +344,7 @@
 			&i.CreatedAt,
 			&i.UpdatedAt,
 			&i.ApprovedAt,
+			&i.ReadyForApproval,
 		); err != nil {
 			return nil, err
 		}
@@ -350,7 +357,7 @@
 }
 
 const tasksForWorkflow = `-- name: TasksForWorkflow :many
-SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at
+SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at, tasks.ready_for_approval
 FROM tasks
 WHERE workflow_id = $1
 ORDER BY created_at
@@ -374,6 +381,7 @@
 			&i.CreatedAt,
 			&i.UpdatedAt,
 			&i.ApprovedAt,
+			&i.ReadyForApproval,
 		); err != nil {
 			return nil, err
 		}
@@ -388,7 +396,7 @@
 const unfinishedWorkflows = `-- name: UnfinishedWorkflows :many
 SELECT workflows.id, workflows.params, workflows.name, workflows.created_at, workflows.updated_at, workflows.finished, workflows.output, workflows.error
 FROM workflows
-WHERE workflows.finished = false
+WHERE workflows.finished = FALSE
 `
 
 func (q *Queries) UnfinishedWorkflows(ctx context.Context) ([]Workflow, error) {
@@ -420,6 +428,37 @@
 	return items, nil
 }
 
+const updateTaskReadyForApproval = `-- name: UpdateTaskReadyForApproval :one
+UPDATE tasks
+SET ready_for_approval = $3
+WHERE workflow_id = $1
+  AND name = $2
+RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at, ready_for_approval
+`
+
+type UpdateTaskReadyForApprovalParams struct {
+	WorkflowID       uuid.UUID
+	Name             string
+	ReadyForApproval bool
+}
+
+func (q *Queries) UpdateTaskReadyForApproval(ctx context.Context, arg UpdateTaskReadyForApprovalParams) (Task, error) {
+	row := q.db.QueryRow(ctx, updateTaskReadyForApproval, arg.WorkflowID, arg.Name, arg.ReadyForApproval)
+	var i Task
+	err := row.Scan(
+		&i.WorkflowID,
+		&i.Name,
+		&i.Finished,
+		&i.Result,
+		&i.Error,
+		&i.CreatedAt,
+		&i.UpdatedAt,
+		&i.ApprovedAt,
+		&i.ReadyForApproval,
+	)
+	return i, err
+}
+
 const upsertTask = `-- name: UpsertTask :one
 INSERT INTO tasks (workflow_id, name, finished, result, error, created_at, updated_at)
 VALUES ($1, $2, $3, $4, $5, $6, $7)
@@ -430,7 +469,7 @@
         result      = excluded.result,
         error       = excluded.error,
         updated_at  = excluded.updated_at
-RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at
+RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at, ready_for_approval
 `
 
 type UpsertTaskParams struct {
@@ -463,6 +502,7 @@
 		&i.CreatedAt,
 		&i.UpdatedAt,
 		&i.ApprovedAt,
+		&i.ReadyForApproval,
 	)
 	return i, err
 }
diff --git a/internal/relui/migrations/20220622151222_add_ready_for_approval_to_tasks.down.sql b/internal/relui/migrations/20220622151222_add_ready_for_approval_to_tasks.down.sql
new file mode 100644
index 0000000..00e1ad6
--- /dev/null
+++ b/internal/relui/migrations/20220622151222_add_ready_for_approval_to_tasks.down.sql
@@ -0,0 +1,7 @@
+-- Copyright 2022 The Go Authors. All rights reserved.
+-- Use of this source code is governed by a BSD-style
+-- license that can be found in the LICENSE file.
+
+-- Back-filling the name prefix would break workflows, as names are
+-- significant.
+ALTER TABLE tasks DROP COLUMN ready_for_approval;
diff --git a/internal/relui/migrations/20220622151222_add_ready_for_approval_to_tasks.up.sql b/internal/relui/migrations/20220622151222_add_ready_for_approval_to_tasks.up.sql
new file mode 100644
index 0000000..694222c
--- /dev/null
+++ b/internal/relui/migrations/20220622151222_add_ready_for_approval_to_tasks.up.sql
@@ -0,0 +1,10 @@
+-- Copyright 2022 The Go Authors. All rights reserved.
+-- Use of this source code is governed by a BSD-style
+-- license that can be found in the LICENSE file.
+
+ALTER TABLE tasks
+    ADD COLUMN ready_for_approval bool NOT NULL DEFAULT FALSE;
+
+UPDATE tasks
+SET ready_for_approval = TRUE
+WHERE SUBSTRING(name FROM '^APPROVE-') != '';
diff --git a/internal/relui/queries/workflows.sql b/internal/relui/queries/workflows.sql
index f85926b..85f6a21 100644
--- a/internal/relui/queries/workflows.sql
+++ b/internal/relui/queries/workflows.sql
@@ -18,8 +18,8 @@
 RETURNING *;
 
 -- name: CreateTask :one
-INSERT INTO tasks (workflow_id, name, finished, result, error, created_at, updated_at, approved_at)
-VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
+INSERT INTO tasks (workflow_id, name, finished, result, error, created_at, updated_at, approved_at, ready_for_approval)
+VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)
 RETURNING *;
 
 -- name: UpsertTask :one
@@ -72,7 +72,7 @@
 -- name: UnfinishedWorkflows :many
 SELECT workflows.*
 FROM workflows
-WHERE workflows.finished = false;
+WHERE workflows.finished = FALSE;
 
 -- name: WorkflowFinished :one
 UPDATE workflows
@@ -85,7 +85,7 @@
 
 -- name: ResetTask :one
 UPDATE tasks
-SET finished   = false,
+SET finished   = FALSE,
     result     = DEFAULT,
     error      = DEFAULT,
     updated_at = $3
@@ -95,7 +95,7 @@
 
 -- name: ResetWorkflow :one
 UPDATE workflows
-SET finished   = false,
+SET finished   = FALSE,
     output     = DEFAULT,
     error      = DEFAULT,
     updated_at = $2
@@ -105,7 +105,14 @@
 -- name: ApproveTask :one
 UPDATE tasks
 SET approved_at = $3,
-    updated_at = $3
+    updated_at  = $3
+WHERE workflow_id = $1
+  AND name = $2
+RETURNING *;
+
+-- name: UpdateTaskReadyForApproval :one
+UPDATE tasks
+SET ready_for_approval = $3
 WHERE workflow_id = $1
   AND name = $2
 RETURNING *;
diff --git a/internal/relui/templates/task_list.html b/internal/relui/templates/task_list.html
index 48cfc4f..b5df9c5 100644
--- a/internal/relui/templates/task_list.html
+++ b/internal/relui/templates/task_list.html
@@ -69,7 +69,7 @@
                 </form>
               </div>
             {{end}}
-            {{if and (not $task.ApprovedAt.Valid) (hasPrefix $task.Name "APPROVE-")}}
+            {{if and (not $task.ApprovedAt.Valid) ($task.ReadyForApproval)}}
               <div class="TaskList-approveTask">
                 <form action="{{baseLink (printf "/workflows/%s/tasks/%s/approve" $workflow.ID $task.Name)}}" method="post">
                   <input type="hidden" id="workflow.id" name="workflow.id" value="{{$workflow.ID}}" />
diff --git a/internal/relui/web_test.go b/internal/relui/web_test.go
index 25b8f8c..6060e73 100644
--- a/internal/relui/web_test.go
+++ b/internal/relui/web_test.go
@@ -592,7 +592,7 @@
 			wantCode: http.StatusNotFound,
 			want: db.Task{
 				WorkflowID: wfID,
-				Name:       "APPROVE-please",
+				Name:       "approve please",
 				CreatedAt:  hourAgo,
 				UpdatedAt:  hourAgo,
 			},
@@ -603,7 +603,7 @@
 			wantCode: http.StatusBadRequest,
 			want: db.Task{
 				WorkflowID: wfID,
-				Name:       "APPROVE-please",
+				Name:       "approve please",
 				CreatedAt:  hourAgo,
 				UpdatedAt:  hourAgo,
 			},
@@ -614,7 +614,7 @@
 			wantCode: http.StatusNotFound,
 			want: db.Task{
 				WorkflowID: wfID,
-				Name:       "APPROVE-please",
+				Name:       "approve please",
 				CreatedAt:  hourAgo,
 				UpdatedAt:  hourAgo,
 			},
@@ -625,21 +625,21 @@
 			wantCode: http.StatusNotFound,
 			want: db.Task{
 				WorkflowID: wfID,
-				Name:       "APPROVE-please",
+				Name:       "approve please",
 				CreatedAt:  hourAgo,
 				UpdatedAt:  hourAgo,
 			},
 		},
 		{
 			desc:     "successful approval",
-			params:   map[string]string{"id": wfID.String(), "name": "APPROVE-please"},
+			params:   map[string]string{"id": wfID.String(), "name": "approve please"},
 			wantCode: http.StatusSeeOther,
 			wantHeaders: map[string]string{
 				"Location": "/",
 			},
 			want: db.Task{
 				WorkflowID: wfID,
-				Name:       "APPROVE-please",
+				Name:       "approve please",
 				CreatedAt:  hourAgo,
 				UpdatedAt:  time.Now(),
 				ApprovedAt: sql.NullTime{Time: time.Now(), Valid: true},
@@ -663,7 +663,7 @@
 			}
 			gtg := db.CreateTaskParams{
 				WorkflowID: wf.ID,
-				Name:       "APPROVE-please",
+				Name:       "approve please",
 				Finished:   false,
 				CreatedAt:  hourAgo,
 				UpdatedAt:  hourAgo,
@@ -672,7 +672,7 @@
 				t.Fatalf("CreateTask(_, %v) = _, %v, wanted no error", gtg, err)
 			}
 
-			req := httptest.NewRequest(http.MethodPost, path.Join("/workflows/", c.params["id"], "tasks", c.params["name"], "approve"), nil)
+			req := httptest.NewRequest(http.MethodPost, path.Join("/workflows/", c.params["id"], "tasks", url.PathEscape(c.params["name"]), "approve"), nil)
 			req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 			rec := httptest.NewRecorder()
 			s := NewServer(p, NewWorker(NewDefinitionHolder(), p, &PGListener{p}), nil, SiteHeader{})
@@ -693,7 +693,7 @@
 			}
 			task, err := q.Task(ctx, db.TaskParams{
 				WorkflowID: wf.ID,
-				Name:       "APPROVE-please",
+				Name:       "approve please",
 			})
 			if err != nil {
 				t.Fatalf("q.Task() = %v, %v, wanted no error", task, err)
diff --git a/internal/relui/workflows.go b/internal/relui/workflows.go
index 6b29be5..a6cf9c3 100644
--- a/internal/relui/workflows.go
+++ b/internal/relui/workflows.go
@@ -478,11 +478,21 @@
 	}
 }
 
-// AwaitAction is a workflow.Action that is a convenience wrapper
+// AwaitAction returns a workflow.Action that is a convenience wrapper
 // around AwaitFunc.
-func AwaitAction(ctx *workflow.TaskContext, period time.Duration, awaitCondition AwaitConditionFunc) error {
-	_, err := AwaitFunc(ctx, period, awaitCondition)
-	return err
+func AwaitAction(period time.Duration, awaitCondition AwaitConditionFunc) func(*workflow.TaskContext) error {
+	return func(ctx *workflow.TaskContext) error {
+		_, err := AwaitFunc(ctx, period, awaitCondition)
+		return err
+	}
+}
+
+// AwaitActionDep returns a workflow.Action with a single dependency
+// that is a convenience wrapper around AwaitFunc.
+func AwaitActionDep(period time.Duration, awaitCondition AwaitConditionFunc) func(*workflow.TaskContext, interface{}) error {
+	return func(ctx *workflow.TaskContext, _ interface{}) error {
+		return AwaitAction(period, awaitCondition)(ctx)
+	}
 }
 
 func checkTaskApproved(ctx *workflow.TaskContext, p *pgxpool.Pool, taskName string) (bool, error) {
@@ -491,20 +501,39 @@
 		WorkflowID: ctx.WorkflowID,
 		Name:       taskName,
 	})
+	if !t.ReadyForApproval {
+		_, err := q.UpdateTaskReadyForApproval(ctx, db.UpdateTaskReadyForApprovalParams{
+			ReadyForApproval: true,
+			Name:             taskName,
+			WorkflowID:       ctx.WorkflowID,
+		})
+		if err != nil {
+			return false, err
+		}
+	}
 	return t.ApprovedAt.Valid, err
 }
 
-func approveActionDep(p *pgxpool.Pool, taskName string) func(*workflow.TaskContext, interface{}) error {
-	return func(ctx *workflow.TaskContext, _ interface{}) error {
-		return AwaitAction(ctx, 10*time.Second, func(ctx *workflow.TaskContext) (done bool, err error) {
-			return checkTaskApproved(ctx, p, taskName)
-		})
-	}
-}
-
+// ApproveActionDep returns a function for defining approval Actions.
+//
+// ApproveActionDep takes a single *pgxpool.Pool argument, which is
+// used to query the database to determine if a task has been marked
+// approved. ApproveActionDep returns a function that accepts a single
+// string argument that must match the name of the task being defined.
+// The returned function is suitable for a single workflow Action with
+// a single dependency.
+//
+// ApproveActionDep marks the task as requiring approval in the
+// database once the task is started. This can be used to show an
+// "approve" control in the UI.
+//
+//	actionName := "Wait for Approval"
+//	waitAction := wd.Action(actionName, ApproveActionDep(db)(actionName), someDependency)
 func ApproveActionDep(p *pgxpool.Pool) func(taskName string) func(*workflow.TaskContext, interface{}) error {
 	return func(taskName string) func(*workflow.TaskContext, interface{}) error {
-		return approveActionDep(p, taskName)
+		return AwaitActionDep(5*time.Second, func(ctx *workflow.TaskContext) (done bool, err error) {
+			return checkTaskApproved(ctx, p, taskName)
+		})
 	}
 }
 
@@ -574,7 +603,7 @@
 		return err
 	}
 
-	verifiedName := "APPROVE-Wait for Release Coordinator Approval"
+	verifiedName := "Wait for Release Coordinator Approval"
 	verified := wd.Action(verifiedName, build.ApproveActionFunc(verifiedName), signedAndTestedArtifacts)
 
 	// Tag version and upload to CDN/website.
diff --git a/internal/relui/workflows_test.go b/internal/relui/workflows_test.go
index b81f977..163eb3e 100644
--- a/internal/relui/workflows_test.go
+++ b/internal/relui/workflows_test.go
@@ -114,7 +114,7 @@
 	}
 	gtg := db.CreateTaskParams{
 		WorkflowID: wf.ID,
-		Name:       "APPROVE-please",
+		Name:       "approve please",
 		Finished:   true,
 		Error:      nullString("internal explosion"),
 		CreatedAt:  hourAgo,
@@ -129,6 +129,14 @@
 	if err != nil || got {
 		t.Errorf("checkTaskApproved(_, %v, %q) = %t, %v wanted %t, %v", p, gtg.Name, got, err, false, nil)
 	}
+	tp := db.TaskParams{WorkflowID: wf.ID, Name: gtg.Name}
+	task, err := q.Task(ctx, tp)
+	if err != nil {
+		t.Fatalf("q.Task(_, %v) = %v, %v, wanted no error", tp, task, err)
+	}
+	if !task.ReadyForApproval {
+		t.Errorf("task.ReadyForApproval = %v, wanted %v", task.ReadyForApproval, true)
+	}
 
 	atp := db.ApproveTaskParams{
 		WorkflowID: wf.ID,