A library I consult for is interested in Koha as an ILS solution. There are two blockers currently for the adoption of Koha. The first is an Interlibrary Loan module. The library currently uses Innovative Millennium software, so that functionality will serve as the base requirements for this feature. This bug will serve as the enhancement request for Interlibrary Loan. I plan to contribute this feature over the coming months. There will be a Circulation piece and an OPAC piece for this module.
Hi Sarah, thank you for opening a bug about this :) There is already a page on the wiki where we have started to collect information about ILL protocols and core functionality: http://wiki.koha-community.org/wiki/Inter_Library_Loan I suggest putting your ideas and plans in an RFC on the wiki too. This way other libraries interested in that feature can see what's being planned. It can also help to get ideas and create discussion about how how this can fit into Koha in a good and flexible way. There is also another open source project related to ILL that might be interesting: http://www.fulfillment-ill.org/about.php
I second the recommendation to look at the FulfILLment project. I've just volunteered to help test the project's Koha connector. I haven't gotten as far as seeing anything in action yet but Equinox has a great reputation so I'm looking forward to it. At the very least getting to know that project better might give you some guidance on what problems it *doesn't* solve that you would have yours do or vice versa.
Katrin, I will be documenting the current functionality and developing and RFC wiki over the next week or so. Owen, My understanding is that the FulfILLment project will not be ready for some time yet. I'm thinking about creating an iterative feature where we support ILL "base", which would allow the receiving library to track a request and manage the asset and then in the future add the enhanced functionality that would be a holistic system for both sides of the transaction (likely via FulfiLLment). Thoughts? -Sarah
(In reply to comment #3) > My understanding is that the FulfILLment project will not be ready for some > time yet. I don't know. I certainly wouldn't undertake a big development project without knowing for sure. On the other hand, a solution that didn't require a separate system (like FulfILLment) might be preferable to some libraries. > Thoughts? I don't have any smart things to say about how an ILL addon should or shouldn't work, I just wanted to make sure you were taking FilfILLment into account.
Switching this to unassigned/ new because there has never been any work made public in over three years.
Hello, PTFS Europe is working on ILL functionality within Koha. See the URL above for a link to the RFC. Best wishes, Alex
(In reply to Alex Sassmannshausen from comment #6) > Hello, > > PTFS Europe is working on ILL functionality within Koha. See the URL above > for a link to the RFC. > > Best wishes, > > Alex Is there any update here?
(In reply to Bob Birchall from comment #7) > (In reply to Alex Sassmannshausen from comment #6) > > Hello, > > > > PTFS Europe is working on ILL functionality within Koha. See the URL above > > for a link to the RFC. > > > > Best wishes, > > > > Alex > > Is there any update here? Hi Bob, We are currently investing time into this again. We are hoping to provide code to this bug number by next Friday. Best wishes, Alex
Created attachment 59893 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI.
Created attachment 59894 [details] [review] DO NOT PUSH: Updated Schema files. * Koha/Schema/Result/Borrower.pm: New file. * Koha/Schema/Result/Branch.pm: New file. * Koha/Schema/Result/Illrequest.pm: New file. * Koha/Schema/Result/Illrequestattribute.pm: New file.
Created attachment 59895 [details] [review] ILLModule
Installing the Dummy ILL connector ================================== The Dummy connector returns canned responses to search requests and should be used for basic testing of ILL functionality. INSTALL ------- - Change to the correct directory: cd <your_Koha_root_directory>/Koha - If the 'Illbackends' directory does not exist, create it: mkdir Illbackends - Change to the backends directory: cd Illbackends - Clone the Dummy connector repository: git clone https://github.com/a-sassmannshausen/Dummy
Should this be "needs signoff", or should it be considered more of a preview?
A preview at this stage. Work is ongoing, but it's functionally ready for people to play with. Any feedback will be gratefully received.
Created attachment 59920 [details] [review] Remove BLDSS configuration from Config.pm. * Koha/Illrequest/Config.pm (getLibraryPrivileges, getDefaultFormats) (getCredentials, getApiUrl, getApiSpecFile): Remove procedures. (_load_configuration): Remove BLDSS specific config.
Created attachment 60485 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI.
Created attachment 60486 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files.
Created attachment 60487 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS
Created attachment 60488 [details] [review] Bug 7317: ILLModule * Koha/Illrequest.pm * Koha/Illrequest/Config.pm * Koha/Illrequestattribute.pm * Koha/Illrequestattributes.pm * Koha/Illrequests.pm * Koha/REST/V1/Illrequests.pm * ill/ill-requests.pl * api/v1/swagger/paths.json * api/v1/swagger/paths/illrequests.json * etc/koha-conf.xml * koha-tmpl/intranet-tmpl/prog/css/staff-global.css * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc * koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc * koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt * koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt * koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results-grouped.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt * koha-tmpl/opac-tmpl/bootstrap/less/opac.less * opac/opac-illrequests.pl * t/db_dependent/Illrequest.t * t/db_dependent/Illrequest/Config.t * t/db_dependent/Illrequest/Dummy.t * t/db_dependent/api/v1/illrequests.t
Table of Contents ───────────────── 1 Change Log (since last set of patches pushed to bugzilla) .. 1.1 Core module .. 1.2 Backends ..... 1.2.1 New backend: FreeForm ..... 1.2.2 New backend: BLDSS (British Library) ..... 1.2.3 Dummy 2 Module Features .. 2.1 What is handled by the module? .. 2.2 What is not yet handled 3 Installation 4 Optional 5 Missing before inclusion in community 6 Test plan .. 6.1 Core Module (+ Dummy backend) ..... 6.1.1 Creating requests (Staff) ..... 6.1.2 Creating requests (Patrons) ..... 6.1.3 Editing requests (Staff) ..... 6.1.4 Editing requests (Patrons) ..... 6.1.5 Requesting Cancellation (Patrons) ..... 6.1.6 Confirming requests (Staff) ..... 6.1.7 Reverting requests (Staff) ..... 6.1.8 Deleting requests (Staff) ..... 6.1.9 Place request with partners (Staff) .. 6.2 BLDSS backend .. 6.3 FreeForm backend 1 Change Log (since last set of patches pushed to bugzilla) ═══════════════════════════════════════════════════════════ 1.1 Core module ─────────────── • Many improvements to general documentation in UI and configuration. • Many improvements to the user interface. • OPAC interface to viewing and commenting on their requests. • OPAC interface for creating new requests with backends of choice. • Add 'metadata' abstraction allowing backends to translate raw data from their source to user friendly content. • Add 'custom_capability's, means by which backends can extend the workflow of the ILL module in a seamless manner. • Add support for custom JS blocks in ILL backend template includes. • Add generic 'backend request -> email to partners' functionality. • Remove loads of legacy code. 1.2 Backends ──────────── 1.2.1 New backend: FreeForm ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • Allows creation of ILL requests using a manual form, which can be extended at creation time. 1.2.2 New backend: BLDSS (British Library) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • Allows searching stock held by the british library, and placing requests against it. • By default uses test account, which means it is useful for learning. 1.2.3 Dummy ╌╌╌╌╌╌╌╌╌╌╌ • Upgrade to support latest features. 2 Module Features ═════════════════ • Extensible core for handling interlibrary loan requests in Koha • As many backends can be added as desirable • OPAC request form as well as Intranet request form • Intranet request management • Ability to create items against a backend (e.g. British Library), using its advanced search features, and to then request the item from peer libraries. 2.1 What is handled by the module? ────────────────────────────────── • Create new requests • Allow staff and customer notes • Process those requests using custom workflows for backends • Retain associations between requests and biblioitems that may be created locally • Peer lending libraries contact management (using Koha's Organizational borrower types) • Storage of core request information (borrower links, branch links, request status, staff and customer notes, unique identifiers) • Storage of arbitrary data returned by backend, and display of that data. 2.2 What is not yet handled ─────────────────────────── • Automatic processing of received ILLs (biblio + biblioitem creation) • Integration into specialized ILL loan & reminder policies 3 Installation ══════════════ • Apply patches (they should apply cleanly and without trouble on master) • Run databaseupdate to generate the ILL tables. • Copy the 'interlibrary_loans' element from '$kohaclone_root/etc/koha-conf.xml' to your active koha_conf.xml • Update the path in 'interlibrary_loans > backend_dir' in koha_conf.xml to point to '$kohaclone_root/Koha/Illbackends' • git clone backends into the backend directory: ┌──── │ cd $kohaclone_root/Koha/ && mkdir Illbackends && cd Illbackends │ git clone https://github.com/a-sassmannshausen/{Dummy,FreeForm,BLDSS} └──── • Enable ILLModule in syspreferences 4 Optional ══════════ • Ensure latest Koha-Conf configuration values for interlibrary_loans are set (see $kohaclone_root/etc/koha-conf.xml) • Create peer lending libraries (by default they are expected to be part of the 'ILLLIBS' borrower category, which also needs to be created). 5 Missing before inclusion in community ═══════════════════════════════════════ • Unit tests • Documentation (code + manual) • Unmediated ILL Request placement workflow • Advanced configuration • Complete Generic Requests workflow (allow setting requests to completed in this way, allow requests to resume backend specific workflows after generic email is sent) • Friendlier error handling. 6 Test plan ═══════════ 6.1 Core Module (+ Dummy backend) ───────────────────────────────── • Ensure the Dummy backend is installed (see [Installation]) • Enable the ILLModule (located under Administration sysprefs) • Create a test patron, grant them at least ILL and usual staff permissions. • Browse to 'Home > ILL requests' (button should be visible) [Installation] See section 3 6.1.1 Creating requests (Staff) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • Click on the 'New ILL request' button. ⁃ If you have multiple backends installed, select 'Dummy' • Complete the form ⁃ Borrowernumber must be a borrowernumber, not a cardnumber. ⁃ You will always be shown the same 2 results • Request one of the items • You have now 'created' the request 6.1.2 Creating requests (Patrons) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • Log in to the OPAC • Click on 'Your interlibrary loan requests' • Click on 'Create a new request' ⁃ If you have multiple backends installed, select 'Dummy' • Complete the form ⁃ As above, you will always be shown the same 2 results • Request one of the items • You have now 'created' the request. 6.1.3 Editing requests (Staff) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • From the ill-requests list, click on 'Manage request' • Click on 'Edit request' • Add some staff notes (or change other values) • Click 'Submit'. 6.1.4 Editing requests (Patrons) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • From the patron screen, click on 'Your interlibrary loan requests' • click on 'View' • Enter some notes. • Click on 'Submit modifications' 6.1.5 Requesting Cancellation (Patrons) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • From the patron screen, click on 'Your interlibrary loan requests' • Click on 'View' • Click on 'Request Cancellation' • The request status should have changed • The request can now be cancelled by staff if they consider the cancellation is still possible 6.1.6 Confirming requests (Staff) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ Once a request is created, it exists in the ILLModule. But the request has not yet been ordered from the source. To do the latter, a request must be confirmed by staff: • From the ill-requests list, click on 'Manage request' • Click on 'Confirm request'. ⁃ The following process will be different for each backend. ⁃ In the case of Dummy, a simple click on the button is sufficient. ⁃ Other backends may take you through several screens. 6.1.7 Reverting requests (Staff) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ Once a request is confirmed it may still be possible to undo this operation. This depends on the backend used, and the policies at the source. • From the ill-requests list, click on 'Manage request' • Click on 'Revert request'. ⁃ The following process will be different for each backend ⁃ In the case of Dummy, a simple click on the button is sufficient. 6.1.8 Deleting requests (Staff) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ • From the ill-requests list, click on 'Manage request' • Click on 'Delete request' ⁃ This button will only be visible when the request has not yet been confirmed. 6.1.9 Place request with partners (Staff) ╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌ Rather than use the sources workflows to source a title for a specific interlibrary loan, it may be desirable to ask a friendly library whether they are able to satisfy the request. The ILLModule supports this through "Generic requests". To use this you will need to create 'Organizational patrons' to represent the partner libraries. They must be of the 'ILLLIBS' patron category (this can be changed by editing the koha-conf.xml file), which must first be created. Ensure that these patrons have email addresses! Once a few patrons have been set up: • From the ill-requests list, click on 'Manage request' • Click on 'Place request with partners' ⁃ This button will only be visible if the request has not yet been confirmed. • Complete the form ⁃ You may select multiple partners in the select list ⁃ You may edit the email as desired. • Click on Send email • If your server is correctly configured to send emails, all should have worked fine and an email should have been sent. 6.2 BLDSS backend ───────────────── A backend to place and manage requests with the British library. • Repeat tests as above. • The workflows will be different for: ⁃ 'create' ⁃ 'confirm' • There is an additional workflow: ⁃ 'status': retrieve the current status of the request at the British library. 6.3 FreeForm backend ──────────────────── A backend to allow arbitrary manual creation of requests using an unrestricted form of fields & values. This backend has no 'source'. As such, requests that are created need to then be 'confirmed' outside of ILLModule (or using the generic requests functionality). • Repeat tests as above. • The workflows will be different for: ⁃ 'create' ⁃ 'confirm' ⁃ 'revert request'
* Reported Issues - We have had a report of the "interlibrary_loans > backend_directory" __PERL_MODULE_DIR__ make variable not expanding in the context of a dev box. If you come across this, please get in touch!
It all went pretty good and worked fine. We just found a couple of things that are worth pointing out: 6.1.1 – Why is the biblio number empty on this screen? Actually one of them has the value zero now that we went through the whole process. 6.1.2 – we noticed that we could request the same title more than once. Maybe not an issue but worth mentioning. 6.1.9 – we received the email fine but it does not say who it is from, ther than it has the from email address. Should it have the from organisation above the status or is the staff member expected to customise the email text to indicate their organisation? Dear Sir/Madam, We would like to request an interlibrary loan for a title matching the following description: - Status: New - Author: A.N. Other - ID: 5678 - Title: Interlibrary loans in Koha Please let us know if you are able to supply this to us. Kind Regards 6.2 – We found it difficult to find any matching titles. What is keywords for? (especially as title has a separate entry field) image attached Also we got an error when we tried this search – didn’t seem to like the book filter: Error performing operation image attached When trying to confirm a request from the BL Software error: The API responded with an error: 500 Internal Server Error Detail: <?xml version="1.0" encoding="UTF-8"?><apiResponse><timestamp>2017-03-28 13:15:56.941 BST</timestamp><status>8</status><message>Authentication failed: Exception occurred processing authentication, please contact BL Customer Services</message><result /></apiResponse> at /usr/share/koha-interleaf/Koha/Illbackends/BLDSS/Base.pm line 481. For help, please send mail to the webmaster (alex.sassmannshausen@ptfs-europe.com), giving this error message and the time and date of the error.
Hi Daphne, Thank you very much for taking the time to test! I've added some comments in line. (In reply to Daphne Hoolahan from comment #22) > It all went pretty good and worked fine. > > We just found a couple of things that are worth pointing out: > > 6.1.1 – Why is the biblio number empty on this screen? Actually one of them > has the value zero now that we went through the whole process. The biblio number would be the number that a library assigns to an ILL when they receive it, before they lend it out to a borrower. As a result, the ILLmodule will just allow it to be empty and never assigns a value to it. It is the librarian that manually assigns a number to it. I need to investigate why the biblio is set to 0 in more detail, but I believe this is because a blank field is converted to a 0 at some point in conversion from form input to koha object. > 6.1.2 – we noticed that we could request the same title more than once. > Maybe not an issue but worth mentioning. Indeed — the module currently carries out no validation. This might be an interesting enhancement at a later stage! > 6.1.9 – we received the email fine but it does not say who it is from, ther > than it has the from email address. Should it have the from organisation > above the status or is the staff member expected to customise the email text > to indicate their organisation? > > Dear Sir/Madam, > > We would like to request an interlibrary loan for a title matching the > following description: > > - Status: New > - Author: A.N. Other > - ID: 5678 > - Title: Interlibrary loans in Koha > > Please let us know if you are able to supply this to us. > > Kind Regards The email headers (from address) should be set at the server level. I.e. changing the server configuration should fix that issue. Having said that, it may well be a good idea to add a hard-coded block with the libraries delivery address to this template. A future patch should use notice templates to allow customization of these emails. > 6.2 – We found it difficult to find any matching titles. What is keywords > for? (especially as title has a separate entry field) The keywords field should just search "anywhere". Adding more keywords should significantly reduce the number of hits. The searches against the test system will be searching against the British Library test API — which I'm very confident has an extremely limited result set. I believe that explains the difficulty you had in finding results? > image attached > > Also we got an error when we tried this search – didn’t seem to like the > book filter: > Error performing operation > image attached > > When trying to confirm a request from the BL > > Software error: > > The API responded with an error: 500 Internal Server Error > Detail: <?xml version="1.0" > encoding="UTF-8"?><apiResponse><timestamp>2017-03-28 13:15:56.941 > BST</timestamp><status>8</status><message>Authentication failed: Exception > occurred processing authentication, please contact BL Customer > Services</message><result /></apiResponse> at > /usr/share/koha-interleaf/Koha/Illbackends/BLDSS/Base.pm line 481. > For help, please send mail to the webmaster > (alex.sassmannshausen@ptfs-europe.com), giving this error message and the > time and date of the error. This is a peculiar error message, as it's to do with authentication. I expect a great number of request confirmations against the BL test system to fail, but normally with different error messages. I will investigate this issue in more detail. I will also add a block with library details to the generic email template. Probably just after the "Kind regards". What do you think? Best wishes, Alex
I think I foud a problem in Koha/Illrequest.pm, about line 646. In this block of code: # Almost there, but category code didn't quite work. my ( $branch_limit, $brw_limit ) = ( $branch_rules->{count}, $brw_rules->{count} ); my ( $branch_count, $brw_count ) = ( $self->_limit_counter( $branch_rules->{method}, { branch_id => $branchcode } ), $self->_limit_counter( $brw_rules->{method}, { borrower_id => $patron->borrowernumber } ), ); branch_id should be branchcode, and borrower_id should be borrowernumber, I think?
I will also add a block with library details to the generic email template. Probably just after the "Kind regards". What do you think? That's an excellent idea.
The keywords field should just search "anywhere". Adding more keywords should significantly reduce the number of hits. The searches against the test system will be searching against the British Library test API — which I'm very confident has an extremely limited result set. I believe that explains the difficulty you had in finding results? Daphne:Agreed
(In reply to Magnus Enger from comment #24) > I think I foud a problem in Koha/Illrequest.pm, about line 646. In this > block of code: > > # Almost there, but category code didn't quite work. > my ( $branch_limit, $brw_limit ) > = ( $branch_rules->{count}, $brw_rules->{count} ); > my ( $branch_count, $brw_count ) = ( > $self->_limit_counter( > $branch_rules->{method}, { branch_id => $branchcode } > ), > $self->_limit_counter( > $brw_rules->{method}, { borrower_id => $patron->borrowernumber } > ), > ); > > branch_id should be branchcode, and borrower_id should be borrowernumber, I > think? You are absolutely right! I have committed a fix in my dev branch and hope to push the new additions later today. Alex
It also looks like there is a missing "use Koha::Illrequests;" in Koha/Illrequest.pm? When I try to use Koha::Illrequest in the Dancer-based NCIPServer I get this error: [24015] error @1.447857> [hit #5]request to POST / crashed: Can't locate object method "search" via package "Koha::Illrequests" (perhaps you forgot to load "Koha::Illrequests"?) at /path/to/kohaclone/Koha/Illrequest.pm line 684. in /usr/share/perl5/Dancer/Handler.pm l. 98
Created attachment 63081 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI.
Created attachment 63082 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files.
Created attachment 63083 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS
Created attachment 63084 [details] [review] Bug 7317: ILLModule * Koha/Illrequest.pm * Koha/Illrequest/Config.pm * Koha/Illrequestattribute.pm * Koha/Illrequestattributes.pm * Koha/Illrequests.pm * Koha/REST/V1/Illrequests.pm * ill/ill-requests.pl * api/v1/swagger/paths.json * api/v1/swagger/paths/illrequests.json * etc/koha-conf.xml * koha-tmpl/intranet-tmpl/prog/css/staff-global.css * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc * koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc * koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt * koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt * koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results-grouped.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt * koha-tmpl/opac-tmpl/bootstrap/less/opac.less * opac/opac-illrequests.pl * t/db_dependent/Illrequest.t * t/db_dependent/Illrequest/Config.t * t/db_dependent/Illrequest/Dummy.t * t/db_dependent/api/v1/illrequests.t
Created attachment 63085 [details] [review] Bug 7317: Bugfix: copy ill folder during installation. * Makefile.PL: Add './ill' to list of installable directories.
Created attachment 63086 [details] [review] Bug 7317: Allow journey to 'COMP' status. * Koha/Illrequest.pm (_core_status_graph): Add 'COMP' journey, and method. (mark_completed): new procedure. * ill/ill-requests.pl: Handle mark_completed.
Created attachment 63087 [details] [review] Bug 7317: Make error messages slightly more user friendly. * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt: Change layout of errors, don't display cancel heading on cancel error.
Created attachment 63088 [details] [review] Bug 7317: Allow display/hiding of arbitrary request columns Initially display a small selection of core fields, allow the user to progressively display additional metadata fields if required
Created attachment 63089 [details] [review] Bug 7317: Bugfix: Use correct column names in check_limits. * Koha/Illrequest.pm (check_limits): Change column names used to `branchcode` & `borrowernumber`.
Created attachment 63090 [details] [review] Bug 7317: Add missing reference for `Koha::Illrequest`. * Koha/Illrequest.pm: Add missing `use` statement.
(In reply to Magnus Enger from comment #28) > It also looks like there is a missing "use Koha::Illrequests;" in > Koha/Illrequest.pm? > > When I try to use Koha::Illrequest in the Dancer-based NCIPServer I get this > error: > > [24015] error @1.447857> [hit #5]request to POST / crashed: Can't locate > object method "search" via package "Koha::Illrequests" (perhaps you forgot > to load "Koha::Illrequests"?) at /path/to/kohaclone/Koha/Illrequest.pm line > 684. in /usr/share/perl5/Dancer/Handler.pm l. 98 Hi, Thanks for pointing that out. I have now added a patch fixing this. I have also rebased and pushed up a few more patches we've been working on. Alex
The patches do apply cleanly, so thanks for that! There is however a problem with userflags. The patches have this: installer/data/mysql/atomicupdate/ill_tables.sql +INSERT INTO userflags (bit,flag,flagdesc,defaulton) +VALUES (21,'ill','The Interlibrary Loans Module',0); But the User Clubs feature have "stolen" number 21. This *is* reflected in installer/data/mysql/userflags.sql: +(22,'ill','The Interlibrary Loans Module',0)
Created attachment 63113 [details] [review] Bug 7317: Followup: Update atomicupdate file. * installer/data/mysql/atomicupdate/ill_tables.sql: Set ILL permission id to 22.
(In reply to Magnus Enger from comment #40) > The patches do apply cleanly, so thanks for that! There is however a problem > with userflags. The patches have this: > > installer/data/mysql/atomicupdate/ill_tables.sql > +INSERT INTO userflags (bit,flag,flagdesc,defaulton) > +VALUES (21,'ill','The Interlibrary Loans Module',0); > > But the User Clubs feature have "stolen" number 21. This *is* reflected in > installer/data/mysql/userflags.sql: > > +(22,'ill','The Interlibrary Loans Module',0) You are quite right! Thanks for pointing that out — that should now be fixed. Alex
Created attachment 63175 [details] [review] Bug 7317: Add `USE Koha` to intranet-main.tt. * koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt: Add `USE Koha`. This is to be able to display link on ILLModule being enabled. Through use cases we have found that permissions to access staff ILL is very cumbersome. At some point we should add a per borrower category permission, rather than a per user permission.
Created attachment 63176 [details] [review] Bug 7317: Add branch details to generic emails. * Koha/Illrequest.pm (generic_confirm): Add a branch details block at end of generic email template.
Hi Daphne (In reply to Daphne Hoolahan from comment #25) > I will also add a block with library details to the generic email template. > Probably just after the "Kind regards". What do you think? > > That's an excellent idea. I've now added logic for adding this kind of branch details block. Please feel free to test and let me know what you think! Alex
Created attachment 63354 [details] [review] Bug 7317: Invoke `handle_commit_maybe` on custom capabilities. * ill/ill-requests.pl: Invoke `handle_commit_maybe` on custom capabilities in our controller to handle 'commit' stage.
(In reply to Alex Sassmannshausen from comment #46) > Created attachment 63354 [details] [review] [review] > Bug 7317: Invoke `handle_commit_maybe` on custom capabilities. > > * ill/ill-requests.pl: Invoke `handle_commit_maybe` on custom > capabilities in our controller to handle 'commit' stage. That fixes the problem of returning to detail view after executing the relevant code! Thanks for a quick fix.
Created attachment 63421 [details] [review] Bug 7317: Bugfix: Ensure current branch exists. * Koha/Illrequest.pm (generic_confirm): Load $branch variable earlier, with 'current_branchcode'.
+sub available_backends { + my ( $self ) = @_; + my $backend_dir = $self->_config->backend_dir; + my @backends = (); + my @backends = <$backend_dir/*> if ( $backend_dir ); + my @backends = map { basename($_) } @backends; + return \@backends; +} This gives a warning: "my" variable @backends masks earlier declaration in same scope at /path/to/Koha/Illrequest.pm line 375.
Created attachment 63472 [details] [review] Bug 7317: Bugfix: remove `my` statements. * Koha/Illrequest.pm (available_backends): Remove incorrect `my` statements for @backends variable.
(In reply to Magnus Enger from comment #49) > +sub available_backends { > + my ( $self ) = @_; > + my $backend_dir = $self->_config->backend_dir; > + my @backends = (); > + my @backends = <$backend_dir/*> if ( $backend_dir ); > + my @backends = map { basename($_) } @backends; > + return \@backends; > +} > > This gives a warning: "my" variable @backends masks earlier declaration in > same scope at /path/to/Koha/Illrequest.pm line 375. Thanks for the report, should be fixed with the latest patch! Alex
Created attachment 63475 [details] [review] Bug 7317: Bugfix: remove `my` statements & dereference. * Koha/Illrequest.pm (available_backends): Remove incorrect `my` statements for @backends variable. (custom_capability): Explicitly dereference `$self->capabilities`.
Comment on attachment 63084 [details] [review] Bug 7317: ILLModule Review of attachment 63084 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt @@ +156,5 @@ > + //]]> > +</script> > +</head> > + > +<body id="acq_suggestion" class="acq"> The id on the body tag should probably not be acq_suggestion?
HI I have tried to apply the patch on master, but I get an error Applying: Bug 7317: DO NOT PUSH: Update OPAC CSS Using index info to reconstruct a base tree... M koha-tmpl/opac-tmpl/bootstrap/css/opac.css Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/opac-tmpl/bootstrap/css/opac.css CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/css/opac.css Failed to merge in the changes. Patch failed at 0001 Bug 7317: DO NOT PUSH: Update OPAC CSS The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-7317-DO-NOT-PUSH-Update-OPAC-CSS-xvmzXj.patch
Iam ready to sign off on this, when Alex has provided the missing tests (and possibly a rebase).
Created attachment 65469 [details] [review] Bug 7317: Bugfix: remove `my` statements & dereference. * Koha/Illrequest.pm (available_backends): Remove incorrect `my` statements for @backends variable. (custom_capability): Explicitly dereference `$self->capabilities`.
Created attachment 65470 [details] [review] Bug 7317: Switch to borrowername from borrowernumber
Created attachment 65471 [details] [review] Bug 7317: Add date requested to OPAC requests table. * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt: Add request.placed to requests output table.
Created attachment 65472 [details] [review] Bug 7317: Add ILLModuleCopyrightClearance syspref. * installer/data/mysql/atomicupdate/ill_tables.sql: Add default syspref for upgrade. * installer/data/mysql/sysprefs.sql: Add default syspref for new installs. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Define syspref in UI.
Created attachment 65473 [details] [review] Bug 7317: Add generic Copyright Clearance functionality. * Koha/Illrequest.pm (backend_create): Consider Copyright Clearance syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt: Display Copyright Clearance information.
Created attachment 65474 [details] [review] Bug 7317: Implement generic optional capability system. * Koha/Illrequest.pm (_backend_capability): New method.
Created attachment 65475 [details] [review] Bug 7317: Fix _limit_counter. * Koha/Illrequest.pm (_limit_counter): Update SQL logic.
Created attachment 65476 [details] [review] Bug 7317: Pass full ILL configuration along within config. * Koha/Illrequest/Config.pm (_load_configuration): Store full ILL configuration for export.
Created attachment 66266 [details] [review] Bug 7317: Only allow cancellation request when status is new. * Koha/Illrequest.pm (_core_status_graph): Update status graph. * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt: Only show button when status is NEW.
Created attachment 67242 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI.
Created attachment 67243 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files.
Created attachment 67244 [details] [review] Bug 7317: ILLModule * Koha/Illrequest.pm * Koha/Illrequest/Config.pm * Koha/Illrequestattribute.pm * Koha/Illrequestattributes.pm * Koha/Illrequests.pm * Koha/REST/V1/Illrequests.pm * ill/ill-requests.pl * api/v1/swagger/paths.json * api/v1/swagger/paths/illrequests.json * etc/koha-conf.xml * koha-tmpl/intranet-tmpl/prog/css/staff-global.css * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc * koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc * koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt * koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt * koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results-grouped.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt * koha-tmpl/opac-tmpl/bootstrap/less/opac.less * opac/opac-illrequests.pl * t/db_dependent/Illrequest.t * t/db_dependent/Illrequest/Config.t * t/db_dependent/Illrequest/Dummy.t * t/db_dependent/api/v1/illrequests.t
Created attachment 67245 [details] [review] Bug 7317: Bugfix: copy ill folder during installation. * Makefile.PL: Add './ill' to list of installable directories.
Created attachment 67246 [details] [review] Bug 7317: Allow journey to 'COMP' status. * Koha/Illrequest.pm (_core_status_graph): Add 'COMP' journey, and method. (mark_completed): new procedure. * ill/ill-requests.pl: Handle mark_completed.
Created attachment 67247 [details] [review] Bug 7317: Make error messages slightly more user friendly. * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt: Change layout of errors, don't display cancel heading on cancel error.
Created attachment 67248 [details] [review] Bug 7317: Allow display/hiding of arbitrary request columns Initially display a small selection of core fields, allow the user to progressively display additional metadata fields if required
Created attachment 67249 [details] [review] Bug 7317: Bugfix: Use correct column names in check_limits. * Koha/Illrequest.pm (check_limits): Change column names used to `branchcode` & `borrowernumber`.
Created attachment 67250 [details] [review] Bug 7317: Add missing reference for `Koha::Illrequest`. * Koha/Illrequest.pm: Add missing `use` statement.
Created attachment 67251 [details] [review] Bug 7317: Followup: Update atomicupdate file. * installer/data/mysql/atomicupdate/ill_tables.sql: Set ILL permission id to 22.
Created attachment 67252 [details] [review] Bug 7317: Add branch details to generic emails. * Koha/Illrequest.pm (generic_confirm): Add a branch details block at end of generic email template.
Created attachment 67253 [details] [review] Bug 7317: Invoke `handle_commit_maybe` on custom capabilities. * ill/ill-requests.pl: Invoke `handle_commit_maybe` on custom capabilities in our controller to handle 'commit' stage.
Created attachment 67254 [details] [review] Bug 7317: Bugfix: Ensure current branch exists. * Koha/Illrequest.pm (generic_confirm): Load $branch variable earlier, with 'current_branchcode'.
Created attachment 67255 [details] [review] Bug 7317: Bugfix: remove `my` statements & dereference. * Koha/Illrequest.pm (available_backends): Remove incorrect `my` statements for @backends variable. (custom_capability): Explicitly dereference `$self->capabilities`.
Created attachment 67256 [details] [review] Bug 7317: Switch to borrowername from borrowernumber
Created attachment 67257 [details] [review] Bug 7317: Add date requested to OPAC requests table. * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt: Add request.placed to requests output table.
Created attachment 67258 [details] [review] Bug 7317: Add ILLModuleCopyrightClearance syspref. * installer/data/mysql/atomicupdate/ill_tables.sql: Add default syspref for upgrade. * installer/data/mysql/sysprefs.sql: Add default syspref for new installs. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Define syspref in UI.
Created attachment 67259 [details] [review] Bug 7317: Add generic Copyright Clearance functionality. * Koha/Illrequest.pm (backend_create): Consider Copyright Clearance syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt: Display Copyright Clearance information.
Created attachment 67260 [details] [review] Bug 7317: Implement generic optional capability system. * Koha/Illrequest.pm (_backend_capability): New method.
Created attachment 67261 [details] [review] Bug 7317: Fix _limit_counter. * Koha/Illrequest.pm (_limit_counter): Update SQL logic.
Created attachment 67262 [details] [review] Bug 7317: Pass full ILL configuration along within config. * Koha/Illrequest/Config.pm (_load_configuration): Store full ILL configuration for export.
Created attachment 67263 [details] [review] Bug 7317: Only allow cancellation request when status is new. * Koha/Illrequest.pm (_core_status_graph): Update status graph. * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt: Only show button when status is NEW.
Created attachment 67264 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS
Easy conflict fixed! If you want this into 17.11, be involved in the signoff and QA process.
Created attachment 67281 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67282 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67283 [details] [review] Bug 7317: ILLModule * Koha/Illrequest.pm * Koha/Illrequest/Config.pm * Koha/Illrequestattribute.pm * Koha/Illrequestattributes.pm * Koha/Illrequests.pm * Koha/REST/V1/Illrequests.pm * ill/ill-requests.pl * api/v1/swagger/paths.json * api/v1/swagger/paths/illrequests.json * etc/koha-conf.xml * koha-tmpl/intranet-tmpl/prog/css/staff-global.css * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc * koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc * koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt * koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt * koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results-grouped.tt * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt * koha-tmpl/opac-tmpl/bootstrap/less/opac.less * opac/opac-illrequests.pl * t/db_dependent/Illrequest.t * t/db_dependent/Illrequest/Config.t * t/db_dependent/Illrequest/Dummy.t * t/db_dependent/api/v1/illrequests.t Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67284 [details] [review] Bug 7317: Bugfix: copy ill folder during installation. * Makefile.PL: Add './ill' to list of installable directories. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67285 [details] [review] Bug 7317: Allow journey to 'COMP' status. * Koha/Illrequest.pm (_core_status_graph): Add 'COMP' journey, and method. (mark_completed): new procedure. * ill/ill-requests.pl: Handle mark_completed. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67286 [details] [review] Bug 7317: Make error messages slightly more user friendly. * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt: Change layout of errors, don't display cancel heading on cancel error. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67287 [details] [review] Bug 7317: Allow display/hiding of arbitrary request columns Initially display a small selection of core fields, allow the user to progressively display additional metadata fields if required Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67288 [details] [review] Bug 7317: Bugfix: Use correct column names in check_limits. * Koha/Illrequest.pm (check_limits): Change column names used to `branchcode` & `borrowernumber`. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67289 [details] [review] Bug 7317: Add missing reference for `Koha::Illrequest`. * Koha/Illrequest.pm: Add missing `use` statement. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67290 [details] [review] Bug 7317: Followup: Update atomicupdate file. * installer/data/mysql/atomicupdate/ill_tables.sql: Set ILL permission id to 22. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67291 [details] [review] Bug 7317: Add branch details to generic emails. * Koha/Illrequest.pm (generic_confirm): Add a branch details block at end of generic email template. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67292 [details] [review] Bug 7317: Invoke `handle_commit_maybe` on custom capabilities. * ill/ill-requests.pl: Invoke `handle_commit_maybe` on custom capabilities in our controller to handle 'commit' stage. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67293 [details] [review] Bug 7317: Bugfix: Ensure current branch exists. * Koha/Illrequest.pm (generic_confirm): Load $branch variable earlier, with 'current_branchcode'. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67294 [details] [review] Bug 7317: Bugfix: remove `my` statements & dereference. * Koha/Illrequest.pm (available_backends): Remove incorrect `my` statements for @backends variable. (custom_capability): Explicitly dereference `$self->capabilities`. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67295 [details] [review] Bug 7317: Switch to borrowername from borrowernumber Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67296 [details] [review] Bug 7317: Add date requested to OPAC requests table. * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt: Add request.placed to requests output table. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67297 [details] [review] Bug 7317: Add ILLModuleCopyrightClearance syspref. * installer/data/mysql/atomicupdate/ill_tables.sql: Add default syspref for upgrade. * installer/data/mysql/sysprefs.sql: Add default syspref for new installs. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Define syspref in UI. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67298 [details] [review] Bug 7317: Add generic Copyright Clearance functionality. * Koha/Illrequest.pm (backend_create): Consider Copyright Clearance syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt: Display Copyright Clearance information. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67299 [details] [review] Bug 7317: Implement generic optional capability system. * Koha/Illrequest.pm (_backend_capability): New method. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67300 [details] [review] Bug 7317: Fix _limit_counter. * Koha/Illrequest.pm (_limit_counter): Update SQL logic. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67301 [details] [review] Bug 7317: Pass full ILL configuration along within config. * Koha/Illrequest/Config.pm (_load_configuration): Store full ILL configuration for export. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67302 [details] [review] Bug 7317: Only allow cancellation request when status is new. * Koha/Illrequest.pm (_core_status_graph): Update status graph. * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt: Only show button when status is NEW. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 67303 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS Signed-off-by: Magnus Enger <magnus@libriotech.no> It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. I think this is one of those things we need to get into Koha so we can start using it and find any remaining bugs. One non-functional thing that needs to be looked at are the tests.
Have tested on a live system now, with the NNCIPP backend module from Magnus, and after some fixes we can confirm the entire processof ordering from a remote library, receiving, lending out and returning the item has been successful. Have yet to test the opposite direction, but will have a second look on this module patch and hopefully sign off when we're done testing
I'm getting failures on all the unit tests : \
(In reply to Kyle M Hall from comment #113) > I'm getting failures on all the unit tests : \ Yeah, acording to Alex, those tests are mostly written for an older iteration of the code. It has been said that Alex will be working one day a week on ILL in the coming weeks, so I hope writing those tests will be his number one priority...
Created attachment 68185 [details] [review] Bug 7317: Remove obsolete files. The tests files will be replaced with t/db_dependent/Illrequests.t. Dummy backend files are now in their own repository. * Koha/Illrequest/Backend/Dummy/Base.pm: Delete. * t/db_dependent/Illrequest.t: Delete. * t/db_dependent/Illrequest/Dummy.t: Delete.
Created attachment 68186 [details] [review] Bug 7317: Add unit tests & fix, add comments to Illrequest.pm. * Koha/Illrequest.pm: Add code commentary. (_backend_capability): We need to invoke our code ref. * t/db_dependent/Illrequest/Config.t: Improve tests. * t/db_dependent/Illrequestattributes.t: Improve tests. * t/db_dependent/Illrequests.t: Improve tests.
I added a page to the wiki where we can document backends that have been/are being implemented: https://wiki.koha-community.org/wiki/ILL_backends
Created attachment 68328 [details] [review] Bug 7317: Add unit tests & fix, add comments to Illrequest.pm. * Koha/Illrequest.pm: Add code commentary. (_backend_capability): We need to invoke our code ref. * Koha/Illrequest/Config.pm: Correct reference to potentially uninstantiated $reply_date variable. * t/db_dependent/Illrequest/Config.t: Improve tests. * t/db_dependent/Illrequestattributes.t: Improve tests. * t/db_dependent/Illrequests.t: Improve tests.
Created attachment 68391 [details] [review] Bug 7317: Add unit tests & fix, add comments to Illrequest.pm. * Koha/Illrequest.pm: Add code commentary. (_backend_capability): We need to invoke our code ref. * Koha/Illrequest/Config.pm: Correct reference to potentially uninstantiated $reply_date variable. Minor improvements as result of new tests. * t/db_dependent/Illrequest/Config.t: Improve tests. * t/db_dependent/Illrequestattributes.t: Improve tests. * t/db_dependent/Illrequests.t: Improve tests.
Created attachment 68406 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 68407 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 68408 [details] [review] Bug 7317: Interlibrary loans framework for Koha. This Commit is at the heart of adding an interlibrary loans framework for Koha. The framework does not prescribe a particular workflow. Instead it provides a general framework that can be extended & implemented by individual backends whose responsibility it is to implement a specific workflow. The module is largely self-sufficient: it adds new tables to the Koha database and touches only a few files in the Koha source tree. Primarily, we add our files to the Makefile and the koha-conf.xml, define ill paths for the REST API, and introduce links from the main intranet, opac pages & user permissions. Outside of this we simply add new files & functionality.
Created attachment 68409 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS Signed-off-by: Magnus Enger <magnus@libriotech.no> It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. I think this is one of those things we need to get into Koha so we can start using it and find any remaining bugs. One non-functional thing that needs to be looked at are the tests.
Created attachment 68418 [details] ILL overview & light documentation Please see attached text file for an initial attempt at user-friendly documentation of the module. It should give an overview of this module's impact on Koha itself, of the architecture of the module, and of the notion of 'backends'. Cheers, Alex
Setting this to "Needs signoff" now, as everything should be in place. Benjamin and I will have a go at this tomorrow, if noone beats us to it. :-)
Created attachment 68444 [details] [review] Bug 7317: (QA folowup) 404 if module is disabled (intranet) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68445 [details] [review] Bug 7317: (followup) Migrate endpoint to OpenAPI This patch moves the current endpoint implementation from Swagger2 to the OpenAPI plugin. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68483 [details] [review] Bug 7317: (QA folowup) 404 if module is disabled (intranet) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68484 [details] [review] Bug 7317: Overload Koha::Illrequest::TO_JSON This patch implements an overloaded TO_JSON method, that introduces the option to (selectively) embed information required on the REST api. Tests are included for the new method. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68485 [details] [review] Bug 7317: (followup) Migrate endpoint to OpenAPI This patch moves the current endpoint implementation from Swagger2 to the OpenAPI plugin. It also takes advantage of the overloaded Koha::Illrequest::TO_JSON method which has now the option to embed what's needed for the REST api. The path spec is adjusted to fit OpenAPI, and some minor fixes are applied: - Missing 'metadata' query param - 'ill' permissions should be required instead of 'borrowers' - Full test coverage To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68487 [details] [review] Bug 7317: Adjust packages koha-conf.xml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68516 [details] [review] Bug 7317: Adjust packages koha-conf.xml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Original sign off message: It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. Update: I have applied the new patchset and done some exploratory testing, using the Dummy backend. Everything seems to be fine after the patches were re-arranged and with the followups from Tomas. All tests pass.
Created attachment 68517 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 68518 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files.
Created attachment 68519 [details] [review] Bug 7317: Interlibrary loans framework for Koha. This Commit is at the heart of adding an interlibrary loans framework for Koha. The framework does not prescribe a particular workflow. Instead it provides a general framework that can be extended & implemented by individual backends whose responsibility it is to implement a specific workflow. The module is largely self-sufficient: it adds new tables to the Koha database and touches only a few files in the Koha source tree. Primarily, we add our files to the Makefile and the koha-conf.xml, define ill paths for the REST API, and introduce links from the main intranet, opac pages & user permissions. Outside of this we simply add new files & functionality. Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 68520 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS Signed-off-by: Magnus Enger <magnus@libriotech.no> It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. I think this is one of those things we need to get into Koha so we can start using it and find any remaining bugs. One non-functional thing that needs to be looked at are the tests.
Created attachment 68521 [details] [review] Bug 7317: (QA folowup) 404 if module is disabled (intranet) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 68522 [details] [review] Bug 7317: Overload Koha::Illrequest::TO_JSON This patch implements an overloaded TO_JSON method, that introduces the option to (selectively) embed information required on the REST api. Tests are included for the new method. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 68523 [details] [review] Bug 7317: (followup) Migrate endpoint to OpenAPI This patch moves the current endpoint implementation from Swagger2 to the OpenAPI plugin. It also takes advantage of the overloaded Koha::Illrequest::TO_JSON method which has now the option to embed what's needed for the REST api. The path spec is adjusted to fit OpenAPI, and some minor fixes are applied: - Missing 'metadata' query param - 'ill' permissions should be required instead of 'borrowers' - Full test coverage To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 68524 [details] [review] Bug 7317: Adjust packages koha-conf.xml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Original sign off message: It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. Update: I have applied the new patchset and done some exploratory testing, using the Dummy backend. Everything seems to be fine after the patches were re-arranged and with the followups from Tomas. All tests pass.
Created attachment 68525 [details] [review] Bug 7317: (followup) Fix id and class on body tag The HTML body tag looked like this: <body id="acq_suggestion" class="acq"> Probably an overlooked copypasta. This patch changes it to: <body id="illrequests" class="ill"> This should not have any visual side effects.
Created attachment 68542 [details] [review] Bug 7317: Rewrite atomicupdate file Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68543 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI. Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68544 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68545 [details] [review] Bug 7317: Interlibrary loans framework for Koha. This Commit is at the heart of adding an interlibrary loans framework for Koha. The framework does not prescribe a particular workflow. Instead it provides a general framework that can be extended & implemented by individual backends whose responsibility it is to implement a specific workflow. The module is largely self-sufficient: it adds new tables to the Koha database and touches only a few files in the Koha source tree. Primarily, we add our files to the Makefile and the koha-conf.xml, define ill paths for the REST API, and introduce links from the main intranet, opac pages & user permissions. Outside of this we simply add new files & functionality. Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68546 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS Signed-off-by: Magnus Enger <magnus@libriotech.no> It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. I think this is one of those things we need to get into Koha so we can start using it and find any remaining bugs. One non-functional thing that needs to be looked at are the tests. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68547 [details] [review] Bug 7317: (QA folowup) 404 if module is disabled (intranet) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68548 [details] [review] Bug 7317: Overload Koha::Illrequest::TO_JSON This patch implements an overloaded TO_JSON method, that introduces the option to (selectively) embed information required on the REST api. Tests are included for the new method. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68549 [details] [review] Bug 7317: (followup) Migrate endpoint to OpenAPI This patch moves the current endpoint implementation from Swagger2 to the OpenAPI plugin. It also takes advantage of the overloaded Koha::Illrequest::TO_JSON method which has now the option to embed what's needed for the REST api. The path spec is adjusted to fit OpenAPI, and some minor fixes are applied: - Missing 'metadata' query param - 'ill' permissions should be required instead of 'borrowers' - Full test coverage To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68550 [details] [review] Bug 7317: Adjust packages koha-conf.xml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Original sign off message: It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. Update: I have applied the new patchset and done some exploratory testing, using the Dummy backend. Everything seems to be fine after the patches were re-arranged and with the followups from Tomas. All tests pass. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68551 [details] [review] Bug 7317: (followup) Fix id and class on body tag The HTML body tag looked like this: <body id="acq_suggestion" class="acq"> Probably an overlooked copypasta. This patch changes it to: <body id="illrequests" class="ill"> This should not have any visual side effects. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68552 [details] [review] Bug 7317: Rewrite atomicupdate file Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68558 [details] [review] Bug 7317: Add SQL templates & sysprefs. * installer/data/mysql/atomicupdate/ill_tables.sql: New file. * installer/data/mysql/kohastructure.sql: Add tables. * installer/data/mysql/sysprefs.sql: Add sysprefs. * installer/data/mysql/userflags.sql: Add userflags. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/admin.pref: Add sysprefs to UI. Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68559 [details] [review] Bug 7317: DO NOT PUSH: Updated Schema Files. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68560 [details] [review] Bug 7317: Interlibrary loans framework for Koha. This Commit is at the heart of adding an interlibrary loans framework for Koha. The framework does not prescribe a particular workflow. Instead it provides a general framework that can be extended & implemented by individual backends whose responsibility it is to implement a specific workflow. The module is largely self-sufficient: it adds new tables to the Koha database and touches only a few files in the Koha source tree. Primarily, we add our files to the Makefile and the koha-conf.xml, define ill paths for the REST API, and introduce links from the main intranet, opac pages & user permissions. Outside of this we simply add new files & functionality. Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68561 [details] [review] Bug 7317: DO NOT PUSH: Update OPAC CSS Signed-off-by: Magnus Enger <magnus@libriotech.no> It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. I think this is one of those things we need to get into Koha so we can start using it and find any remaining bugs. One non-functional thing that needs to be looked at are the tests. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68562 [details] [review] Bug 7317: (QA folowup) 404 if module is disabled (intranet) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68563 [details] [review] Bug 7317: Overload Koha::Illrequest::TO_JSON This patch implements an overloaded TO_JSON method, that introduces the option to (selectively) embed information required on the REST api. Tests are included for the new method. To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/Illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68564 [details] [review] Bug 7317: (followup) Migrate endpoint to OpenAPI This patch moves the current endpoint implementation from Swagger2 to the OpenAPI plugin. It also takes advantage of the overloaded Koha::Illrequest::TO_JSON method which has now the option to embed what's needed for the REST api. The path spec is adjusted to fit OpenAPI, and some minor fixes are applied: - Missing 'metadata' query param - 'ill' permissions should be required instead of 'borrowers' - Full test coverage To test: - Apply this patch - Run: $ kshell k$ prove t/db_dependent/api/v1/illrequests.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68565 [details] [review] Bug 7317: Adjust packages koha-conf.xml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Magnus Enger <magnus@libriotech.no> Original sign off message: It's impossible to give a test plan for this... I have implemented a "plugin" for Norwegian ILL (based on NCIP): https://github.com/Libriotech/koha-illbackend-nncipp Several issues have been discovered while writing the plugin, and Alex has fixed them along the way. Currently I do not know about any funcional issues, and I am happy to sign off. Update: I have applied the new patchset and done some exploratory testing, using the Dummy backend. Everything seems to be fine after the patches were re-arranged and with the followups from Tomas. All tests pass. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68566 [details] [review] Bug 7317: (followup) Fix id and class on body tag The HTML body tag looked like this: <body id="acq_suggestion" class="acq"> Probably an overlooked copypasta. This patch changes it to: <body id="illrequests" class="ill"> This should not have any visual side effects. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Created attachment 68567 [details] [review] Bug 7317: Rewrite atomicupdate file Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Please fix QA script failures.
1/ +TODO: + +- Anything invoking the ->status method; annotated with: + + # Old use of ->status ! What do it mean? 2/ I will not make you rewrite everything, but it would have been better to stay consistent and not mix snake_case and camelCase for method names. First steps with the GUI: 3/ This is what I see when I enable the module: https://screenshots.firefox.com/z9dNoRL91NQ5F3cD/pro.kohadev.org Looks wrong. 4/ Hit /cgi-bin/koha/ill/ill-requests.pl and got: Loading failed for the <script> with source “http://pro.kohadev.org/intranet-tmpl/prog/en/lib/jquery/plugins/jquery.tablesorter.min.js”. ill-requests.pl:151 Loading failed for the <script> with source “http://pro.kohadev.org/intranet-tmpl/prog/en/lib/jquery/plugins/jquery.checkboxes.min.js”. ill-requests.pl:152 5/ Clicked "New ILL request" button and got: Can't locate Koha/Illbackends//Base.pm
Created attachment 68858 [details] [review] Bug 7317: QA followup This fixes some of the issues reported by the QA script, but not all.
Created attachment 68874 [details] [review] Bug 7317: Handle backend absense more gracefuly 5/ This patch is WIP, it makes Koha::Illrequest->load_backend raise an exception if the passed backend is invalid. This way we will catch more errors introduced. The patch also disables the 'New Ill request' when no backends are available. Gets rid of a related warning. TODO: Should add a noticeable warning on the UI, or a tooltip on the button. TODO: add a couple tests for the exception 4/ This patch fixes the path for the checkboxes jquery plugin, and removes the include for tablesorter, as this implementation uses Datatables. This is obviously code for older Koha, ported to master. 3/ I know it doesn't look right, but am not sure how to display. This is the usual empty datatable display we have everywhere in Koha. 2/ We could fix it with a proper IDE, I use Komodo and refactoring code is pretty straigh-forward. Worth a separate bug report. 1/ That TODO is misleading. It refers to their own migration from a different design. I checked this code and cannot find occurences of ->status with that annotation. I think this relates to backends code. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
As far as I can see, these are the remaining problems pointed to by the qa script, that can actually be fixed: FAIL Koha/Exceptions/Ill.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 OK spelling OK valid I think Tomas introduced this in a followup. He can probably add a little bit of POD to it. FAIL Koha/Illrequestattributes.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'object_class' OK spelling OK valid FAIL Koha/Illrequests.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'object_class' OK spelling OK valid These two files do have some subs that are missing POD. I can look at that again tomorrow. FAIL installer/data/mysql/atomicupdate/ill_tables.sql FAIL git manipulation The file has been added and deleted in the same patchset Not sure how we can fix this without redoing completely the patches from Alex? FAIL koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt OK forbidden patterns OK git manipulation OK spelling FAIL tt_valid lines 665 FAIL valid_template : not found The codes mixes TT directives and HTML rather intimately: 665 <table 666 [% FOREACH filter IN prefilters %] 667 data-filter-[% filter.name %]="[% filter.value %]" 668 [% END %] 669 id="ill-requests"> Not sure how we can do it differently, though?
Created attachment 68877 [details] [review] Bug 7317: QA followup This fixes some of the issues reported by the QA script, but not all.
Created attachment 68878 [details] [review] Bug 7317: Handle backend absense more gracefuly 5/ This patch is WIP, it makes Koha::Illrequest->load_backend raise an exception if the passed backend is invalid. This way we will catch more errors introduced. The patch also disables the 'New Ill request' when no backends are available. Gets rid of a related warning. TODO: Should add a noticeable warning on the UI, or a tooltip on the button. TODO: add a couple tests for the exception 4/ This patch fixes the path for the checkboxes jquery plugin, and removes the include for tablesorter, as this implementation uses Datatables. This is obviously code for older Koha, ported to master. 3/ I know it doesn't look right, but am not sure how to display. This is the usual empty datatable display we have everywhere in Koha. 2/ We could fix it with a proper IDE, I use Komodo and refactoring code is pretty straigh-forward. Worth a separate bug report. 1/ That TODO is misleading. It refers to their own migration from a different design. I checked this code and cannot find occurences of ->status with that annotation. I think this relates to backends code. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Trying to summarize the status of this bug now. *** QA script ** POD coverage Some FAILS are "POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666". It sounds like these can be ignored. ** Atomicupdate FAIL installer/data/mysql/atomicupdate/ill_tables.sql FAIL git manipulation The file has been added and deleted in the same patchset Alex provided ill_tables.sql. Tomas rewrote it as bug_7317_ill.perl. Not sure there is anything to fix here? ** tt_valid FAIL koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt OK forbidden patterns OK git manipulation OK spelling FAIL tt_valid lines 665 FAIL valid_template : not found TT is used to set some HTML attributes. Not sure if this can be fixed. Tomas? *** (In reply to Jonathan Druart from comment #164) > 1/ > +TODO: > + > +- Anything invoking the ->status method; annotated with: > + + # Old use of ->status ! > > What do it mean? OK, according to Tomas. > 2/ I will not make you rewrite everything, but it would have been better to > stay consistent and not mix snake_case and camelCase for method names. OK > First steps with the GUI: > 3/ This is what I see when I enable the module: > https://screenshots.firefox.com/z9dNoRL91NQ5F3cD/pro.kohadev.org > Looks wrong. Not sure if this can be fixed? > 4/ Hit /cgi-bin/koha/ill/ill-requests.pl and got: > Loading failed for the <script> with source > “http://pro.kohadev.org/intranet-tmpl/prog/en/lib/jquery/plugins/jquery. > tablesorter.min.js”. ill-requests.pl:151 > Loading failed for the <script> with source > “http://pro.kohadev.org/intranet-tmpl/prog/en/lib/jquery/plugins/jquery. > checkboxes.min.js”. ill-requests.pl:152 OK. Fixed by Tomas > 5/ Clicked "New ILL request" button and got: > Can't locate Koha/Illbackends//Base.pm OK. Fixed by Tomas. But it sounds like Tomas will be working a bit more on related things.
Created attachment 68885 [details] [review] Bug 7317: Handle backend absense more gracefuly 5/ This patch is WIP, it makes Koha::Illrequest->load_backend raise an exception if the passed backend is invalid. This way we will catch more errors introduced. The patch also disables the 'New Ill request' when no backends are available. Gets rid of a related warning. TODO: Should add a noticeable warning on the UI, or a tooltip on the button. TODO: add a couple tests for the exception 4/ This patch fixes the path for the checkboxes jquery plugin, and removes the include for tablesorter, as this implementation uses Datatables. This is obviously code for older Koha, ported to master. 3/ I know it doesn't look right, but am not sure how to display. This is the usual empty datatable display we have everywhere in Koha. 2/ We could fix it with a proper IDE, I use Komodo and refactoring code is pretty straigh-forward. Worth a separate bug report. 1/ That TODO is misleading. It refers to their own migration from a different design. I checked this code and cannot find occurences of ->status with that annotation. I think this relates to backends code. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68886 [details] [review] Bug 7317: (QA followup) Get rid of warnings from the tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68921 [details] [review] Bug 7317: Handle backend absense more gracefuly 5/ This patch makes Koha::Illrequest->load_backend raise an exception if the passed backend is invalid. This way we will catch more errors introduced. The patch also disables the 'New Ill request' when no backends are available. Gets rid of a related warnings. Both OPAC and Intranet now display a warning message when no backends are available. Tests are added for the load_backend changes. 4/ This patch fixes the path for the checkboxes jquery plugin, and removes the include for tablesorter, as this implementation uses Datatables. This is obviously code for older Koha, ported to master. TODO: There's something wrong on the styling. My idea is to get rid of the custom column visualization tool, and have it display as regular DataTables. We can then introduce the use of colvis on a separate bug report. Note: POD coverage for the exceptions file is wrongly tested. It is a false positive. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68922 [details] [review] Bug 7317: (QA followup) Get rid of warnings from the tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68926 [details] [review] Bug 7317: opac.css update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68927 [details] [review] Bug 7317: Handle backend absense more gracefuly 5/ This patch makes Koha::Illrequest->load_backend raise an exception if the passed backend is invalid. This way we will catch more errors introduced. The patch also disables the 'New Ill request' when no backends are available. Gets rid of a related warnings. Both OPAC and Intranet now display a warning message when no backends are available. Tests are added for the load_backend changes. 4/ This patch fixes the path for the checkboxes jquery plugin, and removes the include for tablesorter, as this implementation uses Datatables. This is obviously code for older Koha, ported to master. TODO: There's something wrong on the styling. My idea is to get rid of the custom column visualization tool, and have it display as regular DataTables. We can then introduce the use of colvis on a separate bug report. Note: POD coverage for the exceptions file is wrongly tested. It is a false positive. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68928 [details] [review] Bug 7317: (QA followup) Get rid of warnings from the tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68929 [details] [review] Bug 7317: opac.css update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68931 [details] [review] Bug 7317: Handle backend absense more gracefuly 5/ This patch makes Koha::Illrequest->load_backend raise an exception if the passed backend is invalid. This way we will catch more errors introduced. The patch also disables the 'New Ill request' when no backends are available. Gets rid of a related warnings. Both OPAC and Intranet now display a warning message when no backends are available. Tests are added for the load_backend changes. 4/ This patch fixes the path for the checkboxes jquery plugin, and removes the include for tablesorter, as this implementation uses Datatables. This is obviously code for older Koha, ported to master. TODO: There's something wrong on the styling. My idea is to get rid of the custom column visualization tool, and have it display as regular DataTables. We can then introduce the use of colvis on a separate bug report. Note: POD coverage for the exceptions file is wrongly tested. It is a false positive. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68932 [details] [review] Bug 7317: (QA followup) Get rid of warnings from the tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68933 [details] [review] Bug 7317: opac.css update Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
1/ Error handling on borrowernumber when creating or updating a request (backend issue?) 2/ Capitalization problems, deprecated words (Branch, borrowers), etc 3/ Use dateformat to display dates 4/ ill/ill-requests.pl?method=generic_confirm&illrequest_id=3 click "send email" Software error to handle correctly: No target email addresses found. Either select at least one partner or check your ILL partner library records. at /home/vagrant/kohaclone/Koha/Illrequest.pm line 896. 5/ Same for "Your branch has no email address. Please set it. at /home/vagrant/kohaclone/Koha/Illrequest.pm line 901." 6/ Should not we use message_queue instead of sendmail directly? 7/ Maybe we should add a warning to the about page if the partner_code defined in the config does not match a category_code (?) (not finished)
Created attachment 68988 [details] [review] Bug 7317: Add partner_code checks to abot page This patch adds checks to the configured (or fallback) partner_code. It produces two different warnings: - Configured partner_code doesn't match an existing patron category code. - partner_code is not defined, a fallback to ILLLIBS is done Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 68989 [details] [review] Bug 7317: (followup) Make tests independent of data on the DB Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Jonathan Druart from comment #182) > 1/ Error handling on borrowernumber when creating or updating a request > (backend issue?) This is definitely a backend issue. The Dummy backend does no check, it just passes the borrowernumber from the UI to the Koha::Illrequest->store method with no error catching. The FreeForm plugin does some checks before the ->store, though. Out of the scope of the bug anyway. > 6/ Should not we use message_queue instead of sendmail directly? There's a bug filed for this: bug 18588. > 7/ Maybe we should add a warning to the about page if the partner_code > defined in the config does not match a category_code (?) Done in one of the recent followups.
Created attachment 68999 [details] [review] Bug 7317: (QA followup) ILL sysprefs belong to circulation section Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69000 [details] [review] Bug 7317: Handle missing email addresses gracefuly Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69006 [details] [review] Bug 7317: (QA followup) Fix capitalization and terminology issues This patch fixed terminology issues on the UI, and removes CSS-based case forcing for column names. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69008 [details] [review] Bug 7317: Add missing 'use Encode' statement
Created attachment 69009 [details] [review] Bug 7317: (QA followup 2) Fix capitalization and terminology issues
Created attachment 69012 [details] [review] Bug 7317: use preventDefault to prevent the scrollbar to move to the top
Created attachment 69014 [details] [review] Bug 7317: Fix more translation issues
Some more: 1. This looks wrong 260 here_link => "/cgi-bin/koha/ill/ill-requests.pl" url should be in template I'd say 2. This is not used, you are using an ajax call 219 } elsif ( $op eq 'illlist') { 220 # Display all current ILLs 221 my $requests = $illRequests->search(); 222 223 $template->param( 224 requests => $requests 225 ); 3. 257 media => [ "Book", "Article", "Journal" ], Is this used? 4. Config.pm 312 die "No DEFAULT_FORMATS has been defined in koha-conf.xml, but UNMEDIATEDILL is active." 313 if ( $unmediated && !$configuration->{default_formats}->{default} ); Not used yet, please remove 5. 794 \"YEAR(placed) = YEAR(NOW())" This looks like a mysqlism 6. The branchcode is displayed instead of the branchname in the list of ILL requests. Sounds easy to fix as we embed "branch" (must be library?) 7. api/v1/swagger/paths/*.json "name": "branchcode" vs "name": "branch_id" same with borrowernumber vs borrower_id and biblionumber vs biblio_id (to be continued...)
Comment on attachment 69009 [details] [review] Bug 7317: (QA followup 2) Fix capitalization and terminology issues This patch doesn't apply because the attached patches already fix this. Not sure how it happened.
(In reply to Jonathan Druart from comment #193) > Some more: > > 1. This looks wrong > 260 here_link => "/cgi-bin/koha/ill/ill-requests.pl" > > url should be in template I'd say The path shouldn't need to be added manually, I'd say any option is wrong, fixed anyway in an upcoming patch. > 2. This is not used, you are using an ajax call > 219 } elsif ( $op eq 'illlist') { > 220 # Display all current ILLs > > 221 my $requests = $illRequests->search(); > 222 > 223 $template->param( > > 224 requests => $requests > > 225 ); This is not used and is removed on an upcoming patch. > 3. 257 media => [ "Book", "Article", "Journal" ], > Is this used? This is used as a fallback in case the backends don't implement their own list. Needs to be kept (e.g. for the Dummy backend to work). > 4. > Config.pm > 312 die "No DEFAULT_FORMATS has been defined in koha-conf.xml, but > UNMEDIATEDILL is active." > 313 if ( $unmediated && > !$configuration->{default_formats}->{default} ); > > Not used yet, please remove Yup, removed in an upcoming patch. > 5. 794 \"YEAR(placed) = YEAR(NOW())" > This looks like a mysqlism True. But I cannot find any option to specify the same behaviour in SQL::Abstract or the DBIC docs. The very same line of code can be found on the DBIC docs as an example. > 6. The branchcode is displayed instead of the branchname in the list of ILL > requests. Sounds easy to fix as we embed "branch" (must be library?) Fixed in an upcoming patch. > 7. api/v1/swagger/paths/*.json > "name": "branchcode" > vs > "name": "branch_id" > same with > borrowernumber vs borrower_id > and > biblionumber vs biblio_id I can work on this tomorrow. Today's been enough!
Created attachment 69044 [details] [review] Bug 7317: (QA followup) Rename 'branch' for 'library' Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69045 [details] [review] Bug 7317: (followup) Remove extra columns from backends on the main list This patch removes the extra columns functionality, simplifying the code. It removes redundant code (in both Perl and JS), unused vars. It removes the use of here_link and hardcodes the script path on the template. It also adjusts the AJAX call so it uses the 'library' param instead of 'branch'. The library column now displays the library name instead of the ID. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69046 [details] [review] Bug 7317: Remove traces of UnmediatedILL This patch removes references to UnmediatedILL. This feature is introduced on a separate bug. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69056 [details] [review] Bug 7317: (QA followup) Make query parameters consistent with other endpoints Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69059 [details] [review] Bug 7317: (QA followup) Make query parameters consistent with other endpoints Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69060 [details] [review] Bug 7317: Fix infinite loop in OPAC Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69061 [details] [review] Bug 7317: (QA followup) Remove MySQL-ism from Koha::Illrequest::_limit_counter This trivial fix changes the query syntax to avoid MySQL-isms. To test: - Run: $ kshell k$ prove t/db_dependent/Illrequests.t => SUCCESS: Tests pass! - Apply this patch - Run: k$ prove t/db_dependent/Illrequests.t => SUCCESS: Tests still pass! Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 69063 [details] [review] Bug 7317: Help translators removing TT tags from po files
Created attachment 69064 [details] [review] Bug 7317: Help translators removing TT tags from po files
Created attachment 69065 [details] [review] Bug 7317: Simplify column headers It will help translators (remove TT tags from po files) and simplify the code. We do not need this translate_column that is used only once. It also removes sorting option on the last column (actions)
Created attachment 69066 [details] [review] Bug 7317: Do not use GET for deletion requests
Created attachment 69069 [details] [review] Bug 7317: Make fields mandatory when sending emails
Created attachment 69070 [details] [review] Bug 7317: Display all the libraries, not only one Before this patch only the first library defined in the system where displayed. TODO: Make sure we want to display all the libraries and not a "filtered" list see `git grep PROCESS options_for_libraries libraries|grep unfiltered`
Created attachment 69071 [details] [review] Bug 7317: Display all the libraries, not only one Before this patch only the first library defined in the system where displayed. TODO: Make sure we want to display all the libraries and not a "filtered" list see `git grep PROCESS options_for_libraries libraries|grep unfiltered`
Pushed to master for 17.11, thanks to everybody involved!
Created attachment 69074 [details] [review] Bug 7317: Fix translatable-templates.t failure Failed with /tmp/Lqv0BH2xC9:7979:13: invalid multibyte sequence msgmerge: found 1 fatal error
Is there any way to make the configuration of the ILL module more user friendly? I wish I didn't have to go in the conf file and on the server to configure it. Here are some suggestions - Tab in system preferences for all ILL configurations - Make StaffRequestComments (hide/show) a sys pref - Make ReplyDate (hide/show) a sys pref - Make DigitalRecipient (borrower/branch) a sys pref - Make PartnerCode (text entry, with by default ILLLIBS entered) a sys pref - In Tools or Administration, add a page with a form to add backends with our own providers So far, I've only been able to enable the ILLModule sys pref and create the database tables, so I don't really have any suggestions for the other parts... I will add them once I figure out the rest.
(In reply to Caroline Cyr La Rose from comment #212) > Is there any way to make the configuration of the ILL module more user > friendly? I wish I didn't have to go in the conf file and on the server to > configure it. > > Here are some suggestions > > - Tab in system preferences for all ILL configurations > - Make StaffRequestComments (hide/show) a sys pref > - Make ReplyDate (hide/show) a sys pref > - Make DigitalRecipient (borrower/branch) a sys pref > - Make PartnerCode (text entry, with by default ILLLIBS entered) a sys pref > - In Tools or Administration, add a page with a form to add backends with > our own providers > > So far, I've only been able to enable the ILLModule sys pref and create the > database tables, so I don't really have any suggestions for the other > parts... I will add them once I figure out the rest. Hi Caroline, please don't hesitate to file your enhancement ideas and suggestions as separate bugs and link them depending on this one.
(In reply to Caroline Cyr La Rose from comment #212) > - In Tools or Administration, add a page with a form to add backends with > our own providers There is bug 19605, which suggests making the ILL backends into regular "KPZ" plugins. I think Tomas will be working on that.
(In reply to Katrin Fischer from comment #213) > (In reply to Caroline Cyr La Rose from comment #212) > > Is there any way to make the configuration of the ILL module more user > > friendly? I wish I didn't have to go in the conf file and on the server to > > configure it. > > > > Here are some suggestions > > > > - Tab in system preferences for all ILL configurations > > - Make StaffRequestComments (hide/show) a sys pref > > - Make ReplyDate (hide/show) a sys pref > > - Make DigitalRecipient (borrower/branch) a sys pref > > - Make PartnerCode (text entry, with by default ILLLIBS entered) a sys pref > > - In Tools or Administration, add a page with a form to add backends with > > our own providers > > > > So far, I've only been able to enable the ILLModule sys pref and create the > > database tables, so I don't really have any suggestions for the other > > parts... I will add them once I figure out the rest. > > Hi Caroline, please don't hesitate to file your enhancement ideas and > suggestions as separate bugs and link them depending on this one. Done! Bug 19822
(In reply to Magnus Enger from comment #214) > (In reply to Caroline Cyr La Rose from comment #212) > > - In Tools or Administration, add a page with a form to add backends with > > our own providers > > There is bug 19605, which suggests making the ILL backends into regular > "KPZ" plugins. I think Tomas will be working on that. I'm not working on this in a short term, so anyone willing to pick that please do. Having ILLbackends be plugins will help removing the configuration from koha-conf.xml too.
*** Bug 9852 has been marked as a duplicate of this bug. ***
Comment on attachment 68560 [details] [review] Bug 7317: Interlibrary loans framework for Koha. Review of attachment 68560 [details] [review]: ----------------------------------------------------------------- ::: Koha/Illrequest.pm @@ +838,5 @@ > + } elsif ( 'draft' eq $params->{stage} ) { > + # Create the to header > + my $to = $params->{partners}; > + $to =~ s/^\x00//; # Strip leading NULLs > + $to =~ s/\x00/; /; # Replace others with '; ' Why would you need to replace a null byte with a semicolon (;)? It seems to me that email addresses should be separated by a comma?
(In reply to David Cook from comment #218) > Why would you need to replace a null byte with a semicolon (;)? > > It seems to me that email addresses should be separated by a comma? Actually, looking at https://metacpan.org/release/Mail-Sendmail/source/lib/Mail/Sendmail.pm#L334, it seems that Mail::Sendmail doesn't care about separators. It just iterates through a string looking for valid email addresses... yikes...