Bug 22321

Summary: Make it possible to edit illrequests.borrowernumber
Product: Koha Reporter: Magnus Enger <magnus>
Component: ILLAssignee: Lari Taskula <lari.taskula>
Status: Signed Off --- QA Contact:
Severity: enhancement    
Priority: P5 - low CC: cslone, lari.taskula, Niamh.WalkerHeadon, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 31324    
Attachments: Bug 22321: Add unit tests
Bug 22321: Update holds borrowernumber when ILL request borrowernumber changes
Bug 22321: Allow managing the borrowernumber tied to ILL request
Bug 22321: Unit test for new()->borrowernumber()->store()
Bug 22321: Fix store() crash when modifying borrowernumber of a new ILL request
Bug 22321: Add unit tests
Bug 22321: Update holds borrowernumber when ILL request borrowernumber changes
Bug 22321: Allow managing the borrowernumber tied to ILL request
Bug 22321: Unit test for new()->borrowernumber()->store()
Bug 22321: Fix store() crash when modifying borrowernumber of a new ILL request
Bug 22321: Add unit tests
Bug 22321: Update holds borrowernumber when ILL request borrowernumber changes
Bug 22321: Allow managing the borrowernumber tied to ILL request
Bug 22321: Unit test for new()->borrowernumber()->store()
Bug 22321: Fix store() crash when modifying borrowernumber of a new ILL request
Bug 22321: (follow-up) Fix number of planned tests in Illrequests.t

Description Magnus Enger 2019-02-13 08:40:14 UTC
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.
Comment 1 Magnus Enger 2021-09-02 09:24:10 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.
Comment 2 Lari Taskula 2022-02-14 11:30:37 UTC Comment hidden (obsolete)
Comment 3 Lari Taskula 2022-02-14 11:30:41 UTC Comment hidden (obsolete)
Comment 4 Lari Taskula 2022-02-14 11:30:44 UTC Comment hidden (obsolete)
Comment 5 Magnus Enger 2022-02-15 10:43:16 UTC
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?
Comment 6 Lari Taskula 2022-02-22 14:10:37 UTC Comment hidden (obsolete)
Comment 7 Lari Taskula 2022-02-22 14:11:54 UTC
(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.
Comment 8 Lari Taskula 2022-02-22 14:16:02 UTC Comment hidden (obsolete)
Comment 9 Magnus Enger 2022-02-24 08:57:55 UTC
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>
Comment 10 Magnus Enger 2022-02-24 08:58:02 UTC
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>
Comment 11 Magnus Enger 2022-02-24 08:58:07 UTC
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>
Comment 12 Magnus Enger 2022-02-24 08:58:12 UTC
Created attachment 131058 [details] [review]
Bug 22321: Unit test for new()->borrowernumber()->store()

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 13 Magnus Enger 2022-02-24 08:58:17 UTC
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.
Comment 14 Magnus Enger 2022-02-24 09:00:23 UTC
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.
Comment 15 Katrin Fischer 2022-02-26 19:41:10 UTC
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?
Comment 16 Katrin Fischer 2022-08-09 06:15:11 UTC
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
Comment 17 Katrin Fischer 2022-08-09 06:16:39 UTC
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>
Comment 18 Katrin Fischer 2022-08-09 06:16:43 UTC
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>
Comment 19 Katrin Fischer 2022-08-09 06:16:47 UTC
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>
Comment 20 Katrin Fischer 2022-08-09 06:16:51 UTC
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>
Comment 21 Katrin Fischer 2022-08-09 06:16:56 UTC
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>
Comment 22 Katrin Fischer 2022-08-09 06:17:00 UTC
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>