diff --git a/CHANGELOG.md b/CHANGELOG.md index dd2f610..34882b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,15 @@ 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. - **Public is no longer offered where the instance cannot publish.** On an instance with public profiles turned off, choosing Public for a member, group, custom field, relationship or your system was a dead end that failed 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 3f674c9..8c82d44 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 @@ -628,6 +628,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" }) + } + } +}