[Koha-bugs] [Bug 5670] Housebound Readers Module

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5670

--- Comment #19 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 27288
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27288
Bug 5670: Housebound Readers Module

Review of attachment 27288:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=5670&attachment=27288)
-----------------------------------------------------------------

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are watching all bug changes.