Bug 12019: ensure that it is optional to define an owner if a fund is restricted

Before this patch, the C4::Budgets::CanUserUseBudget assumed that
budget_owner_id was set if a restriction (budget_permission) exists.
see
        && $budget->{budget_owner_id}
        && $budget->{budget_owner_id} != $borrower->{borrowernumber}

Actually a restriction could exists on users and/or library without
being forced to define an owner.

Test plan:
Create a fund A without restriction
Create a fund B restricted to an owner
Create a fund C restricted to a non defined owner
Create a fund D restricted to owner and users (try defining/no
defining an owner and/or users)
Create a fund E restricted to owner, users and library (try
defining/no defined an owner and/or users)

With different logged in users, try to show/edit these differents funds.
The restriction should be correctly applied.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested various permission combinatons, visibility of funds is now ok.
- not superlibrarian, no buget_manage_all,
  no owner, no users, no library, no restrictions on the fund
  = visible
- changed: library = staff patron library,
  restriction = Owner, users and library
  = visible
- changed: library = not staff patron library
  = invisible
- changed: budget_manage_all
  = visible
- changed: owner = staff patron
  no budget_manage_all
  = visible
- changed: no owner, user = staff patron
  = visible
- changed: no user, owner = another user, restriction = owner
  = invisible
- changed: budget_manage_all
  = visible
- changed: no budget_manage_all but superlibrarian
  = visible
...

