Bug 37255 - Creating default waiting hold cancellation policy for all patron categories and itemtypes breaks Koha
Summary: Creating default waiting hold cancellation policy for all patron categories a...
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Emmi Takkinen
QA Contact: Pedro Amorim
URL:
Keywords:
: 37531 (view as bug list)
Depends on: 36672
Blocks: 37764
  Show dependency treegraph
 
Reported: 2024-07-04 10:45 UTC by Emmi Takkinen
Modified: 2024-09-18 03:44 UTC (History)
10 users (show)

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


Attachments
Bug 37255: Fix handling of "All" values on waiting hold cancellation policy (2.36 KB, patch)
2024-07-04 11:51 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 37255: Fix handling of "All" values on waiting hold cancellation policy (2.41 KB, patch)
2024-07-05 01:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 37255: Fix handling of "All" values on waiting hold cancellation policy (2.47 KB, patch)
2024-07-05 14:38 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2024-07-04 10:45:11 UTC
If one creates a default waiting hold cancellation policy with patron categories set as "All" and itemtype set as "All", Koha breaks on 500 error. Logs read:

Template process failed: undef error - Can't call method "description" on an undefined value at ../Koha/Koha/Template/Plugin/Categories.pm line 39.
Comment 1 Emmi Takkinen 2024-07-04 11:51:01 UTC
Created attachment 168494 [details] [review]
Bug 37255: Fix handling of "All" values on waiting hold cancellation policy

If one creates a default waiting hold cancellation policy with
patron categories set as "All" and itemtype set as "All", Koha
breaks on 500 error. This happens because in we try to match
template policy with "All" values either in category or itemtype
with *, not undef. This patch fixes this.

To test:
1. Create a new default waiting hold cancellation policy and
set both patron category and itemtype as "All".
2. Save policy.
=> Error page for error 500 is displayed.
3. Apply this patch.
4. Reload page.
=> Page is displayed and policy listing displays new policy
as it should.

Sponsored-by: Koha-Suomi Oy
Comment 2 Chris Cormack 2024-07-05 01:18:56 UTC
Created attachment 168524 [details] [review]
Bug 37255: Fix handling of "All" values on waiting hold cancellation policy

If one creates a default waiting hold cancellation policy with
patron categories set as "All" and itemtype set as "All", Koha
breaks on 500 error. This happens because in we try to match
template policy with "All" values either in category or itemtype
with *, not undef. This patch fixes this.

To test:
1. Create a new default waiting hold cancellation policy and
set both patron category and itemtype as "All".
2. Save policy.
=> Error page for error 500 is displayed.
3. Apply this patch.
4. Reload page.
=> Page is displayed and policy listing displays new policy
as it should.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 3 Pedro Amorim 2024-07-05 14:38:35 UTC
Created attachment 168552 [details] [review]
Bug 37255: Fix handling of "All" values on waiting hold cancellation policy

If one creates a default waiting hold cancellation policy with
patron categories set as "All" and itemtype set as "All", Koha
breaks on 500 error. This happens because in we try to match
template policy with "All" values either in category or itemtype
with *, not undef. This patch fixes this.

To test:
1. Create a new default waiting hold cancellation policy and
set both patron category and itemtype as "All".
2. Save policy.
=> Error page for error 500 is displayed.
3. Apply this patch.
4. Reload page.
=> Page is displayed and policy listing displays new policy
as it should.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 4 Pedro Amorim 2024-07-05 14:44:16 UTC
Same pattern causing bug 37263.
Comment 5 Katrin Fischer 2024-07-08 15:50:50 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 6 Emily Lamancusa (emlam) 2024-07-31 15:12:05 UTC
Increasing the severity on this one and flagging it for backport, since it crashes the page.
Comment 7 Emily Lamancusa (emlam) 2024-08-09 19:06:35 UTC
*** Bug 37531 has been marked as a duplicate of this bug. ***
Comment 8 Tomás Cohen Arazi (tcohen) 2024-08-12 18:38:16 UTC
Pushed to main for 24.11.

