Bug 21486 - SIP does not return checked out (charged) items on patron_information request
Summary: SIP does not return checked out (charged) items on patron_information request
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Josef Moravec
URL:
Keywords:
Depends on: 19935
Blocks:
  Show dependency treegraph
 
Reported: 2018-10-04 11:12 UTC by Josef Moravec
Modified: 2019-10-14 19:58 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21486: Re-added get_pending_issues function to C4/SIP/ILS/Patron.pm (5.72 KB, patch)
2018-10-09 00:59 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21486: Fix 'AU' for SIP (1.05 KB, patch)
2018-10-12 15:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Signed off patch (1.11 KB, patch)
2018-10-16 14:07 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 21486: Fix 'AU' for SIP (1.11 KB, patch)
2018-10-17 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21486: Only fetch the items' barcodes (1.06 KB, patch)
2018-10-17 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
patch restricting to barcodes (1.12 KB, patch)
2018-10-22 12:57 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 21486: Fix 'AU' for SIP (1.16 KB, patch)
2018-10-23 10:34 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21486: Only fetch the items' barcodes (1.24 KB, patch)
2018-10-23 10:35 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 21486: Fix 'AU' for SIP (1.22 KB, patch)
2018-10-24 06:31 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 21486: Only fetch the items' barcodes (1.30 KB, patch)
2018-10-24 06:31 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2018-10-04 11:12:49 UTC
You can test it with this:

misc/sip_cli_emulator.pl -a localhost -p 6001 -su koha -sp koha -l CPL --patron 23529000126806 -s "  Y       " -m patron_information

And see the "AU" field is empty:

64              00120181004    110729000000000001000000000000AOCPL|AA23529000126806|AEBrent Hopkins|BLY|CC5|AU|BD6692 Library Rd. Santa Cruz, CA 21234|BF(212) 555-1212|PB19771113|PCT|PIY|AFGreetings from Koha. |


This is because commit
bc001c2352fee419a2bbe20c0cf707afd9e5bd78 Bug 19935: Replace GetPendingIssues - SIP
Comment 1 Alex Buckley 2018-10-09 00:59:19 UTC
Created attachment 80251 [details] [review]
Bug 21486: Re-added get_pending_issues function to C4/SIP/ILS/Patron.pm

This allows the list of a borrower's checked out items to be returned by
SIP.

Test plan:
1. Make sure SIP is working with the commands:
sudo koha-enable-sip <instancename>
sudo koha-start-sip <instancename>

2. Check SIP is working with the command:
telnet localhost 6001

3. Choose a patron with items checked out to them and set their username
and password in the SIPconfig.xml file

4. From inside the Koha-shell from the Koha home directory run the command:
misc/sip_cli_emulator.pl -a localhost -p 6001 -su <sip_user_username> -sp <sip_user_password> -l <sip_user_librarybranch>
--patron <sip_user_cardnumber> -s "  Y       " -m patron_information

This command is PATRON_INFORMATION, it should return information about
the patron.

The Y in the 3rd position of the quotes means return the list of the
borrowers charged items (items issued to the borrower).

5. Notice the "AU" field is empty:

e.g. READ: 64              00120181008
161428000100000001000000000002AOCPL|AA8765432|AEMika
SMith|BLY|CC5|AU|PCB|PIY|AFGreetings from Koha. |

i.e. There should be a barcode after the AU field as the borrower has a
item issued to them

6. Apply patch

7. Restart memcached and plack with the commands:
sudo service memcached restart
sudo koha-plack --restart <instancename>

8. Stop SIP:
sudo koha-stop-sip <instancename>

9. Then enable and start SIP again:
sudo koha-enable-sip <instancename>
sudo koha-start-sip <instancename>

10. Repeat step 4 and notice the AU field does have a item barcode
value:

e.g. READ: 64              00120181008
161859000100000001000000000002AOCPL|AA8765432|AEMika
SMith|BLY|CC5|AUTEST7474747474|PCB|PIY|AFGreetings from Koha. |

In this case the item barcode value for the AU field is: TEST7474747474

Sponsored-By: Brimbank Library, Australia
Comment 2 Alex Buckley 2018-10-09 01:20:35 UTC
Actually I want to make a few more changes to this solution for master.
Comment 3 Colin Campbell 2018-10-09 13:12:32 UTC
Surely you dont need to fetch the info from borrowers - its being called from the ILS::Patron object we should have that info already
Comment 4 Jonathan Druart 2018-10-12 15:47:55 UTC
Created attachment 80522 [details] [review]
Bug 21486: Fix 'AU' for SIP
Comment 5 Jonathan Druart 2018-10-12 15:49:31 UTC
(In reply to Jonathan Druart from comment #4)
> Created attachment 80522 [details] [review] [review]
> Bug 21486: Fix 'AU' for SIP

We should not re-add code we removed :)
One possible fix would be to call unblessed_all_relateds, but we should be more specific. The problem here is that I have no idea which info is needed in $ilspatron{items}.
Comment 6 Colin Campbell 2018-10-16 14:07:02 UTC
Created attachment 80655 [details] [review]
Signed off patch

