Bug 5670 - Housebound Readers Module
Summary: Housebound Readers Module
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Alex Sassmannshausen
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords:
Depends on: 17216
Blocks: 17576 17891
  Show dependency treegraph
 
Reported: 2011-01-31 13:54 UTC by Colin Campbell
Modified: 2017-06-14 22:18 UTC (History)
18 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this: 2013-10-30 00:00:00
Who signed the patch off:
Text to go in the release notes:
The Housebound module is an addition to Koha to allow the library to link together housebound patrons, volunteers, delivers and book choosers. - 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').


Attachments
Initial Spec (300.15 KB, application/pdf)
2011-01-31 13:54 UTC, Colin Campbell
Details
Housebound module for v3.6 (43.58 KB, patch)
2013-10-21 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Added syspref for housebound previously issued items (4.27 KB, patch)
2013-10-21 16:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Remove unused variables from C4::Housebound (9.70 KB, patch)
2013-10-21 16:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Commit Housebound Updates (19.13 KB, patch)
2013-10-21 16:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 5670: Housebound Readers Module (62.54 KB, patch)
2014-04-04 11:16 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: Housebound Readers Module (62.60 KB, patch)
2014-04-18 13:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Revised Housebound Patch (56.04 KB, patch)
2016-04-28 15:08 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Rebase on master (56.05 KB, patch)
2016-05-10 12:06 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Housebound Schema Updates (6.42 KB, patch)
2016-05-27 12:10 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: Housebound Readers Module (60.67 KB, patch)
2016-08-12 16:15 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
DO NOT PUSH: Add Housebound Schema files. (6.48 KB, patch)
2016-08-12 16:17 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: Housebound Readers Module (60.73 KB, patch)
2016-09-08 08:47 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5670: [Followup] Rename test files. (1.77 KB, patch)
2016-09-08 12:11 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [Followup] Return Koha::Patron objects. (2.32 KB, patch)
2016-09-08 12:11 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [Followup] Refactor .pl; error messages. (7.96 KB, patch)
2016-09-08 12:11 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Don't force ISO dates. (6.84 KB, patch)
2016-09-21 17:01 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Fix category_type ref. (7.58 KB, patch)
2016-09-21 17:02 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Avoid CGI::param in list context. (953 bytes, patch)
2016-09-21 17:02 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Housebound link from patron edit. (1.27 KB, patch)
2016-09-21 17:02 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Small QA fixes. (2.38 KB, patch)
2016-09-21 17:02 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Fix `div.patroninfo`. (3.86 KB, patch)
2016-09-21 17:02 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: Add the 'required' class to required fields (4.78 KB, patch)
2016-09-28 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: mv housebound_* to search_housebound_* (3.70 KB, patch)
2016-09-28 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: remove useless call to ->new when ->search is enought (1.07 KB, patch)
2016-09-28 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Improve template style. (4.56 KB, patch)
2016-09-28 15:40 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] More "required" messages. (2.58 KB, patch)
2016-09-28 15:40 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Abandon Extended Patron Attr. (5.70 KB, patch)
2016-10-12 15:48 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Create HouseboundRole objects. (3.63 KB, patch)
2016-10-12 15:48 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Correct housebound role search. (7.51 KB, patch)
2016-10-12 15:48 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Display HouseboundRole info. (2.32 KB, patch)
2016-10-12 15:48 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [QA Followup] HouseboundRole CRUD from UI. (7.08 KB, patch)
2016-10-12 15:49 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
DO NOT PUSH: Moar Housebound Schema files. (5.05 KB, patch)
2016-10-13 08:07 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: Add auth_val_cat to auth_val_cat.sql. (1.82 KB, patch)
2016-10-13 10:11 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
Bug 5670: [Followup] Add 'step 4' edit button. (8.12 KB, patch)
2016-10-13 10:39 UTC, Alex Sassmannshausen
Details | Diff | Splinter Review
DO NOT PUSH: Add Housebound Schema files. (6.54 KB, patch)
2016-10-13 11:45 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: Housebound Readers Module (60.78 KB, patch)
2016-10-13 11:45 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [Followup] Rename test files. (1.83 KB, patch)
2016-10-13 11:45 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [Followup] Return Koha::Patron objects. (2.38 KB, patch)
2016-10-13 11:45 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [Followup] Refactor .pl; error messages. (8.01 KB, patch)
2016-10-13 11:45 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Don't force ISO dates. (6.90 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Fix category_type ref. (7.63 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Avoid CGI::param in list context. (1013 bytes, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Housebound link from patron edit. (1.33 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Small QA fixes. (2.44 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Fix `div.patroninfo`. (3.92 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: Add the 'required' class to required fields (4.84 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: mv housebound_* to search_housebound_* (3.75 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: remove useless call to ->new when ->search is enought (1.13 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Improve template style. (4.62 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] More "required" messages. (2.64 KB, patch)
2016-10-13 11:46 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Create housebound roles table. (5.76 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Create HouseboundRole objects. (3.69 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Correct housebound role search. (7.57 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Display HouseboundRole info. (2.38 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [QA Followup] HouseboundRole CRUD from UI. (7.14 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
DO NOT PUSH: Moar Housebound Schema files. (5.11 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: Add auth_val_cat to auth_val_cat.sql. (1.88 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 5670: [Followup] Add 'step 4' edit button. (8.18 KB, patch)
2016-10-13 11:47 UTC, Claire Gravely
Details | Diff | Splinter Review
DO NOT PUSH: Add Housebound Schema files. (6.54 KB, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Housebound Readers Module (61.27 KB, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [Followup] Return Koha::Patron objects. (2.38 KB, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [Followup] Refactor .pl; error messages. (8.01 KB, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Don't force ISO dates. (6.90 KB, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Avoid CGI::param in list context. (1013 bytes, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Housebound link from patron edit. (1.33 KB, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Small QA fixes. (2.44 KB, patch)
2016-10-17 16:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Fix `div.patroninfo`. (3.92 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Add the 'required' class to required fields (4.84 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: mv housebound_* to search_housebound_* (3.75 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: remove useless call to ->new when ->search is enought (1.13 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Improve template style. (4.62 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] More "required" messages. (2.64 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Create housebound roles table. (5.76 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Create HouseboundRole objects. (3.69 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Correct housebound role search. (7.57 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] Display HouseboundRole info. (2.38 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [QA Followup] HouseboundRole CRUD from UI. (7.14 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
DO NOT PUSH: Moar Housebound Schema files. (5.11 KB, patch)
2016-10-17 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Add auth_val_cat to auth_val_cat.sql. (1.88 KB, patch)
2016-10-17 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: [Followup] Add 'step 4' edit button. (8.18 KB, patch)
2016-10-17 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Update constraint name to match with the one in updatedatabase (1.12 KB, patch)
2016-10-17 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Does not return 0 in Koha::Patron->housebound_* methods (935 bytes, patch)
2016-10-17 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Use Koha.Preference to retrieve syspref from templates (2.75 KB, patch)
2016-10-17 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Use the fa icon for the save button (3.64 KB, patch)
2016-10-17 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5670: Add a confirmation message on delete (1.11 KB, patch)
2016-10-17 16:42 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2011-01-31 13:54:42 UTC
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
Comment 1 Nicole C. Engard 2011-01-31 13:59:08 UTC
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
Comment 2 Joann Ransom 2012-03-26 06:33:59 UTC
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.
Comment 3 Katrin Fischer 2013-04-14 09:35:01 UTC
Hi Colin, maybe you can give us an update on this development?
Comment 4 Colin Campbell 2013-04-15 11:49:52 UTC
Its in production at Halton. I'll pull the patches together ensure they apply against master and post them here for inspection
Comment 5 Magnus Enger 2013-06-28 09:19:22 UTC
(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? :-)
Comment 6 Viktor Sarge 2013-07-01 14:36:09 UTC
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.
Comment 7 Martin Renvoize 2013-10-21 16:10:19 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2013-10-21 16:10:57 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2013-10-21 16:11:18 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2013-10-21 16:11:36 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2013-10-21 16:26:00 UTC
Patches submitted on behalf of Colin and Mark...
Comment 12 I'm just a bot 2013-10-30 07:42:05 UTC
Patch applied cleanly, go forth and signoff
Comment 13 Chris Cormack 2014-01-01 08:34:19 UTC
This mostly applies cleanly on master. 

I think what we are missing is unit tests, and maybe we should move to the Koha:: namespace?
Comment 14 Magnus Enger 2014-03-11 09:56:40 UTC
Marking this "Failed QA" based on Chris' comments. See coding guideline "PERL17: Unit tests are required".
Comment 15 Alex Sassmannshausen 2014-04-04 11:16:34 UTC Comment hidden (obsolete)
Comment 16 Alex Sassmannshausen 2014-04-04 11:18:26 UTC
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
Comment 17 Kyle M Hall 2014-04-18 13:04:28 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2014-04-18 13:06:03 UTC
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 19 Katrin Fischer 2014-04-18 15:29:32 UTC
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.
Comment 20 Owen Leonard 2014-04-18 15:37:04 UTC
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.
Comment 21 Alex Sassmannshausen 2014-04-24 20:12:50 UTC
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
Comment 22 Alex Sassmannshausen 2016-04-28 15:08:30 UTC Comment hidden (obsolete)
Comment 23 Alex Sassmannshausen 2016-05-10 12:06:13 UTC Comment hidden (obsolete)
Comment 24 Alex Sassmannshausen 2016-05-25 14:05:45 UTC
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.
Comment 25 Alex Sassmannshausen 2016-05-25 14:36:32 UTC
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
Comment 26 s.paltineanu 2016-05-25 14:55:37 UTC
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 27 M. Tompsett 2016-05-27 00:51:10 UTC
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?
Comment 28 Alex Sassmannshausen 2016-05-27 08:03:05 UTC
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
Comment 29 Alex Sassmannshausen 2016-05-27 12:10:22 UTC Comment hidden (obsolete)
Comment 30 Alex Sassmannshausen 2016-06-03 07:42:49 UTC
The 'Frequency' authorized value category does not exist, so I cannot add an entry there.

Alex
Comment 31 Alex Sassmannshausen 2016-08-12 16:15:28 UTC Comment hidden (obsolete)
Comment 32 Alex Sassmannshausen 2016-08-12 16:17:15 UTC
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.
Comment 33 Chris Cormack 2016-09-08 08:10:42 UTC
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
Comment 34 Chris Cormack 2016-09-08 08:47:24 UTC
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>
Comment 35 Alex Sassmannshausen 2016-09-08 08:52:42 UTC
Cheers for looking at this Chris — glad to see it still applies :-)
Comment 36 Jonathan Druart 2016-09-08 09:45:33 UTC
Koha::Patron::HouseboundVisits->special_search is not used anywhere in the code, should be removed?
Comment 37 Jonathan Druart 2016-09-08 09:51:13 UTC
 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 :)
Comment 38 Alex Sassmannshausen 2016-09-08 09:55:15 UTC
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
Comment 39 Jonathan Druart 2016-09-08 09:56:38 UTC
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
Comment 40 Jonathan Druart 2016-09-08 09:58:27 UTC
(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.
Comment 41 Alex Sassmannshausen 2016-09-08 12:11:09 UTC
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.
Comment 42 Alex Sassmannshausen 2016-09-08 12:11:46 UTC
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.
Comment 43 Alex Sassmannshausen 2016-09-08 12:11:50 UTC
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.
Comment 44 Jonathan Druart 2016-09-13 11:57:09 UTC
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
Comment 45 Jonathan Druart 2016-09-13 12:00:25 UTC
(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.
Comment 46 Jonathan Druart 2016-09-13 12:19:17 UTC
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.
Comment 47 Alex Sassmannshausen 2016-09-21 17:01:57 UTC
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.
Comment 48 Alex Sassmannshausen 2016-09-21 17:02:08 UTC
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`.
Comment 49 Alex Sassmannshausen 2016-09-21 17:02:15 UTC
Created attachment 55736 [details] [review]
Bug 5670: [QA Followup] Avoid CGI::param in list context.

* members/housebound.pl: Force `borrowernumber` to scalar context.
Comment 50 Alex Sassmannshausen 2016-09-21 17:02:21 UTC
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.
Comment 51 Alex Sassmannshausen 2016-09-21 17:02:28 UTC
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.
Comment 52 Alex Sassmannshausen 2016-09-21 17:02:34 UTC
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.
Comment 53 Alex Sassmannshausen 2016-09-21 17:03:56 UTC
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 54 Jonathan Druart 2016-09-28 08:57:26 UTC
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?
Comment 55 Jonathan Druart 2016-09-28 09:24:37 UTC
Created attachment 55865 [details] [review]
Bug 5670: Add the 'required' class to required fields
Comment 56 Jonathan Druart 2016-09-28 09:24:42 UTC
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_'.
Comment 57 Jonathan Druart 2016-09-28 09:24:47 UTC
Created attachment 55867 [details] [review]
Bug 5670: remove useless call to ->new when ->search is enought
Comment 58 Jonathan Druart 2016-09-28 09:40:16 UTC
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?
Comment 59 Alex Sassmannshausen 2016-09-28 15:40:32 UTC
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.
Comment 60 Alex Sassmannshausen 2016-09-28 15:40:38 UTC
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.
Comment 61 Alex Sassmannshausen 2016-09-28 15:50:39 UTC
(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
Comment 62 Jonathan Druart 2016-09-29 07:25:42 UTC
(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?
Comment 63 Alex Sassmannshausen 2016-09-29 08:45:38 UTC
(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?
Comment 64 Jonathan Druart 2016-09-29 09:34:05 UTC
(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.
Comment 65 Jonathan Druart 2016-09-29 09:35:59 UTC
(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?
Comment 66 Alex Sassmannshausen 2016-09-29 10:05:22 UTC
(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.
Comment 67 Jonathan Druart 2016-09-29 10:22:54 UTC
(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.
Comment 68 Katrin Fischer 2016-09-29 10:27:10 UTC
Wondering, could be chooser and deliverer be the same person?
Comment 69 Katrin Fischer 2016-09-29 10:28:33 UTC
oh, I might have misread - chooser and deliverer would also each link to a borrowernumber.
Comment 70 Alex Sassmannshausen 2016-09-29 10:49:35 UTC
(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.
Comment 71 Alex Sassmannshausen 2016-09-29 11:28:59 UTC
(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.
Comment 72 Alex Sassmannshausen 2016-10-12 15:48:20 UTC
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.
Comment 73 Alex Sassmannshausen 2016-10-12 15:48:34 UTC
Created attachment 56299 [details] [review]
Bug 5670: [QA Followup] Create HouseboundRole objects.

* Koha/Patron/HouseboundRole.pm: New file.
* Koha/Patron/HouseboundRoles.pm: New file.
Comment 74 Alex Sassmannshausen 2016-10-12 15:48:44 UTC
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.
Comment 75 Alex Sassmannshausen 2016-10-12 15:48:51 UTC
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.
Comment 76 Alex Sassmannshausen 2016-10-12 15:49:02 UTC
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.
Comment 77 Alex Sassmannshausen 2016-10-12 15:50:42 UTC
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
Comment 78 Alex Sassmannshausen 2016-10-13 08:07:23 UTC
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.
Comment 79 Alex Sassmannshausen 2016-10-13 08:20:41 UTC
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
Comment 80 Alex Sassmannshausen 2016-10-13 10:11:24 UTC
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.
Comment 81 Alex Sassmannshausen 2016-10-13 10:39:41 UTC
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.
Comment 82 Claire Gravely 2016-10-13 11:45:17 UTC
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>
Comment 83 Claire Gravely 2016-10-13 11:45:39 UTC
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>
Comment 84 Claire Gravely 2016-10-13 11:45:44 UTC
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>
Comment 85 Claire Gravely 2016-10-13 11:45:50 UTC
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>
Comment 86 Claire Gravely 2016-10-13 11:45:56 UTC
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>
Comment 87 Claire Gravely 2016-10-13 11:46:02 UTC
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>
Comment 88 Claire Gravely 2016-10-13 11:46:08 UTC
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>
Comment 89 Claire Gravely 2016-10-13 11:46:13 UTC
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>
Comment 90 Claire Gravely 2016-10-13 11:46:18 UTC
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>
Comment 91 Claire Gravely 2016-10-13 11:46:23 UTC
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>
Comment 92 Claire Gravely 2016-10-13 11:46:29 UTC
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>
Comment 93 Claire Gravely 2016-10-13 11:46:34 UTC
Created attachment 56357 [details] [review]
Bug 5670: Add the 'required' class to required fields

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>
Comment 94 Claire Gravely 2016-10-13 11:46:39 UTC
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>
Comment 95 Claire Gravely 2016-10-13 11:46:44 UTC
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>
Comment 96 Claire Gravely 2016-10-13 11:46:50 UTC
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>
Comment 97 Claire Gravely 2016-10-13 11:46:55 UTC
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>
Comment 98 Claire Gravely 2016-10-13 11:47:00 UTC
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>
Comment 99 Claire Gravely 2016-10-13 11:47:07 UTC
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>
Comment 100 Claire Gravely 2016-10-13 11:47:12 UTC
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>
Comment 101 Claire Gravely 2016-10-13 11:47:17 UTC
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>
Comment 102 Claire Gravely 2016-10-13 11:47:22 UTC
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>
Comment 103 Claire Gravely 2016-10-13 11:47:28 UTC
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>
Comment 104 Claire Gravely 2016-10-13 11:47:33 UTC
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>
Comment 105 Claire Gravely 2016-10-13 11:47:38 UTC
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>
Comment 106 Jonathan Druart 2016-10-17 16:40:17 UTC
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>
Comment 107 Jonathan Druart 2016-10-17 16:40:23 UTC
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>
Comment 108 Jonathan Druart 2016-10-17 16:40:29 UTC
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>
Comment 109 Jonathan Druart 2016-10-17 16:40:34 UTC
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>
Comment 110 Jonathan Druart 2016-10-17 16:40:39 UTC
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>
Comment 111 Jonathan Druart 2016-10-17 16:40:44 UTC
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>
Comment 112 Jonathan Druart 2016-10-17 16:40:49 UTC
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>
Comment 113 Jonathan Druart 2016-10-17 16:40:55 UTC
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>
Comment 114 Jonathan Druart 2016-10-17 16:41:00 UTC
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>
Comment 115 Jonathan Druart 2016-10-17 16:41:06 UTC
Created attachment 56589 [details] [review]
Bug 5670: Add the 'required' class to required fields

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>
Comment 116 Jonathan Druart 2016-10-17 16:41:12 UTC
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>
Comment 117 Jonathan Druart 2016-10-17 16:41:17 UTC
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>
Comment 118 Jonathan Druart 2016-10-17 16:41:22 UTC
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>
Comment 119 Jonathan Druart 2016-10-17 16:41:28 UTC
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>
Comment 120 Jonathan Druart 2016-10-17 16:41:33 UTC
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>
Comment 121 Jonathan Druart 2016-10-17 16:41:38 UTC
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>
Comment 122 Jonathan Druart 2016-10-17 16:41:43 UTC
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>
Comment 123 Jonathan Druart 2016-10-17 16:41:49 UTC
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>
Comment 124 Jonathan Druart 2016-10-17 16:41:54 UTC
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>
Comment 125 Jonathan Druart 2016-10-17 16:41:59 UTC
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>
Comment 126 Jonathan Druart 2016-10-17 16:42:05 UTC
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>
Comment 127 Jonathan Druart 2016-10-17 16:42:10 UTC
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>
Comment 128 Jonathan Druart 2016-10-17 16:42:15 UTC
Created attachment 56602 [details] [review]
Bug 5670: Update constraint name to match with the one in updatedatabase
Comment 129 Jonathan Druart 2016-10-17 16:42:21 UTC
Created attachment 56603 [details] [review]
Bug 5670: Does not return 0 in Koha::Patron->housebound_* methods
Comment 130 Jonathan Druart 2016-10-17 16:42:26 UTC
Created attachment 56604 [details] [review]
Bug 5670: Use Koha.Preference to retrieve syspref from templates
Comment 131 Jonathan Druart 2016-10-17 16:42:31 UTC
Created attachment 56605 [details] [review]
Bug 5670: Use the fa icon for the save button
Comment 132 Jonathan Druart 2016-10-17 16:42:36 UTC
Created attachment 56606 [details] [review]
Bug 5670: Add a confirmation message on delete
Comment 133 Jonathan Druart 2016-10-17 16:46:05 UTC
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
Comment 134 Alex Sassmannshausen 2016-10-17 17:05:39 UTC
(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!
Comment 135 Kyle M Hall 2016-10-21 18:33:19 UTC
Pushed to master for 16.11, thanks Alex, Jonathan!
Comment 136 Paul Poulain 2017-01-11 17:00:55 UTC
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 ;-)
Comment 137 Paul Poulain 2017-01-11 17:02:14 UTC
PS : sorry Claire, I know you're not a guy ;-)
Comment 138 Jonathan Druart 2017-01-11 17:50:34 UTC
(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 :)
Comment 139 Paul Poulain 2017-01-11 17:53:39 UTC
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")
Comment 140 Jonathan Druart 2017-01-11 18:05:22 UTC
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: )
Comment 141 Alex Sassmannshausen 2017-01-12 09:11:32 UTC
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