Bug 12019 - funds displayed on acquisition home show funds of all branches
Summary: funds displayed on acquisition home show funds of all branches
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-31 18:34 UTC by Marjorie Barry-Vila
Modified: 2014-12-07 20:07 UTC (History)
5 users (show)

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


Attachments
funds (172.08 KB, image/png)
2014-03-31 18:34 UTC, Marjorie Barry-Vila
Details
Bug 12019: FIX budget_owner_id node is duplicated (1.29 KB, patch)
2014-04-02 11:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12019: If a fund has a restriction, a owner is mandatory (1.59 KB, patch)
2014-04-02 11:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12019: FIX budget_owner_id node is duplicated (1.36 KB, patch)
2014-04-08 13:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12019: If a fund has a restriction, a owner is mandatory (1.67 KB, patch)
2014-04-08 13:57 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 12019: It should not be mandatory to define an owner if a fund is restricted (8.53 KB, patch)
2014-04-14 12:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 12019: FIX budget_owner_id node is duplicated (1.42 KB, patch)
2014-04-15 21:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12019: FIX budget_owner_id node is duplicated (1.43 KB, patch)
2014-04-15 21:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12019: It should not be mandatory to define an owner if a fund is restricted (9.31 KB, patch)
2014-04-15 21:15 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12019: FIX budget_owner_id node is duplicated (1.48 KB, patch)
2014-04-18 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12019: It should not be mandatory to define an owner if a fund is restricted (9.37 KB, patch)
2014-04-18 12:04 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marjorie Barry-Vila 2014-03-31 18:34:41 UTC
Created attachment 26710 [details]
funds

Hi,
In 3.12, funds displayed on acquisition home show funds of all branches.
In 3.8, funds of librarian's branch were displayed.
Is this an enhancement? I forgot a sys pref?
See the capture.

Regards,
Marjorie
Comment 1 Katrin Fischer 2014-03-31 18:46:34 UTC
Hi Marjorie,

some ideas:
- do you have superlibrarian permission?
- are your funds 'limited to' the library?
Comment 2 Marjorie Barry-Vila 2014-03-31 19:32:09 UTC
Thanks Katrin.
Yes, i have superlibrarian permissions and funds are limited to each branch.
May i test something else?

