Bug 6390 - Basket only visible for librarian who created it
Summary: Basket only visible for librarian who created it
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) normal (vote)
Assignee: Edgar Fuß
QA Contact: Bugs List
URL: /cgi-bin/koha/acqui/booksellers.pl
Keywords:
: 4132 (view as bug list)
Depends on: 7002
Blocks: 6676 6824
  Show dependency treegraph
 
Reported: 2011-05-23 14:02 UTC by Katrin Fischer
Modified: 2012-10-25 23:10 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Implement ``see mine/my branches/all baskets''-syspref (2.82 KB, patch)
2011-07-31 13:52 UTC, Edgar Fuß
Details | Diff | Splinter Review
Same patch as before in GIT format (3.05 KB, patch)
2011-07-31 21:26 UTC, Edgar Fuß
Details | Diff | Splinter Review
Same as previous patch (3.35 KB, patch)
2011-08-05 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6390: Implement ``see mine/my branches/all baskets'-syspref' (3.42 KB, patch)
2011-09-06 19:01 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Follow up for Bu 6390: Add upatedatabase.pl (1.25 KB, patch)
2011-09-06 19:03 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 6390 - Basket only visible for librarian who created it (Follow up) (1.55 KB, patch)
2011-09-19 19:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 6390: Implement 'see mine/my branches/all baskets'-syspref (3.62 KB, patch)
2011-09-19 19:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Follow up for Bug 6390: Add upatedatabase.pl (1.35 KB, patch)
2011-09-19 19:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 6390 - Basket only visible for librarian who created it (Follow up) (1.55 KB, patch)
2011-09-19 19:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6390 - Basket only visible for librarian who created it (Follow up) (1.58 KB, patch)
2011-09-21 18:53 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 6390: Implement 'see mine/my branches/all baskets'-syspref (4.49 KB, patch)
2011-10-08 16:50 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 6390: Implement 'see mine/my branches/all baskets'-syspref (4.43 KB, patch)
2011-10-12 20:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 6390: Implement 'see mine/my branches/all baskets'-syspref (5.59 KB, patch)
2011-10-12 20:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6390: Implement 'see mine/my branches/all baskets'-syspref (5.66 KB, patch)
2011-10-17 16:15 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Follow up patch (1.73 KB, patch)
2011-10-18 20:19 UTC, Ian Walls
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6390 Followup: Basket's authorisedby might not be a borrowernumber (1.87 KB, patch)
2011-10-18 20:51 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2011-05-23 14:02:30 UTC
At the moment only staff with superlibrarian rights can see baskets created by other staff logins in acquisition. 

This applies, when you search for a vendor from the acquistion start page and when you search for the vendor using the search on top.

When you search an order by title you can go from the search results to the basket.

In a library with only a few librarians working together and student workers creating orders it's not what the library wants. They want the ability to search for all baskets open from a vendor and see what others have done.

I talked to Paul Poulain on IRC and we can't just change the behaviour, because some libraries want it to work the way it does now.

So what is the way to go?

Paul suggested adding new permissions to granular permissions:
 * see all
 * see my library
 *see mine only

Is this the way to go or are there other ideas how to solve the problem and make the view more flexible to work for different workflows?
Comment 1 Edgar Fuß 2011-07-31 13:47:03 UTC
As I need this functionality too, I just implemented the ``see mine/my branch/all'' suggestion.

I added a new syspref, AcqViewBaskets, taking the values user/branch/all.
The required MySQL statement to add this to the database is:

INSERT INTO `systempreferences` (variable,value,options,explanation,type) VALUES ('AcqViewBaskets','user','user|branch|all','Define which baskets a user is allowed to view: his own only, any within his branch or all','Choice');

I don't feel comfortable enough with the automatic version tracking/database updating machinery to implement the changes need for that.

For the change proper, see attached patch.

However, that's all no real security as long as you can just pass ?basketno=nnn to all of the scripts handling baskets. You either have to change these or to randomise basket numbers.
Comment 2 Edgar Fuß 2011-07-31 13:52:27 UTC Comment hidden (obsolete)
Comment 3 Edgar Fuß 2011-07-31 21:26:02 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2011-08-01 11:28:09 UTC
$git bz apply 6390
Bug 6390 - Basket only visible for librarian who created it

Implement ``see mine/my branches/all baskets''-syspref
Apply? [yn] y

