|
Description
Kyle M Hall (khall)
2018-02-21 08:00:10 UTC
Created attachment 87226 [details] [review] Bug 20256: Update database Created attachment 87227 [details] [review] Bug 20256: Add new permission to editor Created attachment 87228 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Created attachment 87229 [details] [review] Bug 20256: Add new methods for checking item editing permissions Created attachment 87230 [details] [review] Bug 20256: Update Schema file Created attachment 87231 [details] [review] Bug 20256: Update groups editor Created attachment 87232 [details] [review] Bug 20256: Use new methods Created attachment 87233 [details] [review] Bug 20256: Add unit tests Needs a test plan... Created attachment 87262 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 87263 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 87264 [details] [review] Bug 20256: Add new permission to editor Created attachment 87265 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Created attachment 87266 [details] [review] Bug 20256: Add new methods for checking item editing permissions Created attachment 87267 [details] [review] Bug 20256: Update Schema file Created attachment 87268 [details] [review] Bug 20256: Update groups editor Created attachment 87269 [details] [review] Bug 20256: Use new methods Created attachment 87270 [details] [review] Bug 20256: Add unit tests Created attachment 88537 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88539 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88541 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88542 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88543 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88544 [details] [review] Bug 20256: Update Schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88545 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88546 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 88547 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Comment on attachment 88546 [details] [review] Bug 20256: Use new methods Review of attachment 88546 [details] [review]: ----------------------------------------------------------------- ::: C4/Items.pm @@ +1746,4 @@ > $status = "book_on_loan"; > } > elsif ( defined C4::Context->userenv > + and !Koha::Patrons->find( C4::Context->userenv->{number} )->can_edit_item( $item ) ) What if patron does not exist? This subroutine is called by misc/cronjobs/delete_items.pl ::: catalogue/detail.pl @@ +340,4 @@ > $item->{'course_reserves'} = GetItemCourseReservesInfo( itemnumber => $item->{'itemnumber'} ); > } > > + $item->{cannot_be_edited} = !$patron->can_edit_item( $item_object ); Why do you use negative and then again negative (UNLESS) in template? ::: cataloguing/additem.pl @@ +810,3 @@ > #verifying rights > + unless ( $patron->can_edit_item($subfieldvalue) ) { > + warn "NOMOD"; debug message, should be removed ::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt @@ +223,5 @@ > [% IF (StaffDetailItemSelection) %] > <td style="text-align:center;vertical-align:middle"> > + [% UNLESS item.cannot_be_edited %] > + <input type="checkbox" value="[% item.itemnumber | html %]" name="itemnumber" /> > + [% END %] This could be [% IF item.can_be_edited %] See my previous comment on detail.pl Comment on attachment 88543 [details] [review] Bug 20256: Add new methods for checking item editing permissions Review of attachment 88543 [details] [review]: ----------------------------------------------------------------- ::: Koha/Patron.pm @@ +1147,5 @@ > } > > +=head3 libraries_where_can_see_patrons > + > +my $libraries = $patron-libraries_where_can_see_patrons; Should be $patron->libraries_where_can_see_patrons; missing ">" character I think it is great enhancement, just need to polish some little issues ;) Comment on attachment 88546 [details] [review] Bug 20256: Use new methods Review of attachment 88546 [details] [review]: ----------------------------------------------------------------- ::: C4/Items.pm @@ +1746,4 @@ > $status = "book_on_loan"; > } > elsif ( defined C4::Context->userenv > + and !Koha::Patrons->find( C4::Context->userenv->{number} )->can_edit_item( $item ) ) cli scripts have no userenv, so this is cli safe. ::: catalogue/detail.pl @@ +340,4 @@ > $item->{'course_reserves'} = GetItemCourseReservesInfo( itemnumber => $item->{'itemnumber'} ); > } > > + $item->{cannot_be_edited} = !$patron->can_edit_item( $item_object ); Fair enough. I'll submit a followup for fix that! Created attachment 88632 [details] [review] Bug 20256: (QA Follow-up) Remove double negative from code (In reply to Kyle M Hall from comment #31) > Comment on attachment 88546 [details] [review] [review] > Bug 20256: Use new methods > > Review of attachment 88546 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: C4/Items.pm > @@ +1746,4 @@ > > $status = "book_on_loan"; > > } > > elsif ( defined C4::Context->userenv > > + and !Koha::Patrons->find( C4::Context->userenv->{number} )->can_edit_item( $item ) ) > > cli scripts have no userenv, so this is cli safe. Sorry, I totally misread the code, you are right (In reply to Josef Moravec from comment #33) > (In reply to Kyle M Hall from comment #31) > > Comment on attachment 88546 [details] [review] [review] [review] > > Bug 20256: Use new methods > > > > Review of attachment 88546 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: C4/Items.pm > > @@ +1746,4 @@ > > > $status = "book_on_loan"; > > > } > > > elsif ( defined C4::Context->userenv > > > + and !Koha::Patrons->find( C4::Context->userenv->{number} )->can_edit_item( $item ) ) > > > > cli scripts have no userenv, so this is cli safe. > > > Sorry, I totally misread the code, you are right Created attachment 88702 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Created attachment 88703 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Created attachment 88704 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Created attachment 88705 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing (In reply to Josef Moravec from comment #33) > (In reply to Kyle M Hall from comment #31) > > Comment on attachment 88546 [details] [review] [review] [review] > > Bug 20256: Use new methods > > > > Review of attachment 88546 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: C4/Items.pm > > @@ +1746,4 @@ > > > $status = "book_on_loan"; > > > } > > > elsif ( defined C4::Context->userenv > > > + and !Koha::Patrons->find( C4::Context->userenv->{number} )->can_edit_item( $item ) ) > > > > cli scripts have no userenv, so this is cli safe. > > > Sorry, I totally misread the code, you are right I misread the code, but it is still valid, since bug 22600 added userenv to commandline/cron scripts... I added follow-up for this and for some other minor issues. But there is still one bigger problem: On biblio detail page, the checkbox is shown only when user can edit the item, but the edit button in the most right column of items table does not respect this logic. Even worse, I am able to edit the item, when i click it or reach the edit item form directly. This should be fixed on module level I think... Created attachment 88935 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Created attachment 88936 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Quick review:
1. ->can_edit_item
if ( C4::Context->preference('IndependentBranches') ) {
return $userenv->{branch} eq $branchcode;
}
We certainly want to handle non-existing $userenv
2. On update, should not we set edit_any_item for patrons who where already allowed to edit items?
3. on /cataloguing/additem.pl, the whole dropdown list (edit, delete, print, etc.) is not displayed, I would expect to see some of them
4. You can add items to other libraries but then you cannot longer edit them?
Maybe add and update should be impacted by this change?
5. Restriction is only done at template level (at least for batchMod), there should be a low level exception raised if the logged in user is trying to edit/delete (add?) an item outside of their group.
Patches still apply, QA tools look good. Can you please take a look at Joubu's questions on comment 42? I think it would be nice to also limit update and add... as otherwise this leaves potential for 'locking yourself out' of an item you just added. Created attachment 96015 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 96016 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 96017 [details] [review] Bug 20256: Add new permission to editor Created attachment 96018 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Created attachment 96019 [details] [review] Bug 20256: Add new methods for checking item editing permissions Created attachment 96020 [details] [review] Bug 20256: Update Schema file Created attachment 96021 [details] [review] Bug 20256: Update groups editor Created attachment 96022 [details] [review] Bug 20256: Use new methods Created attachment 96023 [details] [review] Bug 20256: Add unit tests Created attachment 96024 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Created attachment 96025 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Created attachment 96026 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Created attachment 96027 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Created attachment 96028 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Created attachment 96029 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Created attachment 96030 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Created attachment 96031 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Created attachment 96032 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Created attachment 96033 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Hi Kyle, I am not able to test: Can't call method "libraries_where_can_edit_items" on an undefined value at /kohadevbox/koha/cataloguing/additem.pl line 178 Also, do not you think "edit_any_items" description should refer to "libraries in the same group" and not only "home library"? *** Bug 22923 has been marked as a duplicate of this bug. *** Created attachment 102622 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 102623 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 102624 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 102625 [details] [review] Bug 20256: Add new permission to editor Created attachment 102626 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Created attachment 102627 [details] [review] Bug 20256: Add new methods for checking item editing permissions Created attachment 102628 [details] [review] Bug 20256: Update groups editor Created attachment 102629 [details] [review] Bug 20256: Use new methods Created attachment 102630 [details] [review] Bug 20256: Add unit tests Created attachment 102631 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Created attachment 102632 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Created attachment 102633 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Created attachment 102634 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Created attachment 102635 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Created attachment 102636 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Created attachment 102637 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Created attachment 102638 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Created attachment 102639 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Created attachment 102640 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Created attachment 102641 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Created attachment 102642 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] In my tests this is working correctly. I would like to have a more thorough test plan because I worry I was missing something I should have been looking at. However, there are a couple of complaints by the QA tool which need to be addressed. Created attachment 102703 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 102704 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Created attachment 102705 [details] [review] Bug 20256: Add new permission to editor Created attachment 102706 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Created attachment 102707 [details] [review] Bug 20256: Add new methods for checking item editing permissions Created attachment 102708 [details] [review] Bug 20256: Update groups editor Created attachment 102709 [details] [review] Bug 20256: Use new methods Created attachment 102710 [details] [review] Bug 20256: Add unit tests Created attachment 102711 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Created attachment 102712 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Created attachment 102713 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Created attachment 102714 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Created attachment 102715 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Created attachment 102716 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Created attachment 102717 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Created attachment 102718 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Created attachment 102719 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Created attachment 102720 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Created attachment 102721 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Created attachment 102722 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Test Plan: * Apply this patch set * Run updatedatabase.pl * Restart all the things! * Create two library groups with half the libraries in each * Enable the new item editing limits for the groups * Note you can now only edit items from the same group as your logged in users home library * Verify you cannot edit other library group's items using the batch item editor * Give your user the new "edit_any_item" user permission, verify the user can now edit any item Created attachment 102732 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102733 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102734 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102735 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102736 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102737 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102738 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102739 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102740 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102741 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102742 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102743 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102744 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102745 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102746 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102747 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102748 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102749 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102750 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 102751 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Patches curently no longer apply, can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20256: Update database Applying: Bug 20256: Add new permission to editor Applying: Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Applying: Bug 20256: Add new methods for checking item editing permissions Applying: Bug 20256: Update groups editor Applying: Bug 20256: Use new methods Applying: Bug 20256: Add unit tests Using index info to reconstruct a base tree... M t/db_dependent/Koha/Patrons.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Koha/Patrons.t Applying: Bug 20256: (QA follow-up) Remove double negative from code Applying: Bug 20256: (QA follow-up) Change meaning should change code Applying: Bug 20256: (QA follow-up) test borrowernumber in userenv too Applying: Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Applying: Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Applying: Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Applying: Bug 20256: (QA follow-up) Handle non-existing userenv Applying: Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Applying: Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit .git/rebase-apply/patch:14: trailing whitespace. my $i = 1; error: sha1 information is lacking or useless (tools/batchMod.pl). error: could not build fake ancestor Patch failed at 0001 Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-20256-QA-follow-up-Skip-batch-editing-items-th-PQV8Ge.patch Created attachment 107312 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107313 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107314 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107315 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107316 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107317 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107318 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107319 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107320 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107321 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107322 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107323 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107324 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107325 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107326 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107327 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107328 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107329 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 107330 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> This no longer applies, please rebase! Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20256: Update database Applying: Bug 20256: Add new permission to editor Applying: Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Applying: Bug 20256: Add new methods for checking item editing permissions Applying: Bug 20256: Update groups editor Applying: Bug 20256: Use new methods Using index info to reconstruct a base tree... M Koha/Item.pm M catalogue/detail.pl M catalogue/moredetail.pl M cataloguing/additem.pl M koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.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/catalogue/detail.tt Auto-merging cataloguing/additem.pl Auto-merging catalogue/moredetail.pl Auto-merging catalogue/detail.pl Auto-merging Koha/Item.pm Applying: Bug 20256: Add unit tests Applying: Bug 20256: (QA follow-up) Remove double negative from code Applying: Bug 20256: (QA follow-up) Change meaning should change code Applying: Bug 20256: (QA follow-up) test borrowernumber in userenv too Applying: Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Applying: Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Applying: Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item error: sha1 information is lacking or useless (cataloguing/additem.pl). error: could not build fake ancestor Patch failed at 0001 Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-20256-QA-follow-up-Redirect-to-record-details--3RxR9W.patch Created attachment 108851 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108852 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108853 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108854 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108855 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108856 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108857 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108858 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108859 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108860 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108861 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108862 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108863 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108864 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108865 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108866 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108867 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108868 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108869 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 108870 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Hi Kyle, first review here: 1) QA tools: FAIL cataloguing/additem.pl FAIL valid "my" variable $item masks earlier declaration in same scope 2) Unit tests Ran the database update, but tests are failing: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Patrons.t .. 33/41 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Duplicate entry 'OPLKEwkZw0DMeIN2d05GnFYtiGaCnqBtN4N03FSuWrNz' for key 'userid' [for Statement "UPDATE `borrowers` SET `userid` = ? WHERE ( `borrowernumber` = ? )" with ParamValues: 0='OPLKEwkZw0DMeIN2d05GnFYtiGaCnqBtN4N03FSuWrNz', 1='199'] at /home/vagrant/kohaclone/Koha/Object.pm line 166 t/db_dependent/Koha/Patrons.t .. 42/41 # Looks like you planned 41 tests but ran 42. t/db_dependent/Koha/Patrons.t .. Dubious, test returned 255 (wstat 65280, 0xff00) All 41 subtests passed Test Summary Report ------------------- t/db_dependent/Koha/Patrons.t (Wstat: 65280 Tests: 42 Failed: 1) Failed test: 42 Non-zero exit status: 255 Parse errors: Bad plan. You planned 41 tests but ran 42. 3) Methods libraries_where_can_see_patrons is added to Patron.pm I am not sure how it fits into this functionality, maybe a leftover from an earlier development? 4) Item edit checks I think we might also need a check on the permission from the item search edit item link. Created attachment 109939 [details] [review] Bug 20256: (QA follow-up) Fix variable masking warning in additem.pl > 1) QA tools: > > FAIL cataloguing/additem.pl > FAIL valid > "my" variable $item masks earlier declaration in same scope Fixed! > 2) Unit tests > > Ran the database update, but tests are failing: I wasn't able to recreate your unit test failure. Can you try again? > 3) Methods > > libraries_where_can_see_patrons is added to Patron.pm > I am not sure how it fits into this functionality, maybe a leftover from an > earlier development? This subroutine is from Jonathan's patch that added group limits for viewing patrons. This patch set takes that logic and makes it more generic for use with other objects that also have branchcodes. libraries_where_can_see_patrons is the specific implementation for jonathans previous feature, which is now a wrapper around the more generic method that can also be used for items and other objects in the future. > 4) Item edit checks > > I think we might also need a check on the permission from the item search > edit item link. I've been working on this patch, but it occurs to me that the edit item link shows even if a record has no items. It's a useful way to go straight to the items editor to create a new item. As such, I don't think we should hide the link because it can still be used in that way even if the librarian can't edit any existing items.
> > 4) Item edit checks
> >
> > I think we might also need a check on the permission from the item search
> > edit item link.
>
> I've been working on this patch, but it occurs to me that the edit item link
> shows even if a record has no items. It's a useful way to go straight to the
> items editor to create a new item. As such, I don't think we should hide the
> link because it can still be used in that way even if the librarian can't
> edit any existing items.
Hi Kyle, I meant the "item search" - in this case the form for editing this specific item opens. So I think it makes sense to hide it there and I think there is already some logic for that now.
Created attachment 110005 [details] [review] Bug 20256: (QA follow-up) Fix number of tests in Patrons.t (In reply to Katrin Fischer from comment #172) > > > 4) Item edit checks > > > > > > I think we might also need a check on the permission from the item search > > > edit item link. > > > > I've been working on this patch, but it occurs to me that the edit item link > > shows even if a record has no items. It's a useful way to go straight to the > > items editor to create a new item. As such, I don't think we should hide the > > link because it can still be used in that way even if the librarian can't > > edit any existing items. > > Hi Kyle, I meant the "item search" - in this case the form for editing this > specific item opens. So I think it makes sense to hide it there and I think > there is already some logic for that now. I identified 3 places now that require action: - Edit item link from course reserves - Edit item link form item search (not catalog search) - Edit item link from items tab (moredetail.pl) Created attachment 113706 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113707 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113708 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113709 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113710 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113711 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113712 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113713 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113714 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113715 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113716 [details] [review] Bug 20256: (QA follow-up) Fix column description for ft_limit_item_editing Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113717 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113718 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113719 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113720 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113721 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113722 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113723 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113724 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 113725 [details] [review] Bug 20256: (QA follow-up) Fix variable masking warning in additem.pl Created attachment 113726 [details] [review] Bug 20256: (QA follow-up) Fix number of tests in Patrons.t Created attachment 113727 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Created attachment 113728 [details] [review] Bug 20256: (QA follow-up) Implement for item search (In reply to Katrin Fischer from comment #174) > (In reply to Katrin Fischer from comment #172) > > > > 4) Item edit checks > > > > > > > > I think we might also need a check on the permission from the item search > > > > edit item link. > > > > > > I've been working on this patch, but it occurs to me that the edit item link > > > shows even if a record has no items. It's a useful way to go straight to the > > > items editor to create a new item. As such, I don't think we should hide the > > > link because it can still be used in that way even if the librarian can't > > > edit any existing items. > > > > Hi Kyle, I meant the "item search" - in this case the form for editing this > > specific item opens. So I think it makes sense to hide it there and I think > > there is already some logic for that now. > I identified 3 places now that require action: > > - Edit item link from course reserves > - Edit item link form item search (not catalog search) > - Edit item link from items tab (moredetail.pl) The edit item link on moredetail.pl was already implemented, is it not working for you? Created attachment 116042 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116043 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116044 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116045 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116046 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116047 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116048 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116049 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116050 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116051 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116052 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116053 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116054 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116055 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116056 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116057 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116058 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116059 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 116060 [details] [review] Bug 20256: (QA follow-up) Fix variable masking warning in additem.pl Created attachment 116061 [details] [review] Bug 20256: (QA follow-up) Fix number of tests in Patrons.t Created attachment 116062 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Created attachment 116063 [details] [review] Bug 20256: (QA follow-up) Implement for item search Created attachment 116064 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Hi Kyle, could you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20256: Update database Applying: Bug 20256: Add new permission to editor Applying: Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Applying: Bug 20256: Add new methods for checking item editing permissions error: sha1 information is lacking or useless (Koha/Patron.pm). error: could not build fake ancestor Patch failed at 0001 Bug 20256: Add new methods for checking item editing permissions The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-20256-Add-new-methods-for-checking-item-editin-QeAIqS.patch *** Bug 28174 has been marked as a duplicate of this bug. *** Created attachment 120055 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120056 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120057 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120058 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120059 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120060 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120061 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120062 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120063 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120064 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120065 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120066 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120067 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120068 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120069 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120070 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120071 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120072 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Created attachment 120073 [details] [review] Bug 20256: (QA follow-up) Fix variable masking warning in additem.pl Created attachment 120074 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Created attachment 120075 [details] [review] Bug 20256: (QA follow-up) Implement for item search Created attachment 120076 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] This looks good to me, and in my testing, all seems to be working well. As Katrin appears to already be most of the way through testing however.. I'll leave it to her to do the final signoff and PQA. Created attachment 121771 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121772 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121773 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121774 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121775 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121776 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121777 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121778 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121779 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121780 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121781 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121782 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121783 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121785 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121786 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121787 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121788 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121789 [details] [review] Bug 20256: (QA follow-up) Fix variable masking warning in additem.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121790 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121791 [details] [review] Bug 20256: (QA follow-up) Implement for item search Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 121792 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Minor rebase and added my SO line as I've tested this.. still think it would be good to let Katrin do the final QA Can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20256: Update database Applying: Bug 20256: Add new permission to editor Applying: Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Applying: Bug 20256: Add new methods for checking item editing permissions Applying: Bug 20256: Update groups editor Applying: Bug 20256: Use new methods Applying: Bug 20256: Add unit tests Applying: Bug 20256: (QA follow-up) Remove double negative from code Applying: Bug 20256: (QA follow-up) Change meaning should change code Applying: Bug 20256: (QA follow-up) test borrowernumber in userenv too Applying: Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Applying: Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Applying: Bug 20256: (QA follow-up) Handle non-existing userenv Applying: Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Applying: Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit error: sha1 information is lacking or useless (tools/batchMod.pl). error: could not build fake ancestor Patch failed at 0001 Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-20256-QA-follow-up-Skip-batch-editing-items-th-RbN33J.patch Created attachment 124286 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124287 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124288 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124289 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124290 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124291 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124292 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124293 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124294 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124295 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124296 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124297 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124298 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124299 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124300 [details] [review] Bug 20256: (QA follow-up) Skip batch editing items the user is not allowed to edit Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124301 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124302 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124303 [details] [review] Bug 20256: (QA follow-up) Fix variable masking warning in additem.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124304 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124305 [details] [review] Bug 20256: (QA follow-up) Implement for item search Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124306 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 124307 [details] [review] Bug 20256: (follow-up) Fix spelling Created attachment 124321 [details] [review] Bug 20256: (follow-up) Don't filter if no libraries are restricted Patches no longer apply, could you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20256: Update database Applying: Bug 20256: Add new permission to editor Applying: Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Applying: Bug 20256: Add new methods for checking item editing permissions Applying: Bug 20256: Update groups editor Applying: Bug 20256: Use new methods Using index info to reconstruct a base tree... M Koha/Item.pm M catalogue/detail.pl M cataloguing/additem.pl M tools/batchMod.pl Falling back to patching base and 3-way merge... Auto-merging tools/batchMod.pl CONFLICT (content): Merge conflict in tools/batchMod.pl Auto-merging cataloguing/additem.pl CONFLICT (content): Merge conflict in cataloguing/additem.pl Auto-merging catalogue/detail.pl Auto-merging Koha/Item.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 20256: Use new methods The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-20256-Use-new-methods-G2R6lg.patch Created attachment 128734 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128735 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128736 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128737 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128738 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128739 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128740 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128741 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128742 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128743 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128744 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128745 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128746 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128747 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128748 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128749 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128750 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128751 [details] [review] Bug 20256: (QA follow-up) Implement for item search Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128752 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 128753 [details] [review] Bug 20256: (follow-up) Fix spelling Created attachment 131676 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131677 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131678 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131679 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131680 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131681 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131682 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131683 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131684 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131685 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131686 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131687 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131688 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131689 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131690 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131691 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131692 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131693 [details] [review] Bug 20256: (QA follow-up) Implement for item search Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131694 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 131695 [details] [review] Bug 20256: (follow-up) Fix spelling Created attachment 131696 [details] [review] Bug 20256: (QA follow-up) Fix number of tests in Patrons.t Created attachment 131697 [details] [review] Bug 20256: (QA follow-up) Fix typo in comment on kohastructure.sql Patches still apply with a tiny rebase :) I've added the rebased patch set with 2 tiny follow-ups, but have some issues I am struggling with: 1) Tests don't pass and there is a warning that might relate to some rebase issue: kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Koha/Patrons.t t/db_dependent/Koha/Patrons.t .. Subroutine libraries_where_can_see_patrons redefined at /kohadevbox/koha/Koha/Patron.pm line 1610. 2) Permission descriptions We now have: * Edit any item. If not set the logged in user can only edit items whose home library matches the logged in library. (edit_any_item) * Edit items (edit_items) I think we need to actually rephrase Edit items if the note on edit_any_item rings true. Also, we need to factor in the library group feature somehow... Edit all items with user's home library and from libaries within the edit item library group (edit_items) It makes me realize I am not super sure yet how the library groups and permissions work together, can you please check and maybe explain a little more? 3) Library groups <li>Limit item editing for librarians outside of this group.</li> Maybe this would be clearer as: <li>Limit item editing to librarians inside of this group</li> But I might be misunderstanding something? Created attachment 132483 [details] [review] Bug 20256: (QA follow-up) Remove duplicated sub Created attachment 132484 [details] [review] Bug 20256: (QA follow-up) Update permission descriptions Created attachment 132486 [details] [review] Bug 20256: (QA follow-up) Update group feature description (In reply to Katrin Fischer from comment #336) > Patches still apply with a tiny rebase :) > > I've added the rebased patch set with 2 tiny follow-ups, but have some > issues I am struggling with: > > 1) Tests don't pass and there is a warning that might relate to some rebase > issue: > > kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Koha/Patrons.t > t/db_dependent/Koha/Patrons.t .. Subroutine libraries_where_can_see_patrons > redefined at /kohadevbox/koha/Koha/Patron.pm line 1610. Should be fixed with this latest followup! > > 2) Permission descriptions > > We now have: > > * Edit any item. If not set the logged in user can only edit items whose > home library matches the logged in library. (edit_any_item) > * Edit items (edit_items) > > I think we need to actually rephrase Edit items if the note on edit_any_item > rings true. Also, we need to factor in the library group feature somehow... > > Edit all items with user's home library and from libaries within the edit > item library group (edit_items) > > It makes me realize I am not super sure yet how the library groups and > permissions work together, can you please check and maybe explain a little > more? I've added a followup to update those descriptions. I think the new ones are better, but if anyone has more improvements they would be welcome! > > 3) Library groups > > <li>Limit item editing for librarians outside of this group.</li> > > Maybe this would be clearer as: > > <li>Limit item editing to librarians inside of this group</li> > > But I might be misunderstanding something? That sounds good to me! Created attachment 132917 [details] [review] Bug 20256: (QA follow-up) Fix typo in permission descriptions Started to test functionality a bit more and I feel like there is a bigger issue with how this will change how item editing is set up. We now have: * Edit items (not including items restricted by library group) (edit_items) * Edit any item including items that would otherwise be restricted (edit_any_item) Test case 1: * edit_items * Independentbranches is off * No library groups are set up Items from other libraries cannot be edited. This means that now both permissions are required in order to have the current behaviour when Independentbranches is turned off. As the permissions for existing staff users are not updated with this patch set, it will cause a behaviour change and require a lot of config changes (depending on staff numbers) to fix. So what can we do? 1) Update staff permissions for existing staff users. This will still mean that 2 checkboxes will have to be set to allow for full item editing. 2) Change the checks to see if there are actually any library groups for 'item edit' set up and only apply the limit if there are. What do you think? I lean a bit more towards 2 as this will make the default config a little easier (= no restrictions set up, no restrictions applied) Ok, I misread the database update, there actually is an update!
+ $dbh->do(q{
+ INSERT INTO user_permissions ( borrowernumber, module_bit, code )
+ SELECT borrowernumber, '9', 'edit_any_item'
+ FROM user_permissions
+ WHERE module_bit = '9'
+ AND code = 'edit_items'
+ });
Still wondering if we could get around the 2 permissions for simple item edit somehow... but maybe not.
Because: if we implemented my suggestion 2) libraries wanting to limit on home branch for editing would have to set up a group for each branch, which is also not nice.
I'll test more!
=Testing restrictions= Test case 1: * edit_items = yes * edit_any_items = no * Independentbranches = NO * No library groups defined Only items from home branch can be edited. Test case 2: * edit_items = yes * edit_any_items = yes * Independentbranches = NO * No library groups defined All items can be edited. Test case 3: * edit_items = yes * edit_any_items = no * Independentbranches = YES * No library groups defined Only items from home branch can be edited. Same as with Indybranches off. Test case 4: * edit_items = yes * edit_any_items = yes * Independentbranches = YES * No library groups defined All items can be edited. Same as with Indybranches off. Test case 5: * edit_items = yes * edit_any_items = no * Independentbranches = YES or NO * Library group includes staff users home branch (MPL) and other library branch (CPL) Only items from home branch (CPL) and group library (MPL) can be edited. Test case 6: * edit_items = yes * edit_any_items = yes * Independentbranches = YES or NO * No library groups defined All items can be edited. Summary: ==> The new edit_any_items overwrites the Indybranches pref. In consequence it appears it no longer has any effect. Correct? Should we update the IndependantBranches description to reflect that? Currently reads: Prevent staff (but not superlibrarians) from modifying objects (holds, items, patrons, etc.) belonging to other libraries: [Yes|No] ==> We update all staff users to have edit_any_items, but we should possibly only update if IndependentBranches is set to off, to keep current behaviour! (blocker) =Edit buttons= Tested with configuration from Test case 5. 1) OK - detail page - holdings table 2) OK - detail page - items tab 3) NOT OK - item edit page The Edit link in the Actions menu always shows, but redirects to detail page, if no permission. It shoudl only show if editing is possible. (blocker) 4) OK - item search It could be improved later, so that if there is only "edit record" we show this option directly (no blocker). 5) NOT OK - Course reserves a) Use a barcode to add a reserve to a course from another branch, don't change home branch The edit link is removed, but something with the table structure is broken because of it and causes breakage of the datatable: Uncaught TypeError: Cannot set property '_DT_CellIndex' of undefined. (blocker) b) Use a barcode to add a reserve to a course from another branch, update home branch to an editable one The links are shown and the table keeps working. This one is tricky, I think for sure we need to fix the table to not be broken (minimum requirement). But this could be easily a thing where a patron 'traps' themselves with a reserve they can only 'remove' using the 'remove all' feature. Conclusion: An idea: As there sure is a use case for getting items from other branches for your course reserve, maybe we can do this: - Always show button to remove the item from the reserve (that's not editing right?) Also might fix the table issue. - Maybe: Allow editing of the item for this use case after the item has been added to the course reserve. If we allow on adding it... it would be consequential and it's only temporary changes. 6) OK - detail page - batch editing 7) NOT OK - tools - items batch edit - Added barcodes of one editable and a non-editable item. Exlosion (blocker) Can't locate object method "params" via package "Koha::UI::Table::Builder::Items" at /kohadevbox/koha/Koha/UI/Table/Builder/Items.pm line 75 Created attachment 134375 [details] [review] Bug 20256: (QA follow-up) hide Edit chooice in Actions on items editor if item cannot be edited Created attachment 134376 [details] [review] Bug 20256: (QA follow-up) Allow removal of course reserves for items that cannot be edited Created attachment 134377 [details] [review] Bug 20256: (QA follow-up) Fix explosion for items batch edit > Summary: > ==> The new edit_any_items overwrites the Indybranches pref. In consequence > it appears it no longer has any effect. Correct? IndependentBranches is built into this feature. If IndependentBranches is enabled, only a superlibrarian can edit all items, and non-superlibrarians can only edit items from his or her home library. > 3) NOT OK - item edit page > The Edit link in the Actions menu always shows, but redirects to detail > page, if no permission. > It shoudl only show if editing is possible. (blocker) Fixed! > 5) NOT OK - Course reserves > a) Use a barcode to add a reserve to a course from another branch, don't > change home branch > > The edit link is removed, but something with the table structure is > broken because of it and causes breakage of the datatable: > Uncaught TypeError: Cannot set property '_DT_CellIndex' of undefined. > (blocker) Fixed! > 7) NOT OK - tools - items batch edit > - Added barcodes of one editable and a non-editable item. Exlosion > (blocker) > Can't locate object method "params" via package > "Koha::UI::Table::Builder::Items" at > /kohadevbox/koha/Koha/UI/Table/Builder/Items.pm line 75 Fixed! (In reply to Kyle M Hall from comment #348) > > Summary: > > ==> The new edit_any_items overwrites the Indybranches pref. In consequence > > it appears it no longer has any effect. Correct? > > IndependentBranches is built into this feature. If IndependentBranches is > enabled, only a superlibrarian can edit all items, and non-superlibrarians > can only edit items from his or her home library. I think that's not the case, I tried all the combinations. It depends on the edit_any_items preference with this patch set. I was testing with a non-superlibrarian user: "Test case 4: * edit_items = yes * edit_any_items = yes * Independentbranches = YES * No library groups defined All items can be edited. Same as with Indybranches off." Can you please test yourself and take a look at my summary? > "Test case 4: > * edit_items = yes > * edit_any_items = yes > * Independentbranches = YES > * No library groups defined > > All items can be edited. Same as with Indybranches off." > > Can you please test yourself and take a look at my summary? As far as I can tell, it's working as expected. Can you take a look at this video and let me know if I'm missing anything? https://www.youtube.com/watch?v=kkwd-qCoM50 Thanks! *** Bug 6423 has been marked as a duplicate of this bug. *** Created attachment 139135 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139136 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139137 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139138 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139139 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139140 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139141 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139142 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139143 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139144 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139145 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139146 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139147 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139148 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139149 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139150 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139151 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139152 [details] [review] Bug 20256: (QA follow-up) Implement for item search Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139153 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 139154 [details] [review] Bug 20256: (follow-up) Fix spelling Created attachment 139155 [details] [review] Bug 20256: (QA follow-up) Fix typo in comment on kohastructure.sql Created attachment 139156 [details] [review] Bug 20256: (QA follow-up) Remove duplicated sub Created attachment 139157 [details] [review] Bug 20256: (QA follow-up) Update permission descriptions Created attachment 139158 [details] [review] Bug 20256: (QA follow-up) Update group feature description Created attachment 139159 [details] [review] Bug 20256: (QA follow-up) Fix typo in permission descriptions Created attachment 139160 [details] [review] Bug 20256: (QA follow-up) hide Edit chooice in Actions on items editor if item cannot be edited Created attachment 139161 [details] [review] Bug 20256: (QA follow-up) Allow removal of course reserves for items that cannot be edited Created attachment 139162 [details] [review] Bug 20256: (QA follow-up) Fix explosion for items batch edit Created attachment 139163 [details] [review] Bug 20256: (QA follow-up) Hide link to delete item if unable to delete the item Hi Kyle, could you please rebase? I was hoping someone else would look into the case we disagreed about, but now ready to try and tackle this again. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 20256: Update database Applying: Bug 20256: Add new permission to editor Applying: Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Applying: Bug 20256: Add new methods for checking item editing permissions Applying: Bug 20256: Update groups editor Applying: Bug 20256: Use new methods Using index info to reconstruct a base tree... M Koha/Item.pm M Koha/UI/Table/Builder/Items.pm M catalogue/detail.pl M catalogue/moredetail.pl M cataloguing/additem.pl M koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc M koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.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/catalogue/detail.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc Auto-merging cataloguing/additem.pl Auto-merging catalogue/moredetail.pl CONFLICT (content): Merge conflict in catalogue/moredetail.pl Auto-merging catalogue/detail.pl CONFLICT (content): Merge conflict in catalogue/detail.pl Auto-merging Koha/UI/Table/Builder/Items.pm CONFLICT (content): Merge conflict in Koha/UI/Table/Builder/Items.pm Auto-merging Koha/Item.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 20256: Use new methods Created attachment 143527 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143529 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143530 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143531 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143532 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143533 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143534 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143535 [details] [review] Bug 20256: (QA follow-up) Remove double negative from code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143536 [details] [review] Bug 20256: (QA follow-up) Change meaning should change code Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143537 [details] [review] Bug 20256: (QA follow-up) test borrowernumber in userenv too Bug 22600 added userenv for scripts, so userenv is defined, but some of values in userenv not Test plan: Run with some params: misc/cronjobs/delete_items.pl --> without this patch you will se error message: Can't call method "can_edit_item" on an undefined value at /home/vagrant/kohaclone/C4/Items.pm line 1748 --> with this patch everything works smoothly Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143538 [details] [review] Bug 20256: (QA follow-up) Fix missed change from item.cannot_be_edited to item.can_be_edited Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143539 [details] [review] Bug 20256: (QA follow-up) Redirect to record details page if user cannot edit this item Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143540 [details] [review] Bug 20256: (QA follow-up) Handle non-existing userenv Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143541 [details] [review] Bug 20256: (QA follow-up) Give existing users with edit_items permission edit_any_item as well Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143542 [details] [review] Bug 20256: (QA follow-up) Filter out branchcodes the user has no permissions for Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143543 [details] [review] Bug 20256: Mark ft_limit_item_editing as boolean in schema file Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143544 [details] [review] Bug 20256: (QA follow-up) Implement for course reserves Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143545 [details] [review] Bug 20256: (QA follow-up) Implement for item search Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143546 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 143547 [details] [review] Bug 20256: (follow-up) Fix spelling Created attachment 143548 [details] [review] Bug 20256: (QA follow-up) Fix typo in comment on kohastructure.sql Created attachment 143549 [details] [review] Bug 20256: (QA follow-up) Remove duplicated sub Created attachment 143550 [details] [review] Bug 20256: (QA follow-up) Update permission descriptions Created attachment 143551 [details] [review] Bug 20256: (QA follow-up) Update group feature description Created attachment 143552 [details] [review] Bug 20256: (QA follow-up) Fix typo in permission descriptions Created attachment 143553 [details] [review] Bug 20256: (QA follow-up) hide Edit chooice in Actions on items editor if item cannot be edited Created attachment 143554 [details] [review] Bug 20256: (QA follow-up) Allow removal of course reserves for items that cannot be edited Created attachment 143555 [details] [review] Bug 20256: (QA follow-up) Fix explosion for items batch edit Created attachment 143556 [details] [review] Bug 20256: (QA follow-up) Hide link to delete item if unable to delete the item Created attachment 145286 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145287 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145288 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145289 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145290 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145291 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145292 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145293 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Follow-ups have been squashed into the main patch set to make everything manageable and readable again. Hi Kyle, I would like to test this feature but it was not possible to create a sandbox: https://sandbox.bywatersolutions.com/provision_log/bug20256 Could you please have a look? Thanks! :-) Created attachment 145482 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145483 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145484 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145485 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145486 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145487 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145488 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145489 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> (In reply to Michaela Sieber from comment #420) > Hi Kyle, > I would like to test this feature but it was not possible to create a > sandbox: https://sandbox.bywatersolutions.com/provision_log/bug20256 > > Could you please have a look? > Thanks! > :-) Please try again! Patches needed a rebase against master. (In reply to Kyle M Hall from comment #429) > > Please try again! Patches needed a rebase against master. Error when trying to see the detail view of a biblio: Could not compile /kohadevbox/koha/catalogue/detail.pl.... http://staff-edititems.sandboxes.ptfs-europe.co.uk/cgi-bin/koha/catalogue/detail.pl?biblionumber=30 (In reply to Michaela Sieber from comment #430) > (In reply to Kyle M Hall from comment #429) > > > > Please try again! Patches needed a rebase against master. > > > Error when trying to see the detail view of a biblio: > > Could not compile /kohadevbox/koha/catalogue/detail.pl.... > > http://staff-edititems.sandboxes.ptfs-europe.co.uk/cgi-bin/koha/catalogue/ > detail.pl?biblionumber=30 Full error is: Could not compile /kohadevbox/koha/catalogue/detail.pl: Global symbol "$item_object" requires explicit package name (did you forget to declare "my $item_object"?) at /kohadevbox/koha/catalogue/detail.pl line 445. BEGIN not safe after errors--compilation aborted at /kohadevbox/koha/catalogue/detail.pl line 693. at /usr/share/perl5/CGI/Compile.pm line 144 Maybe a rebase issue with some changes on master? Created attachment 145809 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145810 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145811 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145812 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145813 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145814 [details] [review] Bug 20256: Use new methods Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145815 [details] [review] Bug 20256: Add unit tests Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145816 [details] [review] Bug 20256: Update DB Schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Hi Kyle, does the last patch set fix the error Michaela noted on comment 430? Michaela also told that in her testing it showed the behavior as you described it - so I need to do my homework and retest. I aim to have another go at this patch set on Friday. Fingers crossed. Created attachment 145887 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145888 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145889 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145890 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145891 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145892 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145893 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145894 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145895 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145896 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> (In reply to Katrin Fischer from comment #440) > Hi Kyle, > > does the last patch set fix the error Michaela noted on comment 430? > > Michaela also told that in her testing it showed the behavior as you > described it - so I need to do my homework and retest. I aim to have another > go at this patch set on Friday. Fingers crossed. Yes it should! I'm getting "413 Request Entity Too Large" when I attach my new patchset, so I've posted the github url in the URL field for this bug. https://github.com/bywatersolutions/bywater-koha-devel/tree/bug_20256 I will deprecate the attached patches. Created attachment 145951 [details] [review] Bug 20256: Update database Test Plan: 1) Apply this patch set 2) Run updatedatabase.pl 3) Create two library groups with some libraries in them, make sure to enable the new feature to limit editing of items 4) Test the functionality, if an item is owned by a library in your group, you should have full editing abilities. If an item is from a different group, you should only be able to make the same changes you would be able to if IndependentBranches were. Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145952 [details] [review] Bug 20256: Add new permission to editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145953 [details] [review] Bug 20256: Refactor subs for rights to view patrons to make them generic for general use Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145954 [details] [review] Bug 20256: Add new methods for checking item editing permissions Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 145955 [details] [review] Bug 20256: Update groups editor Signed-off-by: Bob Bennhoff - CLiC <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I tested the feature and after a rebase fix Kyle did it is working properly. The QA scripts are happy, things are covered by tests and they pass. Pushed to master for 23.05. Nice work everyone, thanks! (In reply to Katrin Fischer from comment #440) > Hi Kyle, > > does the last patch set fix the error Michaela noted on comment 430? > > Michaela also told that in her testing it showed the behavior as you > described it - so I need to do my homework and retest. I aim to have another > go at this patch set on Friday. Fingers crossed. It's Friday, but I am no longer needed here :D Kyle, we talked about a fix for a random failure on t/db_dependent/Koha/Acquisition/Order.t related to this bug. Please check. 1530 my $branchcode =
1531 $ref eq 'Koha::Item' ? $item->homebranch
1532 : $ref eq 'HASH' ? $item->{homebranch}
1533 : $ref eq q{} ? $item
1534 : undef;
This reads bad, and introduce an incorrect pattern, can you have another look at it please?
Why not simply having can_edit_items_from that takes the branchcode?
(In reply to Tomás Cohen Arazi from comment #460) > Kyle, we talked about a fix for a random failure on > t/db_dependent/Koha/Acquisition/Order.t related to this bug. Please check. Failing again, see bug 33054. I'm not sure if this is somehow covered in this huge bug but I noticed that if you are limiting the editing per library group you lose the ability to choose items on record details page. This is enabled in StaffDetailItemSelection. You cannot choose your own items either. It would be ok not to choose items that you cannot edit/delete but if you cannot choose your own items it is a big disadvantage. You cannot choose multiple items and do batch edits or deletes to them from details page. I tested the feature in PTFS Europe's sandbox (v. 23.06). (In reply to Anneli Österman from comment #463) > I'm not sure if this is somehow covered in this huge bug but I noticed that > if you are limiting the editing per library group you lose the ability to > choose items on record details page. This is enabled in > StaffDetailItemSelection. You cannot choose your own items either. It would > be ok not to choose items that you cannot edit/delete but if you cannot > choose your own items it is a big disadvantage. You cannot choose multiple > items and do batch edits or deletes to them from details page. I tested the > feature in PTFS Europe's sandbox (v. 23.06). Can you file a dependent bug report for this issue? (In reply to Kyle M Hall from comment #464) > (In reply to Anneli Österman from comment #463) > > I'm not sure if this is somehow covered in this huge bug but I noticed that > > if you are limiting the editing per library group you lose the ability to > > choose items on record details page. This is enabled in > > StaffDetailItemSelection. You cannot choose your own items either. It would > > be ok not to choose items that you cannot edit/delete but if you cannot > > choose your own items it is a big disadvantage. You cannot choose multiple > > items and do batch edits or deletes to them from details page. I tested the > > feature in PTFS Europe's sandbox (v. 23.06). > > Can you file a dependent bug report for this issue? Sure, I added bug 34814 (hope I did it right). > Sure, I added bug 34814 (hope I did it right).
Looks good. Thanks!
Druart's comment 42 (4) was ignored? I'm able (v23.11.07) to create an item, then switch homebranch (and holdingbranch), and therefore no longer being able to edit it. Does _edit_ imply create and delete? I though so. |