Implement support for SLUB Dresden

[StefRe]

This API is specific for the new OPAC of Sächsische Landes- und Universitätsbibliothek in Dresden: www.slub-dresden.de.
See also #546.

[StefRe]

Oops, I guess I messed up my git log. I rebased my feat/SLUB-API branch onto the current master (to get the 'upgrade toolchain' commit because I locally already upgraded Kotlin too) and now all the commits that occurred meanwhile are included in this PR. How should I proceed from here?

[raphaelm]

I'm not sure what you did, but git can be nasty at times ;) I pulled your branch, re-based it again properly (git rebase -i master, then resolved two pseudeo-conflicts) and force-pushed it back again. You can force-pull it, if you want (but if it's done, you don't need to), but before you do anything to your local history, please double-check I included all your changes ;)

[StefRe]

After a long pause I finally sorted it out to correctly rebase and update the PR without having all old history appended here (thanks to @raphaelm and this SO answer).

Now I see the all new additions fail at CI with a long list of lint issues at Google Play Services Debug. Is this something I should worry about? As far as I can see none of them pertain to libopac code but it's a bit difficult to browse the 6000+ lines of lint output.

[johan12345]

Great, thank you!

The error in the Travis CI log seems to be:

de.geeksfactory.opacclient.apis.SLUBAccountTest > testParseEmptyAccountData FAILED
    org.junit.ComparisonFailure at SLUBTest.kt:23

so there is a failure in the SLUBTest. Maybe the parsing of validUntil is not working correctly?

[StefRe]

Oops, I completely overlooked this one in the sheer stream of lint warnings. The reason seems to be LocalDateTime(it) where it is a string "2020-03-31". Maybe it's a localization issue as the tests passes just fine on my German locale. I'll have to look into it.
CI says in line 1661 There were failing tests. See the report at: file:///home/travis/build/opacapp/opacclient/opacclient/libopac/build/reports/tests/test/index.html. Is there any way to retrieve this file to see what was the actual value or the detailed error?

[johan12345]

The reason seems to be LocalDateTime(it) where it is a string "2020-03-31"

Hmm, maybe LocalDate(it) would be sufficient if the time is not included anyway?

Is there any way to retrieve this file to see what was the actual value or the detailed error?

Well, you can let Travis CI output that file by adjusting the cat command in travis.yml.

[StefRe]

Thank you @johan12345, I solved this silly error of mine: I hardcoded the expected formatted date value in the test according to German style but it should have been formatted according to the current locale. So the code was OK, just the test was faulty.

[johan12345]

I quickly tested your API today and it already looks really good!

A few tips regarding the remaining TODO points:

• //TODO: get status (one request per item!)

  Probably the best way to do this without affecting the performance too much is to use the asyncGet function implemented in the OkHttpBaseApi. Then, you can put all the CompletableFutures you constructed into a list before calling get() on them, so that the requests are executed in parallel.

• getSupportFlags: Probably, you can at least support OpacApi.SUPPORT_FLAG_ENDLESS_SCROLLING. This just means that getResultById is not affected by which page of the result list was last loaded - so if you don't return it, the app will first go back to the corresponding page before loading the details.

• filterResults: This is not needed in the Open Source version of the app, you can just return null

• reservation, cancel: If these are supported by the OPAC, you can implement them, but we could also first merge without support for these, if you like.

• getSupportedLanguages, setLanguage: It seems that the SLUB OPAC supports German and English, so support for setting the language correctly would be nice (but not absolutely required).

[StefRe]

Thanks for taking the time and testing the API.

• get status: thanks for the suggestion, I'll certainly have a look a it and get back to you if I'm stuck here, but right now I guess this is of lower priority. This is due to the limited usefulness of the status as in many many cases it just says "click for more information about availability" (in the online web catalogue), so you'll have to open the details page anyway.

• getSupportFlags: thanks for the explanation, i didn't understand the implications of this flag. Will add OpacApi.SUPPORT_FLAG_ENDLESS_SCROLLING

• filterResults: setFilters seems easy to implement here but my understanding is that even if I implement filterResults it won't work in the FOSS version, right? If it will work I'll definetly try to implement it as due to the usually huge amount of results the search is not very useful without the possibility to further narrow it down to at least physical vs electronic copies.

