From 2cd579a574fc95b9cd7f49b10669491c8177cd3d Mon Sep 17 00:00:00 2001 From: Jon Seager Date: Tue, 10 Feb 2026 20:04:24 +0000 Subject: [PATCH] refactor: deduplicate update handlers and edit templates Add HasChanges trait with impl_has_changes! macro, validate_update() and update_response() helpers to reduce boilerplate across all 7 entity update handlers. Extract edit form actions (error, spinner, buttons) into a shared Askama macro. Also adds missing no-changes validation to the bag update handler. --- src/application/routes/api/coffee/bags.rs | 16 ++++++- src/application/routes/api/coffee/brews.rs | 46 +++++++++---------- src/application/routes/api/coffee/cafes.rs | 28 ++++------- src/application/routes/api/coffee/cups.rs | 13 ++---- src/application/routes/api/coffee/gear.rs | 22 +++------ src/application/routes/api/coffee/roasters.rs | 24 +++------- src/application/routes/api/coffee/roasts.rs | 29 ++++++------ src/application/routes/support.rs | 46 ++++++++++++++++++- templates/pages/edit_bag.html | 31 +------------ templates/pages/edit_brew.html | 31 +------------ templates/pages/edit_cafe.html | 31 +------------ templates/pages/edit_cup.html | 31 +------------ templates/pages/edit_gear.html | 31 +------------ templates/pages/edit_roast.html | 31 +------------ templates/pages/edit_roaster.html | 31 +------------ templates/partials/detail_cards.html | 32 +++++++++++++ tests/e2e/edit_tests.rs | 2 +- 17 files changed, 170 insertions(+), 305 deletions(-) diff --git a/src/application/routes/api/coffee/bags.rs b/src/application/routes/api/coffee/bags.rs index 2d10d80..0f6dd4a 100644 --- a/src/application/routes/api/coffee/bags.rs +++ b/src/application/routes/api/coffee/bags.rs @@ -11,7 +11,8 @@ use crate::application::errors::{ApiError, AppError}; use crate::application::routes::api::images::save_deferred_image; use crate::application::routes::api::macros::{define_delete_handler, define_enriched_get_handler}; use crate::application::routes::support::{ - FlexiblePayload, ListQuery, PayloadSource, is_datastar_request, + FlexiblePayload, ListQuery, PayloadSource, impl_has_changes, is_datastar_request, + validate_update, }; use crate::application::state::AppState; use crate::domain::bags::{BagFilter, BagSortKey, BagWithRoast, NewBag, UpdateBag}; @@ -156,6 +157,17 @@ impl UpdateBagSubmission { } } +impl_has_changes!( + UpdateBag, + roast_id, + roast_date, + amount, + remaining, + closed, + finished_at, + created_at +); + #[tracing::instrument(skip(state, _auth_user, headers, query, payload))] pub(crate) async fn update_bag( State(state): State, @@ -180,6 +192,8 @@ pub(crate) async fn update_bag( created_at: body_update.created_at, }; + validate_update(&update, image_data_url.as_ref())?; + // When the bag amount changes, recompute remaining based on how much has been consumed. if let Some(new_amount) = update.amount && update.remaining.is_none() diff --git a/src/application/routes/api/coffee/brews.rs b/src/application/routes/api/coffee/brews.rs index ac14be3..3015023 100644 --- a/src/application/routes/api/coffee/brews.rs +++ b/src/application/routes/api/coffee/brews.rs @@ -11,7 +11,8 @@ use crate::application::errors::{ApiError, AppError}; use crate::application::routes::api::images::save_deferred_image; use crate::application::routes::api::macros::{define_delete_handler, define_enriched_get_handler}; use crate::application::routes::support::{ - FlexiblePayload, ListQuery, PayloadSource, is_datastar_request, + FlexiblePayload, ListQuery, PayloadSource, impl_has_changes, is_datastar_request, + validate_update, }; use crate::application::state::AppState; use crate::domain::bags::BagFilter; @@ -375,6 +376,21 @@ impl UpdateBrewSubmission { } } +impl_has_changes!( + UpdateBrew, + bag_id, + coffee_weight, + grinder_id, + grind_setting, + brewer_id, + filter_paper_id, + water_volume, + water_temp, + quick_notes, + brew_time, + created_at +); + #[tracing::instrument(skip(state, _auth_user, headers, payload))] pub(crate) async fn update_brew( State(state): State, @@ -386,22 +402,7 @@ pub(crate) async fn update_brew( let (submission, source) = payload.into_parts(); let (update, image_data_url) = submission.into_parts(); - let has_changes = update.bag_id.is_some() - || update.coffee_weight.is_some() - || update.grinder_id.is_some() - || update.grind_setting.is_some() - || update.brewer_id.is_some() - || update.filter_paper_id.is_some() - || update.water_volume.is_some() - || update.water_temp.is_some() - || update.quick_notes.is_some() - || update.brew_time.is_some() - || update.created_at.is_some() - || image_data_url.is_some(); - - if !has_changes { - return Err(AppError::validation("no changes provided").into()); - } + validate_update(&update, image_data_url.as_ref())?; state .brew_repo @@ -414,12 +415,6 @@ pub(crate) async fn update_brew( save_deferred_image(&state, "brew", i64::from(id), image_data_url.as_deref()).await; - let enriched = state - .brew_repo - .get_with_details(id) - .await - .map_err(AppError::from)?; - let detail_url = format!("/brews/{id}"); if is_datastar_request(&headers) { @@ -428,6 +423,11 @@ pub(crate) async fn update_brew( } else if matches!(source, PayloadSource::Form) { Ok(Redirect::to(&detail_url).into_response()) } else { + let enriched = state + .brew_repo + .get_with_details(id) + .await + .map_err(AppError::from)?; Ok(Json(enriched).into_response()) } } diff --git a/src/application/routes/api/coffee/cafes.rs b/src/application/routes/api/coffee/cafes.rs index 23cb391..d09a615 100644 --- a/src/application/routes/api/coffee/cafes.rs +++ b/src/application/routes/api/coffee/cafes.rs @@ -10,8 +10,10 @@ use crate::application::routes::api::images::save_deferred_image; use crate::application::routes::api::macros::{ define_delete_handler, define_get_handler, define_list_fragment_renderer, }; +use crate::application::routes::support::impl_has_changes; use crate::application::routes::support::{ FlexiblePayload, ListQuery, PayloadSource, is_datastar_request, render_redirect_script, + update_response, validate_update, }; use crate::application::state::AppState; use crate::domain::cafes::{Cafe, CafeSortKey, NewCafe, UpdateCafe}; @@ -175,6 +177,10 @@ impl UpdateCafeSubmission { } } +impl_has_changes!( + UpdateCafe, name, city, country, latitude, longitude, website, created_at +); + #[tracing::instrument(skip(state, _auth_user, headers, payload))] pub(crate) async fn update_cafe( State(state): State, @@ -186,18 +192,7 @@ pub(crate) async fn update_cafe( let (submission, source) = payload.into_parts(); let (update, image_data_url) = submission.into_parts(); - let has_changes = update.name.is_some() - || update.city.is_some() - || update.country.is_some() - || update.latitude.is_some() - || update.longitude.is_some() - || update.website.is_some() - || update.created_at.is_some() - || image_data_url.is_some(); - - if !has_changes { - return Err(AppError::validation("no changes provided").into()); - } + validate_update(&update, image_data_url.as_ref())?; let cafe = state .cafe_repo @@ -216,14 +211,7 @@ pub(crate) async fn update_cafe( .await; let detail_url = format!("/cafes/{}", cafe.slug); - - if is_datastar_request(&headers) { - render_redirect_script(&detail_url).map_err(ApiError::from) - } else if matches!(source, PayloadSource::Form) { - Ok(Redirect::to(&detail_url).into_response()) - } else { - Ok(Json(cafe).into_response()) - } + update_response(&headers, source, &detail_url, Json(cafe).into_response()) } define_delete_handler!( diff --git a/src/application/routes/api/coffee/cups.rs b/src/application/routes/api/coffee/cups.rs index ca67edd..1164c73 100644 --- a/src/application/routes/api/coffee/cups.rs +++ b/src/application/routes/api/coffee/cups.rs @@ -11,8 +11,10 @@ use crate::application::routes::api::images::save_deferred_image; use crate::application::routes::api::macros::{ define_delete_handler, define_enriched_get_handler, define_list_fragment_renderer, }; +use crate::application::routes::support::impl_has_changes; use crate::application::routes::support::{ FlexiblePayload, ListQuery, PayloadSource, is_datastar_request, render_redirect_script, + validate_update, }; use crate::application::state::AppState; use crate::domain::cups::{CupFilter, CupSortKey, CupWithDetails, NewCup, UpdateCup}; @@ -116,6 +118,8 @@ impl UpdateCupSubmission { } } +impl_has_changes!(UpdateCup, roast_id, cafe_id, created_at); + #[tracing::instrument(skip(state, _auth_user, headers, payload))] pub(crate) async fn update_cup( State(state): State, @@ -127,14 +131,7 @@ pub(crate) async fn update_cup( let (submission, source) = payload.into_parts(); let (update, image_data_url) = submission.into_parts(); - let has_changes = update.roast_id.is_some() - || update.cafe_id.is_some() - || update.created_at.is_some() - || image_data_url.is_some(); - - if !has_changes { - return Err(AppError::validation("no changes provided").into()); - } + validate_update(&update, image_data_url.as_ref())?; let cup = state .cup_repo diff --git a/src/application/routes/api/coffee/gear.rs b/src/application/routes/api/coffee/gear.rs index 2a4a3ef..64bcad8 100644 --- a/src/application/routes/api/coffee/gear.rs +++ b/src/application/routes/api/coffee/gear.rs @@ -14,8 +14,10 @@ use crate::application::routes::api::images::save_deferred_image; use crate::application::routes::api::macros::{ define_delete_handler, define_get_handler, define_list_fragment_renderer, }; +use crate::application::routes::support::impl_has_changes; use crate::application::routes::support::{ FlexiblePayload, ListQuery, PayloadSource, is_datastar_request, render_redirect_script, + update_response, validate_update, }; use crate::application::state::AppState; use crate::domain::gear::{Gear, GearCategory, GearFilter, GearSortKey, NewGear, UpdateGear}; @@ -148,6 +150,8 @@ impl UpdateGearSubmission { } } +impl_has_changes!(UpdateGear, make, model, created_at); + #[tracing::instrument(skip(state, _auth_user, headers, payload))] pub(crate) async fn update_gear( State(state): State, @@ -159,14 +163,7 @@ pub(crate) async fn update_gear( let (submission, source) = payload.into_parts(); let (update, image_data_url) = submission.into_parts(); - let has_changes = update.make.is_some() - || update.model.is_some() - || update.created_at.is_some() - || image_data_url.is_some(); - - if !has_changes { - return Err(AppError::validation("no changes provided").into()); - } + validate_update(&update, image_data_url.as_ref())?; let gear = state .gear_repo @@ -186,14 +183,7 @@ pub(crate) async fn update_gear( .await; let detail_url = format!("/gear/{}", gear.id); - - if is_datastar_request(&headers) { - render_redirect_script(&detail_url).map_err(ApiError::from) - } else if matches!(source, PayloadSource::Form) { - Ok(Redirect::to(&detail_url).into_response()) - } else { - Ok(Json(gear).into_response()) - } + update_response(&headers, source, &detail_url, Json(gear).into_response()) } define_delete_handler!( diff --git a/src/application/routes/api/coffee/roasters.rs b/src/application/routes/api/coffee/roasters.rs index 5715094..c8b3d6e 100644 --- a/src/application/routes/api/coffee/roasters.rs +++ b/src/application/routes/api/coffee/roasters.rs @@ -10,8 +10,10 @@ use crate::application::routes::api::images::save_deferred_image; use crate::application::routes::api::macros::{ define_delete_handler, define_get_handler, define_list_fragment_renderer, }; +use crate::application::routes::support::impl_has_changes; use crate::application::routes::support::{ FlexiblePayload, ListQuery, PayloadSource, is_datastar_request, render_redirect_script, + update_response, validate_update, }; use crate::application::state::AppState; use crate::domain::ids::RoasterId; @@ -168,6 +170,8 @@ impl UpdateRoasterSubmission { } } +impl_has_changes!(UpdateRoaster, name, country, city, homepage, created_at); + #[tracing::instrument(skip(state, _auth_user, headers, payload))] pub(crate) async fn update_roaster( State(state): State, @@ -180,16 +184,7 @@ pub(crate) async fn update_roaster( let (update, image_data_url) = submission.into_parts(); let update = update.normalize(); - let has_changes = update.name.is_some() - || update.country.is_some() - || update.city.is_some() - || update.homepage.is_some() - || update.created_at.is_some() - || image_data_url.is_some(); - - if !has_changes { - return Err(AppError::validation("no changes provided").into()); - } + validate_update(&update, image_data_url.as_ref())?; let roaster = state .roaster_repo @@ -208,14 +203,7 @@ pub(crate) async fn update_roaster( .await; let detail_url = format!("/roasters/{}", roaster.slug); - - if is_datastar_request(&headers) { - render_redirect_script(&detail_url).map_err(ApiError::from) - } else if matches!(source, PayloadSource::Form) { - Ok(Redirect::to(&detail_url).into_response()) - } else { - Ok(Json(roaster).into_response()) - } + update_response(&headers, source, &detail_url, Json(roaster).into_response()) } define_delete_handler!( diff --git a/src/application/routes/api/coffee/roasts.rs b/src/application/routes/api/coffee/roasts.rs index 7814209..d808a5f 100644 --- a/src/application/routes/api/coffee/roasts.rs +++ b/src/application/routes/api/coffee/roasts.rs @@ -12,7 +12,8 @@ use crate::application::routes::api::macros::{ define_delete_handler, define_enriched_get_handler, define_list_fragment_renderer, }; use crate::application::routes::support::{ - FlexiblePayload, ListQuery, PayloadSource, is_datastar_request, render_redirect_script, + FlexiblePayload, ListQuery, PayloadSource, impl_has_changes, is_datastar_request, + render_redirect_script, validate_update, }; use crate::application::state::AppState; use crate::domain::ids::{RoastId, RoasterId}; @@ -214,6 +215,18 @@ impl UpdateRoastSubmission { } } +impl_has_changes!( + UpdateRoast, + roaster_id, + name, + origin, + region, + producer, + tasting_notes, + process, + created_at +); + #[tracing::instrument(skip(state, _auth_user, headers, payload))] pub(crate) async fn update_roast( State(state): State, @@ -225,19 +238,7 @@ pub(crate) async fn update_roast( let (submission, source) = payload.into_parts(); let (update, image_data_url) = submission.into_parts(); - let has_changes = update.roaster_id.is_some() - || update.name.is_some() - || update.origin.is_some() - || update.region.is_some() - || update.producer.is_some() - || update.tasting_notes.is_some() - || update.process.is_some() - || update.created_at.is_some() - || image_data_url.is_some(); - - if !has_changes { - return Err(AppError::validation("no changes provided").into()); - } + validate_update(&update, image_data_url.as_ref())?; state .roast_repo diff --git a/src/application/routes/support.rs b/src/application/routes/support.rs index 1e9aa25..17c2169 100644 --- a/src/application/routes/support.rs +++ b/src/application/routes/support.rs @@ -1,7 +1,7 @@ use askama::Template; use axum::extract::{Form, FromRequest, Json as JsonPayload, Request}; use axum::http::{HeaderMap, HeaderValue, header::CONTENT_TYPE}; -use axum::response::{Html, IntoResponse, Response}; +use axum::response::{Html, IntoResponse, Redirect, Response}; use serde::Deserialize; use tracing::warn; @@ -32,6 +32,50 @@ impl FlexiblePayload { } } +/// Trait for update structs that can report whether any field was set. +pub(crate) trait HasChanges { + fn has_changes(&self) -> bool; +} + +/// Implement `HasChanges` for an update struct by checking `is_some()` on each field. +macro_rules! impl_has_changes { + ($type:ty, $($field:ident),+) => { + impl $crate::application::routes::support::HasChanges for $type { + fn has_changes(&self) -> bool { + $(self.$field.is_some())||+ + } + } + }; +} +pub(crate) use impl_has_changes; + +/// Return `400 Bad Request` if neither the update struct nor the image has any changes. +pub(crate) fn validate_update( + update: &T, + image: Option<&String>, +) -> Result<(), ApiError> { + if !update.has_changes() && image.is_none() { + return Err(AppError::validation("no changes provided").into()); + } + Ok(()) +} + +/// Three-way response for update handlers: Datastar redirect, form redirect, or JSON. +pub(crate) fn update_response( + headers: &HeaderMap, + source: PayloadSource, + detail_url: &str, + json_body: Response, +) -> Result { + if is_datastar_request(headers) { + render_redirect_script(detail_url).map_err(ApiError::from) + } else if matches!(source, PayloadSource::Form) { + Ok(Redirect::to(detail_url).into_response()) + } else { + Ok(json_body) + } +} + #[derive(Debug, Default, Deserialize)] pub(crate) struct ListQuery { page: Option, diff --git a/templates/pages/edit_bag.html b/templates/pages/edit_bag.html index 24d07b2..191eebe 100644 --- a/templates/pages/edit_bag.html +++ b/templates/pages/edit_bag.html @@ -1,5 +1,6 @@ {% extends "base.html" %} {% import "partials/icons.html" as icons %} +{% import "partials/detail_cards.html" as detail_cards %} {% block title %}Brewlog · Edit Bag{% endblock %} {% block content %} @@ -75,35 +76,7 @@ /> - - -
- - -
+ {{ detail_cards::edit_form_actions() }} {% endblock %} diff --git a/templates/pages/edit_brew.html b/templates/pages/edit_brew.html index f604be4..8faea3d 100644 --- a/templates/pages/edit_brew.html +++ b/templates/pages/edit_brew.html @@ -1,6 +1,7 @@ {% extends "base.html" %} {% import "partials/icons.html" as icons %} {% import "partials/image_section.html" as img %} +{% import "partials/detail_cards.html" as detail_cards %} {% block title %}Brewlog · Edit Brew{% endblock %} {% block content %} @@ -361,35 +362,7 @@ {{ img::deferred_upload_with_preview("edit-brew-image", "Brew Image", "brew", id, image_url) }} - - -
- - -
+ {{ detail_cards::edit_form_actions() }} {% endblock %} diff --git a/templates/pages/edit_cafe.html b/templates/pages/edit_cafe.html index 5b3960e..798ece2 100644 --- a/templates/pages/edit_cafe.html +++ b/templates/pages/edit_cafe.html @@ -1,6 +1,7 @@ {% extends "base.html" %} {% import "partials/icons.html" as icons %} {% import "partials/image_section.html" as img %} +{% import "partials/detail_cards.html" as detail_cards %} {% block title %}Brewlog · Edit Cafe{% endblock %} {% block content %} @@ -118,35 +119,7 @@ {{ img::deferred_upload_with_preview("edit-cafe-image", "Cafe Image", "cafe", id, image_url) }} - - -
- - -
+ {{ detail_cards::edit_form_actions() }} {% endblock %} diff --git a/templates/pages/edit_cup.html b/templates/pages/edit_cup.html index 6b096e0..9206464 100644 --- a/templates/pages/edit_cup.html +++ b/templates/pages/edit_cup.html @@ -1,6 +1,7 @@ {% extends "base.html" %} {% import "partials/icons.html" as icons %} {% import "partials/image_section.html" as img %} +{% import "partials/detail_cards.html" as detail_cards %} {% block title %}Brewlog · Edit Cup{% endblock %} {% block content %} @@ -72,35 +73,7 @@ {{ img::deferred_upload_with_preview("edit-cup-image", "Cup Image", "cup", id, image_url) }} - - -
- - -
+ {{ detail_cards::edit_form_actions() }} {% endblock %} diff --git a/templates/pages/edit_gear.html b/templates/pages/edit_gear.html index 63bcec5..8679740 100644 --- a/templates/pages/edit_gear.html +++ b/templates/pages/edit_gear.html @@ -1,6 +1,7 @@ {% extends "base.html" %} {% import "partials/icons.html" as icons %} {% import "partials/image_section.html" as img %} +{% import "partials/detail_cards.html" as detail_cards %} {% block title %}Brewlog · Edit Gear{% endblock %} {% block content %} @@ -64,35 +65,7 @@ {{ img::deferred_upload_with_preview("edit-gear-image", "Gear Image", "gear", id, image_url) }} - - -
- - -
+ {{ detail_cards::edit_form_actions() }} {% endblock %} diff --git a/templates/pages/edit_roast.html b/templates/pages/edit_roast.html index 5b9b79a..9611b27 100644 --- a/templates/pages/edit_roast.html +++ b/templates/pages/edit_roast.html @@ -1,6 +1,7 @@ {% extends "base.html" %} {% import "partials/icons.html" as icons %} {% import "partials/image_section.html" as img %} +{% import "partials/detail_cards.html" as detail_cards %} {% block title %}Brewlog · Edit Roast{% endblock %} {% block content %} @@ -140,35 +141,7 @@ {{ img::deferred_upload_with_preview("edit-roast-image", "Roast Image", "roast", id, image_url) }} - - -
- - -
+ {{ detail_cards::edit_form_actions() }} {% endblock %} diff --git a/templates/pages/edit_roaster.html b/templates/pages/edit_roaster.html index fec1d63..bb0c128 100644 --- a/templates/pages/edit_roaster.html +++ b/templates/pages/edit_roaster.html @@ -1,6 +1,7 @@ {% extends "base.html" %} {% import "partials/icons.html" as icons %} {% import "partials/image_section.html" as img %} +{% import "partials/detail_cards.html" as detail_cards %} {% block title %}Brewlog · Edit Roaster{% endblock %} {% block content %} @@ -82,35 +83,7 @@ {{ img::deferred_upload_with_preview("edit-roaster-image", "Roaster Image", "roaster", id, image_url) }} - - -
- - -
+ {{ detail_cards::edit_form_actions() }} {% endblock %} diff --git a/templates/partials/detail_cards.html b/templates/partials/detail_cards.html index 221ef8c..0db14ae 100644 --- a/templates/partials/detail_cards.html +++ b/templates/partials/detail_cards.html @@ -146,6 +146,38 @@ {% endmacro %} +{% macro edit_form_actions() %} + + +
+ + +
+{% endmacro %} + {% macro edit_delete_buttons(edit_url, entity_label, api_path, id) %}