From 4ee3e69b44e15c046b5b85091618b8504f836a8f Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 14 Apr 2014 13:11:12 +0200 Subject: [PATCH] [PASSED QA] Bug 12019: It should not be mandatory 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 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 --- C4/Budgets.pm | 58 +++++++++++++++++++++++++++------------ t/Budgets/CanUserModifyBudget.t | 14 +++++----- t/Budgets/CanUserUseBudget.t | 14 +++++----- 3 files changed, 54 insertions(+), 32 deletions(-) diff --git a/C4/Budgets.pm b/C4/Budgets.pm index 758e98f..d0dc923 100644 --- a/C4/Budgets.pm +++ b/C4/Budgets.pm @@ -827,30 +827,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 --- 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 --- 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)); -- 1.7.2.5