Toggle only a target's active state, so it cannot undo an edit (closes #431)

The target toggle loaded the target, flipped its active flag and saved the whole row, so an edit of the same target's name or settings saved in between was written back over and lost, although it reported success. The toggle now updates only the active column, so it can no longer undo an edit. A test saves an edit just after the toggle has read the target and shows the edit survives and the state flips. The entrypoint toggle is unchanged, since an entrypoint has no edit form.

Model: opus-5-5

internal/handlers/auth.go

@@ -139,7 +139,8 @@ func (h *Handlers) renderLoginError(
 		),
 	}
 
-	h.renderTemplateStatus(w, r, "login.html", data, status)
+	w.WriteHeader(status)
+	h.renderTemplate(w, r, "login.html", data)
 }
 
 // authenticateUser looks up and verifies a user's credentials.

internal/handlers/auth_test.go

@@ -3,7 +3,6 @@ package handlers_test
 import (
 	"context"
 	"fmt"
-	"html/template"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -405,60 +404,6 @@ func TestLogin_MissingCredentialsRejectedBeforeAnyHash(t *testing.T) {
 	)
 }
 
-// TestLogin_FormErrorAnswersItsStatusWithThePage proves that the login
-// form shown again with an error still answers 400 with the whole page.
-func TestLogin_FormErrorAnswersItsStatusWithThePage(t *testing.T) {
-	t.Parallel()
-
-	var h *handlers.Handlers
-
-	app := newTestApp(t, &h)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	w := submitLogin(h, sharedProxyPeer, "", "")
-
-	assert.Equal(t, http.StatusBadRequest, w.Code)
-	assert.Contains(
-		t, w.Body.String(), "Username and password are required",
-	)
-	assert.Contains(
-		t, w.Body.String(), "</html>",
-		"the page must render to completion",
-	)
-}
-
-// TestLogin_FormErrorRenderFailureAnswers500 proves that a login form
-// error page whose template fails answers 500 with the error page and
-// none of the form page, rather than the 400 it meant to send.
-func TestLogin_FormErrorRenderFailureAnswers500(t *testing.T) {
-	t.Parallel()
-
-	var h *handlers.Handlers
-
-	app := newTestApp(t, &h)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	// The page prints its error message and then fails.
-	h.AddTemplateForTest("login.html", template.Must(
-		template.New("login").Funcs(template.FuncMap{
-			"fail": func() (string, error) { return "", errMidRender },
-		}).Parse(`{{.Error}}{{fail}}`),
-	))
-
-	w := submitLogin(h, sharedProxyPeer, "", "")
-
-	assert.Equal(t, http.StatusInternalServerError, w.Code)
-	assert.NotContains(
-		t, w.Body.String(), "Username and password are required",
-		"the response must carry no part of the aborted page",
-	)
-	assert.Contains(t, w.Body.String(), "500 Internal Server Error")
-}
-
 // TestLogin_SuccessCreatesSession is the control for the tests above:
 // the success path they assert on really does authenticate.
 func TestLogin_SuccessCreatesSession(t *testing.T) {

internal/handlers/handlers.go

@@ -309,26 +309,12 @@ func (s *Handlers) getUserInfo(
 }
 
 // renderTemplate renders a pre-parsed template with common
-// data and answers 200.
+// data
 func (s *Handlers) renderTemplate(
 	w http.ResponseWriter,
 	r *http.Request,
 	pageTemplate string,
 	data any,
-) {
-	s.renderTemplateStatus(w, r, pageTemplate, data, http.StatusOK)
-}
-
-// renderTemplateStatus is renderTemplate answering with status, for a
-// form shown again with an error. Call it instead of WriteHeader
-// followed by renderTemplate: the status is written only once the page
-// has rendered, so a failed render can still answer 500.
-func (s *Handlers) renderTemplateStatus(
-	w http.ResponseWriter,
-	r *http.Request,
-	pageTemplate string,
-	data any,
-	status int,
 ) {
 	tmpl, ok := s.templates[pageTemplate]
 	if !ok {
@@ -341,9 +327,7 @@ func (s *Handlers) renderTemplateStatus(
 		return
 	}
 
-	s.executeTemplate(
-		w, r, tmpl, s.pageData(r, data, noticeFor(r)), status,
-	)
+	s.executeTemplate(w, r, tmpl, s.pageData(r, data, noticeFor(r)))
 }
 
 // pageData adds the fields the shared layout renders to a page's own
@@ -379,20 +363,19 @@ func (s *Handlers) pageData(
 	}
 }
 
-// executeTemplate renders the template into a buffer and writes status
-// and the page to the response only once rendering has fully
-// succeeded. Executing straight into the ResponseWriter commits a
-// partial body and the status before a mid-render error can be
-// reported, leaving no way to serve a 500. Buffering makes a page's
-// rendered size resident memory per concurrent viewer, so every page
-// owes it a bound: the event log caps each stored body at
-// maxRenderedBodyBytes for exactly this reason.
+// executeTemplate renders the template into a buffer and writes to
+// the response only once rendering has fully succeeded. Executing
+// straight into the ResponseWriter commits a partial body and a 200
+// status before a mid-render error can be reported, leaving no way
+// to serve a 500. Buffering makes a page's rendered size resident
+// memory per concurrent viewer, so every page owes it a bound: the
+// event log caps each stored body at maxRenderedBodyBytes for exactly
+// this reason.
 func (s *Handlers) executeTemplate(
 	w http.ResponseWriter,
 	r *http.Request,
 	tmpl *template.Template,
 	data any,
-	status int,
 ) {
 	var buf bytes.Buffer
 
@@ -407,7 +390,6 @@ func (s *Handlers) executeTemplate(
 	}
 
 	w.Header().Set("Content-Type", "text/html; charset=utf-8")
-	w.WriteHeader(status)
 
 	_, err = buf.WriteTo(w)
 	if err != nil {

internal/handlers/source_management.go

@@ -350,12 +350,12 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
 		retentionStr := r.PostFormValue("retention_days")
 
 		if name == "" {
-			h.renderTemplateStatus(
+			w.WriteHeader(http.StatusBadRequest)
+			h.renderTemplate(
 				w, r, "sources_new.html",
 				newSourceFormData(
 					"Name is required", name, description,
 				),
-				http.StatusBadRequest,
 			)
 
 			return
@@ -365,13 +365,13 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
 			retentionStr, database.DefaultRetentionDays,
 		)
 		if retErr != nil {
-			h.renderTemplateStatus(
+			w.WriteHeader(http.StatusBadRequest)
+			h.renderTemplate(
 				w, r, "sources_new.html",
 				newSourceFormData(
 					retentionErrorMessage(retErr),
 					name, description,
 				),
-				http.StatusBadRequest,
 			)
 
 			return
@@ -644,7 +644,8 @@ func (h *Handlers) applyWebhookEdit(
 			tmplKeyError:   "Name is required",
 		}
 
-		h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)
+		w.WriteHeader(http.StatusBadRequest)
+		h.renderTemplate(w, r, "source_edit.html", data)
 
 		return
 	}
@@ -664,7 +665,8 @@ func (h *Handlers) applyWebhookEdit(
 			tmplKeyError:   retentionErrorMessage(retErr),
 		}
 
-		h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusBadRequest)
+		w.WriteHeader(http.StatusBadRequest)
+		h.renderTemplate(w, r, "source_edit.html", data)
 
 		return
 	}
@@ -701,7 +703,8 @@ func (h *Handlers) applyWebhookEdit(
 					"it, then save again.",
 			}
 
-			h.renderTemplateStatus(w, r, "source_edit.html", data, http.StatusConflict)
+			w.WriteHeader(http.StatusConflict)
+			h.renderTemplate(w, r, "source_edit.html", data)
 
 			return
 		}
@@ -1908,9 +1911,13 @@ func (h *Handlers) HandleTargetToggle() http.HandlerFunc {
 				return false, err
 			}
 
-			tgt.Active = !tgt.Active
+			// Only the active column: saving the whole row would
+			// write back the name and settings read above over an
+			// edit saved since.
+			active := !tgt.Active
 
-			return tgt.Active, h.db.DB().Save(&tgt).Error
+			return active, h.db.DB().Model(&tgt).
+				Update("active", active).Error
 		},
 		"failed to toggle target",
 		targetActivated, targetDeactivated,

