Bug 38375

Summary: SIP2 syspref SIP2SortBinMapping is not working
Product: Koha Reporter: Lari Strand <lmstrand>
Component: SIP2Assignee: Lari Strand <lmstrand>
Status: Pushed to stable --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: critical    
Priority: P5 - low CC: bibliothek, clemens.tubach, dcook, ewa.gozd, fridolin.somers, janne.seppanen, magnus, martin.renvoize, michaela.sieber, paul.derscheid
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.05.00,24.11.03
Circulation function:
Attachments: Bug 38375: Splitting SIP2SortBinMapping syspref does not support all operating systems' line endings
Bug 38375: Splitting SIP2SortBinMapping syspref does not support all operating systems' line endings
Bug 38375: Use perl 5.10+ \R
Bug 38375: Unit tests

Description Lari Strand 2024-11-06 11:19:21 UTC
Sort bin rules in system preference SIP2SortBinMapping are not being used by the SIP server. This functionality has broken between the 23.11 and 24.05 versions.
We are adding the mappings directly to the SIP2 server configuration XML file as a workaround in 24.05 for now since sort bin mappings still work using the sip config file.
Comment 1 Janne Seppänen 2024-12-10 09:21:11 UTC
I can confirm that the system preference SIP2SortBinMapping is currently not working in 24.05. We had built our entire sorting system around this feature in 23.11 and it worked fine, but after the 24.05 update the SIP messages that Koha sends do not include the CL field at all.

As we make regular updates to sorting rules (e.g. with seasonal materials) it's not a sustainable way to do this manually on the server side on all self-service stations, one by one. This is an important feature to us, and we hope that the bug could be found and fixed soon.
Comment 2 David Cook 2024-12-12 05:03:30 UTC
Marking as CONFIRMED on behalf of Janne
Comment 3 Jan Kissig 2025-01-04 19:06:16 UTC
I just tried to confirm the bug on my ktd setup (v24.05.05) to dig into it but in my case it worked:

SIP2SortBinMapping: CPL:itype:eq:BK:5

kohadev-koha@kohadevbox:misc((v24.05.05))$ perl sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1   -l CPL  -m checkout --item 39999000000498 --patron 42
Attempting socket connection to 127.0.0.1:6001...connected!
Use of uninitialized value $hold_mode in string eq at sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'checkout'
SEND: 11YN20250104    19020520250104    190205AOCPL|AA42|AB39999000000498|ACterm1|BIN|
READ: 121NNY20250104    190205AOCPL|AA42|AB39999000000498|AJCGI programming with Perl /|AH20250109    235900|

kohadev-koha@kohadevbox:misc((v24.05.05))$ perl sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1   -l CPL  -m checkin --item 39999000000498 --patron 42
Attempting socket connection to 127.0.0.1:6001...connected!
Use of uninitialized value $hold_mode in string eq at sip_cli_emulator.pl line 123.
SEND: 9300CNterm1|COterm1|CPCPL|
READ: 941

Trying 'checkin'
SEND: 09N20250104    19020620250104    190206APCPL|AOCPL|AB39999000000498|ACterm1|BIN|
READ: 101YNN20250104    190207AOCPL|AB39999000000498|AQCPL|AJCGI programming with Perl /|CL5|AA42|CRREF|

---

In the checkin response the mentioned field CL is set to 5. Also tried on main (24.12.) and it worked. Can you give a little more info on your sort bin rules?
Comment 4 Janne Seppänen 2025-01-14 12:10:07 UTC
Hmm, that's strange. We don't have access to an emulator to check if would differ from a self-service station in our environment, but we have a similar result from two different libraries using stations from different manufacturers.

Our rules are extensive, but we have also tried to leave only one row/rule to the Sip2SortBinMapping syspref without any impact to the behaviour. We also use comments for structuring the rules (there are around 400 rules in total for our library consortium) but they don't seem to be cause.
 
Here are a few examples:

