Recorded Books / Oneclickdigital are a provider of electronic audio books. They have an API that lets users checkout/checkin/search/place holds etc on their items. It would be great if users could do this within Koha http://developer.oneclickdigital.us/
Created attachment 57402 [details] [review] Bug 17602 : Add OneClickDigital Integration to Koha Koha::ExternalContent::OneClickDigital - a wrapper around WebService::ILS::OneClickDigital::PartnerPatron Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_OneClickDigital.t
Created attachment 57403 [details] [review] Bug 17602 OneClickDigital* sysprefs
Created attachment 57404 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Testing this with the OneClick digital credentials shared between ByWater/Catalyst I am not able to get any search results, I either get an error or a constant spin - does this work in your testing currently?
Created attachment 60855 [details] [review] Bug 17602 : Add OneClickDigital Integration to Koha Koha::ExternalContent::OneClickDigital - a wrapper around WebService::ILS::OneClickDigital::PartnerPatron Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_OneClickDigital.t
Created attachment 60856 [details] [review] Bug 17602 OneClickDigital* sysprefs
Created attachment 60857 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 61866 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 61932 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Basic functionality seems to be working right now, a couple issues: 1 - We should be able to get results without being signed in - this would mirror functionality for other sources 2 - The account page should offer a 'logout' option 3 - When no items are checked out or on hold the account tab should not display the headings in the account tab 4 - Image links in OneClick results lead no where, ideally these would point out to the oneclick site for the library 5 - It would be nice to have a few more details on the results page if possible
Created attachment 62247 [details] [review] Bug 17602 Koha::ExternalContent->koha_patron() will return undef rather than die if user not logged in
Created attachment 62248 [details] [review] Bug 17602 Add OneClickDigital Integration to Koha Koha::ExternalContent::OneClickDigital - a wrapper around WebService::ILS::OneClickDigital::PartnerPatron Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_OneClickDigital.t
Created attachment 62249 [details] [review] Bug 17602 OneClickDigital* sysprefs
Created attachment 62250 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Searchign when nit signed in works, but I am required to sign in once I click the results link While my acocunt works on the oneclick site I am getting this error on results page: Invalid user_id nick@bywatersolutions.com Invalid Basic Token or permission denied at /usr/local/share/perl/5.20.2/WebService/ILS.pm line 748. at /home/vagrant/kohaclone/Koha/ExternalContent/OneClickDigital.pm line 61. When I click the image on the results page it points to: http://localhost:8080/cgi-bin/koha/XXX User tab either has error above or: Bad Request
Created attachment 63392 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 63778 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 63779 [details] [review] Bug 17602 Add OneClickDigital Integration to Koha Koha::ExternalContent::OneClickDigital - a wrapper around WebService::ILS::OneClickDigital::PartnerPatron OneClickDigital* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_OneClickDigital.t
Created attachment 63780 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 63857 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 64070 [details] [review] Bug 17602 Add OneClickDigital Integration to Koha Koha::ExternalContent::OneClickDigital - a wrapper around WebService::ILS::OneClickDigital::PartnerPatron OneClickDigital* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_OneClickDigital.t
Created attachment 64071 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 64095 [details] [review] Bug 17602 Add OneClickDigital Integration to Koha Koha::ExternalContent::OneClickDigital - a wrapper around WebService::ILS::OneClickDigital::PartnerPatron OneClickDigital* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_OneClickDigital.t
Created attachment 64096 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 64098 [details] [review] Bug 17602 Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 64099 [details] [review] Bug 17602 Add OneClickDigital Integration to Koha Koha::ExternalContent::OneClickDigital - a wrapper around WebService::ILS::OneClickDigital::PartnerPatron OneClickDigital* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_OneClickDigital.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 64100 [details] [review] Bug 17602 Add Oneclickdigital integration to Koha Add OneClickdigital to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with Oneclickdigital) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Hi everyone here. I tested this with some credentials Nick gave me. I have a couple observations about this one. Important ========= - I first tested opac-oneclick-search.pl using the q= query param. That way I found stuff in Oneclick. The searched on the main opac search for a term I knew should give results (q=1776), and I got (the first time) "Error searching OneClickDigital collection: Not Found". I then reloaded the results page, and got "Found 1 results in OneClickDigital collection". This one seemed ok - When clicking in the '1 results' link, I got a red message "Error searching OneClickDigital collection: Not Found", on reloading, I got the search results for 1776. => FAIL: It seems there's a problem on the first query, or a throttling stuff getting in the middle. Moderate ======== => FAIL: The search results on opac-oneclick-search.pl have a different styling than Koha's search results. They look nicer, but I think the same XSLT's shold be used. I understood that this is a transition until this search results are displayed inline along with the regular (local) search results. I wouldn't say the second item is a blocker, but it might be a reason for others to vote against its inclusion. Minor ===== - Another (minor) thing I would consider changing is the wording for the sysprefs description. I think they should clearly match what Oneclick provides the user to configure Koha (LibraryID vs. library #) - Warnings are logged: Use of uninitialized value in concatenation (.) or string at /usr/local/share/perl/5.20.2/WebService/ILS/OneClickDigital/PartnerPatron.pm line 58. Use of uninitialized value in concatenation (.) or string at /home/vagrant/kohaclone/Koha/ExternalContent/OneClickDigital.pm line 73. QA scripts ========== The QA scripts complain about oneclick-checkout.inc => FAIL koha-tmpl/opac-tmpl/bootstrap/en/includes/oneclick-checkout.inc FAIL forbidden patterns forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 1)
(In reply to Tomás Cohen Arazi from comment #28) > QA scripts > ========== > The QA scripts complain about oneclick-checkout.inc > => > FAIL koha-tmpl/opac-tmpl/bootstrap/en/includes/oneclick-checkout.inc > FAIL forbidden patterns > forbidden pattern: Contains old bootstrap style for buttons (see bug > 16239) (line 1) That is due to <div class="modal hide"> for pop-ups. There's a number of templates using that. What should we use under the circumstances?
Created attachment 65988 [details] [review] Bug 17602 RecordedBooks Integration to Koha - sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t
Created attachment 65989 [details] [review] Bug 17602 RecordedBooks integration to Koha - OPAC Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab
Created attachment 66341 [details] [review] Bug 17602 Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 66342 [details] [review] Bug 17602 RecordedBooks Integration to Koha - sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 66343 [details] [review] Bug 17602 RecordedBooks integration to Koha - OPAC Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 67236 [details] [review] Bug 17602: Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 67237 [details] [review] Bug 17602: Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 67238 [details] [review] Bug 17602: RecordedBooks Integration to Koha This patch introduces the required sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 67240 [details] [review] Bug 17602: OPAC integration of RecordedBooks Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ok, I tested this patchset and have some comments. I used the credentials ByWater provided, with a logged-in user with Nick's email address set as primary (which is recognized as valid for the RecordedBooks credentials). From the functional perspective, everything works as expected: * Searches * Checkout * Checkin * Holds I would only try to make the UI tell the users it is working on something and prevent them from (re)clicking other RecordedBooks buttons (spinning symbol or similar). Not worth failing the patchset because of it, but definitely a needed followup. Regarding the whole patchset, I'd say tests coverage is not good enough. And having the tests require valid credentials for the proprietary external system seems too much. There are two options IMHO: - If the provider does have a sandbox/testbed infrastructure, use it - If it doesn't, keep the tests (which skip if credentials are missing) but add new db-independent tests with mocked behaviour for WebService::ILS. Auth_with_ldap.t can be a good source of examples for completely mocking the tests. I'm looking forward to help having this in 17.11, so don't hesitate to contact me for quick feedback.
I'm not sure how to deal with this kind of patchsets. I'm passing it to the RM. The functionality is the expected one. The main problem is the fact that the vendor is not providing good testing tools. I think it deserves to go in. The users sponsoring it will for sure make us improve it if needed.
(In reply to Tomás Cohen Arazi from comment #39) > Regarding the whole patchset, I'd say tests coverage is not good enough. And > having the tests require valid credentials for the proprietary external > system seems too much. There are two options IMHO: > > - If the provider does have a sandbox/testbed infrastructure, use it > - If it doesn't, keep the tests (which skip if credentials are missing) but > add new db-independent tests with mocked behaviour for WebService::ILS. > Auth_with_ldap.t can be a good source of examples for completely mocking the > tests. Chris, Srdjan, any ideas how we could improve that? Can someone share the test credentials with me?
Created attachment 67859 [details] [review] Bug 17602 follow-up: t/Koha_ExternalContent_RecordedBooks.t
Created attachment 67860 [details] [review] t/Koha_ExternalContent_OverDrive.t correction
(In reply to Jonathan Druart from comment #41) > (In reply to Tomás Cohen Arazi from comment #39) > > Regarding the whole patchset, I'd say tests coverage is not good enough. And > > having the tests require valid credentials for the proprietary external > > system seems too much. There are two options IMHO: > > > > - If the provider does have a sandbox/testbed infrastructure, use it > > - If it doesn't, keep the tests (which skip if credentials are missing) but > > add new db-independent tests with mocked behaviour for WebService::ILS. > > Auth_with_ldap.t can be a good source of examples for completely mocking the > > tests. > > Chris, Srdjan, any ideas how we could improve that? > Can someone share the test credentials with me? We have no contractual relationship, or any clients who use it. It is probably best to talk to Bywater or someone who have clients that might have a test account.
I've added a patch with some limited value. And another seemingly unrelated, which you may choose to apply or drop depending on the policies/feelings. My position is that as is any proper automated testing would be difficult and expensive in terms of effort. Considering it is a niche area, I doubt it is worth it. If it is any comfort you can take my word that automated tests for WebService::ILS are passing. It is unfortunate that I started this work at the time when api was not established. The right thing would be to move svc code to api/v1, at which point it would be much easier and make sense to do Mojo tests. But again, would it be worth the effort.
Quick note: use String.format() when needed.
(In reply to Jonathan Druart from comment #46) > Quick note: use String.format() when needed. Money or somewhere else?
(In reply to Srdjan Jankovic from comment #47) > (In reply to Jonathan Druart from comment #46) > > Quick note: use String.format() when needed. > > Money or somewhere else? Hum? Money? I am talking about that kind of generated strings: $( '#recordedbooks-status' ).html( '<strong>' + _("Found") + ' ' + data.total + ' ' + _("results in the library's RecordedBooks collection.") + '</strong>' ); should be _("Found %s results in the library's RecordedBooks collection.").format(data.total) But it was a not for myself.
(In reply to Srdjan Jankovic from comment #43) > Created attachment 67860 [details] [review] [review] > t/Koha_ExternalContent_OverDrive.t correction Why this change? See commit c1185533c61e8266f9d89136e8e0debddf71d497 Bug 18851: Make OverDrive.T pass if dbms is not running
Comment on attachment 67860 [details] [review] t/Koha_ExternalContent_OverDrive.t correction Obsoleted by 18851
(In reply to Jonathan Druart from comment #48) > _("Found %s results in the library's RecordedBooks > collection.").format(data.total) > > But it was a not for myself. Did not know how we store translatables. Want me to change it?
Created attachment 69019 [details] [review] Bug 17602: Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69020 [details] [review] Bug 17602: RecordedBooks Integration to Koha This patch introduces the required sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69021 [details] [review] Bug 17602: OPAC integration of RecordedBooks Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69022 [details] [review] Bug 17602 follow-up: t/Koha_ExternalContent_RecordedBooks.t
Created attachment 69023 [details] [review] Bug 17602: Update opac.css
Created attachment 69024 [details] [review] Bug 17602: Ease translation Use String.format() to make translation easier
Created attachment 69025 [details] [review] Bug 17602: Handle '60+' if there are more than 60 results The service returns "60+" if there are more than 60 results. Without this patch the "Found %s results in RecordedBooks collection" line is removed.
Created attachment 69026 [details] [review] Bug 17602: Adapt test plan to make the tests pass
Created attachment 69027 [details] [review] Bug 17602: Fix few minor QA issues
Some additional remarks: 1. At some point I got the following error: {"error":"Checkout item already exists"}Status: 200 OK Pragma: no-cache X-frame-options: SAMEORIGIN Cache-control: no-cache Content-Type: application/json; charset=UTF-8 {"action":null,"is_identified":true,"error":"ARRAY(0xbb2fe50)"} When I clicked several time on the "checkout" button. I think we should hide them or add a spinner to tell the user something is going on. 2. Strings from .js files are not translatable, `git grep MSG_` to know how to make them translatable. 3. I see code related to pagination, but I do not see the previous/next buttons. I only see the OPACnumSearchResults first results. And a question: Koha/ExternalContent/RecordedBooks.pm 76 user_agent_params => { agent => $class->agent_string } Where is this used?
Created attachment 69101 [details] [review] Bug 17602: Translations rework
(In reply to Jonathan Druart from comment #61) > Some additional remarks: > 1. At some point I got the following error: > {"error":"Checkout item already exists"}Status: 200 OK I have to let Nick decide on that one > 2. Strings from .js files are not translatable, `git grep MSG_` to know how > to make them translatable. Done. Please let me know if it's not what you had in mind. > 3. I see code related to pagination, but I do not see the previous/next > buttons. I only see the OPACnumSearchResults first results. Yes, at some stage the idea was to integrate all external searches into the main results page, so I stopped short of implementing paging - will be thrown away. Is it acceptable to leave as is? > > And a question: > Koha/ExternalContent/RecordedBooks.pm > 76 user_agent_params => { agent => $class->agent_string } > Where is this used? It is a param for WebService::ILS. Cannot remember any more, possibly was a requirement for a provider that even did not get implemented.
(In reply to Srdjan Jankovic from comment #63) > (In reply to Jonathan Druart from comment #61) > > Some additional remarks: > > 1. At some point I got the following error: > > {"error":"Checkout item already exists"}Status: 200 OK > > I have to let Nick decide on that one Nick? > > 2. Strings from .js files are not translatable, `git grep MSG_` to know how > > to make them translatable. > > Done. Please let me know if it's not what you had in mind. I am not sure to understand what you did, I already fixed the some translations (patch "Bug 17602: Ease translation") from .tt, you needed to fix the strings in .js files (.inc and .tt were ok) > > 3. I see code related to pagination, but I do not see the previous/next > > buttons. I only see the OPACnumSearchResults first results. > > Yes, at some stage the idea was to integrate all external searches into the > main results page, so I stopped short of implementing paging - will be > thrown away. Is it acceptable to leave as is? I do not think we should integrate code that is not used. > > And a question: > > Koha/ExternalContent/RecordedBooks.pm > > 76 user_agent_params => { agent => $class->agent_string } > > Where is this used? > > It is a param for WebService::ILS. Cannot remember any more, possibly was a > requirement for a provider that even did not get implemented. So we remove? Moving out the NSO queue.
(In reply to Jonathan Druart from comment #64) > I am not sure to understand what you did, I already fixed the some > translations (patch "Bug 17602: Ease translation") from .tt, you needed to > fix the strings in .js files (.inc and .tt were ok) Right. Ok, before I do that. how do we handle messages that are the same in both OPAC and intranet?
(In reply to Srdjan Jankovic from comment #65) > (In reply to Jonathan Druart from comment #64) > > > I am not sure to understand what you did, I already fixed the some > > translations (patch "Bug 17602: Ease translation") from .tt, you needed to > > fix the strings in .js files (.inc and .tt were ok) > > Right. Ok, before I do that. how do we handle messages that are the same in > both OPAC and intranet? I suggest you just duplicate them.
Created attachment 69357 [details] [review] Bug 17602: Translations rework
Created attachment 69358 [details] [review] Bug 17602: fix RecordedBooks results pagination
Created attachment 69359 [details] [review] Bug 17602: Removed unused param to RecordedBooks API
(In reply to Jonathan Druart from comment #64) > > > 2. Strings from .js files are not translatable, `git grep MSG_` to know how > > > to make them translatable. > > > > Done. Please let me know if it's not what you had in mind. > > I am not sure to understand what you did, I already fixed the some > translations (patch "Bug 17602: Ease translation") from .tt, you needed to > fix the strings in .js files (.inc and .tt were ok) This should be fine now. > > > > 3. I see code related to pagination, but I do not see the previous/next > > > buttons. I only see the OPACnumSearchResults first results. > > > > Yes, at some stage the idea was to integrate all external searches into the > > main results page, so I stopped short of implementing paging - will be > > thrown away. Is it acceptable to leave as is? > > I do not think we should integrate code that is not used. Actually that was a bug. Fixed. > > > > And a question: > > > Koha/ExternalContent/RecordedBooks.pm > > > 76 user_agent_params => { agent => $class->agent_string } > > > Where is this used? > > > > It is a param for WebService::ILS. Cannot remember any more, possibly was a > > requirement for a provider that even did not get implemented. > > So we remove? Removed.
(In reply to Jonathan Druart from comment #64) > (In reply to Srdjan Jankovic from comment #63) > > (In reply to Jonathan Druart from comment #61) > > > Some additional remarks: > > > 1. At some point I got the following error: > > > {"error":"Checkout item already exists"}Status: 200 OK > > > > I have to let Nick decide on that one > > Nick? > I was not able to recreate the error, a spinner would be nice here to indicate action is being performed
Created attachment 73708 [details] [review] Bug 17602: Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 73709 [details] [review] Bug 17602: RecordedBooks Integration to Koha This patch introduces the required sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 73710 [details] [review] Bug 17602: OPAC integration of RecordedBooks Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 73711 [details] [review] Bug 17602 follow-up: t/Koha_ExternalContent_RecordedBooks.t
Created attachment 73712 [details] [review] Bug 17602: Ease translation Use String.format() to make translation easier
Created attachment 73713 [details] [review] Bug 17602: Handle '60+' if there are more than 60 results The service returns "60+" if there are more than 60 results. Without this patch the "Found %s results in RecordedBooks collection" line is removed.
Created attachment 73714 [details] [review] Bug 17602: Adapt test plan to make the tests pass
Created attachment 73715 [details] [review] Bug 17602: Fix few minor QA issues
Created attachment 73716 [details] [review] Bug 17602: Translations rework
Created attachment 73717 [details] [review] Bug 17602: fix RecordedBooks results pagination
Created attachment 73718 [details] [review] Bug 17602: Removed unused param to RecordedBooks API
Created attachment 73719 [details] [review] Bug 17602: (follow-up) Adjust js paths to use version add spinner
Created attachment 73725 [details] [review] Bug 17602: (follow-up) Adjust js paths to use version add spinner Also included recompiled version of opac.css that we forgot. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
1. Maybe add a ref to OneClickdigital in the pref? "No system preferences matched your search for: OneClickdigital" (not blocking) 2. Found 241+ results in the library's RecordedBooks collection "+" ? 3. /opac-tmpl/lib/jquery/plugins/themes/classic/throbber.gif must be using [% theme %]/[% interface %] 4. In the checkout list there is a link .epub (below "downloads"), but it downloads a .acsm file (??) Is it expected? 5. expires.toLocaleString() does not work what you are expecting, the date is always displayed in the same format (do not depend on dateformat) 6. https://screenshots.firefox.com/C5OmithULkvMQtTe/catalogue.kohadev.org "Check out" and "Place hold" button are too close and not well styled (not blocking) 7. Place hold, hit cgi-bin/koha/opac-user.pl, cancel the hold. The hold is still listed (not blocking) 8. /cgi-bin/koha/opac-recordedbooks-search.pl?q=d click on an image, https://carnegiestoutia.oneclickdigital.com/#titles/9781470360603 => "Your connection is not secure", awesome...
Created attachment 76180 [details] [review] Bug 17602: Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 76181 [details] [review] Bug 17602: RecordedBooks Integration to Koha This patch introduces the required sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 76182 [details] [review] Bug 17602: OPAC integration of RecordedBooks Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 76183 [details] [review] Bug 17602 follow-up: t/Koha_ExternalContent_RecordedBooks.t
Created attachment 76184 [details] [review] Bug 17602: Ease translation Use String.format() to make translation easier
Created attachment 76185 [details] [review] Bug 17602: Handle '60+' if there are more than 60 results The service returns "60+" if there are more than 60 results. Without this patch the "Found %s results in RecordedBooks collection" line is removed.
Created attachment 76186 [details] [review] Bug 17602: Adapt test plan to make the tests pass
Created attachment 76187 [details] [review] Bug 17602: Fix few minor QA issues
Created attachment 76188 [details] [review] Bug 17602: Translations rework
Created attachment 76189 [details] [review] Bug 17602: fix RecordedBooks results pagination
Created attachment 76190 [details] [review] Bug 17602: Removed unused param to RecordedBooks API
Created attachment 76191 [details] [review] Bug 17602: (follow-up) Adjust js paths to use version add spinner Also included recompiled version of opac.css that we forgot. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Created attachment 76192 [details] [review] Bug 17602: update compiled opac.css
Created attachment 76193 [details] [review] Bug 17602: Use Asset TT plugin and fix indentation
Please keep changes to opac.css in a separated patch to ease rebases.
Created attachment 78602 [details] [review] Bug 17602: Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 78603 [details] [review] Bug 17602: RecordedBooks Integration to Koha This patch introduces the required sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 78604 [details] [review] Bug 17602: OPAC integration of RecordedBooks Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 78605 [details] [review] Bug 17602 follow-up: t/Koha_ExternalContent_RecordedBooks.t
Created attachment 78606 [details] [review] Bug 17602: Ease translation Use String.format() to make translation easier
Created attachment 78607 [details] [review] Bug 17602: Handle '60+' if there are more than 60 results The service returns "60+" if there are more than 60 results. Without this patch the "Found %s results in RecordedBooks collection" line is removed.
Created attachment 78608 [details] [review] Bug 17602: Adapt test plan to make the tests pass
Created attachment 78609 [details] [review] Bug 17602: Fix few minor QA issues
Created attachment 78610 [details] [review] Bug 17602: Translations rework
Created attachment 78611 [details] [review] Bug 17602: fix RecordedBooks results pagination
Created attachment 78612 [details] [review] Bug 17602: Removed unused param to RecordedBooks API
Created attachment 78613 [details] [review] Bug 17602: (follow-up) Adjust js paths to use version add spinner Also included recompiled version of opac.css that we forgot. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Created attachment 78614 [details] [review] Bug 17602: Use Asset TT plugin and fix indentation
Created attachment 78615 [details] [review] Bug 17602: (QA follow-up) Fix tests
(In reply to Jonathan Druart from comment #85) > 1. Maybe add a ref to OneClickdigital in the pref? > "No system preferences matched your search for: OneClickdigital" > (not blocking) They are changing the name over to RecordedBooks, I think we should stick with this > > 2. Found 241+ results in the library's RecordedBooks collection > "+" ? This is a function of the values returned from the API, unfortunate but not in our control > > 3. /opac-tmpl/lib/jquery/plugins/themes/classic/throbber.gif > must be using [% theme %]/[% interface %] This stil needs to be fixed, these lines are in the js file so don't have template variables > > 4. In the checkout list there is a link .epub (below "downloads"), but it > downloads a .acsm file (??) Is it expected? Yes, due to DRM you must get your epub via the adobe app which uses acsm > > 5. expires.toLocaleString() does not work what you are expecting, the date > is always displayed in the same format (do not depend on dateformat) This is what exists in the code for overdrive currently I believe - fix on a future bug? > > 6. https://screenshots.firefox.com/C5OmithULkvMQtTe/catalogue.kohadev.org > "Check out" and "Place hold" button are too close and not well styled (not > blocking) I will ask Owen for an eye > > 7. Place hold, hit cgi-bin/koha/opac-user.pl, cancel the hold. The hold is > still listed (not blocking) Let's leave as improvement > > 8. /cgi-bin/koha/opac-recordedbooks-search.pl?q=d > click on an image, > https://carnegiestoutia.oneclickdigital.com/#titles/9781470360603 => "Your > connection is not secure", awesome... This is a configuratio issue - testing link used *.oneclickdigital.com - should now use *.rbdigital.com
Created attachment 79247 [details] [review] Bug 17602: Move spinner location to include file
Created attachment 79668 [details] [review] Bug 17602: Koha::ExternalContent->koha_patron() will retutn undef rather than die if user not logged in Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79669 [details] [review] Bug 17602: RecordedBooks Integration to Koha This patch introduces the required sysprefs and Koha::ExternalContent::RecordedBooks Koha::ExternalContent::RecordedBooks - a wrapper around WebService::ILS::RecordedBooks::PartnerPatron RecordedBooks* sysprefs Nothing functional to test with this patch yet. But you can run the tests that come with it t/db_dependent/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79670 [details] [review] Bug 17602: OPAC integration of RecordedBooks Add RecordedBooks to the users page in the public interface To Test: 1/ Apply all 3 patches 2/ Set the sysprefs to valid values (you will need a test account with RecordedBooks) 3/ Try a search 4/ Login to the OPAC, try to place a hold, or check an item out 5/ Check the opac-user page, see if your items are showing on the oneclickdigital tab Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79671 [details] [review] Bug 17602 follow-up: t/Koha_ExternalContent_RecordedBooks.t Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79672 [details] [review] Bug 17602: Ease translation Use String.format() to make translation easier Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79673 [details] [review] Bug 17602: Handle '60+' if there are more than 60 results The service returns "60+" if there are more than 60 results. Without this patch the "Found %s results in RecordedBooks collection" line is removed. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79674 [details] [review] Bug 17602: Adapt test plan to make the tests pass Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79675 [details] [review] Bug 17602: Fix few minor QA issues Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79676 [details] [review] Bug 17602: Translations rework Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79677 [details] [review] Bug 17602: fix RecordedBooks results pagination Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79678 [details] [review] Bug 17602: Removed unused param to RecordedBooks API Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79679 [details] [review] Bug 17602: (follow-up) Adjust js paths to use version add spinner Also included recompiled version of opac.css that we forgot. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79680 [details] [review] Bug 17602: Use Asset TT plugin and fix indentation Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79681 [details] [review] Bug 17602: (QA follow-up) Fix tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 79682 [details] [review] Bug 17602: Move spinner location to include file Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Awesome work all! Pushed to master for 18.11
Created attachment 79752 [details] [review] Bug 17602: Add missing html filters
(In reply to Jonathan Druart from comment #133) > Created attachment 79752 [details] [review] [review] > Bug 17602: Add missing html filters Followup pushed to master, thanks Jonathan!
New Feature, will not be backported to 18.05.x series.
From the tests: t/Koha_ExternalContent_RecordedBooks.t: skip "no RecordedBooks partner credentials", 1 unless $ENV{RECORDEDBOOKS_TEST_LIBRARY_ID}; t/Koha_ExternalContent_RecordedBooks.t: t::lib::Mocks::mock_preference('RecordedBooksLibraryID', $ENV{RECORDEDBOOKS_TEST_LIBRARY_ID}); We never set this ENV var, are there data we could use to make it pass? See bug 21567.
It is a huge pain, you have to have their test account. Which includes a password, and I'm not sure how is that dealt with in automated tests scenarios. The test is more or less a repeat of what is done in WebService::ILS itself, just through Koha. The main reason why I made WebService::ILS outside Koha is to cheat a bit - I could tests the whole structure locally and relieve that pain to some extent.