Bug 25816

Created attachment 106060 [details] [review]
Bug 25816: Add opac messages in SIP display.

Test plan:

 - Add one or more opac messages to a patron
   (patron detail -> add message -> Add a message for: OPAC)

 - Check that the messages will be displayed, with correctly formatted dates.

You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.

Created attachment 139688 [details] [review]
Bug 25816: Add opac messages in SIP display.

Test plan:

 - Add one or more opac messages to a patron
   (patron detail -> add message -> Add a message for: OPAC)

 - Check that the messages will be displayed, with correctly formatted dates.

You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Works as expected.. signing off.

QA comment.. we should probably have Unit Tests for this. Also, my brain fails me right now; but are there any characters we should be escaping/filtering from messages here?

(In reply to Martin Renvoize from comment #3)
> Works as expected.. signing off.
> 
> QA comment.. we should probably have Unit Tests for this. Also, my brain
> fails me right now; but are there any characters we should be
> escaping/filtering from messages here?

Agreed with the unit test. Also, I wonder if this should be controlled by yet another sip account level switch in the sip config. Characters should already get escaped automatically.

However, it's not working for me atm:
CLI:
./misc/sip_cli_emulator.pl -a 127.0.0.1 -su koha -sp koha -l MPL --patron 23529000035676 -m patron_information
Attempting socket connection to 127.0.0.1:6001...connected!
Use of uninitialized value $hold_mode in string eq at ./misc/sip_cli_emulator.pl line 120.
SEND: 9300CNkoha|COkoha|CPMPL|
READ: 941

Trying 'patron_information'
SEND: 6300120220823    155418          AOMPL|AA23529000035676|ACkoha|
Use of uninitialized value $data in concatenation (.) or string at ./misc/sip_cli_emulator.pl line 352, <GEN0> chunk 1.
READ:

Logs:
[2022/08/23 15:55:51] [26838] [DEBUG] [undef]@[undef]: Configuration::find_service: Trying 6001/tcp C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:51] [26838] [DEBUG] [undef]@[undef]: Configuration::find_service: Trying *:6001/tcp C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:51] [26838] [DEBUG] [undef]@[undef]: Configuration::find_service: Trying 127.0.0.1:6001/tcp C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:51] [26838] [DEBUG] [undef]@[undef]: raw_transport: timeout is 60 C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:51] [26838] [DEBUG] [undef]@[undef]: read_request trimmed 1 character(s)  C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:51] [26838] [INFO] [undef]@[undef]: INPUT MSG: '9300CNkoha|COkoha|CPMPL' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:51] [26838] [DEBUG] [undef]@[undef]: Sip::MsgType::new('C4::SIP::Sip::MsgType', '9300CNkoha...', '93'): seq.no '0', protocol 2 C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:51] [26838] [DEBUG] [undef]@[undef]: Sip::MsgType::_initialize('Login', '00CNkoha|COkoha|CPMPL', 'A1A1', '2', ...) C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [INFO] [undef]@[undef]: Successful login/auth for 'koha' of 'kohalibrary' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [DEBUG] [undef]@[undef]: login_core: $VAR1 = 'ILS';
 C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [DEBUG] [undef]@[undef]: new ILS 'kohalibrary' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [INFO] [undef]@[undef]: OUTPUT MSG: '941' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [DEBUG] koha@127.0.0.1: raw_transport: uname/inst: 'koha/kohalibrary' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [DEBUG] koha@127.0.0.1: read_request trimmed 2 character(s)  C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [INFO] koha@127.0.0.1: INPUT MSG: '6300120220823    155551          AOMPL|AA23529000035676|ACkoha' C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [DEBUG] koha@127.0.0.1: Sip::MsgType::new('C4::SIP::Sip::MsgType', '6300120220...', '63'): seq.no '0', protocol 2 C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [DEBUG] koha@127.0.0.1: Sip::MsgType::_initialize('Patron Info', '00120220823    155551          AOMPL|AA23529000035676|ACkoha', 'A3A18A10', '31', ...) C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)
[2022/08/23 15:55:52] [26838] [INFO] koha@127.0.0.1: raw_transport: shutting down C4::SIP::Sip::siplog /kohadevbox/koha/C4/SIP/Sip.pm (223)

Created attachment 154545 [details] [review]
Bug 25816: Add opac messages in SIP display.

Test plan:

 - Add one or more opac messages to a patron
   (patron detail -> add message -> Add a message for: OPAC)

 - Check that the messages will be displayed, with correctly formatted dates.

You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Hello !

I've rebased the patch against master, and added unit tests.

Kyle, could you have another try now that it is rebased?

It's working for me with the rebased patch:

kohadev-koha@kohadevbox:/kohadevbox/koha/misc$ ./sip_cli_emulator.pl -a 127.0.0.1 -su koha -sp koha -l MPL --patron 23529000035676 -m patron_information
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: 9300CNkoha|COkoha|CPMPL|
READ: 941

Trying 'patron_information'
SEND: 6300120230817    113324          AOMPL|AA23529000035676|ACkoha|
READ: 64              00120230817    113324000000000000000000000000AOkohalibrary|AA23529000035676|AEHenry Acevedo|BLY|BV0|CC5|BD4345 Library Rd. Springfield, MA 44224|BF(212) 555-1212|PB19580530|PCS|PIY|AFGreetings from Koha. . Messages for you: 08/17/2023: My message|

|AFGreetings from Koha. |

becomes:

|AFGreetings from Koha. . Messages for you: 09/11/2023: OPAC MESSAGE|

There's an extra period in there. I think you'll need to tweak how you combine everything. You might consider using join on everything.

I'm going to say I don't see how this can pass qa without at least a simple on/off switch in the SIP config at the account level. A change like this could break existing systems.

(In reply to Kyle M Hall from comment #7)
> |AFGreetings from Koha. |
> 
> becomes:
> 
> |AFGreetings from Koha. . Messages for you: 09/11/2023: OPAC MESSAGE|
> 
> There's an extra period in there. I think you'll need to tweak how you
> combine everything. You might consider using join on everything.
> 
> I'm going to say I don't see how this can pass qa without at least a simple
> on/off switch in the SIP config at the account level. A change like this
> could break existing systems.

After reviewing the code, I think making this an account level setting is overly difficult. I think a system preference would be acceptable.

Created attachment 155592 [details] [review]
Bug 25816: Add opac messages in SIP display.

Test plan:

 - Add one or more opac messages to a patron
   (patron detail -> add message -> Add a message for: OPAC)

 - Check that the messages will be displayed, with correctly formatted dates.

You can use src/C4/SIP/interactive_patron_dump.pl for easier testing.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 155596 [details] [review]
Bug 25816: (QA follow-up) Add system preference

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thanks for the follow-up, Kyle!

Pushed to master for 23.11.

Nice work everyone, thanks!

Enhancement not pushed to 23.05.x