From e374a0573a74fda00d563baf9c504c0e87aed7fc Mon Sep 17 00:00:00 2001 From: SiteRelEnby <125829806+SiteRelEnby@users.noreply.github.com> Date: Wed, 30 Sep 2026 04:01:01 -0400 Subject: [PATCH] fix(history): record a past front without disturbing the live one Adding a history entry created it open and then patched it closed. Between those two calls the new entry IS the current front, and no replace_fronts was passed so the system default applied, which is on. The create therefore ended whoever was actually fronting and back-dated their end to the historical start. Where that start preceded the live front's own start, the end landed before its own beginning, ck_fronts_ended_after_started rejected it and the whole request 500d: back-dating failed with "Internal server error" whenever anybody was fronting, which is the common case, and appeared to work only when nobody was. Where the start did not precede it, there was no constraint to trip and the live front was simply ended in the past without a word. The constraint is the only thing that stood between this pattern and quiet data loss. POST /v1/fronts now takes ended_at and records a closed entry in one call, skipping both live-roster behaviours: no auto-end, and no refusal of a member set that is already fronting, so noting that the same people fronted last week works while they are fronting today. The request also sends replace_fronts=false whenever an end is supplied. A server that understands ended_at ignores it, since a closed entry replaces nothing. A server that predates the field drops ended_at and opens a front instead, and this is what stops that front taking the live one with it; the entry comes back open, which the caller notices and closes with the old PATCH. That is detection rather than a version check, and the fallback is now the safe version of what it used to do. Starting a front is untouched: with no end supplied, replace_fronts stays null and the system default decides, which is what it is for. Every other createFront call in the app is a live switch with a started_at of now and is unaffected. --- CHANGELOG.md | 10 ++ .../systems/lupine/sheaf/data/model/Models.kt | 13 ++ .../sheaf/ui/history/HistoryViewModel.kt | 24 +++- .../sheaf/ui/history/AddFrontEntryTest.kt | 136 ++++++++++++++++++ 4 files changed, 181 insertions(+), 2 deletions(-) create mode 100644 sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/AddFrontEntryTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index b5fe50f..767292c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,16 @@ uses semantic versioning (`MAJOR.MINOR.PATCH`). ### Fixed +- **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 + was ended, and their end was back-dated to the historical start. Where that + start was earlier than the live front began, the whole thing failed with + "Internal server error", which is why this only ever seemed to work when + nobody was fronting. Entries are now recorded in one step that leaves the + live roster alone. On a server too old to support that, the app takes the + older path without the part that ended the current front. + - **Re-authentication now reaches the server when editing members, groups and system settings.** The password and code you typed were dropped before the request went out, so a change that needed them could never save. diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt index 5574a47..cab0bce 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt @@ -624,6 +624,19 @@ data class FrontRead( data class FrontCreate( @Json(name = "member_ids") val memberIds: List, @Json(name = "started_at") val startedAt: String? = null, + /** + * Set to record a front that already happened, rather than start one now. + * + * A closed entry can never be the current front, so a server that + * understands this field skips both live-roster behaviours for it: it does + * not auto-end the open fronts, and it does not refuse a member set that is + * already fronting. Omit it and the call starts a front exactly as before. + * + * A server that predates the field ignores it and creates an open front, + * which is why [replaceFronts] must be sent as false alongside it; see + * HistoryViewModel.addFrontEntry. + */ + @Json(name = "ended_at") val endedAt: String? = null, // null = let the server fall back to system.replace_fronts_default. @Json(name = "replace_fronts") val replaceFronts: Boolean? = null, @Json(name = "custom_status") val customStatus: String? = null, 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 c3b53fd..58b4ae8 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 @@ -348,14 +348,34 @@ class HistoryViewModel @Inject constructor( FrontCreate( memberIds = memberIds, startedAt = startedAt, + // Recording history, not switching. The server skips + // the live-roster behaviours for a closed entry. + endedAt = endedAt, + // Load-bearing on a server that predates ended_at, + // which ignores it and opens a front instead. Without + // this, that front replaces the live one and back-dates + // its end to the historical start: where that precedes + // the live front's own start the database constraint + // rejects it and the whole request 500s, and where it + // does not, whoever was really fronting is silently + // ended in the past. A server that does understand + // ended_at ignores this, since a closed entry never + // replaces anything. + replaceFronts = if (endedAt != null) false else null, customStatus = customStatus, ) ) - if (endedAt != null) api.updateFront(front.id, FrontUpdate(endedAt = endedAt)) + // Detection rather than a version check: if the entry came back + // open despite an end being asked for, the server is old and + // dropped the field. Closing it now is safe because nothing was + // replaced on the way in. + if (endedAt != null && front.endedAt == null) { + api.updateFront(front.id, FrontUpdate(endedAt = endedAt)) + } }.onSuccess { loadInitial() }.onFailure { e -> - _state.update { it.copy(error = e.toUserMessage()) } + _state.update { it.copy(error = e.toUserMessage("Couldn't add the front entry")) } } } } diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/AddFrontEntryTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/AddFrontEntryTest.kt new file mode 100644 index 0000000..71ac3db --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/history/AddFrontEntryTest.kt @@ -0,0 +1,136 @@ +package systems.lupine.sheaf.ui.history + +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.mockk +import io.mockk.slot +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.flowOf +import retrofit2.Response +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import systems.lupine.sheaf.MainDispatcherRule +import systems.lupine.sheaf.data.api.SheafApiService +import systems.lupine.sheaf.data.db.LocalCache +import systems.lupine.sheaf.data.model.FrontCreate +import systems.lupine.sheaf.data.model.FrontRead +import systems.lupine.sheaf.data.network.NetworkMonitor +import systems.lupine.sheaf.data.repository.PreferencesRepository +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * Writing down a front that already happened. + * + * This was create-then-patch: open the entry, then close it. Between the two + * calls the new entry IS the current front and replace_fronts_default is on, + * so the create ended whoever was really fronting and back-dated their end to + * the historical start. Where that start preceded the live front's own start + * the database constraint rejected it and the request 500d; where it did not, + * the live front was silently ended in the past. Reproduced against a running + * server, not reasoned about. + */ +class AddFrontEntryTest { + + @get:Rule val mainDispatcher = MainDispatcherRule() + + private val api = mockk(relaxed = true) + private val cache = mockk(relaxed = true) + private val networkMonitor = mockk(relaxed = true) + private val prefs = mockk(relaxed = true) + + private fun front(id: String, startedAt: String, endedAt: String?) = FrontRead( + id = id, + systemId = "s1", + memberIds = listOf("m1"), + startedAt = startedAt, + endedAt = endedAt, + customStatus = null, + ) + + private fun viewModel(): HistoryViewModel { + every() + return HistoryViewModel(api, cache, networkMonitor, prefs) + } + + private fun every() { + coEvery { prefs.historyView } returns flowOf("infinite") + coEvery { prefs.historyPageSize } returns flowOf(50) + coEvery { networkMonitor.isOnline } returns MutableStateFlow(true) + coEvery { api.listFrontsPaginated(any(), any(), any(), any()) } returns + Response.success(emptyList()) + coEvery { api.listMembers() } returns emptyList() + } + + @Test fun `a closed entry is one call that replaces nothing`() = runTest { + val body = slot() + coEvery { api.createFront(capture(body)) } answers { + front("f1", body.captured.startedAt!!, body.captured.endedAt) + } + val vm = viewModel() + advanceUntilIdle() + + vm.addFrontEntry( + memberIds = listOf("m1"), + startedAt = "2026-01-02T10:00:00Z", + endedAt = "2026-01-02T12:30:00Z", + customStatus = null, + ) + advanceUntilIdle() + + assertEquals("2026-01-02T12:30:00Z", body.captured.endedAt) + // The whole bug in one assertion: a historical entry must never take + // the live roster with it. + assertEquals(false, body.captured.replaceFronts) + // Nothing to close afterwards, so no second call. + coVerify(exactly = 0) { api.updateFront(any(), any()) } + } + + @Test fun `an ongoing entry is still a switch`() = runTest { + val body = slot() + coEvery { api.createFront(capture(body)) } answers { + front("f2", body.captured.startedAt!!, null) + } + val vm = viewModel() + advanceUntilIdle() + + vm.addFrontEntry( + memberIds = listOf("m1"), + startedAt = "2026-01-02T10:00:00Z", + endedAt = null, + customStatus = null, + ) + advanceUntilIdle() + + assertNull(body.captured.endedAt) + // Null, not false: starting a front is exactly what replace_fronts is + // for, and the system's own default decides. + assertNull(body.captured.replaceFronts) + } + + @Test fun `a server that drops ended_at gets the entry closed afterwards`() = runTest { + // Detection rather than a version check. An older server ignores the + // field and hands back an open front; closing it then is safe because + // replace_fronts=false meant nothing was ended on the way in. + val body = slot() + coEvery { api.createFront(capture(body)) } answers { + front("f3", body.captured.startedAt!!, endedAt = null) + } + val vm = viewModel() + advanceUntilIdle() + + vm.addFrontEntry( + memberIds = listOf("m1"), + startedAt = "2026-01-02T10:00:00Z", + endedAt = "2026-01-02T12:30:00Z", + customStatus = null, + ) + advanceUntilIdle() + + coVerify(exactly = 1) { + api.updateFront("f3", match { it.endedAt == "2026-01-02T12:30:00Z" }) + } + } +}