Bug 23112

Summary: Add circulation process to inter-library loans
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: ILLAssignee: Andrew Isherwood <bugzilla>
Status: CLOSED FIXED QA Contact:
Severity: new feature    
Priority: P5 - low CC: 1joynelson, cgdavis, cslone, jonathan.druart, jonathan.field, katrin.fischer, lisettepalouse+koha, magnus, martin.renvoize, niamh.walker-headon, nick
Version: master   
Hardware: All   
OS: All   
URL: https://wiki.koha-community.org/wiki/ILL_Circulation_RFC
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21468
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This new feature, when enabled, adds the option for library staff to immediately issue a received inter-library loan item to the patron who requested it. The checkout is immediate and due date is set, either as a fixed date entered by the librarian or based upon the standard circulation rules. **New system preference**: `CirculateILL` defaults to disabled.
Version(s) released in:
20.05.00
Bug Depends on: 7137, 23384    
Bug Blocks: 30575, 26114, 26115    
Attachments: Bug 23122: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23122: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23112: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues
Bug 23112: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues
Bug 23112: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues
Bug 23112: (follow-up) Address QA feedback
Bug 23112: (follow-up) Fix test as per feedback
Bug 23112: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues
Bug 23112: (follow-up) Address QA feedback
Bug 23112: (follow-up) Fix test as per feedback
Bug 23112: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues
Bug 23112: (follow-up) Address QA feedback
Bug 23112: (follow-up) Fix test as per feedback
Bug 23112: (QA follow-up) Fix database update, add filters, remove tabs
Bug 23051: (follow-up) Conditionally prevent checkout
Bug 23112: (follow-up) Conditionally prevent checkout
Bug 23112: (follow-up) Unit test fixes
Bug 23112: Add CirculateILL syspref
Bug 23112: Add circulation to ILL requests
Bug 23112: Add permission checks
Bug 23112: Update request status on return
Bug 23112: Add unit tests
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues
Bug 23112: (follow-up) Address QA feedback
Bug 23112: (follow-up) Fix test as per feedback
Bug 23112: (QA follow-up) Fix database update, add filters, remove tabs
Bug 23112: (follow-up) Conditionally prevent checkout
Bug 23112: (follow-up) Unit test fixes
Bug 23112: (QA follow-up) Clean up tests, catch warnings

Description Martin Renvoize 2019-06-13 07:51:52 UTC
The ILL framework we have supports the tracking of ILL requests well, but once the item has arrived at the library there isn't a clear workflow for lending it to the end user.

https://wiki.koha-community.org/wiki/ILL_Circulation_RFC
Comment 1 Lisette Scheer 2019-06-13 19:54:10 UTC
Martin, 
I think this is a great idea. 

In the Step: Places a hold on the item for the patron and marks it ready for collection, I'd like to recommend a preference if it marks it as available for pickup and also to make sure that if the person handling incoming ILLs isn't at the same branch as the patron, it would be put in transit to the correct library instead of ready for pickup. 

