|
Description
Magnus Enger
2019-02-13 08:40:14 UTC
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?
Tests are passing now, but will provide a follow-up to fix the number of planned tests. The UX is not good, we should use a proper patron search here instead of requiring to enter the borrowernumber. I've filed this as a separate issue: Bug 31324 - Use patron search when editing the borrowernumber of an ILL request Created attachment 138873 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 138874 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 138875 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 138876 [details] [review] Bug 22321: Unit test for new()->borrowernumber()->store() Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 138877 [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. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 138878 [details] [review] Bug 22321: (follow-up) Fix number of planned tests in Illrequests.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 141342 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 141343 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 141344 [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> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 141345 [details] [review] Bug 22321: Unit test for new()->borrowernumber()->store() Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 141346 [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. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 141347 [details] [review] Bug 22321: (follow-up) Fix number of planned tests in Illrequests.t Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Another nice improvement, all working as advertised. No QA failures or regressions found. Passing QA Pushed to master for 22.11. Nice work everyone, thanks! Enhancement will not be backported to 22.05.x series |