[Koha-bugs] [Bug 17168] Add a command line script for updating patron category based on status

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

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

--- Comment #27 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
Comment on attachment 76974
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=76974
Bug 17168: Add a command line script for updating patron category based on
status

Review of attachment 76974:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=17168&attachment=76974)
-----------------------------------------------------------------

A few more QA notes.

::: Koha/Patrons.pm
@@ +209,4 @@
>      return $nb_rows;
>  }
>  
> +=head3 search_patrons_to_update

This is a very generic name for a pretty specific task..
`search_patrons_to_update_category` might be better...

@@ +210,5 @@
>  }
>  
> +=head3 search_patrons_to_update
> +
> +    my $patrons = Koha::Patrons->search_patrons_to_anonymise( {

This signature doesn't match the method name ;)

@@ +218,5 @@
> +                      au     => $au,
> +                      ao    => $ao,
> +                  });
> +
> +This method returns all patron who should be updated form one category to another meeting criteria:

Typo 'form' -> 'from'

@@ +223,5 @@
> +
> +from - original category
> +fine_min - with fines totaling at least this amount
> +fine_max - with fines above this amount
> +au - under the age limit for 'from'

Looks like you've renamed this to 'ageunder', also the description could be
clearer... it's a boolean right.. not an age in years that can be passed.

I'd probably go with `under_age - boolean denoting whether to filter down to
those patrons who are under the age limit as defined by their category`

I may even go so far as to say can it be 'too_young' or 'is_under_age' adding
the adverb clarifies the intention and boolean requirement.

@@ +224,5 @@
> +from - original category
> +fine_min - with fines totaling at least this amount
> +fine_max - with fines above this amount
> +au - under the age limit for 'from'
> +ao - over the agelimit for 'from'

As above but for overage

@@ +254,5 @@
> +        $query{group_by} = ["borrowernumber"];
> +        $query{having}{total_fines}{'<='}=$params->{fine_max} if defined $params->{fine_max};
> +        $query{having}{total_fines}{'>='}=$params->{fine_min} if defined $params->{fine_min};
> +    }
> +    return Koha::Patrons->search($search_params,\%query);

rather than returning a new Koha::Patrons object here you should use
$self->search($search_params,\%query) to allow for future chaining.

@@ +257,5 @@
> +    }
> +    return Koha::Patrons->search($search_params,\%query);
> +}
> +
> +=head3 update_category

Hmm, it actually scares me a little that this isn't handled inside the 'store'
routine.  This isn't the only way to update a patrons category and as such the
guarantor handling won't get triggered during over forms of the update which
could lead to bad data and bugs down the line..

@@ +260,5 @@
> +
> +=head3 update_category
> +
> +    Koha::Patrons->search->update_category( {
> +            to   => $to,

feels an odd signature.. I'd suggest `->update_category_to($category_code);`

-- 
You are receiving this mail because:
You are watching all bug changes.