Bug 22031 - C4::Auth->haspermission should allow checking for more than one subpermission
Summary: C4::Auth->haspermission should allow checking for more than one subpermission
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 20128 22483
  Show dependency treegraph
 
Reported: 2018-12-20 13:40 UTC by Nick Clemens
Modified: 2019-03-12 08:42 UTC (History)
1 user (show)

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


Attachments
Bug 22031: Unit tests (2.14 KB, patch)
2018-12-20 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22031: Adjust haspermission to take an array (2.05 KB, patch)
2018-12-20 13:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22031: Add SQL::Abstract like syntax to haspermission (13.39 KB, patch)
2018-12-22 13:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22031: Add SQL::Abstract like syntax to haspermission (13.46 KB, patch)
2019-03-06 17:13 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22031: Add SQL::Abstract like syntax to haspermission (13.53 KB, patch)
2019-03-06 18:38 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-12-20 13:40:48 UTC
Currently we can only check one subpermission per module

It would be nice to check multiple

Testing code suggests this was possible, however, it was flawed

flagsrequired => {
   tools => 'batch_upload_patron_images',
   tools => 'upload_local_cover_images'
}

Above does not test for two permissions, it simply checks for the second
Comment 1 Nick Clemens 2018-12-20 13:43:24 UTC
Created attachment 83414 [details] [review]
Bug 22031: Unit tests

To test:
1 - Apply this patch only
2 - prove -v t/db_dependent/Auth/haspermission.t
3 - It will fail
Comment 2 Nick Clemens 2018-12-20 13:43:27 UTC
Created attachment 83415 [details] [review]
Bug 22031: Adjust haspermission to take an array

To test:
1 - Apply this patch on top of unit tests
2 - Unit tests now pass
3 - Ensure you can use Koha as before with no regressions
Comment 3 Martin Renvoize 2018-12-21 14:07:04 UTC
This works well enough... But.. what if we wanted to introduce the ability to compare logically with OR or AND in the future.. by adding arrayref as 'AND' here we'll be doing the exact opposite of what SQL::Abstract does...  

I'm going to try and come up with an alternative that more closely resembles SQL::Abstract so we're not dealing with two very similar but opposing constructs in Koha.
Comment 4 Martin Renvoize 2018-12-22 13:57:46 UTC
Created attachment 83474 [details] [review]
Bug 22031: Add SQL::Abstract like syntax to haspermission

This patch adds an SQL::Abstract inspired query syntax to the
haspermission method in C4::Auth.  One can now pass Arrayrefs to denote
an OR list of flags, a Hashref to denote a AND list of flags.

Structures can be nested at arbitrary depth.
Comment 5 Martin Renvoize 2018-12-22 13:58:42 UTC
My last attachment is an alternative implementation (and includes additional tests).

Let me know what you think Nick.
Comment 6 Nick Clemens 2019-03-06 17:13:21 UTC
I think this is fantastic, it will need documentation but should expand ability to use/combine permissions. Excellent work!
Comment 7 Nick Clemens 2019-03-06 17:13:45 UTC
Created attachment 86197 [details] [review]
Bug 22031: Add SQL::Abstract like syntax to haspermission

This patch adds an SQL::Abstract inspired query syntax to the
haspermission method in C4::Auth.  One can now pass Arrayrefs to denote
an OR list of flags, a Hashref to denote a AND list of flags.

Structures can be nested at arbitrary depth.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Jonathan Druart 2019-03-06 18:38:13 UTC
Created attachment 86204 [details] [review]
Bug 22031: Add SQL::Abstract like syntax to haspermission

This patch adds an SQL::Abstract inspired query syntax to the
haspermission method in C4::Auth.  One can now pass Arrayrefs to denote
an OR list of flags, a Hashref to denote a AND list of flags.

Structures can be nested at arbitrary depth.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Nick Clemens 2019-03-07 21:20:54 UTC
Awesome work all!

Pushed to master for 19.05
Comment 10 Martin Renvoize 2019-03-08 08:20:01 UTC
Hi Nick, 

Wow, really pleased to see this one going through.. I thought it might come up against a bit more opposition :).

So.. you mention additional documentation.. did you have any thoughts on what you might like.. some examples in the POD perhaps or were you thinking something on the wiki?

I'm happy to oblige.
Comment 11 Martin Renvoize 2019-03-12 08:42:54 UTC
Enhancement, will not be backported to 18.11.x series.