Bug 25808

Summary: Renewal via the SIP 'checkout' message gives incorrect message
Product: Koha Reporter: Kyle M Hall <kyle>
Component: SIP2Assignee: Kyle M Hall <kyle>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrew, caroline.cyr-la-rose, colin.campbell, fridolin.somers, jonathan.druart, martin.renvoize, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09
Attachments: Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message
Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message
Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message
Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message
Bug 25808: (QA follow-up) Add regression test
Bug 25808: (QA follow-up) Fix unit tests

Description Kyle M Hall 2020-06-18 17:21:46 UTC
If a renewal via SIP cannot be made because the patron has reached the maximum number of renewals, the AF screen message should read "Item has reached maximum renewals!" but instead, we get "Item checked out to another patron".
Comment 1 Kyle M Hall 2020-06-18 17:25:52 UTC
Created attachment 106028 [details] [review]
Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message

If a renewal via SIP cannot be made because the patron has reached the maximum number of renewals,
the AF screen message should read "Item has reached maximum renewals!",
instead we get "Item checked out to another patron".

Test Plan:
1) Check out an item to a patron
2) Using the SIP CLI tool, run checkout messages until the checkout has
   reached the maximum number of renewals
3) Note the incorrect message in the AF field
4) Apply this patch
5) Restart the SIP server
6) Run another SIP checkout message
7) Note the message is now correct!
Comment 2 Kyle M Hall 2020-08-26 15:08:51 UTC
Created attachment 109188 [details] [review]
Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message

If a renewal via SIP cannot be made because the patron has reached the maximum number of renewals,
the AF screen message should read "Item has reached maximum renewals!",
instead we get "Item checked out to another patron".

Test Plan:
1) Check out an item to a patron
2) Using the SIP CLI tool, run checkout messages until the checkout has
   reached the maximum number of renewals
3) Note the incorrect message in the AF field
4) Apply this patch
5) Restart the SIP server
6) Run another SIP checkout message
7) Note the message is now correct!
Comment 3 Victor Grousset/tuxayo 2020-12-09 18:14:52 UTC
Created attachment 114289 [details] [review]
Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message

If a renewal via SIP cannot be made because the patron has reached the maximum number of renewals,
the AF screen message should read "Item has reached maximum renewals!",
instead we get "Item checked out to another patron".

Test Plan:
1) Check out an item to a patron
2) Using the SIP CLI tool, run checkout messages until the checkout has
   reached the maximum number of renewals
3) Note the incorrect message in the AF field
4) Apply this patch
5) Restart the SIP server
6) Run another SIP checkout message
7) Note the message is now correct!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 4 Victor Grousset/tuxayo 2020-12-09 18:18:31 UTC
It works :)

Testing notes:
https://wiki.koha-community.org/wiki/Koha_SIP2_server_setup#Example

./misc/sip_cli_emulator.pl -a 127.0.0.1 -p 6001 -su term1 -sp term1 \
-l CPL --patron 23529001000463 -m checkout --item 39999000001259

Same command for 1) and 2) and 6)
Comment 5 Martin Renvoize 2021-01-06 16:32:01 UTC
Created attachment 114898 [details] [review]
Bug 25808: Renewal via the SIP 'checkout' message gives incorrect message

If a renewal via SIP cannot be made because the patron has reached the maximum number of renewals,
the AF screen message should read "Item has reached maximum renewals!",
instead we get "Item checked out to another patron".

Test Plan:
1) Check out an item to a patron
2) Using the SIP CLI tool, run checkout messages until the checkout has
   reached the maximum number of renewals
3) Note the incorrect message in the AF field
4) Apply this patch
5) Restart the SIP server
6) Run another SIP checkout message
7) Note the message is now correct!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-01-06 16:34:00 UTC
We're early in the cycle.. whilst this works.. I'd really like to see an accompanying unit test... t/db_dependant/SIP/ILS.t exists and there's prior are for adding tests for a SIP function in there 'cancel_hold'... any chance you could add a subtest for the basics of 'checkout' there to prevent future regressions like this Kyle?

Failing for now to get feedback on the above.
Comment 7 Martin Renvoize 2021-01-14 14:41:33 UTC
Created attachment 115161 [details] [review]
Bug 25808: (QA follow-up) Add regression test

This patch adds a basic regression test for the bug.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2021-01-14 14:41:58 UTC
I came back and wrote the test for you.. Passing QA
Comment 9 Martin Renvoize 2021-01-14 14:42:32 UTC
*** Bug 26375 has been marked as a duplicate of this bug. ***
Comment 10 Jonathan Druart 2021-01-19 14:44:29 UTC
The test is failing for me

t/db_dependent/SIP/ILS.t .. 2/11 
    #   Failed test 'Renewals has been reduced'
    #   at t/db_dependent/SIP/ILS.t line 173.
    #          got: '0'
    #     expected: '1'
    # Looks like you planned 1 test but ran 4.
    # Looks like you failed 1 test of 4 run.
t/db_dependent/SIP/ILS.t .. 11/11
Comment 11 Jonathan Druart 2021-01-19 14:44:54 UTC
Also QA failure

 FAIL   t/db_dependent/SIP/ILS.t
   FAIL   forbidden patterns
                forbidden pattern: trailing space char (line 166)
Comment 12 Kyle M Hall 2021-01-20 14:35:54 UTC
Created attachment 115436 [details] [review]
Bug 25808: (QA follow-up) Fix unit tests
Comment 13 Jonathan Druart 2021-01-21 15:25:09 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 14 Fridolin Somers 2021-01-25 15:27:08 UTC
Pushed to 20.11.x for 20.11.03
Comment 15 Andrew Fuerste-Henry 2021-02-01 21:53:03 UTC
Pushed to 20.05.x for 20.05.09
Comment 16 Victor Grousset/tuxayo 2021-02-02 01:20:38 UTC
Not backported to oldoldstable (19.11.x), tests don't pass.
If needed for 19.11, please send a patch. Note that it's not a major bug so it's not a big deal if it's not backported. Just tried to backport since I already knew the test plan.

t/db_dependent/SIP/ILS.t .. 2/11 
    #   Failed test 'We get a success message when issue is renewed'
    #   at t/db_dependent/SIP/ILS.t line 170.
    #          got: 'Issue failed : NO_MORE_RENEWALS'
    #     expected: 'Item already checked out to you: renewing item.'

    #   Failed test 'Renewals has been reduced'
    #   at t/db_dependent/SIP/ILS.t line 173.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 2 tests of 4.
t/db_dependent/SIP/ILS.t .. 11/11 
#   Failed test 'checkout'
#   at t/db_dependent/SIP/ILS.t line 174.
# Looks like you failed 1 test of 11.

For the first failure, it seems the messages where simpler (NO_MORE_RENEWALS) in 19.11 which can be fixed in the test, however this is not the same semantic between the actual and expected message.