internal/handlers/target_toggle_test.go

@@ -0,0 +1,66 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+)
+
+// TestHandleTargetToggle_DoesNotUndoAnEdit proves that a toggle which
+// loaded the target before an edit of it was saved does not write the
+// old name and settings back over the edit. The edit is submitted from
+// a callback on the toggle's own read of the target, so it is saved
+// after that read and before the toggle writes.
+func TestHandleTargetToggle_DoesNotUndoAnEdit(t *testing.T) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+	wh, tgt := seedHTTPTarget(t, env, "", "")
+	require.True(t, tgt.Active)
+
+	var (
+		edited   bool
+		editCode int
+	)
+
+	require.NoError(t, env.db.DB().Callback().Query().
+		After("gorm:query").
+		Register("test:edit_after_toggle_read", func(tx *gorm.DB) {
+			// The edit reads the target too; only the toggle's read,
+			// the first, submits it.
+			if tx.Statement.Table != "targets" || edited {
+				return
+			}
+
+			edited = true
+			editCode = submitTargetEdit(
+				env, wh.ID, tgt.ID,
+				editForm(editReplacedURL, "", ""),
+			).Code
+		}),
+	)
+
+	req := postRequest(
+		"/hook/"+wh.ID+"/targets/"+tgt.ID+"/toggle",
+		env.cookies,
+		map[string]string{paramSourceID: wh.ID, paramTargetID: tgt.ID},
+	)
+	w := httptest.NewRecorder()
+
+	env.handlers.HandleTargetToggle().ServeHTTP(w, req)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+	require.Equal(t, http.StatusSeeOther, editCode)
+
+	stored := storedTarget(t, env, tgt.ID)
+	assert.False(t, stored.Active)
+	assert.Equal(t, "edited-name", stored.Name)
+	assert.Equal(t, 5, stored.MaxRetries)
+	assert.Equal(
+		t, editReplacedURL, storedHTTPConfig(t, env, tgt.ID).URL,
+	)
+}

