Colin this is awesome! I just got back from a library that is keeping all patrons in a family on one card to get this kind of functionality. Remember to add the RFC to the wiki using the RFC template. http://wiki.koha-community.org/wiki/Enhancement_Request_Guidelines#RFC_Template Hi Colin, I am really interested in this module. Have you done any more work on this? I'm quite keen to get it working on an Android smart phone so that titles could be checked on the library floor using a wireless connection. Cheers Jo. Hi Colin, maybe you can give us an update on this development? Its in production at Halton. I'll pull the patches together ensure they apply against master and post them here for inspection (In reply to Colin Campbell from comment #4) > Its in production at Halton. I'll pull the patches together ensure they > apply against master and post them here for inspection Any news on this? :-) This could be quite useful as many libraries in Sweden have this type of service. I'd be happy to see it as a part of community Koha. Created attachment 22162 [details] [review] Housebound module for v3.6 Conflicts: admin/systempreferences.pl Created attachment 22163 [details] [review] Added syspref for housebound previously issued items Conflicts: C4/Circulation.pm koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt Created attachment 22164 [details] [review] Remove unused variables from C4::Housebound localized lexical vars to where they are used Avoided unnecessary method calls Remove a finish No need to interpolate to constant strings Fix some violations of perlstyle Created attachment 22165 [details] [review] Commit Housebound Updates Patches submitted on behalf of Colin and Mark... Patch applied cleanly, go forth and signoff This mostly applies cleanly on master. I think what we are missing is unit tests, and maybe we should move to the Koha:: namespace? Marking this "Failed QA" based on Chris' comments. See coding guideline "PERL17: Unit tests are required". Created attachment 26816 [details] [review] Bug 5670: Housebound Readers Module New module to handle management of circulation to Housebound readers. - Ability to record users as housebound (HB category), volunteers (combined deliverers and choosers, VOL category), deliverers (DELIV category) choosers (CHO category). - Ability to link users to individual delivery runs. - Ability to check on issuing to housebound users flagging if title has previously been issued to this user, to aid choosers. - 'Delivery Days' and 'Delivery Frequencies' are customizable through authorised values ('Day' and 'Frequency' categories respectively. Sponsored by Halton Borough Council * C4/Auth.pm: Add check: useHouseboundModule. * C4/Circulation.pm: use Housebound to check for previous issue. * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: add 'previous checkout' warning. * C4/Housebound.pm: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/houseboundedit.tt: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/houseboundinstances.tt: new. * members/housebound.pl: new. * members/houseboundedit.pl: new. * members/houseboundinstances.pl: new. * installer/data/mysql/atomicupdate/housebound_tables.sql: new. - create 'housebound', 'housebound_instance' tables, - create 'HB', 'VOL', 'DELIV', 'CHO' borrower categories, - create sample 'Day', 'Frequency' authorised values, - add 'useHouseboundModule', 'useHouseboundCheckPrevious' syspref. * t/Housebound.t: new. Test plan: - Apply patch. - Run atomic update script. - Optionally, run Unit Tests (t/Housebound.t) - Create at least 1 of each: 'HB', 'VOL', 'DELIV', 'CHO' borrower. - Optionally, add additional authorised values of 'Day' and 'Frequency' categories. - Verify 'useHouseboundModule' & 'useHouseboundCheckPrevious' sysprefs are on. - On the page of the 'HB' borrower, open the new 'Housebound' menu. - create housebound profile + ensure Day and Frequency values are from correct authorised values - create 'housebound instances' (deliveries) + ensure chooser/deliverer lists are populated with VOL, DELIV, CHO borrowers - edit the profile/instances. - delete the profile/instances. - For a borrower with a housebound profile, issue an item, then - return that item - attempt to re-issue that item (you should get a warning) - Switch off 'useHouseboundCheckPrevious', repeat last test - the warning should not appear - Switch off 'useHouseboundModule' - the Housebound menu should disappear - the issue warnings should no longer appear. Largely based on prior work from Mark Gavillet and Colin Campbell. Hello, We have now had some time to revise this patch series. The different patches are now replaced with one big patch, which includes unit tests and a polished version of this module. I believe this is ready for sign off and QA. Let me know if you have any queries. Best wishes, Alex Created attachment 27288 [details] [review] Bug 5670: Housebound Readers Module New module to handle management of circulation to Housebound readers. - Ability to record users as housebound (HB category), volunteers (combined deliverers and choosers, VOL category), deliverers (DELIV category) choosers (CHO category). - Ability to link users to individual delivery runs. - Ability to check on issuing to housebound users flagging if title has previously been issued to this user, to aid choosers. - 'Delivery Days' and 'Delivery Frequencies' are customizable through authorised values ('Day' and 'Frequency' categories respectively. Sponsored by Halton Borough Council * C4/Auth.pm: Add check: useHouseboundModule. * C4/Circulation.pm: use Housebound to check for previous issue. * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: add 'previous checkout' warning. * C4/Housebound.pm: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/houseboundedit.tt: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/houseboundinstances.tt: new. * members/housebound.pl: new. * members/houseboundedit.pl: new. * members/houseboundinstances.pl: new. * installer/data/mysql/atomicupdate/housebound_tables.sql: new. - create 'housebound', 'housebound_instance' tables, - create 'HB', 'VOL', 'DELIV', 'CHO' borrower categories, - create sample 'Day', 'Frequency' authorised values, - add 'useHouseboundModule', 'useHouseboundCheckPrevious' syspref. * t/Housebound.t: new. Test plan: - Apply patch. - Run atomic update script. - Optionally, run Unit Tests (t/Housebound.t) - Create at least 1 of each: 'HB', 'VOL', 'DELIV', 'CHO' borrower. - Optionally, add additional authorised values of 'Day' and 'Frequency' categories. - Verify 'useHouseboundModule' & 'useHouseboundCheckPrevious' sysprefs are on. - On the page of the 'HB' borrower, open the new 'Housebound' menu. - create housebound profile + ensure Day and Frequency values are from correct authorised values - create 'housebound instances' (deliveries) + ensure chooser/deliverer lists are populated with VOL, DELIV, CHO borrowers - edit the profile/instances. - delete the profile/instances. - For a borrower with a housebound profile, issue an item, then - return that item - attempt to re-issue that item (you should get a warning) - Switch off 'useHouseboundCheckPrevious', repeat last test - the warning should not appear - Switch off 'useHouseboundModule' - the Housebound menu should disappear - the issue warnings should no longer appear. Largely based on prior work from Mark Gavillet and Colin Campbell. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Works as advertised. A few QA notes though: * HB is already a patron type, your sql fails to create all the patron types if one already exists * In your test plan, the authorised values are *not* optional as stated * The system preferences need to be defined in a pref file, right now they only show up under the local use tab Comment on attachment 27288 [details] [review] Bug 5670: Housebound Readers Module Review of attachment 27288 [details] [review]: ----------------------------------------------------------------- Hi Alex, first of all: thx for resubmitting this! I have done a code review and looked at the feature for a bit and have found some problems: 1) Database update I think the database update should be moved from the atomicupdate into a real database update using updatedatabase.pl and modifying kohastructure.sql. You will then also want to add the new system preferences to syspref.sql. For existing installations, new features like this should be turned off by default, for new installations it's debatable, but it would be ok for me to have it turned on by default to show that the feature is there. 2) Templates Owen is going to look at the template side of things, so I am leaving those out of my code review :) 3) Design/Architecture To be honest, I have a problem with the hardcoded patron category codes. One point being, that the codes might already be in use in some libraries with a totally different meaning. It also takes away flexibility and is a bit of a translation issue. Also, if you have patrons that have normal patron accounts, but also serves as choosers and deliverers, you have to add 3 accounts for those. My thought is, that those are not new 'persons', but persons with a new role. I'd suggest looking at other options here - system preferences, patron_attributes, options on patron category level, a new database table. Another thought: if you think of a multi branch library system, the lists of choosers and deliverers will get too long to handle. I'd suggest a search option - there are plenty of examples in Koha: autocomplete patron search, modal windows, etc. used for adding patrons to routing lists, patron lists, finding guarantors, fund owners etc. To me this feels currently a bit more like a plugin, where the heavy use of authorized_values and hardcoded bits would make more sense. But as it seems intended for inclusion in Koha, more heavy code changes and a tighter integration seem favourable. The other way I could see would be implementing the check on 'previously issued' in a general way, and move the other functionality into a plugin. ::: C4/Circulation.pm @@ +845,5 @@ > > + # If Housebound patron and useHouseboundCheckPrev syspref is ON > + # check for previous issue of item to patron. > + if ( C4::Context->preference('useHouseboundCheckPrevious') == 1 > + && $borrower->{categorycode} eq 'HB' ) I think we should avoid a hardcoded patron category code (see comment above). ::: C4/Housebound.pm @@ +1,1 @@ > +package C4::Housebound; In general, it would be good if new modules added used DBIX instead of accessing the database directly. I know this code has been around for a while, but it might be worth taking a look at rewriting it. @@ +78,5 @@ > + > +=head2 CreateHouseboundDetails > + > + CreateHouseboundDetails( $borrowernumber, $day, $frequency, > + $Itype_quant, $Item_subject, $Item_authors, I think it's not an official coding guideline, but normally lower case variable names are used in Koha. ::: installer/data/mysql/atomicupdate/housebound_tables.sql @@ +1,2 @@ > +CREATE TABLE IF NOT EXISTS `housebound` ( > + `hbnumber` int(11) NOT NULL auto_increment, `hbnumber` int(11) NOT NULL auto_increment, -- Please add some documentation about what the columns and tables are used for (in kohastructure.sql). @@ +4,5 @@ > + `frequency` text, > + `borrowernumber` int(11) default NULL, > + `Itype_quant` varchar(10) default NULL, > + `Item_subject` text, > + `Item_authors` text, I think these should be lower case as well. @@ +30,5 @@ > +insert into systempreferences (variable,value,options,explanation,type) values > +('useHouseboundModule',1,'','If ON, use the Housebound module','YesNo'); > + > +insert into systempreferences (variable,value,options,explanation,type) values > +('useHouseboundCheckPrevious',1,'','If ON, checks if Housebound patrons have previous issued items to be checked out', Ideally I'd like to see this functionality on a separate bug, as this seems a feature, that a lot of people would like to see (partial duplicate to bug 6906 and bug 10812). It could be a setting on patron category level. @@ +34,5 @@ > +('useHouseboundCheckPrevious',1,'','If ON, checks if Housebound patrons have previous issued items to be checked out', > +'YesNo'); > + > +insert into categories (categorycode, description, enrolmentperiod, upperagelimit, dateofbirthrequired, category_type) > +values ('VOL','Housebound volunteer','99','999','1','A'),('HB','Housebound patron','99','999','1','A'),('DELIV','Housebound deliverer','99','999','1','A'),('CHO','Housebound chooser','99','999','1','A'); Hardcoded patron categories - see comments above. @@ +37,5 @@ > +insert into categories (categorycode, description, enrolmentperiod, upperagelimit, dateofbirthrequired, category_type) > +values ('VOL','Housebound volunteer','99','999','1','A'),('HB','Housebound patron','99','999','1','A'),('DELIV','Housebound deliverer','99','999','1','A'),('CHO','Housebound chooser','99','999','1','A'); > + > +insert into authorised_values (category, authorised_value, lib) > +values ('Day','Example_Day','Change Me In Authorised Values (Day)'), ('Frequency','Example_Freq','Change Me In Authorised Values (Frequency)'); I am not sure if I understand the purpose of Day right, but if it's days of the week it would probably be easier to move those to the template - so they would be translatable and not require additional configuration. I am not sure what goes into Frequency. Authorised value codes should be all uppercase. Also: it seems they are mandatory for the feature to work correctly, so they should go into the sample sql files of the web installer. Please include all translated sample files as well - a non-translated value will still give more of a hint than a feature not working because of unknown reasons. I like the idea of the "Change me in..." here. Especially for the database update. ::: members/housebound.pl @@ +31,5 @@ > + GetCurrentHouseboundInstanceList ); > + > +my $input = CGI->new(); > + > +sub nl2br { I think TT can handle this for you with a filter. We have one example in serials I think, with a formatted note field. But this doesn't seem to be used? @@ +58,5 @@ > +my $categorydetail = GetMember( borrowernumber => $borrowernumber); > +my $housebound = GetHouseboundDetails($borrowernumber); > +my $housebound_instances = GetCurrentHouseboundInstanceList($borrowernumber); > + > +$template->param( It would be better to use $tempate->{VARS}->{'...'} ::: members/houseboundinstances.pl @@ +125,5 @@ > + > +$template->param( > + houseboundview => 'on', > + housebound => $housebound, > + DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(), We no longer use DHTMLcalendar, new code should be using Koha::DateUtils. ::: t/Housebound.t @@ +2,5 @@ > +use strict; > +use warnings; > + > +use C4::Context; > +use Test::More tests => 29; Yay tests! You went the hard route here with Mock, which is good, because they will pass when no database is available. I have checked that they do by turning of the MySQL server before running them. There is also the option to rollback transactions and add db_dependent tests. I would be happy to submit a follow-up with some template corrections but I don't want to do so if QA follow-ups will introduce many changes to the templates. Hello, Thanks for all your work reviewing this patch. I'm working on the issues raised, and as a first step I have followed Cait's advice, splitting the CheckPreviousIssue functionality into a separate patch (bug 6906). I will be adding to this series in the near future, obsoleting my old patch, but if you are interested in this, please test the attachment at bug 6906 — it is required for this functionality. Let me know if you have any questions or further feedback. Best wishes, Alex Created attachment 50930 [details] [review] Revised Housebound Patch Created attachment 51377 [details] [review] Rebase on master Same patch rebased on master. New module to handle management of circulation to Housebound readers. - Ability to record users as housebound (HB category), volunteers (combined deliverers and choosers, VOL category), deliverers (DELIV category) choosers (CHO category). - Ability to link users to individual delivery runs. - Ability to check on issuing to housebound users flagging if title has previously been issued to this user, to aid choosers. - 'Delivery Days' and 'Delivery Frequencies' are customizable through authorised values ('Day' and 'Frequency' categories respectively. Sponsored by Halton Borough Council * C4/Auth.pm: Add check: useHouseboundModule. * C4/Circulation.pm: use Housebound to check for previous issue. * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt: add 'previous checkout' warning. * C4/Housebound.pm: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/houseboundedit.tt: new. * koha-tmpl/intranet-tmpl/prog/en/modules/members/houseboundinstances.tt: new. * members/housebound.pl: new. * members/houseboundedit.pl: new. * members/houseboundinstances.pl: new. * installer/data/mysql/atomicupdate/housebound_tables.sql: new. - create 'housebound', 'housebound_instance' tables, - create 'HB', 'VOL', 'DELIV', 'CHO' borrower categories, - create sample 'Day', 'Frequency' authorised values, - add 'useHouseboundModule', 'useHouseboundCheckPrevious' syspref. * t/Housebound.t: new. Test plan: - Apply patch. - Run atomic update script. - Optionally, run Unit Tests (t/Housebound.t) - Create at least 1 of each: 'HB', 'VOL', 'DELIV', 'CHO' borrower. - Optionally, add additional authorised values of 'Day' and 'Frequency' categories. - Verify 'useHouseboundModule' & 'useHouseboundCheckPrevious' sysprefs are on. - On the page of the 'HB' borrower, open the new 'Housebound' menu. - create housebound profile + ensure Day and Frequency values are from correct authorised values - create 'housebound instances' (deliveries) + ensure chooser/deliverer lists are populated with VOL, DELIV, CHO borrowers - edit the profile/instances. - delete the profile/instances. - For a borrower with a housebound profile, issue an item, then - return that item - attempt to re-issue that item (you should get a warning) - Switch off 'useHouseboundCheckPrevious', repeat last test - the warning should not appear - Switch off 'useHouseboundModule' - the Housebound menu should disappear - the issue warnings should no longer appear. Largely based on prior work from Mark Gavillet and Colin Campbell. New module to handle management of circulation to Housebound readers. - Ability to create housebound profiles & scheduled visits for patrons. - Ability to record users as Deliverers or Choosers (or both), using extended patron attributes. - Ability to link choosers and deliverers to individual delivery runs. - 'Delivery Frequencies' are customizable through authorised values ('Frequency' category respectively). * C4/Auth.pm: Add check: useHouseboundModule. * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * Koha/Patron.pm (housebound_profile): New method. * Koha/Patron.pm (housebound_choosers, housebound_deliverers): New methods. * Koha/Patron/Housebound.pm: New File. * Koha/Patron/HouseboundProfile.pm: New File. * Koha/Patron/HouseboundProfiles.pm: New File. * Koha/Patron/HouseboundVisits.pm: New File. * Koha/Patron/HouseboundVisit.pm: New File. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: New file. * members/housebound.pl: New file. * installer/data/mysql/kohastructure.sql: Add housebound_* tables. * installer/data/mysql/sysprefs.sql: Add HouseboundModule syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref: Add HouseboundModule syspref. * installer/data/mysql/atomicupdate/housebound_tables.sql: New file. * t/db_dependent/Patron/Housebound.t: New file. * t/db_dependent/Patron/HouseboundProfiles.t: New file. * t/db_dependent/Patron/HouseboundVisits.t: New file. Test plan: - Apply patch. - Run atomic update script. - Run Unit Tests (t/db_dependent/Patron/Housebound*) - Optionally, add additional authorised values of 'Frequency' categories. - Verify 'HouseboundModule', and 'ExtendedPatronAttributes sysprefs are on. - Ona patron page, the Housebound menu should have been added. - create housebound profile + ensure Frequency values are from correct authorised values. - create 'housebound visits' (deliveries) + ensure chooser/deliverer lists are populated with patrons that have the Chooser or Deliverer Attribute type (none by default: you will have to grant some users those attributes). - edit the profile/instances. - delete the instances. - Switch off 'HouseboundModule' - the Housebound menu should disappear DBIx::Class::Schema::source(): Can't find source for HouseboundProfile at /home/sandbox1/src/Koha/Objects.pm line 253 I got this error when clicking on the "Housebound" tab, in a new patron's profile. (testing in a sandbox) Comment on attachment 51377 [details] [review] Rebase on master Review of attachment 51377 [details] [review]: ----------------------------------------------------------------- ::: C4/Auth.pm @@ +435,4 @@ > ); > if ( $in->{'type'} eq "intranet" ) { > $template->param( > + useHouseboundModule => C4::Context->preference("useHouseboundModule"), There is no need to do this. The preferences are accessible in the template, see other templates which grab preferences already. ::: koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc @@ +105,4 @@ > [% IF CAN_user_borrowers && useDischarge %] > [% IF dischargeview %]<li class="active">[% ELSE %]<li>[% END %]<a href="/cgi-bin/koha/members/discharge.pl?borrowernumber=[% borrowernumber %]">Discharge</a></li> > [% END %] > + [% IF Koha.Preference('HouseboundModule') %] See, like here. ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt @@ +110,5 @@ > + <option value="friday">Friday</option> > + <option value="saturday">Saturday</option> > + <option value="sunday">Sunday</option> > + [% END %] > + </select> Couldn't all this select logic be simplified with a loop? @@ +122,5 @@ > + <option value="[% frequency.value %]" selected="selected">[% frequency.label %]</option> > + [% ELSE %] > + <option value="[% frequency.value %]">[% frequency.label %]</option> > + [% END %] > + [% END %] Like this. @@ +335,5 @@ > + [% ELSIF hpd == 'saturday' %] > + Saturday > + [% ELSIF hpd == 'sunday' %] > + Sunday > + [% END %] http://template-toolkit.org/docs/manual/VMethods.html#section_ucfirst ::: members/housebound.pl @@ +82,5 @@ > + fav_itemtypes => $input->param('fav_itemtypes'), > + fav_subjects => $input->param('fav_subjects'), > + fav_authors => $input->param('fav_authors'), > + referral => $input->param('referral'), > + notes => $input->param('notes'), Perhaps // q{} or something similar might be nice, just in case there's undefined parameters. ::: t/db_dependent/Patron/Housebound.t @@ +64,5 @@ > +foreach my $cho ( @{$found_choosers} ) { > + $cho_counter ++ > + if ( $cho->borrowernumber eq $patron_chooser->{borrowernumber} ); > +}; > +is(1, 1, "Return our patron_chooser!"); What is the point of this test? @@ +73,5 @@ > +foreach my $del ( @{$found_deliverers} ) { > + $del_counter ++ > + if ( $del->borrowernumber eq $patron_deliverer->{borrowernumber} ); > +}; > +is(1,1,"Return our patron_deliverer!"); What is the point of this test? Hello, Thank you for the review. A few comments and questions from my side, but will be implementing the rest of your suggestions. > ::: C4/Auth.pm > @@ +435,4 @@ >> ); >> if ( $in->{'type'} eq "intranet" ) { >> $template->param( >> + useHouseboundModule => C4::Context->preference("useHouseboundModule"), > There is no need to do this. The preferences are accessible in the template, see other templates which grab preferences already. Good point, will remove this. > ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt > @@ +110,5 @@ >> + <option value="friday">Friday</option> >> + <option value="saturday">Saturday</option> >> + <option value="sunday">Sunday</option> >> + [% END %] >> + </select> > Couldn't all this select logic be simplified with a loop? I'd be worried about translation — I'm not 100% sure how it works, but aren't simple strings in the UI the way to go for translation? > @@ +122,5 @@ >> + <option value="[% frequency.value %]" selected="selected">[% frequency.label %]</option> >> + [% ELSE %] >> + <option value="[% frequency.value %]">[% frequency.label %]</option> >> + [% END %] >> + [% END %] > Like this. I'm not concerned about translation here, as Frequencies are directly editables as authorized values by end users. > @@ +335,5 @@ >> + [% ELSIF hpd == 'saturday' %] >> + Saturday >> + [% ELSIF hpd == 'sunday' %] >> + Sunday >> + [% END %] > http://template-toolkit.org/docs/manual/VMethods.html#section_ucfirst Again, I would have concerns about translation here — but if this is not a concern for translation, then I'd be happy to implement that. > ::: members/housebound.pl > @@ +82,5 @@ >> + fav_itemtypes => $input->param('fav_itemtypes'), >> + fav_subjects => $input->param('fav_subjects'), >> + fav_authors => $input->param('fav_authors'), >> + referral => $input->param('referral'), >> + notes => $input->param('notes'), > Perhaps // q{} or something similar might be nice, just in case there's undefined parameters. Fair point, will implement :-) > ::: t/db_dependent/Patron/Housebound.t > @@ +64,5 @@ >> +foreach my $cho ( @{$found_choosers} ) { >> + $cho_counter ++ >> + if ( $cho->borrowernumber eq $patron_chooser->{borrowernumber} ); >> +}; >> +is(1, 1, "Return our patron_chooser!"); > What is the point of this test? This is testing that the Extended Borrower Atrributes work as expected in this context: - that our `housebound_choosers` method works as expected - that we, in this controlled environment, only return one chooser patron - and that chooser is the one that we expect it to be. > @@ +73,5 @@ >> +foreach my $del ( @{$found_deliverers} ) { >> + $del_counter ++ >> + if ( $del->borrowernumber eq $patron_deliverer->{borrowernumber} ); >> +}; >> +is(1,1,"Return our patron_deliverer!"); > What is the point of this test? Same as above, but for deliverers. WDYT? Alex Created attachment 51875 [details] [review] Housebound Schema Updates Apply this patch to no longer have to manually regenerate the Schema files. The 'Frequency' authorized value category does not exist, so I cannot add an entry there. Alex Created attachment 54384 [details] [review] Bug 5670: Housebound Readers Module New module to handle management of circulation to Housebound readers. - Ability to create housebound profiles & scheduled visits for patrons. - Ability to record users as Deliverers or Choosers (or both), using extended patron attributes. - Ability to link choosers and deliverers to individual delivery runs. - 'Delivery Frequencies' are customizable through authorised values ('HSBND_FREQ'). * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * Koha/Patron.pm (housebound_profile): New method. * Koha/Patrons.pm (housebound_choosers, housebound_deliverers): New methods. * Koha/Patron/HouseboundProfile.pm: New File. * Koha/Patron/HouseboundProfiles.pm: New File. * Koha/Patron/HouseboundVisits.pm: New File. * Koha/Patron/HouseboundVisit.pm: New File. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: New file. * members/housebound.pl: New file. * installer/data/mysql/kohastructure.sql: Add housebound_* tables. * installer/data/mysql/sysprefs.sql: Add HouseboundModule syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref: Add HouseboundModule syspref. * installer/data/mysql/atomicupdate/housebound_tables.sql: New file. * t/db_dependent/Patron/Borrower_Housebound.t: New file. * t/db_dependent/Patron/Borrower_HouseboundProfiles.t: New file. * t/db_dependent/Patron/Borrower_HouseboundVisits.t: New file. Test plan: - Apply patch. - Run atomic update script. - Run Unit Tests (t/db_dependent/Patron/Housebound*) - Optionally, add additional authorised values to 'HSBND_FREQ'. - Switch on 'HouseboundModule' syspref. - Ensure 'ExtendedPatronAttributes syspref is on. - On patron pages, when editing, add some to the Housebound deliverer and chooser groups by setting their extended attributes. - On a patron page, the Housebound menu should now be present. - create housebound profile + ensure Frequency values seem pulled from 'HSBND_FREQ'. - create 'housebound visits' (deliveries) + ensure chooser/deliverer lists are populated with patrons that have the Chooser or Deliverer Attribute type. - edit visits. - delete visits. - Switch off 'HouseboundModule' - the Housebound menu should disappear Created attachment 54385 [details] [review] DO NOT PUSH: Add Housebound Schema files. * Koha/Schema/Result/HouseboundProfile.pm: New file. * Koha/Schema/Result/HouseboundVisit.pm: New file. Testing this now, pleased to see you thought about translations and you are right, using the ucfirst and database values would make it untranslatable. So far so good, I'll keep testing a bit more then sign it off unless I find something off Created attachment 55326 [details] [review] Bug 5670: Housebound Readers Module New module to handle management of circulation to Housebound readers. - Ability to create housebound profiles & scheduled visits for patrons. - Ability to record users as Deliverers or Choosers (or both), using extended patron attributes. - Ability to link choosers and deliverers to individual delivery runs. - 'Delivery Frequencies' are customizable through authorised values ('HSBND_FREQ'). * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * Koha/Patron.pm (housebound_profile): New method. * Koha/Patrons.pm (housebound_choosers, housebound_deliverers): New methods. * Koha/Patron/HouseboundProfile.pm: New File. * Koha/Patron/HouseboundProfiles.pm: New File. * Koha/Patron/HouseboundVisits.pm: New File. * Koha/Patron/HouseboundVisit.pm: New File. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: New file. * members/housebound.pl: New file. * installer/data/mysql/kohastructure.sql: Add housebound_* tables. * installer/data/mysql/sysprefs.sql: Add HouseboundModule syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref: Add HouseboundModule syspref. * installer/data/mysql/atomicupdate/housebound_tables.sql: New file. * t/db_dependent/Patron/Borrower_Housebound.t: New file. * t/db_dependent/Patron/Borrower_HouseboundProfiles.t: New file. * t/db_dependent/Patron/Borrower_HouseboundVisits.t: New file. Test plan: - Apply patch. - Run atomic update script. - Run Unit Tests (t/db_dependent/Patron/Housebound*) - Optionally, add additional authorised values to 'HSBND_FREQ'. - Switch on 'HouseboundModule' syspref. - Ensure 'ExtendedPatronAttributes syspref is on. - On patron pages, when editing, add some to the Housebound deliverer and chooser groups by setting their extended attributes. - On a patron page, the Housebound menu should now be present. - create housebound profile + ensure Frequency values seem pulled from 'HSBND_FREQ'. - create 'housebound visits' (deliveries) + ensure chooser/deliverer lists are populated with patrons that have the Chooser or Deliverer Attribute type. - edit visits. - delete visits. - Switch off 'HouseboundModule' - the Housebound menu should disappear Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Cheers for looking at this Chris — glad to see it still applies :-) Koha::Patron::HouseboundVisits->special_search is not used anywhere in the code, should be removed? 50 sub chooser { 51 my ( $self ) = @_; 52 return $self->_result->chooser_brwnumber; 53 } should be sub chooser { my ( $self ) = @_ my $rs = $self->_result->chooser_brwnumber; return Koha::Patron->_new_from_dbic( $rs ); } See bug 17094. And then you can remove the TODO in the tests :) Hi Jonathan, I think it is used in HouseboundProfile.pm->housebound_visits. I'll join IRC now for easier comms if you have any questions. Alex members/housebound.pl should not die but display an error message instead :) t/db_dependent/Patron/Borrower_*.t should be moved to /db_dependent/Patron/*.t I'd say (In reply to Alex Sassmannshausen from comment #38) > Hi Jonathan, > > I think it is used in HouseboundProfile.pm->housebound_visits. Yes it is! I have misread the git grep output. Created attachment 55334 [details] [review] Bug 5670: [Followup] Rename test files. * t/db_dependent/Patron/Borrower_Housebound.t: Rename to t/db_dependent/Patron/Housebound.t. * t/db_dependent/Patron/Borrower_HouseboundProfiles.t: Rename to t/db_dependent/Patron/HouseboundProfiles.t. * t/db_dependent/Patron/Borrower_HouseboundVisits.t: Rename to t/db_dependent/Patron/HouseboundVisits.t. Created attachment 55335 [details] [review] Bug 5670: [Followup] Return Koha::Patron objects. * Koha/Patron/HouseboundVisit.pm (chooser, deliverer): Use `_new_from_dbic` to return Koha::Patron objects. * t/db_dependent/Patron/HouseboundVisits.t: Remove TODO section for this. Created attachment 55336 [details] [review] Bug 5670: [Followup] Refactor .pl; error messages. * members/housebound.pl: Refactor & store messages for encountered errors. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Show messages. I am not able to test: - Go on members/housebound.pl?borrowernumber=42 - in the logs: No method housebound_profile! at /home/koha/src/members/housebound.pl line 66. - Fill the form (are some fields mandatory?) - Edit the form => fields are blank (In reply to Jonathan Druart from comment #44) > I am not able to test: > - Go on members/housebound.pl?borrowernumber=42 > - in the logs: > No method housebound_profile! at /home/koha/src/members/housebound.pl line > 66. > - Fill the form (are some fields mandatory?) > - Edit the form => fields are blank Sorry about that, plack was not restarted correctly. QA comments: 1/ The date field should have the datepicker class, you should not force the dateformat to yyyy-mm-dd 2/ from the logs: No method category_type! at /home/koha/src/koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt line 294. The borrowers table does not have a category_type column 3/ When editing a patron, the "Housebound" tab does not appear 4/ From the logs again: CGI::param called in list context from /home/koha/src/members/housebound.pl line 56, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 407. 5/ FAIL koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt FAIL forbidden patterns forbidden pattern: console.log (line 208) forbidden pattern: console.log (line 329) FAIL members/housebound.pl FAIL critic "$deliverer" is declared but not used at line 78, column 1. Unused variables clutter code and make it harder to read. "$chooser" is declared but not used at line 78, column 1. Unused variables clutter code and make it harder to read. "$houseboundvisits" is declared but not used at line 77, column 1. Unused variables clutter code and make it harder to 6/ Compare the div.patroninfo with another page from the members module (members/moremember.pl?borrowernumber=42 for instance), lot of infos are missing. Created attachment 55734 [details] [review] Bug 5670: [QA Followup] Don't force ISO dates. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Use ".datepicker" class for date input, use `$KohaDates` to filter our iso dates. * members/housebound.pl: Use DateTime and Koha::DateUtils to parse incoming date to ISO format. Created attachment 55735 [details] [review] Bug 5670: [QA Followup] Fix category_type ref. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Replace references to `category_type` with `categorycode`. Created attachment 55736 [details] [review] Bug 5670: [QA Followup] Avoid CGI::param in list context. * members/housebound.pl: Force `borrowernumber` to scalar context. Created attachment 55737 [details] [review] Bug 5670: [QA Followup] Housebound link from patron edit. * koha-tmpl/intranet-tmpl/prog/en/includes/members-menu.inc: Add a link to the housebound module view if it is switched on. Created attachment 55738 [details] [review] Bug 5670: [QA Followup] Small QA fixes. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Remove `console.log` invocations. * members/housebound.pl ($houseboundvisits, $chooser, $deliverer): Remove unused variables. Created attachment 55739 [details] [review] Bug 5670: [QA Followup] Fix `div.patroninfo`. * members/housebound.pl: Provide full patron hash keys, extended patron attributes & picture to template. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Refactor to use new information. Hi Jonathan, Thank you very much for the quick review! I believe I have now addressed all the issues you raised, with one commit per issue. I hope this looks good to you. Best wishes, Alex Comment on attachment 55734 [details] [review] Bug 5670: [QA Followup] Don't force ISO dates. Review of attachment 55734 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt @@ +208,4 @@ > <!-- Create or edit housebound_visit --> > [% ELSIF ( method == 'visit_update_or_create' ) %] > <h4>Manage housebound deliveries</h4> > + <form name="form" id="instance_form" method="post" class="toggler" This 'toggler' class does not seem useful, could you confirm? Created attachment 55865 [details] [review] Bug 5670: Add the 'required' class to required fields Created attachment 55866 [details] [review] Bug 5670: mv housebound_* to search_housebound_* I think it's better to prefix the subroutine names used to search with 'search_'. Created attachment 55867 [details] [review] Bug 5670: remove useless call to ->new when ->search is enought Alex, Sorry, not ready yet. 1/ see comment 54 2/ see last 3 patches, feel free to disagree. 3/ please adapt the Edit/Delete/Add a new delivery buttons to use the bootstrap buttons (you can contact Owen if you need help) 4/ Some thinkings (not blocker): I think I would have preferred not to see this feature using the patron attributes. Specific columns in the borrowers table may have been better (?). What could be the different values of HSBND_FREQ? It seems that we could have avoid it if the user had the ability to select several days. But I may be missing something here. Same for HSBND_ROLE, it seems that another value is not taken into account completely. It appears when editing a patron, but not on the housebound details. What is the use case exactly? Created attachment 55890 [details] [review] Bug 5670: [QA Followup] Improve template style. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Remove toggler class from form, improve look and layout of action buttons. Created attachment 55891 [details] [review] Bug 5670: [QA Followup] More "required" messages. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Mark houseboundvisit fields as required. (In reply to Jonathan Druart from comment #58) > Alex, > > Sorry, not ready yet. > > 1/ see comment 54 This should be fixed now. > 2/ see last 3 patches, feel free to disagree. Yeah, I like the patches — also added some more "required" markup to the houseboundvisit page now. Looks a bit naff, but at least it's clear. > 3/ please adapt the Edit/Delete/Add a new delivery buttons to use the > bootstrap buttons (you can contact Owen if you need help) This is done. > 4/ Some thinkings (not blocker): > I think I would have preferred not to see this feature using the patron > attributes. Specific columns in the borrowers table may have been better (?). > What could be the different values of HSBND_FREQ? It seems that we could > have avoid it if the user had the ability to select several days. But I may > be missing something here. Right, these are interesting questions. Let's talk about HSBND_ROLE first. I guess they could be implemented as a separate table or as extensions to the borrowers table. I don't consider either approach superior in this context though. The approach I have taken has the great benefit that it is working now — and I'm not sure how much time I can still take to put into this feature. So I'd be tempted to keep it as is, unless there is a compelling case to change it. HSBND_FREQ is an authorised value because it will allow customers to specify "distribution rotas" for their needs. The field basically states, how often does a visit for this profile take place? This could take the shape of, for instance, once every week, once every month, every other week, etc. I.e. this is metadata about with what regularity a profile is acted upon. Individual visits are then organised according to that metadata. The individual dates associated with a visit should normally fall within the pattern given by the HSBND_FREQ selected. > Same for HSBND_ROLE, it seems that another value is not taken into account > completely. It appears when editing a patron, but not on the housebound > details. What is the use case exactly? You should find that HSBND_ROLE appears when creating an individual housebound_visit. Any patrons tagged as chooser or deliverer will then appear as values which can be selected in the chooser and deliverer dropdowns in the edit/create visit screen. HTH! Alex (In reply to Alex Sassmannshausen from comment #61) > > Same for HSBND_ROLE, it seems that another value is not taken into account > > completely. It appears when editing a patron, but not on the housebound > > details. What is the use case exactly? > > You should find that HSBND_ROLE appears when creating an individual > housebound_visit. Any patrons tagged as chooser or deliverer will then > appear as values which can be selected in the chooser and deliverer > dropdowns in the edit/create visit screen. Ok but what could be a third value? What could be a third value and how it is supposed to behave? (In reply to Jonathan Druart from comment #62) > (In reply to Alex Sassmannshausen from comment #61) > > > Same for HSBND_ROLE, it seems that another value is not taken into account > > > completely. It appears when editing a patron, but not on the housebound > > > details. What is the use case exactly? > > > > You should find that HSBND_ROLE appears when creating an individual > > housebound_visit. Any patrons tagged as chooser or deliverer will then > > appear as values which can be selected in the chooser and deliverer > > dropdowns in the edit/create visit screen. > > Ok but what could be a third value? What could be a third value and how it > is supposed to behave? Thinking about this some more last night, it feels like the current design is not appropriate. You're quite right asking whether there could be third values, and the answer is there cannot. The UI expects there to be only two values. My alternative proposal now would be: - create a new custom table "housebound_role" - with three columns (borrower_id, deliverer, chooser) - remove the current dependency on extended patron attributes - remove HSBND_ROLE authorised values So within this approach, I reduce complexity (no extended attributes, fewer authorised values), and confirm that housebound roles are not supposed to be extensible. WDYT? (In reply to Alex Sassmannshausen from comment #63) > (In reply to Jonathan Druart from comment #62) > > (In reply to Alex Sassmannshausen from comment #61) > > > > Same for HSBND_ROLE, it seems that another value is not taken into account > > > > completely. It appears when editing a patron, but not on the housebound > > > > details. What is the use case exactly? > > > > > > You should find that HSBND_ROLE appears when creating an individual > > > housebound_visit. Any patrons tagged as chooser or deliverer will then > > > appear as values which can be selected in the chooser and deliverer > > > dropdowns in the edit/create visit screen. > > > > Ok but what could be a third value? What could be a third value and how it > > is supposed to behave? > > Thinking about this some more last night, it feels like the current design > is not appropriate. You're quite right asking whether there could be third > values, and the answer is there cannot. The UI expects there to be only two > values. > > My alternative proposal now would be: > - create a new custom table "housebound_role" > - with three columns (borrower_id, deliverer, chooser) > - remove the current dependency on extended patron attributes > - remove HSBND_ROLE authorised values > > So within this approach, I reduce complexity (no extended attributes, fewer > authorised values), and confirm that housebound roles are not supposed to be > extensible. > > WDYT? Yes, I think that it makes more sense and will be less confusing for developers and users. (In reply to Jonathan Druart from comment #64) > > My alternative proposal now would be: > > - create a new custom table "housebound_role" > > - with three columns (borrower_id, deliverer, chooser) The PK for this new table would be borrower_id, so should not it be 2 new columns in the borrowers table instead? (In reply to Jonathan Druart from comment #65) > (In reply to Jonathan Druart from comment #64) > > > My alternative proposal now would be: > > > - create a new custom table "housebound_role" > > > - with three columns (borrower_id, deliverer, chooser) > > The PK for this new table would be borrower_id, so should not it be 2 new > columns in the borrowers table instead? I considered that, but proposed a new table because: - the borrowers table is quite large as it is. Adding columns to it would make it slightly less discoverable - the vast majority of borrowers in the system will have neither a "deliverer" nor a "chooser" role, so those columns will just be empty/false in the borrowers table (this is particularly the case if the housebound module is switched off!). - in the housebound_role table the columns will only exist if the borrower is in fact a volunteer of some form (either chooser or deliverer), so each entry will be "doing something". Conversely, joins are quite cheap, so I think I prefer this design. (In reply to Alex Sassmannshausen from comment #66) > (In reply to Jonathan Druart from comment #65) > > (In reply to Jonathan Druart from comment #64) > > > > My alternative proposal now would be: > > > > - create a new custom table "housebound_role" > > > > - with three columns (borrower_id, deliverer, chooser) > > > > The PK for this new table would be borrower_id, so should not it be 2 new > > columns in the borrowers table instead? > > I considered that, but proposed a new table because: > - the borrowers table is quite large as it is. Adding columns to it would > make it slightly less discoverable > - the vast majority of borrowers in the system will have neither a > "deliverer" nor a "chooser" role, so those columns will just be empty/false > in the borrowers table (this is particularly the case if the housebound > module is switched off!). > - in the housebound_role table the columns will only exist if the borrower > is in fact a volunteer of some form (either chooser or deliverer), so each > entry will be "doing something". > > Conversely, joins are quite cheap, so I think I prefer this design. My concern is more about the DB normalization, I think we need to keep that in mind and normalize it as much as possible to keep data integrity. I'd like to get more opinions before you start. Wondering, could be chooser and deliverer be the same person? oh, I might have misread - chooser and deliverer would also each link to a borrowernumber. (In reply to Katrin Fischer from comment #69) > oh, I might have misread - chooser and deliverer would also each link to a > borrowernumber. Yeah, either way, a borrower could be either chooser or deliverer or both. That should be possible with either a separate table for the module, or an extension of the borrowers table. (In reply to Jonathan Druart from comment #67) > (In reply to Alex Sassmannshausen from comment #66) > > (In reply to Jonathan Druart from comment #65) > > > (In reply to Jonathan Druart from comment #64) > > > > > My alternative proposal now would be: > > > > > - create a new custom table "housebound_role" > > > > > - with three columns (borrower_id, deliverer, chooser) > > > > > > The PK for this new table would be borrower_id, so should not it be 2 new > > > columns in the borrowers table instead? > > > > I considered that, but proposed a new table because: > > - the borrowers table is quite large as it is. Adding columns to it would > > make it slightly less discoverable > > - the vast majority of borrowers in the system will have neither a > > "deliverer" nor a "chooser" role, so those columns will just be empty/false > > in the borrowers table (this is particularly the case if the housebound > > module is switched off!). > > - in the housebound_role table the columns will only exist if the borrower > > is in fact a volunteer of some form (either chooser or deliverer), so each > > entry will be "doing something". > > > > Conversely, joins are quite cheap, so I think I prefer this design. > > My concern is more about the DB normalization, I think we need to keep that > in mind and normalize it as much as possible to keep data integrity. > I'd like to get more opinions before you start. Fair enough, I've opened a thread on Koha-Dev. Let's see where that goes. If worst comes to worst we might be able to hammer it out at the hackfest. Created attachment 56298 [details] [review] Bug 5670: [QA Followup] Abandon Extended Patron Attr. This patch series abandons the use of Extended Patron Attributes in the HouseboundModule. Instead we provide a separate table with a UI from borrower edit/create. Created attachment 56299 [details] [review] Bug 5670: [QA Followup] Create HouseboundRole objects. * Koha/Patron/HouseboundRole.pm: New file. * Koha/Patron/HouseboundRoles.pm: New file. Created attachment 56300 [details] [review] Bug 5670: [QA Followup] Correct housebound role search. * Koha/Patrons.pm (search_housebound_choosers) (search_housebound_deliverers): Use new table. Created attachment 56301 [details] [review] Bug 5670: [QA Followup] Display HouseboundRole info. * Koha/Patron.pm (housebound_role): New method. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Add section for HouseboundRole information * members/moremember.pl: Pass HouseboundRole info to template. Created attachment 56302 [details] [review] Bug 5670: [QA Followup] HouseboundRole CRUD from UI. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Add HouseboundRole form. * members/memberentry.pl: Handle HouseboundRole CRUD. The migration to using a new table, housebound_role, is now complete. Thanks for all your work on this so far, hopefully we now have a pretty solid feature! Alex Created attachment 56308 [details] [review] DO NOT PUSH: Moar Housebound Schema files. * Koha/Schema/Result/Borrower.pm: New method. * Koha/Schema/Result/HouseboundRole.pm: New file. New Test Plan: Test plan: - Apply patches (DO NOT PUSH contain the schema updates). - Run atomic update script. - Run Unit Tests (t/db_dependent/Patron/Housebound*) - Optionally, add additional authorised values of 'Frequency' categories. - Enable HouseboundModule module. - On a patron page, you should see details of housebound roles - When editing or creating a patron you can set the housebound roles. - On a patron page, the Housebound menu should have been added. - create housebound profile + ensure Frequency values are from correct authorised values. - create 'housebound visits' (deliveries) + ensure chooser/deliverer lists are populated with patrons that have the Chooser or Deliverer Attribute type (none by default: you will have to grant some users those attributes). - edit the profile/instances. - delete the instances. - Switch off 'HouseboundModule' - the Housebound menu should disappear Created attachment 56337 [details] [review] Bug 5670: Add auth_val_cat to auth_val_cat.sql. * installer/data/mysql/atomicupdate/housebound_tables.sql: Insert HSBND_FREQ in authorised value categories. * installer/data/mysql/mandatory/auth_val_cat.sql: Add HSBND_FREQ. Created attachment 56341 [details] [review] Bug 5670: [Followup] Add 'step 4' edit button. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Place housebound role form inside the 'step 4' section. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Add 'step 4' edit button for housebound role. Created attachment 56346 [details] [review] DO NOT PUSH: Add Housebound Schema files. * Koha/Schema/Result/HouseboundProfile.pm: New file. * Koha/Schema/Result/HouseboundVisit.pm: New file. https://bugs.koha-community.org/show_bug.cgi?id=5670 Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56347 [details] [review] Bug 5670: Housebound Readers Module New module to handle management of circulation to Housebound readers. - Ability to create housebound profiles & scheduled visits for patrons. - Ability to record users as Deliverers or Choosers (or both), using extended patron attributes. - Ability to link choosers and deliverers to individual delivery runs. - 'Delivery Frequencies' are customizable through authorised values ('HSBND_FREQ'). * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * Koha/Patron.pm (housebound_profile): New method. * Koha/Patrons.pm (housebound_choosers, housebound_deliverers): New methods. * Koha/Patron/HouseboundProfile.pm: New File. * Koha/Patron/HouseboundProfiles.pm: New File. * Koha/Patron/HouseboundVisits.pm: New File. * Koha/Patron/HouseboundVisit.pm: New File. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: New file. * members/housebound.pl: New file. * installer/data/mysql/kohastructure.sql: Add housebound_* tables. * installer/data/mysql/sysprefs.sql: Add HouseboundModule syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref: Add HouseboundModule syspref. * installer/data/mysql/atomicupdate/housebound_tables.sql: New file. * t/db_dependent/Patron/Borrower_Housebound.t: New file. * t/db_dependent/Patron/Borrower_HouseboundProfiles.t: New file. * t/db_dependent/Patron/Borrower_HouseboundVisits.t: New file. Test plan: - Apply patch. - Run atomic update script. - Run Unit Tests (t/db_dependent/Patron/Housebound*) - Optionally, add additional authorised values to 'HSBND_FREQ'. - Switch on 'HouseboundModule' syspref. - Ensure 'ExtendedPatronAttributes syspref is on. - On patron pages, when editing, add some to the Housebound deliverer and chooser groups by setting their extended attributes. - On a patron page, the Housebound menu should now be present. - create housebound profile + ensure Frequency values seem pulled from 'HSBND_FREQ'. - create 'housebound visits' (deliveries) + ensure chooser/deliverer lists are populated with patrons that have the Chooser or Deliverer Attribute type. - edit visits. - delete visits. - Switch off 'HouseboundModule' - the Housebound menu should disappear Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56348 [details] [review] Bug 5670: [Followup] Rename test files. * t/db_dependent/Patron/Borrower_Housebound.t: Rename to t/db_dependent/Patron/Housebound.t. * t/db_dependent/Patron/Borrower_HouseboundProfiles.t: Rename to t/db_dependent/Patron/HouseboundProfiles.t. * t/db_dependent/Patron/Borrower_HouseboundVisits.t: Rename to t/db_dependent/Patron/HouseboundVisits.t. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56349 [details] [review] Bug 5670: [Followup] Return Koha::Patron objects. * Koha/Patron/HouseboundVisit.pm (chooser, deliverer): Use `_new_from_dbic` to return Koha::Patron objects. * t/db_dependent/Patron/HouseboundVisits.t: Remove TODO section for this. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56350 [details] [review] Bug 5670: [Followup] Refactor .pl; error messages. * members/housebound.pl: Refactor & store messages for encountered errors. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Show messages. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56351 [details] [review] Bug 5670: [QA Followup] Don't force ISO dates. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Use ".datepicker" class for date input, use `$KohaDates` to filter our iso dates. * members/housebound.pl: Use DateTime and Koha::DateUtils to parse incoming date to ISO format. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56352 [details] [review] Bug 5670: [QA Followup] Fix category_type ref. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Replace references to `category_type` with `categorycode`. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56353 [details] [review] Bug 5670: [QA Followup] Avoid CGI::param in list context. * members/housebound.pl: Force `borrowernumber` to scalar context. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56354 [details] [review] Bug 5670: [QA Followup] Housebound link from patron edit. * koha-tmpl/intranet-tmpl/prog/en/includes/members-menu.inc: Add a link to the housebound module view if it is switched on. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56355 [details] [review] Bug 5670: [QA Followup] Small QA fixes. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Remove `console.log` invocations. * members/housebound.pl ($houseboundvisits, $chooser, $deliverer): Remove unused variables. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56356 [details] [review] Bug 5670: [QA Followup] Fix `div.patroninfo`. * members/housebound.pl: Provide full patron hash keys, extended patron attributes & picture to template. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Refactor to use new information. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56357 [details] [review] Bug 5670: Add the 'required' class to required fields Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56358 [details] [review] Bug 5670: mv housebound_* to search_housebound_* I think it's better to prefix the subroutine names used to search with 'search_'. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56359 [details] [review] Bug 5670: remove useless call to ->new when ->search is enought Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56360 [details] [review] Bug 5670: [QA Followup] Improve template style. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Remove toggler class from form, improve look and layout of action buttons. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56361 [details] [review] Bug 5670: [QA Followup] More "required" messages. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Mark houseboundvisit fields as required. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56362 [details] [review] Bug 5670: [QA Followup] Create housebound roles table. * installer/data/mysql/atomicupdate/housebound_tables.sql (housebound_role): New table. * installer/data/mysql/kohastructure.sql (housebound_role): New table. * installer/data/mysql/en/mandatory/auth_values.sql: Remove HSBND_ROLE entries. * installer/data/mysql/en/mandatory/patron_attributes.sql: Remove HSBND_ROLE entry. * installer/data/mysql/en/mandatory/patron_attributes.txt: Remove explanatory text. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56363 [details] [review] Bug 5670: [QA Followup] Create HouseboundRole objects. * Koha/Patron/HouseboundRole.pm: New file. * Koha/Patron/HouseboundRoles.pm: New file. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56364 [details] [review] Bug 5670: [QA Followup] Correct housebound role search. * Koha/Patrons.pm (search_housebound_choosers) (search_housebound_deliverers): Use new table. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56365 [details] [review] Bug 5670: [QA Followup] Display HouseboundRole info. * Koha/Patron.pm (housebound_role): New method. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Add section for HouseboundRole information * members/moremember.pl: Pass HouseboundRole info to template. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56366 [details] [review] Bug 5670: [QA Followup] HouseboundRole CRUD from UI. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Add HouseboundRole form. * members/memberentry.pl: Handle HouseboundRole CRUD. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56367 [details] [review] DO NOT PUSH: Moar Housebound Schema files. * Koha/Schema/Result/Borrower.pm: New method. * Koha/Schema/Result/HouseboundRole.pm: New file. https://bugs.koha-community.org/show_bug.cgi?id=5670 Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56368 [details] [review] Bug 5670: Add auth_val_cat to auth_val_cat.sql. * installer/data/mysql/atomicupdate/housebound_tables.sql: Insert HSBND_FREQ in authorised value categories. * installer/data/mysql/mandatory/auth_val_cat.sql: Add HSBND_FREQ. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56369 [details] [review] Bug 5670: [Followup] Add 'step 4' edit button. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Place housebound role form inside the 'step 4' section. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Add 'step 4' edit button for housebound role. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56580 [details] [review] DO NOT PUSH: Add Housebound Schema files. * Koha/Schema/Result/HouseboundProfile.pm: New file. * Koha/Schema/Result/HouseboundVisit.pm: New file. https://bugs.koha-community.org/show_bug.cgi?id=5670 Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56581 [details] [review] Bug 5670: Housebound Readers Module New module to handle management of circulation to Housebound readers. - Ability to create housebound profiles & scheduled visits for patrons. - Ability to record users as Deliverers or Choosers (or both), using extended patron attributes. - Ability to link choosers and deliverers to individual delivery runs. - 'Delivery Frequencies' are customizable through authorised values ('HSBND_FREQ'). * koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc: add Housebound menu if appropriate. * Koha/Patron.pm (housebound_profile): New method. * Koha/Patrons.pm (housebound_choosers, housebound_deliverers): New methods. * Koha/Patron/HouseboundProfile.pm: New File. * Koha/Patron/HouseboundProfiles.pm: New File. * Koha/Patron/HouseboundVisits.pm: New File. * Koha/Patron/HouseboundVisit.pm: New File. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: New file. * members/housebound.pl: New file. * installer/data/mysql/kohastructure.sql: Add housebound_* tables. * installer/data/mysql/sysprefs.sql: Add HouseboundModule syspref. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref: Add HouseboundModule syspref. * installer/data/mysql/atomicupdate/housebound_tables.sql: New file. * t/db_dependent/Patron/Borrower_Housebound.t: New file. * t/db_dependent/Patron/Borrower_HouseboundProfiles.t: New file. * t/db_dependent/Patron/Borrower_HouseboundVisits.t: New file. Test plan: - Apply patch. - Run atomic update script. - Run Unit Tests (t/db_dependent/Patron/Housebound*) - Optionally, add additional authorised values to 'HSBND_FREQ'. - Switch on 'HouseboundModule' syspref. - Ensure 'ExtendedPatronAttributes syspref is on. - On patron pages, when editing, add some to the Housebound deliverer and chooser groups. - On a patron page, the Housebound menu should now be present. - create housebound profile + ensure Frequency values seem pulled from 'HSBND_FREQ'. - create 'housebound visits' (deliveries) + ensure chooser/deliverer lists are populated with patrons that have the Chooser or Deliverer Attribute type. - edit visits. - delete visits. - Switch off 'HouseboundModule' - the Housebound menu should disappear Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Bug 5670: [Followup] Rename test files. * t/db_dependent/Patron/Borrower_Housebound.t: Rename to t/db_dependent/Patron/Housebound.t. * t/db_dependent/Patron/Borrower_HouseboundProfiles.t: Rename to t/db_dependent/Patron/HouseboundProfiles.t. * t/db_dependent/Patron/Borrower_HouseboundVisits.t: Rename to t/db_dependent/Patron/HouseboundVisits.t. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Bug 5670: [QA Followup] Fix category_type ref. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Replace references to `category_type` with `categorycode`. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56582 [details] [review] Bug 5670: [Followup] Return Koha::Patron objects. * Koha/Patron/HouseboundVisit.pm (chooser, deliverer): Use `_new_from_dbic` to return Koha::Patron objects. * t/db_dependent/Patron/HouseboundVisits.t: Remove TODO section for this. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56583 [details] [review] Bug 5670: [Followup] Refactor .pl; error messages. * members/housebound.pl: Refactor & store messages for encountered errors. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Show messages. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56584 [details] [review] Bug 5670: [QA Followup] Don't force ISO dates. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Use ".datepicker" class for date input, use `$KohaDates` to filter our iso dates. * members/housebound.pl: Use DateTime and Koha::DateUtils to parse incoming date to ISO format. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56585 [details] [review] Bug 5670: [QA Followup] Avoid CGI::param in list context. * members/housebound.pl: Force `borrowernumber` to scalar context. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56586 [details] [review] Bug 5670: [QA Followup] Housebound link from patron edit. * koha-tmpl/intranet-tmpl/prog/en/includes/members-menu.inc: Add a link to the housebound module view if it is switched on. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56587 [details] [review] Bug 5670: [QA Followup] Small QA fixes. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Remove `console.log` invocations. * members/housebound.pl ($houseboundvisits, $chooser, $deliverer): Remove unused variables. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56588 [details] [review] Bug 5670: [QA Followup] Fix `div.patroninfo`. * members/housebound.pl: Provide full patron hash keys, extended patron attributes & picture to template. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Refactor to use new information. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56589 [details] [review] Bug 5670: Add the 'required' class to required fields Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56590 [details] [review] Bug 5670: mv housebound_* to search_housebound_* I think it's better to prefix the subroutine names used to search with 'search_'. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56591 [details] [review] Bug 5670: remove useless call to ->new when ->search is enought Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56592 [details] [review] Bug 5670: [QA Followup] Improve template style. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Remove toggler class from form, improve look and layout of action buttons. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56593 [details] [review] Bug 5670: [QA Followup] More "required" messages. * koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt: Mark houseboundvisit fields as required. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56594 [details] [review] Bug 5670: [QA Followup] Create housebound roles table. * installer/data/mysql/atomicupdate/housebound_tables.sql (housebound_role): New table. * installer/data/mysql/kohastructure.sql (housebound_role): New table. * installer/data/mysql/en/mandatory/auth_values.sql: Remove HSBND_ROLE entries. * installer/data/mysql/en/mandatory/patron_attributes.sql: Remove HSBND_ROLE entry. * installer/data/mysql/en/mandatory/patron_attributes.txt: Remove explanatory text. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56595 [details] [review] Bug 5670: [QA Followup] Create HouseboundRole objects. * Koha/Patron/HouseboundRole.pm: New file. * Koha/Patron/HouseboundRoles.pm: New file. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56596 [details] [review] Bug 5670: [QA Followup] Correct housebound role search. * Koha/Patrons.pm (search_housebound_choosers) (search_housebound_deliverers): Use new table. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56597 [details] [review] Bug 5670: [QA Followup] Display HouseboundRole info. * Koha/Patron.pm (housebound_role): New method. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Add section for HouseboundRole information * members/moremember.pl: Pass HouseboundRole info to template. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56598 [details] [review] Bug 5670: [QA Followup] HouseboundRole CRUD from UI. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Add HouseboundRole form. * members/memberentry.pl: Handle HouseboundRole CRUD. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56599 [details] [review] DO NOT PUSH: Moar Housebound Schema files. * Koha/Schema/Result/Borrower.pm: New method. * Koha/Schema/Result/HouseboundRole.pm: New file. https://bugs.koha-community.org/show_bug.cgi?id=5670 Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56600 [details] [review] Bug 5670: Add auth_val_cat to auth_val_cat.sql. * installer/data/mysql/atomicupdate/housebound_tables.sql: Insert HSBND_FREQ in authorised value categories. * installer/data/mysql/mandatory/auth_val_cat.sql: Add HSBND_FREQ. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56601 [details] [review] Bug 5670: [Followup] Add 'step 4' edit button. * koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt: Place housebound role form inside the 'step 4' section. * koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt: Add 'step 4' edit button for housebound role. Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> Created attachment 56602 [details] [review] Bug 5670: Update constraint name to match with the one in updatedatabase Created attachment 56603 [details] [review] Bug 5670: Does not return 0 in Koha::Patron->housebound_* methods Created attachment 56604 [details] [review] Bug 5670: Use Koha.Preference to retrieve syspref from templates Created attachment 56605 [details] [review] Bug 5670: Use the fa icon for the save button Created attachment 56606 [details] [review] Bug 5670: Add a confirmation message on delete Great work Alex, thanks for your perseverance! :) I have squashed 2 or 3 patches for the readability (add and deletion of the same files). Marked as Passed QA (In reply to Jonathan Druart from comment #133) > Great work Alex, thanks for your perseverance! :) Indeed, thanks to you and your perseverance too! :-) > Marked as Passed QA Great news! Pushed to master for 16.11, thanks Alex, Jonathan! OK, guys, I've a functional question: * I'm a chooser = how do I know that today (or tomorrow, or next weeks) I'll have to do some choices ? * I'm a deliverer = same question : how do I know I've to deliver ? * I want to take profit of this service, can I register from the OPAC ? Thank you for any answer/feedback ;-) PS : sorry Claire, I know you're not a guy ;-) (In reply to Paul Poulain from comment #136) > OK, guys, I've a functional question: > * I'm a chooser = how do I know that today (or tomorrow, or next weeks) > I'll have to do some choices ? > * I'm a deliverer = same question : how do I know I've to deliver ? The information is only displayed on the patron detail pages at the intranet (show+edit views) and on the housebound page (tab HouseBound in the member module). Nothing at the OPAC. > * I want to take profit of this service, can I register from the OPAC ? Nope :) thx for your quick answer joubu. Then, in my opinion, it lowers a lot the interest of the feature. Anyone around planning/thinking of adding this kind of feature (a kind of "housebound dashboard") Yep, IIRC the goal with this first patch was to get a first shoot in. As you can see with the quantity of patches and the low number of this bug report, it was a long standing one. AFAIK there is nothing planned yet to improve this feature. But Alex must know better than me: ) Hello Paul, Jonathan, Jonathan mostly covered the facts in his response. To confirm: (In reply to Paul Poulain from comment #136) > OK, guys, I've a functional question: > * I'm a chooser = how do I know that today (or tomorrow, or next weeks) > I'll have to do some choices ? > * I'm a deliverer = same question : how do I know I've to deliver ? The primary workflow here is that choosers and deliverers work in close concert with the library anyway: they have a weekly day, where they all get together and work through their respective tasks for instance. I like what you're proposing, which is, by the sound of it, a decentralised version of the above. > * I want to take profit of this service, can I register from the OPAC ? Again, currently this is managed by staff at the branch, or even indirectly, by the library service as a whole. It would be nice to allow people to sign up to it. At present we don't have specific time-scales to further develop those features — but the architecture certainly should make the above options easy to implement, and I'm happy to provide guidance to anyone seeking to do so. HTH, Alex |
Created attachment 3038 [details] Initial Spec New module to handle management of circulation to Housebound readers. Ability to record users as housebound, as volunteer delivers and choosers. Link users to individual delivery runs. To aid choosers add a check on issuing to housebound users flagging if title has previously been issued to this user. Work In Progress can be seen on the PTFS-Europe github repository: git://github.com/PTFS-Europe/koha.git Sponsered by Halton Borough Council