Bug 7673 - New patron permissions to manage items (edit, batchmod, delete all)
Summary: New patron permissions to manage items (edit, batchmod, delete all)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15173
  Show dependency treegraph
 
Reported: 2012-03-08 12:59 UTC by delaye
Modified: 2016-08-16 13:29 UTC (History)
12 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 7673 New patron permissions (12.05 KB, patch)
2012-03-30 12:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (12.87 KB, patch)
2012-05-31 13:22 UTC, Matthias Meusburger
Details | Diff | Splinter Review
liste de cb for test une sandbox one (226 bytes, application/octet-stream)
2012-06-19 09:29 UTC, delaye
Details
Bug 7673 : New patron permissions (12.87 KB, patch)
2012-09-10 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (12.87 KB, patch)
2012-10-23 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (12.04 KB, patch)
2013-03-18 10:19 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (15.87 KB, patch)
2013-03-18 13:32 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (16.05 KB, patch)
2013-03-19 11:07 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (15.92 KB, patch)
2013-03-19 11:30 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (16.65 KB, patch)
2013-12-03 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: QA Followup: trailing whitespace and perlcritic issues (3.14 KB, patch)
2013-12-03 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (16.65 KB, patch)
2013-12-03 16:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: QA Followup: trailing whitespace and perlcritic issues (3.14 KB, patch)
2013-12-03 16:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (16.05 KB, patch)
2014-03-10 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: QA Followup: trailing whitespace and perlcritic issues (3.14 KB, patch)
2014-03-10 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: The delete all items link should only be displayed if delete_all_items is set (1.08 KB, patch)
2014-03-31 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Add permission delete_all_items if user has edit_items (1.38 KB, patch)
2014-03-31 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: the limitation on editing an item does not affect FA framework (1.24 KB, patch)
2014-04-01 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (9.76 KB, patch)
2014-04-25 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: QA Followup: trailing whitespace and perlcritic issues (3.14 KB, patch)
2014-04-25 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: The delete all items link should only be displayed if delete_all_items is set (1.08 KB, patch)
2014-04-25 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: the limitation on editing an item does not affect FA framework (1.24 KB, patch)
2014-04-25 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB Changes - add system preferences (2.97 KB, patch)
2014-04-25 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update syspref names in script (1.97 KB, patch)
2014-04-25 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB changes - Add permissions (13.19 KB, patch)
2014-04-25 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update permission names in script (7.84 KB, patch)
2014-04-25 14:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Change the superlibrarian check (2.03 KB, patch)
2014-04-25 14:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB Changes - add system preferences (3.29 KB, patch)
2014-04-25 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (9.83 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: QA Followup: trailing whitespace and perlcritic issues (3.20 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: The delete all items link should only be displayed if delete_all_items is set (1.14 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: the limitation on editing an item does not affect FA framework (1.30 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update syspref names in script (2.03 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB changes - Add permissions (13.21 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update permission names in script (7.90 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Change the superlibrarian check (2.09 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB Changes - add system preferences (3.39 KB, patch)
2014-05-07 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB changes - (follow-up) Add permissions (3.74 KB, patch)
2014-07-07 14:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (9.81 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: QA Followup: trailing whitespace and perlcritic issues (3.19 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: The delete all items link should only be displayed if delete_all_items is set (1.14 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: the limitation on editing an item does not affect FA framework (1.30 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update syspref names in script (2.01 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB changes - Add permissions (13.18 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update permission names in script (7.87 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Change the superlibrarian check (2.08 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB Changes - add system preferences (3.39 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB changes - (follow-up) Add permissions (3.74 KB, patch)
2014-08-18 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673 : New patron permissions (9.81 KB, patch)
2014-08-20 10:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: QA Followup: trailing whitespace and perlcritic issues (3.19 KB, patch)
2014-08-20 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: The delete all items link should only be displayed if delete_all_items is set (1.14 KB, patch)
2014-08-20 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: the limitation on editing an item does not affect FA framework (1.30 KB, patch)
2014-08-20 10:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update syspref names in script (2.01 KB, patch)
2014-08-20 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Update permission names in script (7.87 KB, patch)
2014-08-20 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Change the superlibrarian check (2.08 KB, patch)
2014-08-20 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: DB changes - Add permissions (19.84 KB, patch)
2014-08-20 10:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: add syspref in sysprefs.pl (3.05 KB, patch)
2014-10-10 19:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7673: Assume that all fields can be edited if the pref is empty (4.61 KB, patch)
2014-10-10 19:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 7673 : New patron permissions (9.87 KB, patch)
2014-10-10 21:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: QA Followup: trailing whitespace and perlcritic issues (2.82 KB, patch)
2014-10-10 21:37 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: The delete all items link should only be displayed if delete_all_items is set (1.20 KB, patch)
2014-10-10 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: the limitation on editing an item does not affect FA framework (1.36 KB, patch)
2014-10-10 21:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: Update syspref names in script (2.07 KB, patch)
2014-10-10 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: Update permission names in script (7.93 KB, patch)
2014-10-10 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: Change the superlibrarian check (2.15 KB, patch)
2014-10-10 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: DB changes - Add permissions (19.91 KB, patch)
2014-10-10 21:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: Assume that all fields can be edited if the pref is empty (4.68 KB, patch)
2014-10-10 21:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7673: add syspref in sysprefs.pl (3.87 KB, patch)
2014-10-10 21:40 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7673 [Terminology follow-up] New patron permissions to manage items (18.22 KB, patch)
2014-10-29 16:49 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 7673 [Terminology follow-up] New patron permissions to manage items (18.24 KB, patch)
2014-10-30 07:26 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 7673 [Terminology follow-up] New patron permissions to manage items (18.38 KB, patch)
2014-10-30 17:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7673: (RM followup) typo in sysprefs.sql (2.14 KB, patch)
2014-11-12 14:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description delaye 2012-03-08 12:59:27 UTC
refine permissions again : 
# limited_item_edition Limit item modification to barcode, status and note (please note that edit_item is still required)
# manage_shelves Allows to manage shelves
# delete_all_items Delete all items
# merge_from_shelves Allows to merge records from shelves 
# items_limited_batchmod Limit batch item modification to item status (please note that items_batchmod is still required)
Comment 1 Jonathan Druart 2012-03-30 12:03:29 UTC Comment hidden (obsolete)
Comment 2 Matthias Meusburger 2012-05-31 13:22:09 UTC Comment hidden (obsolete)
Comment 3 Matthias Meusburger 2012-05-31 13:26:23 UTC
Matthias, you are the original author of this patch, I assign it to you. Could you verify these permissions are still useful (see Bug 7310)

-> As far as i'm concerned, this patch is still relevant, as the permissions added are different from those added by Bug 7310

and this rebased patch is correct ?

-> cat-toolbar.inc didn't apply anymore
-> cataloguing/additem.pl had a bug that caused dysfonctions in limited_item_edition.

Moreover, I improved the whole patch in order to make it marcflavour-agnostic.
Comment 4 Katrin Fischer 2012-05-31 14:50:03 UTC
Hi Matthias, 

I was loooking at your description and I have a few questions:
Which status are you referring too? (damaged, notforloan, lost, restricted, ?)

I am not sure about the use case for limited_item_edition and items_limited_batchmod. Why the fields barcode, status and note? I am worried that this is something only a very limited number of libraries want to work this way, so not sure about those permissions.
Comment 5 Matthias Meusburger 2012-06-07 11:13:28 UTC
Hi Katrin,

Which status are you referring too? (damaged, notforloan, lost, restricted, ?)

-> notforloan

I am not sure about the use case for limited_item_edition and items_limited_batchmod.[...]

I agree with that. What if the user could define which fields have to be limited (or allowed, like in Bug 7412 :)) ? Would that be ok for you ?
Comment 6 Katrin Fischer 2012-06-07 13:56:41 UTC
Hi Matthias, I think I would really like being able to configure a list of item fields for the permission. :)
Comment 7 Matthias Meusburger 2012-06-10 10:12:33 UTC
After discussing with Katrin (hackfest), I'll implement something to specify the fields to *allow*.
Comment 8 delaye 2012-06-19 09:29:00 UTC Comment hidden (obsolete)
Comment 9 claire.hernandez@biblibre.com 2012-07-02 09:40:49 UTC
Comment on attachment 10360 [details]
liste de cb for test une sandbox one

bad buzilla ticket
Comment 10 delaye 2012-09-10 08:17:30 UTC
i have this message when i want apply this patch in a sandbox


Applying: Bug 7673 : New patron permissions
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging cataloguing/additem.pl
CONFLICT (content): Merge conflict in cataloguing/additem.pl
Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 7673 : New patron permissions
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 7673 - New patron permissions
Comment 11 Jonathan Druart 2012-09-10 08:36:40 UTC Comment hidden (obsolete)
Comment 12 Koha Team AMU 2012-10-23 08:38:45 UTC
Sandbox setup by koha.aixmarseille@gmail.com with database -1 and bug 7673 on Tue Oct 23 10:05:40 2012

Something went wrong !
Applying: Bug 7673 : New patron permissions
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc
Auto-merging tools/batchMod.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 7673 : New patron permissions
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 7673 - New patron permissions

Bug 7673 : New patron permissions
Apply? [yn]
Patch left in /tmp/Bug-7673--New-patron-permissions-dv6m4o.patch

=== translation installation ===

Posté le 2012-01-01 Modifier | Supprimer | Ajouter
Comment 13 Jonathan Druart 2012-10-23 09:49:11 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2012-11-21 14:33:50 UTC
limited_item_edition works, however it is enabled by default. Thus I cannot edit fields that I could pre-patch.

manage_shelves doesn't show up
merge_from_shelves doesn't show up

I think all the permissions that limit actions should be off by default.

If I am testing incorrectly please let me know!
Comment 15 Matthias Meusburger 2013-03-18 10:19:56 UTC Comment hidden (obsolete)
Comment 16 Matthias Meusburger 2013-03-18 10:22:02 UTC
Last attachment is a rebased patch.
Comment 17 Matthias Meusburger 2013-03-18 10:39:37 UTC
Kyle, I think the shelves permissions should not be here. I've created a separate bug for them ( http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9833 ).

As for the limited_item_edition, I do agree with you, it should not be enabled by default. And I don't see how it could be, as there's nothing in the code to enable it.
Comment 18 Matthias Meusburger 2013-03-18 13:32:26 UTC Comment hidden (obsolete)
Comment 19 Matthias Meusburger 2013-03-18 13:33:23 UTC
Katrin, this new patch allows to define which subfields should be allowed for edition, see commit message for more informations.
Comment 20 Koha Team AMU 2013-03-19 10:39:06 UTC
Hi

Test OK.
I tried separately the 3 permissions, with different subfields.

It would be improved if the comment of the syspref explain that we have to choose for exemple 995$f.
Comment 21 Matthias Meusburger 2013-03-19 11:07:22 UTC Comment hidden (obsolete)
Comment 22 Matthias Meusburger 2013-03-19 11:07:58 UTC
This new patch adds an example for both sysprefs.
Comment 23 Paul Poulain 2013-03-19 11:30:24 UTC
Patch tested with a sandbox, by Leila <koha.aixmarseille@gmail.com>
Comment 24 Paul Poulain 2013-03-19 11:30:46 UTC Comment hidden (obsolete)
Comment 25 Matthias Meusburger 2013-03-19 13:57:30 UTC
This new patch with examples has to be signed-off again.
Comment 26 I'm just a bot 2013-09-18 11:26:27 UTC
Applying: Bug 7673 : New patron permissions
Using index info to reconstruct a base tree...
M	cataloguing/additem.pl
M	installer/data/mysql/en/mandatory/userpermissions.sql
M	installer/data/mysql/sysprefs.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
M	tools/batchMod.pl
Falling back to patching base and 3-way merge...
Auto-merging tools/batchMod.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql
Auto-merging cataloguing/additem.pl
CONFLICT (content): Merge conflict in cataloguing/additem.pl
Patch failed at 0001 Bug 7673 : New patron permissions
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 27 Jonathan Druart 2013-12-03 14:10:45 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2013-12-03 14:10:55 UTC Comment hidden (obsolete)
Comment 29 Koha Team AMU 2013-12-03 16:03:31 UTC
Impossible to test because the permissions are missing.
We need  :
# limited_item_edition Limit item modification to barcode, status and note (please note that edit_item is still required)
# manage_shelves Allows to manage shelves
# delete_all_items Delete all items
# merge_from_shelves Allows to merge records from shelves 
# items_limited_batchmod
Comment 30 Jonathan Druart 2013-12-03 16:56:26 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2013-12-03 16:56:33 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2013-12-03 16:57:05 UTC
(In reply to Koha Team AMU from comment #29)
> Impossible to test because the permissions are missing.

Sorry, it was caused by the updatedb version. It should be fixed now.
Comment 33 Jonathan Druart 2014-03-10 13:32:05 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2014-03-10 13:32:11 UTC Comment hidden (obsolete)
Comment 35 Sonia Lemaire 2014-03-11 11:00:17 UTC
limited_item_edition : ok, but you can still delete all items. This should not be possible because it's not selected.

delete_all_items : ok

items_limited_batchmod : not ok. The fields don't appear.
Comment 36 Magnus Enger 2014-03-11 11:35:52 UTC
I applied the patch, then tried creating a librarian *without* the editcatalogue.delete_all_items permission. But this patron was still able to delete all items from a record. As far as I can see, the actual implementation of the permission is missing from the most reet versin of the patches. Specifically this part seems to be missing:

[% IF ( CAN_user_editcatalogue_delete_all_items ) %]{ text: _("Delete all items"), onclick: {fn: confirm_items_deletion }[% UNLESS ( count ) %],id:'disabled'[% END %] }[% END %]

See this for context:
http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9857&action=diff#a/koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc_sec1
Comment 37 Koha Team AMU 2014-03-26 14:40:46 UTC
I create a patron with theses permissions selected : 
- edit_catalogue
- edit_items
- limited_item_edition with SubfieldsToAllowForLimitedEdition syspref definied (995$f 995$o 995$u) 
- tools.items_batchmod
- tools.items_limited_batchmod with SubfieldsToAllowForLimitedBatchmod syspref defined (995$o)
- fast_cataloging

- delete_all_items not selected


1/ I can modify the items' subfields definied, but it's only possible in the items of the default's framework and not with items from the fast cataloging framework. In fact i just can modify the subfield definied by the syspref SubfieldsToAllowForLimitedEdition. 
In the fast cataloging's framework we don't need the same subfields like in the default's framework (we need only 995$b $c $e $f $h $k $m $n $o $u).

2/ I can still delete all items. This should not be possible because it's not selected.
Comment 38 Jonathan Druart 2014-03-31 12:09:13 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2014-03-31 12:09:57 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2014-04-01 08:35:21 UTC Comment hidden (obsolete)
Comment 41 Koha Team AMU 2014-04-01 15:00:59 UTC
 - The pref SubfieldsToAllowForLimitedEdition should not affect Fast
cataloguing framework (FA) : ok the framework FA is exclued

- The delete_all_items is ok
Comment 42 Katrin Fischer 2014-04-18 11:00:44 UTC
Hi Julian,

I did some initial testing and code review:

1) I wonder about the names of the new permissions.
   I think a limited edition would be something different - maybe we
   can get a native speaker's opinion on this?
   My suggestion would be:
   - limited_item_edition = restricted_edit_items
   - items_limited_batchmod = restricted_item_batchmod

2) Please include all .sql permission files, so new installations won't 
   miss them. You can check with xt/permissions.t

3) Fix sequence of system preferences
 FAIL   installer/data/mysql/sysprefs.sql
   FAIL   sysprefs_order
        Not blocker: Sysprefs SubfieldsToAllowForLimitedBatchmod 
        is bad placed (see bug 10610)

4) If we are going to rewrite this bit of code, it would be good to 
   remove the use of CGI::scrolling_list (bug 766):
+                $subfield_data{marc_value} =CGI::scrolling_list(@scrparam);

5) System preference description could be more clear. It should note that
   the FA framework is excluded from the permission. Also it could include
   a hint that the example is for UNIMARC and also include a MARC21 example
   to avoid confusion. For us developers it's clear that we use different
   item fields for both flavours, but not so much for librarians I guess.

6) I have my doubts about the superlibrarian checks you used:
&& $limitededition->{'superlibrarian'} eq 1);
+$limitededition = 0 if ($limitededition != 0 && $limitededition->{'superlibrarian'} eq 1);
   We now have a specific method to check those: C4::Context::IsSuperLibrarian()
Comment 43 Katrin Fischer 2014-04-18 11:04:50 UTC
Sorry, I missed that this patch is from Matthias... and Jonathan?
Comment 44 Kyle M Hall 2014-04-18 11:06:40 UTC
> 1) I wonder about the names of the new permissions.
>    I think a limited edition would be something different - maybe we
>    can get a native speaker's opinion on this?
>    My suggestion would be:
>    - limited_item_edition = restricted_edit_items
>    - items_limited_batchmod = restricted_item_batchmod

I agree with Katrin on this. 

I would suggest limited_item_edition => edit_items_restricted so that it stays right near edit_items on the permissions screen.

I would also suggest items_limited_batchmod = item_batchmod_restricted in case item_batchmod becomes a permission, which I'm sure it will in the future.
Comment 45 Kyle M Hall 2014-04-18 11:19:15 UTC
> I would also suggest items_limited_batchmod = item_batchmod_restricted in
> case item_batchmod becomes a permission, which I'm sure it will in the
> future.

Correction item_batchmod *is* a permission now.
Comment 46 Katrin Fischer 2014-04-18 11:24:13 UTC
Kyle's suggestions sound good to me - valid point about the sorting.
Comment 47 Jonathan Druart 2014-04-25 14:54:39 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2014-04-25 14:54:46 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2014-04-25 14:54:52 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2014-04-25 14:54:57 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2014-04-25 14:55:01 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2014-04-25 14:55:07 UTC Comment hidden (obsolete)
Comment 53 Jonathan Druart 2014-04-25 14:55:12 UTC Comment hidden (obsolete)
Comment 54 Jonathan Druart 2014-04-25 14:55:16 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2014-04-25 14:58:48 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2014-04-25 15:06:03 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2014-04-25 15:07:46 UTC
(In reply to Katrin Fischer from comment #42)

> 1) I wonder about the names of the new permissions.
>    I think a limited edition would be something different - maybe we
>    can get a native speaker's opinion on this?

Done.


> 2) Please include all .sql permission files, so new installations won't 
>    miss them. You can check with xt/permissions.t

Done.


> 3) Fix sequence of system preferences

Done.

> 4) If we are going to rewrite this bit of code, it would be good to 
>    remove the use of CGI::scrolling_list (bug 766):
> +                $subfield_data{marc_value} =CGI::scrolling_list(@scrparam);

It will be done when bug 766 will be pushed.

> 5) System preference description could be more clear. It should note that
>    the FA framework is excluded from the permission. Also it could include
>    a hint that the example is for UNIMARC and also include a MARC21 example
>    to avoid confusion. For us developers it's clear that we use different
>    item fields for both flavours, but not so much for librarians I guess.

Done.

> 6) I have my doubts about the superlibrarian checks you used:
> && $limitededition->{'superlibrarian'} eq 1);
> +$limitededition = 0 if ($limitededition != 0 &&
> $limitededition->{'superlibrarian'} eq 1);
>    We now have a specific method to check those:
> C4::Context::IsSuperLibrarian()

Done.
Comment 58 Koha Team AMU 2014-05-02 15:00:04 UTC
I just test this part of the patch.

1/ delete_all-items : ok

2/ New test with these permissions:

- edit catalogue:
edit_catalogue Edit catalogue
edit_items Editer les exemplaires
fast_cataloging Fast cataloging
edit_items_restricted Limit item modification to barcode, status and note

- tools :
items_batchmod Perform batch modification of items
items_limited_batchmod resctricted : limit item modification to item status 

- sysprefs :
SubfieldsToAllowForRestrictedEdition : 995$f 995$o 995$u
SubfieldsToAllowForRestrictedBatchmod : 995 $o

test's result :

- ok : the modification of items is retricted to the subfilds definied in these 2 sysprefs.

- ok : The modification of all subfields of an item of a fast cataloging is ok. The framework fast cataloging is not concerned by SubfieldsToAllowForRestrictedEdition
Comment 59 Jonathan Druart 2014-05-07 09:12:04 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2014-05-07 09:12:09 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2014-05-07 09:12:13 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2014-05-07 09:12:18 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2014-05-07 09:12:21 UTC Comment hidden (obsolete)
Comment 64 Jonathan Druart 2014-05-07 09:12:26 UTC Comment hidden (obsolete)
Comment 65 Jonathan Druart 2014-05-07 09:12:30 UTC Comment hidden (obsolete)
Comment 66 Jonathan Druart 2014-05-07 09:12:34 UTC Comment hidden (obsolete)
Comment 67 Jonathan Druart 2014-05-07 09:12:38 UTC Comment hidden (obsolete)
Comment 68 Jonathan Druart 2014-05-07 09:13:36 UTC
I added the Koha Team AMU' signoff to the patches.
Comment 69 Katrin Fischer 2014-07-06 21:27:09 UTC
I fixed the conflicts, but don't want to make too many changes and maybe mistakes. One of the tests fails:

#   Failed test 'No user permissions to add'
#   at xt/permissions.t line 95.
# User permissions to add in uk-UA/mandatory/permissions_and_user_flags.sql: delete_all_items, edit_items_restricted, items_batchmod_restricted

#   Failed test 'No user permissions to add'
#   at xt/permissions.t line 95.
# User permissions to add in ru-RU/mandatory/permissions_and_user_flags.sql: delete_all_items, edit_items_restricted, items_batchmod_restricted
# Looks like you failed 2 tests of 34.

Could you take a look Jonathan?
Comment 70 Jonathan Druart 2014-07-07 14:16:28 UTC Comment hidden (obsolete)
Comment 71 Katrin Fischer 2014-08-04 21:13:09 UTC
I am sorry, there is a conflict in additem.pl - I fixed it, but then can't get some of the later patches to apply :(

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 7673: Update syspref names in script
fatal: sha1 information is lacking or useless (cataloguing/additem.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7673: Update syspref names in script
Comment 72 Jonathan Druart 2014-08-18 09:10:06 UTC Comment hidden (obsolete)
Comment 73 Jonathan Druart 2014-08-18 09:10:11 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2014-08-18 09:10:15 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2014-08-18 09:10:21 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2014-08-18 09:10:26 UTC Comment hidden (obsolete)
Comment 77 Jonathan Druart 2014-08-18 09:10:32 UTC Comment hidden (obsolete)
Comment 78 Jonathan Druart 2014-08-18 09:10:37 UTC Comment hidden (obsolete)
Comment 79 Jonathan Druart 2014-08-18 09:10:41 UTC Comment hidden (obsolete)
Comment 80 Jonathan Druart 2014-08-18 09:10:47 UTC Comment hidden (obsolete)
Comment 81 Jonathan Druart 2014-08-18 09:10:53 UTC Comment hidden (obsolete)
Comment 82 Katrin Fischer 2014-08-18 13:13:37 UTC
From code review: 

1) (normal) The descriptions for edit_items_restricted and items_bachmod_restricted are now no longer correct, since the preferences have been added to avoid hardcoding MARC fields - maybe refer to the prefs instead?

2) (non blocker) Patch keeps(?) a CGI::scrolling_list instance - could be in conflict with one of Bernardo's patches there:
additem.pl:
$subfield_data{marc_value} =CGI::scrolling_list(@scrparam);

3) (non blocker) Database updates could be merged.

