Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package com.pinakes.app.staffe2e

import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.ViewModelStore
import com.pinakes.app.data.model.BookDetail
import com.pinakes.app.data.model.DigitalAttachment
import com.pinakes.app.data.model.Envelope
import com.pinakes.app.data.network.ApiResult
import com.pinakes.app.data.network.ErrorCodes
import com.pinakes.app.ui.common.UiState
import com.pinakes.app.ui.navigation.Routes
import com.pinakes.app.ui.screens.detail.BookDetailViewModel
import kotlinx.coroutines.runBlocking
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
import org.junit.After
import org.junit.Assert.*
import org.junit.Test

/** Real repository/ViewModel, HTTP cache and access decisions on a disposable emulator. */
class DigitalLinkRefreshFlowTest : AppFlowTest() {
private val old = DigitalAttachment("https://library.example/a.epub?pk_exp=1000&pk_uid=3&pk_sig=old", access = "granted")
private val fresh = old.copy(url = old.url.replace("1000", "4102444800").replace("old", "new"))
private val book = BookDetail(id = 1, title = "Digital", digitalAttachments = listOf(old))
private val store = ViewModelStore()

private fun FakePinakes.answer(data: BookDetail = book) = body("GET catalog/books/1", 200,
Json.encodeToString(Envelope(data = data)), mapOf("ETag" to "\"digital-v1\"", "Cache-Control" to "private, max-age=3600"))

private fun viewModel(): BookDetailViewModel {
lateinit var vm: BookDetailViewModel
instrumentation.runOnMainSync {
vm = BookDetailViewModel(SavedStateHandle(mapOf(Routes.ARG_BOOK_ID to 1)),
deps.catalogRepository(), deps.libraryRepository(), deps.wishlistRepository(), deps.connectivity())
store.put("digital", vm)
}
compose.waitUntil(10_000) { vm.state.value.content is UiState.Success }
return vm
}

@After fun clearViewModels() { instrumentation.runOnMainSync { store.clear() } }

@Test fun forcedRenewalBypassesTheHttpCacheAndGetsANewSignature() {
launch { answer() }
val repository = deps.catalogRepository()
assertTrue(runBlocking { repository.book(1) } is ApiResult.Success)
api.answer(book.copy(digitalAttachments = listOf(fresh)))
val response = runBlocking { repository.book(1, forceRefresh = true) }
assertEquals(fresh.url, (response as ApiResult.Success).data.digitalFiles.single().url)
val request = api.recorded("GET", "catalog/books/1").last()
assertNull(request.getHeader("If-None-Match"))
assertEquals("no-cache, no-store", request.getHeader("Cache-Control"))
assertEquals(2, api.recorded("GET", "catalog/books/1").size)
}

@Test fun aServerFailureDuringRenewalCannotReturnTheCachedSignedLink() {
launch { answer() }
val repository = deps.catalogRepository()
assertTrue(runBlocking { repository.book(1) } is ApiResult.Success)
api.body("GET catalog/books/1", 503, """{"error":{"code":"server_error","message":"Unavailable"}}""")
val response = runBlocking { repository.book(1, forceRefresh = true) }
assertTrue(response is ApiResult.Failure)
assertEquals(503, (response as ApiResult.Failure).httpStatus)
}

@Test fun anUnexpected304CannotPretendTheExpiredPayloadWasRenewed() {
launch { answer() }
val repository = deps.catalogRepository()
assertTrue(runBlocking { repository.book(1) } is ApiResult.Success)
api.body("GET catalog/books/1", 304, "")
assertTrue(runBlocking { repository.book(1, forceRefresh = true) } is ApiResult.Failure)
}

@Test fun renewalFailurePreservesTheLoadedPageAndASubsequentRetrySucceeds() {
launch { answer() }
val vm = viewModel()
val before = vm.state.value.content
api.body("GET catalog/books/1", 500, """{"error":{"code":"server_error","message":"Unavailable"}}""")
assertTrue(runBlocking { vm.refreshDigitalFile(old) } is ApiResult.Failure)
assertSame(before, vm.state.value.content)
api.answer(book.copy(digitalAttachments = listOf(fresh)))
val result = runBlocking { vm.refreshDigitalFile(old) }
assertEquals(fresh.url, (result as ApiResult.Success).data.url)
assertEquals(fresh.url, (vm.state.value.content as UiState.Success).data.digitalFiles.single().url)
}

@Test fun renewalSelectsTheSameFileEvenWhenTheServerReordersAndRenamesEntries() {
launch { answer() }
val vm = viewModel()
val other = fresh.copy(url = fresh.url.replace("a.epub", "b.epub"))
api.answer(book.copy(digitalAttachments = listOf(other, fresh.copy(label = "Renamed"))))
val result = runBlocking { vm.refreshDigitalFile(old) }
assertEquals(fresh.url, (result as ApiResult.Success).data.url)
}

@Test fun revokedFileAccessUpdatesTheCardAndReturnsNoOpenableAddress() {
launch { answer() }
val vm = viewModel()
api.answer(book.copy(digitalAttachments = listOf(old.copy(url = "", access = "loan_required"))))
val result = runBlocking { vm.refreshDigitalFile(old) }
assertEquals(ErrorCodes.FORBIDDEN, (result as ApiResult.Failure).code)
val file = (vm.state.value.content as UiState.Success).data.digitalFiles.single()
assertTrue(file.locked)
assertNull(file.safeUrl)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,11 @@ class FakePinakes {
}
}

fun body(route: String, status: Int, body: String) {
routes[route] = { MockResponse().setResponseCode(status).setHeader("Content-Type", "application/json").setBody(body) }
fun body(route: String, status: Int, body: String, headers: Map<String, String> = emptyMap()) {
routes[route] = {
MockResponse().setResponseCode(status).setHeader("Content-Type", "application/json").setBody(body)
.apply { headers.forEach { (name, value) -> setHeader(name, value) } }
}
}

fun recorded(method: String, path: String): List<RecordedRequest> =
Expand Down
43 changes: 41 additions & 2 deletions app/src/main/java/com/pinakes/app/data/model/Models.kt
Original file line number Diff line number Diff line change
Expand Up @@ -347,17 +347,56 @@ data class BookDetail(
audioUrl?.takeIf { hasAudio && it.isNotBlank() }?.let { DigitalAttachment(it, kind = "audio") },
ebookUrl?.takeIf { hasEbook && it.isNotBlank() }?.let { DigitalAttachment(it, kind = "ebook") },
)
}.filter { it.safeUrl != null }.distinctBy { it.url }
}.filter { it.safeUrl != null || it.locked }.let { files ->
// A closed file has no address (the server sends url = null): several of them are
// all "", so only the open ones are told apart by their address.
val seen = HashSet<String>()
files.filter { it.locked || seen.add(it.url) }
}
}