• cancel: is implemented but not yet pushed: when I cancel an item it first does nothing (no dialog appears) and updates the data (which remain unchanged). Only when I cancel the item the second time the dialog "do you really want to cancel yes/no" appears and now it works correctly. Not sure if it's an issue of my cancel implementation or the app, the problem is I don't want to play around with this function too much on my account for obvious reasons :).

• reservation is not yet implemented. With SLUB reservation is per copy, not per item and it's a two step process: you'll have to select the pick-up location from a list of offered locations. I haven't yet looked into it if this is supported by the app.

• getSupportedLanguages, setLanguage: it's true that the web catalogue offers English and German but the English version doesn't switch completely to English: all information concerning inter-library loans, most other account information and most of the captions/titles for links remain in German. So it mostly only translates the search and result field names. That's why this is of low priority for me but I'm going to add it later.

Although I used my debug version for over a year now I still find occasional errors and I have a couple of commits still sitting on my hard disk and also some uncommitted work and tests mainly regarding item details.
For all kinds of reservations I'll have to study how the json information changes in the different stages of the process as it not always coincides with what is displayed in the account view of the online catalogue. I finished this for simple reservations of lent out items, for stack requests (Magazinbestellungen) and for request for usage on premises (Benutzung nur im Haus). Still working on the inter library loan process.

One question about cover images: the catalogue doesn't supply thumbnails but in some cases there are links to cover images, mainly at http://swbplus.bsz-bw.de but also at the editors' pages. The size of these images typically ranges from about 40 kB to 1MB. Do you think I should include these URLs given that the images could be rather big in some cases? (for items from Deutsche Fotothek the API supplies thumbnails and I use them.)

I think I can finish the open issues by beginning of February so you can merge it and then I can complete any open items later.

Lastly, from time to time I get leakage warnings (Dumping memory, app will freeze. Brrrr.) when I run the debug version on my phone. This is mainly after a new debug install, didn't get it later when running the app for a longer time. The strange thing is that I can't see further information in the Leaks app/icon on my phone anymore - it just says "Leaks in de.geeksfactory.opacclient.debug". This also happens if I don't query my account or search anything, so I guess this is more related to the app than to libopac. What couId I do here to narrow down the problem?

[johan12345]

filterResults: setFilters seems easy to implement here but my understanding is that even if I implement filterResults it won't work in the FOSS version, right? If it will work I'll definetly try to implement it as due to the usually huge amount of results the search is not very useful without the possibility to further narrow it down to at least physical vs electronic copies.

Yeah, filterResults won't work in the FOSS app, the UI for that is only implemented in the Plus Edition. Most of the time, that is not such a big problem as fields such as media type are also available in the search form itself - but it seems that this is not the case in the SLUB OPAC. Maybe it would still be possible to add some of the filtering fields to the search form?

One question about cover images: the catalogue doesn't supply thumbnails but in some cases there are links to cover images, mainly at http://swbplus.bsz-bw.de but also at the editors' pages. The size of these images typically ranges from about 40 kB to 1MB. Do you think I should include these URLs given that the images could be rather big in some cases? (for items from Deutsche Fotothek the API supplies thumbnails and I use them.)

Hm, do you have some links with examples for this? Sometimes there are even URL parameters to specify the size of the cover (like with the Amazon cover server, which some OPACs use), but this doesn't sound like it is the case here.
But I would say a 1MB cover is not a huge problem in the detail view (getResultById), I just wouldn't use it in the search result list.

reservation is not yet implemented. With SLUB reservation is per copy, not per item and it's a two step process: you'll have to select the pick-up location from a list of offered locations. I haven't yet looked into it if this is supported by the app.

Yes, that's possible: the ReservationResult has the possibility to return some selections or confirmations that the user needs to do before the reservation is performed - you can see examples for this in most of the other APIs.

ReservationResult res = new ReservationResult(MultiStepResult.Status.SELECTION_NEEDED);
res.setSelection(options);
res.setActionIdentifier(ReservationResult.ACTION_BRANCH);
return res