internal/middleware/middleware_test.go

@@ -12,7 +12,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/gorilla/sessions"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"sneak.berlin/go/webhooker/internal/config"
@@ -78,14 +77,7 @@ func newTestSessionManager(
 		key[i] = byte(i)
 	}
 
-	store := sessions.NewCookieStore(key)
-	store.Options = &sessions.Options{
-		Path:     "/",
-		MaxAge:   86400 * 7,
-		HttpOnly: true,
-		Secure:   false,
-		SameSite: http.SameSiteLaxMode,
-	}
+	store := session.NewStore(key)
 
 	var now func() time.Time
 
@@ -931,8 +923,7 @@ func metricsAuthMiddleware(
 	}
 
 	key := make([]byte, testKeySize)
-	store := sessions.NewCookieStore(key)
-	store.Options = &sessions.Options{Path: "/", MaxAge: 86400}
+	store := session.NewStore(key)
 
 	sessManager := session.NewForTest(store, cfg, log, key, nil)
 

internal/session/export_test.go

@@ -1,10 +0,0 @@
-package session
-
-import "github.com/gorilla/sessions"
-
-// NewStore exposes the production cookie-store constructor so tests
-// exercise the store the application actually runs with, rather than a
-// lookalike assembled in the test.
-func NewStore(key []byte) *sessions.CookieStore {
-	return newStore(key)
-}

internal/session/testing.go

@@ -8,6 +8,13 @@ import (
 	"sneak.berlin/go/webhooker/internal/config"
 )
 
+// NewStore exposes the production cookie-store constructor so tests
+// exercise the store the application actually runs with, rather than a
+// lookalike assembled in the test.
+func NewStore(key []byte) *sessions.CookieStore {
+	return newStore(key)
+}
+
 // NewForTest creates a Session with a pre-configured cookie store for use
 // in tests. This bypasses the fx lifecycle and database dependency, allowing
 // middleware and handler tests to use real session functionality. The key