3) Suggestion: should we squash the patches (maybe 1-9 + )? Because the permissions got renamed and some other things that could avoid some confusion. 

Also an updated summary of the changes and a test plan would be great.
Comment 83 Martin Renvoize 2014-08-18 13:22:52 UTC
Some Minor comments to start the ball rolling.. Nothing fail worthy so far.

1. Z3950 references in the staff client. There are references to Z3950 in a number of places in the staff client, for instance admin-home where we go to configure z3950 targets should make it clear that we're now configuruing z3950 OR sru.. and 'Add new from z3950' under cataloguing should also be updated.  Perhaps 'New from external database' or similar?
2. 'Database' on the 'Add Server' page is a required field, but it's also not obviose what it expects
3. I'de like a more visual queue somewhere in the Add SRU server page that one really aught to define the 'SRU Search fields mappings' else all searches will be against the 'any' index on the remote server.  Maybe this is a documentation thing.  Either a stronger hint to fill that stuff in.. or to drop the index specific search boxes in the 'Add from ....' popup when the sru server is selected.. Hard call really.. we want people to map them to get sensible results really.  Maybe set some sensible defaults and make it a required field?

Back to testing
Comment 84 Martin Renvoize 2014-08-18 13:23:14 UTC
poo.. worng bug for the above comment.
Comment 85 Jonathan Druart 2014-08-20 10:44:57 UTC Comment hidden (obsolete)
Comment 86 Jonathan Druart 2014-08-20 10:45:44 UTC Comment hidden (obsolete)
Comment 87 Jonathan Druart 2014-08-20 10:45:49 UTC Comment hidden (obsolete)
Comment 88 Jonathan Druart 2014-08-20 10:45:55 UTC Comment hidden (obsolete)
Comment 89 Jonathan Druart 2014-08-20 10:46:02 UTC Comment hidden (obsolete)
Comment 90 Jonathan Druart 2014-08-20 10:46:10 UTC Comment hidden (obsolete)
Comment 91 Jonathan Druart 2014-08-20 10:46:15 UTC Comment hidden (obsolete)
Comment 92 Jonathan Druart 2014-08-20 10:46:25 UTC Comment hidden (obsolete)
Comment 93 Jonathan Druart 2014-08-20 10:56:19 UTC
(In reply to Katrin Fischer from comment #82)
> From code review: 
> 
> 1) (normal) The descriptions for edit_items_restricted and
> items_bachmod_restricted are now no longer correct, since the preferences
> have been added to avoid hardcoding MARC fields - maybe refer to the prefs
> instead?