cancel: is implemented but not yet pushed: when I cancel an item it first does nothing (no dialog appears) and updates the data (which remain unchanged). Only when I cancel the item the second time the dialog "do you really want to cancel yes/no" appears and now it works correctly. Not sure if it's an issue of my cancel implementation or the app, the problem is I don't want to play around with this function too much on my account for obvious reasons :).

Hmm, interesting... If you push this change somewhere I can take a look, it doesn't sound like this is a general bug in the app.

Lastly, from time to time I get leakage warnings (Dumping memory, app will freeze. Brrrr.) when I run the debug version on my phone. This is mainly after a new debug install, didn't get it later when running the app for a longer time. The strange thing is that I can't see further information in the Leaks app/icon on my phone anymore - it just says "Leaks in de.geeksfactory.opacclient.debug". This also happens if I don't query my account or search anything, so I guess this is more related to the app than to libopac. What couId I do here to narrow down the problem?

That's probably not due to your changes - we added LeakCanary to the debug version of the app a long time ago but never had much time to further investigate the leaks that it is sometimes showing (and it wasn't a high priority as we never heard of any real issues arising from these). Sorry!

[StefRe]

One question about strings in libopac: most of the strings from interface StringProvider are implemented in strings_api_errors.xml, including many strings that aren't errors but just information. Only three strings (status, free_search and no_results) come from strings.xml. Where should I put my implementation of general information strings (e.g. "ready for pick-up since ...", i.e. mostly status related information) - in strings.xml (where I thought it should be) or in strings_api_errors.xml (where de facto almost all strings are placed, be it error related or not)?

[johan12345]

strings_api_errors.xml is (despite the name) made for everything that comes from StringProvider - where originally most of the strings were error messages. The reason why we are separating these is that Android Lint would mark these as unused as we are not referencing their IDs in the AndroidStringProvider class.

The three strings that are in strings.xml despite this are probably there because they were already used by something else in the app.

[StefRe]

Hm, do you have some links with examples for this?

example of cover image: https://katalog.slub-dresden.de/id/0-81817546X/ with a 780 kB image

Hmm, interesting... If you push this change somewhere I can take a look,

I pushed it to this PR as dde1bba.

[StefRe]

I tested my cancel implementation dde1bba in a command line application like this:

    OpacApi.CancelResult cr = api.cancel(id, account, 0, null);

It worked correctly and returned the following cancel result:

{
  "status" : "OK",
  "selection" : null,
  "details" : null,
  "message" : null,
  "actionIdentifier" : 0
}

For the app I'd prefer however to be asked if I really want to cancel the reservation. Do I have to process the useraction and selection parameters in some way for this? I looked in some of the other API implementations but couldn't find anything there.

[StefRe]

@johan12345 - regarding this last commit b5ea66f:
The copy string was introduced in 213fff6. Now I get a warning Overriding '@string/copy' which is marked as private in androidx.preference:preference.. As everything worked fine so far I followed Android studios's recommendation If deliberate, use tools:override="true", otherwise pick a different name. without fully understanding what I'm doing here. Hope this is OK.

[johan12345]

Hm, that sounds like the string copy (probably in the sense of the verb, i.e. copy = kopieren) is already defined in the preference support library. I'm not sure where that string is actually used by that library and whether that appears in our app, but just to be safe I would suggest to pick a different name instead of overriding it. Thanks!

[StefRe]

@johan12345 thanks for the reply, I renamed it, see 3b7ca5e.

[StefRe]

I've been using with my debug version for over a year now without any problems, so I think you can review this PR for merging. All methods except for setLanguage and filterResults are implemented and working. Test coverage is 86 % of LOC; if need be I can write more tests to increase coverage.

There are still some TODOs, but I don't feel like being able to close them in the next couple of weeks. I may get back to them later.

• line # 155: get status: low priority due to the limited usefulness of the status as in many many cases it just says "click for more information about availability" (in the online web catalogue), so you'll have to open the details page anyway.
  Implementation idea: use asyncGet and then put all the CompletableFutures constructed into a list before calling get() on them (thanks to @johan12345 here and here)

• line # 160: filterResults: not to implement in FOSS version

