Sometimes an ILL request is connected to the wrong borrower, and it would be nice to be able to connect it to the right one. In Swedish ILL the cardnumber of the patron connected with an ILL request is entered into the central Libris system and then propagated to Koha through the Libris API. This can lead to errors, and we have created a dummy "ILL patron" to catch these. It would then be good to be able to connect the ILL to the right patron in Koha. We could implement this in the Libris backend, but I think it is relevant for other backends too, and should be implemented in the ILL module itself.
If the ILL request is connected to a bibliographic record and that record has a reservation connected to the original patron, that reservation also needs to be moved to the new patron.
Created attachment 130544 [details] [review] Bug 22321: Add unit tests To test: 1. prove t/db_dependent/Illrequests.t 2. Observe test failure 1..3 ok 1 - before change, original borrowernumber found not ok 2 - after change, changed borrowernumber found in holds # Failed test 'after change, changed borrowernumber found in holds' # at t/db_dependent/Illrequests.t line 167. # got: '3786' # expected: '3787' ok 3 - after change, changed borrowernumber found in illrequests # Looks like you failed 1 test of 3. not ok 4 - store borrowernumber change also updates holds
Created attachment 130545 [details] [review] Bug 22321: Update holds borrowernumber when ILL request borrowernumber changes To test: 1. prove t/db_dependent/Illrequests.t
Created attachment 130546 [details] [review] Bug 22321: Allow managing the borrowernumber tied to ILL request Sometimes an ILL request is connected to the wrong borrower, and it would be nice to be able to connect it to the right one. In Swedish ILL the cardnumber of the patron connected with an ILL request is entered into the central Libris system and then propagated to Koha through the Libris API. This can lead to errors, and we have created a dummy "ILL patron" to catch these. It would then be good to be able to connect the ILL to the right patron in Koha. This commit makes it possible to edit illrequests.borrowernumber. To test: 1. Apply patch 2. Create two patrons, A and B 3. Create a new ILL request for patron A 4. Add a hold for patron A where biblionumber is the biblio tied to this ILL request 5. Go to cgi-bin/koha/ill/ill-requests.pl?method=edit_action&illrequest_id=161 6. Observe Patron ID is editable 7. Change borrowernumber to patron B's borrowernumber 8. Click save 9. Observe ILL request is now for patron B 10. Go to patron B's details at cgi-bin/koha/members/moremember.pl?borrowernumber=B_borrowernumber 11. At the bottom section of the page, click 1 Hold(s) 12. Observe the hold you placed at step 4 has transferred to patron b
After applying these patches I get this when I try to create a new illrequest: Can't call method "borrowernumber" on an undefined value at /home/me/kohaclone/Koha/Illrequest.pm line 1643. 1637 my @holds; 1638 if( defined $updated_columns{'borrowernumber'} and 1639 Koha::Patrons->find( $updated_columns{'borrowernumber'} ) ) 1640 { 1641 # borrowernumber has changed 1642 my $old_illreq = $self->get_from_storage; 1643 @holds = Koha::Holds->search( { 1644 borrowernumber => $old_illreq->borrowernumber, 1645 biblionumber => $self->biblio_id, 1646 } )->as_list; 1647 } Not sure what the problem is, but could it be that this works for existing requests, but for new ones there just isn't an $old_illreq to get the borrowernumber from?
Created attachment 131012 [details] [review] Bug 22321: Unit test for new()->borrowernumber()->store()
(In reply to Magnus Enger from comment #5) > After applying these patches I get this when I try to create a new > illrequest: > > Can't call method "borrowernumber" on an undefined value at > /home/me/kohaclone/Koha/Illrequest.pm line 1643. > > 1637 my @holds; > 1638 if( defined $updated_columns{'borrowernumber'} and > 1639 Koha::Patrons->find( $updated_columns{'borrowernumber'} ) ) > 1640 { > 1641 # borrowernumber has changed > 1642 my $old_illreq = $self->get_from_storage; > 1643 @holds = Koha::Holds->search( { > 1644 borrowernumber => $old_illreq->borrowernumber, > 1645 biblionumber => $self->biblio_id, > 1646 } )->as_list; > 1647 } > > Not sure what the problem is, but could it be that this works for existing > requests, but for new ones there just isn't an $old_illreq to get the > borrowernumber from? Good catch. I've added an unit test to cover this bug.
Created attachment 131013 [details] [review] Bug 22321: Fix store() crash when modifying borrowernumber of a new ILL request To test: 1. prove t/db_dependent/Illrequests.t
Created attachment 131055 [details] [review] Bug 22321: Add unit tests To test: 1. prove t/db_dependent/Illrequests.t 2. Observe test failure 1..3 ok 1 - before change, original borrowernumber found not ok 2 - after change, changed borrowernumber found in holds # Failed test 'after change, changed borrowernumber found in holds' # at t/db_dependent/Illrequests.t line 167. # got: '3786' # expected: '3787' ok 3 - after change, changed borrowernumber found in illrequests # Looks like you failed 1 test of 3. not ok 4 - store borrowernumber change also updates holds Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 131056 [details] [review] Bug 22321: Update holds borrowernumber when ILL request borrowernumber changes To test: 1. prove t/db_dependent/Illrequests.t Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 131057 [details] [review] Bug 22321: Allow managing the borrowernumber tied to ILL request Sometimes an ILL request is connected to the wrong borrower, and it would be nice to be able to connect it to the right one. In Swedish ILL the cardnumber of the patron connected with an ILL request is entered into the central Libris system and then propagated to Koha through the Libris API. This can lead to errors, and we have created a dummy "ILL patron" to catch these. It would then be good to be able to connect the ILL to the right patron in Koha. This commit makes it possible to edit illrequests.borrowernumber. To test: 1. Apply patch 2. Create two patrons, A and B 3. Create a new ILL request for patron A 4. Add a hold for patron A where biblionumber is the biblio tied to this ILL request 5. Go to cgi-bin/koha/ill/ill-requests.pl?method=edit_action&illrequest_id=161 6. Observe Patron ID is editable 7. Change borrowernumber to patron B's borrowernumber 8. Click save 9. Observe ILL request is now for patron B 10. Go to patron B's details at cgi-bin/koha/members/moremember.pl?borrowernumber=B_borrowernumber 11. At the bottom section of the page, click 1 Hold(s) 12. Observe the hold you placed at step 4 has transferred to patron b Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 131058 [details] [review] Bug 22321: Unit test for new()->borrowernumber()->store() Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 131059 [details] [review] Bug 22321: Fix store() crash when modifying borrowernumber of a new ILL request To test: 1. prove t/db_dependent/Illrequests.t Signed-off-by: Magnus Enger <magnus@libriotech.no> Tried to change the borrowernumber connected to an ILL request and checked that the associated hold was updated to the new patron as well.
I have signed off on these patches, as they work for me. However, I am involved in commissioning these patches, so I do not update the BZ status from NSO to SO.
I'd like to sign-off, but have issues with the tests here before and after applying the patch. This is without the patches: kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Illrequests.t t/db_dependent/Illrequests.t .. 9/13 # Failed test 'We get expected error on notforloan of item' # at t/db_dependent/Illrequests.t line 1150. # Structures begin differing at: # $got->{confirm} = HASH(0x564c474ded60) # $expected->{confirm} = Does not exist # Failed test at t/db_dependent/Illrequests.t line 1173. # got: 'form' # expected: 'done_check_out' # Failed test at t/db_dependent/Illrequests.t line 1175. # got: '0' # expected: '1' # Failed test at t/db_dependent/Illrequests.t line 1176. # got: 'YPI9JroljKCZerYRd4NATNNudqRk7gIs0eisCksKqghavsp' # expected: 'CHK' # Looks like you failed 4 tests of 17. t/db_dependent/Illrequests.t .. 10/13 # Failed test 'Checking out' # at t/db_dependent/Illrequests.t line 1179. t/db_dependent/Illrequests.t .. 13/13 # Looks like you failed 1 test of 13. t/db_dependent/Illrequests.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/13 subtests Test Summary Report ------------------- t/db_dependent/Illrequests.t (Wstat: 256 Tests: 13 Failed: 1) Failed test: 10 Non-zero exit status: 1 Files=1, Tests=13, 10 wallclock secs ( 0.06 usr 0.00 sys + 8.58 cusr 1.42 csys = 10.06 CPU) Result: FAIL Any idea about this? Do they pass for you, Magnus?