internal/reviews, internal/goreviews: add categories
Split the CL dashboard into categories based on filter expressions.
Use a default set of categories, and also let users change the JSON.
Change-Id: I8651bc00064fd28a36242d37cc7bfc188ae1ea08
Reviewed-on: https://go-review.googlesource.com/c/oscar/+/656575
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
diff --git a/internal/goreviews/changes.go b/internal/goreviews/changes.go
index 1818904..91999ed 100644
--- a/internal/goreviews/changes.go
+++ b/internal/goreviews/changes.go
@@ -9,7 +9,9 @@
"html/template"
"log/slog"
"net/http"
+ "strings"
"sync"
+ "time"
"golang.org/x/oscar/internal/gerrit"
"golang.org/x/oscar/internal/reviews"
@@ -59,5 +61,56 @@
cps := ch.cps
ch.mu.Unlock()
- reviews.Display(ctx, ch.slog, displayDoc, endpoint, cps, w, r)
+ categories := strings.ReplaceAll(categoriesJSON, "$ONEYEARAGO", time.Now().AddDate(-1, 0, 0).Format(`\"2006-01-02\"`))
+
+ reviews.Display(ctx, ch.slog, displayDoc, categories, endpoint, cps, w, r)
}
+
+// categoriesJSON is the default list of ways to categorize issues
+// when displaying them. This is decoded into a []reviews.categoryDef.
+// The string $ONEYEARAGO is replaced before use.
+//
+// Issues are matched against these categories in order, and stop at
+// the first match. That is, an issue that matches "Ready with Comments"
+// will not be compared against "Ready with Merge Conflict".
+// Therefore, the filters do not specifically exclude cases that are
+// handled by earlier categories.
+const categoriesJSON = `
+[
+ {
+ "name": "Ready",
+ "doc": "approved and ready to submit",
+ "filter": "Predicates.Name:hasPlusTwo AND Predicates.Name:trybotsPassed AND NOT Predicates.Name:hasUnresolvedComments AND NOT Predicates.Name:mergeConflict"
+ },
+ {
+ "name": "Ready with Comments",
+ "doc": "approved but has unresolved comments",
+ "filter": "Predicates.Name:hasPlusTwo AND Predicates.Name:trybotsPassed AND NOT Predicates.Name:mergeConflict"
+ },
+ {
+ "name": "Ready with Merge Conflict",
+ "doc": "approved but has a merge conflict",
+ "filter": "Predicates.Name:hasPlusTwo AND Predicates.Name:trybotsPassed"
+ },
+ {
+ "name": "From Maintainer",
+ "doc": "from a maintainer",
+ "filter": "Predicates.Name:authorMaintainer AND Change.Created>$ONEYEARAGO"
+ },
+ {
+ "name": "Older From Maintainer",
+ "doc": "from a maintainer, but created more than a year ago",
+ "filter": "Predicates.Name:authorMaintainer"
+ },
+ {
+ "name": "From Major Contributor",
+ "doc": "from a major contributor",
+ "filter": "Predicates.Name:authorMajorContributor AND Change.Created>$ONEYEARAGO"
+ },
+ {
+ "name": "Older From Major Contributor",
+ "doc": "from a major contributor, but created more than a year ago",
+ "filter": "Predicates.Name:authorMajorContributor"
+ }
+]
+`
diff --git a/internal/reviews/display.go b/internal/reviews/display.go
index e46b925..fa25354 100644
--- a/internal/reviews/display.go
+++ b/internal/reviews/display.go
@@ -5,7 +5,9 @@
package reviews
import (
+ "bytes"
"context"
+ "encoding/json"
"errors"
"fmt"
"html/template"
@@ -19,7 +21,7 @@
// Display is an HTTP handler function that displays the
// changes to review.
-func Display(ctx context.Context, lg *slog.Logger, doc template.HTML, endpoint string, cps []ChangePreds, w http.ResponseWriter, r *http.Request) {
+func Display(ctx context.Context, lg *slog.Logger, doc template.HTML, defaultCategoriesJSON, endpoint string, cps []ChangePreds, w http.ResponseWriter, r *http.Request) {
if len(cps) == 0 {
io.WriteString(w, reportNoData)
return
@@ -32,23 +34,84 @@
return
}
- sc := make([]CP, 0, len(cps))
+ categoriesJSON := r.FormValue("categories")
+ if categoriesJSON == "" {
+ categoriesJSON = defaultCategoriesJSON
+ }
+
+ var categoryDefs []categoryDef
+ if categoriesJSON != "" {
+ if err := json.Unmarshal([]byte(categoriesJSON), &categoryDefs); err != nil {
+ http.Error(w, fmt.Sprintf("can't unmarshal JSON categories %q: %v", categoriesJSON, err), http.StatusBadRequest)
+ return
+ }
+ }
+
+ if len(categoryDefs) == 0 {
+ categoryDefs = []categoryDef{
+ {
+ Name: "",
+ Doc: "",
+ Filter: "",
+ },
+ }
+ } else {
+ categoryDefs = append(categoryDefs,
+ categoryDef{
+ Name: "Remainder",
+ Doc: "CLs not matched by earlier categories",
+ Filter: "",
+ },
+ )
+ }
+
+ categoryFilters := make([]func(context.Context, ChangePreds) bool, 0, len(categoryDefs))
+ for _, catDef := range categoryDefs {
+ fn, err := makeFilter(catDef.Filter)
+ if err != nil {
+ http.Error(w, fmt.Sprintf("invalid category filter %q: %v", catDef.Filter, err), http.StatusBadRequest)
+ return
+ }
+ categoryFilters = append(categoryFilters, fn)
+ }
+
+ categories := make([]category, 0, len(categoryDefs))
+ for _, catDef := range categoryDefs {
+ categories = append(categories, category{
+ Name: catDef.Name,
+ Doc: catDef.Doc,
+ })
+ }
+
for _, v := range cps {
if filterFn != nil && !filterFn(ctx, v) {
continue
}
- sc = append(sc, CP{v})
+ for i := range categories {
+ if categoryFilters[i] == nil || categoryFilters[i](ctx, v) {
+ categories[i].Changes = append(categories[i].Changes, CP{v})
+ break
+ }
+ }
+ }
+
+ var jsonBuf bytes.Buffer
+ if len(categoriesJSON) > 0 {
+ if err := json.Indent(&jsonBuf, []byte(categoriesJSON), "", " "); err != nil {
+ lg.Error("reviews.Display: json.Indent failure", "input", categoriesJSON, "err", err)
+ }
}
data := &displayType{
- Endpoint: endpoint,
- Doc: doc,
- Filter: userFilter,
- Changes: sc,
+ Endpoint: endpoint,
+ Doc: doc,
+ Filter: userFilter,
+ Categories: categories,
+ CategoriesJSON: jsonBuf.String(),
}
if err := displayTemplate.Execute(w, data); err != nil {
- lg.Error("template execution failed", "err", err)
+ lg.Error("reviews.Display: template execution failed", "err", err)
}
}
@@ -72,6 +135,14 @@
return fn, nil
}
+// categoryDef is a category definition.
+// This is a name, documentation, and a predicate.
+type categoryDef struct {
+ Name string `json:"name,omitempty"`
+ Doc string `json:"doc,omitempty"`
+ Filter string `json:"filter,omitempty"`
+}
+
// reportNoData is the HTML that we report if we have not finished
// applying predicates to changes.
const reportNoData = `
@@ -93,16 +164,25 @@
// displayTemplate is used to display the changes.
var displayTemplate = template.Must(template.New("display").Parse(displayHTML))
-// displayType is the type expected by displayHTML
+// displayType is the type expected by displayHTML and displayTemplate.
type displayType struct {
- Ctx context.Context
- Endpoint string // Relative URL being served.
- Doc template.HTML // Documentation string.
- Filter string // Filter value.
- Changes []CP // List of changes with predicates.
+ Ctx context.Context
+ Endpoint string // Relative URL being served.
+ Doc template.HTML // Documentation string.
+ Filter string // Filter value.
+ Categories []category // Changes grouped by category.
+ CategoriesJSON string // Category definitions as JSON string.
}
-// CP is the type we pass to the HTML template displayTemplate.
+// category is a group of changes.
+type category struct {
+ Name string // Name of category.
+ Doc string // Documentation for category.
+ Changes []CP // List of changes with predicates.
+}
+
+// CP is a list of changes. This is not just ChangePreds
+// because we give the template a FormattedLastUpdate method.
type CP struct {
ChangePreds
}
@@ -148,6 +228,15 @@
color: #666;
font-size: .9em;
}
+.categoryname {
+ padding: 1em 0;
+ font-size: 1.5em;
+ font-weight: bold;
+}
+.categorydoc {
+ color: #666;
+ font-size: .9em;
+}
.row {
border-bottom: 1px solid #f1f2f3;
display: flex;
@@ -244,40 +333,54 @@
<a href="https://go.googlesource.com/oscar/+/master/internal/goreviews/display.go">Source code</a>
</div>
</header>
- <div class="filter">
<form id="form" action="{{.Endpoint}}" method="GET">
- <span>
- <label for="filter">Filter</label>
- <input id="filter" type="text" size=75 name="filter" value="{{.Filter}}"/>
- </span>
+ <div class="filter">
+ <span>
+ <label for="filter">Filter</label>
+ <input id="filter" type="text" size=75 name="filter" value="{{.Filter}}"/>
+ </span>
+ </div>
+ <div class="header-subtitle">
+ Sample filters: "Russ Cox" / Change.Author.Name:"rsc" / Change.Subject:"runtime" / Predicates.Name:hasUnresolvedComments / Change.Created > "2024-01-01"
+ </div>
+ {{$ctx := .Ctx}}
+ {{range $category := .Categories}}
+ {{if eq (len .Changes) 0}}{{continue}}{{end}}
+ <div class="categoryname">{{.Name}}</div>
+ <div class="categorydoc">{{.Doc}}</div>
+ {{range $change := .Changes}}
+ <div class="row">
+ <span class="date">{{.FormattedLastUpdate $ctx}}</span>
+ <span class="owner">{{with $author := .Change.Author $ctx}}{{$author.DisplayName $ctx}}{{end}}</span>
+ <a class="number" href="https://go.dev/cl/{{.Change.ID $ctx}}" target="_blank">{{.Change.ID $ctx}}</a>
+ <span class="subject">{{.Change.Subject $ctx}}</span>
+ <span class="predicates">
+ {{range $pred := .Predicates}}
+ {{if ge .Score 10}}
+ <span class="highscore">
+ {{else if gt .Score 0}}
+ <span class="posscore">
+ {{else if le .Score -10}}
+ <span class="lowscore">
+ {{else if lt .Score 0}}
+ <span class="negscore">
+ {{else}}
+ <span class="zeroscore">
+ {{end}}
+ {{.Name}}
+ </span>
+ {{end}}
+ </span>
+ </div>
+ {{end}}
+ {{end}}
+ <div class="categories">
+ <span>
+ <label for="categories">Categories</label>
+ <textarea id="categories" name="categories" rows="9", cols="80">{{.CategoriesJSON}}</textarea>
+ </span>
+ </div>
</form>
- </div>
- {{$ctx := .Ctx}}
- {{range $change := .Changes}}
- <div class="row">
- <span class="date">{{.FormattedLastUpdate $ctx}}</span>
- <span class="owner">{{with $author := .Change.Author $ctx}}{{$author.DisplayName $ctx}}{{end}}</span>
- <a class="number" href="https://go.dev/cl/{{.Change.ID $ctx}}" target="_blank">{{.Change.ID $ctx}}</a>
- <span class="subject">{{.Change.Subject $ctx}}</span>
- <span class="predicates">
- {{range $pred := .Predicates}}
- {{if ge .Score 10}}
- <span class="highscore">
- {{else if gt .Score 0}}
- <span class="posscore">
- {{else if le .Score -10}}
- <span class="lowscore">
- {{else if lt .Score 0}}
- <span class="negscore">
- {{else}}
- <span class="zeroscore">
- {{end}}
- {{.Name}}
- </span>
- {{end}}
- </span>
- </div>
- {{end}}
</body>
</html>
`