Passes tests and QA script, also t/Budgets/*

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Galen Charlton <gmc@esilibrary.com>

diff --git a/C4/Budgets.pm b/C4/Budgets.pm
index 7236a06..38b658d 100644 (file)
--- a/C4/Budgets.pm
+++ b/C4/Budgets.pm
@@ -825,30 +825,52 @@ sub CanUserUseBudget {
         }
 
         # Budget restricted to owner
-        if ($budget->{budget_permission} == 1
-        && $budget->{budget_owner_id}
-        && $budget->{budget_owner_id} != $borrower->{borrowernumber}) {
-            return 0;
+        if ( $budget->{budget_permission} == 1 ) {
+            if (    $budget->{budget_owner_id}
+                and $budget->{budget_owner_id} != $borrower->{borrowernumber} )
+            {
+                return 0;
+            }
         }
 
-        my @budget_users = GetBudgetUsers($budget->{budget_id});
-
         # Budget restricted to owner, users and library
-        if ($budget->{budget_permission} == 2
-        && $budget->{budget_owner_id}
-        && $budget->{budget_owner_id} != $borrower->{borrowernumber}
-        && (0 == grep {$borrower->{borrowernumber} == $_} @budget_users)
-        && defined $budget->{budget_branchcode}
-        && $budget->{budget_branchcode} ne C4::Context->userenv->{branch}) {
-            return 0;
+        elsif ( $budget->{budget_permission} == 2 ) {
+            my @budget_users = GetBudgetUsers( $budget->{budget_id} );
+
+            if (
+                (
+                        $budget->{budget_owner_id}
+                    and $budget->{budget_owner_id} !=
+                    $borrower->{borrowernumber}
+                    or not $budget->{budget_owner_id}
+                )
+                and ( 0 == grep { $borrower->{borrowernumber} == $_ }
+                    @budget_users )
+                and defined $budget->{budget_branchcode}
+                and $budget->{budget_branchcode} ne
+                C4::Context->userenv->{branch}
+              )
+            {
+                return 0;
+            }
         }
 
         # Budget restricted to owner and users
-        if ($budget->{budget_permission} == 3
-        && $budget->{budget_owner_id}
-        && $budget->{budget_owner_id} != $borrower->{borrowernumber}
-        && (0 == grep {$borrower->{borrowernumber} == $_} @budget_users)) {
-            return 0;
+        elsif ( $budget->{budget_permission} == 3 ) {
+            my @budget_users = GetBudgetUsers( $budget->{budget_id} );
+            if (
+                (
+                        $budget->{budget_owner_id}
+                    and $budget->{budget_owner_id} !=
+                    $borrower->{borrowernumber}
+                    or not $budget->{budget_owner_id}
+                )
+                and ( 0 == grep { $borrower->{borrowernumber} == $_ }
+                    @budget_users )
+              )
+            {
+                return 0;
+            }
         }
     }
 

diff --git a/t/Budgets/CanUserModifyBudget.t b/t/Budgets/CanUserModifyBudget.t
index e75a622..bef4237 100644 (file)
--- a/t/Budgets/CanUserModifyBudget.t
+++ b/t/Budgets/CanUserModifyBudget.t
@@ -182,12 +182,12 @@ ok (CanUserModifyBudget($borrower2, $budget11, $flags));
 ok (CanUserModifyBudget($borrower2, $budget12, $flags));
 
 # Restriction is 'owner and users'
-ok (CanUserModifyBudget($borrower1, $budget13, $flags));
+ok (!CanUserModifyBudget($borrower1, $budget13, $flags)); # no owner, no user
 ok (CanUserModifyBudget($borrower1, $budget14, $flags));
 ok (CanUserModifyBudget($borrower1, $budget15, $flags));
 ok (CanUserModifyBudget($borrower1, $budget16, $flags));
-ok (CanUserModifyBudget($borrower2, $budget13, $flags));
-ok (CanUserModifyBudget($borrower2, $budget14, $flags));
+ok (!CanUserModifyBudget($borrower2, $budget13, $flags)); # no owner, no user
+ok (!CanUserModifyBudget($borrower2, $budget14, $flags)); # No owner and user list contains borrower1
 ok (CanUserModifyBudget($borrower2, $budget15, $flags));
 ok (!CanUserModifyBudget($borrower2, $budget16, $flags));
 
@@ -220,17 +220,17 @@ ok (CanUserModifyBudget($borrower1, $budget10, $flags));
 ok (CanUserModifyBudget($borrower1, $budget11, $flags));
 ok (CanUserModifyBudget($borrower1, $budget12, $flags));
 ok (CanUserModifyBudget($borrower2, $budget9, $flags));
-ok (CanUserModifyBudget($borrower2, $budget10, $flags));
+ok (!CanUserModifyBudget($borrower2, $budget10, $flags)); # Limited to library B1
 ok (CanUserModifyBudget($borrower2, $budget11, $flags));
 ok (!CanUserModifyBudget($borrower2, $budget12, $flags));
 
 # Restriction is 'owner and users'
-ok (CanUserModifyBudget($borrower1, $budget13, $flags));
+ok (!CanUserModifyBudget($borrower1, $budget13, $flags)); # No owner, no user
 ok (CanUserModifyBudget($borrower1, $budget14, $flags));
 ok (CanUserModifyBudget($borrower1, $budget15, $flags));
 ok (CanUserModifyBudget($borrower1, $budget16, $flags));
-ok (CanUserModifyBudget($borrower2, $budget13, $flags));
-ok (CanUserModifyBudget($borrower2, $budget14, $flags));
+ok (!CanUserModifyBudget($borrower2, $budget13, $flags)); # No owner, no user
+ok (!CanUserModifyBudget($borrower2, $budget14, $flags)); # No owner and user list contains borrower1
 ok (CanUserModifyBudget($borrower2, $budget15, $flags));
 ok (!CanUserModifyBudget($borrower2, $budget16, $flags));
 

diff --git a/t/Budgets/CanUserUseBudget.t b/t/Budgets/CanUserUseBudget.t
index 8219197..ca94065 100644 (file)
--- a/t/Budgets/CanUserUseBudget.t
+++ b/t/Budgets/CanUserUseBudget.t
@@ -182,12 +182,12 @@ ok (CanUserUseBudget($borrower2, $budget11, $flags));
 ok (CanUserUseBudget($borrower2, $budget12, $flags));
 
 # Restriction is 'owner and users'
-ok (CanUserUseBudget($borrower1, $budget13, $flags));
+ok (!CanUserUseBudget($borrower1, $budget13, $flags)); # No user, no owner
 ok (CanUserUseBudget($borrower1, $budget14, $flags));
 ok (CanUserUseBudget($borrower1, $budget15, $flags));
 ok (CanUserUseBudget($borrower1, $budget16, $flags));
-ok (CanUserUseBudget($borrower2, $budget13, $flags));
-ok (CanUserUseBudget($borrower2, $budget14, $flags));
+ok (!CanUserUseBudget($borrower2, $budget13, $flags)); # No user, no owner
+ok (!CanUserUseBudget($borrower2, $budget14, $flags)); # No owner and user list contains borrower1
 ok (CanUserUseBudget($borrower2, $budget15, $flags));
 ok (!CanUserUseBudget($borrower2, $budget16, $flags));
 
@@ -220,17 +220,17 @@ ok (CanUserUseBudget($borrower1, $budget10, $flags));
 ok (CanUserUseBudget($borrower1, $budget11, $flags));
 ok (CanUserUseBudget($borrower1, $budget12, $flags));
 ok (CanUserUseBudget($borrower2, $budget9, $flags));
-ok (CanUserUseBudget($borrower2, $budget10, $flags));
+ok (!CanUserUseBudget($borrower2, $budget10, $flags)); # Limited to library B1
 ok (CanUserUseBudget($borrower2, $budget11, $flags));
 ok (!CanUserUseBudget($borrower2, $budget12, $flags));
 
 # Restriction is 'owner and users'
-ok (CanUserUseBudget($borrower1, $budget13, $flags));
+ok (!CanUserUseBudget($borrower1, $budget13, $flags)); # No owner, no user
 ok (CanUserUseBudget($borrower1, $budget14, $flags));
 ok (CanUserUseBudget($borrower1, $budget15, $flags));
 ok (CanUserUseBudget($borrower1, $budget16, $flags));
-ok (CanUserUseBudget($borrower2, $budget13, $flags));
-ok (CanUserUseBudget($borrower2, $budget14, $flags));
+ok (!CanUserUseBudget($borrower2, $budget13, $flags)); # No owner, no user
+ok (!CanUserUseBudget($borrower2, $budget14, $flags)); # No owner and user list contains borrower1
 ok (CanUserUseBudget($borrower2, $budget15, $flags));
 ok (!CanUserUseBudget($borrower2, $budget16, $flags));