Bug 35293 - Regression: Bug 33390 (QA follow-up) patch overwrote the template changes to bug 25560
Summary: Regression: Bug 33390 (QA follow-up) patch overwrote the template changes to ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
: 35692 (view as bug list)
Depends on: 25560 30407
Blocks: 33431
  Show dependency treegraph
 
Reported: 2023-11-08 16:14 UTC by Lisette Scheer
Modified: 2024-02-23 14:39 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.02


Attachments
Bug 35293: Add ONLYMESSAGE back into UpdateNotForLoanStatusOnCheckin syspref description (2.83 KB, patch)
2023-11-12 21:54 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 35293: Re-implement bug 30407 functionality (1.83 KB, patch)
2023-11-12 21:57 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 35293: Re-implement bug 30407 functionality (2.03 KB, patch)
2023-12-28 22:40 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 35293: Add ONLYMESSAGE back into UpdateNotForLoanStatusOnCheckin syspref description (2.98 KB, patch)
2023-12-29 06:44 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35293: Re-implement bug 30407 functionality (2.18 KB, patch)
2023-12-29 06:45 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35293: Add ONLYMESSAGE back into UpdateNotForLoanStatusOnCheckin syspref description (3.03 KB, patch)
2024-01-04 19:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35293: Re-implement bug 30407 functionality (2.24 KB, patch)
2024-01-04 19:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35293: (follow-up) Handle all rules as well (2.25 KB, patch)
2024-01-04 19:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 35293: Unit test (2.04 KB, patch)
2024-01-04 19:25 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lisette Scheer 2023-11-08 16:14:50 UTC
After bug 25560, the description for UpdateNotForLoanStatusOnCheckin read:

>>   - "This is a list of item types and value pairs.<br/><br/>"
>>            - "<strong>Examples:</strong><br/>"
>>            - "_ALL_:<br/>"
>>            - "&nbsp-1: 0<br/><br/>"
>>            - "CR:<br/>"
>>            - "&nbsp1: 0<br/><br/>"
>>            - "When an item is checked in, if it's item type matches CR then when the value on the left (1) matches the items' not for loan value it will be updated to the value on the right.<br/><br/>The special term _ALL_ is used on the left side of the colon (:) to affect all item types, <strong>this does not override all other rules</strong><br/><br/>"
>>            - "Each item type needs to be defined on a separate line on the left side of the colon (:).<br/>Each pair of not for loan values, for that item type, should be listed on separate lines below the item type, each indented by a leading space<br/><br/>"

After Bug 33390 was pushed, it now reads again as: 

>>            - This is a list of value pairs. When an item is checked in, if the not for loan value on the left matches the items not for loan value
>>             - "it will be updated to the right-hand value. For example, '-1: 0' will cause an item that was set to 'Ordered' to now be available for loan."
>>             - "Can be used for showing only the not for loan description. E.g. '-1: ONLYMESSAGE'."
>>             - Each pair of values should be on a separate line.
Comment 1 Katrin Fischer 2023-11-08 16:20:14 UTC
This is a rebase issue most likely. If you look at my follow-up, you can see it only changed a line break (for nicer translatability).

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=158202

We still need to fix tho.
Comment 2 Katrin Fischer 2023-11-09 20:56:03 UTC
I think it's not as easy as it sounds. The patch you refer to removed the mention of ONLYMESSAGE making it an undocumented feature.

               type: textarea
               syntax: text/x-yaml
               class: code
