From 95b36beb60fd69a27278c66a23e043579e576fe1 Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:52:49 -0400 Subject: [PATCH] fix(history): say why a back-dated ongoing entry cannot be saved An entry left open is a switch rather than history, so the server ends every open front at its start. Back-date that start to before somebody currently fronting began, and their end would land before their own beginning: ck_fronts_ended_after_started refuses it and the request answers 500, which surfaces as "Internal server error" with nothing to act on. 5xx keeps the generic wording on purpose, so there was no way to explain it after the fact. The sheet now checks before sending, names who it clashes with and when they started, and offers the two ways out: a later start, or an end time, which records it as history and leaves the live roster alone. The check is narrow on purpose. Editing never replaces anything, a closed entry is recorded without touching the live roster, and with the system's replace default off nothing is ended in the first place; all three skip it. Equal timestamps pass, since a front ending at the instant it began is a zero-length entry rather than a backwards one, which the constraint allows. A started_at this client cannot parse is left out rather than treated as a clash: the server is the authority on its own rows, and refusing a legitimate entry over a formatting disagreement is the worse error. Who is fronting comes from /v1/fronts/current when the sheet opens, not from the loaded history page. The page is paginated, so an open front is only probably on it, and a validation that is only probably right is worse than none. --- CHANGELOG.md | 7 ++ .../lupine/sheaf/ui/history/HistoryScreen.kt | 55 +++++++++++++- .../sheaf/ui/history/HistoryViewModel.kt | 56 +++++++++++++++ .../sheaf/ui/history/OpenFrontClashTest.kt | 72 +++++++++++++++++++ 4 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/OpenFrontClashTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index 4933172..06ef95e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,13 @@ uses semantic versioning (`MAJOR.MINOR.PATCH`). ### Fixed +- **Adding an ongoing front entry explains itself instead of failing.** An + entry left open starts a front, which ends the ones already running. If you + back-dated it to before someone currently fronting began, that was impossible + and the server answered "Internal server error". The sheet now says who it + clashes with and when they started, and suggests a later start or giving the + entry an end time to record it as history. + - **Adding a past front entry no longer disturbs who is fronting now.** The app created the entry as if it were a switch and then closed it, so between those two steps it counted as the current front: whoever was really fronting diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryScreen.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryScreen.kt index 2121b12..975befb 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryScreen.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryScreen.kt @@ -162,11 +162,18 @@ fun HistoryScreen( } if (showAddSheet) { + // Fresh every time the sheet opens: who is fronting can change between + // openings, and a stale answer either blocks a valid entry or waves + // through the one that 500s. + LaunchedEffect(Unit) { viewModel.loadOpenFrontContext() } FrontEntrySheet( allMembers = state.allMembers, initialFront = null, groups = state.groups, memberGroups = state.memberGroups, + openFronts = state.openFronts, + replaceFrontsDefault = state.replaceFrontsDefault, + members = state.members, onDismiss = { showAddSheet = false }, onConfirm = { memberIds, startedAt, endedAt, customStatus -> viewModel.addFrontEntry(memberIds, startedAt, endedAt, customStatus) @@ -181,6 +188,10 @@ fun HistoryScreen( initialFront = front, groups = state.groups, memberGroups = state.memberGroups, + // Editing never replaces anything, so the check does not apply. + openFronts = emptyList(), + replaceFrontsDefault = state.replaceFrontsDefault, + members = state.members, onDismiss = { frontToEdit = null }, onConfirm = { memberIds, startedAt, endedAt, customStatus -> viewModel.updateFrontEntry(front.id, memberIds, startedAt, endedAt, customStatus) @@ -704,6 +715,10 @@ private fun FrontEntrySheet( initialFront: FrontRead?, groups: List, memberGroups: Map>, + // Only consulted for a new entry left open; see the check below. + openFronts: List, + replaceFrontsDefault: Boolean, + members: Map, onDismiss: () -> Unit, onConfirm: (memberIds: List, startedAt: String, endedAt: String?, customStatus: String?) -> Unit, ) { @@ -907,6 +922,44 @@ private fun FrontEntrySheet( ) } + // A new entry left open is a switch, not history: the server ends + // every open front at this start. Back-date it past when one of + // them began and that end would land before its own beginning, + // which the database refuses and the whole request 500s on. Only + // for a new open entry: editing never replaces anything, a closed + // entry is recorded without touching the live roster, and with + // replace off nothing gets ended in the first place. + val clashes = remember(isEditing, stillOngoing, replaceFrontsDefault, openFronts, startInstant) { + if (isEditing || !stillOngoing || !replaceFrontsDefault) emptyList() + else openFrontsStartingAfter(startInstant, openFronts) + } + if (clashes.isNotEmpty()) { + val who = clashes + .flatMap { it.memberIds } + .distinct() + .mapNotNull { members[it]?.displayNameOrName } + .ifEmpty { listOf("whoever is fronting") } + val earliest = clashes + .mapNotNull { runCatching { Instant.parse(it.startedAt) }.getOrNull() } + .minOrNull() + Text( + buildString { + append("An entry left open starts a front, which ends the ones already ") + append("running. ") + append(who.joinToString(", ")) + append(if (who.size == 1) " has been fronting" else " have been fronting") + earliest?.let { + append(" since ${formatTimestamp(it.toString(), LocalDisplayTimeZone.current)}") + } + append(", which is after this start, so it can't be ended there. ") + append("Pick a later start, or give this entry an end time to record it ") + append("as history instead.") + }, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.error, + ) + } + Button( onClick = { val statusOut = customStatusDraft.trim().ifEmpty { null } @@ -917,7 +970,7 @@ private fun FrontEntrySheet( statusOut, ) }, - enabled = selectedIds.isNotEmpty() && !endBeforeStart, + enabled = selectedIds.isNotEmpty() && !endBeforeStart && clashes.isEmpty(), modifier = Modifier.fillMaxWidth(), ) { Text(if (isEditing) "Save Changes" else "Add Entry") } } diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryViewModel.kt index 58b4ae8..9ebadca 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/history/HistoryViewModel.kt @@ -34,6 +34,10 @@ data class HistoryUiState( val deleteError: String? = null, val groups: List = emptyList(), val memberGroups: Map> = emptyMap(), + // Who is fronting right now, and whether starting a front would end them. + // Both only for validating a new open entry; see loadOpenFrontContext. + val openFronts: List = emptyList(), + val replaceFrontsDefault: Boolean = true, // Pagination config. val view: HistoryView = HistoryView.Infinite, val pageSize: Int = DEFAULT_PAGE_SIZE, @@ -53,6 +57,31 @@ data class HistoryUiState( val PAGE_SIZE_OPTIONS = listOf(25, 50, 100, 200) +/** + * The open fronts that a new front starting at [startedAt] could not legally + * end, because they had not begun yet. + * + * Starting a front ends every open one *at the new start*. An open front that + * began after that moment would get an end before its own beginning, which + * `ck_fronts_ended_after_started` refuses, taking the whole request down with + * it. Empty means the start is safe. + * + * Equal timestamps are fine: a front that began at exactly this instant ends + * at exactly this instant, which is a zero-length entry rather than a + * backwards one, and the constraint allows it. + * + * A front with an unparseable `started_at` is left out rather than treated as + * a clash: the server is the authority on its own rows, and guessing here + * would block a legitimate entry over a timestamp this client could not read. + */ +fun openFrontsStartingAfter( + startedAt: java.time.Instant, + openFronts: List, +): List = openFronts.filter { front -> + val began = runCatching { java.time.Instant.parse(front.startedAt) }.getOrNull() + began != null && began.isAfter(startedAt) +} + @HiltViewModel class HistoryViewModel @Inject constructor( private val api: SheafApiService, @@ -412,6 +441,33 @@ class HistoryViewModel @Inject constructor( fun clearError() { _state.update { it.copy(error = null) } } + /** + * What a new *open* entry would do to the fronts already running. + * + * Only the add sheet needs this, and only for the "still ongoing" case: + * an open entry is a switch, so the server ends every open front at the + * new start. Back-date that start past when one of them began and its end + * lands before its own beginning, which the database refuses and the + * request 500s on. Checking it here turns that into a sentence. + * + * Asked for rather than read out of the loaded page: the history list is + * paginated, so an open front is only *probably* on it, and a validation + * that is only probably right is worse than none. + */ + fun loadOpenFrontContext() { + viewModelScope.launch { + val replaceDefault = runCatching { cache.getSystem()?.replaceFrontsDefault } + .getOrNull() ?: true + val open = runCatching { api.getCurrentFronts() }.getOrNull() + _state.update { + it.copy( + openFronts = open ?: it.openFronts, + replaceFrontsDefault = replaceDefault, + ) + } + } + } + fun loadGroupsForFilter() { if (_state.value.groups.isNotEmpty()) return viewModelScope.launch { diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/OpenFrontClashTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/OpenFrontClashTest.kt new file mode 100644 index 0000000..e06ee90 --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/OpenFrontClashTest.kt @@ -0,0 +1,72 @@ +package systems.lupine.sheaf.ui.history + +import systems.lupine.sheaf.data.model.FrontRead +import java.time.Instant +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Which open fronts a back-dated start would break. + * + * Starting a front ends every open one at the new start. An open front that + * began *after* that moment would be given an end before its own beginning, + * which `ck_fronts_ended_after_started` refuses, and the server answers the + * whole request with a 500 the user cannot act on. This is the check that + * turns that into a sentence before the request goes out. + */ +class OpenFrontClashTest { + + private fun front(id: String, startedAt: String) = FrontRead( + id = id, + systemId = "s1", + memberIds = listOf("m1"), + startedAt = startedAt, + endedAt = null, + customStatus = null, + ) + + private val noon = Instant.parse("2026-10-12T12:00:00Z") + + @Test fun `a start before an open front is the case that breaks`() { + val live = front("f1", "2026-10-12T13:00:00Z") + assertEquals(listOf("f1"), openFrontsStartingAfter(noon, listOf(live)).map { it.id }) + } + + @Test fun `a start after every open front is fine`() { + val live = front("f1", "2026-10-12T09:00:00Z") + assertTrue(openFrontsStartingAfter(noon, listOf(live)).isEmpty()) + } + + @Test fun `an identical start is allowed`() { + // The front would end at the instant it began: a zero-length entry, + // not a backwards one, and the constraint permits it. + val live = front("f1", "2026-10-12T12:00:00Z") + assertTrue(openFrontsStartingAfter(noon, listOf(live)).isEmpty()) + } + + @Test fun `every clashing front is reported, not just the first`() { + // All of them get ended, so all of them have to be named: fixing one + // and resubmitting into the next is a bad way to find out. + val fronts = listOf( + front("early", "2026-10-12T09:00:00Z"), + front("late", "2026-10-12T13:00:00Z"), + front("later", "2026-10-12T14:00:00Z"), + ) + assertEquals( + listOf("late", "later"), + openFrontsStartingAfter(noon, fronts).map { it.id }, + ) + } + + @Test fun `nothing open means nothing to clash with`() { + assertTrue(openFrontsStartingAfter(noon, emptyList()).isEmpty()) + } + + @Test fun `an unreadable timestamp does not block the entry`() { + // The server is the authority on its own rows. Treating a timestamp + // this client cannot parse as a clash would refuse a legitimate entry + // over a formatting disagreement. + assertTrue(openFrontsStartingAfter(noon, listOf(front("f1", "whenever"))).isEmpty()) + } +}