Bug 24669 - Editing circulation rule breaks holds when total holds unlimited
Summary: Editing circulation rule breaks holds when total holds unlimited
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
Depends on: 18936
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-14 21:13 UTC by Katrin Fischer
Modified: 2022-11-17 08:28 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:
20.05.00


Attachments
Bug 24669: Improve numeric input handling in smart-rules.pl (5.71 KB, patch)
2020-02-17 11:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 24669: Show "Unlimited" values correctly in smart-rules.pl (3.21 KB, patch)
2020-02-17 11:00 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 24669: Improve numeric input handling in smart-rules.pl (5.78 KB, patch)
2020-02-18 10:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24669: Show "Unlimited" values correctly in smart-rules.pl (3.29 KB, patch)
2020-02-18 10:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24669: Improve numeric input handling in smart-rules.pl (5.85 KB, patch)
2020-02-18 19:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24669: Show "Unlimited" values correctly in smart-rules.pl (3.34 KB, patch)
2020-02-18 19:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24669: Fix selenium test (1.22 KB, patch)
2020-02-19 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2020-02-14 21:13:57 UTC
Add a new rule, leave holds values empty and save.
After saving, the values show in the table as 'unlimitd'
Edit the rule, the editor shows the values not empty, but the string 'unlimited'.
Saving the rule now without fixing this means you can no longer place holds.
Comment 1 Joonas Kylmälä 2020-02-17 11:00:38 UTC
Created attachment 99079 [details] [review]
Bug 24669: Improve numeric input handling in smart-rules.pl

This removes code duplication by introducing a new subroutine
strip_non_numeric() that removes whitespace and makes the rule value
'' if it is a string value instead of a digit. The call to
strip_non_numeric() is now added also to all the rules it is needed
in.

To test:
1. Write some string like "test" to rules:
  - Current checkouts allowed
  - Current on-site checkouts allowed
  - Holds allowed (total)
  - Holds allowed (daily)
  - Holds per record (count)
  - Holds per record (count) – can be found in default policy
2. Save rules
3. Apply patch
4. Edit the rule line where you just typed all those strings and save
5. Notice how the values are now empty strings (some say "unlimited" but
   if you go check from DB it should show empty value there)

Sponsored-by: The National Library of Finland
Comment 2 Joonas Kylmälä 2020-02-17 11:00:41 UTC
Created attachment 99080 [details] [review]
Bug 24669: Show "Unlimited" values correctly in smart-rules.pl

The rules "Holds allowed (total)", "Holds allowed (daily)", "Holds per
record (count)" were not set empty when in rule editing. Also, the
text "Unlimited" was not shown for rule "Holds per record (count)"
even though it should have been since empty value means unlimited
holds per record.

To test:
1. Apply patch
2. Edit the rules "Holds allowed (total)", "Holds allowed (daily)", "Holds per
record (count)" to be empty and save
3. Make sure all of the above rules now say Unlimited
4. Edit the rules again and make sure the above rules are now in the
rule editing empty.

Sponsored-by: The National Library of Finland
Comment 3 Jonathan Druart 2020-02-18 10:50:00 UTC
Created attachment 99171 [details] [review]
Bug 24669: Improve numeric input handling in smart-rules.pl

This removes code duplication by introducing a new subroutine
strip_non_numeric() that removes whitespace and makes the rule value
'' if it is a string value instead of a digit. The call to
strip_non_numeric() is now added also to all the rules it is needed
in.

To test:
1. Write some string like "test" to rules:
  - Current checkouts allowed
  - Current on-site checkouts allowed
  - Holds allowed (total)
  - Holds allowed (daily)
  - Holds per record (count)
  - Holds per record (count) – can be found in default policy
2. Save rules
3. Apply patch
4. Edit the rule line where you just typed all those strings and save
5. Notice how the values are now empty strings (some say "unlimited" but
   if you go check from DB it should show empty value there)

Sponsored-by: The National Library of Finland

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 4 Jonathan Druart 2020-02-18 10:50:04 UTC
Created attachment 99172 [details] [review]
Bug 24669: Show "Unlimited" values correctly in smart-rules.pl

The rules "Holds allowed (total)", "Holds allowed (daily)", "Holds per
record (count)" were not set empty when in rule editing. Also, the
text "Unlimited" was not shown for rule "Holds per record (count)"
even though it should have been since empty value means unlimited
holds per record.

To test:
1. Apply patch
2. Edit the rules "Holds allowed (total)", "Holds allowed (daily)", "Holds per
record (count)" to be empty and save
3. Make sure all of the above rules now say Unlimited
4. Edit the rules again and make sure the above rules are now in the
rule editing empty.

Sponsored-by: The National Library of Finland

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 5 Martin Renvoize 2020-02-18 19:04:14 UTC
Created attachment 99216 [details] [review]
Bug 24669: Improve numeric input handling in smart-rules.pl

This removes code duplication by introducing a new subroutine
strip_non_numeric() that removes whitespace and makes the rule value
'' if it is a string value instead of a digit. The call to
strip_non_numeric() is now added also to all the rules it is needed
in.

To test:
1. Write some string like "test" to rules:
  - Current checkouts allowed
  - Current on-site checkouts allowed
  - Holds allowed (total)
  - Holds allowed (daily)
  - Holds per record (count)
  - Holds per record (count) – can be found in default policy
2. Save rules
3. Apply patch
4. Edit the rule line where you just typed all those strings and save
5. Notice how the values are now empty strings (some say "unlimited" but
   if you go check from DB it should show empty value there)

Sponsored-by: The National Library of Finland
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2020-02-18 19:04:17 UTC
Created attachment 99217 [details] [review]
Bug 24669: Show "Unlimited" values correctly in smart-rules.pl

The rules "Holds allowed (total)", "Holds allowed (daily)", "Holds per
record (count)" were not set empty when in rule editing. Also, the
text "Unlimited" was not shown for rule "Holds per record (count)"
even though it should have been since empty value means unlimited
holds per record.

To test:
1. Apply patch
2. Edit the rules "Holds allowed (total)", "Holds allowed (daily)", "Holds per
record (count)" to be empty and save
3. Make sure all of the above rules now say Unlimited
4. Edit the rules again and make sure the above rules are now in the
rule editing empty.

Sponsored-by: The National Library of Finland
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-02-18 19:05:04 UTC
Thanks for the quick followup here Joonas, Passing QA
Comment 8 Martin Renvoize 2020-02-19 11:35:17 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Jonathan Druart 2020-02-19 14:02:50 UTC
Created attachment 99251 [details] [review]
Bug 24669: Fix selenium test

There is one more "Unlimited" on the circ rules now.
Comment 10 Joy Nelson 2020-03-13 00:12:05 UTC
not backporting to 19.11.x due to missing dependencies