Lisette
Comment 2 Magnus Enger 2019-06-14 07:27:49 UTC
As I said on the RFC comments page, we have implemented a lot of this in the Swedish "Libris" backend. It would be great to move the functionality from the backend to the module! https://github.com/Libriotech/koha-illbackend-libris
Comment 3 Martin Renvoize 2019-06-14 07:38:12 UTC
(In reply to Lisette Scheer from comment #1)
> In the Step: Places a hold on the item for the patron and marks it ready for
> collection, I'd like to recommend a preference if it marks it as available
> for pickup.

Happy to add a preference, though I do wonder how the item is then meant to get to 'ready for collection'?

> and also to make sure that if the person handling incoming ILLs
> isn't at the same branch as the patron, it would be put in transit to the
> correct library instead of ready for pickup. 

Great idea, I hadn't thought about that requirement when we wrote this spec.. I'll add it to the spec now.
Comment 4 Martin Renvoize 2019-06-14 07:42:37 UTC
(In reply to Magnus Enger from comment #2)
> As I said on the RFC comments page, we have implemented a lot of this in the
> Swedish "Libris" backend. It would be great to move the functionality from
> the backend to the module!
> https://github.com/Libriotech/koha-illbackend-libris

Yeah, I think it quickly became a pattern in a few backends, make sense to move to core and prevent every backend having to impliment their own version.

I do however still like the idea of introducing hooks in a few places in circulation, could make for some interesting plugins and enable alterations to the generic flow if a particular backend needed to make such modifications.
Comment 5 Lisette Scheer 2019-06-14 16:02:01 UTC
(In reply to Martin Renvoize from comment #3)
> (In reply to Lisette Scheer from comment #1)
> > In the Step: Places a hold on the item for the patron and marks it ready for
> > collection, I'd like to recommend a preference if it marks it as available
> > for pickup.
> 
> Happy to add a preference, though I do wonder how the item is then meant to
> get to 'ready for collection'?
> 
If a hold is still placed for the patron, the hold could be captured by checking the item in, perhaps a place hold and marks it ready for collection button and a separate place hold button where they would have to scan it in again to mark the hold ready? That would solve both issues I brought up in my comment. 

Lisette
Comment 6 Martin Renvoize 2019-07-10 10:29:43 UTC
An alternative specification has been sent to the customer and agreed.
Comment 7 Katrin Fischer 2019-07-10 11:10:40 UTC
Can the alternative be shared? There seemed to be agreement on this one (see dicussion on wiki and comments here)
Comment 8 Martin Renvoize 2019-07-10 11:17:18 UTC
It's on the wiki
Comment 9 Andrew Isherwood 2019-08-05 13:02:08 UTC
Created attachment 91987 [details] [review]
Bug 23122: Add CirculateILL syspref
Comment 10 Andrew Isherwood 2019-08-05 13:02:12 UTC Comment hidden (obsolete)
Comment 11 Andrew Isherwood 2019-08-05 13:02:15 UTC Comment hidden (obsolete)
Comment 12 Andrew Isherwood 2019-08-05 13:02:18 UTC Comment hidden (obsolete)
Comment 13 Andrew Isherwood 2019-08-05 13:02:21 UTC Comment hidden (obsolete)
Comment 14 Andrew Isherwood 2019-08-05 13:05:23 UTC
This development has been carried out according to the originally stated requirements of the customer that sponsored it, detailed here: https://wiki.koha-community.org/wiki/ILL_Circulation_RFC

Test plan:

1. Ensure the FreeForm ILL backend is available
2. Enable the "CirculateILL" syspref
3. Ensure you have a statistical patron category defined (patron category type "Statistical")
4. Ensure you have at least one patron in your statistical patron category
5. Create a new FreeForm request (make a note of the library you select when creating it)
6. Mark the new request as confirmed by clicking the "Confirm request" button on the "Manage ILL request" page
7. TEST: Observe that a "Check out" button is now displayed in the request toolbar
8. Click the "Check out" button in the "Manage ILL request" page
9. In the "Issue requested item to..." screen:
  a. Do not select a statistical patron at this time
  b. You can at this point choose an item type, this will determine the type of the item that will be created for this request
  c. TEST: Observe that the default selected "Library" matches that that was defined when creating the request
  d. Do not select a due date at this time
10. Click "Submit"
11. TEST: Observe that the "Item checked out" screen displays, issued to the requesting patron with a due date corresponding to appropriate circ rules
12. Click "Return to request"
13. TEST: Observe that the request's status is now "Checked out"
14. Click the "Bibliographic record ID" link
15. TEST: Observe that the bibliographic record now has one item attached to it which is checked out
16. TEST: Observe that the item barcode is "ILL-" + the ILL request ID
17. Return to step 5., however, this time select a statistical patron and test that the item use is recorded and the item is not issued
18. Return to step 5., however, this time manually select a due date and test that the item's due date is set correctly on check out
19. Check in the item
20. TEST: Observe that the request's status is updated to "Returned to library"
21. Now implement a restriction on the patron (perhaps a fine) which would prevent them from checking out an item
22. Return to step 5. follow the instructions to step 10.
23. TEST: Observe that a banner is displayed at the top of the screen informing you that there was a problem checking the item out, containing a link to the patron's account page
24. Resolve the problem with the patron's account
25. Return to step 8.
26. TEST: Observe that the item is now successfully checked out
27. Disable the "CirculateILL" syspref
28. Return to step 5. at step 7. Observe that the "Check out" button is NOT displayed
Comment 15 Andrew Isherwood 2019-08-05 14:33:05 UTC Comment hidden (obsolete)
Comment 16 Andrew Isherwood 2019-08-05 14:33:08 UTC Comment hidden (obsolete)
Comment 17 Andrew Isherwood 2019-08-05 14:33:11 UTC Comment hidden (obsolete)
Comment 18 Andrew Isherwood 2019-08-05 14:33:15 UTC Comment hidden (obsolete)
Comment 19 Andrew Isherwood 2019-08-05 14:33:18 UTC Comment hidden (obsolete)
Comment 20 Magnus Enger 2019-10-03 08:40:54 UTC Comment hidden (obsolete)
Comment 21 Magnus Enger 2019-10-03 08:44:47 UTC Comment hidden (obsolete)
Comment 22 Andrew Isherwood 2019-10-03 08:48:33 UTC
Created attachment 93530 [details] [review]
Bug 23112: Add CirculateILL syspref

Sponsored-by: Loughborough University
Comment 23 Andrew Isherwood 2019-10-03 08:48:36 UTC
Created attachment 93531 [details] [review]
Bug 23112: Add circulation to ILL requests

This patch adds the ability to circulate ILL requests. Once a request has a suitable status, a "Check out" button is displayed on the "Manage request" toolbar. Clicking this will enable the user to check out the item either to the user who made the request or an in-house statistical  user. A due date can be specified, but if not circ rules are used.

Prior to the check out, an item is created which is attached to the biblio record that was created when the request was added

Sponsored-by: Loughborough University
Comment 24 Andrew Isherwood 2019-10-03 08:48:39 UTC
Created attachment 93532 [details] [review]
Bug 23112: Add permission checks

We only want the ILL circulation functionality to be available if the
appropriate syspref and perm are available, this patch adds those
checks.

Sponsored-by: Loughborough University
Comment 25 Andrew Isherwood 2019-10-03 08:48:42 UTC
Created attachment 93533 [details] [review]
Bug 23112: Update request status on return

This patch updates a request's status to RET upon return

Sponsored-by: Loughborough University
Comment 26 Andrew Isherwood 2019-10-03 08:48:46 UTC
Created attachment 93534 [details] [review]
Bug 23112: Add unit tests

This patch adds unit test for the new Koha::Illrequest::check_out method

Sponsored-by: Loughborough University
Comment 27 Andrew Isherwood 2019-10-03 08:49:43 UTC
Thanks for that Magnus, the bug number is now fixed, as it the failure to apply
Comment 28 Magnus Enger 2019-10-03 09:20:51 UTC
Thanks for fixing the number the applicability! 

Sorry, I get an error on step 5: "Create a new FreeForm request":

Template process failed: undef error - Undefined subroutine &Koha::Illrequest::Logger::GetAuthorisedValues called at /home/vagrant/kohaclone/Koha/Illrequest/Logger.pm line 213.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

This happens when I click on "New ILL request", fill in the form an click on "Create". It does not happen on current master. 

It does seem weird though, since the patches on the current bug do not seem to touch Koha/Illrequest/Logger.pm?
Comment 29 Magnus Enger 2019-10-03 09:25:42 UTC
Same thing happes when I go to "View ILL requests" an then click on "Manage request":

Template process failed: undef error - Undefined subroutine &Koha::Illrequest::Logger::GetAuthorisedValues called at /home/vagrant/kohaclone/Koha/Illrequest/Logger.pm line 213.
 at /home/vagrant/kohaclone/C4/Templates.pm line 122

Again, it does not happen on master without the current patches.
Comment 30 Andrew Isherwood 2019-10-09 11:16:54 UTC
Hi Magnus

I've just applied this bug on current master and cannot replicate either of these problems, I've just run through the whole test plan without a problem.

The error you're seeing is weird, it's not aware that GetAuthorisedValues should be coming from C4::Koha, it's trying to find it in Illrequest::Logger. I'm "use"ing C4::Koha, but not explicitly importing GetAuthorisedValues, which shouldn't be necessary since it's exported by C4::Koha. This is done all over the place in Koha, so you should be seeing this problem everywhere.

Explicitly importing GetAuthorisedValues would fix it, but that feels like a hack, I'd rather know why it's not being imported by default.

Any chance you could try again? It could I suppose have been a merge problem, I'd like to eliminate that.
Comment 31 Andrew Isherwood 2019-10-10 14:39:22 UTC
Hi Magnus

I just managed to replicate this on one of our test instances which is running plack, so I'm wondering if it's a plack thing. I've learnt the hard way not to try and understand why plack does what it does, so I'm just going to create a patch now which will just explicity import GetAuthorisedValues and hope that'll keep plack happy.
Comment 32 Andrew Isherwood 2019-10-10 14:43:41 UTC
Created attachment 93958 [details] [review]
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues

In response to the problem Magnus was experiencing in comment #28 & #29
we now call GetAuthorisedValues by it's full path
Comment 33 Andrew Isherwood 2019-10-16 09:03:58 UTC
Created attachment 94264 [details] [review]
Bug 23112: Add CirculateILL syspref

Sponsored-by: Loughborough University
Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Comment 34 Andrew Isherwood 2019-10-16 09:04:01 UTC
Created attachment 94265 [details] [review]
Bug 23112: Add circulation to ILL requests

This patch adds the ability to circulate ILL requests. Once a request has a suitable status, a "Check out" button is displayed on the "Manage request" toolbar. Clicking this will enable the user to check out the item either to the user who made the request or an in-house statistical  user. A due date can be specified, but if not circ rules are used.

Prior to the check out, an item is created which is attached to the biblio record that was created when the request was added

Sponsored-by: Loughborough University
Comment 35 Andrew Isherwood 2019-10-16 09:04:05 UTC
Created attachment 94266 [details] [review]
Bug 23112: Add permission checks

We only want the ILL circulation functionality to be available if the
appropriate syspref and perm are available, this patch adds those
checks.

Sponsored-by: Loughborough University
Comment 36 Andrew Isherwood 2019-10-16 09:04:08 UTC
Created attachment 94267 [details] [review]
Bug 23112: Update request status on return

This patch updates a request's status to RET upon return

Sponsored-by: Loughborough University
Comment 37 Andrew Isherwood 2019-10-16 09:04:13 UTC
Created attachment 94268 [details] [review]
Bug 23112: Add unit tests

This patch adds unit test for the new Koha::Illrequest::check_out method

Sponsored-by: Loughborough University
Comment 38 Andrew Isherwood 2019-10-16 09:04:16 UTC
Created attachment 94269 [details] [review]
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues

In response to the problem Magnus was experiencing in comment #28 & #29
we now call GetAuthorisedValues by it's full path
Comment 39 Andrew Isherwood 2019-11-19 14:59:56 UTC
Created attachment 95565 [details] [review]
Bug 23112: Add CirculateILL syspref

Sponsored-by: Loughborough University
Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Comment 40 Andrew Isherwood 2019-11-19 15:00:01 UTC
Created attachment 95566 [details] [review]
Bug 23112: Add circulation to ILL requests

This patch adds the ability to circulate ILL requests. Once a request has a suitable status, a "Check out" button is displayed on the "Manage request" toolbar. Clicking this will enable the user to check out the item either to the user who made the request or an in-house statistical  user. A due date can be specified, but if not circ rules are used.

Prior to the check out, an item is created which is attached to the biblio record that was created when the request was added

Sponsored-by: Loughborough University
Comment 41 Andrew Isherwood 2019-11-19 15:00:06 UTC
Created attachment 95567 [details] [review]
Bug 23112: Add permission checks

We only want the ILL circulation functionality to be available if the
appropriate syspref and perm are available, this patch adds those
checks.

Sponsored-by: Loughborough University
Comment 42 Andrew Isherwood 2019-11-19 15:00:11 UTC
Created attachment 95568 [details] [review]
Bug 23112: Update request status on return

This patch updates a request's status to RET upon return

Sponsored-by: Loughborough University
Comment 43 Andrew Isherwood 2019-11-19 15:00:16 UTC
Created attachment 95569 [details] [review]
Bug 23112: Add unit tests

This patch adds unit test for the new Koha::Illrequest::check_out method

Sponsored-by: Loughborough University
Comment 44 Andrew Isherwood 2019-11-19 15:00:20 UTC
Created attachment 95570 [details] [review]
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues

In response to the problem Magnus was experiencing in comment #28 & #29
we now call GetAuthorisedValues by it's full path
Comment 45 Jonathan Druart 2019-12-26 12:41:56 UTC
Comment on attachment 95566 [details] [review]
Bug 23112: Add circulation to ILL requests

Review of attachment 95566 [details] [review]:
-----------------------------------------------------------------

Also, I think you should use patron-title.inc to display patron's info.

::: Koha/Illrequest.pm
@@ +1211,5 @@
> +        # For some reason, AddIssue requires an unblessed Patron
> +        $issue_args[0] = $patron->unblessed;
> +        my $issue = C4::Circulation::AddIssue(@issue_args);
> +
> +        if ($issue && %{$issue}) {

AddIssue return a Koha::Checkout object. The test will work but I think it's confusing.

::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
@@ +704,4 @@
>      </script>
>      [% INCLUDE 'ill-list-table-strings.inc' %]
>      [% Asset.js("js/ill-list-table.js") | $raw %]
> +    [% Asset.js("js/ill-check-out.js") | $raw %]

Where does this file come from?
Comment 46 Andrew Isherwood 2020-01-06 12:05:44 UTC
(In reply to Jonathan Druart from comment #45)
> Comment on attachment 95566 [details] [review] [review]
> Bug 23112: Add circulation to ILL requests
> 
> Review of attachment 95566 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Also, I think you should use patron-title.inc to display patron's info.

Aha, good spot, thanks! I'm about to add a patch to address that.

> 
> ::: Koha/Illrequest.pm
> @@ +1211,5 @@
> > +        # For some reason, AddIssue requires an unblessed Patron
> > +        $issue_args[0] = $patron->unblessed;
> > +        my $issue = C4::Circulation::AddIssue(@issue_args);
> > +
> > +        if ($issue && %{$issue}) {
> 
> AddIssue return a Koha::Checkout object. The test will work but I think it's
> confusing.

Do you mean the variable name $issue is confusing? If so, I thought it made sense since the function returning it is called AddIssue and, internally, AddIssue also calls the returned value $issue.

> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
> @@ +704,4 @@
> >      </script>
> >      [% INCLUDE 'ill-list-table-strings.inc' %]
> >      [% Asset.js("js/ill-list-table.js") | $raw %]
> > +    [% Asset.js("js/ill-check-out.js") | $raw %]
> 
> Where does this file come from?

Another good spot, this include was leftover junk from an earlier attempt at this bug. About to attach a patch that removes this include.
Comment 47 Andrew Isherwood 2020-01-06 12:06:12 UTC
Created attachment 96834 [details] [review]
Bug 23112: (follow-up) Address QA feedback

As per comment #46
Comment 48 Jonathan Druart 2020-01-06 20:59:02 UTC
(In reply to Andrew Isherwood from comment #46)
> > ::: Koha/Illrequest.pm
> > @@ +1211,5 @@
> > > +        # For some reason, AddIssue requires an unblessed Patron
> > > +        $issue_args[0] = $patron->unblessed;
> > > +        my $issue = C4::Circulation::AddIssue(@issue_args);
> > > +
> > > +        if ($issue && %{$issue}) {
> > 
> > AddIssue return a Koha::Checkout object. The test will work but I think it's
> > confusing.
> 
> Do you mean the variable name $issue is confusing? If so, I thought it made
> sense since the function returning it is called AddIssue and, internally,
> AddIssue also calls the returned value $issue.

You should only test for $issue, the "&& %$issue" does not make sense to me.
Comment 49 Andrew Isherwood 2020-01-07 10:18:52 UTC
Created attachment 96870 [details] [review]
Bug 23112: (follow-up) Fix test as per feedback

As per comment #48

https://bugs.koha-community.org/show_bug.cgi?id=23113
Comment 50 Andrew Isherwood 2020-01-07 10:20:50 UTC
(In reply to Jonathan Druart from comment #48)
> (In reply to Andrew Isherwood from comment #46)
> > > ::: Koha/Illrequest.pm
> > > @@ +1211,5 @@
> > > > +        # For some reason, AddIssue requires an unblessed Patron
> > > > +        $issue_args[0] = $patron->unblessed;
> > > > +        my $issue = C4::Circulation::AddIssue(@issue_args);
> > > > +
> > > > +        if ($issue && %{$issue}) {
> > > 
> > > AddIssue return a Koha::Checkout object. The test will work but I think it's
> > > confusing.
> > 
> > Do you mean the variable name $issue is confusing? If so, I thought it made
> > sense since the function returning it is called AddIssue and, internally,
> > AddIssue also calls the returned value $issue.
> 
> You should only test for $issue, the "&& %$issue" does not make sense to me.

I've now attached a patch to address this. I'm puzzled why I did this extra test, it seems like the sort of thing that would be deliberate. Looking at AddIssue, I can't see a case where an empty hashref could be returned, so I'm not sure why I would have added this extra test. Anyway, thanks for pointing it out Jonathan.
Comment 51 Andrew Isherwood 2020-03-02 14:18:25 UTC
Created attachment 99910 [details] [review]
Bug 23112: Add CirculateILL syspref

Sponsored-by: Loughborough University
Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Comment 52 Andrew Isherwood 2020-03-02 14:18:28 UTC
Created attachment 99911 [details] [review]
Bug 23112: Add circulation to ILL requests

This patch adds the ability to circulate ILL requests. Once a request has a suitable status, a "Check out" button is displayed on the "Manage request" toolbar. Clicking this will enable the user to check out the item either to the user who made the request or an in-house statistical  user. A due date can be specified, but if not circ rules are used.

Prior to the check out, an item is created which is attached to the biblio record that was created when the request was added

Sponsored-by: Loughborough University
Comment 53 Andrew Isherwood 2020-03-02 14:18:32 UTC
Created attachment 99912 [details] [review]
Bug 23112: Add permission checks

We only want the ILL circulation functionality to be available if the
appropriate syspref and perm are available, this patch adds those
checks.

Sponsored-by: Loughborough University
Comment 54 Andrew Isherwood 2020-03-02 14:18:36 UTC
Created attachment 99913 [details] [review]
Bug 23112: Update request status on return

This patch updates a request's status to RET upon return

Sponsored-by: Loughborough University
Comment 55 Andrew Isherwood 2020-03-02 14:18:39 UTC
Created attachment 99914 [details] [review]
Bug 23112: Add unit tests

This patch adds unit test for the new Koha::Illrequest::check_out method

Sponsored-by: Loughborough University
Comment 56 Andrew Isherwood 2020-03-02 14:18:43 UTC
Created attachment 99915 [details] [review]
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues

In response to the problem Magnus was experiencing in comment #28 & #29
we now call GetAuthorisedValues by it's full path
Comment 57 Andrew Isherwood 2020-03-02 14:18:47 UTC
Created attachment 99916 [details] [review]
Bug 23112: (follow-up) Address QA feedback

As per comment #46
Comment 58 Andrew Isherwood 2020-03-02 14:18:50 UTC
Created attachment 99917 [details] [review]
Bug 23112: (follow-up) Fix test as per feedback

As per comment #48

https://bugs.koha-community.org/show_bug.cgi?id=23113
Comment 59 Andrew Isherwood 2020-03-02 14:19:33 UTC
Needed rebase due to dependent patch in bug 23384 changing
Comment 60 Nick Clemens 2020-03-04 12:11:09 UTC
Created attachment 100100 [details] [review]
Bug 23112: Add CirculateILL syspref

Sponsored-by: Loughborough University
Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>
Comment 61 Nick Clemens 2020-03-04 12:11:13 UTC
Created attachment 100101 [details] [review]
Bug 23112: Add circulation to ILL requests

This patch adds the ability to circulate ILL requests. Once a request has a suitable status, a "Check out" button is displayed on the "Manage request" toolbar. Clicking this will enable the user to check out the item either to the user who made the request or an in-house statistical  user. A due date can be specified, but if not circ rules are used.

Prior to the check out, an item is created which is attached to the biblio record that was created when the request was added

This development has been carried out according to the originally stated requirements of the customer that sponsored it, detailed here: https://wiki.koha-community.org/wiki/ILL_Circulation_RFC

Test plan:

1. Ensure the FreeForm ILL backend is available
2. Enable the "CirculateILL" syspref
3. Ensure you have a statistical patron category defined (patron category type "Statistical")
4. Ensure you have at least one patron in your statistical patron category
5. Create a new FreeForm request (make a note of the library you select when creating it)
6. Mark the new request as confirmed by clicking the "Confirm request" button on the "Manage ILL request" page
7. TEST: Observe that a "Check out" button is now displayed in the request toolbar
8. Click the "Check out" button in the "Manage ILL request" page
9. In the "Issue requested item to..." screen:
  a. Do not select a statistical patron at this time
  b. You can at this point choose an item type, this will determine the type of the item that will be created for this request
  c. TEST: Observe that the default selected "Library" matches that that was defined when creating the request
  d. Do not select a due date at this time
10. Click "Submit"
11. TEST: Observe that the "Item checked out" screen displays, issued to the requesting patron with a due date corresponding to appropriate circ rules
12. Click "Return to request"
13. TEST: Observe that the request's status is now "Checked out"
14. Click the "Bibliographic record ID" link
15. TEST: Observe that the bibliographic record now has one item attached to it which is checked out
16. TEST: Observe that the item barcode is "ILL-" + the ILL request ID
17. Return to step 5., however, this time select a statistical patron and test that the item use is recorded and the item is not issued
18. Return to step 5., however, this time manually select a due date and test that the item's due date is set correctly on check out
19. Check in the item
20. TEST: Observe that the request's status is updated to "Returned to library"
21. Now implement a restriction on the patron (perhaps a fine) which would prevent them from checking out an item
22. Return to step 5. follow the instructions to step 10.
23. TEST: Observe that a banner is displayed at the top of the screen informing you that there was a problem checking the item out, containing a link to the patron's account page
24. Resolve the problem with the patron's account
25. Return to step 8.
26. TEST: Observe that the item is now successfully checked out
27. Disable the "CirculateILL" syspref
28. Return to step 5. at step 7. Observe that the "Check out" button is NOT displayed

Sponsored-by: Loughborough University
Comment 62 Nick Clemens 2020-03-04 12:11:16 UTC
Created attachment 100102 [details] [review]
Bug 23112: Add permission checks

We only want the ILL circulation functionality to be available if the
appropriate syspref and perm are available, this patch adds those
checks.

Sponsored-by: Loughborough University
Comment 63 Nick Clemens 2020-03-04 12:11:20 UTC
Created attachment 100103 [details] [review]
Bug 23112: Update request status on return

This patch updates a request's status to RET upon return

Sponsored-by: Loughborough University
Comment 64 Nick Clemens 2020-03-04 12:11:23 UTC
Created attachment 100104 [details] [review]
Bug 23112: Add unit tests

This patch adds unit test for the new Koha::Illrequest::check_out method

Sponsored-by: Loughborough University
Comment 65 Nick Clemens 2020-03-04 12:11:27 UTC
Created attachment 100105 [details] [review]
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues

In response to the problem Magnus was experiencing in comment #28 & #29
we now call GetAuthorisedValues by it's full path
Comment 66 Nick Clemens 2020-03-04 12:11:30 UTC
Created attachment 100106 [details] [review]
Bug 23112: (follow-up) Address QA feedback

As per comment #46
Comment 67 Nick Clemens 2020-03-04 12:11:34 UTC
Created attachment 100107 [details] [review]
Bug 23112: (follow-up) Fix test as per feedback

As per comment #48

https://bugs.koha-community.org/show_bug.cgi?id=23113
Comment 68 Nick Clemens 2020-03-04 12:11:37 UTC
Created attachment 100108 [details] [review]
Bug 23112: (QA follow-up) Fix database update, add filters, remove tabs

Looks like the atomicupdate and sysprefs.sql changes were lost at some point.

Don't forget to run QA tools on your patches before submitting
Comment 69 Nick Clemens 2020-03-04 12:13:09 UTC
This all works as expected and is a great enhancement.

I added a follow-up to address a few minor issues.

The one complaint I have is that if I create a 'chapter' request there is no biblio created but I still get a 'checkout' button on the request, but have no way to attach/create a biblio - we should not show the button in this case
Comment 70 Andrew Isherwood 2020-03-04 12:27:55 UTC
Thanks for this Nick, it is much appreciated. Many thanks for the follow up, I do wish my editor would not randomly switch to tabs sometimes! Good point about the Checkout button, I'll create a follow up shortly and re-submit :)
Comment 71 Andrew Isherwood 2020-03-05 09:30:17 UTC
Created attachment 100169 [details] [review]
Bug 23051: (follow-up) Conditionally prevent checkout

As mentioned by Nick in comment #69, the checkout button displays even
for requests without a biblio attached (such as chapters). This patch
fixes that.
Comment 72 Andrew Isherwood 2020-03-06 09:37:05 UTC
Created attachment 100224 [details] [review]
Bug 23112: (follow-up) Conditionally prevent checkout

As mentioned by Nick in comment #69, the checkout button displays even
for requests without a biblio attached (such as chapters). This patch
fixes that.
Comment 73 Andrew Isherwood 2020-03-06 09:37:09 UTC
Created attachment 100225 [details] [review]
Bug 23112: (follow-up) Unit test fixes

As requested by Nick, fixes unit tests and added a test for the new
Koha::Illrequest::biblio method
Comment 74 Nick Clemens 2020-03-06 11:37:07 UTC
Created attachment 100252 [details] [review]
Bug 23112: Add CirculateILL syspref

Sponsored-by: Loughborough University
Signed-off-by: Chris Walton <C.J.Walton@lboro.ac.uk>

https://bugs.koha-community.org/show_bug.cgi?id=23112

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Nick Clemens 2020-03-06 11:37:11 UTC
Created attachment 100253 [details] [review]
Bug 23112: Add circulation to ILL requests

This patch adds the ability to circulate ILL requests. Once a request has a suitable status, a "Check out" button is displayed on the "Manage request" toolbar. Clicking this will enable the user to check out the item either to the user who made the request or an in-house statistical  user. A due date can be specified, but if not circ rules are used.

Prior to the check out, an item is created which is attached to the biblio record that was created when the request was added

This development has been carried out according to the originally stated requirements of the customer that sponsored it, detailed here: https://wiki.koha-community.org/wiki/ILL_Circulation_RFC

Test plan:

1. Ensure the FreeForm ILL backend is available
2. Enable the "CirculateILL" syspref
3. Ensure you have a statistical patron category defined (patron category type "Statistical")
4. Ensure you have at least one patron in your statistical patron category
5. Create a new FreeForm request (make a note of the library you select when creating it)
6. Mark the new request as confirmed by clicking the "Confirm request" button on the "Manage ILL request" page
7. TEST: Observe that a "Check out" button is now displayed in the request toolbar
8. Click the "Check out" button in the "Manage ILL request" page
9. In the "Issue requested item to..." screen:
  a. Do not select a statistical patron at this time
  b. You can at this point choose an item type, this will determine the type of the item that will be created for this request
  c. TEST: Observe that the default selected "Library" matches that that was defined when creating the request
  d. Do not select a due date at this time
10. Click "Submit"
11. TEST: Observe that the "Item checked out" screen displays, issued to the requesting patron with a due date corresponding to appropriate circ rules
12. Click "Return to request"
13. TEST: Observe that the request's status is now "Checked out"
14. Click the "Bibliographic record ID" link
15. TEST: Observe that the bibliographic record now has one item attached to it which is checked out
16. TEST: Observe that the item barcode is "ILL-" + the ILL request ID
17. Return to step 5., however, this time select a statistical patron and test that the item use is recorded and the item is not issued
18. Return to step 5., however, this time manually select a due date and test that the item's due date is set correctly on check out
19. Check in the item
20. TEST: Observe that the request's status is updated to "Returned to library"
21. Now implement a restriction on the patron (perhaps a fine) which would prevent them from checking out an item
22. Return to step 5. follow the instructions to step 10.
23. TEST: Observe that a banner is displayed at the top of the screen informing you that there was a problem checking the item out, containing a link to the patron's account page
24. Resolve the problem with the patron's account
25. Return to step 8.
26. TEST: Observe that the item is now successfully checked out
27. Disable the "CirculateILL" syspref
28. Return to step 5. at step 7. Observe that the "Check out" button is NOT displayed

Sponsored-by: Loughborough University

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 76 Nick Clemens 2020-03-06 11:37:14 UTC
Created attachment 100254 [details] [review]
Bug 23112: Add permission checks

We only want the ILL circulation functionality to be available if the
appropriate syspref and perm are available, this patch adds those
checks.

Sponsored-by: Loughborough University

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 77 Nick Clemens 2020-03-06 11:37:18 UTC
Created attachment 100255 [details] [review]
Bug 23112: Update request status on return

This patch updates a request's status to RET upon return

Sponsored-by: Loughborough University

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 78 Nick Clemens 2020-03-06 11:37:21 UTC
Created attachment 100256 [details] [review]
Bug 23112: Add unit tests

This patch adds unit test for the new Koha::Illrequest::check_out method

Sponsored-by: Loughborough University

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 79 Nick Clemens 2020-03-06 11:37:25 UTC
Created attachment 100257 [details] [review]
Bug 23112: (follow-up) Call C4::Koha::GetAuthorisedValues

In response to the problem Magnus was experiencing in comment #28 & #29
we now call GetAuthorisedValues by it's full path

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Nick Clemens 2020-03-06 11:37:28 UTC
Created attachment 100258 [details] [review]
Bug 23112: (follow-up) Address QA feedback

As per comment #46

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 81 Nick Clemens 2020-03-06 11:37:32 UTC
Created attachment 100259 [details] [review]
Bug 23112: (follow-up) Fix test as per feedback

As per comment #48

https://bugs.koha-community.org/show_bug.cgi?id=23113

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 82 Nick Clemens 2020-03-06 11:37:36 UTC
Created attachment 100260 [details] [review]
Bug 23112: (QA follow-up) Fix database update, add filters, remove tabs

Looks like the atomicupdate and sysprefs.sql changes were lost at some point.

Don't forget to run QA tools on your patches before submitting

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 83 Nick Clemens 2020-03-06 11:37:39 UTC
Created attachment 100261 [details] [review]
Bug 23112: (follow-up) Conditionally prevent checkout

As mentioned by Nick in comment #69, the checkout button displays even
for requests without a biblio attached (such as chapters). This patch
fixes that.

https://bugs.koha-community.org/show_bug.cgi?id=23112

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 84 Nick Clemens 2020-03-06 11:37:43 UTC
Created attachment 100262 [details] [review]
Bug 23112: (follow-up) Unit test fixes

As requested by Nick, fixes unit tests and added a test for the new
Koha::Illrequest::biblio method

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 85 Nick Clemens 2020-03-06 11:37:47 UTC
Created attachment 100263 [details] [review]
Bug 23112: (QA follow-up) Clean up tests, catch warnings

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 86 Martin Renvoize 2020-03-10 11:37:31 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 87 Joy Nelson 2020-04-02 18:54:04 UTC
new feature not backported to 19.11.x
Comment 88 Jonathan Druart 2020-06-04 12:39:51 UTC
+use Carp::Always;

That should be removed, right?
Comment 89 Katrin Fischer 2020-06-07 13:17:32 UTC
(In reply to Jonathan Druart from comment #88)
> +use Carp::Always;
> 
> That should be removed, right?

Should we file a new bug?
Comment 90 Jonathan Druart 2020-06-16 09:57:59 UTC
(In reply to Katrin Fischer from comment #89)
> (In reply to Jonathan Druart from comment #88)
> > +use Carp::Always;
> > 
> > That should be removed, right?
> 
> Should we file a new bug?

Yes, Andrew can you deal with it please?