Description
Kyle M Hall (khall)
2013-07-10 13:12:22 UTC
Created attachment 19726 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from the "More" pulldown on the main menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Go back to the Patron Lists feature and delete your lists. Created attachment 19727 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 0) Apply the patch for Bug 8798 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from Koha's Tools menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Go back to the Patron Lists feature and delete your lists. Created attachment 19728 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 0) Apply the patch for Bug 8798 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from Koha's Tools menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Try manipulating a list of patrons using the batch patron modification tool. 15) Go back to the Patron Lists feature and delete your lists. Unit tests will be forthcoming, qa fyi ; ) A template file seems to be missing: patron_lists/list.tt Created attachment 19732 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 0) Apply the patch for Bug 8798 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from Koha's Tools menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Try manipulating a list of patrons using the batch patron modification tool. 15) Go back to the Patron Lists feature and delete your lists. 16) Run 'prove t/db_dependent/PatronLists.t' Created attachment 19733 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 0) Apply the patch for Bug 8798 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from Koha's Tools menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Try manipulating a list of patrons using the batch patron modification tool. 15) Go back to the Patron Lists feature and delete your lists. 16) Run 'prove t/db_dependent/PatronLists.t' Good catch, fixed! (In reply to Owen Leonard from comment #5) > A template file seems to be missing: patron_lists/list.tt Created attachment 19734 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 0) Apply the patch for Bug 8798 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from Koha's Tools menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Try manipulating a list of patrons using the batch patron modification tool. 15) Go back to the Patron Lists feature and delete your lists. 16) Run 'prove t/db_dependent/PatronLists.t' After you create a new list, it would be nice if it redirected you to the page for adding patrons to that list. That's how it works with book lists. I like this a lot. Like, a LOT. As in, I squeed when I was playing with it. However, there is something really strange about the hover state of the buttons - see http://img844.imageshack.us/img844/6169/iq7r.png That's Firefox 22 on Ubuntu Precise. Also, the unit test seems to fail: devlibrary-koha@wingardium-leviosa:/home/liz/koha-src/koha$ prove t/db_dependent/PatronLists.t t/db_dependent/PatronLists.t .. 1/9 DBIx::Class::Carp::__ANON__(): "might_have/has_one" must not be on columns with is_nullable set to true (Koha::Schema::Result::Borrower/cardnumber). This might indicate an incorrect use of those relationship helpers instead of belongs_to. at /usr/share/perl5/DBIx/Class/Relationship/HasOne.pm line 96 t/db_dependent/PatronLists.t .. 5/9 # Failed test 'AddPatronsToList works for cardnumbers' # at t/db_dependent/PatronLists.t line 38. # Looks like you failed 1 test of 9. t/db_dependent/PatronLists.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/9 subtests Test Summary Report ------------------- t/db_dependent/PatronLists.t (Wstat: 256 Tests: 9 Failed: 1) Failed test: 5 Non-zero exit status: 1 Files=1, Tests=9, 1 wallclock secs ( 0.02 usr 0.00 sys + 0.70 cusr 0.04 csys = 0.76 CPU) Result: FAIL Functional tests pass though... and I mean it when I say this is really fantastic. People will throw confetti. Created attachment 19757 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 0) Apply the patch for Bug 8798 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from Koha's Tools menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Try manipulating a list of patrons using the batch patron modification tool. 15) Go back to the Patron Lists feature and delete your lists. 16) Run 'prove t/db_dependent/PatronLists.t' Created attachment 19759 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup - Fix DBIx::Class error Fixes this error: DBIx::Class::Relationship::HasOne::might_have(): "might_have/has_one" must not be on columns with is_nullable set to true (Koha::Schema::Result::Borrower/cardnumber). This might indicate an incorrect use of those relationship helpers instead of belongs_to. Created attachment 19761 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons The patron lists feature is somewhat similar to the record lists feature in that it allows a librarian to create a list of patrons for later retrieval and manipluation. These lists can then be used with the batch patron modification tool. Test Plan: 0) Apply the patch for Bug 8798 1) Apply this patch 2) Run updatedatabase.pl 3) Access the patron lists feature from Koha's Tools menu. 4) Create a new list via the "New patron list" button. 5) For this list, click the "Edit" button, and change the list name. 6) For this list, click the "Add patrons" button, and search for and add some patrons to your list. 7) For this list select some patrons to remove them. 8) Try both adding some new patrons, and removing some old patrons as a single action. 9) Click the "Patrons" link on the Koha toolbar 10) Search the patrons, or browse by letter to get patron results 11) Check the checkboxes next to one or more patrons, and add the selected patrons to your existing list. 12) Change the "Selected patrons" pulldown to "All resultant patrons" and add them to your list. 13) Check the checkboxes next to one or more patrons, and add the selected patrons to a new list. 14) Try manipulating a list of patrons using the batch patron modification tool. 15) Go back to the Patron Lists feature and delete your lists. 16) Run 'prove t/db_dependent/PatronLists.t' Created attachment 19762 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup - Fix DBIx::Class error Fixes this error: DBIx::Class::Relationship::HasOne::might_have(): "might_have/has_one" must not be on columns with is_nullable set to true (Koha::Schema::Result::Borrower/cardnumber). This might indicate an incorrect use of those relationship helpers instead of belongs_to. (In reply to Liz Rea from comment #11) > I like this a lot. Like, a LOT. As in, I squeed when I was playing with it. > However, there is something really strange about the hover state of the > buttons - see http://img844.imageshack.us/img844/6169/iq7r.png > > That's Firefox 22 on Ubuntu Precise. It looks like the bootstrap css isn't playing well with Koha's css somewhere. I have it fixed in this new edition of the patch. > > Also, the unit test seems to fail: > > devlibrary-koha@wingardium-leviosa:/home/liz/koha-src/koha$ prove > t/db_dependent/PatronLists.t > t/db_dependent/PatronLists.t .. 1/9 DBIx::Class::Carp::__ANON__(): > "might_have/has_one" must not be on columns with is_nullable set to true > (Koha::Schema::Result::Borrower/cardnumber). This might indicate an > incorrect use of those relationship helpers instead of belongs_to. at > /usr/share/perl5/DBIx/Class/Relationship/HasOne.pm line 96 > t/db_dependent/PatronLists.t .. 5/9 > # Failed test 'AddPatronsToList works for cardnumbers' > # at t/db_dependent/PatronLists.t line 38. > # Looks like you failed 1 test of 9. > t/db_dependent/PatronLists.t .. Dubious, test returned 1 (wstat 256, 0x100) > Failed 1/9 subtests > > Test Summary Report > ------------------- > t/db_dependent/PatronLists.t (Wstat: 256 Tests: 9 Failed: 1) > Failed test: 5 > Non-zero exit status: 1 > Files=1, Tests=9, 1 wallclock secs ( 0.02 usr 0.00 sys + 0.70 cusr 0.04 > csys = 0.76 CPU) > Result: FAIL That's odd, I've run the test with multiple databases without an issue. Please try again with this followup patch. > Functional tests pass though... and I mean it when I say this is really > fantastic. People will throw confetti. Excellent! Done! (In reply to Owen Leonard from comment #10) > After you create a new list, it would be nice if it redirected you to the > page for adding patrons to that list. That's how it works with book lists. Created attachment 19770 [details] [review] Bug 10565 [Follow-up] Add a "Patron List" feature for storing and manipulating collections of patrons This patch makes the following changes: On the list add/edit page: - Improvements to breadcrumbs and titles on the add/edit page. - Removal of custom validation JavaScript in favor of built-in validation plugin. On the list view page: - Corrected title on list page: Showing list name instead of "New list" - Added "select all" and "clear all" to a toolbar like on the biblio list page. Moved the "remove selected" button to this toolbar like it is on the biblio list page. - Added more standard DataTables configuration with default sort, correctly ignored columns, and pager count options. - Display patron names as "surname, firstname." - Improve JavaScript for removing entries from the list of patrons to add (to avoid "javascript:" pseudo-protocol). - Move "add patrons" controls so that they are only shown after patrons have been added. - Add toolbar to patron list view page with button for "Edit list" and "Batch modify." The "Batch modify" button takes you directly to a batch modification operation on your list. - Added zipcode to displayed information. - Added message to be displayed when there are no items on the list (instead of showing empty table). On the list of lists page: - Added more standard DataTables configuration with default sort, correctly ignored columns, and pager count options. - Wrapped "new list" button in standard toolbar div. - Show message instead of empty table if there are no lists. - Style in-table buttons with "btn-mini" for more compact look. On scripts: - Changed "authnotrequired" from 1 to 0, added 'borrowers' permission requirement. By the way, I removed the Bootstrap styling from the primary submit buttons because I'd rather change that standard interface element in a more deliberate way. I think if we want to start using different styling on submit buttons we should choose a style which is at least somewhat different than toolbar buttons, perhaps a different color of a more subtle style difference. On the other hand, I think Bootstrap's "btn-mini" style is good for controls inside tables, and is something we should start expanding our use of. Created attachment 20511 [details] [review] [SIGNED-OFf] Bug 10656: add regression test This patch adds regression tests to verify the sort order when fetching authorised values in either staff or OPAC mode. This patch also wraps the Koha.t tests in a transaction, and better handles skipping tests if Test::Deep is not installed. To test: [1] Verify that prove -v t/db_dependent/Koha.t passes Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Tests pass with all patches applied. Sorry... typoed the bug number. Question, for fields like "Library" - Under what condition would you ever want to delete that field entirely? Is it possibly dangerous to allow? Also, after a batch modification it would probably be best to show all of the fields in the result table, and if not that, then at least show the columns that were modified. er just realised that the last two comments really have nothing to do with this. Sorry about that. A few legitimate questions - What happens if the borrower you are looking for to add to a list has a name that is fewer than 3 characters? It seems one cannot submit a search manually without the autocomplete. I'm not sure I'd fail it for this, but maybe worth thinking about. It seems also that you cannot add borrowers to a list that do not have a cardnumber. Feature, or bug? Liz I also see that you *can* add borrowers with no cardnumber to a borrower list if you tick the box next to their name in a result list and -> add to list. patron_lists/list.pl?patron_list_id=2 < from here you cannot add borrowers with no cardnumber, so I think I've come to think this is a bug and not a feature. Liz Created attachment 20721 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup 3 * Borrower search works for names with < 3 characters * Borrowers without cardnumbers can now be added Liz, all your comments and concerns are valid. Thanks! I believe this latest followup should address all those issues. Kyle One weirdness: http://img716.imageshack.us/img716/7357/rgya.png The add patron button looks a bit strange here. I wonder if you could just go with the word "Add" ? Nice to have, but not required to have (future maybe?) - add a borrower to a list from their detail or circulation screen (in the more menu perhaps?) Add a link or button to the list interface from the borrower result screen? Add a link or button to delete a list from a displayed list - (patron_lists/list.pl?patron_list_id=10 for example) The ability to export the contents of a patron list as CSV. Even cooler if you could choose which fields to export - I'm thinking of a method by which librarians could, without using saved reports, get a list of borrowers to email, for example. (I know! I know. Scope creep. Just writing it down so the idea doesn't get lost. :)) Functionally this works great - I'm still having trouble with the tests though: devlibrary-koha@wingardium-leviosa:/home/liz/koha-src/koha$ prove t/db_dependent/PatronLists.t t/db_dependent/PatronLists.t .. 1/9 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_devlibrary`.`patron_lists`, CONSTRAINT `patron_lists_ibfk_1` FOREIGN KEY (`owner`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO patron_lists ( name, owner) VALUES ( ?, ? )" with ParamValues: 0='Test List 1', 1=1] at Koha/List/Patron.pm line 121 # Looks like you planned 9 tests but ran 2. # Looks like your test exited with 255 just after 2. t/db_dependent/PatronLists.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 7/9 subtests Test Summary Report ------------------- t/db_dependent/PatronLists.t (Wstat: 65280 Tests: 2 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 9 tests but ran 2. Files=1, Tests=2, 1 wallclock secs ( 0.02 usr 0.01 sys + 0.52 cusr 0.02 csys = 0.57 CPU) Result: FAIL Of note, and possibly extremely relevant, is that my failure results came from an *upgraded* database, and not a fresh one. All tests pass on a fresh database with only the sample data. I'm putting this in failed qa only because I'd like an answer about the upgraded DB tests - assuming that is all ok, I'm happy to sign off on this as-is. Thanks for your continued work, Liz Created attachment 20768 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup 4 - Don't assume borrowernumber 1 exists in unit test Liz, I think the problem with the unit test is that I'm assuming there is a borrower with a borrowernumber of 1 that is the inital Koha Admin account created during installation. Is this an improper assumption? I'm guessing it is. I've attached another followup to remedy the situation. I am extremely happy with this and would like to sign off on it. Created attachment 21084 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons Created attachment 21086 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup - Fix DBIx::Class error Created attachment 21087 [details] [review] Bug 10565 [Follow-up] Add a "Patron List" feature for storing and manipulating collections of patrons Created attachment 21088 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup 3 Created attachment 21089 [details] [review] Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup 4 - Don't assume borrowernumber 1 exists in unit test Pushed to master, along with follow-ups, including one that adds a new tools permission for the feature. Note that I did not include your first three follow-ups as they ended up conflicting with the fix for 10636. Thanks, Kyle! I will add, however, that this made it in by the skin of its teeth, and primarily because it exercises DBIx::Class support. In particular, it was rather careless that the patch as submitted did not require staff authentication. This is a BIG deal -- even if the feature were suitable for all staff users (which is debatable for technical services staff) -- it is obviously not suitable for anybody who happens to be good add guessing URL patterns. Just did a fetch and get this error when trying to open the new tool: Base class package "DBIx::Class::Schema" is empty. (Perhaps you need to 'use' the module which defines that package first, or make that module available in @INC (@INC contains: /home/nengard/kohaclone /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl .). at /home/nengard/kohaclone/Koha/Schema.pm line 10 BEGIN failed--compilation aborted at /home/nengard/kohaclone/Koha/Schema.pm line 10. Compilation failed in require at /home/nengard/kohaclone/Koha/Database.pm line 38. BEGIN failed--compilation aborted at /home/nengard/kohaclone/Koha/Database.pm line 38. Compilation failed in require at /home/nengard/kohaclone/Koha/List/Patron.pm line 32. BEGIN failed--compilation aborted at /home/nengard/kohaclone/Koha/List/Patron.pm line 32. Compilation failed in require at /home/nengard/kohaclone/patron_lists/lists.pl line 26. BEGIN failed--compilation aborted at /home/nengard/kohaclone/patron_lists/lists.pl line 26. nevermind - fixed Why is this feature not accessible from the HTML header "More" -> "Lists" ? I found a character encoding issue when displaying the Patrons "circ notes" inside the Patron list. Hm, I'd say it's not a new module, but an additional feature set for the patrons module, so it wouldn't go under more, but it's accessible from the tools page. It is a list, the header has "more" -> "lists" implying more lists :) imho all kind of lists (In reply to Olli-Antti Kivilahti from comment #43) > It is a list, the header has "more" -> "lists" implying more lists :) > imho all kind of lists Agreed that it could be confusing to make the distinction between lists (of books, sometimes referred to as virtual shelves in Koha) and patron lists (brand new), please note that the patron lists are staff-based, while lists are opac-based (with support in staff). Too late to do much about it now, but this doesn't follow: http://wiki.koha-community.org/wiki/Coding_Guidelines#PERL14:_Exports and arguably the next one too. We should try to avoid littering the namespace with default exports. |