Marjorie
Comment 3 Katrin Fischer 2014-03-31 19:52:39 UTC
Maybe try removing your superlibrairan flag for a test? And keep all other permissions? I think that could be the difference, that a superlibriarian can always see the funds now.
Comment 4 Marjorie Barry-Vila 2014-04-01 20:27:06 UTC
(In reply to Katrin Fischer from comment #3)
> Maybe try removing your superlibrairan flag for a test? And keep all other
> permissions? I think that could be the difference, that a superlibriarian
> can always see the funds now.

I tested several flags but nothing change on acquisition home.
Have you tested it?
Comment 5 Katrin Fischer 2014-04-02 04:38:39 UTC
Hi Marjorie, I did test now - and I think you are right. Something about the visibility is broken. I removed the permission to manage all funds, I am not a superlibrarian, the fund access is restricted to another branch and I can still see it :( I am quite sure this used to work differently and it quite a problem. The view is also the same on the acquisition start page and the fund list for the budget.
Comment 6 Katrin Fischer 2014-04-02 04:50:06 UTC
Hm, I tried a git bisect, because I wondered when it changed and the change seems to have been introduced by:

203757e353cbbb2add934523fdbd7113c6180207 is the first bad commit
commit 203757e353cbbb2add934523fdbd7113c6180207
Author: Julian Maurice <julian.maurice@biblibre.com>
Date:   Fri May 18 09:12:43 2012 +0200

    Bug 7304: More permissions for budgets
Comment 7 Katrin Fischer 2014-04-02 05:43:12 UTC
Note: IndependentBranches also doesn't make any difference.

I have upgraded the severity, as this is quite a problem for bigger libraries that don't want other branches to see their funds.
Comment 8 Jonathan Druart 2014-04-02 11:09:41 UTC
Marjorie,
Try to define an owner for the fund. If no owner is defined, it seems there is no restriction.
Comment 9 Jonathan Druart 2014-04-02 11:33:04 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-04-02 11:33:10 UTC Comment hidden (obsolete)
Comment 11 Marjorie Barry-Vila 2014-04-02 13:50:04 UTC
(In reply to Jonathan Druart from comment #8)
> Marjorie,
> Try to define an owner for the fund. If no owner is defined, it seems there
> is no restriction.

Jonathan,
I defined an owner for the fund but for me it is the same on acquisition home page, i can see funds of the other branches and of the other owners.
I should see only the budgets that I am an owner, right?
We need to change something in the drop down menu restrictions?

Thanks!

Marjorie
Comment 12 Jonathan Druart 2014-04-02 15:30:10 UTC
(In reply to Marjorie Barry-Vila from comment #11)
> I defined an owner for the fund but for me it is the same on acquisition
> home page, i can see funds of the other branches and of the other owners.
> I should see only the budgets that I am an owner, right?
> We need to change something in the drop down menu restrictions?

Could you confirm the issue still appears with these patches?
Comment 13 Marjorie Barry-Vila 2014-04-07 13:15:08 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Marjorie Barry-Vila from comment #11)
> > I defined an owner for the fund but for me it is the same on acquisition
> > home page, i can see funds of the other branches and of the other owners.
> > I should see only the budgets that I am an owner, right?
> > We need to change something in the drop down menu restrictions?
> 
> Could you confirm the issue still appears with these patches?

Hi Jonathan,
I tested it on a sandbox but it is not working for me. I think it's me I do something wrong. I added an owner, a library and a restriction to only owner and i can still see fund of the other library.
I changed flags too but without results.

See the video attached, maybe you will see what i do wrong

Marjorie
Comment 14 Marjorie Barry-Vila 2014-04-07 13:18:47 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Marjorie Barry-Vila from comment #11)
> > I defined an owner for the fund but for me it is the same on acquisition
> > home page, i can see funds of the other branches and of the other owners.
> > I should see only the budgets that I am an owner, right?
> > We need to change something in the drop down menu restrictions?
> 
> Could you confirm the issue still appears with these patches?

see the video here http://bit.ly/1mU7Hlt

Marjorie
Comment 15 Jonathan Druart 2014-04-08 08:30:26 UTC
(In reply to Marjorie Barry-Vila from comment #13)
> Hi Jonathan,
> I tested it on a sandbox but it is not working for me. I think it's me I do
> something wrong. I added an owner, a library and a restriction to only owner
> and i can still see fund of the other library.
> I changed flags too but without results.

The "test" patron has the manage_all_buggets permission. So he can edit all budgets, even if a restriction is set.
Comment 16 Marjorie Barry-Vila 2014-04-08 13:56:43 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Marjorie Barry-Vila from comment #13)
> > Hi Jonathan,
> > I tested it on a sandbox but it is not working for me. I think it's me I do
> > something wrong. I added an owner, a library and a restriction to only owner
> > and i can still see fund of the other library.
> > I changed flags too but without results.
> 
> The "test" patron has the manage_all_buggets permission. So he can edit all
> budgets, even if a restriction is set.

ok! it's works! Sorry for all my comments!
I signed off.

Marjorie
Comment 17 Biblibre Sandboxes 2014-04-08 13:57:08 UTC
Patch tested with a sandbox, by marjorie barry-vila <marjorie.barry-vila@ccsr.qc.ca>
Comment 18 Biblibre Sandboxes 2014-04-08 13:57:35 UTC Comment hidden (obsolete)
Comment 19 Biblibre Sandboxes 2014-04-08 13:57:38 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2014-04-09 21:02:42 UTC
I think this works, but I am not sure that making the owner mandatory is the right fix.

We have funds in Koha that have been created before the owner feature has been introduced and we can't expect libraries to add an owner to all those now to bring back a feature that worked in the past. Especially if the owner has no real meaning - it would just be added, to make the library restriction take effect again.

I think we need to fix the visibility check to work without an owner.
Comment 21 Jonathan Druart 2014-04-14 10:32:45 UTC
The "owner" field has been introduced in 3.01.00.077 and the restriction by owner already existed before bug 7304.
Could you please detail what could be broken?
Comment 22 Jonathan Druart 2014-04-14 10:34:06 UTC
Ok, maybe if a restriction exist on "Owner, users and library" and no owner is defined.
Comment 23 Katrin Fischer 2014-04-14 11:35:39 UTC
Hi Jonathan, the git bisect I did led me back to bug 7304 - so somehow the behaviour change was introduced at this point. I am not trying to be difficult here, but I think fixing the logic would really be better. 

If you select owners, users and library, and there are no users, it still works - why rely on an owner if the library is set?
Comment 24 Jonathan Druart 2014-04-14 12:03:30 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2014-04-15 21:13:10 UTC Comment hidden (obsolete)
Comment 26 Katrin Fischer 2014-04-15 21:15:00 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2014-04-15 21:15:13 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2014-04-18 12:03:57 UTC
Created attachment 27282 [details] [review]
[PASSED QA] Bug 12019: FIX budget_owner_id node is duplicated

The input witht the budget_owner_id id is duplicated.
It breaks the jQuery selector
  $('#budget_owner_id').val('');

Signed-off-by: marjorie barry-vila <marjorie.barry-vila@ccsr.qc.ca>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2014-04-18 12:04:11 UTC
Created attachment 27283 [details] [review]
[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 <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>
Comment 30 Galen Charlton 2014-05-04 18:49:09 UTC
Pushed to master.  Thanks, Jonathan!