Bug 24602 - The fallback value for onshelfholds should be 0
Summary: The fallback value for onshelfholds should be 0
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 18936
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-06 14:08 UTC by Tomás Cohen Arazi
Modified: 2020-11-30 21:45 UTC (History)
2 users (show)

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


Attachments
Bug 24602: Tests for get_onshlefholds_policy (2.62 KB, patch)
2020-02-06 14:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24602: Make get_onshelfholds_policy fallback to 0 (1.50 KB, patch)
2020-02-06 14:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24602: Tests for get_onshlefholds_policy (2.67 KB, patch)
2020-02-07 11:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 24602: Make get_onshelfholds_policy fallback to 0 (1.55 KB, patch)
2020-02-07 11:23 UTC, David Nind
Details | Diff | Splinter Review
Bug 24602: Tests for get_onshlefholds_policy (2.67 KB, patch)
2020-02-07 18:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24602: Make get_onshelfholds_policy fallback to 0 (1.55 KB, patch)
2020-02-07 18:36 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-02-06 14:08:22 UTC
The onshelfholds circulation rules values are more of an ENUM type (as opposed to other rules with integers like max*qty). In the quantities cases, it makes sense to have undef mean unlimited. In the enum type it is clearer to just set a default value (0) as the code already expects (Note: undef and 0 both eval to false so there's no behaviour problem, but warnings or we get forced to check for defined when the rule is used. It seems trivial to just make the rule fallback to a chosen default value)
Comment 1 Tomás Cohen Arazi 2020-02-06 14:19:03 UTC
Created attachment 98532 [details] [review]
Bug 24602: Tests for get_onshlefholds_policy

Ths patch introduces tests for get_onshelf_policy.
To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/CirculationRules.t
=> FAIL: Tests fail because the current code returns undef instead of 0
Comment 2 Tomás Cohen Arazi 2020-02-06 14:19:07 UTC
Created attachment 98533 [details] [review]
Bug 24602: Make get_onshelfholds_policy fallback to 0

The onshelfholds circulation rules values are more of an ENUM type (as
opposed to other rules with integers like max*qty). In the
quantities cases, it makes sense to have undef mean unlimited. In the
enum type it is clearer to just set a default value (0) as the code
already expects (Note: undef and 0 both eval to false so there's no
behaviour problem, but warnings or we get forced to check for
defined when the rule is used. It seems trivial to just make the
rule fallback to a chosen default value).

This patch makes it fallback to 0.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/CirculationRules.t \
           t/db_dependent/Reserves.t
=> FAIL: CirculationRules.t fails and Reserves.t prints a warning
2. Apply this patch
3. Repeat (1)
=> SUCCESS: Tests pass! No warning!
4. Sign off :-D
Comment 3 David Nind 2020-02-07 11:02:27 UTC
Is this a warning?

root@8881cb995d76:koha(bz24602)$ prove t/db_dependent/Reserves.t
t/db_dependent/Reserves.t .. 8/62 Argument "book_reserved" isn't numeric in numeric eq (==) at /kohadevbox/koha/C4/Items.pm line 1776.
t/db_dependent/Reserves.t .. ok     
All tests successful.
Files=1, Tests=62,  8 wallclock secs ( 0.04 usr  0.00 sys +  6.64 cusr  0.63 csys =  7.31 CPU)
Result: PASS
Comment 4 Tomás Cohen Arazi 2020-02-07 11:06:42 UTC
(In reply to David Nind from comment #3)
> Is this a warning?
> 
> root@8881cb995d76:koha(bz24602)$ prove t/db_dependent/Reserves.t
> t/db_dependent/Reserves.t .. 8/62 Argument "book_reserved" isn't numeric in
> numeric eq (==) at /kohadevbox/koha/C4/Items.pm line 1776.
> t/db_dependent/Reserves.t .. ok     
> All tests successful.
> Files=1, Tests=62,  8 wallclock secs ( 0.04 usr  0.00 sys +  6.64 cusr  0.63
> csys =  7.31 CPU)
> Result: PASS

That's new, this is the warning I'm referring to:

Use of uninitialized value $on_shelf_holds in numeric eq (==) at /kohadevbox/koha/C4/Reserves.pm line 1239.
Comment 5 David Nind 2020-02-07 11:23:34 UTC
Created attachment 98554 [details] [review]
Bug 24602: Tests for get_onshlefholds_policy

Ths patch introduces tests for get_onshelf_policy.
To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/CirculationRules.t
=> FAIL: Tests fail because the current code returns undef instead of 0

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2020-02-07 11:23:37 UTC
Created attachment 98555 [details] [review]
Bug 24602: Make get_onshelfholds_policy fallback to 0

The onshelfholds circulation rules values are more of an ENUM type (as
opposed to other rules with integers like max*qty). In the
quantities cases, it makes sense to have undef mean unlimited. In the
enum type it is clearer to just set a default value (0) as the code
already expects (Note: undef and 0 both eval to false so there's no
behaviour problem, but warnings or we get forced to check for
defined when the rule is used. It seems trivial to just make the
rule fallback to a chosen default value).

This patch makes it fallback to 0.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/CirculationRules.t \
           t/db_dependent/Reserves.t
=> FAIL: CirculationRules.t fails and Reserves.t prints a warning
2. Apply this patch
3. Repeat (1)
=> SUCCESS: Tests pass! No warning!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Nick Clemens 2020-02-07 18:36:36 UTC
Created attachment 98578 [details] [review]
Bug 24602: Tests for get_onshlefholds_policy

Ths patch introduces tests for get_onshelf_policy.
To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/CirculationRules.t
=> FAIL: Tests fail because the current code returns undef instead of 0

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Nick Clemens 2020-02-07 18:36:40 UTC
Created attachment 98579 [details] [review]
Bug 24602: Make get_onshelfholds_policy fallback to 0

The onshelfholds circulation rules values are more of an ENUM type (as
opposed to other rules with integers like max*qty). In the
quantities cases, it makes sense to have undef mean unlimited. In the
enum type it is clearer to just set a default value (0) as the code
already expects (Note: undef and 0 both eval to false so there's no
behaviour problem, but warnings or we get forced to check for
defined when the rule is used. It seems trivial to just make the
rule fallback to a chosen default value).

This patch makes it fallback to 0.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/CirculationRules.t \
           t/db_dependent/Reserves.t
=> FAIL: CirculationRules.t fails and Reserves.t prints a warning
2. Apply this patch
3. Repeat (1)
=> SUCCESS: Tests pass! No warning!
4. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Martin Renvoize 2020-02-10 09:23:48 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 10 Joy Nelson 2020-03-05 00:12:23 UTC
not pushed to 19.11.x branch due to dependencies