-            - This is a list of value pairs. When an item is checked in, if the not for loan value on the left matches the items not for loan value
-            - "it will be updated to the right-hand value. For example, '-1: 0' will cause an item that was set to 'Ordered' to now be available for loan."
-            - "Can be used for showing only the not for loan description. E.g. '-1: ONLYMESSAGE'."
-            - Each pair of values should be on a separate line.
+            - "This is a list of item types and value pairs.<br/><br/>"
+            - "<strong>Examples:</strong><br/>"
+            - "_ALL_:<br/>"
+            - "&nbsp-1: 0<br/><br/>"
+            - "CR:<br/>"
+            - "&nbsp1: 0<br/><br/>"
+            - "When an item is checked in, if it's item type matches CR then when the value on the left (1) matches the items' not for loan value it will be updated to the value on the right.<br/><br/>The special term _ALL_ is used on the left side of the colon (:) to affect all item types, <strong>this does not override all other rules</strong><br/><br/>"
+            - "Each item type needs to be defined on a separate line on the left side of the colon (:).<br/>Each pair of not for loan values, for that item type, should be listed on separate lines below the item type, each indented by a leading space<br/><br/>"
Comment 3 Katrin Fischer 2023-11-09 20:58:14 UTC
We are about to string freeze tomorrow and I am not sure I understand it all well enough yet :(

Alex, could you confirm that ONLYMESSAGE should remain?

It was added as a feature by bug 30407.
Comment 4 Alex Buckley 2023-11-12 19:05:51 UTC
(In reply to Katrin Fischer from comment #3)
> We are about to string freeze tomorrow and I am not sure I understand it all
> well enough yet :(
> 
> Alex, could you confirm that ONLYMESSAGE should remain?
> 
> It was added as a feature by bug 30407.

Hi Katrin, 

My apologies for my slow reply.

I've checked and yes can confirm ONLYMESSAGE should remain. The way I confirmed this for sure was running the t/db_dependent/Circulation/issue.t unit test file, which passes both for my changes in bug 25560 and the ONLYMESSAGE changes in bug 30407.

Please let me know if there is anything I can do to help, and apologies again for my delay.
Comment 5 Katrin Fischer 2023-11-12 19:39:52 UTC
If you have a moment to write the merged/combined system preference description I could sign off on it in the morning. But we are already in string freeze.
Comment 6 Alex Buckley 2023-11-12 19:43:26 UTC
(In reply to Katrin Fischer from comment #5)
> If you have a moment to write the merged/combined system preference
> description I could sign off on it in the morning. But we are already in
> string freeze.

Hi Katrin, 

Thanks for that.

Yes sure thing. Writing that now, attaching shortly
Comment 7 Alex Buckley 2023-11-12 21:54:30 UTC
Created attachment 158850 [details] [review]
Bug 35293: Add ONLYMESSAGE back into UpdateNotForLoanStatusOnCheckin syspref description

Sponsored-by: Catalyst IT, New Zealand
Comment 8 Alex Buckley 2023-11-12 21:57:13 UTC
Created attachment 158851 [details] [review]
Bug 35293: Re-implement bug 30407 functionality

To test:
1. Set items notforloan status as -1 (or create new one)
2. Add line "-1: ONLYMESSAGE" to UpdateNotForLoanStatusOnCheckin
3. Check item out for patron.
4. Check item in.
=> Description of notforloan status should be displayed.
=> Confirm notforloan status HAS changed.

5. Apply patch and update database if needed
6. Set items notforloan status as -1 (or create new one)
7. Check item out for patron.
8. Check item in.
=> Description of notforloan status should be displayed.
=> Confirm notforloan status HAS NOT changed.

Also prove t/db_dependent/Circulation/issue.t

Sponsored-by: Catalyst IT, New Zealand
Comment 9 Alex Buckley 2023-11-12 22:46:36 UTC
Hi Katrin, 

I have attached a patch for combining the system preference description. 

I found that although the issue.t unit test passed successfully on master, the behaviour for the user in the bug 30407 test plan does not actually happen on master.
i.e. when you check in an item it's notforloan status does change when it shouldn't.

So I have attached a second patch which re-implements the behaviour from bug 30407 - not sure why that was lost on master. 

Please feel free to test or handle these patches as you see fit. I hope they are of some help.

Thanks
Alex
Comment 10 Anneli Österman 2023-11-17 12:44:35 UTC
We tested this but could not reproduce the problem.

1. Notforloan item cannot be checked out with the provided systempreferences.
2. If you check in a not for loan item without checkin in it first, Koha only tells that the item was not onloan.
3. If you allow checkin out of notforloan items and then do the check out and check in, Koha does not tell anything about the notforloan status. Even when ONLYMESSAGE is configured to UpdateNotForLoanStatusOnCheckin. 
4. The notforloan status did not dissapear on check in.
Comment 11 Alex Buckley 2023-12-28 22:40:44 UTC
Created attachment 160360 [details] [review]
Bug 35293: Re-implement bug 30407 functionality

To test:
1. Set a book items notforloan status as -1 and set the AllowNotForLoanOverride system preference to 'Allow'
2. Add the following lines to UpdateNotForLoanStatusOnCheckin (the
formatting and indentation is important to keep):

BK:
 -1: ONLYMESSAGE

3. Check a book item out for patron.
4. Check book item in.
=> Description of notforloan status should be displayed under 'Check in message'.
=> Confirm notforloan status HAS changed.

5. Apply patch and update database if needed
6. Set book items notforloan status as -1 (or create new one)
7. Check book item out for patron.
8. Check book item in.
=> Description of notforloan status should be displayed under 'Check in
message'.
=> Confirm notforloan status HAS NOT changed.

Also prove t/db_dependent/Circulation/issue.t

Sponsored-by: Catalyst IT, New Zealand
Comment 12 Alex Buckley 2023-12-28 22:42:22 UTC
(In reply to Anneli Österman from comment #10)
> We tested this but could not reproduce the problem.
> 
> 1. Notforloan item cannot be checked out with the provided systempreferences.
> 2. If you check in a not for loan item without checkin in it first, Koha
> only tells that the item was not onloan.
> 3. If you allow checkin out of notforloan items and then do the check out
> and check in, Koha does not tell anything about the notforloan status. Even
> when ONLYMESSAGE is configured to UpdateNotForLoanStatusOnCheckin. 
> 4. The notforloan status did not dissapear on check in.

Hi Anneli, 

Happy holidays!

Thank you for testing. I've tweaked the test plan - to include adding the item type into the UpdateNotForLoanStatusOnCheckin syspref then issuing and returning an item of that type. 

Would you mind please re-testing?

Thanks very much,
Alex
Comment 13 PTFS Europe Sandboxes 2023-12-29 06:44:59 UTC
Created attachment 160361 [details] [review]
Bug 35293: Add ONLYMESSAGE back into UpdateNotForLoanStatusOnCheckin syspref description

Sponsored-by: Catalyst IT, New Zealand
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 14 PTFS Europe Sandboxes 2023-12-29 06:45:02 UTC
Created attachment 160362 [details] [review]
Bug 35293: Re-implement bug 30407 functionality

To test:
1. Set a book items notforloan status as -1 and set the AllowNotForLoanOverride system preference to 'Allow'
2. Add the following lines to UpdateNotForLoanStatusOnCheckin (the
formatting and indentation is important to keep):

BK:
 -1: ONLYMESSAGE

3. Check a book item out for patron.
4. Check book item in.
=> Description of notforloan status should be displayed under 'Check in message'.
=> Confirm notforloan status HAS changed.

5. Apply patch and update database if needed
6. Set book items notforloan status as -1 (or create new one)
7. Check book item out for patron.
8. Check book item in.
=> Description of notforloan status should be displayed under 'Check in
message'.
=> Confirm notforloan status HAS NOT changed.

Also prove t/db_dependent/Circulation/issue.t

Sponsored-by: Catalyst IT, New Zealand
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 15 Nick Clemens (kidclamp) 2024-01-04 19:25:31 UTC
Created attachment 160553 [details] [review]
Bug 35293: Add ONLYMESSAGE back into UpdateNotForLoanStatusOnCheckin syspref description

Sponsored-by: Catalyst IT, New Zealand
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens (kidclamp) 2024-01-04 19:25:33 UTC
Created attachment 160554 [details] [review]
Bug 35293: Re-implement bug 30407 functionality

To test:
1. Set a book items notforloan status as -1 and set the AllowNotForLoanOverride system preference to 'Allow'
2. Add the following lines to UpdateNotForLoanStatusOnCheckin (the
formatting and indentation is important to keep):

BK:
 -1: ONLYMESSAGE

3. Check a book item out for patron.
4. Check book item in.
=> Description of notforloan status should be displayed under 'Check in message'.
=> Confirm notforloan status HAS changed.

5. Apply patch and update database if needed
6. Set book items notforloan status as -1 (or create new one)
7. Check book item out for patron.
8. Check book item in.
=> Description of notforloan status should be displayed under 'Check in
message'.
=> Confirm notforloan status HAS NOT changed.

Also prove t/db_dependent/Circulation/issue.t

Sponsored-by: Catalyst IT, New Zealand
Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens (kidclamp) 2024-01-04 19:25:35 UTC
Created attachment 160555 [details] [review]
Bug 35293: (follow-up) Handle all rules as well

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens (kidclamp) 2024-01-04 19:25:37 UTC
Created attachment 160556 [details] [review]
Bug 35293: Unit test

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens (kidclamp) 2024-01-04 19:25:49 UTC
*** Bug 35692 has been marked as a duplicate of this bug. ***
Comment 20 Katrin Fischer 2024-01-11 11:12:00 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 21 Fridolin Somers 2024-01-15 13:39:56 UTC
Pushed to 23.11.x for 23.11.02
Comment 22 Lucas Gass 2024-01-19 20:56:10 UTC
Missing dependencies for 23.05.x, no backport.