Bug 7976 - "Borrow Permission" is not used and can be removed
Summary: "Borrow Permission" is not used and can be removed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 16447
  Show dependency treegraph
 
Reported: 2012-04-17 22:52 UTC by Joy Nelson
Modified: 2016-12-05 21:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 7976: Remove the borrow permission (26.32 KB, patch)
2015-03-30 16:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED OFF] Bug 7976: Remove the borrow permission (26.60 KB, patch)
2015-04-15 19:44 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 7976: Remove the borrow permission (26.66 KB, patch)
2015-04-15 21:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7976: Update borrowers.flags (1.16 KB, patch)
2015-04-16 07:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7976: Update borrowers.flags (2.45 KB, patch)
2015-05-06 18:39 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7976: Update borrowers.flags (2.46 KB, patch)
2015-05-06 18:41 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 7976: Update borrowers.flags (2.52 KB, patch)
2015-05-08 12:41 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 Joy Nelson 2012-04-17 22:52:02 UTC
The 'borrow permission' for staff category borrowers does not appear to be functional. Staff are still able to borrow items regardless of this permissions setting.
Comment 1 Christopher Brannon 2014-06-10 19:09:13 UTC
I'm trying to understand the purpose of this permission, myself.  It seems there is no documentation on it.  What I would assume it means is if it is checked, the patron can borrow.  However, if this actually worked, it would mean all regular patrons would need this permission turned on.

I would propose that the nature of this switch be change and read (cannot_borrow).  This switch should limit the account so that it cannot be borrowed on.

So, for example, if the account were a general circulation staff account, one could not check out items to this account if the permission were selected.

Christopher
Comment 2 Nicole C. Engard 2014-06-10 19:36:49 UTC
I'd gladly update the documentation if someone can tell me what it does.

Nicole
Comment 3 Katrin Fischer 2014-11-09 22:09:37 UTC
It appears that there are some references to the borrow permission in the codebase, but I am not sure about the intended functionality either:

Some examples:
...
basket/basket.pl:        flagsrequired   => { borrow => 1 },
basket/downloadcart.pl:        flagsrequired   => { borrow => 1 },
basket/sendbasket.pl:        flagsrequired   => { borrow => 1 },
basket/sendbasket.pl:            flagsrequired   => { borrow => 1 },
...
Comment 4 Jonathan Druart 2015-03-30 16:24:26 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2015-04-15 19:44:12 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2015-04-15 21:19:52 UTC
Created attachment 37925 [details] [review]
[PASSED QA] Bug 7976: Remove the borrow permission

The borrow permission was used but uselessly.
For instance, at the opac, the flagsrequired parameter was set to
'borrow' but the 'authnotrequired' was set also (which means no auth
required).
At the end, this permission was used at only 1 place: for the basket,
intranet side.
This can be replaced with the catalogue permission (which is used to
search).

Test plan:
1/ Confirm that you are able to show/download/sent the cart (intranet side)
with the catalogue permission.
2/ At the OPAC, you should be able to access the same pages as before
with any other permissions.

Concretely it is quite difficult to test this patch, you should have a
look at the code.

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2015-04-15 21:20:40 UTC
Should we also adjust the borrowres.flags for staff users who had borrows permission? Any negative side effects if we don't?
Comment 8 Jonathan Druart 2015-04-16 07:48:52 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-04-16 07:49:26 UTC
(In reply to Katrin Fischer from comment #7)
> Should we also adjust the borrowres.flags for staff users who had borrows
> permission? Any negative side effects if we don't?

Yes you are right, to be safe it's certainly better :)
Comment 10 Bernardo Gonzalez Kriegel 2015-05-06 18:39:00 UTC Comment hidden (obsolete)
Comment 11 Bernardo Gonzalez Kriegel 2015-05-06 18:41:48 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2015-05-08 12:41:19 UTC
Created attachment 38974 [details] [review]
[PASSED QA] Bug 7976: Update borrowers.flags

The borrowers.flags values need to be updated to prevent side effects.

Test plan:
Set the borrow permission flags for some borrowers
Execute the updatedb entry and verify the flags has been updated (-128)
only for borrowers with this permission set.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Moved updatedatabase entry (was in wrong position) to atomicupdate
with commented copy of orignal

No koha-qa errors

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Tomás Cohen Arazi 2015-06-05 19:13:05 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 14 Bouzid 2016-05-04 20:08:14 UTC
Hi all,
The borrow permission is removed, but it existing in this files: 

opac/opac-idref.pl:40:        flagsrequired   => { borrow => 1 },
opac/opac-news-rss.pl:37:        flagsrequired   => { borrow => 1 },
opac/opac-restrictedpage.pl:43:        flagsrequired   => { borrow => 1 },
opac/svc/login:46  checkauth( $query, 1,  { borrow => 1 }, 'opac', $decoded_json->{'email'} );

I think, it's a bug.

Bouzid.
Comment 15 Jonathan Druart 2016-05-05 13:51:56 UTC
(In reply to Bouzid from comment #14)
> Hi all,
> The borrow permission is removed, but it existing in this files: 
> 
> opac/opac-idref.pl:40:        flagsrequired   => { borrow => 1 },
> opac/opac-news-rss.pl:37:        flagsrequired   => { borrow => 1 },
> opac/opac-restrictedpage.pl:43:        flagsrequired   => { borrow => 1 },
> opac/svc/login:46  checkauth( $query, 1,  { borrow => 1 }, 'opac',
> $decoded_json->{'email'} );
> 
> I think, it's a bug.
> 
> Bouzid.

Yes it is, it always better to open a new bug report, especially when the original one is marked as resolved.

See bug 16447
Comment 16 Bouzid 2016-05-05 14:09:01 UTC
Ok Jonathan.
thanks.
Bouzid.