# Varastoaineisto
LAPK:location:eq:AIV:03
LAPK:location:eq:MUV:03
LAPK:location:eq:LAV:03
LAPK:location:eq:NUV:03

# Omat lehdet (jää palautusalueelle)
LAPK:itype:eq:14VRKLEHTI:homebranch:eq:LAPK:00
LAPK:itype:eq:14VRKLEHLA:homebranch:eq:LAPK:00

We've tried to remove the leading zero to no effect.
Comment 5 Jan Kissig 2025-01-14 12:33:30 UTC
(In reply to Janne Seppänen from comment #4)
> Hmm, that's strange. We don't have access to an emulator to check if would
> differ from a self-service station in our environment, but we have a similar
> result from two different libraries using stations from different
> manufacturers.
> 
> Our rules are extensive, but we have also tried to leave only one row/rule
> to the Sip2SortBinMapping syspref without any impact to the behaviour. We
> also use comments for structuring the rules (there are around 400 rules in
> total for our library consortium) but they don't seem to be cause.
>  
> Here are a few examples:
> 
> # Varastoaineisto
> LAPK:location:eq:AIV:03
> LAPK:location:eq:MUV:03
> LAPK:location:eq:LAV:03
> LAPK:location:eq:NUV:03
> 
> # Omat lehdet (jää palautusalueelle)
> LAPK:itype:eq:14VRKLEHTI:homebranch:eq:LAPK:00
> LAPK:itype:eq:14VRKLEHLA:homebranch:eq:LAPK:00
> 
> We've tried to remove the leading zero to no effect.

You can find the emulator directly on your koha server: /usr/share/koha/bin/sip_cli_emulator.pl
Comment 6 Katrin Fischer 2025-01-14 13:42:19 UTC
I am not sure if this needs to be done, but have you tried restarting the SIP server after changing the values?
Comment 7 Lari Strand 2025-01-17 11:58:57 UTC
Our 09 checkin messages are in a form like this:
09N20250116    16144920250116    161449AP|AOORPK|ABOM0L154262|AC|AY5AZEFAB

Maybe it has something to do with the message structure that breaks stuff.
Could this be translated to the sip emulator for verifying the issue exists with this message structure?
Comment 8 Jan Kissig 2025-01-17 12:18:11 UTC
In your checkin the AP is empty:
currentLocation AP=
institutionId   AO=ORPK

Maybe you can check via emulator when setting 

-l ORPK

so that the whole request looks like:

perl sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su USER -sp PASS   -l ORPK  -m checkin --item OM0L154262

if the response includes a CL field.
Comment 9 Lari Strand 2025-01-20 13:44:53 UTC
It seems parsing the mappings break when there are more than one line of them defined in SIP2SortBinMapping syspref. This gets logged in case of more than one line of mappings:

[2025/01/20 14:25:02] [3705123] [WARN] TESTSIP1@10.0.2.202: Malformed preference line found: 'LAPK:location:eq:MUV:02
LAPK:location:eq:AIV:03' at /home/koha/Koha/C4/SIP/ILS/Transaction/Checkin.pm line 282, <STDIN> line 2.

If there is only one line, the line's mapping rule works and the CL field gets set accordingly.

I tried to hunt down a commit that's breaking the parsing of the syspref but I only found bug 34737 that touches the sub _get_sort_bin and reverting the commits did not resolve the issue with multiple lines of sort mappings.
Comment 10 Lari Strand 2025-01-20 14:24:28 UTC
So it seems the issue is that splitting the lines does not happen unless the syspref is written in windows with a carriage return + newline \r\n? We should also support UNIX LF.
Comment 11 Lari Strand 2025-01-21 07:47:12 UTC
"Windows typically uses CRLF (\r\n), macOS uses CR (\r), and Linux/Unix systems use LF (\n)".
We need to support macOS \r as well.
Comment 12 Magnus Enger 2025-01-21 10:04:07 UTC
Sounds like this is the problem, then: 

        # Get the mapping and split on newlines
        my $raw_map = C4::Context->preference('SIP2SortBinMapping');
        return unless $raw_map;
        @lines = split /\r\n/, $raw_map;

https://git.koha-community.org/Koha-community/Koha/src/commit/bb2e88a1e3ebbed3d291cdf7450933aca0ff88ec/C4/SIP/ILS/Transaction/Checkin.pm#L264
Comment 13 Magnus Enger 2025-01-21 10:08:49 UTC
And it looks like "split /\R/" should do the trick? 

POC: 

#!/usr/bin/perl
use Modern::Perl;
foreach my $string ( "a\r\nb", "a\rb", "a\nb" ) {
    say "==========";
    say $string;
    say "----------";
    my ( $a, $b ) = split /\R/, $string;
    say "$a + $b";
}

https://perldoc.perl.org/perlrebackslash#Misc
Comment 14 Lari Strand 2025-01-21 10:09:20 UTC
Created attachment 176853 [details] [review]
Bug 38375: Splitting SIP2SortBinMapping syspref does not support all operating systems' line endings
Comment 15 Lari Strand 2025-01-21 10:12:54 UTC
Maybe someone can help with a test plan if this needs one. I'll just post the patch here.
Comment 16 Magnus Enger 2025-01-22 12:13:50 UTC
It would have been nice to have the relevant code in a subroutine, so we could have tests for it. Not sure if we can test it with different line endings, without testing in three different OSes?
Comment 17 Lari Strand 2025-01-23 11:11:22 UTC
Could we generate the rules like in the existing test, but with different line endings and rerun the same tests again?

existing test (Transactions.t line 921):

    my $rules = <<"RULES";
$branch:homebranch:ne:\$holdingbranch:X\r
$branch:effective_itemtype:eq:CD:0\r
$branch:itemcallnumber:<:340:1\r
$branch:itemcallnumber:<:370:2\r
$branch:itemcallnumber:<:600:3\r
$branch2:homebranch:ne:\$holdingbranch:X\r
$branch2:effective_itemtype:eq:CD:4\r
$branch2:itemcallnumber:>:600:5\r
RULES

new test for unix:

$branch:homebranch:ne:\$holdingbranch:X\n
$branch:effective_itemtype:eq:CD:0\n
$branch:itemcallnumber:<:340:1\n
$branch:itemcallnumber:<:370:2\n
$branch:itemcallnumber:<:600:3\n
$branch2:homebranch:ne:\$holdingbranch:X\n
$branch2:effective_itemtype:eq:CD:4\n
$branch2:itemcallnumber:>:600:5\n
RULES
Comment 18 Lari Strand 2025-01-23 11:41:30 UTC
Oops I was watching the wrong Koha version. The test rules are found at /Koha/t/db_dependent/SIP/Transaction.t line 1108 forwards in main branch.
Comment 19 Martin Renvoize (ashimema) 2025-02-19 09:57:47 UTC
Yikes, this is nasty.. my apologies for not thinking about line-ending differences sooner.
Comment 20 Martin Renvoize (ashimema) 2025-02-19 11:05:24 UTC
Created attachment 178317 [details] [review]
Bug 38375: Splitting SIP2SortBinMapping syspref does not support all operating systems' line endings

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize (ashimema) 2025-02-19 11:05:27 UTC
Created attachment 178318 [details] [review]
Bug 38375: Use perl 5.10+ \R
Comment 22 Martin Renvoize (ashimema) 2025-02-19 11:05:29 UTC
Created attachment 178319 [details] [review]
Bug 38375: Unit tests
Comment 23 Katrin Fischer 2025-02-19 16:07:37 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 24 Paul Derscheid 2025-03-19 23:01:07 UTC
Nice work everyone!

Pushed to 24.11.x for 24.11.03
Comment 25 Fridolin Somers 2025-06-19 08:41:38 UTC
Needed for 24.05.x ?