Add WISE as a new library type #643
Conversation
Fix some translation issues
|
Wow, thanks! I'm traveling the next days and a little overcommitted on things generally right now, but I'll try to take a closer look at this asap :) |
|
Hi Raphael,
Your welcome. Take your time (but not ages :-))
Henk
Op di 31 aug. 2021 om 23:02 schreef Raphael Michel ***@***.***
…:
Wow, thanks! I'm traveling the next days and a little overcommitted on
things generally right now, but I'll try to take a closer look at this asap
:)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#643 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AH6OB2GQZG7FHEQTNG7JRHDT7U7MVANCNFSM5DDKGNYQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
There was a problem hiding this comment.
Thank you again!
I've had a first read of the code. I did not test it and I did not yet read all of it, but I have some comments already that I wanted to share with you. Nothing dramatic, but it's lacking a consist code style and a few details will likely cause problems in the future. Let me know if anything is not clear from the review.
| <string name="no_criteria_input">You haven\'t input any search criteria.</string> | ||
| <string name="combination_not_supported">You used a combination of search criteria that is not supported by this library. Please try another search query.</string> | ||
| <string name="unknown_error_account">Unknown Error. Please check if your login data is correct.</string> | ||
| <string name="unknown_error_account_with_description">Unknown Error: \"%s\" Please check if your login data is correct.</string> | ||
| <string name="unknown_error">Unknown error.</string> | ||
| <string name="unknown_error_with_description">Unknown error: \"%s\"</string> | ||
| <string name="internal_error">Internal Error.</string> | ||
| <string name="internal_error_with_description">Internal Error: \"%s\"</string> | ||
| <string name="login_failed">Login failed.</string> | ||
| <string name="could_not_load_account">Account could not be loaded.</string> | ||
| <string name="api_connection_error">Connection error.</string> | ||
| <string name="lent_until">lent until</string> | ||
| <string name="subtitle">Subtitle</string> | ||
| <string name="pica_which_copy">Which copy do you want to reserve/order? (already available copies will be ordered)</string> | ||
| <string name="no_copy_reservable">There is no reservable copy.</string> | ||
| <string name="could_not_load_detail">Details could not be loaded. Please try again.</string> | ||
| <string name="error">Error</string> | ||
| <string name="download">Download</string> | ||
| <string name="reminders">warnings</string> | ||
| <string name="prolonged_abbr">renew.</string> | ||
| <string name="prolonging_expired">renewal no longer possible</string> | ||
| <string name="prolonging_impossible">renewal not possible</string> | ||
| <string name="prolonging_waiting">not yet renewable</string> | ||
| <string name="wrong_account_data">The user data you entered is wrong.</string> | ||
| <string name="order">Sort order</string> | ||
| <string name="order_default">Default order</string> | ||
| <string name="order_year_asc">By year (descending)</string> | ||
| <string name="order_category_asc">By category (ascending)</string> | ||
| <string name="order_year_desc">By year (descending)</string> | ||
| <string name="order_category_desc">By category (descending)</string> | ||
| <string name="interlib_branch">Interlibrary: %s</string> | ||
| <string name="stacks_branch">Stacks: %s</string> | ||
| <string name="provision_branch">Provision: %s</string> | ||
| <string name="unsupported_in_library">This feature is not supported in this library.</string> | ||
| <string name="reservation_warning">This reservation might cost you a fee.</string> | ||
| <plurals name="reservations_number"> | ||
| <item quantity="one">%d reservation</item> | ||
| <item quantity="other">%d reservations</item> | ||
| </plurals> |
There was a problem hiding this comment.
I don't think we should add English strings to the Dutch language file :)
| <string name="reader_needed">To read the e-book, you need a separate app, for example \'Aldiko\'. Click \'Ignore\' if you already have such an app, this message won\'t show up again in this case. Click \'Download\' to open up the Play Store to download Aldiko.</string> | ||
| <string name="reader_needed_overdrive">To open this medium, you need a separate app, for example OverDrive\'s own app. Click \'Ignore\' if you already have such an app, this message won\'t show up again in this case. Click \'Download\' to open up the Play Store to download the OverDrive app.</string> |
There was a problem hiding this comment.
If you do not want to translate something, just omit it from the file. Including English text in Dutch files makes it very hard to find what translations are missing.
| put("baseurl", "https://catalogus.biblionetdrenthe.nl") | ||
| put("imgurl", "/cgi-bin/momredir.pl?") | ||
| put("api-key-id", "4251d316-b964-44cc-b5fc-2f888d3716a8") | ||
| put("api-key", "218f81af-dcab-4cca-97f3-a4cffe19a0c9") |
There was a problem hiding this comment.
Our general tests should not do real network calls to avoid breaking tests when library servers are down. I see that you're mocking things, which is good, but in that case we can probably also skip hardcoding API keys?
| */ | ||
|
|
||
|
|
||
| class WiseKeyInterceptor : Interceptor { |
There was a problem hiding this comment.
Hmm, I'm not completely sure if an interceptor is the best design to add an Authorization header. Is this a best practise with okhttp? It feels like it makes it harder to follow the code and debug what is actually being sent.
I'd personally prefer extending OkhttpBaseApi with something like
open fun prepareRequestBuilder(): RequestBilder
that can then be used to add the header to every request.
| val originalRequest = chain.request() | ||
| val requestBuilder = originalRequest.newBuilder() | ||
| .header("WISE_KEY", genWisekey()) | ||
| if ( auth != null) requestBuilder.addHeader("Authorization", auth!!.token) |
There was a problem hiding this comment.
Please make sure all new code is auto-formatted with Android Studios default formatting (Code > Reformat file)
| apiKeyId = libraryData.getString("api-key-id") | ||
| apiKey = libraryData.getString("api-key") | ||
| applicationName = libraryData.getString("app-name") | ||
| } |
There was a problem hiding this comment.
What happens in the else case? These are lazyinit properties, do things just crash?
There was a problem hiding this comment.
These api settings items are always needed. I will remove if-check
| searchResultList, | ||
| getInt("total"), | ||
| getInt("total") / getInt("perpage"), | ||
| getInt("offset") / getInt("perpage") |
There was a problem hiding this comment.
Is this correct, wouldn't we need to round up?
There was a problem hiding this comment.
integer/integer will result in integer: it will do an implicit floor
| val response = httpPost( | ||
| "$baseurl/cgi-bin/bx.pl", | ||
| ("prt=INTERNET&var=portal&vestnr=$homeBranch&fmt=json&search_in=$scope&amount=$AMOUNT&catalog=default&event=osearch" + | ||
| "&preset=all&offset=${page*AMOUNT}$mediaTypeQS&qs=$queryString&vcgrpf=0&backend=wise&vcgrpt=$vcgrpt").toRequestBody("application/x-www-form-urlencoded".toMediaTypeOrNull()), |
There was a problem hiding this comment.
I think there's some missing encoding here, what if the search query contains e.g. a &? Better use something like FormBuilder!
| val itemId = media!!.split(":")[2] | ||
|
|
||
| httpDelete("$baseurl/restapi/patron/${auth!!.patronSystemId}/library/${auth!!.libraryId}/hold/$itemId") | ||
| return OpacApi.CancelResult(OpacApi.MultiStepResult.Status.OK) |
There was a problem hiding this comment.
We should not return OK without any check if this actually worked, maybe at least check the HTTP response code?
|
|
||
|
|
||
| override fun checkAccountData(account: Account) { | ||
| auth = login(account) |
There was a problem hiding this comment.
have you checked that this fails properly for wrong credentials?
WISE is used by 75% of the public libraries in NL, dutch speaking BE and some US public libraries. https://www.oclc.org/go/nl/wise-nl.html