internal/relui: add approved_at to tasks
This change adds an approved_at column to the tasks table for checking
task approval, replacing the flimsy log-based determination for whether
a task has been approved.
Updates golang/go#53295
Change-Id: I3c2bd73134dc4a81e13c328aa085065a7b24a5d5
Reviewed-on: https://go-review.googlesource.com/c/build/+/413427
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Alex Rakoczy <alex@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Auto-Submit: Alex Rakoczy <alex@golang.org>
diff --git a/internal/relui/db/models.go b/internal/relui/db/models.go
index c376a03..8377f14 100644
--- a/internal/relui/db/models.go
+++ b/internal/relui/db/models.go
@@ -19,6 +19,7 @@
Error sql.NullString
CreatedAt time.Time
UpdatedAt time.Time
+ ApprovedAt sql.NullTime
}
type TaskLog struct {
diff --git a/internal/relui/db/workflows.sql.go b/internal/relui/db/workflows.sql.go
index ed512da..9ea56e8 100644
--- a/internal/relui/db/workflows.sql.go
+++ b/internal/relui/db/workflows.sql.go
@@ -13,10 +13,41 @@
"github.com/google/uuid"
)
+const approveTask = `-- name: ApproveTask :one
+UPDATE tasks
+SET approved_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
+`
+
+type ApproveTaskParams struct {
+ WorkflowID uuid.UUID
+ Name string
+ ApprovedAt sql.NullTime
+}
+
+func (q *Queries) ApproveTask(ctx context.Context, arg ApproveTaskParams) (Task, error) {
+ row := q.db.QueryRow(ctx, approveTask, arg.WorkflowID, arg.Name, arg.ApprovedAt)
+ var i Task
+ err := row.Scan(
+ &i.WorkflowID,
+ &i.Name,
+ &i.Finished,
+ &i.Result,
+ &i.Error,
+ &i.CreatedAt,
+ &i.UpdatedAt,
+ &i.ApprovedAt,
+ )
+ return i, err
+}
+
const createTask = `-- name: CreateTask :one
-INSERT INTO tasks (workflow_id, name, finished, result, error, created_at, updated_at)
-VALUES ($1, $2, $3, $4, $5, $6, $7)
-RETURNING workflow_id, name, finished, result, error, created_at, updated_at
+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
`
type CreateTaskParams struct {
@@ -27,6 +58,7 @@
Error sql.NullString
CreatedAt time.Time
UpdatedAt time.Time
+ ApprovedAt sql.NullTime
}
func (q *Queries) CreateTask(ctx context.Context, arg CreateTaskParams) (Task, error) {
@@ -38,6 +70,7 @@
arg.Error,
arg.CreatedAt,
arg.UpdatedAt,
+ arg.ApprovedAt,
)
var i Task
err := row.Scan(
@@ -48,6 +81,7 @@
&i.Error,
&i.CreatedAt,
&i.UpdatedAt,
+ &i.ApprovedAt,
)
return i, err
}
@@ -122,7 +156,7 @@
updated_at = $3
WHERE workflow_id = $1
AND name = $2
-RETURNING workflow_id, name, finished, result, error, created_at, updated_at
+RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at
`
type ResetTaskParams struct {
@@ -142,6 +176,7 @@
&i.Error,
&i.CreatedAt,
&i.UpdatedAt,
+ &i.ApprovedAt,
)
return i, err
}
@@ -178,7 +213,7 @@
}
const task = `-- name: Task :one
-SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at
+SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at
FROM tasks
WHERE workflow_id = $1
AND name = $2
@@ -201,6 +236,7 @@
&i.Error,
&i.CreatedAt,
&i.UpdatedAt,
+ &i.ApprovedAt,
)
return i, err
}
@@ -279,7 +315,7 @@
}
const tasks = `-- name: Tasks :many
-SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at
+SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at
FROM tasks
ORDER BY updated_at
`
@@ -301,6 +337,7 @@
&i.Error,
&i.CreatedAt,
&i.UpdatedAt,
+ &i.ApprovedAt,
); err != nil {
return nil, err
}
@@ -313,7 +350,7 @@
}
const tasksForWorkflow = `-- name: TasksForWorkflow :many
-SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at
+SELECT tasks.workflow_id, tasks.name, tasks.finished, tasks.result, tasks.error, tasks.created_at, tasks.updated_at, tasks.approved_at
FROM tasks
WHERE workflow_id = $1
ORDER BY created_at
@@ -336,6 +373,7 @@
&i.Error,
&i.CreatedAt,
&i.UpdatedAt,
+ &i.ApprovedAt,
); err != nil {
return nil, err
}
@@ -392,7 +430,7 @@
result = excluded.result,
error = excluded.error,
updated_at = excluded.updated_at
-RETURNING workflow_id, name, finished, result, error, created_at, updated_at
+RETURNING workflow_id, name, finished, result, error, created_at, updated_at, approved_at
`
type UpsertTaskParams struct {
@@ -424,6 +462,7 @@
&i.Error,
&i.CreatedAt,
&i.UpdatedAt,
+ &i.ApprovedAt,
)
return i, err
}
diff --git a/internal/relui/migrations/20220621201140_add_approved_at_to_tasks.down.sql b/internal/relui/migrations/20220621201140_add_approved_at_to_tasks.down.sql
new file mode 100644
index 0000000..62eaa1b
--- /dev/null
+++ b/internal/relui/migrations/20220621201140_add_approved_at_to_tasks.down.sql
@@ -0,0 +1,29 @@
+-- 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.
+
+BEGIN;
+
+WITH approved_task_logs AS (
+ SELECT DISTINCT workflow_id, task_name
+ FROM task_logs
+ WHERE body = 'USER-APPROVED'
+)
+
+INSERT
+INTO task_logs (workflow_id, task_name, body, created_at, updated_at)
+SELECT tasks.workflow_id, tasks.name, 'USER-APPROVED', tasks.approved_at, tasks.approved_at
+FROM tasks
+WHERE tasks.approved_at IS NOT NULL
+ AND NOT EXISTS(
+ SELECT DISTINCT 1
+ FROM approved_task_logs atl
+ WHERE atl.workflow_id = tasks.workflow_id
+ AND atl.task_name = tasks.name
+ )
+;
+
+ALTER TABLE tasks
+ DROP COLUMN approved_at;
+
+COMMIT;
diff --git a/internal/relui/migrations/20220621201140_add_approved_at_to_tasks.up.sql b/internal/relui/migrations/20220621201140_add_approved_at_to_tasks.up.sql
new file mode 100644
index 0000000..0e51d8d
--- /dev/null
+++ b/internal/relui/migrations/20220621201140_add_approved_at_to_tasks.up.sql
@@ -0,0 +1,23 @@
+-- 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.
+
+BEGIN;
+
+ALTER TABLE tasks
+ ADD COLUMN approved_at timestamp WITH TIME ZONE NULL;
+
+WITH approved_tasks AS (
+ SELECT workflow_id, task_name, max(created_at) AS created_at
+ FROM task_logs
+ WHERE body = 'USER-APPROVED'
+ GROUP BY workflow_id, task_name
+)
+
+UPDATE tasks
+SET approved_at = approved_tasks.created_at
+FROM approved_tasks
+WHERE tasks.workflow_id = approved_tasks.workflow_id
+ AND tasks.name = approved_tasks.task_name;
+
+COMMIT;
diff --git a/internal/relui/queries/workflows.sql b/internal/relui/queries/workflows.sql
index 09c8e52..f85926b 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)
-VALUES ($1, $2, $3, $4, $5, $6, $7)
+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 *;
-- name: UpsertTask :one
@@ -101,3 +101,11 @@
updated_at = $2
WHERE id = $1
RETURNING *;
+
+-- name: ApproveTask :one
+UPDATE tasks
+SET approved_at = $3,
+ updated_at = $3
+WHERE workflow_id = $1
+ AND name = $2
+RETURNING *;
diff --git a/internal/relui/static/styles.css b/internal/relui/static/styles.css
index 56a0469..cc77db1 100644
--- a/internal/relui/static/styles.css
+++ b/internal/relui/static/styles.css
@@ -231,6 +231,11 @@
color: white;
padding: 0.5rem 1rem;
}
+.TaskList-itemLogLineApproved {
+ background-color: #3b65b3;
+ color: white;
+ padding: 0.5rem 1rem;
+}
.TaskList-itemHeader {
align-items: center;
font-size: 0.8125rem;
diff --git a/internal/relui/templates/task_list.html b/internal/relui/templates/task_list.html
index c198478..48cfc4f 100644
--- a/internal/relui/templates/task_list.html
+++ b/internal/relui/templates/task_list.html
@@ -54,7 +54,11 @@
{{$task.UpdatedAt.UTC.Format "Mon Jan _2 2006 15:04:05"}}
</td>
<td class="TaskList-itemCol TaskList-itemResult">
- {{ $resultDetail.Kind }}
+ {{if $task.ApprovedAt.Valid}}
+ Approved
+ {{else}}
+ {{ $resultDetail.Kind }}
+ {{end}}
</td>
<td class="TaskList-itemCol TaskList-itemAction">
{{if $task.Error.Valid}}
@@ -65,7 +69,7 @@
</form>
</div>
{{end}}
- {{if and (not $task.Finished) (hasPrefix $task.Name "APPROVE-")}}
+ {{if and (not $task.ApprovedAt.Valid) (hasPrefix $task.Name "APPROVE-")}}
<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}}" />
@@ -82,6 +86,11 @@
{{- $task.Error.Value -}}
</div>
{{end}}
+ {{if $task.ApprovedAt.Valid}}
+ <div class="TaskList-itemLogLine TaskList-itemLogLineApproved">
+ {{- printf "Approved at: %s" ($task.ApprovedAt.Value.UTC.Format "2006/01/02 15:04:05") -}}
+ </div>
+ {{end}}
{{range $log := index $.TaskLogs $task.Name}}
<div class="TaskList-itemLogLine">
{{- printf "%s %s" ($log.CreatedAt.UTC.Format "2006/01/02 15:04:05") $log.Body -}}
diff --git a/internal/relui/web.go b/internal/relui/web.go
index b44de81..651763e 100644
--- a/internal/relui/web.go
+++ b/internal/relui/web.go
@@ -329,16 +329,19 @@
return
}
q := db.New(s.db)
- t, err := q.Task(r.Context(), db.TaskParams{WorkflowID: id, Name: params.ByName("name")})
+ t, err := q.ApproveTask(r.Context(), db.ApproveTaskParams{
+ WorkflowID: id,
+ Name: params.ByName("name"),
+ ApprovedAt: sql.NullTime{Time: time.Now(), Valid: true},
+ })
if errors.Is(err, sql.ErrNoRows) || errors.Is(err, pgx.ErrNoRows) {
http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
return
} else if err != nil {
- log.Printf("q.Task(_, %q): %v", id, err)
+ log.Printf("q.ApproveTask(_, %q) = %v, %v", id, t, err)
http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
return
}
- // This log entry serves as approval.
s.w.l.Logger(id, t.Name).Printf("USER-APPROVED")
http.Redirect(w, r, s.BaseLink("/"), http.StatusSeeOther)
}
diff --git a/internal/relui/web_test.go b/internal/relui/web_test.go
index c3c72f0..25b8f8c 100644
--- a/internal/relui/web_test.go
+++ b/internal/relui/web_test.go
@@ -585,41 +585,65 @@
params map[string]string
wantCode int
wantHeaders map[string]string
- wantLogs []db.TaskLog
+ want db.Task
}{
{
desc: "no params",
wantCode: http.StatusNotFound,
+ want: db.Task{
+ WorkflowID: wfID,
+ Name: "APPROVE-please",
+ CreatedAt: hourAgo,
+ UpdatedAt: hourAgo,
+ },
},
{
desc: "invalid workflow id",
params: map[string]string{"id": "invalid", "name": "greeting"},
wantCode: http.StatusBadRequest,
+ want: db.Task{
+ WorkflowID: wfID,
+ Name: "APPROVE-please",
+ CreatedAt: hourAgo,
+ UpdatedAt: hourAgo,
+ },
},
{
desc: "wrong workflow id",
params: map[string]string{"id": uuid.New().String(), "name": "greeting"},
wantCode: http.StatusNotFound,
+ want: db.Task{
+ WorkflowID: wfID,
+ Name: "APPROVE-please",
+ CreatedAt: hourAgo,
+ UpdatedAt: hourAgo,
+ },
},
{
desc: "invalid task name",
params: map[string]string{"id": wfID.String(), "name": "invalid"},
wantCode: http.StatusNotFound,
+ want: db.Task{
+ WorkflowID: wfID,
+ Name: "APPROVE-please",
+ CreatedAt: hourAgo,
+ UpdatedAt: hourAgo,
+ },
},
{
- desc: "successful reset",
+ desc: "successful approval",
params: map[string]string{"id": wfID.String(), "name": "APPROVE-please"},
wantCode: http.StatusSeeOther,
wantHeaders: map[string]string{
"Location": "/",
},
- wantLogs: []db.TaskLog{{
+ want: db.Task{
WorkflowID: wfID,
- TaskName: "APPROVE-please",
- Body: "USER-APPROVED",
- CreatedAt: time.Now(),
+ Name: "APPROVE-please",
+ CreatedAt: hourAgo,
UpdatedAt: time.Now(),
- }},
+ ApprovedAt: sql.NullTime{Time: time.Now(), Valid: true},
+ },
},
}
for _, c := range cases {
@@ -640,8 +664,7 @@
gtg := db.CreateTaskParams{
WorkflowID: wf.ID,
Name: "APPROVE-please",
- Finished: true,
- Error: nullString("internal explosion"),
+ Finished: false,
CreatedAt: hourAgo,
UpdatedAt: hourAgo,
}
@@ -668,15 +691,15 @@
if c.wantCode == http.StatusBadRequest {
return
}
- logs, err := q.TaskLogsForTask(ctx, db.TaskLogsForTaskParams{
- WorkflowID: wfID,
- TaskName: "APPROVE-please",
+ task, err := q.Task(ctx, db.TaskParams{
+ WorkflowID: wf.ID,
+ Name: "APPROVE-please",
})
if err != nil {
- t.Fatalf("q.TaskLogsForTask() = %v, %v, wanted no error", logs, err)
+ t.Fatalf("q.Task() = %v, %v, wanted no error", task, err)
}
- if diff := cmp.Diff(c.wantLogs, logs, SameUUIDVariant(), cmpopts.EquateApproxTime(time.Minute), cmpopts.IgnoreFields(db.TaskLog{}, "ID")); diff != "" {
- t.Fatalf("q.TaskLogsForTask() mismatch (-want +got):\n%s", diff)
+ if diff := cmp.Diff(c.want, task, cmpopts.EquateApproxTime(time.Minute)); diff != "" {
+ t.Fatalf("q.Task() mismatch (-want +got):\n%s", diff)
}
})
}
diff --git a/internal/relui/workflows.go b/internal/relui/workflows.go
index 9990ff5..56ed742 100644
--- a/internal/relui/workflows.go
+++ b/internal/relui/workflows.go
@@ -13,7 +13,6 @@
"math/rand"
"net/http"
"path"
- "strings"
"sync"
"time"
@@ -323,19 +322,11 @@
func checkTaskApproved(ctx *workflow.TaskContext, p *pgxpool.Pool, taskName string) (bool, error) {
q := db.New(p)
- logs, err := q.TaskLogsForTask(ctx, db.TaskLogsForTaskParams{
+ t, err := q.Task(ctx, db.TaskParams{
WorkflowID: ctx.WorkflowID,
- TaskName: taskName,
+ Name: taskName,
})
- if err != nil {
- return false, err
- }
- for _, l := range logs {
- if strings.Contains(l.Body, "USER-APPROVED") {
- return true, nil
- }
- }
- return false, nil
+ return t.ApprovedAt.Valid, err
}
func approveActionDep(p *pgxpool.Pool, taskName string) func(*workflow.TaskContext, interface{}) error {
diff --git a/internal/relui/workflows_test.go b/internal/relui/workflows_test.go
index 4cecad1..b81f977 100644
--- a/internal/relui/workflows_test.go
+++ b/internal/relui/workflows_test.go
@@ -2,6 +2,7 @@
import (
"context"
+ "database/sql"
"errors"
"testing"
"time"
@@ -129,14 +130,14 @@
t.Errorf("checkTaskApproved(_, %v, %q) = %t, %v wanted %t, %v", p, gtg.Name, got, err, false, nil)
}
- ctlp := db.CreateTaskLogParams{
+ atp := db.ApproveTaskParams{
WorkflowID: wf.ID,
- TaskName: gtg.Name,
- Body: "USER-APPROVED",
+ Name: gtg.Name,
+ ApprovedAt: sql.NullTime{Time: time.Now(), Valid: true},
}
- _, err = q.CreateTaskLog(ctx, ctlp)
+ _, err = q.ApproveTask(ctx, atp)
if err != nil {
- t.Errorf("CreateTaskLog(_, %v) = _, %v, wanted no error", ctlp, err)
+ t.Errorf("q.ApproveTask(_, %v) = _, %v, wanted no error", atp, err)
}
got, err = checkTaskApproved(tctx, p, gtg.Name)
