Refactorings

- fix typo in nav.html and template functions
- implement template cache and render function
- refactor motion list methods to reduce cyclomatic complexity

cmd/boardvoting/handlers.go

@@ -18,58 +18,134 @@ limitations under the License.
 package main
 
 import (
+	"fmt"
 	"html/template"
 	"net/http"
+	"path/filepath"
 	"strings"
 	"time"
 
-	"git.cacert.org/cacert-boardvoting/internal/models"
 	"github.com/Masterminds/sprig/v3"
 	"github.com/gorilla/csrf"
-)
 
-const motionsPerPage = 10
+	"git.cacert.org/cacert-boardvoting/internal/models"
+)
 
-func (app *application) motionList(w http.ResponseWriter, r *http.Request) {
-	if r.URL.Path != "/motions/" {
-		app.notFound(w)
+func newTemplateCache() (map[string]*template.Template, error) {
+	cache := map[string]*template.Template{}
 
-		return
+	pages, err := filepath.Glob("./ui/html/pages/*.html")
+	if err != nil {
+		return nil, fmt.Errorf("could not find page templates: %w", err)
 	}
 
-	var err error
+	for _, page := range pages {
+		name := filepath.Base(page)
+
+		files := []string{
+			"./ui/html/base.html",
+			"./ui/html/partials/motion_actions.html",
+			"./ui/html/partials/motion_display.html",
+			"./ui/html/partials/motion_status_class.html",
+			"./ui/html/partials/nav.html",
+			"./ui/html/partials/pagination.html",
+			page,
+		}
 
-	ctx := r.Context()
+		funcMaps := sprig.FuncMap()
+		funcMaps["nl2br"] = func(text string) template.HTML {
+			// #nosec G203 input is sanitized
+			return template.HTML(strings.ReplaceAll(template.HTMLEscapeString(text), "\n", "<br>"))
+		}
+		funcMaps["canManageUsers"] = func(*models.Voter) bool {
+			return false
+		}
+		funcMaps[csrf.TemplateTag] = csrf.TemplateField
 
-	listOptions := &models.MotionListOptions{Limit: motionsPerPage}
+		ts, err := template.New("").Funcs(funcMaps).ParseFiles(files...)
+		if err != nil {
+			return nil, fmt.Errorf("could not parse templates: %w", err)
+		}
 
-	const queryParamBefore = "before"
-	const queryParamAfter = "after"
+		cache[name] = ts
+	}
 
-	if r.URL.Query().Has(queryParamAfter) {
-		var after time.Time
+	return cache, nil
+}
 
-		err := after.UnmarshalText([]byte(r.URL.Query().Get(queryParamAfter)))
-		if err != nil {
-			app.clientError(w, http.StatusBadRequest)
+func (app *application) render(w http.ResponseWriter, status int, page string, data interface{}) {
+	ts, ok := app.templateCache[page]
+	if !ok {
+		app.serverError(w, fmt.Errorf("the template %s does not exist", page))
 
-			return
+		return
+	}
+
+	w.WriteHeader(status)
+
+	err := ts.ExecuteTemplate(w, "base", data)
+	if err != nil {
+		app.serverError(w, err)
+	}
+}
+
+type motionListTemplateData struct {
+	Voter   *models.Voter
+	Flashes []string
+	Params  struct {
+		Flags struct {
+			Unvoted bool
 		}
+	}
+	PrevPage, NextPage string
+	Motions            []*models.MotionForDisplay
+}
 
-		listOptions.After = &after
-	} else if r.URL.Query().Has(queryParamBefore) {
-		var before time.Time
+func (m *motionListTemplateData) setPaginationParameters(first, last *time.Time) error {
+	motions := m.Motions
 
-		err := before.UnmarshalText([]byte(r.URL.Query().Get(queryParamBefore)))
+	if len(motions) > 0 && first.Before(motions[len(motions)-1].Proposed) {
+		marshalled, err := motions[len(motions)-1].Proposed.MarshalText()
 		if err != nil {
-			app.clientError(w, http.StatusBadRequest)
+			return fmt.Errorf("could not serialize timestamp: %w", err)
+		}
+
+		m.NextPage = string(marshalled)
+	}
 
-			return
+	if len(motions) > 0 && last.After(motions[0].Proposed) {
+		marshalled, err := motions[0].Proposed.MarshalText()
+		if err != nil {
+			return fmt.Errorf("could not serialize timestamp: %w", err)
 		}
 
-		listOptions.Before = &before
+		m.PrevPage = string(marshalled)
 	}
 
+	return nil
+}
+
+func (app *application) motionList(w http.ResponseWriter, r *http.Request) {
+	if r.URL.Path != "/motions/" {
+		app.notFound(w)
+
+		return
+	}
+
+	var (
+		listOptions *models.MotionListOptions
+		err         error
+	)
+
+	listOptions, err = calculateMotionListOptions(r)
+	if err != nil {
+		app.clientError(w, http.StatusBadRequest)
+
+		return
+	}
+
+	ctx := r.Context()
+
 	motions, err := app.motions.GetMotions(ctx, listOptions)
 	if err != nil {
 		app.serverError(w, err)
@@ -84,72 +160,48 @@ func (app *application) motionList(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	files := []string{
-		"./ui/html/base.html",
-		"./ui/html/partials/nav.html",
-		"./ui/html/partials/pagination.html",
-		"./ui/html/partials/motion_actions.html",
-		"./ui/html/partials/motion_display.html",
-		"./ui/html/partials/motion_status_class.html",
-		"./ui/html/pages/motions.html",
-	}
+	templateData := &motionListTemplateData{Motions: motions}
 
-	funcMaps := sprig.FuncMap()
-	funcMaps["nl2br"] = func(text string) template.HTML {
-		// #nosec G203 input is sanitized
-		return template.HTML(strings.ReplaceAll(template.HTMLEscapeString(text), "\n", "<br>"))
-	}
-	funcMaps["canMangageUsers"] = func(*models.Voter) bool {
-		return false
-	}
-	funcMaps[csrf.TemplateTag] = func() template.HTML {
-		return csrf.TemplateField(r)
-	}
-
-	ts, err := template.New("").Funcs(funcMaps).ParseFiles(files...)
+	err = templateData.setPaginationParameters(first, last)
 	if err != nil {
 		app.serverError(w, err)
 
 		return
 	}
 
-	templateCtx := struct {
-		Voter   *models.Voter
-		Flashes []string
-		Params  struct {
-			Flags struct {
-				Unvoted bool
-			}
-		}
-		PrevPage, NextPage string
-		Motions            []*models.MotionForDisplay
-	}{Motions: motions}
+	app.render(w, http.StatusOK, "motions.html", &templateData)
+}
 
-	if len(motions) > 0 && first.Before(motions[len(motions)-1].Proposed) {
-		marshalled, err := motions[len(motions)-1].Proposed.MarshalText()
-		if err != nil {
-			app.serverError(w, err)
+func calculateMotionListOptions(r *http.Request) (*models.MotionListOptions, error) {
+	const (
+		queryParamBefore = "before"
+		queryParamAfter  = "after"
+		motionsPerPage   = 10
+	)
 
-			return
-		}
+	listOptions := &models.MotionListOptions{Limit: motionsPerPage}
 
-		templateCtx.NextPage = string(marshalled)
-	}
-	if len(motions) > 0 && last.After(motions[0].Proposed) {
-		marshalled, err := motions[0].Proposed.MarshalText()
+	if r.URL.Query().Has(queryParamAfter) {
+		var after time.Time
+
+		err := after.UnmarshalText([]byte(r.URL.Query().Get(queryParamAfter)))
 		if err != nil {
-			app.serverError(w, err)
+			return nil, fmt.Errorf("could not unmarshal timestamp: %w", err)
+		}
+
+		listOptions.After = &after
+	} else if r.URL.Query().Has(queryParamBefore) {
+		var before time.Time
 
-			return
+		err := before.UnmarshalText([]byte(r.URL.Query().Get(queryParamBefore)))
+		if err != nil {
+			return nil, fmt.Errorf("could not unmarshal timestamp: %w", err)
 		}
 
-		templateCtx.PrevPage = string(marshalled)
+		listOptions.Before = &before
 	}
 
-	err = ts.ExecuteTemplate(w, "base", templateCtx)
-	if err != nil {
-		app.serverError(w, err)
-	}
+	return listOptions, nil
 }
 
 func (app *application) home(w http.ResponseWriter, r *http.Request) {

cmd/boardvoting/main.go

@@ -23,6 +23,7 @@ import (
 	"database/sql"
 	"flag"
 	"fmt"
+	"html/template"
 	"log"
 	"net/http"
 	"os"
@@ -48,6 +49,7 @@ type application struct {
 	mailNotifier      *MailNotifier
 	mailConfig        *mailConfig
 	baseURL           string
+	templateCache     map[string]*template.Template
 }
 
 func main() {
@@ -82,13 +84,19 @@ func main() {
 		errorLog.Fatalf("could not setup decision model: %v", err)
 	}
 
+	templateCache, err := newTemplateCache()
+	if err != nil {
+		errorLog.Fatal(err)
+	}
+
 	app := &application{
-		errorLog:   errorLog,
-		infoLog:    infoLog,
-		motions:    &models.MotionModel{DB: db, InfoLog: infoLog},
-		voters:     &models.VoterModel{DB: db},
-		mailConfig: config.MailConfig,
-		baseURL:    config.BaseURL,
+		errorLog:      errorLog,
+		infoLog:       infoLog,
+		motions:       &models.MotionModel{DB: db, InfoLog: infoLog},
+		voters:        &models.VoterModel{DB: db},
+		mailConfig:    config.MailConfig,
+		baseURL:       config.BaseURL,
+		templateCache: templateCache,
 	}
 
 	app.NewMailNotifier()

internal/models/motions.go

@@ -436,62 +436,22 @@ func (m *MotionModel) TimestampRange(ctx context.Context) (*time.Time, *time.Tim
 }
 
 func (m *MotionModel) GetMotions(ctx context.Context, options *MotionListOptions) ([]*MotionForDisplay, error) {
-	var rows *sqlx.Rows
-	var err error
-
-	if options.Before != nil {
-		rows, err = m.DB.QueryxContext(
-			ctx,
-			`SELECT decisions.id, decisions.tag, decisions.proponent, voters.name AS proposer, decisions.proposed, decisions.title,
-       decisions.content, decisions.votetype, decisions.status, decisions.due, decisions.modified
-FROM   decisions
-JOIN   voters ON decisions.proponent=voters.id
-WHERE decisions.proposed < $1
-ORDER BY proposed DESC
-LIMIT $2`,
-			options.Before,
-			options.Limit,
-		)
-	} else if options.After != nil {
-		rows, err = m.DB.QueryxContext(
-			ctx,
-			`WITH display_decision AS (SELECT decisions.id,
-                                 decisions.tag,
-                                 decisions.proponent,
-                                 voters.name AS proposer,
-                                 decisions.proposed,
-                                 decisions.title,
-                                 decisions.content,
-                                 decisions.votetype,
-                                 decisions.status,
-                                 decisions.due,
-                                 decisions.modified
-                          FROM decisions
-                                   JOIN voters ON decisions.proponent = voters.id
-                          WHERE decisions.proposed > $1
-                          ORDER BY proposed
-                          LIMIT $2)
-SELECT *
-FROM display_decision
-ORDER BY proposed DESC`,
-			options.After,
-			options.Limit,
-		)
-	} else {
-		rows, err = m.DB.QueryxContext(
-			ctx,
-			`SELECT decisions.id, decisions.tag, decisions.proponent, voters.name AS proposer, decisions.proposed, decisions.title,
-       decisions.content, decisions.votetype, decisions.status, decisions.due, decisions.modified
-FROM   decisions
-JOIN   voters ON decisions.proponent=voters.id
-ORDER BY proposed DESC
-LIMIT $1`,
-			options.Limit,
-		)
+	var (
+		rows *sqlx.Rows
+		err  error
+	)
+
+	switch {
+	case options.Before != nil:
+		rows, err = m.GetMotionRowsBefore(ctx, options)
+	case options.After != nil:
+		rows, err = m.GetMotionRowsAfter(ctx, options)
+	default:
+		rows, err = m.GetFirstMotionRows(ctx, options)
 	}
 
 	if err != nil {
-		return nil, fmt.Errorf("could not execute query: %w", err)
+		return nil, err
 	}
 
 	defer func(rows *sqlx.Rows) {
@@ -578,3 +538,91 @@ func (m *MotionModel) FillVoteSums(ctx context.Context, decisions []*MotionForDi
 
 	return nil
 }
+
+func (m *MotionModel) GetMotionRowsBefore(ctx context.Context, options *MotionListOptions) (*sqlx.Rows, error) {
+	rows, err := m.DB.QueryxContext(
+		ctx,
+		`SELECT decisions.id,
+       decisions.tag,
+       decisions.proponent,
+       voters.name AS proposer,
+       decisions.proposed,
+       decisions.title,
+       decisions.content,
+       decisions.votetype,
+       decisions.status,
+       decisions.due,
+       decisions.modified
+FROM decisions
+         JOIN voters ON decisions.proponent = voters.id
+WHERE decisions.proposed < $1
+ORDER BY proposed DESC
+LIMIT $2`,
+		options.Before,
+		options.Limit,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("could not query motions before %s: %w", options.Before, err)
+	}
+
+	return rows, nil
+}
+
+func (m *MotionModel) GetMotionRowsAfter(ctx context.Context, options *MotionListOptions) (*sqlx.Rows, error) {
+	rows, err := m.DB.QueryxContext(
+		ctx,
+		`WITH display_decision AS (SELECT decisions.id,
+                                 decisions.tag,
+                                 decisions.proponent,
+                                 voters.name AS proposer,
+                                 decisions.proposed,
+                                 decisions.title,
+                                 decisions.content,
+                                 decisions.votetype,
+                                 decisions.status,
+                                 decisions.due,
+                                 decisions.modified
+                          FROM decisions
+                                   JOIN voters ON decisions.proponent = voters.id
+                          WHERE decisions.proposed > $1
+                          ORDER BY proposed
+                          LIMIT $2)
+SELECT *
+FROM display_decision
+ORDER BY proposed DESC`,
+		options.After,
+		options.Limit,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("could not query motions after %s: %w", options.After, err)
+	}
+
+	return rows, nil
+}
+
+func (m *MotionModel) GetFirstMotionRows(ctx context.Context, options *MotionListOptions) (*sqlx.Rows, error) {
+	rows, err := m.DB.QueryxContext(
+		ctx,
+		`SELECT decisions.id,
+       decisions.tag,
+       decisions.proponent,
+       voters.name AS proposer,
+       decisions.proposed,
+       decisions.title,
+       decisions.content,
+       decisions.votetype,
+       decisions.status,
+       decisions.due,
+       decisions.modified
+FROM decisions
+         JOIN voters ON decisions.proponent = voters.id
+ORDER BY proposed DESC
+LIMIT $1`,
+		options.Limit,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("could not query motions: %w", err)
+	}
+
+	return rows, nil
+}

ui/html/partials/nav.html

@@ -1,5 +1,5 @@
 {{ define "nav" }}
-{{ if .Voter | canMangageUsers }}
+{{ if .Voter | canManageUsers }}
 <nav class="ui top attached tabular menu">
     <a class="active item" href="/motions/">Motions</a>
     <a class="item" href="/users/">User management</a>