Archived
One form per row in the admin, and buttons that don't trip its validation
The catalogue screen had a form per control: 467 forms, 464 CSRF tokens, 317 KB of HTML for a page that gets opened on a phone in a bakery. It is now one form per row with several submit buttons — the same `name="do"` pattern the catering table editor already used — which is 89 forms and 206 KB, and less markup to read. Only `save` looks at the text boxes, so moving a row cannot save a half-typed name. The buttons that ignore them carry `formnovalidate`, because the name box is `required` and a browser would otherwise refuse to submit "move down" while that box was empty — a validation error about something the button has nothing to do with. Verified on the running container: 89 forms, move and photo-remove still land, and the seeded catalogue comes back the same after a database reset.
This commit is contained in:
@@ -58,14 +58,51 @@ public class AdminController {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Everything you can do to one item, from one form.
|
||||
*
|
||||
* <p>One form per item rather than one per button: the page carries forty items, and a separate form
|
||||
* for each control meant 467 forms and 464 CSRF tokens — 317 KB of HTML for a screen that is opened
|
||||
* on a phone, in a bakery. {@code name="do"} says which button was pressed and its value carries the
|
||||
* argument, exactly as the catering table editor does.
|
||||
*
|
||||
* <p>Only {@code save} looks at the name and category boxes. The other actions deliberately ignore
|
||||
* them, so pressing "move down" halfway through retyping a name doesn't save the half-typed name.
|
||||
*
|
||||
* @param action {@code save}, {@code move:-1}, {@code move:1}, {@code delete}, or
|
||||
* {@code photo:<key>:<-1|1|0>} — earlier, later, or remove
|
||||
*/
|
||||
@PostMapping("/items/{id}")
|
||||
public String describe(@PathVariable Long id,
|
||||
@RequestParam String name,
|
||||
@RequestParam String category,
|
||||
RedirectAttributes flash) {
|
||||
public String item(@PathVariable Long id,
|
||||
@RequestParam(name = "do", defaultValue = "save") String action,
|
||||
@RequestParam(required = false) String name,
|
||||
@RequestParam(required = false) String category,
|
||||
RedirectAttributes flash) {
|
||||
String[] parts = action.split(":");
|
||||
return run(flash, () -> {
|
||||
catalogue.describeItem(id, name, category);
|
||||
flash.addFlashAttribute("done", "Saved " + name.trim() + ".");
|
||||
switch (parts[0]) {
|
||||
case "save" -> {
|
||||
catalogue.describeItem(id, name, category);
|
||||
flash.addFlashAttribute("done", "Saved " + name.trim() + ".");
|
||||
}
|
||||
case "move" -> catalogue.moveItem(id, Integer.parseInt(parts[1]));
|
||||
case "delete" -> {
|
||||
catalogue.removeItem(id);
|
||||
flash.addFlashAttribute("done", "Removed from the products page.");
|
||||
}
|
||||
case "photo" -> {
|
||||
// The key is the middle field; it can contain slashes and dots but never a colon.
|
||||
String key = parts[1];
|
||||
int move = Integer.parseInt(parts[2]);
|
||||
if (move == 0) {
|
||||
catalogue.removePhoto(id, key);
|
||||
} else {
|
||||
catalogue.movePhoto(id, key, move);
|
||||
}
|
||||
}
|
||||
// A stale page, or a hand-edited form. Do nothing rather than guess.
|
||||
default -> { }
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@@ -79,38 +116,6 @@ public class AdminController {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* @param move {@code -1} or {@code 1} to shuffle the photo along, {@code 0} to remove it. One
|
||||
* endpoint for the three buttons under a photo, because they are the same edit — which
|
||||
* photos, in which order — and the server is what decides the resulting list.
|
||||
*/
|
||||
@PostMapping("/items/{id}/photos/arrange")
|
||||
public String arrangePhoto(@PathVariable Long id,
|
||||
@RequestParam String key,
|
||||
@RequestParam int move,
|
||||
RedirectAttributes flash) {
|
||||
return run(flash, () -> {
|
||||
if (move == 0) {
|
||||
catalogue.removePhoto(id, key);
|
||||
} else {
|
||||
catalogue.movePhoto(id, key, move);
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
@PostMapping("/items/{id}/move")
|
||||
public String move(@PathVariable Long id, @RequestParam int by, RedirectAttributes flash) {
|
||||
return run(flash, () -> catalogue.moveItem(id, by));
|
||||
}
|
||||
|
||||
@PostMapping("/items/{id}/delete")
|
||||
public String remove(@PathVariable Long id, RedirectAttributes flash) {
|
||||
return run(flash, () -> {
|
||||
catalogue.removeItem(id);
|
||||
flash.addFlashAttribute("done", "Removed from the products page.");
|
||||
});
|
||||
}
|
||||
|
||||
// --- filters -------------------------------------------------------------
|
||||
|
||||
@PostMapping("/categories")
|
||||
@@ -121,24 +126,31 @@ public class AdminController {
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Everything you can do to one filter, from one form — same shape as an item.
|
||||
*
|
||||
* @param action {@code save}, {@code move:-1}, {@code move:1} or {@code delete}
|
||||
*/
|
||||
@PostMapping("/categories/{id}")
|
||||
public String renameFilter(@PathVariable Long id, @RequestParam String name, RedirectAttributes flash) {
|
||||
public String filter(@PathVariable Long id,
|
||||
@RequestParam(name = "do", defaultValue = "save") String action,
|
||||
@RequestParam(required = false) String name,
|
||||
RedirectAttributes flash) {
|
||||
String[] parts = action.split(":");
|
||||
return run(flash, () -> {
|
||||
catalogue.renameFilter(id, name);
|
||||
flash.addFlashAttribute("done", "Renamed to " + name.trim() + ", and everything filed under it moved too.");
|
||||
});
|
||||
}
|
||||
|
||||
@PostMapping("/categories/{id}/move")
|
||||
public String moveFilter(@PathVariable Long id, @RequestParam int by, RedirectAttributes flash) {
|
||||
return run(flash, () -> catalogue.moveFilter(id, by));
|
||||
}
|
||||
|
||||
@PostMapping("/categories/{id}/delete")
|
||||
public String removeFilter(@PathVariable Long id, RedirectAttributes flash) {
|
||||
return run(flash, () -> {
|
||||
catalogue.removeFilter(id);
|
||||
flash.addFlashAttribute("done", "Category deleted.");
|
||||
switch (parts[0]) {
|
||||
case "save" -> {
|
||||
catalogue.renameFilter(id, name);
|
||||
flash.addFlashAttribute("done",
|
||||
"Renamed to " + name.trim() + ", and everything filed under it moved too.");
|
||||
}
|
||||
case "move" -> catalogue.moveFilter(id, Integer.parseInt(parts[1]));
|
||||
case "delete" -> {
|
||||
catalogue.removeFilter(id);
|
||||
flash.addFlashAttribute("done", "Category deleted.");
|
||||
}
|
||||
default -> { }
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -4,8 +4,18 @@
|
||||
<body>
|
||||
<div th:fragment="content">
|
||||
|
||||
<!--/* The filter buttons. Renaming one carries everything filed under it, which is why the rename is a
|
||||
form of its own rather than an inline edit that might get half-submitted. */-->
|
||||
<!--/*
|
||||
One form per row, not one per button.
|
||||
|
||||
Every control in a row submits that row's form, and `name="do"` says which was pressed — `move:-1`,
|
||||
`photo:<key>:0`, `delete`. Forty items with a form per control came to 467 forms and 464 CSRF tokens,
|
||||
which is 317 KB of HTML for a screen that gets opened on a phone in a bakery; this is 89 forms.
|
||||
|
||||
Only `save` reads the text boxes, so moving a row never saves a half-typed name. Those buttons also
|
||||
carry `formnovalidate`: the name box is `required`, and without it a browser would refuse to submit
|
||||
"move down" while the box was empty — which is nothing to do with moving the row.
|
||||
*/-->
|
||||
|
||||
<section class="card">
|
||||
<h2 class="card-heading">Categories</h2>
|
||||
<p class="mt-1 text-sm text-bakery-600">
|
||||
@@ -14,34 +24,26 @@
|
||||
</p>
|
||||
|
||||
<ul class="mt-3 divide-y divide-bakery-100">
|
||||
<li th:each="filter, f : ${filters}" class="flex flex-wrap items-center gap-2 py-2">
|
||||
<div class="flex gap-1">
|
||||
<form method="post" th:action="@{/admin/categories/{id}/move(id=${filter.id})}">
|
||||
<input type="hidden" name="by" value="-1">
|
||||
<button type="submit" class="btn-icon" th:disabled="${f.first}"
|
||||
th:aria-label="|Move ${filter.name} up|">↑</button>
|
||||
</form>
|
||||
<form method="post" th:action="@{/admin/categories/{id}/move(id=${filter.id})}">
|
||||
<input type="hidden" name="by" value="1">
|
||||
<button type="submit" class="btn-icon" th:disabled="${f.last}"
|
||||
th:aria-label="|Move ${filter.name} down|">↓</button>
|
||||
</form>
|
||||
</div>
|
||||
|
||||
<li th:each="filter, f : ${filters}">
|
||||
<form method="post" th:action="@{/admin/categories/{id}(id=${filter.id})}"
|
||||
class="flex flex-1 min-w-60 items-center gap-2">
|
||||
<label class="flex-1">
|
||||
class="flex flex-wrap items-center gap-2 py-2">
|
||||
<div class="flex gap-1">
|
||||
<button type="submit" name="do" value="move:-1" formnovalidate class="btn-icon" th:disabled="${f.first}"
|
||||
th:aria-label="|Move ${filter.name} up|">↑</button>
|
||||
<button type="submit" name="do" value="move:1" formnovalidate class="btn-icon" th:disabled="${f.last}"
|
||||
th:aria-label="|Move ${filter.name} down|">↓</button>
|
||||
</div>
|
||||
|
||||
<label class="flex-1 min-w-52">
|
||||
<span class="sr-only" th:text="|Name of the ${filter.name} category|">Name</span>
|
||||
<input class="field" name="name" th:value="${filter.name}" required>
|
||||
</label>
|
||||
<button type="submit" class="btn-secondary">Rename</button>
|
||||
</form>
|
||||
<button type="submit" name="do" value="save" class="btn-secondary">Rename</button>
|
||||
|
||||
<span class="text-sm text-bakery-500 whitespace-nowrap"
|
||||
th:text="|${filter.used} item${filter.used == 1 ? '' : 's'}|">0 items</span>
|
||||
<span class="text-sm text-bakery-500 whitespace-nowrap"
|
||||
th:text="|${filter.used} item${filter.used == 1 ? '' : 's'}|">0 items</span>
|
||||
|
||||
<form method="post" th:action="@{/admin/categories/{id}/delete(id=${filter.id})}">
|
||||
<button type="submit" class="btn-danger" th:disabled="${filter.used > 0}"
|
||||
<button type="submit" name="do" value="delete" formnovalidate class="btn-danger" th:disabled="${filter.used > 0}"
|
||||
th:title="${filter.used > 0} ? 'Move its items somewhere else first' : 'Delete'"
|
||||
th:aria-label="|Delete the ${filter.name} category|">Delete</button>
|
||||
</form>
|
||||
@@ -86,30 +88,22 @@
|
||||
</form>
|
||||
</section>
|
||||
|
||||
<!--/* The catalogue itself. One card per item, and every control on it is a form: there is no
|
||||
client-side state here, so a reload is always the truth. */-->
|
||||
<section>
|
||||
<h2 class="card-heading" th:text="|On the page (${#lists.size(items)})|">On the page</h2>
|
||||
|
||||
<ul class="mt-3 space-y-3">
|
||||
<li th:each="item, i : ${items}" class="card">
|
||||
<div class="flex flex-col gap-4 sm:flex-row sm:items-start">
|
||||
<form method="post" th:action="@{/admin/items/{id}(id=${item.id})}"
|
||||
class="flex flex-col gap-4 sm:flex-row sm:items-start">
|
||||
<div class="flex sm:flex-col gap-1 sm:pt-1">
|
||||
<form method="post" th:action="@{/admin/items/{id}/move(id=${item.id})}">
|
||||
<input type="hidden" name="by" value="-1">
|
||||
<button type="submit" class="btn-icon" th:disabled="${i.first}"
|
||||
th:aria-label="|Move ${item.name} up|">↑</button>
|
||||
</form>
|
||||
<form method="post" th:action="@{/admin/items/{id}/move(id=${item.id})}">
|
||||
<input type="hidden" name="by" value="1">
|
||||
<button type="submit" class="btn-icon" th:disabled="${i.last}"
|
||||
th:aria-label="|Move ${item.name} down|">↓</button>
|
||||
</form>
|
||||
<button type="submit" name="do" value="move:-1" formnovalidate class="btn-icon" th:disabled="${i.first}"
|
||||
th:aria-label="|Move ${item.name} up|">↑</button>
|
||||
<button type="submit" name="do" value="move:1" formnovalidate class="btn-icon" th:disabled="${i.last}"
|
||||
th:aria-label="|Move ${item.name} down|">↓</button>
|
||||
</div>
|
||||
|
||||
<div class="flex-1 min-w-0 space-y-3">
|
||||
<form method="post" th:action="@{/admin/items/{id}(id=${item.id})}"
|
||||
class="grid gap-2 sm:grid-cols-[1fr_12rem_auto]">
|
||||
<div class="grid gap-2 sm:grid-cols-[1fr_12rem_auto]">
|
||||
<label class="block">
|
||||
<span class="sr-only">Name</span>
|
||||
<input class="field" name="name" th:value="${item.name}" required>
|
||||
@@ -124,58 +118,47 @@
|
||||
th:selected="${filter.name == item.category}">Cakes</option>
|
||||
</select>
|
||||
</label>
|
||||
<button type="submit" class="btn-secondary">Save</button>
|
||||
</form>
|
||||
<button type="submit" name="do" value="save" class="btn-secondary">Save</button>
|
||||
</div>
|
||||
|
||||
<!--/* Photos, in the order the products page shows them: the first is the one the card
|
||||
leads with. Left/right rather than a drag target, which is far easier to hit on a
|
||||
phone. */-->
|
||||
<!--/* Photos, in the order the products page shows them: the first is the one the card leads
|
||||
with. Left/right rather than a drag target, which is far easier to hit on a phone. */-->
|
||||
<div class="flex flex-wrap gap-3">
|
||||
<figure th:each="photo, p : ${item.photos}" class="w-28">
|
||||
<img th:src="${photo.url}" alt="" loading="lazy"
|
||||
class="w-28 h-28 rounded-md object-cover border border-bakery-200 bg-bakery-100">
|
||||
<figcaption class="mt-1 flex items-center justify-between gap-1">
|
||||
<div class="flex gap-1">
|
||||
<form method="post" th:action="@{/admin/items/{id}/photos/arrange(id=${item.id})}">
|
||||
<input type="hidden" name="key" th:value="${photo.key}">
|
||||
<input type="hidden" name="move" value="-1">
|
||||
<button type="submit" class="btn-icon" th:disabled="${p.first}"
|
||||
aria-label="Move photo earlier">←</button>
|
||||
</form>
|
||||
<form method="post" th:action="@{/admin/items/{id}/photos/arrange(id=${item.id})}">
|
||||
<input type="hidden" name="key" th:value="${photo.key}">
|
||||
<input type="hidden" name="move" value="1">
|
||||
<button type="submit" class="btn-icon" th:disabled="${p.last}"
|
||||
aria-label="Move photo later">→</button>
|
||||
</form>
|
||||
<button type="submit" name="do" th:value="|photo:${photo.key}:-1|" formnovalidate class="btn-icon"
|
||||
th:disabled="${p.first}" aria-label="Move photo earlier">←</button>
|
||||
<button type="submit" name="do" th:value="|photo:${photo.key}:1|" formnovalidate class="btn-icon"
|
||||
th:disabled="${p.last}" aria-label="Move photo later">→</button>
|
||||
</div>
|
||||
<form method="post" th:action="@{/admin/items/{id}/photos/arrange(id=${item.id})}">
|
||||
<input type="hidden" name="key" th:value="${photo.key}">
|
||||
<input type="hidden" name="move" value="0">
|
||||
<!--/* The server refuses to leave an item with no photos; saying so up front beats an
|
||||
error message. */-->
|
||||
<button type="submit" class="btn-icon" th:disabled="${#lists.size(item.photos) == 1}"
|
||||
th:title="${#lists.size(item.photos) == 1} ? 'An item needs at least one photo' : 'Remove photo'"
|
||||
aria-label="Remove photo">×</button>
|
||||
</form>
|
||||
<!--/* The server refuses to leave an item with no photos; saying so up front beats an
|
||||
error message. */-->
|
||||
<button type="submit" name="do" th:value="|photo:${photo.key}:0|" formnovalidate class="btn-icon"
|
||||
th:disabled="${#lists.size(item.photos) == 1}"
|
||||
th:title="${#lists.size(item.photos) == 1} ? 'An item needs at least one photo' : 'Remove photo'"
|
||||
aria-label="Remove photo">×</button>
|
||||
</figcaption>
|
||||
</figure>
|
||||
</div>
|
||||
|
||||
<div class="flex flex-wrap items-center gap-2">
|
||||
<form method="post" th:action="@{/admin/items/{id}/photos(id=${item.id})}"
|
||||
enctype="multipart/form-data" class="flex flex-wrap items-center gap-2">
|
||||
<input type="file" name="photos" accept="image/*" multiple required
|
||||
class="text-sm text-bakery-800 file:btn file:btn-secondary file:mr-3">
|
||||
<button type="submit" class="btn-secondary">Add photos</button>
|
||||
</form>
|
||||
<form method="post" th:action="@{/admin/items/{id}/delete(id=${item.id})}" class="ml-auto">
|
||||
<button type="submit" class="btn-danger"
|
||||
th:aria-label="|Remove ${item.name} from the products page|">Delete</button>
|
||||
</form>
|
||||
<button type="submit" name="do" value="delete" formnovalidate class="btn-danger ml-auto"
|
||||
th:aria-label="|Remove ${item.name} from the products page|">Delete item</button>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
</form>
|
||||
|
||||
<!--/* Its own form, because a file input needs a multipart encoding and the rest of the row
|
||||
doesn't. */-->
|
||||
<form method="post" th:action="@{/admin/items/{id}/photos(id=${item.id})}"
|
||||
enctype="multipart/form-data" class="mt-2 flex flex-wrap items-center gap-2 sm:pl-12">
|
||||
<input type="file" name="photos" accept="image/*" multiple required
|
||||
class="text-sm text-bakery-800 file:btn file:btn-secondary file:mr-3">
|
||||
<button type="submit" class="btn-secondary">Add photos</button>
|
||||
</form>
|
||||
</li>
|
||||
</ul>
|
||||
|
||||
|
||||
@@ -94,6 +94,7 @@ class AdminPagesTest {
|
||||
@Test
|
||||
void renamingAnItemLandsAndSaysSo() throws Exception {
|
||||
mvc.perform(post("/admin/items/1").with(user("morissa")).with(csrf())
|
||||
.param("do", "save")
|
||||
.param("name", "76th Birthday Cake (chocolate)")
|
||||
.param("category", "Cakes"))
|
||||
.andExpect(status().is3xxRedirection())
|
||||
@@ -107,7 +108,7 @@ class AdminPagesTest {
|
||||
@Test
|
||||
void aRefusalComesBackAsASentenceTheEditorCanActOn() throws Exception {
|
||||
// Cookies has items filed under it, and deleting the button shouldn't decide what happens to them.
|
||||
mvc.perform(post("/admin/categories/1/delete").with(user("morissa")).with(csrf()))
|
||||
mvc.perform(post("/admin/categories/1").with(user("morissa")).with(csrf()).param("do", "delete"))
|
||||
.andExpect(redirectedUrl("/admin"))
|
||||
.andExpect(flash().attribute("problem", containsString("still filed under Cookies")));
|
||||
}
|
||||
@@ -115,7 +116,7 @@ class AdminPagesTest {
|
||||
@Test
|
||||
void movingAnItemUpFromTheTopIsNotAnError() throws Exception {
|
||||
// The button is disabled in the page, but a stale page could still post this.
|
||||
mvc.perform(post("/admin/items/1/move").with(user("morissa")).with(csrf()).param("by", "-1"))
|
||||
mvc.perform(post("/admin/items/1").with(user("morissa")).with(csrf()).param("do", "move:-1"))
|
||||
.andExpect(redirectedUrl("/admin"))
|
||||
.andExpect(flash().attributeCount(0));
|
||||
}
|
||||
|
||||
@@ -3,7 +3,6 @@ package com.itsthevine.web;
|
||||
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
|
||||
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
|
||||
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
|
||||
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put;
|
||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
|
||||
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
@@ -78,7 +77,8 @@ class AdminSecurityTest {
|
||||
mvc.perform(get("/admin/catering/tables/1")).andExpect(status().isUnauthorized());
|
||||
mvc.perform(post("/admin/items").with(csrf()).param("name", "Free cake").param("category", "Cakes"))
|
||||
.andExpect(status().isUnauthorized());
|
||||
mvc.perform(post("/admin/items/1/delete").with(csrf())).andExpect(status().isUnauthorized());
|
||||
mvc.perform(post("/admin/items/1").with(csrf()).param("do", "delete"))
|
||||
.andExpect(status().isUnauthorized());
|
||||
mvc.perform(post("/admin/categories").with(csrf()).param("name", "x"))
|
||||
.andExpect(status().isUnauthorized());
|
||||
// The prices are the one thing on this site a stranger would most enjoy editing.
|
||||
@@ -113,8 +113,9 @@ class AdminSecurityTest {
|
||||
|
||||
@Test
|
||||
void theContactFormStillNeedsItsCsrfToken() throws Exception {
|
||||
// Enabling the security starter enables CSRF for the PUBLIC contact form too. Without the token
|
||||
// it 403s; the SPA reads the XSRF-TOKEN cookie and sends X-XSRF-TOKEN.
|
||||
// Enabling the security starter enables CSRF for the PUBLIC contact form too. Without a token it
|
||||
// 403s. The page's own form carries a hidden field (Spring Security fills it in for any th:action
|
||||
// form); this JSON endpoint needs the X-XSRF-TOKEN header from the cookie.
|
||||
mvc.perform(post("/api/contact").contentType(MediaType.APPLICATION_JSON)
|
||||
.content("{\"name\":\"Ada\",\"email\":\"[email protected]\",\"message\":\"hi\"}"))
|
||||
.andExpect(status().isForbidden());
|
||||
|
||||
Reference in New Issue
Block a user