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.

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.

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 ?

Hi Matthias, I think I would really like being able to configure a list of item fields for the permission. :)

After discussing with Katrin (hackfest), I'll implement something to specify the fields to *allow*.

Comment on attachment 10360 [details]
liste de cb for test une sandbox one

bad buzilla ticket

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

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

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!

Last attachment is a rebased patch.

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.

Katrin, this new patch allows to define which subfields should be allowed for edition, see commit message for more informations.

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.

This new patch adds an example for both sysprefs.

Patch tested with a sandbox, by Leila <koha.aixmarseille@gmail.com>

This new patch with examples has to be signed-off again.

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".

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

(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.

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.

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

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.

 - The pref SubfieldsToAllowForLimitedEdition should not affect Fast
cataloguing framework (FA) : ok the framework FA is exclued

- The delete_all_items is ok

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()

Sorry, I missed that this patch is from Matthias... and Jonathan?

> 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.

> 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.

Kyle's suggestions sound good to me - valid point about the sorting.

(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.

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

I added the Koha Team AMU' signoff to the patches.

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?

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

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.

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

poo.. worng bug for the above comment.

(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...

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.

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.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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.

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.

Patches pushed to master.

Thanks Jonathan!

Created attachment 33487 [details] [review]
Bug 7673: (RM followup) typo in sysprefs.sql

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