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()) + } +}