https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17168 --- Comment #27 from Martin Renvoize <martin.renvoize@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.