/**
* One digital file of a record. Servers from Pinakes 0.8.6 (issue #480) say who may open
* it and what this reader may do: a closed file keeps its entry with an empty [url] and
* the reason in [access]. Older servers send neither field, and every file is open.
*/
@Serializable
data class DigitalAttachment(val url: String = "", val label: String = "", val kind: String = "ebook") {
data class DigitalAttachment(
val url: String = "",
val label: String = "",
val kind: String = "ebook",
/** public | users | staff | loan, as resolved by the server; null on older servers. */
val visibility: String? = null,
/** granted | login_required | loan_required | staff_only; null on older servers (= granted). */
val access: String? = null,
) {
val locked: Boolean get() = access != null && access != ACCESS_GRANTED
val safeUrl: String? get() = url.takeIf {
val uri = runCatching { java.net.URI(it) }.getOrNull()
uri?.scheme?.lowercase() in listOf("http", "https") && !uri?.host.isNullOrBlank()
}

/** When a signed link stops working (its pk_exp, in epoch seconds); null for an unsigned link. */
val expiresAt: Long? get() = runCatching { java.net.URI(url).rawQuery }.getOrNull()
?.split('&')?.firstOrNull { it.startsWith("pk_exp=") }?.substringAfter('=')?.toLongOrNull()

/** Identity of the file across signature renewals; keep every non-signature query parameter. */
val resourceKey: String get() {
val base = url.substringBefore('#').substringBefore('?')
val query = runCatching { java.net.URI(url).rawQuery }.getOrNull()
?.split('&')?.filterNot { it.substringBefore('=') in setOf("pk_exp", "pk_uid", "pk_sig") }
?.filter { it.isNotEmpty() }?.sorted().orEmpty()
return base + if (query.isEmpty()) "" else "?" + query.joinToString("&")
}

val format: String get() = runCatching { java.net.URI(url).path.substringAfterLast('.').uppercase() }.getOrDefault("")
val isPdf: Boolean get() = format == "PDF"

companion object {
const val ACCESS_GRANTED = "granted"
const val ACCESS_LOGIN_REQUIRED = "login_required"
const val ACCESS_LOAN_REQUIRED = "loan_required"
}
}

@Serializable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ interface PinakesApi {
suspend fun book(
@Path("id") id: Int,
@Header("If-None-Match") etag: String? = null, // -> 304 cache
@Header("Cache-Control") cacheControl: String? = null,
): Response<Envelope<BookDetail>> // Response<> to read ETag header + 304

@GET("catalog/books/{id}/availability")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,13 +256,15 @@ class CatalogRepository(
}
}

suspend fun book(id: Int): ApiResult<BookDetail> {
/** A signed-file renewal must reach the server and must never fall back to a stale payload. */
suspend fun book(id: Int, forceRefresh: Boolean = false): ApiResult<BookDetail> {
val api = network.api()
val cached = detailCache[id]
val (result, response) = apiResponse { api.book(id, etag = cached?.first) }
val (result, response) = apiResponse { api.book(id, etag = if (forceRefresh) null else cached?.first,
cacheControl = if (forceRefresh) "no-cache, no-store" else null) }

// 304 Not Modified → reuse the cached payload.
if (response?.code() == 304 && cached != null) {
if (!forceRefresh && response?.code() == 304 && cached != null) {
return ApiResult.Success(cached.second)
}
return when (result) {
Expand All @@ -273,7 +275,7 @@ class CatalogRepository(
}
is ApiResult.Failure -> {
// If the conditional request errored but we have a cached copy, serve it.
if (cached != null && result.code != ErrorCodes.NETWORK) {
if (!forceRefresh && cached != null && result.code != ErrorCodes.NETWORK) {
ApiResult.Success(cached.second)
} else {
result
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import androidx.compose.material.icons.outlined.FavoriteBorder
import androidx.compose.material.icons.outlined.HourglassEmpty
import androidx.compose.material.icons.outlined.Schedule
import androidx.compose.material.icons.outlined.LocalOffer
import androidx.compose.material.icons.outlined.Lock
import androidx.compose.material3.CircularProgressIndicator
import androidx.compose.material3.ExperimentalMaterial3Api
import androidx.compose.material3.Icon
Expand Down Expand Up @@ -76,7 +77,10 @@ import com.pinakes.app.R
import com.pinakes.app.data.model.AvailabilityCalendar
import com.pinakes.app.data.model.BookDetail
import com.pinakes.app.data.model.CirculationRequestKind
import com.pinakes.app.data.model.DigitalAttachment
import com.pinakes.app.data.model.PersonalHistory
import com.pinakes.app.data.network.ApiResult
import com.pinakes.app.data.network.ErrorCodes
import com.pinakes.app.ui.common.AppViewModel
import com.pinakes.app.ui.common.OfflinePolicy
import com.pinakes.app.ui.components.PinakesBottomBar
Expand Down Expand Up @@ -200,6 +204,7 @@ fun BookDetailScreen(
onReserve = vm::openLoanSheet,
onToggleWishlist = vm::toggleWishlist,
onFindWorks = onFindWorks,
onRefreshFile = vm::refreshDigitalFile,
onShowMessage = { msg -> scope.launch { snackbarHost.showSnackbar(msg) } },
offlineNotice = if (state.offlineNow) stringResource(R.string.offline_actions_unavailable) else null,
offlineSyncedAt = state.syncedAt,
Expand Down Expand Up @@ -338,16 +343,31 @@ internal fun DetailContent(
onToggleWishlist: () -> Unit,
onShowMessage: (String) -> Unit,
onFindWorks: (String, Int?) -> Unit = { _, _ -> },
/** Revalidates a signed file on the server, preserving this page while the request runs. */
onRefreshFile: suspend (DigitalAttachment) -> ApiResult<DigitalAttachment> = {
ApiResult.Failure(ErrorCodes.NETWORK, "")
},
/** Set when the page is built from the offline cache: shown above the cover. */
offlineNotice: String? = null,
/** Last sync time of the offline copy: availability is shown as "last known", never current. */
offlineSyncedAt: String? = null,
) {
val offline = offlineNotice != null
val context = LocalContext.current
val scope = rememberCoroutineScope()
var refreshingFile by remember(book.id) { mutableStateOf<String?>(null) }
var showCover by remember { mutableStateOf(false) }
var selectedPdf by remember(book.id) { mutableStateOf<String?>(null) }
var selectedAudio by remember(book.id) { mutableStateOf<String?>(book.audioUrl?.takeIf { book.hasAudio }) }
var selectedAudio by remember(book.id) { mutableStateOf<String?>(null) }

fun openFile(file: DigitalAttachment) {
if (file.locked || file.safeUrl == null) {
onShowMessage(context.getString(R.string.digital_locked_other))
} else if (file.kind == "audio") selectedAudio = file.url
else if (file.isPdf) selectedPdf = file.url
else runCatching { context.startActivity(Intent(Intent.ACTION_VIEW, Uri.parse(file.url))) }
.onFailure { onShowMessage(context.getString(R.string.ebook_error)) }
}

Column(
Modifier
Expand Down Expand Up @@ -516,15 +536,34 @@ internal fun DetailContent(
DigitalFileCard(type = file.format.ifBlank { if (audio) "Audio" else "eBook" },
name = file.label.ifBlank { digitalFilename(file.url, book.title) },
kind = stringResource(if (audio) R.string.book_section_audiobook else if (file.kind == "supplement") R.string.book_section_related_documents else R.string.book_section_ebook)) {
if (audio && selectedAudio == file.url) {
if (file.locked) {
LockedDigitalFile(file.access)
} else if (audio && selectedAudio == file.url) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The player is matched to its card by the full signed URL. Every renewal replaces content with freshly signed URLs for all files. So if an audiobook is playing and the reader taps Read/Open on another file of the same record, the audio card's file.url changes and selectedAudio == file.url becomes false. The AudioPlayer then leaves composition and playback stops.

Technical details
# Renewing any file stops a playing audiobook

## Affected sites
- app/src/main/java/com/pinakes/app/ui/screens/detail/BookDetailScreen.kt:541-542 — `audio && selectedAudio == file.url` plus `key(file.url) { AudioPlayer(audioUrl = file.url) }`
- app/src/main/java/com/pinakes/app/ui/screens/detail/BookDetailViewModel.kt:116-119 — `refreshDigitalFile` swaps in the whole re-signed `BookDetail`

## Required outcome
- When another file is renewed, a player that is already running keeps playing the URL it was started with.

## Suggested approach (optional)
- Remember the selected audio as the `DigitalAttachment` (or its `resourceKey` plus URL), match cards with `resourceKey`, and pass the remembered URL to `AudioPlayer` and its `key`, not the card's current `file.url`.

androidx.compose.runtime.key(file.url) { AudioPlayer(audioUrl = file.url) }
} else PrimaryButton(label = stringResource(if (audio) R.string.collection_play else if (file.isPdf) R.string.book_read else R.string.ebook_open_external),
onClick = {
if (audio) selectedAudio = file.url
else if (file.isPdf) selectedPdf = file.url
else runCatching { context.startActivity(Intent(Intent.ACTION_VIEW, Uri.parse(file.url))) }
.onFailure { onShowMessage(context.getString(R.string.ebook_error)) }
}, modifier = Modifier.fillMaxWidth(), dark = true)
// The server's clock and access decision are authoritative. Always renew
// a signed URL on demand, then open it in this same tap (no clock-skew loop).
if (refreshingFile == null) {
if (file.expiresAt == null) openFile(file)
else {
refreshingFile = file.resourceKey
scope.launch {
try {
when (val result = onRefreshFile(file)) {
is ApiResult.Success -> openFile(result.data)
is ApiResult.Failure -> onShowMessage(context.getString(
if (result.code == ErrorCodes.FORBIDDEN) R.string.digital_locked_other
else R.string.digital_link_refresh_error))
}
} finally {
refreshingFile = null
}
}
}
}
}, enabled = refreshingFile == null, loading = refreshingFile == file.resourceKey,
modifier = Modifier.fillMaxWidth().testTag("digital-open"), dark = true)
}
Spacer(Modifier.height(16.dp))
}
Expand Down Expand Up @@ -602,6 +641,24 @@ internal fun DetailContent(
}
}

/**
* A file this reader may not open yet: no button, and the reason instead (issue #480).
* The server leaves staff-only files out of the list, so only the other reasons reach here.
*/
@Composable
private fun LockedDigitalFile(access: String?) {
val colors = LocalPinakesColors.current
Row(verticalAlignment = Alignment.CenterVertically, horizontalArrangement = Arrangement.spacedBy(8.dp),
modifier = Modifier.fillMaxWidth().testTag("digital-locked")) {
Icon(Icons.Outlined.Lock, contentDescription = null, tint = colors.muted, modifier = Modifier.size(18.dp))
Text(stringResource(when (access) {
DigitalAttachment.ACCESS_LOGIN_REQUIRED -> R.string.digital_locked_login
DigitalAttachment.ACCESS_LOAN_REQUIRED -> R.string.digital_locked_loan
else -> R.string.digital_locked_other
}), style = MaterialTheme.typography.bodyMedium, color = colors.muted)
}
}

/**
* Personal-status banner driven by [personalHistory]. Priority: active loan (primary-tinted)
* → pending request (neutral secondary-tinted) → reservation (tertiary-tinted) → wishlist
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.pinakes.app.data.model.AvailabilityCalendar
import com.pinakes.app.data.model.BookDetail
import com.pinakes.app.data.model.DigitalAttachment
import com.pinakes.app.data.model.CirculationRequestKind
import com.pinakes.app.data.network.ApiResult
import com.pinakes.app.data.connectivity.ConnectivityObserver
Expand Down Expand Up @@ -107,6 +108,24 @@ class BookDetailViewModel @Inject constructor(
}
}

/** Refresh in place: a failed renewal must not replace the page or reuse a cached URL. */
suspend fun refreshDigitalFile(file: DigitalAttachment): ApiResult<DigitalAttachment> {
return when (val result = catalog.book(bookId, forceRefresh = true)) {
is ApiResult.Failure -> result
is ApiResult.Success -> {
_state.update {
it.copy(content = UiState.Success(result.data), offline = false,
wishlisted = result.data.personalHistory?.hasWishlisted ?: it.wishlisted)
}
val renewed = result.data.digitalFiles.firstOrNull {
it.resourceKey == file.resourceKey && it.kind == file.kind && !it.locked && it.safeUrl != null
}
if (renewed != null) ApiResult.Success(renewed)
else ApiResult.Failure(ErrorCodes.FORBIDDEN, "", 403)
}
}
}

fun toggleWishlist() {
if (_state.value.wishlistBusy || _state.value.offlineNow) return
val nowWishlisted = !_state.value.wishlisted
Expand Down
Loading
Loading