For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|'--- I do not know why this is happening, but there are a couple solutions: 1) Move the $server parameter to be the first parameter of the subroutine call 2) Check the value of $value and swap $value and $server if it is the server object. I will write a patch implementing the latter solution, as it is a two line change, versus a many line change for the former.
Created attachment 101559 [details] [review] Bug 24966: Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone!
Created attachment 101632 [details] [review] Bug 24966: Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov>
Could you provide a test? I did not manage to write one that fails without your patch. I suspect a place where we return () instead of undef
(In reply to Jonathan Druart from comment #3) > Could you provide a test? I did not manage to write one that fails without > your patch. > I suspect a place where we return () instead of undef Try as I might, I cannot replicate the error outside of production. Instead of this patch, would a patch reordering the parameters by more acceptable?
I am not sure I understand, how would you do that? My fair is that the real bug is hidden somewhere else, and fixing it here will keep it hidden.
(In reply to Jonathan Druart from comment #5) > I am not sure I understand, how would you do that? > > My fair is that the real bug is hidden somewhere else, and fixing it here > will keep it hidden. I agree, bug for the life of me I cannot even understand how this is possible. I think we might be even getting into perl internals finding the bug! I will write a version of the alternate I proposed. It's a more 'correct' fix anyway imo.
Created attachment 102584 [details] [review] Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov>
Created attachment 102585 [details] [review] Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value For reasons unknown to me, a call like: -- $resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server ); -- will not work as expected if the item has no callnumber. One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object. What is actually received is: 'CY', and a SIPServer object. We ingest the parameters like so: -- sub maybe_add { my ($fid, $value, $server) = @_; -- So, what happens is $value is populated with the server object! This can cause bad output like this: -- OUTPUT MSG: '101YNN20200324 063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|' -- Test Plan: 1) On master, perform a checkin of an item not on hold using the sip cli tester 2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)' 3) Apply this patch 4) Restart the SIP server 5) Perform the SIP checkin again 6) Those fields from step 2 should be gone! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov>
Looking here
We're not on the right track here. But hang on, found something..
Since you refer to a wrong message with CY, I should have started looking for that in the first place ;) Check CY in Constants FID_HOLD_PATRON_ID => 'CY', Now git grep FID_HOLD_PATRON_ID Sip/Constants.pm: FID_HOLD_PATRON_ID Sip/Constants.pm: FID_HOLD_PATRON_ID => 'CY', Sip/MsgType.pm:$resp .= maybe_add( FID_HOLD_PATRON_ID, $item->hold_patron_bcode, $server ); Ah, this call should be the one. If hold_patron_bcode returns an implicit undef in list context (read: empty list), than we found the culprit. Look for it: sub hold_patron_bcode { my $self = shift; my $borrowernumber = (@_ ? shift: $self->hold_patron_id()) or return; my $holder = Koha::Patrons->find( $borrowernumber ); if ($holder and $holder->cardnumber ) { return $holder->cardnumber; } return; } And yes! The last return in list context makes $server shift to the second position. So how should we address that? It is used only once. We could fix the call with scalar or explicitly return empty string. I choose for the latter here. Preventing a second bad call in future.
Created attachment 102689 [details] [review] Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode It is used in list context, but we need a scalar value. Can be fixed by adding scalar's, or returning empty string as here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Kyle M Hall from comment #6) > I agree, bug for the life of me I cannot even understand how this is > possible. I think we might be even getting into perl internals finding the > bug! No perl internals here ;) Just the difference between scalar and list context..
Another note when git grepping maybe_add: Sip/MsgType.pm: $resp .= maybe_add( FID_SCREEN_MSG, $msg, $server, $server ); two servers ? this occurs more than once, 15x ?
Should not we need another QA review on your patch Marcel? Should not we provide a test to cover the change?
(In reply to Marcel de Rooy from comment #14) > Another note when git grepping maybe_add: > > Sip/MsgType.pm: $resp .= maybe_add( FID_SCREEN_MSG, $msg, $server, > $server ); > two servers ? this occurs more than once, 15x ? Thanks Marcel! That is definitely a bug as well. I'll file a separate report for that!
(In reply to Jonathan Druart from comment #15) > Should not we need another QA review on your patch Marcel? Should not we > provide a test to cover the change? Leaving that to the RM. Thx for feedback.
Created attachment 102697 [details] [review] Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode It is used in list context, but we need a scalar value. Can be fixed by adding scalar's, or returning empty string as here. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 102698 [details] [review] Bug 24966: Add unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
backported to 19.11.x for 19.11.06
backported to 19.05.x for 19.05.11