<div dir="ltr"><div class="gmail_extra"><div>On Wed, Sep 10, 2014 at 5:00 AM, Jonathan Druart <span dir="ltr"><<a href="mailto:jonathan.druart@biblibre.com" target="_blank">jonathan.druart@biblibre.com</a>></span> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi all,<br>
<br>
IMO a ResultSet should not be instantiate in a pl script for some<br>
reasons (I already listed yesterday, nothing new :))<br>
- should be unit tested<br>
- easier to maintain<br>
- easier to reuse<br>
<br>
But I don't think it's a big deal.<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I think someone (don't remember who) explained it is not a good idea<br>
to add too much changes to the ResultSet class. I tend to agree.<br>
<br>
In your example:<br>
  $borrower->is_debarred<br>
Where do you instantiate $borrower? And where is defined is_debarred?<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I would write something like I have written for<br>
Koha::Acquisition::Order (bug 12830)<br>
<br>
  package Koha::Borrower;<br>
  use Koha::Schema;<br>
  use base qw( Class::Accessor );<br>
<br>
  sub fetch {<br>
    my ( $class, $borrowernumber ) = @_;<br>
    my $borrower =<br>
Koha::Database->new->schema->resultset('Borrower')->find($borrowernumber,<br>
{result_class => 'DBIx::Class::ResultClass::HashRefInflator' } );<br>
    return $class->new( $borrower );<br>
  }<br>
<br>
  sub is_debarred {<br>
    my ( $self ) = @_;<br>
    my $debarred_date = $self->{debarred};<br>
    return $debarred_date > $today;<br>
  }<br>
<br>
  in the pl file:<br>
  my $borrower = Koha::Borrower->fetch( $borrowernumber );<br>
<br>
The limitation in this implementation is that $borrower is not a DBIC<br>
object and we would like to get rid of HashRefInflator.<br>
<br>
Please comment this part of code and share yours!<br></blockquote><div><br></div><div>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.</div><div><br></div><div>Instead of all this code we simply need to add one method to Result::Borrower. Here is the gist of it:</div><div><br></div><div>sub is_debarred {</div><div>    my ( $self ) = @_;</div><div>    return $self->debarred_date() > dt_from_string();</div><div>}</div><div><br></div><div>Think about all the overhead and complexity that is removed by this variation.</div><div><br></div><div>Kyle </div></div></div></div>