Patch format detection failed.
Comment 5 Jonathan Druart 2011-08-05 08:17:19 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2011-08-15 20:05:56 UTC
*** Bug 4132 has been marked as a duplicate of this bug. ***
Comment 7 Nicole C. Engard 2011-09-06 18:15:54 UTC
This patch is missing an updatedatabase edit.
Comment 8 Nicole C. Engard 2011-09-06 18:46:44 UTC
I'll try to work up a follow up with the updatedatabase bit
Comment 9 Nicole C. Engard 2011-09-06 19:01:41 UTC Comment hidden (obsolete)
Comment 10 Nicole C. Engard 2011-09-06 19:03:42 UTC Comment hidden (obsolete)
Comment 11 Nicole C. Engard 2011-09-06 19:05:51 UTC
Okay my follow up needs to be signed off on and once that happens the two patches can be applied together to make this work.

Nicole
Comment 12 Ian Walls 2011-09-07 17:21:49 UTC
Neither patch addresses the issue of basket.pl, like bug 6824 does.  Apparently the same kind of check needs to be done here, as well (see patch for that bug report)
Comment 13 Katrin Fischer 2011-09-07 17:26:01 UTC
Patches also need a follow-up for systempreferences.sql. Now that Magnus' patches are pushed only en will be needed. 
The first patch should give credit to Edgar Fuß.
Comment 14 Katrin Fischer 2011-09-19 19:29:27 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2011-09-19 19:29:51 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2011-09-19 19:29:59 UTC
Created attachment 5463 [details] [review]
Follow up for Bug 6390: Add upatedatabase.pl

This patch updated the language of the preference and added to
updatedatabase.pl which was missing in the last patch.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 17 Katrin Fischer 2011-09-19 19:31:03 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2011-09-19 19:31:55 UTC
First 2 patches are signed-off - please sign-off on last patch so this can get into 3.6!
Comment 19 Nicole C. Engard 2011-09-21 18:53:22 UTC Comment hidden (obsolete)
Comment 20 Nicole C. Engard 2011-09-21 18:53:40 UTC
last patch signed off.
Comment 21 Paul Poulain 2011-10-07 14:47:20 UTC
kf, same problems as with another patch tested yesterday
fatal: cannot convert from UTF-8utf-8 to UTF-8
Patch left in /tmp/Bug-6390-Implement-see-minemy-branchesall-baskets--pmwyHR.patch
Comment 22 Ian Walls 2011-10-07 15:41:43 UTC
Patch applied, and here are my test results:

1) the applied patch governs user search in the Vendor tab properly; depending on the setting, when searching for a vendor I either get only mine, only my branch's, or all.

2) superlibrarian privileges to not give me access to all; the preference value trumps superlibrarian

3) search in the Orders tab is completely unaffected, so I can still get to any basket using that search

4) system preference editor text is very andro-centric ("he", "his"); needs to be gender neutral


Points 2 and 4 gives me cause to mark this as Failed QA.  Point 3 just makes me pause; is this system preference intended to govern access universally, or only for this one search page?  Need to understand the intended scope.
Comment 23 Katrin Fischer 2011-10-08 16:50:37 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2011-10-08 17:18:13 UTC
2) Is not solved. I was not able to get it working, perhaps someone else can.
Comment 25 Katrin Fischer 2011-10-11 15:09:50 UTC
My problem might be related to the problem reported in 7002.
Comment 26 Katrin Fischer 2011-10-12 20:06:26 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2011-10-12 20:17:06 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2011-10-12 20:18:40 UTC
New patch (hopefully) resolving all problems noted by Ian attached, waiting for sign-off.
Comment 29 Nicole C. Engard 2011-10-17 16:15:24 UTC
Created attachment 5935 [details] [review]
[SIGNED-OFF] Bug 6390: Implement 'see mine/my branches/all baskets'-syspref

Code written by Edgar Fuß <ef@math.uni-bonn.de>,
Mathematisches Institut der Uniersität Bonn

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
- rebased on current master, fixed a conflict in booksellers.pl
- reworked permission checks to account for general acq permissions and
  superlibrarian permission
- rephrased text of system preference (Thx to Jared Camins-Esakov)

Signed-off-by: Nicole C. Engard <nengard@bywatersolutions.com>
Comment 30 Ian Walls 2011-10-17 16:25:15 UTC
Simple enough change; adds C4::Context dependency to acqui/booksellers (by necessity).  Branch detection method is lean (GetMember instead of GetMemberDetails), and permissions are written out cleanly with comments for each aspect.  Preference language is now gender-neutral (thank you!).

Marking as Passed QA
Comment 31 Chris Cormack 2011-10-17 20:48:06 UTC
Patch pushed, please test. I appreciate all the work gone into this, well done.
Comment 32 Ian Walls 2011-10-18 20:09:29 UTC
....and I missed something.

aqbasket.authorisedby is not a foreign key to borrowers.borrowernumber.  It's *normally* the borrowernumber, but there is no provision for handling deleted borrowernumbers, and migrated acquisitions information could leave this field blank without plenty.

So, when a basket has a non-borrowernumber value in authorisedby, vendor search breaks:

