From 937ee4c5f21dd850f683b1daf3094345a495cd37 Mon Sep 17 00:00:00 2001 From: Jon Seager Date: Tue, 24 Feb 2026 21:38:39 +0000 Subject: [PATCH] feat: store bag finished_at as full datetime for correct timeline ordering The finished_at field previously stored only a date, causing bag "finished" timeline events to sort before same-day brews. Now stores a full datetime and uses the actual close time for timeline ordering. --- migrations/0008_bag_finished_at_datetime.sql | 34 +++++++++++++ src/application/routes/api/coffee/bags.rs | 7 ++- src/application/services/bags.rs | 2 +- src/application/services/timeline_refresh.rs | 25 +++++++--- src/domain/coffee/bags.rs | 49 +++++++++++++++++-- src/infrastructure/backup.rs | 2 +- src/infrastructure/client/bags.rs | 2 +- .../repositories/coffee/bags.rs | 4 +- src/presentation/cli/bags.rs | 4 +- src/presentation/cli/mod.rs | 14 ++++++ tests/server/bags_api.rs | 12 ++--- 11 files changed, 129 insertions(+), 26 deletions(-) create mode 100644 migrations/0008_bag_finished_at_datetime.sql diff --git a/migrations/0008_bag_finished_at_datetime.sql b/migrations/0008_bag_finished_at_datetime.sql new file mode 100644 index 0000000..570b5cc --- /dev/null +++ b/migrations/0008_bag_finished_at_datetime.sql @@ -0,0 +1,34 @@ +-- Convert date-only finished_at values to full ISO 8601 datetimes. +-- Going forward, finished_at stores full datetimes for correct timeline +-- ordering (bag "finished" events must sort after same-day brews). +-- +-- Strategy: set finished_at to 1 minute after the last brew from that bag. +-- If no brews exist for the bag, fall back to end-of-day (23:59:59Z). +UPDATE bags +SET finished_at = COALESCE( + ( + SELECT strftime('%Y-%m-%dT%H:%M:%fZ', brews.created_at, '+1 minute') + FROM brews + WHERE brews.bag_id = bags.id + ORDER BY brews.created_at DESC + LIMIT 1 + ), + finished_at || 'T23:59:59Z' +) +WHERE finished_at IS NOT NULL + AND LENGTH(finished_at) = 10; + +-- Fix existing timeline events so bag "finished" uses the actual finished_at. +UPDATE timeline_events +SET occurred_at = ( + SELECT bags.finished_at + FROM bags + WHERE bags.id = timeline_events.entity_id +) +WHERE entity_type = 'bag' + AND action = 'finished' + AND EXISTS ( + SELECT 1 FROM bags + WHERE bags.id = timeline_events.entity_id + AND bags.finished_at IS NOT NULL + ); diff --git a/src/application/routes/api/coffee/bags.rs b/src/application/routes/api/coffee/bags.rs index ff555da..a186e02 100644 --- a/src/application/routes/api/coffee/bags.rs +++ b/src/application/routes/api/coffee/bags.rs @@ -136,8 +136,11 @@ pub(crate) struct UpdateBagSubmission { remaining: Option, #[serde(default)] closed: Option, - #[serde(default)] - finished_at: Option, + #[serde( + default, + deserialize_with = "crate::domain::bags::deserialize_flexible_finished_at" + )] + finished_at: Option>, #[serde(default)] created_at: Option>, #[serde(default)] diff --git a/src/application/services/bags.rs b/src/application/services/bags.rs index 1317063..c38e857 100644 --- a/src/application/services/bags.rs +++ b/src/application/services/bags.rs @@ -43,7 +43,7 @@ impl BagService { /// repository, and records a "finished" timeline event. pub async fn finish(&self, id: BagId, mut update: UpdateBag) -> Result { if update.finished_at.is_none() { - update.finished_at = Some(chrono::Utc::now().date_naive()); + update.finished_at = Some(chrono::Utc::now()); } let bag = self.bag_repo.update(id, update).await?; self.record_timeline_event(&bag, "finished").await; diff --git a/src/application/services/timeline_refresh.rs b/src/application/services/timeline_refresh.rs index b0a8a31..05f2312 100644 --- a/src/application/services/timeline_refresh.rs +++ b/src/application/services/timeline_refresh.rs @@ -294,15 +294,29 @@ async fn refresh_entity_event( roast_timeline_event(&rwr.roast, &roaster) } EntityType::Bag => { + // Bags have two timeline events ("added" + optional "finished"), + // so delete-and-reinsert to keep occurred_at in sync with finished_at. let bwr = rebuilder .bag_repo .get_with_roast(BagId::new(entity_id)) .await?; let roast = rebuilder.roast_repo.get(bwr.bag.roast_id).await?; let roaster = rebuilder.roaster_repo.get(roast.roaster_id).await?; - // update_by_entity updates all events for this entity, - // preserving each event's original action ("added" or "finished") - bag_timeline_event(&bwr.bag, "added", &roast, &roaster) + rebuilder + .timeline_repo + .delete_by_entity(entity_type, entity_id) + .await?; + rebuilder + .timeline_repo + .insert(bag_timeline_event(&bwr.bag, "added", &roast, &roaster)) + .await?; + if bwr.bag.closed { + rebuilder + .timeline_repo + .insert(bag_timeline_event(&bwr.bag, "finished", &roast, &roaster)) + .await?; + } + return Ok(()); } EntityType::Brew => { let enriched = rebuilder @@ -458,10 +472,7 @@ async fn rebuild_bag_events( warn!(error = %err, id = %bwr.bag.id, "failed to rebuild bag 'added' timeline event"); } if bwr.bag.closed { - let mut finished_event = bag_timeline_event(&bwr.bag, "finished", &roast, &roaster); - if let Some(finished_at) = bwr.bag.finished_at { - finished_event.occurred_at = finished_at.and_time(chrono::NaiveTime::MIN).and_utc(); - } + let finished_event = bag_timeline_event(&bwr.bag, "finished", &roast, &roaster); if let Err(err) = rebuilder.timeline_repo.insert(finished_event).await { warn!(error = %err, id = %bwr.bag.id, "failed to rebuild bag 'finished' timeline event"); } diff --git a/src/domain/coffee/bags.rs b/src/domain/coffee/bags.rs index f559cad..854b80a 100644 --- a/src/domain/coffee/bags.rs +++ b/src/domain/coffee/bags.rs @@ -1,4 +1,4 @@ -use chrono::{DateTime, NaiveDate, Utc}; +use chrono::{DateTime, NaiveDate, NaiveTime, Utc}; use serde::{Deserialize, Serialize}; use crate::define_sort_key; @@ -8,6 +8,40 @@ use crate::domain::roasters::Roaster; use crate::domain::roasts::Roast; use crate::domain::timeline::{NewTimelineEvent, TimelineEventDetail}; +/// 23:59:59 — used when converting a date-only `finished_at` to a datetime +/// so that bag "finished" events sort after same-day brews. +pub const END_OF_DAY: NaiveTime = match NaiveTime::from_hms_opt(23, 59, 59) { + Some(t) => t, + None => unreachable!(), +}; + +/// Deserializes a datetime that accepts both RFC 3339 (`2025-02-24T15:30:00Z`) +/// and date-only (`2025-02-24`) formats. Date-only values become 23:59:59 UTC +/// so bag "finished" events sort after same-day brews. +pub(crate) fn deserialize_flexible_finished_at<'de, D>( + deserializer: D, +) -> Result>, D::Error> +where + D: serde::Deserializer<'de>, +{ + let opt: Option = Option::deserialize(deserializer)?; + match opt { + None => Ok(None), + Some(s) if s.is_empty() => Ok(None), + Some(s) => { + if let Ok(dt) = DateTime::parse_from_rfc3339(&s) { + Ok(Some(dt.with_timezone(&Utc))) + } else if let Ok(date) = NaiveDate::parse_from_str(&s, "%Y-%m-%d") { + Ok(Some(date.and_time(END_OF_DAY).and_utc())) + } else { + Err(serde::de::Error::custom(format!( + "invalid finished_at: expected RFC 3339 or YYYY-MM-DD, got: {s}" + ))) + } + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Bag { pub id: BagId, @@ -16,7 +50,8 @@ pub struct Bag { pub amount: f64, pub remaining: f64, pub closed: bool, - pub finished_at: Option, + #[serde(default, deserialize_with = "deserialize_flexible_finished_at")] + pub finished_at: Option>, pub created_at: DateTime, pub updated_at: DateTime, } @@ -50,7 +85,8 @@ pub struct UpdateBag { pub amount: Option, pub remaining: Option, pub closed: Option, - pub finished_at: Option, + #[serde(default, deserialize_with = "deserialize_flexible_finished_at")] + pub finished_at: Option>, #[serde(default, skip_serializing_if = "Option::is_none")] pub created_at: Option>, } @@ -110,11 +146,16 @@ pub fn bag_timeline_event( roast: &Roast, roaster: &Roaster, ) -> NewTimelineEvent { + let occurred_at = if action == "finished" { + bag.finished_at.unwrap_or(bag.created_at) + } else { + bag.created_at + }; NewTimelineEvent { entity_type: EntityType::Bag, entity_id: bag.id.into_inner(), action: action.to_string(), - occurred_at: bag.created_at, + occurred_at, title: roast.name.clone(), details: vec![ TimelineEventDetail { diff --git a/src/infrastructure/backup.rs b/src/infrastructure/backup.rs index c5d74c3..4d4a85d 100644 --- a/src/infrastructure/backup.rs +++ b/src/infrastructure/backup.rs @@ -685,7 +685,7 @@ struct BagRecord { amount: f64, remaining: f64, closed: bool, - finished_at: Option, + finished_at: Option>, created_at: DateTime, updated_at: DateTime, } diff --git a/src/infrastructure/client/bags.rs b/src/infrastructure/client/bags.rs index 4dcbd06..a6c9c10 100644 --- a/src/infrastructure/client/bags.rs +++ b/src/infrastructure/client/bags.rs @@ -77,7 +77,7 @@ impl<'a> BagsClient<'a> { id: BagId, remaining: Option, closed: Option, - finished_at: Option, + finished_at: Option>, created_at: Option>, ) -> Result { let url = self.inner.endpoint(&format!("api/v1/bags/{id}"))?; diff --git a/src/infrastructure/repositories/coffee/bags.rs b/src/infrastructure/repositories/coffee/bags.rs index ad22c1a..7c75edf 100644 --- a/src/infrastructure/repositories/coffee/bags.rs +++ b/src/infrastructure/repositories/coffee/bags.rs @@ -227,7 +227,7 @@ struct BagRecord { amount: f64, remaining: f64, closed: bool, - finished_at: Option, + finished_at: Option>, created_at: DateTime, updated_at: DateTime, } @@ -256,7 +256,7 @@ struct BagWithRoastRecord { amount: f64, remaining: f64, closed: bool, - finished_at: Option, + finished_at: Option>, created_at: DateTime, updated_at: DateTime, roast_name: String, diff --git a/src/presentation/cli/bags.rs b/src/presentation/cli/bags.rs index f6e4b42..3fcc6a0 100644 --- a/src/presentation/cli/bags.rs +++ b/src/presentation/cli/bags.rs @@ -2,8 +2,8 @@ use anyhow::Result; use clap::{Args, Subcommand}; use super::macros::{define_delete_command, define_get_command}; -use super::parse_created_at; use super::print_json; +use super::{parse_created_at, parse_finished_at}; use crate::domain::ids::{BagId, RoastId}; use crate::infrastructure::client::BrewlogClient; @@ -99,7 +99,7 @@ pub struct UpdateBagCommand { pub async fn update_bag(client: &BrewlogClient, command: UpdateBagCommand) -> Result<()> { let finished_at = command .finished_at - .map(|d| chrono::NaiveDate::parse_from_str(&d, "%Y-%m-%d")) + .map(|d| parse_finished_at(&d)) .transpose()?; let created_at = command .created_at diff --git a/src/presentation/cli/mod.rs b/src/presentation/cli/mod.rs index e5be939..9a43a46 100644 --- a/src/presentation/cli/mod.rs +++ b/src/presentation/cli/mod.rs @@ -158,6 +158,20 @@ pub fn parse_created_at(value: &str) -> anyhow::Result> { ) } +/// Like `parse_created_at` but date-only values become 23:59:59 UTC so +/// bag "finished" events sort after same-day brews. +pub fn parse_finished_at(value: &str) -> anyhow::Result> { + if let Ok(dt) = DateTime::parse_from_rfc3339(value) { + return Ok(dt.with_timezone(&Utc)); + } + if let Ok(date) = NaiveDate::parse_from_str(value, "%Y-%m-%d") { + return Ok(date.and_time(crate::domain::bags::END_OF_DAY).and_utc()); + } + anyhow::bail!( + "invalid date format: expected RFC 3339 (e.g. 2025-08-05T10:00:00Z) or YYYY-MM-DD" + ) +} + pub(crate) fn print_json(value: &T) -> anyhow::Result<()> where T: serde::Serialize, diff --git a/tests/server/bags_api.rs b/tests/server/bags_api.rs index ee4055e..ba86baf 100644 --- a/tests/server/bags_api.rs +++ b/tests/server/bags_api.rs @@ -4,7 +4,7 @@ use crate::helpers::{ }; use crate::test_macros::define_crud_tests; use brewlog::domain::bags::{Bag, BagWithRoast, NewBag, UpdateBag}; -use chrono::NaiveDate; +use chrono::{NaiveDate, TimeZone, Utc}; define_crud_tests!( entity: bag, @@ -186,7 +186,7 @@ async fn updating_a_bag_returns_200_and_updates_data() { let update_payload = UpdateBag { remaining: Some(100.0), closed: Some(true), - finished_at: Some(NaiveDate::from_ymd_opt(2023, 2, 1).unwrap()), + finished_at: Some(Utc.with_ymd_and_hms(2023, 2, 1, 23, 59, 59).unwrap()), ..Default::default() }; @@ -206,7 +206,7 @@ async fn updating_a_bag_returns_200_and_updates_data() { assert!(updated_bag.closed); assert_eq!( updated_bag.finished_at, - Some(NaiveDate::from_ymd_opt(2023, 2, 1).unwrap()) + Some(Utc.with_ymd_and_hms(2023, 2, 1, 23, 59, 59).unwrap()) ); } @@ -308,10 +308,10 @@ async fn closing_a_bag_automatically_sets_finished_at() { // Accept today or yesterday to avoid midnight-boundary flakiness let today = chrono::Utc::now().date_naive(); let yesterday = today - chrono::Duration::days(1); - let finished = updated_bag.finished_at.unwrap(); + let finished_date = updated_bag.finished_at.unwrap().date_naive(); assert!( - finished == today || finished == yesterday, - "expected finished_at to be today or yesterday, got {finished}" + finished_date == today || finished_date == yesterday, + "expected finished_at date to be today or yesterday, got {finished_date}" ); }