• line # 251: references: libero links must be resolved to rsn entries in current SLUB catalog and they then in turn to id entries, similar to next TODO line # 431

• line # 431/487: id of reserved/lent items: bc entries to be resolved to id entries by intercepting redirection

• line # 433: actual ready date of stack request: works but could be refined, need more examples to examine

• line # 479: permanent loans: need example, can't work on this as permantent loans are not granted to external users

• line # 489: inter-library loans renewability: need example (X_is_flrenewable field exists but json but not sure if it's actually used/properly set)

• line # 545/568: getSupportedLanguages/setLanguage: low priority as all information concerning inter-library loans, most other account information and most of the captions/titles for links remain in German even in the English version of the online catalog

[johan12345]

Hi @StefRe! Thank you for your hard work on the SLUB API and sorry for taking so long for the review. Your implementation looks very good to me and it is also great that you added good tests - which is especially helpful as we don't have an account in the library to be able to test account features.

I have added a few minor comments below, most of which can probably be addressed very easily. I haven't compiled and tested it on a device yet, but will of course do so before merging.

I agree that the remaining TODOs are all not essential - for some of them I added comments below with suggestions on how they could be implemented (but that doesn't mean that it needs to be done before we merge this).

Could you also please rebase your branch against the current master? GitHub says there are some conflicts (shouldn't be anything too complicated, probably just a few additions to the strings XML files that we have done in the meantime).

[StefRe]

@johan12345 Thanks for the detailed comments in the review, I resolved all but one where I need some more time. Regarding rebasing: I rebased it already on Sep 26 and it's no problem the rebase it again onto the current master but I think I read somewhere that you should never rebase a branch under review as it could mess up the review history/comments (not sure if this is still correct for current versions of git), so I'd like to wait with rebasing until everything is resolved and you're prepared to merge.

[StefRe]

@johan12345 I resolved all review comments above and will wait for your go-ahead before rebasing onto the current master, see previous comment.
Further I prepared a PR for the SLUB config file, see opacapp/opacapp-config-files#13.

[johan12345]

Thanks! I think after rebasing and implementing the extension functions (see comment at #553 (comment)) this should be good to go :)

[StefRe]

@johan12345 Your commit e573c2c breaks VuFindSearchTest:

opacclient/opacclient/libopac/src/test/java/de/geeksfactory/opacclient/apis/VuFindSearchTest.java

Lines 51 to 52 in e9e14f2

	DetailedItem result = VuFind.parseDetail("0", Jsoup.parse(html), getData(file),
	htmlDesc != null ? Jsoup.parse(htmlDesc) : null);

(parseDetail now takes 5 instead of 4 arguments)

How did you manage to compile the project?

[johan12345]

oops... yes, indeed, I forgot to adjust the tests after that commit. Building the app itself worked though. Will fix that tonight...

[johan12345]

Tests should be fixed now with 85a6790.

[StefRe]

Could you also please rebase your branch against the current master?

I'm having difficulties in rebasing. The last rebasing on Sep 26 went smoothly without any problems (I just had to resolve some conflicts in build.gradle and in the string xml files). If I try to rebase now, I not only need to resolve the same conflicts again (which is very strange) but also need to resolve conflicts for each of my 55 commits of SLUB.kt and SLUBTest.kt relative to the preceding versions of these files. This in principle is no problem but at some point (at commit 22 of 55) git starts to complain that it can't execute the todo command (next pick). I tried some recommendations from stackoverflow but not fully understanding what I'm doing I didn't succeed.

GitHub says there are some conflicts

This is strange as github says "This branch has no conflicts with the base branch". I also forked the current master this afternoon and merged my feature branch in it without any problems (https://github.com/StefRe/opacclient/tree/merge_test, this also includes the use of the new extension functions).

So my question is whether you could merge this PR as is. I could then make a new PR for commit c79243b (use utility functions for JSONArrays) against the new master.

[johan12345]

Ah, interesting. I had selected "Rebase and merge", and there GitHub says there are conflicts:

But if I say "Squash and merge" or "create a merge commit", it works:

So I'll try.

[johan12345]

That worked, thanks again! 🎉 I also cherry-picked your commit to add the JSONArray extension functions (2739579).