The script does not allow to find empty fields or use wildcards It would be helpful to find patrons where a field has been set, or a field not matching, or similar fields
Created attachment 105439 [details] [review] Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category To test: 1 - Run the script with no parameters and verify the help explains the parameters 2 - Try running with various fields, matching and not matching 3 - Test null values 4 - Test like values with wildcards 5 - Test like with the word null to find fields containing the word rather than being unset
Created attachment 105605 [details] [review] Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category To test: 1 - Run the script with no parameters and verify the help explains the parameters 2 - Try running with various fields, matching and not matching 3 - Test null values 4 - Test like values with wildcards 5 - Test like with the word null to find fields containing the word rather than being unset Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov>
Comment on attachment 105605 [details] [review] Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category Review of attachment 105605 [details] [review]: ----------------------------------------------------------------- Can you add pod items for likefield and notlikefield options ? ::: misc/cronjobs/update_patrons_category.pl @@ +135,5 @@ > +Use this flag to specify a column in the borrowers table and update only patrons whose value in that column does not equal the value supplied (repeatable) > + > +e.g. > +--notfield email=NULL > +will update all patrons with no value for email all patrons with null email field ?
No a blocker but a concern: not* options work with empty and non-empty values but not with null values. perl misc/cronjobs/update_patrons_category.pl -f S -t B -dn email=foo -v => matches all patrons with email different from "foo" or email eq ''. But not the null ones. Is it the wanted behavior ?
(In reply to Alex Arnaud from comment #3) > > +will update all patrons with no value for email > > all patrons with null email field ? I mean: all patron with email different from null
Comment on attachment 105605 [details] [review] Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category Review of attachment 105605 [details] [review]: ----------------------------------------------------------------- ::: misc/cronjobs/update_patrons_category.pl @@ +57,5 @@ > -ra=date --regafter update if registration date is after a given date > -d --dbfield name=value where <name> is a column in the borrowers table, patrons will be updated if the field is equal to given <value> > + -dn --notfield name=value where <name> is a column in the borrowers table, patrons will be updated if the field is equal to given <value> > + -dl --likefield name=value where <name> is a column in the borrowers table, patrons will be updated if the field is equal to given <value> > + -dnl --notlikefield name=value where <name> is a column in the borrowers table, patrons will be updated if the field is equal to given <value> All three descriptions here match.. can you clarify the descriptions, please.. replacing 'equal' with the relevant comparison?
Created attachment 106965 [details] [review] Bug 25624: Update documentation
(In reply to Alex Arnaud from comment #5) > (In reply to Alex Arnaud from comment #3) > > > +will update all patrons with no value for email > > > > all patrons with null email field ? > > I mean: all patron with email different from null > => matches all patrons with email different from "foo" or email eq ''. But not > the null ones. Is it the wanted behavior ? -dn email=null will find all not null -d email=null will find all null I think specifically targeting null differently than blank makes sense as it can have different meanings
Created attachment 107291 [details] [review] Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category To test: 1 - Run the script with no parameters and verify the help explains the parameters 2 - Try running with various fields, matching and not matching 3 - Test null values 4 - Test like values with wildcards 5 - Test like with the word null to find fields containing the word rather than being unset Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Created attachment 107292 [details] [review] Bug 25624: Update documentation Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
I am sorry but I don't think it's ideal. What will happen if we want <, <=, >, <=? Cannot we imagine a parameter that takes the operator?
Created attachment 109116 [details] [review] Bug 25624: Add 'operator' option to allow for different comparisons This patch adds the option to specify the operators fper field specified in the script We also add support for testing 'null' fields To test: 1 - Run the script with no parameters and verify the help explains the parameters 2 - Add a patron in category PT (or your choice) with email 'a@b.com' 3 - Add a patron in category PT (or as above) with email 'c@d.com' 4 - Try running with options perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=a@b.com (first patron returned) perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=a@b.com -o email=e (first patron returned) perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=a@b.com -o email=gt (both patrons returned) perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=a@b.com -o email=gte (second patron returned) perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=a@b% -o email=l (first patron returned) perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=a@b% -o email=nl (second patron returned) perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=null -o email=e (neither patron returned) perl misc/cronjobs/update_patrons_category.pl -f PT -t s --field email=null -o email=ne (both patrons returned) 5 - Test other combination 6 - Test with --confirm and ensure updates suceed as before patches https://bugs.koha-community.org/show_bug.cgi?id=25264
(In reply to Jonathan Druart from comment #11) > I am sorry but I don't think it's ideal. > What will happen if we want <, <=, >, <=? > Cannot we imagine a parameter that takes the operator? Running this by our educators and others, they do not like the newer patch. The borrowers table does not have numeric fields where the greater than or less than operators are necessary. The feeling was that offering too many operators added complexity and confusion and would lead to more support issues rather than less. The patches as submitted here cover the use cases requested by the library sponsor and offer flexibility and ease of understanding for setup. Putting this back as passed QA for consideration
Created attachment 109370 [details] [review] Bug 25624: Add more flexibily with --where and literal search
Nick, can you have a look at this approach and let me know what you think about it?
(In reply to Jonathan Druart from comment #14) > Created attachment 109370 [details] [review] [review] > Bug 25624: Add more flexibily with --where and literal search Looks great!
Created attachment 109421 [details] [review] Bug 25624: Add more flexibily with --where and literal search Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I get a SQL error whenever I run this without a --where defined.
Created attachment 109427 [details] [review] Bug 25624: Add --where option to update_patrons_category.pl The script did not allow to find empty fields or use wildcards With this new option we will now have the ability to filter patrons by some of their attributes. Test plan: 1 - Run the script with no parameters and verify the help explains the parameters 2 - Try the script with one or more --where parameters, like: --where "firstname='koha'" 3 - Test null values --where "firstname IS NULL" 4 - Test like values with wildcards --where "firstname LIKE '%a%' 5 - Test like with the word null to find fields containing the word rather than being unset
Now I'm getting a different SQL error when running this with a --where, a --field, or neither: perl misc/cronjobs/update_patrons_category.pl -f PT -t B --where "firstname='koha'" -v No actions will be taken (test mode) Will update patrons from PT to B with conditions below (if any) DBD::mysql::st execute failed: Unknown column 'me.exclude_from_local_holds_priority' in 'field list' [for Statement "SELECT `me`.`categorycode`, `me`.`description`, `me`.`enrolmentperiod`, `me`.`enrolmentperioddate`, `me`.`upperagelimit`, `me`.`dateofbirthrequired`, `me`.`finetype`, `me`.`bulk`, `me`.`enrolmentfee`, `me`.`overduenoticerequired`, `me`.`issuelimit`, `me`.`reservefee`, `me`.`hidelostitems`, `me`.`category_type`, `me`.`BlockExpiredPatronOpacActions`, `me`.`default_privacy`, `me`.`checkprevcheckout`, `me`.`reset_password`, `me`.`change_password`, `me`.`exclude_from_local_holds_priority` FROM `categories` `me` WHERE ( `me`.`categorycode` = ? )" with ParamValues: 0='PT'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'me.exclude_from_local_holds_priority' in 'field list' at /kohadevbox/koha/Koha/Objects.pm line 96
I think you are missing a DB update.
Created attachment 109440 [details] [review] Bug 25624: Add --where option to update_patrons_category.pl The script did not allow to find empty fields or use wildcards With this new option we will now have the ability to filter patrons by some of their attributes. Test plan: 1 - Run the script with no parameters and verify the help explains the parameters 2 - Try the script with one or more --where parameters, like: --where "firstname='koha'" 3 - Test null values --where "firstname IS NULL" 4 - Test like values with wildcards --where "firstname LIKE '%a%' 5 - Test like with the word null to find fields containing the word rather than being unset Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Whoops, yes. Sorry about that!
(In reply to Andrew Fuerste-Henry from comment #23) > Whoops, yes. Sorry about that! haha, no worries. Thanks Andrew!
Created attachment 110747 [details] [review] Bug 25624: Add --where option to update_patrons_category.pl The script did not allow to find empty fields or use wildcards With this new option we will now have the ability to filter patrons by some of their attributes. Test plan: 1 - Run the script with no parameters and verify the help explains the parameters 2 - Try the script with one or more --where parameters, like: --where "firstname='koha'" 3 - Test null values --where "firstname IS NULL" 4 - Test like values with wildcards --where "firstname LIKE '%a%' 5 - Test like with the word null to find fields containing the word rather than being unset Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice enhancement, works as expected. Passing QA
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.05
enhancement, not backported to 19.11.x