https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5670
--- Comment #28 from Alex Sassmannshausen <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
