https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17168
--- Comment #27 from Martin Renvoize <martin.renvo...@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. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/