https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5670 --- Comment #28 from Alex Sassmannshausen <alex.sassmannshausen@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are the QA Contact for the bug. You are watching all bug changes.