Done.

> 2) (non blocker) Patch keeps(?) a CGI::scrolling_list instance - could be in
> conflict with one of Bernardo's patches there:
> additem.pl:
> $subfield_data{marc_value} =CGI::scrolling_list(@scrparam);

Yes, it will be in conflict, but seems quite easy to fix.

> 3) (non blocker) Database updates could be merged.

Done.

> 3) Suggestion: should we squash the patches (maybe 1-9 + )? Because the
> permissions got renamed and some other things that could avoid some
> confusion. 

I squashed DB patches. I would like to keep the history (especially because I'm not the author of the first patch).

> Also an updated summary of the changes and a test plan would be great.

I forgot that...
Comment 94 Jonathan Druart 2014-08-20 11:03:42 UTC
Test plan:
1/ add the following permissions to the logged in patron:
edit_item, edit_items_restricted, delete_all_items, items_batchmod, items_batchmod_restricted
2/ Fill the prefs SubfieldsToAllowForRestrictedEdition and SubfieldsToAllowForRestrictedBatchmod with some subfield (for instance "995$f 995$o" and "995$o")
3/ Verify you are allowed to edit the item fields defined in the pref SubfieldsToAllowForRestrictedEdition.
4/ Try to edit item in a batch and verify you are allowed to edit the item fields defined in the pref SubfieldsToAllowForRestrictedBatchmod.
5/ Try to delete all items of a record

Play with the pref/permissions and verify they are correctly take into account.
Comment 95 Katrin Fischer 2014-10-10 19:19:14 UTC
Hi Jonathan, 

a few small things: 

1) The permission description for items_batchmod_restricted has some small typos.
2) sysprefs.sql is missing.
3) If you have items_batchmod and items_batchmod_restricted both checked (which
   happens automatically with the database update - the items form is not
   showing up on batch modification, because the pref is empty by default.
   After rereading the description - maybe we should just not check the   
   permission on database update then it should not change any behaviour.
Comment 96 Jonathan Druart 2014-10-10 19:52:29 UTC Comment hidden (obsolete)
Comment 97 Jonathan Druart 2014-10-10 19:52:36 UTC Comment hidden (obsolete)
Comment 98 Katrin Fischer 2014-10-10 21:37:13 UTC
Created attachment 32215 [details] [review]
[PASSED QA] Bug 7673 : New patron permissions

    This patch adds the following permissions:

     - editcatalogue.limited_item_edition: Limit item modification to barcode, status and note
     - editcatalogue.delete_all_items: Delete all items at once
     - tools.items_limited_batchmod: Limit batch item modification to item status

    The SubfieldsToAllowForLimitedEdition syspref is used to define which subfields can be edited
    when the editcatalogue.limited_item_edition permission is enabled.

    In the same way, the SubfieldsToAllowForLimitedBatchmod is used to define which subfields
    can be edited when the tools.items_limited_batchmod permission is enabled.

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 99 Katrin Fischer 2014-10-10 21:37:48 UTC
Created attachment 32217 [details] [review]
[PASSED QA] Bug 7673: QA Followup: trailing whitespace and perlcritic issues

This patch fixes the following qa issues:

 FAIL   cataloguing/additem.pl
   FAIL   forbidden patterns
        forbidden pattern: trailing space char (line 833)
   FAIL   critic
        # Variables::ProhibitConditionalDeclarations: Got 2 violation(s).

 FAIL   tools/batchMod.pl
   FAIL   critic
        # Variables::ProhibitConditionalDeclarations: Got 2 violation(s).

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 100 Katrin Fischer 2014-10-10 21:38:18 UTC
Created attachment 32219 [details] [review]
[PASSED QA] Bug 7673: The delete all items link should only be displayed if delete_all_items is set

This test had been lost in a rebase.

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 101 Katrin Fischer 2014-10-10 21:38:38 UTC
Created attachment 32220 [details] [review]
[PASSED QA] Bug 7673: the limitation on editing an item does not affect FA framework

The pref SubfieldsToAllowForLimitedEdition should not affect Fast
cataloguing framework (FA).

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 102 Katrin Fischer 2014-10-10 21:39:08 UTC
Created attachment 32222 [details] [review]
[PASSED QA] Bug 7673: Update syspref names in script

The both syspref names have been changed since the first patch.

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 103 Katrin Fischer 2014-10-10 21:39:21 UTC
Created attachment 32224 [details] [review]
[PASSED QA] Bug 7673: Update permission names in script

Two permission names have been changed since the first patch.

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 104 Katrin Fischer 2014-10-10 21:39:36 UTC
Created attachment 32225 [details] [review]
[PASSED QA] Bug 7673: Change the superlibrarian check

To know if the user is a superlibrarian, we have to call
C4::Context->IsSuperLibrarian

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 105 Katrin Fischer 2014-10-10 21:39:51 UTC
Created attachment 32226 [details] [review]
[PASSED QA] Bug 7673: DB changes - Add permissions

This patch adds:
3 permissions:
- edit_items_restricted
- delete_all_items
- items_batchmod_restricted

2 system preferences:
- SubfieldsToAllowForRestrictedEdition
- SubfieldsToAllowForRestrictedBatchmod.

Signed-off-by: Koha Team AMU <koha.aixmarseille@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 106 Katrin Fischer 2014-10-10 21:40:01 UTC
Created attachment 32227 [details] [review]
[PASSED QA] Bug 7673: Assume that all fields can be edited if the pref is empty

If the sysprefs are empty, we assume that the librarian can edit all
subfields, even if s/he has the restricted permission.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 107 Katrin Fischer 2014-10-10 21:40:17 UTC
Created attachment 32228 [details] [review]
[PASSED QA] Bug 7673: add syspref in sysprefs.pl

Also fix a typo in the permission description

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Patches pass QA script and tests.

Copying the test plan from the bug report:
Test plan:
1/ add the following permissions to the logged in patron:
edit_item, edit_items_restricted, delete_all_items,
items_batchmod, items_batchmod_restricted

2/ Fill the prefs SubfieldsToAllowForRestrictedEdition
and SubfieldsToAllowForRestrictedBatchmod with some
subfield (for instance "995$f 995$o" and "995$o")

3/ Verify you are allowed to edit the item fields defined
in the pref SubfieldsToAllowForRestrictedEdition.

4/ Try to edit item in a batch and verify you are allowed
to edit the item fields defined in the pref
SubfieldsToAllowForRestrictedBatchmod.

5/ Try to delete all items of a record

Play with the pref/permissions and verify they are
correctly taken into account.
Comment 108 Owen Leonard 2014-10-29 16:49:58 UTC Comment hidden (obsolete)
Comment 109 Chris Cormack 2014-10-30 07:26:10 UTC Comment hidden (obsolete)
Comment 110 Katrin Fischer 2014-10-30 17:55:47 UTC
Created attachment 32993 [details] [review]
[PASSED QA] Bug 7673 [Terminology follow-up] New patron permissions to manage items

This bug adds a new preference, "SubfieldsToAllowForRestrictedEdition,"
but the use of the term "Edition" in this context is incorrect. I think
it would be more clear to change the preference name to
"SubfieldsToAllowForRestrictedEditing." This patch makes this change.

I realize this isn't a big issue since the preference has a good
description, but I thought that if we were going to make this as clear
as possible now would be the time to do it.

To test, start with a database which hasn't previously been used to test
Bug 7673. Apply all patches and run the database update. Follow the test
plan as described in the bug.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described, change appears complete.
All tests and QA script still pass.
Comment 111 Tomás Cohen Arazi 2014-11-11 17:13:27 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 112 Tomás Cohen Arazi 2014-11-12 14:21:32 UTC
Created attachment 33487 [details] [review]
Bug 7673: (RM followup) typo in sysprefs.sql

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>