From e897129eae4462a7f8a4972ffa0ba39d30c24322 Mon Sep 17 00:00:00 2001 From: StefRe Date: Sun, 28 Feb 2021 22:56:47 +0100 Subject: [PATCH] SLUB: Add references to other SLUB catalog items as volumes This is a workaround as the only possibility to add links to other detailed items from a detailed item. It's possible as we can display volumes and copies at the same time. References are some kind of details which is why the display order of copies and volumes was swapped: first show volumes (which may be references) right after the details and only then copies. Increase volumes text view to maximum two lines high to be able to put the title of the reference and the reference itself on different lines for better readability. Change resolution of bc or rsn and the new libero identifiers to following head redirects and retrieving the final url to get the id identifier. This requires one request more than the old method (get location parameter from non-redirecting head request) but is much shorter to implement. IDs of references have a single query component (e.g. ?libero_mab21364775) and no key=value pairs. The URLEncodedUtils.format implementation used by Android has a bug and adds a trailing equal sign (=) to the query component which leads to infinite redirects, see #612 for details. The prevent this we use cleanUrl only if there are any = signs in the url, otherwise we use the raw url in head requests. --- .../opacclient/apis/OkHttpBaseApi.java | 19 ++---- .../de/geeksfactory/opacclient/apis/SLUB.kt | 36 +++++------ .../geeksfactory/opacclient/apis/SLUBTest.kt | 64 +++++++++---------- .../frontend/SearchResultDetailFragment.java | 11 ++-- .../src/main/res/layout/listitem_volume.xml | 3 +- 5 files changed, 62 insertions(+), 71 deletions(-) diff --git a/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/OkHttpBaseApi.java b/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/OkHttpBaseApi.java index 88ff02d0f..a4dee7ad3 100644 --- a/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/OkHttpBaseApi.java +++ b/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/OkHttpBaseApi.java @@ -226,31 +226,27 @@ public String httpPost(String url, RequestBody data, String encoding, boolean ig * Perform a HTTP HEAD request to a given URL * * @param url URL to fetch - * @param name Name of the header field - * @param defaultValue Default value of this header field * @param ignore_errors If true, status codes above 400 do not raise an exception - * @param client Http client to use - * @return Answer content + * @return Response * @throws NotReachableException Thrown when server returns a HTTP status code greater or equal * than 400. */ - public String httpHead(String url, String name, String defaultValue, boolean ignore_errors, - OkHttpClient client) throws IOException { + public Response httpHead(String url, boolean ignore_errors) throws IOException { Request request = new Request.Builder() - .url(cleanUrl(url)) + .url(url.contains("=") ? cleanUrl(url) : url) .header("Accept", "*/*") .header("User-Agent", getUserAgent()) .head() .build(); try { - Response response = client.newCall(request).execute(); + Response response = http_client.newCall(request).execute(); if (!ignore_errors && response.code() >= 400) { throw new NotReachableException(response.message()); } - return response.header(name, defaultValue); + return response; } catch (javax.net.ssl.SSLPeerUnverifiedException e) { logHttpError(e); throw new SSLSecurityException(e.getMessage()); @@ -281,11 +277,6 @@ public String httpHead(String url, String name, String defaultValue, boolean ign } } - public String httpHead(String url, String name, String defaultValue, OkHttpClient client) - throws IOException { - return httpHead(url, name, defaultValue, false, client); - } - public CompletableFuture asyncPost(String url, RequestBody data, final boolean ignore_errors) { Request request = new Request.Builder() diff --git a/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/SLUB.kt b/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/SLUB.kt index 228d27fb8..59e1ed996 100644 --- a/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/SLUB.kt +++ b/opacclient/libopac/src/main/java/de/geeksfactory/opacclient/apis/SLUB.kt @@ -175,15 +175,9 @@ open class SLUB : OkHttpBaseApi() { id.startsWith("id/") -> "$baseurl/$id/" id.startsWith("bc/") || id.startsWith("rsn/") -> - http_client.newBuilder() - .followRedirects(false) - .build() - .run { - httpHead("$baseurl/$id/", - "Location", - "", - this) - } + httpHead("$baseurl/$id/", false).request().url().toString() + id.startsWith("http://slubdd.de/katalog?libero_mab") -> + httpHead(id, false).request().url().toString() else -> // legacy case: id identifier without prefix "$baseurl/id/$id/" } + "?type=1369315142&tx_find_find[format]=data&tx_find_find[data-format]=app" @@ -260,7 +254,7 @@ open class SLUB : OkHttpBaseApi() { cover = this } } - // links and references + // links for (link in listOf("linksRelated", "linksAccess", "linksGeneral")) { json.getJSONArray(link).forEach { // assuming that only on of material, note or hostlabel is set @@ -270,10 +264,6 @@ open class SLUB : OkHttpBaseApi() { addDetail(Detail(key, it.optString("uri"))) } } - json.getJSONArray("references").forEach { - // TODO: usually links to old SLUB catalogue, does it make sense to add the link? - addDetail(Detail(it.optString("text"), "${it.optString("name")} (${it.optString("target")})")) - } // copies val cps = json.get("copies") if (cps is JSONArray) { @@ -290,11 +280,19 @@ open class SLUB : OkHttpBaseApi() { } isReservable = hasReservableCopies // volumes - volumes = json.optJSONObject("parts")?.optJSONArray("records")?.map { - Volume(it.optString("id"), - "${it.optString("part")} ${Parser.unescapeEntities(it.optString("name"), false)}") - - } ?: emptyList() + json.getJSONObject("parts").optJSONArray("records")?.forEach { + volumes.add(Volume(it.optString("id"), + "${it.optString("part")} ${Parser.unescapeEntities(it.optString("name"), false)}")) + } + // references + json.getJSONArray("references").forEach { + val link = it.optString("link") + if (link.startsWith("http://slubdd.de/katalog?libero_mab")){ + addVolume(Volume(link, "${it.optString("text")}:\n${it.optString("name")}")) + } else { + addDetail(Detail(it.optString("text"), "${it.optString("name")} (${it.optString("target")})")) + } + } } } diff --git a/opacclient/libopac/src/test/java/de/geeksfactory/opacclient/apis/SLUBTest.kt b/opacclient/libopac/src/test/java/de/geeksfactory/opacclient/apis/SLUBTest.kt index 431de64a4..00b56e36b 100644 --- a/opacclient/libopac/src/test/java/de/geeksfactory/opacclient/apis/SLUBTest.kt +++ b/opacclient/libopac/src/test/java/de/geeksfactory/opacclient/apis/SLUBTest.kt @@ -28,9 +28,7 @@ import de.geeksfactory.opacclient.objects.* import de.geeksfactory.opacclient.searchfields.DropdownSearchField import de.geeksfactory.opacclient.searchfields.SearchQuery import de.geeksfactory.opacclient.searchfields.TextSearchField -import okhttp3.FormBody -import okhttp3.OkHttpClient -import okhttp3.RequestBody +import okhttp3.* import org.hamcrest.MatcherAssert.assertThat import org.hamcrest.Matchers.* import org.joda.time.LocalDate @@ -376,7 +374,7 @@ class SLUBSearchTest : BaseHtmlTest() { addDetail(Detail("Inhaltsverzeichnis", "http://www.gbv.de/dms/tib-ub-hannover/727434322.pdf")) addDetail(Detail("Inhaltstext", "http://deposit.d-nb.de/cgi-bin/dokserv?id=4155321&prov=M&dok_var=1&dok_ext=htm")) addDetail(Detail("Zugang zur Ressource (via ProQuest Ebook Central)", "http://wwwdb.dbod.de/login?url=http://slub.eblib.com/patron/FullRecord.aspx?p=1575685")) - addDetail(Detail("Online-Ausgabe", "Tamm, Michael: JUnit-Profiwissen (SLUB)")) + addVolume(Volume("http://slubdd.de/katalog?libero_mab216187885", "Online-Ausgabe:\nTamm, Michael: JUnit-Profiwissen")) } val item = slub.parseResultById(json) @@ -822,12 +820,16 @@ class SLUBSearchMockTest(@Suppress("unused") private val name: String, class SLUBGetResultByIdMockTest : BaseHtmlTest() { private val slub = spy(SLUB::class.java) - - private class ClientDoesntFollowRedirects : ArgumentMatcher() { - override fun matches(argument: Any): Boolean { - return !(argument as OkHttpClient).followRedirects() - } - } + private val mockHeadResponse = Response.Builder() + .request(Request.Builder() + .url("https://test.de/id/123/") + .build()) + .protocol(Protocol.HTTP_2) + .code(200) + .message("") + .build() + val mockGetResponse = """{"record":{"title":"The title"},"id":"123","thumbnail":"","links":[],"linksRelated":[], + |"linksAccess":[],"linksGeneral":[],"references":[],"copies":[],"parts":{}}""".trimMargin() init { slub.init(Library().apply { @@ -848,39 +850,39 @@ class SLUBGetResultByIdMockTest : BaseHtmlTest() { @Test fun testIdIdentifier() { - val response = """{"record":{"title":"The title"},"id":"123","thumbnail":"","links":[],"linksRelated":[], - |"linksAccess":[],"linksGeneral":[],"references":[],"copies":[],"parts":{}}""".trimMargin() - doReturn(response).`when`(slub).httpGet(Matchers.any(), Matchers.any()) + doReturn(mockGetResponse).`when`(slub).httpGet(Matchers.any(), Matchers.any()) val actual = slub.getResultById("id/123", null) verify(slub).httpGet("https://test.de/id/123/?type=1369315142&tx_find_find[format]=data&tx_find_find[data-format]=app", "UTF-8") - verify(slub, never()).httpHead(any(), any(), any(), any()) + verify(slub, never()).httpHead(any(), anyBoolean()) assertEquals("id/123", actual.id) } @Test fun testBcIdentifier() { - doReturn("https://test.de/id/123/").`when`(slub).httpHead(Matchers.any(), - Matchers.any(), Matchers.any(), Matchers.any()) - val response = """{"record":{"title":"The title"},"id":"123","thumbnail":"","links":[],"linksRelated":[], - |"linksAccess":[],"linksGeneral":[],"references":[],"copies":[],"parts":{}}""".trimMargin() - doReturn(response).`when`(slub).httpGet(Matchers.any(), Matchers.any()) + doReturn(mockHeadResponse).`when`(slub).httpHead(Matchers.any(), Matchers.anyBoolean()) + doReturn(mockGetResponse).`when`(slub).httpGet(Matchers.any(), Matchers.any()) val actual = slub.getResultById("bc/456", null) - verify(slub).httpHead(eq("https://test.de/bc/456/"), eq("Location"), eq(""), - argThat(ClientDoesntFollowRedirects())) + verify(slub).httpHead(eq("https://test.de/bc/456/"), eq(false)) verify(slub).httpGet("https://test.de/id/123/?type=1369315142&tx_find_find[format]=data&tx_find_find[data-format]=app", "UTF-8") assertEquals("id/123", actual.id) } @Test fun testRsnIdentifier() { - doReturn("https://test.de/id/123/").`when`(slub).httpHead(Matchers.any(), - Matchers.any(), Matchers.any(), Matchers.any()) - val response = """{"record":{"title":"The title"},"id":"123","thumbnail":"","links":[],"linksRelated":[], - |"linksAccess":[],"linksGeneral":[],"references":[],"copies":[],"parts":{}}""".trimMargin() - doReturn(response).`when`(slub).httpGet(Matchers.any(), Matchers.any()) + doReturn(mockHeadResponse).`when`(slub).httpHead(Matchers.any(), Matchers.anyBoolean()) + doReturn(mockGetResponse).`when`(slub).httpGet(Matchers.any(), Matchers.any()) val actual = slub.getResultById("rsn/456", null) - verify(slub).httpHead(eq("https://test.de/rsn/456/"), eq("Location"), eq(""), - argThat(ClientDoesntFollowRedirects())) + verify(slub).httpHead(eq("https://test.de/rsn/456/"), eq(false)) + verify(slub).httpGet("https://test.de/id/123/?type=1369315142&tx_find_find[format]=data&tx_find_find[data-format]=app", "UTF-8") + assertEquals("id/123", actual.id) + } + + @Test + fun testLibeorIdentifier() { + doReturn(mockHeadResponse).`when`(slub).httpHead(Matchers.any(), Matchers.anyBoolean()) + doReturn(mockGetResponse).`when`(slub).httpGet(Matchers.any(), Matchers.any()) + val actual = slub.getResultById("http://slubdd.de/katalog?libero_mab456", null) + verify(slub).httpHead(eq("http://slubdd.de/katalog?libero_mab456"), eq(false)) verify(slub).httpGet("https://test.de/id/123/?type=1369315142&tx_find_find[format]=data&tx_find_find[data-format]=app", "UTF-8") assertEquals("id/123", actual.id) } @@ -888,12 +890,10 @@ class SLUBGetResultByIdMockTest : BaseHtmlTest() { @Test fun testLegacyIdentifier() { // id without prefix, e.g. from old favorites list - val response = """{"record":{"title":"The title"},"id":"123","thumbnail":"","links":[],"linksRelated":[], - |"linksAccess":[],"linksGeneral":[],"references":[],"copies":[],"parts":{}}""".trimMargin() - doReturn(response).`when`(slub).httpGet(Matchers.any(), Matchers.any()) + doReturn(mockGetResponse).`when`(slub).httpGet(Matchers.any(), Matchers.any()) val actual = slub.getResultById("123", null) verify(slub).httpGet("https://test.de/id/123/?type=1369315142&tx_find_find[format]=data&tx_find_find[data-format]=app", "UTF-8") - verify(slub, never()).httpHead(any(), any(), any(), any()) + verify(slub, never()).httpHead(any(), anyBoolean()) assertEquals("id/123", actual.id) } } diff --git a/opacclient/opacapp/src/main/java/de/geeksfactory/opacclient/frontend/SearchResultDetailFragment.java b/opacclient/opacapp/src/main/java/de/geeksfactory/opacclient/frontend/SearchResultDetailFragment.java index 9f074f1e6..1b0d35a35 100644 --- a/opacclient/opacapp/src/main/java/de/geeksfactory/opacclient/frontend/SearchResultDetailFragment.java +++ b/opacclient/opacapp/src/main/java/de/geeksfactory/opacclient/frontend/SearchResultDetailFragment.java @@ -382,11 +382,6 @@ protected void display() { addSubhead(joiner, R.string.details_head); joiner.add(new JoinableAdapter(new DetailsAdapter(item.getDetails(), getActivity()))); - if (item.getCopies().size() != 0) { - addSubhead(joiner, R.string.copies_head); - joiner.add(new JoinableAdapter(new CopiesAdapter(item.getCopies(), getActivity()))); - } - if (item.getVolumesearch() != null) { addSubhead(joiner, R.string.volumes); joiner.add(new JoinableLayout( @@ -405,6 +400,12 @@ public void onClick(View v) { addSubhead(joiner, R.string.volumes); joiner.add(new JoinableAdapter(new VolumesAdapter(item.getVolumes(), getActivity()))); } + + if (item.getCopies().size() != 0) { + addSubhead(joiner, R.string.copies_head); + joiner.add(new JoinableAdapter(new CopiesAdapter(item.getCopies(), getActivity()))); + } + rvDetails.setAdapter(joiner.getAdapter()); if (id == null || id.equals("")) { diff --git a/opacclient/opacapp/src/main/res/layout/listitem_volume.xml b/opacclient/opacapp/src/main/res/layout/listitem_volume.xml index a16f57ec3..5779ac2af 100644 --- a/opacclient/opacapp/src/main/res/layout/listitem_volume.xml +++ b/opacclient/opacapp/src/main/res/layout/listitem_volume.xml @@ -14,6 +14,7 @@ android:layout_width="wrap_content" android:layout_height="wrap_content" android:ellipsize="end" - android:singleLine="true"/> + android:maxLines="2" + android:singleLine="false"/> \ No newline at end of file