Nice work everyone, thanks!
Comment 9 Tomás Cohen Arazi (tcohen) 2024-08-13 13:55:15 UTC
Pushed to 23.05.x.

Nice work everyone, thanks!
Comment 10 Tomás Cohen Arazi (tcohen) 2024-08-13 13:55:32 UTC
Pushed to 23.05.x.

Nice work everyone, thanks!
Comment 11 Tomás Cohen Arazi (tcohen) 2024-08-13 13:55:53 UTC
Pushed to 23.05.x.

Nice work everyone, thanks!
Comment 12 Emily Lamancusa (emlam) 2024-08-13 18:25:37 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> Pushed to 23.05.x.
> 
> Nice work everyone, thanks!

Git bz error? Or did this actually get pushed for 23.05.x?
Comment 13 Katrin Fischer 2024-08-13 18:52:09 UTC
(In reply to Emily Lamancusa from comment #12)
> (In reply to Tomás Cohen Arazi from comment #11)
> > Pushed to 23.05.x.
> > 
> > Nice work everyone, thanks!
> 
> Git bz error? Or did this actually get pushed for 23.05.x?

It will be part of the upcoming maintenance releases as an urgent backport. Stay tuned :)
Comment 14 Emily Lamancusa (emlam) 2024-08-13 18:56:33 UTC
(In reply to Katrin Fischer from comment #13)
> (In reply to Emily Lamancusa from comment #12)
> > (In reply to Tomás Cohen Arazi from comment #11)
> > > Pushed to 23.05.x.
> > > 
> > > Nice work everyone, thanks!
> > 
> > Git bz error? Or did this actually get pushed for 23.05.x?
> 
> It will be part of the upcoming maintenance releases as an urgent backport.
> Stay tuned :)

Ah, I see. It shouldn't be backported that far, though - the bug was caused by bug 36672, which is only in main and 24.05
Comment 15 Katrin Fischer 2024-08-13 19:08:13 UTC
Thanks for noticing Emily. 
I guess we can't leave it in? ...
Comment 16 Emily Lamancusa (emlam) 2024-08-13 19:23:45 UTC
Unfortunately, no. :( The cause of this regression was that a variable value was switched from '*' to undef, and a few cases got missed. So applying this patch to 23.05 causes the same 500 error that it's meant to solve in 24.05 and main.
Comment 17 Katrin Fischer 2024-08-13 19:59:15 UTC
It was more of a rhetoric question, I'll try to get that sorted ASAP.
Comment 18 Katrin Fischer 2024-08-14 07:11:57 UTC
Reverted the patch from 23.11, 23.05 and 22.11 branches. 
Thanks again Emily!
Comment 19 David Cook 2024-08-29 02:31:24 UTC
(In reply to Katrin Fischer from comment #18)
> Reverted the patch from 23.11, 23.05 and 22.11 branches. 
> Thanks again Emily!

I don't think this was reverted from 23.11.x, as it's still in 23.11.08.
Comment 20 Katrin Fischer 2024-08-29 07:55:22 UTC
(In reply to David Cook from comment #19)
> (In reply to Katrin Fischer from comment #18)
> > Reverted the patch from 23.11, 23.05 and 22.11 branches. 
> > Thanks again Emily!
> 
> I don't think this was reverted from 23.11.x, as it's still in 23.11.08.

You need to look at the latest tag: 23.11.08-1. That's what we used for the tarballs and packages. It has the revert on top:

https://git.koha-community.org/Koha-community/Koha/commits/tag/v23.11.08-1
Comment 21 Fridolin Somers 2024-09-10 08:01:15 UTC
Depends on Bug 36672 not in 23.11.x
Comment 22 Wainui Witika-Park 2024-09-18 03:44:09 UTC
Not backporting to 23.05.x unless requested