There are some bugs in the list module which are not fixable easily, because of the (very) ugly C4::VirtualShelves::Page package. This module is completely stuck and there is no other way to rewrite it completely to make it move again.
Created attachment 41034 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves
Created attachment 41035 [details] [review] Bug 14544: Get rid of GetShelf
Created attachment 41036 [details] [review] Bug 14544: Get rid of AddShelf
Created attachment 41037 [details] [review] Bug 14544: Get rid of DelShelf
Created attachment 41038 [details] [review] Bug 14544: Get rid of ModShelf
Created attachment 41039 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares
Created attachment 41040 [details] [review] Bug 14544: Get rid of GetAllShelves
Created attachment 41041 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf
Created attachment 41042 [details] [review] [DO NOT APPLY] Bug 14544: WIP - Get rid of GetShelves This does not work.
http://lists.koha-community.org/pipermail/koha-devel/2015-July/041641.html
Based on my initial skimming of the patches, I think this looks great! The only very minor nitpicky suggestion would be the naming of the clases. You could rename Koha::Virtualshelf to Koha::List and Koha::Virtualshelfshares to Koha::List::Sharess or something along those lines so that we can start down the road to having our internal nomenclature match our external nomenclature. What do you think of that Jonathan?
I really like the idea of not naming them virtualshelves - but we have quite some lists now in the interface. Only problem might be that we also have patron lists and possible other types of lists?
(In reply to Kyle M Hall from comment #11) > Based on my initial skimming of the patches, I think this looks great! > > The only very minor nitpicky suggestion would be the naming of the clases. > You could rename Koha::Virtualshelf to Koha::List and > Koha::Virtualshelfshares to Koha::List::Sharess or something along those > lines so that we can start down the road to having our internal nomenclature > match our external nomenclature. > > What do you think of that Jonathan? It seems that a consensus on the name is not reach :) And I really don't want to rewrite the job already done or wait to find a name... I am sure it will be quite easy to rename everything (tables and classes) when this change will be done.
Created attachment 41403 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves
Created attachment 41404 [details] [review] Bug 14544: Get rid of GetShelf
Created attachment 41405 [details] [review] Bug 14544: Get rid of AddShelf
Created attachment 41406 [details] [review] Bug 14544: Get rid of DelShelf
Created attachment 41407 [details] [review] Bug 14544: Get rid of ModShelf
Created attachment 41408 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares
Created attachment 41409 [details] [review] Bug 14544: Get rid of GetAllShelves
Created attachment 41410 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf
Created attachment 41411 [details] [review] Bug 14544: Make the intranet side independent of Page.pm
Created attachment 41412 [details] [review] Bug 14544: Get rid of GetBibliosShelves
Created attachment 41584 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves The 2 modules C4::VirtualShelves and C4::VirtualShelves::Page are really badly coded and buggy. It became complicated to fix and enhance them. This rewrite will make the code more robust and the list feature will be easier to change. Some new modules are added to the Koha namespace (Koha::Virtualshelf, Koha::Virtualshelves, Koha::Virtualshelfshare[s] and Koha::Virtualshelfcontent[s]) to take advantage of Dbix::Class. The patchset fixes some bugs: 1/ Bug 6279 - Can't delete lists from the second page of lists in the OPAC 2/ The owner of a list should not need any permission to remove content from his lists. 3/ No feedback messages when actions are done Test plan: A) Intranet and OPAC 1/ Create more than 20 private and public lists with different users 2/ Edit some and confirm that the changes are taken into account 3/ Share some lists with other users 4/ Add / remove contents Be sure you have some lists with more than 20 items (to test the pagination) 5/ Play with permissions and confirm it works as expected. For private lists: They should not be viewed by anyone else, if they are not shared If they are shared (OPAC only): a. "Allow anyone else to add entries" Should allow the user with whom the list is shared to add entries. b. "Allow anyone to remove his own contributed entries." Should allow the user with whom the list is shared to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow the user with whom the list is shared to remove any entries. For public lists: They should be viewed by anyone else. a. "Allow anyone else to add entries" Should allow any user to add entries. b. "Allow anyone to remove his own contributed entries." Should allow any user to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow any user to remove any entries. 6/ Download and sent lists. 7/ Confirm that the "Add to" feature works as expected. 8/ Confirm that the "Add an item to" works as expected. 9/ Delete some lists. 10/ Print some lists and subscribe to the RSS feed (OPAC only) (with more than 20 items). 11/ Use the remove "Remove share" links.
Created attachment 41585 [details] [review] Bug 14544: Get rid of GetShelf
Created attachment 41586 [details] [review] Bug 14544: Get rid of AddShelf
Created attachment 41587 [details] [review] Bug 14544: Get rid of DelShelf
Created attachment 41588 [details] [review] Bug 14544: Get rid of ModShelf
Created attachment 41589 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares
Created attachment 41590 [details] [review] Bug 14544: Get rid of GetAllShelves
Created attachment 41591 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf
Created attachment 41592 [details] [review] Bug 14544: Make the intranet side independent of Page.pm
Created attachment 41593 [details] [review] Bug 14544: Get rid of GetBibliosShelves
Created attachment 41594 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later.
Created attachment 41595 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm
Created attachment 41596 [details] [review] Bug 14544: Get rid of ShelfPossibleAction
Created attachment 41597 [details] [review] Bug 14544: Get rid of GetShelfContent
Created attachment 41598 [details] [review] Bug 14544: Get rid of GetSomeShelfNames
Created attachment 41599 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members
Created attachment 41600 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page
Created attachment 41601 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents.
Patch "Make the intranet side independent of Page.pm" needs rebase since Bug 14517 is pushed. Nice job btw
Created attachment 41716 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves The 2 modules C4::VirtualShelves and C4::VirtualShelves::Page are really badly coded and buggy. It became complicated to fix and enhance them. This rewrite will make the code more robust and the list feature will be easier to change. Some new modules are added to the Koha namespace (Koha::Virtualshelf, Koha::Virtualshelves, Koha::Virtualshelfshare[s] and Koha::Virtualshelfcontent[s]) to take advantage of Dbix::Class. The patchset fixes some bugs: 1/ Bug 6279 - Can't delete lists from the second page of lists in the OPAC 2/ The owner of a list should not need any permission to remove content from his lists. 3/ No feedback messages when actions are done Test plan: A) Intranet and OPAC 1/ Create more than 20 private and public lists with different users 2/ Edit some and confirm that the changes are taken into account 3/ Share some lists with other users 4/ Add / remove contents Be sure you have some lists with more than 20 items (to test the pagination) 5/ Play with permissions and confirm it works as expected. For private lists: They should not be viewed by anyone else, if they are not shared If they are shared (OPAC only): a. "Allow anyone else to add entries" Should allow the user with whom the list is shared to add entries. b. "Allow anyone to remove his own contributed entries." Should allow the user with whom the list is shared to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow the user with whom the list is shared to remove any entries. For public lists: They should be viewed by anyone else. a. "Allow anyone else to add entries" Should allow any user to add entries. b. "Allow anyone to remove his own contributed entries." Should allow any user to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow any user to remove any entries. 6/ Download and sent lists. 7/ Confirm that the "Add to" feature works as expected. 8/ Confirm that the "Add an item to" works as expected. 9/ Delete some lists. 10/ Print some lists and subscribe to the RSS feed (OPAC only) (with more than 20 items). 11/ Use the remove "Remove share" links.
Created attachment 41717 [details] [review] Bug 14544: Get rid of GetShelf
Created attachment 41718 [details] [review] Bug 14544: Get rid of AddShelf
Created attachment 41719 [details] [review] Bug 14544: Get rid of DelShelf
Created attachment 41720 [details] [review] Bug 14544: Get rid of ModShelf
Created attachment 41721 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares
Created attachment 41722 [details] [review] Bug 14544: Get rid of GetAllShelves
Created attachment 41723 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf
Created attachment 41724 [details] [review] Bug 14544: Make the intranet side independent of Page.pm
Created attachment 41725 [details] [review] Bug 14544: Get rid of GetBibliosShelves
Created attachment 41726 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later.
Created attachment 41727 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm
Created attachment 41728 [details] [review] Bug 14544: Get rid of ShelfPossibleAction
Created attachment 41729 [details] [review] Bug 14544: Get rid of GetShelfContent
Created attachment 41730 [details] [review] Bug 14544: Get rid of GetSomeShelfNames
Created attachment 41731 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members
Created attachment 41732 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page
Created attachment 41733 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents.
Last patchset fixes conflicts with bug 14517 and bug 14529.
There is a little error in opac/opac-addbybiblionumber.pl : Line 120 : $shelf-> This brakes the addition of records in a list. Other point : I see in opac/opac-addbybiblionumber.pl : } else { # TODO } Why not using $errcode ? like in virtualshelves/addbybiblionumber.pl ?
Created attachment 41766 [details] [review] Bug 14544: (follow-up) Get rid of ShelfPossibleAction
(In reply to Fridolin SOMERS from comment #62) > There is a little error in opac/opac-addbybiblionumber.pl : > Line 120 : $shelf-> > This brakes the addition of records in a list. Oops, fixed! > Other point : > I see in opac/opac-addbybiblionumber.pl : > } else { > # TODO > } > Why not using $errcode ? like in virtualshelves/addbybiblionumber.pl ? Because it didn't before the changes, and I would like to submit another patchset to fix the 2 addbybiblionumber scripts. Not in the scope of this bug report.
(In reply to Jonathan Druart from comment #64) > (In reply to Fridolin SOMERS from comment #62) > > There is a little error in opac/opac-addbybiblionumber.pl : > > Line 120 : $shelf-> > > This brakes the addition of records in a list. > > Oops, fixed! > It works fine, i continue testing
Small bugs in : koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt 1/ Line 20, the closing span tag is missing : <span>anyone else to add entries. 2/ In [% BLOCK list_permissions %] the permissions vars must be called from "shelf" : [% IF allow_add %] must be [% IF shelf.allow_add %] Idem for allow_delete_own and allow_delete_other 3/ In breadcrumbs, a <li></li> tag has been removed. It was : <li><a href="/cgi-bin/koha/opac-shelves.pl?display=privateshelves">Your lists</a></li> Direct text is not correct inside a <ul></ul>.
Created attachment 41909 [details] [review] Bug 14544: Fix several minor fixes at the OPAC follow-up for Bug 14544: Remove wrong tips about permissions and Bug 14544: Make the OPAC side independent of Page.pm
Created attachment 41910 [details] [review] Bug 14544: Fix several minor bugs at the OPAC follow-up for Bug 14544: Remove wrong tips about permissions and Bug 14544: Make the OPAC side independent of Page.pm
(In reply to Jonathan Druart from comment #68) > Created attachment 41910 [details] [review] [review] > Bug 14544: Fix several minor bugs at the OPAC > In breadcrumbs, › must also be inside a <li>. Like in ISBD view : <li><a href="/cgi-bin/koha/opac-main.pl">Home</a> <span class="divider">›</span></li> <li><a href="#">ISBD view</a></li>
4/ I've noticed that when editing a list, the HTML warnings show there are 2 useless ending div tags. I think the correction is : diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt index 245a8dd..e5c40db 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt @@ -705,13 +705,13 @@ <p>No private lists.</p> [% END %] [% END %] + </div> + </div> [% ELSIF NOT loggedinusernumber %] [% IF Koha.Preference( 'opacuserlogin' ) == 1 %] <div class="alert alert-info"><a href="/cgi-bin/koha/opac-user.pl">Log in</a> to create new lists.</div> [% END %] [% END # IF loggedinusername %] - </div> - </div> </div> <!-- / #usershelves --> </div> <!-- / .span10/12 --> </div> <!-- / .row-fluid -->
Created attachment 41913 [details] [review] Bug 14544: Fix several minor bugs at the OPAC follow-up for Bug 14544: Remove wrong tips about permissions and Bug 14544: Make the OPAC side independent of Page.pm
Created attachment 41914 [details] [review] Bug 14544: Fix several minor bugs at the OPAC follow-up for Bug 14544: Remove wrong tips about permissions and Bug 14544: Make the OPAC side independent of Page.pm
Created attachment 41915 [details] [review] Bug 14544: Do not use shelf as a variable loop Otherwise the shelf variable send from the pl script is overwrite
Created attachment 41917 [details] [review] Bug 14544: Do not use shelf as a variable loop Otherwise the shelf variable send from the pl script is overwrite
Created attachment 41918 [details] [review] Bug 14544: Fix redirect on editing a list If you edit a list from the list view, after saving the form, you are not redirected to the list view (but on the edit form).
Created attachment 41919 [details] [review] Bug 14544: Cosmetic: › should be a class divider
Created attachment 41924 [details] [review] Bug 14544: Fix regression: checkboxes'values are empty They should be filled with the biblionumber
Created attachment 41927 [details] [review] Bug 14544: Pass the itemtype description and notforloan to the template Before it was returned by GetShelfContents and the join on itemtype.
Created attachment 41931 [details] [review] Bug 14544: Allow a user to delete his own contents
Created attachment 41932 [details] [review] Bug 14544: Remove warnings in tests
New day, new bug :D The creation of a new list after selecting some records fails : It is because after creating Koha::Virtualshelf->new, one must call store() so that it impacts database, used by AddBibliosToShelf(). diff --git a/opac/opac-addbybiblionumber.pl b/opac/opac-addbybiblionumber.pl index 10420fe..9181efd 100755 --- a/opac/opac-addbybiblionumber.pl +++ b/opac/opac-addbybiblionumber.pl @@ -106,7 +106,8 @@ sub HandleNewVirtualShelf { $errcode = 1; return; } - AddBibliosToShelf($shelfnumber, @biblionumber); + $shelf->store(); + AddBibliosToShelf($shelf->shelfnumber, @biblionumber); #Reload the page where you came from print $query->header; print "<html><meta http-equiv=\"refresh\" content=\"0\" /><body onload=\"window.opener.location.reload(true);self.close();\"></body></html>"; Idem on staff interface. You may also allow AddBibliosToShelf() to get a shelf object or a shelnumber as first argument.
Typo in "Bug 14544: Fix several minor bugs at the OPAC" : [% IF op != 'list' %] <li><a href="/cgi-bin/koha/cgi-bin/koha/opac-shelves.pl">Lists</a></li> [% ELSE %] /cgi-bin/koha twice
3/ Share some lists with other users Note that it is managed by syspref OpacAllowSharingPrivateLists
Created attachment 41953 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves The 2 modules C4::VirtualShelves and C4::VirtualShelves::Page are really badly coded and buggy. It became complicated to fix and enhance them. This rewrite will make the code more robust and the list feature will be easier to change. Some new modules are added to the Koha namespace (Koha::Virtualshelf, Koha::Virtualshelves, Koha::Virtualshelfshare[s] and Koha::Virtualshelfcontent[s]) to take advantage of Dbix::Class. The patchset fixes some bugs: 1/ Bug 6279 - Can't delete lists from the second page of lists in the OPAC 2/ The owner of a list should not need any permission to remove content from his lists. 3/ No feedback messages when actions are done Test plan: A) Intranet and OPAC 1/ Create more than 20 private and public lists with different users 2/ Edit some and confirm that the changes are taken into account 3/ Share some lists with other users 4/ Add / remove contents Be sure you have some lists with more than 20 items (to test the pagination) 5/ Play with permissions and confirm it works as expected. For private lists: They should not be viewed by anyone else, if they are not shared If they are shared (OPAC only): a. "Allow anyone else to add entries" Should allow the user with whom the list is shared to add entries. b. "Allow anyone to remove his own contributed entries." Should allow the user with whom the list is shared to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow the user with whom the list is shared to remove any entries. For public lists: They should be viewed by anyone else. a. "Allow anyone else to add entries" Should allow any user to add entries. b. "Allow anyone to remove his own contributed entries." Should allow any user to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow any user to remove any entries. 6/ Download and sent lists. 7/ Confirm that the "Add to" feature works as expected. 8/ Confirm that the "Add an item to" works as expected. 9/ Delete some lists. 10/ Print some lists and subscribe to the RSS feed (OPAC only) (with more than 20 items). 11/ Use the remove "Remove share" links.
Created attachment 41954 [details] [review] Bug 14544: Get rid of GetShelf
Created attachment 41955 [details] [review] Bug 14544: Get rid of AddShelf
Created attachment 41956 [details] [review] Bug 14544: Get rid of DelShelf
Created attachment 41957 [details] [review] Bug 14544: Get rid of ModShelf
Created attachment 41958 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares
Created attachment 41959 [details] [review] Bug 14544: Get rid of GetAllShelves
Created attachment 41960 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf Bug 14544: Allow a user to delete his own contents
Created attachment 41961 [details] [review] Bug 14544: Make the intranet side independent of Page.pm
Created attachment 41962 [details] [review] Bug 14544: Get rid of GetBibliosShelves
Created attachment 41963 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later.
Created attachment 41964 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm Bug 14544: Fix redirect on editing a list If you edit a list from the list view, after saving the form, you are not redirected to the list view (but on the edit form). Bug 14544: Cosmetic: › should be a class divider
Created attachment 41965 [details] [review] Bug 14544: Get rid of ShelfPossibleAction Bug 14544: (follow-up) Get rid of ShelfPossibleAction
Created attachment 41966 [details] [review] Bug 14544: Get rid of GetShelfContent
Created attachment 41967 [details] [review] Bug 14544: Get rid of GetSomeShelfNames
Created attachment 41968 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members
Created attachment 41969 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page
Created attachment 41970 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents.
Created attachment 41973 [details] [review] Bug 14544: Make the intranet side independent of Page.pm
Created attachment 41974 [details] [review] Bug 14544: Get rid of GetBibliosShelves
Created attachment 41975 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later.
Created attachment 41976 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm Bug 14544: Fix redirect on editing a list If you edit a list from the list view, after saving the form, you are not redirected to the list view (but on the edit form). Bug 14544: Cosmetic: › should be a class divider
Created attachment 41977 [details] [review] Bug 14544: Get rid of ShelfPossibleAction Bug 14544: (follow-up) Get rid of ShelfPossibleAction
Created attachment 41978 [details] [review] Bug 14544: Get rid of GetShelfContent
Created attachment 41979 [details] [review] Bug 14544: Get rid of GetSomeShelfNames
Created attachment 41980 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members
Created attachment 41981 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page
Created attachment 41982 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents.
Created attachment 41983 [details] [review] Bug 14544: Fix regression on adding a new list The user should not be allowed to create a new list and get a friendly error message if he tries. The "New list" links are removed.
Created attachment 41986 [details] [review] Bug 14544: Fix regression on adding a new list The user should not be allowed to create a new list and get a friendly error message if he tries. The "New list" links are removed.
Created attachment 41988 [details] [review] Bug 14544: DB Changes: Set a default value for sortfield
A behaviour I'm not sure it is the scope of this bug : - Log-in with a user A at intranet - Create a public list - Add some records - Allow anyone else to add entries and anyone to remove his own contributed entries, do not allow any user to remove any entries - Log-in with a user B at OPAC - View the list - You see links "Remove from this list" on every record, even if they where added by another user => It would be better to show this link only in records you can remove - When clicked and alert accepted, you get the message "No biblio has been removed." (records are not removed) => You should get a message of "unauthorized_on_remove_biblios" - Select some records - Click on "Remove from list" - You get no message (records are not removed) => Not sure what to do here because you may be allowed on some selected records
It's the expected behavior, at least the one before this patch. But I completely agree, it could be improved.
Created attachment 42192 [details] [review] Bug 14544 - Handle database error when creating a shelf with DB admin account
(In reply to Alex Arnaud from comment #117) > Created attachment 42192 [details] [review] [review] > Bug 14544 - Handle database error when creating a shelf with DB admin account I think this is not useful, the DB admin account should not be used for anything else than create a first superlibrarian user.
(In reply to Jonathan Druart from comment #118) > I think this is not useful, the DB admin account should not be used for > anything else than create a first superlibrarian user. It was the behavior before this patches. And, isn't it better to have a message that says we can't created a shelf using db admin account instead of a MySql/DBIX error ?
We cannot track all the places this check should be done. It's a waste of time. But I am not against your patch, except that the Exception shouldn't be VirtualShelves specific (should be Koha::Exceptions::UseDbAdminAccount IMO).
Created attachment 42199 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves The 2 modules C4::VirtualShelves and C4::VirtualShelves::Page are really badly coded and buggy. It became complicated to fix and enhance them. This rewrite will make the code more robust and the list feature will be easier to change. Some new modules are added to the Koha namespace (Koha::Virtualshelf, Koha::Virtualshelves, Koha::Virtualshelfshare[s] and Koha::Virtualshelfcontent[s]) to take advantage of Dbix::Class. The patchset fixes some bugs: 1/ Bug 6279 - Can't delete lists from the second page of lists in the OPAC 2/ The owner of a list should not need any permission to remove content from his lists. 3/ No feedback messages when actions are done Test plan: A) Intranet and OPAC 1/ Create more than 20 private and public lists with different users 2/ Edit some and confirm that the changes are taken into account 3/ Share some lists with other users 4/ Add / remove contents Be sure you have some lists with more than 20 items (to test the pagination) 5/ Play with permissions and confirm it works as expected. For private lists: They should not be viewed by anyone else, if they are not shared If they are shared (OPAC only): a. "Allow anyone else to add entries" Should allow the user with whom the list is shared to add entries. b. "Allow anyone to remove his own contributed entries." Should allow the user with whom the list is shared to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow the user with whom the list is shared to remove any entries. For public lists: They should be viewed by anyone else. a. "Allow anyone else to add entries" Should allow any user to add entries. b. "Allow anyone to remove his own contributed entries." Should allow any user to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow any user to remove any entries. 6/ Download and sent lists. 7/ Confirm that the "Add to" feature works as expected. 8/ Confirm that the "Add an item to" works as expected. 9/ Delete some lists. 10/ Print some lists and subscribe to the RSS feed (OPAC only) (with more than 20 items). 11/ Use the remove "Remove share" links. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42200 [details] [review] Bug 14544: Get rid of GetShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42201 [details] [review] Bug 14544: Get rid of AddShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42202 [details] [review] Bug 14544: Get rid of DelShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42203 [details] [review] Bug 14544: Get rid of ModShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42204 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42205 [details] [review] Bug 14544: Get rid of GetAllShelves Signed-off-by: Koha Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42206 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf Bug 14544: Allow a user to delete his own contents Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42207 [details] [review] Bug 14544: Make the intranet side independent of Page.pm Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42208 [details] [review] Bug 14544: Get rid of GetBibliosShelves Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42209 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42210 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm Bug 14544: Fix redirect on editing a list If you edit a list from the list view, after saving the form, you are not redirected to the list view (but on the edit form). Bug 14544: Cosmetic: › should be a class divider Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42211 [details] [review] Bug 14544: Get rid of ShelfPossibleAction Bug 14544: (follow-up) Get rid of ShelfPossibleAction Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42212 [details] [review] Bug 14544: Get rid of GetShelfContent Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42213 [details] [review] Bug 14544: Get rid of GetSomeShelfNames Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42214 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42215 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42216 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42217 [details] [review] Bug 14544: Fix regression on adding a new list The user should not be allowed to create a new list and get a friendly error message if he tries. The "New list" links are removed. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42218 [details] [review] Bug 14544: DB Changes: Set a default value for sortfield Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 42219 [details] [review] Bug 14544 - Handle database error when creating a shelf with DB admin account
Didn't see any *new* bugs/issues so i signed off all the patches (except mine). But Why don't use this BZ to fix existing problems on virtualshelves system (Like the one shown by Fridolin)?
(In reply to Alex Arnaud from comment #142) > Didn't see any *new* bugs/issues so i signed off all the patches (except > mine). > > But Why don't use this BZ to fix existing problems on virtualshelves system > (Like the one shown by Fridolin)? Because it's a huge change enough :)
Created attachment 42638 [details] [review] Bug 14544 - Handle database error when creating a shelf with DB admin account Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> I don't think this is useful, it will be a mess if we have to check this case everywhere. But it works! I let the cut to the QAer or RM.
I am planning to look at this patch set tomorrow, but I still have: Applying: Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page fatal: sha1 information is lacking or useless (catalogue/detail.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
Created attachment 43223 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves The 2 modules C4::VirtualShelves and C4::VirtualShelves::Page are really badly coded and buggy. It became complicated to fix and enhance them. This rewrite will make the code more robust and the list feature will be easier to change. Some new modules are added to the Koha namespace (Koha::Virtualshelf, Koha::Virtualshelves, Koha::Virtualshelfshare[s] and Koha::Virtualshelfcontent[s]) to take advantage of Dbix::Class. The patchset fixes some bugs: 1/ Bug 6279 - Can't delete lists from the second page of lists in the OPAC 2/ The owner of a list should not need any permission to remove content from his lists. 3/ No feedback messages when actions are done Test plan: A) Intranet and OPAC 1/ Create more than 20 private and public lists with different users 2/ Edit some and confirm that the changes are taken into account 3/ Share some lists with other users 4/ Add / remove contents Be sure you have some lists with more than 20 items (to test the pagination) 5/ Play with permissions and confirm it works as expected. For private lists: They should not be viewed by anyone else, if they are not shared If they are shared (OPAC only): a. "Allow anyone else to add entries" Should allow the user with whom the list is shared to add entries. b. "Allow anyone to remove his own contributed entries." Should allow the user with whom the list is shared to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow the user with whom the list is shared to remove any entries. For public lists: They should be viewed by anyone else. a. "Allow anyone else to add entries" Should allow any user to add entries. b. "Allow anyone to remove his own contributed entries." Should allow any user to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow any user to remove any entries. 6/ Download and sent lists. 7/ Confirm that the "Add to" feature works as expected. 8/ Confirm that the "Add an item to" works as expected. 9/ Delete some lists. 10/ Print some lists and subscribe to the RSS feed (OPAC only) (with more than 20 items). 11/ Use the remove "Remove share" links. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43224 [details] [review] Bug 14544: Get rid of GetShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43225 [details] [review] Bug 14544: Get rid of AddShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43226 [details] [review] Bug 14544: Get rid of DelShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43227 [details] [review] Bug 14544: Get rid of ModShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43228 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43229 [details] [review] Bug 14544: Get rid of GetAllShelves Signed-off-by: Koha Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43230 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf Bug 14544: Allow a user to delete his own contents Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43231 [details] [review] Bug 14544: Make the intranet side independent of Page.pm Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43232 [details] [review] Bug 14544: Get rid of GetBibliosShelves Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43233 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43234 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm Bug 14544: Fix redirect on editing a list If you edit a list from the list view, after saving the form, you are not redirected to the list view (but on the edit form). Bug 14544: Cosmetic: › should be a class divider Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43235 [details] [review] Bug 14544: Get rid of ShelfPossibleAction Bug 14544: (follow-up) Get rid of ShelfPossibleAction Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43236 [details] [review] Bug 14544: Get rid of GetShelfContent Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43237 [details] [review] Bug 14544: Get rid of GetSomeShelfNames Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43238 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43239 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43240 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43241 [details] [review] Bug 14544: Fix regression on adding a new list The user should not be allowed to create a new list and get a friendly error message if he tries. The "New list" links are removed. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43242 [details] [review] Bug 14544: DB Changes: Set a default value for sortfield Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 43243 [details] [review] Bug 14544 - Handle database error when creating a shelf with DB admin account Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> I don't think this is useful, it will be a mess if we have to check this case everywhere. But it works! I let the cut to the QAer or RM.
(In reply to Marcel de Rooy from comment #145) > I am planning to look at this patch set tomorrow, but I still have: > > Applying: Bug 14544: Get rid of C4::VirtualShelves and > C4::VirtualShelves::Page > fatal: sha1 information is lacking or useless (catalogue/detail.pl). > Repository lacks necessary blobs to fall back on 3-way merge. > Cannot fall back to three-way merge. Patches rebased against master.
(In reply to Jonathan Druart from comment #167) > (In reply to Marcel de Rooy from comment #145) > > I am planning to look at this patch set tomorrow, but I still have: > > > > Applying: Bug 14544: Get rid of C4::VirtualShelves and > > C4::VirtualShelves::Page > > fatal: sha1 information is lacking or useless (catalogue/detail.pl). > > Repository lacks necessary blobs to fall back on 3-way merge. > > Cannot fall back to three-way merge. > > Patches rebased against master. Thanks. Applies now.
QA: Looking at this now.
Created attachment 43296 [details] [review] Bug 14544: [QA Follow-up] Fix warning about transaction When running Virtualshelves.t, you have this warning: DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1339. By moving the DBI lines after creating the TestBuilder object, the warning will be gone. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Great job !!! Lot of work involved. Still have some points for consideration: OPAC -- View contents of a list -- Send shelf - Crash Can't call method "can_be_viewed" on an undefined value at /home/koha/testclone/opac/opac-sendshelf.pl line 55. *BLOCKER* OPAC - View contents of a list - Sort by callnumber - Bang DBIx::Class::ResultSet::next(): Unknown column 'itemcallnumber' in 'order clause' at /home/koha/testclone/opac/opac-shelves.pl line 236 *BLOCKER* OPAC - Lists- Open a shared list where you do not have permission to remove items of someone else. Try to remove such an item. No success, but no warning too! IIRC There was a warning in the past about some items not deleted.. Now try to remove a item you added yourself to that list while having permission to remove your own entries. This does not work ! No items are deleted, no message. I am inclined to see this last point as a *BLOCKER* too. OPAC - View list - Delete your own item while the perms are ADD or DDD. Should not be possible. But the owner is able to now. This is change of behavior and not consistent with the wording of those permissions "Do not allow anyone to remove his own contributed entries" (including your own!). I would view it as a sort of Readonly list where you cannot accidentally delete your own items. (Note however that adding is still possible.) I am inclined to see this last point as a *BLOCKER* too (but I may not be completely neutral here :) + # FIXME + # This does not make sense, but it's has been backported from DelFromShelf. + # Why shouldn't we allow to remove his own contribution if allow_delete_other is on? Code Exceptions.pm module comes in now with a dependency Exception::Class. The description => "poeut" still needs some attention apparently :) Not sure what it means btw.. Although this looks good to me on itself, this dev should probably better have gone on its own report. Note there is another report too (13995). It kind of creeps in now. OPAC - Share a list - Enter email address - Return to your lists: This list does not exist ?? op=view && category=1. Adjust the URL? OPAC - I shared a list with AAD perms. The other user could add a few entries. After that removed the share. Now come back as owner of the list and try to remove one item of yourself and one of the other user. You will get the message: The item has been removed from the list. This is not completely true: You could only remove one item; the other is not permitted. OPAC - Create a new list - Starts with permissions D D D - Would it not be better to default to D A D ? Note that if I create a list with save to new list, it does default to DAD. So this is a minor inconsistency.. OPAC - View a list: For a list with ADD or DDD perms, the button Remove could be hidden or disabled? Might be the case now too btw. OPAC Detail - List(s) this item appears in: [nothing filled while expected] I click from view a (private) list on one entry. So go to opac detail. Should I expect that list to be mentioned now? It is not. Staff - View public lists [Related to another report] With the delete public lists permission I can delete the public list of another user. OK The staff user probably has a good reason to do so. Just noting that it would perhaps be more friendly to switch it to Private instead of a rigourous Delete? Code in opac-addbybiblionumber.pl: HandleSelectedShelf: Empty TODO in an else branch. Noting for the record. Code t/db_dependent/Utils/Datatables_Virtualshelves.t and C4/Utils/DataTables/VirtualShelves.pm This is only justified/used by svc/virtualshelves/search Isn't this the time to move this svc script to Koha::Virtualshelves too ? Or at least on another report ? Database design: Do we still need (or use) virtualshelfcontents.flags ? Could a dbrev remove it now? An observation (which could be considered as outside the scope): Add an item to the list with a barcode (in staff) is somewhat strange. You cannot add items, only biblios. This is current situation already. Just for completeness :) Moving the status to FQA now to reflect the need for some adjustments/clarifications.
(In reply to Marcel de Rooy from comment #171) > QA Comment: > Great job !!! Lot of work involved. Thanks a lot for Qaing this! Very useful feedback :) > Still have some points for consideration: > > OPAC -- View contents of a list -- Send shelf - Crash > Can't call method "can_be_viewed" on an undefined value at > /home/koha/testclone/opac/opac-sendshelf.pl line 55. > *BLOCKER* Fixed! The shelfnumber param was not passed. I have also add a test in the pl file to avoid this crash when viewing an non-existing shelf (now display 'not authorized', which is the current behavior). > OPAC - View contents of a list - Sort by callnumber - Bang > DBIx::Class::ResultSet::next(): Unknown column 'itemcallnumber' in 'order > clause' at /home/koha/testclone/opac/opac-shelves.pl line 236 > *BLOCKER* Fixed! But... This is a bad one. In opac-shelves.pl and shelves.pl, the join was only done on biblio. But itemcallnumber is in the items table. I had to add the biblio<=>biblioitems<=>items relationship to the Schema and a PK on virtualshelfcontents table (DBIx::Class::Storage::DBIHacks::_adjust_select_args_for_complex_prefetch(): Unable to perform complex limited prefetch off Virtualshelfcontent without declared primary key). Fixed in a separate patch. Note that bug 14819 is stuck in the NSO status but also adds the relationships. > OPAC - Lists- Open a shared list where you do not have permission to remove > items of someone else. Try to remove such an item. No success, but no > warning too! IIRC There was a warning in the past about some items not > deleted.. > Now try to remove a item you added yourself to that list while having > permission to remove your own entries. This does not work ! No items are > deleted, no message. > I am inclined to see this last point as a *BLOCKER* too. Fixed! The "op" was "view" instead of "remove_biblios" on the submit button. Note that the individual links work (and intranet is ok). > OPAC - View list - Delete your own item while the perms are ADD or DDD. > Should not be possible. But the owner is able to now. This is change of > behavior and not consistent with the wording of those permissions "Do not > allow anyone to remove his own contributed entries" (including your own!). I > would view it as a sort of Readonly list > where you cannot accidentally delete your own items. (Note however that > adding is still possible.) > I am inclined to see this last point as a *BLOCKER* too (but I may not be > completely neutral here :) > + # FIXME > + # This does not make sense, but it's has been > backported from DelFromShelf. > + # Why shouldn't we allow to remove his own contribution > if allow_delete_other is on? Well, not fixed. I have asked on #koha and Nicole agreed (http://irc.koha-community.org/koha/2015-08-18#i_1716925). So I am not sure what to do here. > Code Exceptions.pm module comes in now with a dependency Exception::Class. > The description => "poeut" still needs some attention apparently :) Not sure > what it means btw.. > Although this looks good to me on itself, this dev should probably better > have gone on its own report. Note there is another report too (13995). > It kind of creeps in now. poeut is a typo for pouet which does not mean anything :) The exceptions implementation can be modified easily later (the question was: should we put all exceptions in 1 file, all exceptions of a given module in 1 file or 1 exception per file). > OPAC - Share a list - Enter email address - Return to your lists: This list > does not exist ?? op=view && category=1. Adjust the URL? Fixed! Yes, the op is 'list', not 'view". > OPAC - I shared a list with AAD perms. The other user could add a few > entries. After that removed the share. > Now come back as owner of the list and try to remove one item of yourself > and one of the other user. > You will get the message: The item has been removed from the list. > This is not completely true: You could only remove one item; the other is > not permitted. This is a specific case and I would prefer to manage it in a separate bug report. I am not sure this work on master :) > OPAC - Create a new list - Starts with permissions D D D - Would it not be > better to default to D A D ? > Note that if I create a list with save to new list, it does default to DAD. > So this is a minor inconsistency.. Yes, and it's a regression. Fixed on both interface (I have c/p OPAC code for the intranet). > OPAC - View a list: For a list with ADD or DDD perms, the button Remove > could be hidden or disabled? Might be the case now too btw. Not easily, and there is no regression here: it's how it works currently. The problem is that the queries done in remove_biblios should be executed for each biblio in the list. Maybe not a big deal, to investigate... > OPAC Detail - List(s) this item appears in: [nothing filled while expected] > I click from view a (private) list on one entry. So go to opac detail. > Should I expect that list to be mentioned now? It is not. Fixed! In the template shelves->count should be check, not only the shelves variable (which is always defined). > Staff - View public lists [Related to another report] > With the delete public lists permission I can delete the public list of > another user. OK The staff user probably has a good reason to do so. > Just noting that it would perhaps be more friendly to switch it to Private > instead of a rigourous Delete? Todo later yes. > Code in opac-addbybiblionumber.pl: HandleSelectedShelf: Empty TODO in an > else branch. Noting for the record. Yes, I preferred to add the TODO marked than nothing, but no regression here. > Code t/db_dependent/Utils/Datatables_Virtualshelves.t and > C4/Utils/DataTables/VirtualShelves.pm > This is only justified/used by svc/virtualshelves/search > Isn't this the time to move this svc script to Koha::Virtualshelves too ? Or > at least on another report ? Not ready yet. The rewrite should use the REST api. > Database design: Do we still need (or use) virtualshelfcontents.flags ? > Could a dbrev remove it now? To investigate, but yes, I am not sure to have found any usefulness to this column. > An observation (which could be considered as outside the scope): Add an item > to the list with a barcode (in staff) is somewhat strange. You cannot add > items, only biblios. This is current situation already. Just for > completeness :) Changing the wording could maybe fix the ambiguity.
Created attachment 43334 [details] [review] Bug 14544: Add missing biblio <=> biblioitems <=> items relationships to the schema
Created attachment 43335 [details] [review] Bug 14544: QA fixes - some minor bug fixes See comment 171 and 172 for more details. This patch mainly fixes typo or silly error in templates. It also uses the relationships added by previous patch to join the biblioitems and items tables (changes in opac-shelves.pl and shelves.pl).
From IRC http://irc.koha-community.org/koha/2015-08-18 === 11:24 Joubu: nengard: Don't you think the following does not make sense? : "The owner of a list is always allowed to add entries, but needs permission to remove." 11:25 nengard that makes no sense at all === Well, i do not agree that it make no sense at all.. Although this is not the most important issue on lists, but just a detail, such a short conversation on irc is not the decisive factor for me to justify changing current behavior. Also note that the question is formulated rather suggestive.
(In reply to Marcel de Rooy from comment #175) > From IRC http://irc.koha-community.org/koha/2015-08-18 > === > 11:24 Joubu: nengard: Don't you think the following does not make sense? : > "The owner of a list is always allowed to add entries, but needs permission > to remove." > 11:25 nengard that makes no sense at all > === > > Well, i do not agree that it make no sense at all.. > Although this is not the most important issue on lists, but just a detail, > such a short conversation on irc is not the decisive factor for me to > justify changing current behavior. Also note that the question is formulated > rather suggestive. I don't have a strong opinion on it. I have to admit I did not see the "readonly list" feature, and it could make sense.
Created attachment 43403 [details] [review] Bug 14544: allow_delete_own == 0 could represent a read-only list This patch reintroduces the previous behavior.
Marcel, The last patch reintroduces the read-only list feature/bug :) If we decide later to remove it, we would be able to do it reverting this patch!
I plan to look at this again Friday.
Created attachment 43456 [details] [review] Bug 14544: Add Koha::Virtualshelf and Koha::Virtualshelves The 2 modules C4::VirtualShelves and C4::VirtualShelves::Page are really badly coded and buggy. It became complicated to fix and enhance them. This rewrite will make the code more robust and the list feature will be easier to change. Some new modules are added to the Koha namespace (Koha::Virtualshelf, Koha::Virtualshelves, Koha::Virtualshelfshare[s] and Koha::Virtualshelfcontent[s]) to take advantage of Dbix::Class. The patchset fixes some bugs: 1/ Bug 6279 - Can't delete lists from the second page of lists in the OPAC 2/ The owner of a list should not need any permission to remove content from his lists. 3/ No feedback messages when actions are done Test plan: A) Intranet and OPAC 1/ Create more than 20 private and public lists with different users 2/ Edit some and confirm that the changes are taken into account 3/ Share some lists with other users 4/ Add / remove contents Be sure you have some lists with more than 20 items (to test the pagination) 5/ Play with permissions and confirm it works as expected. For private lists: They should not be viewed by anyone else, if they are not shared If they are shared (OPAC only): a. "Allow anyone else to add entries" Should allow the user with whom the list is shared to add entries. b. "Allow anyone to remove his own contributed entries." Should allow the user with whom the list is shared to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow the user with whom the list is shared to remove any entries. For public lists: They should be viewed by anyone else. a. "Allow anyone else to add entries" Should allow any user to add entries. b. "Allow anyone to remove his own contributed entries." Should allow any user to remove his own entries. c. "Allow anyone to remove other contributed entries." Should allow any user to remove any entries. 6/ Download and sent lists. 7/ Confirm that the "Add to" feature works as expected. 8/ Confirm that the "Add an item to" works as expected. 9/ Delete some lists. 10/ Print some lists and subscribe to the RSS feed (OPAC only) (with more than 20 items). 11/ Use the remove "Remove share" links. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43457 [details] [review] Bug 14544: Get rid of GetShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43458 [details] [review] Bug 14544: Get rid of AddShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43459 [details] [review] Bug 14544: Get rid of DelShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43460 [details] [review] Bug 14544: Get rid of ModShelf Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43461 [details] [review] Bug 14544: Move share routines to Koha::Virtualshelfshare and Koha::Virtualshelfshares Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43462 [details] [review] Bug 14544: Get rid of GetAllShelves Signed-off-by: Koha Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43463 [details] [review] Bug 14544: Koha::Virtualshelfcontent[s] Get rid of AddToShelf and DelFromShelf Bug 14544: Allow a user to delete his own contents Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43464 [details] [review] Bug 14544: Make the intranet side independent of Page.pm Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43465 [details] [review] Bug 14544: Get rid of GetBibliosShelves Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43466 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43467 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm Bug 14544: Fix redirect on editing a list If you edit a list from the list view, after saving the form, you are not redirected to the list view (but on the edit form). Bug 14544: Cosmetic: › should be a class divider Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43469 [details] [review] Bug 14544: Get rid of ShelfPossibleAction Bug 14544: (follow-up) Get rid of ShelfPossibleAction Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43470 [details] [review] Bug 14544: Get rid of GetShelfContent Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43471 [details] [review] Bug 14544: Get rid of GetSomeShelfNames Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43472 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43473 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43474 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43475 [details] [review] Bug 14544: Fix regression on adding a new list The user should not be allowed to create a new list and get a friendly error message if he tries. The "New list" links are removed. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43476 [details] [review] Bug 14544: DB Changes: Set a default value for sortfield Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43477 [details] [review] Bug 14544 - Handle database error when creating a shelf with DB admin account Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> I don't think this is useful, it will be a mess if we have to check this case everywhere. But it works! I let the cut to the QAer or RM. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43478 [details] [review] Bug 14544: [QA Follow-up] Fix warning about transaction When running Virtualshelves.t, you have this warning: DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1339. By moving the DBI lines after creating the TestBuilder object, the warning will be gone. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43479 [details] [review] Bug 14544: Add missing biblio <=> biblioitems <=> items relationships to the schema Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adjusted by a follow-up.
Created attachment 43480 [details] [review] Bug 14544: QA fixes - some minor bug fixes See comment 171 and 172 for more details. This patch mainly fixes typo or silly error in templates. It also uses the relationships added by previous patch to join the biblioitems and items tables (changes in opac-shelves.pl and shelves.pl). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43481 [details] [review] Bug 14544: [QA Follow-up] Added some test descriptions Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 43482 [details] [review] Bug 14544: [QA Follow-up] DBIx::Class changes One of the follow-ups added a belongs_to from Biblioitem to Item just as biblioitemnumber was a foreign key to Item. The relation goes the other way. As a temporary measure, I added set_primary_key in the local section of Virtualshelfcontent. Should be moved later with a real db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment [Continuation]: Thanks again for your hard work. >> OPAC - Share a list - Enter email address - Return to your lists: This list >> does not exist ?? op=view && category=1. Adjust the URL? > Fixed! > Yes, the op is 'list', not 'view". Fixed?? This is still a problem. Did you test it? (No blocker for such a large patch, but ... Do you miss the shelf number?) > Fixed! But... This is a bad one. > In opac-shelves.pl and shelves.pl, the join was only done on biblio. > But itemcallnumber is in the items table. > I had to add the biblio<=>biblioitems<=>items relationship to the Schema and a PK on virtualshelfcontents table > (DBIx::Class::Storage::DBIHacks::_adjust_select_args_for_complex_prefetch(): Unable to perform complex limited prefetch off Virtualshelfcontent > without declared primary key). But this is not completely solved. View a private list in opac or staff: Bang. DBIx::Class::ResultSet::next(): Operation requires a primary key to be declared on 'Virtualshelfcontent' via set_primary_key How did you test this? :) Apparently, you forgot to include this added PK in your patches. Fixed by a hack now: added set_primary_key in Virtualshelfcontent in the local section. Please adjust this by defining the PK on the table and regenerating the DBIx file. Note also that I removed the belongs_to from Biblioitem to Item. This is not a FK but a PK. > Fixed! > The "op" was "view" instead of "remove_biblios" on the submit button. > Note that the individual links work (and intranet is ok). I removed two items. One should not be removed, the other should. This works now, but the message is not really clear. No blocker. > The last patch reintroduces the read-only list feature/bug :) > If we decide later to remove it, we would be able to do it reverting this patch! Applying: Bug 14544: allow_delete_own == 0 could represent a read-only list Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Virtualshelves.t CONFLICT (content): Merge conflict in t/db_dependent/Virtualshelves.t At this time I rather obsolete this additional patch. The wording was removed, the behavior slightly changed. No need to test this now. It has been just a reminder that changing behavior may need more communication than one incidental IRC chat :) Additional observation: The Author and [Date added] column in the shelf view on staff are empty. (With and without the last four follow-up patches.) Passed QA
Note to RM Another db rev may follow from Jonathan to correct the mentioned PK issue with DBIx. Please note that you get Koha::Exception too now. (See earlier QA comment.) An early push may help us to detect if this patch set still contains some further issues. But it looks quite good.
Created attachment 43485 [details] [review] Bug 14544: Fix 'back to lists' link
(In reply to Marcel de Rooy from comment #206) > QA Comment [Continuation]: > > Thanks again for your hard work. > > >> OPAC - Share a list - Enter email address - Return to your lists: This list > >> does not exist ?? op=view && category=1. Adjust the URL? > > Fixed! > > Yes, the op is 'list', not 'view". > Fixed?? This is still a problem. Did you test it? (No blocker for such a > large patch, but ... Do you miss the shelf number?) Hum no, I have tested the link just before the form submit! The last patch fixes that. > > Fixed! But... This is a bad one. > > In opac-shelves.pl and shelves.pl, the join was only done on biblio. > > But itemcallnumber is in the items table. > > I had to add the biblio<=>biblioitems<=>items relationship to the Schema and a PK on virtualshelfcontents table > > (DBIx::Class::Storage::DBIHacks::_adjust_select_args_for_complex_prefetch(): Unable to perform complex limited prefetch off Virtualshelfcontent > > without declared primary key). > But this is not completely solved. > View a private list in opac or staff: Bang. > DBIx::Class::ResultSet::next(): Operation requires a primary key to be > declared on 'Virtualshelfcontent' via set_primary_key > How did you test this? :) Apparently, you forgot to include this added PK in > your patches. > Fixed by a hack now: added set_primary_key in Virtualshelfcontent in the > local section. > Please adjust this by defining the PK on the table and regenerating the DBIx > file. > Note also that I removed the belongs_to from Biblioitem to Item. This is not > a FK but a PK. With my previous branch (so without your follow-ups), I have regenerated the schema and I don't get an error viewing a private list. That's why I have added the belongs_to rs.
Created attachment 43486 [details] [review] Bug 14544: Fixes regression: display author and dateadded on the list view [intranet]
(In reply to Marcel de Rooy from comment #206) > Additional observation: The Author and [Date added] column in the shelf view > on staff are empty. (With and without the last four follow-up patches.) See last patch.
(In reply to Jonathan Druart from comment #208) > Created attachment 43485 [details] [review] [review] > Bug 14544: Fix 'back to lists' link Sure about the location? sendshelf? Creating a share is shareshelf?
(In reply to Marcel de Rooy from comment #212) > (In reply to Jonathan Druart from comment #208) > > Created attachment 43485 [details] [review] [review] [review] > > Bug 14544: Fix 'back to lists' link > > Sure about the location? sendshelf? Creating a share is shareshelf? Erk, no! So there is another one missing.
Created attachment 43487 [details] [review] Bug 14544: Fix 'Back to lists' links on sharing a shelf
(In reply to Marcel de Rooy from comment #212) > (In reply to Jonathan Druart from comment #208) > > Created attachment 43485 [details] [review] [review] [review] > > Bug 14544: Fix 'back to lists' link > > Sure about the location? sendshelf? Creating a share is shareshelf? Sorry about that. I am sure I have fixed it already but I have certainly lost it during a rebase -i. Now it works!
Please rebase.
Created attachment 44467 [details] [review] Bug 14544: Make the intranet side independent of Page.pm Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44468 [details] [review] Bug 14544: Get rid of GetBibliosShelves Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44469 [details] [review] Bug 14544: Get rid of GetShelves This does not work for now, see later. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44470 [details] [review] Bug 14544: Make the OPAC side independent of Page.pm Bug 14544: Fix redirect on editing a list If you edit a list from the list view, after saving the form, you are not redirected to the list view (but on the edit form). Bug 14544: Cosmetic: › should be a class divider Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44471 [details] [review] Bug 14544: Get rid of ShelfPossibleAction Bug 14544: (follow-up) Get rid of ShelfPossibleAction Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44472 [details] [review] Bug 14544: Get rid of GetShelfContent Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44473 [details] [review] Bug 14544: Get rid of GetSomeShelfNames Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44474 [details] [review] Bug 14544: Move HandleDelBorrower to C4::Members Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44475 [details] [review] Bug 14544: Get rid of C4::VirtualShelves and C4::VirtualShelves::Page Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44476 [details] [review] Bug 14544: Remove wrong tips about permissions The owner of a list should not need any permission to remove contents. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44477 [details] [review] Bug 14544: Fix regression on adding a new list The user should not be allowed to create a new list and get a friendly error message if he tries. The "New list" links are removed. Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44478 [details] [review] Bug 14544: DB Changes: Set a default value for sortfield Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44479 [details] [review] Bug 14544 - Handle database error when creating a shelf with DB admin account Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> I don't think this is useful, it will be a mess if we have to check this case everywhere. But it works! I let the cut to the QAer or RM. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44480 [details] [review] Bug 14544: [QA Follow-up] Fix warning about transaction When running Virtualshelves.t, you have this warning: DBD::mysql::db begin_work failed: Already in a transaction at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1339. By moving the DBI lines after creating the TestBuilder object, the warning will be gone. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44481 [details] [review] Bug 14544: Add missing biblio <=> biblioitems <=> items relationships to the schema Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Adjusted by a follow-up.
Created attachment 44482 [details] [review] Bug 14544: QA fixes - some minor bug fixes See comment 171 and 172 for more details. This patch mainly fixes typo or silly error in templates. It also uses the relationships added by previous patch to join the biblioitems and items tables (changes in opac-shelves.pl and shelves.pl). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44483 [details] [review] Bug 14544: [QA Follow-up] Added some test descriptions Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44484 [details] [review] Bug 14544: [QA Follow-up] DBIx::Class changes One of the follow-ups added a belongs_to from Biblioitem to Item just as biblioitemnumber was a foreign key to Item. The relation goes the other way. As a temporary measure, I added set_primary_key in the local section of Virtualshelfcontent. Should be moved later with a real db rev. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 44485 [details] [review] Bug 14544: Fix 'back to lists' link
Created attachment 44486 [details] [review] Bug 14544: Fixes regression: display author and dateadded on the list view [intranet]
Created attachment 44487 [details] [review] Bug 14544: Fix 'Back to lists' links on sharing a shelf
Patches pushed to master. Thanks Jonathan!
Created attachment 44758 [details] [review] Bug 14544: Remove debug line
Thanks for the followup, Jonathan!
Created attachment 44845 [details] [review] Bug 14544: Fix regression on sending an email from staff The shelfnumber was not filled and result in a software error Can't call method "get_contents" on an undefined value at /home/koha/src/virtualshelves/sendshelf.pl line 74.
Created attachment 44852 [details] [review] Bug 14544: Fix regression on sending an email from staff The shelfnumber was not filled and result in a software error Can't call method "get_contents" on an undefined value at /home/koha/src/virtualshelves/sendshelf.pl line 74.
Created attachment 44863 [details] [review] [SIGNED-OFF] Bug 14544: Fix regression on sending an email from staff The shelfnumber was not filled and result in a software error Can't call method "get_contents" on an undefined value at /home/koha/src/virtualshelves/sendshelf.pl line 74. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Whith patch mail is sent. No errors
Created attachment 44872 [details] [review] [PASSED QA] Bug 14544: Fix regression on sending an email from staff The shelfnumber was not filled and result in a software error Can't call method "get_contents" on an undefined value at /home/koha/src/virtualshelves/sendshelf.pl line 74. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Whith patch mail is sent. No errors Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Last patch needs to be pushed.
Followup pushed to master. Thanks Jonathan, again!
This patch added a new dependency (Exception::Class, version 1.39) - is version 1.39+ really necessary? Eq., in Debian Jessie, off-the-shelf Exception::Class is a bit older (1.38). Upgrade attempt from 3.22.6 -> 16.05.01 (on Jessie, from packages) does not trigger installation of the more recent version (I guess it's not packaged) - looks like 1.38 will be listed as unsatisfied dependency after an upgrade?
(In reply to Jacek Ablewicz from comment #247) > This patch added a new dependency (Exception::Class, version 1.39) - is > version 1.39+ really necessary? Eq., in Debian Jessie, off-the-shelf > Exception::Class is a bit older (1.38). > > Upgrade attempt from 3.22.6 -> 16.05.01 (on Jessie, from packages) does not > trigger installation of the more recent version (I guess it's not packaged) > - looks like 1.38 will be listed as unsatisfied dependency after an upgrade? I have just opened bug 16695. Thanks for reporting this!