Signed off Jonathan's patch as it works for me in testing, Alex can you confirm?
Comment 7 Jonathan Druart 2018-10-16 14:16:39 UTC
(In reply to Jonathan Druart from comment #5)
> The problem here is that I have no idea which info is needed
> in $ilspatron{items}.

Colin, do you know the answer?
Comment 8 Colin Campbell 2018-10-16 14:27:58 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Jonathan Druart from comment #5)
> > The problem here is that I have no idea which info is needed
> > in $ilspatron{items}.
> 
> Colin, do you know the answer?

Yes (infact we collided) items is an array of barcodes of items issued to this patron
Comment 9 Jonathan Druart 2018-10-16 15:02:13 UTC
(In reply to Colin Campbell from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > (In reply to Jonathan Druart from comment #5)
> > > The problem here is that I have no idea which info is needed
> > > in $ilspatron{items}.
> > 
> > Colin, do you know the answer?
> 
> Yes (infact we collided) items is an array of barcodes of items issued to
> this patron

So we only need the barcodes, for other (if exist) calls as well?
If so I can provide a better patch.
Comment 10 Colin Campbell 2018-10-17 09:35:37 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Colin Campbell from comment #8)
> > (In reply to Jonathan Druart from comment #7)
> > > (In reply to Jonathan Druart from comment #5)
> > > > The problem here is that I have no idea which info is needed
> > > > in $ilspatron{items}.
> > > 
> > > Colin, do you know the answer?
> > 
> > Yes (infact we collided) items is an array of barcodes of items issued to
> > this patron
> 
> So we only need the barcodes, for other (if exist) calls as well?
> If so I can provide a better patch.

Yes the logic in the client machines is to get a list of relevant barcodes for the queries and it can then issue item information queries of those.
The exception (theres always one) is fine items as item_id does not give you relevant info (thats not specific to Koha, it a fairly useless option in most LMSs) Koha has some specific code there
Comment 11 Jonathan Druart 2018-10-17 15:33:19 UTC
Created attachment 80729 [details] [review]
Bug 21486: Fix 'AU' for SIP

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Comment 12 Jonathan Druart 2018-10-17 15:33:24 UTC
Created attachment 80730 [details] [review]
Bug 21486: Only fetch the items' barcodes

and avoid unecessary processing
Comment 13 Colin Campbell 2018-10-22 12:57:38 UTC
Created attachment 80976 [details] [review]
patch restricting to barcodes

Tested patch and works as advertised - signed off
Comment 14 Colin Campbell 2018-10-22 13:05:16 UTC
I've signed off Jonathan's two patches they restore the previous behaviour. Alex can you confirm this does all you envisioned and can go with it in place of your original patch?

We should endeavour to get this into 18.11 or their is potential for working setups to be broken
Comment 15 Alex Buckley 2018-10-23 10:33:04 UTC
(In response to Colin Campbell in comment 14):

Yes I can confirm Jonathan's patches do what I had envisaged and can replace my patch.
Comment 16 Alex Buckley 2018-10-23 10:34:52 UTC
Created attachment 81001 [details] [review]
Bug 21486: Fix 'AU' for SIP

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 17 Alex Buckley 2018-10-23 10:35:14 UTC
Created attachment 81002 [details] [review]
Bug 21486: Only fetch the items' barcodes

and avoid unecessary processing

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

Tested and found correct AU data is returned, and passes QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 18 Colin Campbell 2018-10-23 15:47:16 UTC
Bonus - fixes a failure to process renew requests I've just spotted
Comment 19 Jonathan Druart 2018-10-23 16:02:19 UTC
Upgrading severity as it impacts the last stable version.
Comment 20 Josef Moravec 2018-10-24 06:31:07 UTC
Created attachment 81050 [details] [review]
Bug 21486: Fix 'AU' for SIP

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Josef Moravec 2018-10-24 06:31:12 UTC
Created attachment 81051 [details] [review]
Bug 21486: Only fetch the items' barcodes

and avoid unecessary processing

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

Tested and found correct AU data is returned, and passes QA test tool

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 22 Magnus Enger 2018-10-24 11:55:39 UTC
(In reply to Jonathan Druart from comment #19)
> Upgrading severity as it impacts the last stable version.

Yes, this is definitely needed in 18.05.x.
Comment 23 Nick Clemens 2018-10-24 17:33:31 UTC
Awesome work all!

Pushed to master for 18.11
Comment 24 Martin Renvoize 2018-11-06 10:21:14 UTC
Pushed to 18.05.x for 18.05.06
Comment 25 Fridolin SOMERS 2018-11-26 08:24:51 UTC
Depends on Bug 19935 not in 17.11.x