Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons
Summary: Add a "Patron List" feature for storing and manipulating collections of patrons
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords: rel_3_14_candidate
Depends on: 8798
Blocks: 9811 11857 11906 11941 11942
  Show dependency treegraph
 
Reported: 2013-07-10 13:12 UTC by Kyle M Hall
Modified: 2016-01-22 02:11 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (42.85 KB, patch)
2013-07-17 14:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (42.82 KB, patch)
2013-07-17 14:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (42.90 KB, patch)
2013-07-17 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (47.29 KB, patch)
2013-07-17 16:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (53.11 KB, patch)
2013-07-17 16:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (53.10 KB, patch)
2013-07-17 16:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (53.21 KB, patch)
2013-07-18 11:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup - Fix DBIx::Class error (2.63 KB, patch)
2013-07-18 11:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (53.18 KB, patch)
2013-07-18 12:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup - Fix DBIx::Class error (2.63 KB, patch)
2013-07-18 12:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 [Follow-up] Add a "Patron List" feature for storing and manipulating collections of patrons (22.51 KB, patch)
2013-07-18 14:36 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFf] Bug 10656: add regression test (4.87 KB, patch)
2013-08-20 21:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup 3 (3.41 KB, patch)
2013-08-29 14:59 UTC, Kyle M Hall
Details | Diff | Splinter 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 (2.34 KB, patch)
2013-09-03 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons (53.23 KB, patch)
2013-09-13 13:15 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup - Fix DBIx::Class error (2.69 KB, patch)
2013-09-13 13:17 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 10565 [Follow-up] Add a "Patron List" feature for storing and manipulating collections of patrons (2.69 KB, patch)
2013-09-13 13:20 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup 3 (2.69 KB, patch)
2013-09-13 13:22 UTC, Melissa Lefebvre
Details | Diff | Splinter 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 (2.40 KB, patch)
2013-09-13 13:26 UTC, Melissa Lefebvre
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-07-10 13:12:22 UTC
Some libraries would like to create lists of patrons for later manipulation. This will involve create a "Patron List" system similar in some ways to the existing Lists feature for records.
Comment 1 Kyle M Hall 2013-07-17 14:13:14 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-07-17 14:20:41 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-07-17 14:24:42 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-07-17 14:33:18 UTC
Unit tests will be forthcoming, qa fyi ; )
Comment 5 Owen Leonard 2013-07-17 15:03:52 UTC
A template file seems to be missing: patron_lists/list.tt
Comment 6 Kyle M Hall 2013-07-17 16:21:23 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-07-17 16:22:32 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-07-17 16:23:10 UTC
Good catch, fixed!

(In reply to Owen Leonard from comment #5)
> A template file seems to be missing: patron_lists/list.tt
Comment 9 Kyle M Hall 2013-07-17 16:34:54 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2013-07-17 19:28:59 UTC
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.
Comment 11 Liz Rea 2013-07-18 03:55:22 UTC
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.
Comment 12 Kyle M Hall 2013-07-18 11:52:07 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-07-18 11:52:44 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-07-18 12:05:47 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-07-18 12:05:57 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2013-07-18 12:10:18 UTC
(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!
Comment 17 Kyle M Hall 2013-07-18 12:10:50 UTC
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.
Comment 18 Owen Leonard 2013-07-18 14:36:32 UTC Comment hidden (obsolete)
Comment 19 Owen Leonard 2013-07-18 14:50:13 UTC
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.
Comment 20 Katrin Fischer 2013-08-20 21:07:07 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2013-08-20 21:12:26 UTC
Sorry... typoed the bug number.
Comment 22 Liz Rea 2013-08-28 00:40:21 UTC
Question, for fields like "Library" - Under what condition would you ever want to delete that field entirely? Is it possibly dangerous to allow?
Comment 23 Liz Rea 2013-08-28 00:42:42 UTC
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.
Comment 24 Liz Rea 2013-08-28 00:43:32 UTC
er just realised that the last two comments really have nothing to do with this. Sorry about that.
Comment 25 Liz Rea 2013-08-28 21:28:42 UTC
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
Comment 26 Liz Rea 2013-08-28 21:33:16 UTC
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
Comment 27 Kyle M Hall 2013-08-29 14:59:46 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2013-08-29 15:01:22 UTC
Liz, all your comments and concerns are valid. Thanks! I believe this latest followup should address all those issues.

Kyle
Comment 29 Liz Rea 2013-08-29 23:54:43 UTC
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
Comment 30 Kyle M Hall 2013-09-03 18:20:30 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2013-09-03 18:21:02 UTC
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.
Comment 32 Nora Blake 2013-09-09 13:04:35 UTC
I am extremely happy with this and would like to sign off on it.
Comment 33 Melissa Lefebvre 2013-09-13 13:15:25 UTC
Created attachment 21084 [details] [review]
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons
Comment 34 Melissa Lefebvre 2013-09-13 13:17:54 UTC
Created attachment 21086 [details] [review]
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup - Fix DBIx::Class error
Comment 35 Melissa Lefebvre 2013-09-13 13:20:51 UTC
Created attachment 21087 [details] [review]
Bug 10565 [Follow-up] Add a "Patron List" feature for storing and manipulating collections of patrons
Comment 36 Melissa Lefebvre 2013-09-13 13:22:55 UTC
Created attachment 21088 [details] [review]
Bug 10565 - Add a "Patron List" feature for storing and manipulating collections of patrons - Followup 3
Comment 37 Melissa Lefebvre 2013-09-13 13:26:03 UTC
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
Comment 38 Galen Charlton 2013-10-14 22:58:20 UTC
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.
Comment 39 Nicole C. Engard 2013-10-15 22:52:38 UTC
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.
Comment 40 Nicole C. Engard 2013-10-21 17:34:22 UTC
nevermind - fixed
Comment 41 Olli-Antti Kivilahti 2014-03-14 11:20:07 UTC
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.
Comment 42 Katrin Fischer 2014-03-14 11:22:23 UTC
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.
Comment 43 Olli-Antti Kivilahti 2014-03-14 11:33:43 UTC
It is a list, the header has "more" -> "lists" implying more lists :)
imho all kind of lists
Comment 44 Marcel de Rooy 2014-03-14 13:26:10 UTC
(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).
Comment 45 Robin Sheat 2014-03-25 04:29:31 UTC
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.