[Koha-devel] Koha and DBIC

[Kyle Hall]

On Wed, Sep 10, 2014 at 5:00 AM, Jonathan Druart <
jonathan.druart at biblibre.com> wrote:

> Hi all,
>
> IMO a ResultSet should not be instantiate in a pl script for some
> reasons (I already listed yesterday, nothing new :))
> - should be unit tested
> - easier to maintain
> - easier to reuse
>
> But I don't think it's a big deal.
>

I agree with all these principles, and none of them conflict with what I am
proposing. Result and ResultSet methods are no different than C4/Koha
methods and subroutines.


>
> I think someone (don't remember who) explained it is not a good idea
> to add too much changes to the ResultSet class. I tend to agree.
>
> In your example:
>   $borrower->is_debarred
> Where do you instantiate $borrower? And where is defined is_debarred?
>

Most likely you would use find(), single() or search(). In this example we
could easily swith from Koha::Borrower::Debarments::IsDebarred to
Koha::Schema::ResultSet::Borrower::is_debarred. There is no functional
difference, and it makes more sense in the long run. I agree that any
search that is even mildly complex and used in multiple places should live
in the ResultSet. I'm not saying we should use search() everyone where in
pl files, I'm just against a hard and fast rule forbidding it which will
cause developers to jump through hoops that don't need to exist.


>
> I would write something like I have written for
> Koha::Acquisition::Order (bug 12830)
>
>   package Koha::Borrower;
>   use Koha::Schema;
>   use base qw( Class::Accessor );
>
>   sub fetch {
>     my ( $class, $borrowernumber ) = @_;
>     my $borrower =
> Koha::Database->new->schema->resultset('Borrower')->find($borrowernumber,
> {result_class => 'DBIx::Class::ResultClass::HashRefInflator' } );
>     return $class->new( $borrower );
>   }
>
>   sub is_debarred {
>     my ( $self ) = @_;
>     my $debarred_date = $self->{debarred};
>     return $debarred_date > $today;
>   }
>
>   in the pl file:
>   my $borrower = Koha::Borrower->fetch( $borrowernumber );
>
> The limitation in this implementation is that $borrower is not a DBIC
> object and we would like to get rid of HashRefInflator.
>
> Please comment this part of code and share yours!
>

Can you see how this gives no advantage, but simply wraps the useful code
in another layer of abstraction? There simply is no advantage to doing
this. In the long run this type of thinking will make things more
complicated and more error prone than they need to be.

Instead of all this code we simply need to add one method to
Result::Borrower. Here is the gist of it:

sub is_debarred {
    my ( $self ) = @_;
    return $self->debarred_date() > dt_from_string();
}

Think about all the overhead and complexity that is removed by this
variation.

Kyle
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.koha-community.org/pipermail/koha-devel/attachments/20140910/f8ad195b/attachment.html>