Can't use an undefined value as a HASH reference at /home/sekjal/kohaclone/acqui/booksellers.pl line 114.

Working on a fix now.
Comment 33 Ian Walls 2011-10-18 20:19:59 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2011-10-18 20:51:39 UTC
Created attachment 5966 [details] [review]
[SIGNED-OFF] Bug 6390 Followup: Basket's authorisedby might not be a borrowernumber

aqbasket.authorisedby is not foreign key constrainted to borrowers.borrowernumber,
so if it's anything other than that, vendor search breaks by using an undefined value
(GetMember() on an invalid borrowernumber) as a HASH ref.

This patch sets the branchcode (the desired value from GetMember) to a blank value, then only
changes it if GetMember is defined.  In the case where the authorisedby librarian has been deleted,
or the value is otherwise invalid, and AcqViewBaskets is set to 'branch', the basket will not appear
except to superlibrarians

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Invalid borrowernumber in aqbasket.authorisedby no longer breaks search.
Comment 35 Paul Poulain 2011-10-19 08:19:29 UTC
aqbasket.authorisedby is not a foreign key to borrowers.borrowernumber.  

=> and it must not : if a librarian leaves the library, the order must not be deleted (for many reasons, one of them being financial statistics) ! The foreign key could be SET NULL, but not DELETE !
So we must handle an empty authorisedby, not prevent it to happend
Comment 36 Katrin Fischer 2011-10-19 08:22:39 UTC
If we set it to NULL we must decide who gets to see it, because this messes up with the whole permission system.

superlibrarian = can see it, always sees all

Syspref settings:

only my own - strict, was default before this patch. Who inherits the NULL baskets? Can I suddenly see what I was not allowed to see before?

only my branch - less strict, but again - how do we determine to with branch this basket belongs to?

all - no problem
Comment 37 Ian Walls 2011-10-19 18:03:55 UTC
We could try a fall-back check of deleted_borrowers.  If the borrower record was deleted by normal means, it should be in there, and we should be able to extract their branchcode.

The issue of who inherits a departing staff member's baskets is still unresolved, but hopefully it will be such a rare occurrence for most libraries that having a rigourous method for dealing with this will be a low priority item.

Personally, I don't think there will be a good solution until we have improved borrowers enough to allow for arbitrary Borrower Relationships (beyond just Guarantor/Guarantee).  Something that will let us specify 'supervisor' or something similar.

I'm going to pass my fix through QA, because it's better than a broken vendor search.  Release Manager, please double-check me on this, and anyone, if you object, speak up!
Comment 38 Paul Poulain 2011-11-04 10:55:37 UTC
before applying to master, a preliminary question: is this really a bug or an ENH ?
Comment 39 Katrin Fischer 2011-11-04 11:00:52 UTC
It's probably debatable. 
The feature is already in 3.6 - we only need the follow-up to prevent vendor search from breaking under certain conditions.
Comment 40 Paul Poulain 2011-11-06 16:02:39 UTC
Just FYI: there is an updatedatabase in this patch. We've proposed a new method to handle database update (http://lists.koha-community.org/pipermail/koha-devel/2011-November/036402.html), and there is no 3.7 updates yet. So I will wait until the new system to updatedatabase to apply this fix (and the patch won't apply anymore, but i'll take care of that)
Comment 41 Katrin Fischer 2011-11-06 16:41:06 UTC
Paul, the syspref is already in. It's only the follow-up that is missing. No database update for that.
Comment 42 Paul Poulain 2011-11-06 17:00:24 UTC
patch "Bug 6390 Followup: Basket's authorisedby might not be a borrowernumber"  applied and pushed on branch new/bug_6390, and then merged onto master.

Please test
Comment 43 Chris Nighswonger 2011-11-16 14:19:55 UTC
This patch does not apply cleanly to the 3.4.x branch. If it should, please rebase against that branch and attach a corrected patch to this bug.

cnighs@erasmus:~/koha.3.2.test (3.4.x)$ git cherry-pick -x -s 58bdfdb08883eb5
Automatic cherry-pick failed.  After resolving the conflicts,
mark the corrected paths with 'git add <paths>' or 'git rm <paths>'
and commit the result with:

        git commit -c 58bdfdb08883eb52777321f883491eaff1493857

cnighs@erasmus:~/koha.3.2.test (3.4.x *+)$ git status
# On branch 3.4.x
# Your branch is ahead of 'origin/3.4.x' by 21 commits.
#
# Unmerged paths:
#   (use "git reset HEAD <file>..." to unstage)
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#       both modified:      acqui/booksellers.pl
Comment 44 Katrin Fischer 2011-11-27 21:17:59 UTC
System preference with permissions was added with version 3.6. I think it's ok to not backport to 3.4