Bug 9833 - New virtual shelves/lists permissions
Summary: New virtual shelves/lists permissions
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-18 10:38 UTC by Matthias Meusburger
Modified: 2023-01-08 01:18 UTC (History)
8 users (show)

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


Attachments
Bug 9833: Refine shelves permissions. (7.28 KB, patch)
2013-03-18 15:12 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 9833: Refine shelves permissions. (7.29 KB, patch)
2013-03-19 09:51 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9833: Refine shelves permissions. (7.29 KB, patch)
2013-03-19 10:12 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9833: Refine shelves permissions. (7.26 KB, patch)
2013-05-20 11:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2013-03-18 10:38:27 UTC
refine permissions again : 

# manage_shelves Allows to manage shelves

# merge_from_shelves Allows to merge records from shelves
Comment 1 Matthias Meusburger 2013-03-18 15:12:55 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2013-03-19 09:51:27 UTC
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Comment 3 Paul Poulain 2013-03-19 09:51:42 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2013-03-19 10:12:31 UTC
Patch tested with a sandbox, by Pierre Angot <tredok.pierre@gmail.com>
Comment 5 Paul Poulain 2013-03-19 10:12:50 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2013-05-20 10:33:35 UTC
I like this patch since we really need some permissions for shelves!!

But I think that we must not add manage shelves under editcatalogue.
Managing shelves should have its own permission (if we allow it in the first place). If this patch adds an entry for shelves/lists, I would also have the idea of adding/allowing Change ownership for instance. Note also the privacy issue here as noted in related reports (should a staff user be able to view these lists at all?).
So, you could remove manage_shelves from this patch or add a root entry like lists and start a discussion on how we should view the underlying manage. What we do need e.g. is a permission to manage shelves that do no longer have an owner (after deleting user).

Merge from shelves is another story: this indeed fits better under edit_catalogue.

A technical detail: To know member->userid you use GetMemberDetails: Isn't that quite overkill?
Also please watch operator priority in $user && $shelf->{owner}==$user) || C4::Auth::haspermission... Place parentheses? 

A test plan should be added too.

Thanks for your attention.
Comment 7 Marcel de Rooy 2013-05-20 11:13:05 UTC
Created attachment 18247 [details] [review]
Bug 9833: Refine shelves permissions.

- This patch adds two new permissions for shelves:
   -  manage_shelves: allows to manage shelves
   - merge_from_shelves: allows to merge records from shelves

Signed-off-by: Pierre Angot <tredok.pierre@gmail.com>
Comment 8 Liz Rea 2013-07-10 23:44:20 UTC
Marcel,

I'm confused by the status of this patch - there's a signed off patch but it's failed QA?

I came up against this today, it would be super cool to get this little nagger fixed.

Liz
Comment 9 Marcel de Rooy 2013-07-11 06:25:31 UTC
(In reply to Liz Rea from comment #8)
> Marcel,
> 
> I'm confused by the status of this patch - there's a signed off patch but
> it's failed QA?
> 
> I came up against this today, it would be super cool to get this little
> nagger fixed.
> 
> Liz

I hope that Matthias as author of the patch (or someone else from Biblibre?) adjusts this patch to make it pass qa.
Comment 10 Marcel de Rooy 2013-07-12 09:22:28 UTC
Picked some discussion from irc log:
==========
wizzyrea: and even better, apparently there was a bug at some point that set the owners to NULL, so there are lists existent that cannot be edited by anyone?!

dcook: That is a bit confusing...I can't tell if Marcel passed or failed it based on the comments.. Although it does look like he's asking for some changes? then he attached a patch...
============= [End of quote]

Do no know exactly any more but I must have attached this patch just for a trivial rebase. Yes, I failed this patch for reasons mentioned in comment 6.

Note also that there are privacy issues on viewing/editing all lists by superlibs. Furthermore, I am still working on development for sharing lists that should also cover the topic of lists without owner (i call them orphaned lists). It should be the third patch of the Sharing a lists- series..

Hope that helps.
Comment 11 Liz Rea 2013-07-14 21:24:16 UTC
The lists owned by NULL happens when the borrower that created the list is deleted. That's probably a different bug than this. 

Matthias, any chance of getting this fixed up?
Comment 12 Fridolin Somers 2019-01-28 10:06:41 UTC
Merge from shelves is more a cataloguing action non ?
Should be a permission under "editcatalogue".
It is actually depending on "edit_catalogue" permission.
Comment 13 Katrin Fischer 2023-01-08 01:18:29 UTC
(In reply to Fridolin Somers from comment #12)
> Merge from shelves is more a cataloguing action non ?
> Should be a permission under "editcatalogue".
> It is actually depending on "edit_catalogue" permission.

I think maybe merging could be a general permission applying to merging in the cataloguing module or in lists and be listed under cataloguing. 

And I would recommend splitting this bug into 2.