From ff93acb65c1cb0f253e4e270141aad336cc5b3e4 Mon Sep 17 00:00:00 2001 From: Jan Dittberner Date: Sat, 21 May 2022 20:49:35 +0200 Subject: [PATCH] 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 | 216 ++++++++++++++++++++++-------------- cmd/boardvoting/main.go | 20 +++- internal/models/motions.go | 152 ++++++++++++++++--------- ui/html/partials/nav.html | 2 +- 4 files changed, 249 insertions(+), 141 deletions(-) diff --git a/cmd/boardvoting/handlers.go b/cmd/boardvoting/handlers.go index 4ad02dd..5c19fc0 100644 --- a/cmd/boardvoting/handlers.go +++ b/cmd/boardvoting/handlers.go @@ -18,17 +18,112 @@ 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" + + "git.cacert.org/cacert-boardvoting/internal/models" ) -const motionsPerPage = 10 +func newTemplateCache() (map[string]*template.Template, error) { + cache := map[string]*template.Template{} + + pages, err := filepath.Glob("./ui/html/pages/*.html") + if err != nil { + return nil, fmt.Errorf("could not find page templates: %w", err) + } + + 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, + } + + funcMaps := sprig.FuncMap() + funcMaps["nl2br"] = func(text string) template.HTML { + // #nosec G203 input is sanitized + return template.HTML(strings.ReplaceAll(template.HTMLEscapeString(text), "\n", "
")) + } + funcMaps["canManageUsers"] = func(*models.Voter) bool { + return false + } + funcMaps[csrf.TemplateTag] = csrf.TemplateField + + ts, err := template.New("").Funcs(funcMaps).ParseFiles(files...) + if err != nil { + return nil, fmt.Errorf("could not parse templates: %w", err) + } + + cache[name] = ts + } + + return cache, nil +} + +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 + } + + 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 +} + +func (m *motionListTemplateData) setPaginationParameters(first, last *time.Time) error { + motions := m.Motions + + if len(motions) > 0 && first.Before(motions[len(motions)-1].Proposed) { + marshalled, err := motions[len(motions)-1].Proposed.MarshalText() + if err != nil { + return fmt.Errorf("could not serialize timestamp: %w", err) + } + + m.NextPage = string(marshalled) + } + + 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) + } + + m.PrevPage = string(marshalled) + } + + return nil +} func (app *application) motionList(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/motions/" { @@ -37,39 +132,20 @@ func (app *application) motionList(w http.ResponseWriter, r *http.Request) { return } - var err error + var ( + listOptions *models.MotionListOptions + err error + ) + + listOptions, err = calculateMotionListOptions(r) + if err != nil { + app.clientError(w, http.StatusBadRequest) + + return + } ctx := r.Context() - listOptions := &models.MotionListOptions{Limit: motionsPerPage} - - const queryParamBefore = "before" - const queryParamAfter = "after" - - if r.URL.Query().Has(queryParamAfter) { - var after time.Time - - err := after.UnmarshalText([]byte(r.URL.Query().Get(queryParamAfter))) - if err != nil { - app.clientError(w, http.StatusBadRequest) - - return - } - - listOptions.After = &after - } else if r.URL.Query().Has(queryParamBefore) { - var before time.Time - - err := before.UnmarshalText([]byte(r.URL.Query().Get(queryParamBefore))) - if err != nil { - app.clientError(w, http.StatusBadRequest) - - return - } - - listOptions.Before = &before - } - 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", "
")) - } - 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() +func calculateMotionListOptions(r *http.Request) (*models.MotionListOptions, error) { + const ( + queryParamBefore = "before" + queryParamAfter = "after" + motionsPerPage = 10 + ) + + listOptions := &models.MotionListOptions{Limit: motionsPerPage} + + 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 + return nil, fmt.Errorf("could not unmarshal timestamp: %w", err) } - templateCtx.NextPage = string(marshalled) - } - if len(motions) > 0 && last.After(motions[0].Proposed) { - marshalled, err := motions[0].Proposed.MarshalText() + listOptions.After = &after + } else if r.URL.Query().Has(queryParamBefore) { + var before time.Time + + err := before.UnmarshalText([]byte(r.URL.Query().Get(queryParamBefore))) if err != nil { - app.serverError(w, err) - - return + 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) { diff --git a/cmd/boardvoting/main.go b/cmd/boardvoting/main.go index 9ed5114..8ba6e23 100644 --- a/cmd/boardvoting/main.go +++ b/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() diff --git a/internal/models/motions.go b/internal/models/motions.go index a026bed..65e207d 100644 --- a/internal/models/motions.go +++ b/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 + var ( + rows *sqlx.Rows + 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, - ) + 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 +} diff --git a/ui/html/partials/nav.html b/ui/html/partials/nav.html index d8e732b..cec8f83 100644 --- a/ui/html/partials/nav.html +++ b/ui/html/partials/nav.html @@ -1,5 +1,5 @@ {{ define "nav" }} -{{ if .Voter | canMangageUsers }} +{{ if .Voter | canManageUsers }}