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

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

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

--- Comment #28 from Alex Sassmannshausen <alex.sassmannshausen at 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.