Bug 25624 - Update patrons category script should allow finding null and not null and wildcards
Summary: Update patrons category script should allow finding null and not null and wil...
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-29 20:04 UTC by Nick Clemens
Modified: 2020-09-01 14:28 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category (4.31 KB, patch)
2020-05-29 20:06 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category (4.37 KB, patch)
2020-06-05 19:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25624: Update documentation (3.79 KB, patch)
2020-07-16 17:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25624: Add not equal, like, not like, and support for nulls to update_patron_category (4.42 KB, patch)
2020-07-24 09:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25624: Update documentation (3.84 KB, patch)
2020-07-24 09:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25624: Add 'operator' option to allow for different comparisons (4.96 KB, patch)
2020-08-25 15:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25624: Add more flexibily with --where and literal search (4.69 KB, patch)
2020-08-31 14:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25624: Add more flexibily with --where and literal search (4.75 KB, patch)
2020-09-01 11:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25624: Add --where option to update_patrons_category.pl (4.10 KB, patch)
2020-09-01 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25624: Add --where option to update_patrons_category.pl (4.17 KB, patch)
2020-09-01 14:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-05-29 20:04:39 UTC
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
Comment 1 Nick Clemens 2020-05-29 20:06:48 UTC
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
Comment 2 Andrew Fuerste-Henry 2020-06-05 19:51:14 UTC
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 3 Alex Arnaud 2020-06-12 12:31:30 UTC
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 ?
Comment 4 Alex Arnaud 2020-06-12 12:36:34 UTC
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 ?
Comment 5 Alex Arnaud 2020-06-12 12:37:56 UTC
(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 6 Martin Renvoize 2020-06-19 11:43:24 UTC
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?
Comment 7 Nick Clemens 2020-07-16 17:49:36 UTC
Created attachment 106965 [details] [review]
Bug 25624: Update documentation
Comment 8 Nick Clemens 2020-07-17 13:50:03 UTC
(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
Comment 9 Alex Arnaud 2020-07-24 09:01:01 UTC
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>
Comment 10 Alex Arnaud 2020-07-24 09:01:05 UTC
Created attachment 107292 [details] [review]
Bug 25624: Update documentation

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 11 Jonathan Druart 2020-07-24 12:07:21 UTC
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?
Comment 12 Nick Clemens 2020-08-25 15:45:53 UTC
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
Comment 13 Nick Clemens 2020-08-28 18:09:22 UTC
(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
Comment 14 Jonathan Druart 2020-08-31 14:00:03 UTC
Created attachment 109370 [details] [review]
Bug 25624: Add more flexibily with --where and literal search
Comment 15 Jonathan Druart 2020-08-31 14:00:36 UTC
Nick, can you have a look at this approach and let me know what you think about it?
Comment 16 Nick Clemens 2020-08-31 14:05:38 UTC
(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!
Comment 17 Nick Clemens 2020-09-01 11:10:18 UTC
Created attachment 109421 [details] [review]
Bug 25624: Add more flexibily with --where and literal search

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Andrew Fuerste-Henry 2020-09-01 12:24:25 UTC
I get a SQL error whenever I run this without a --where defined.
Comment 19 Jonathan Druart 2020-09-01 13:13:37 UTC
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
Comment 20 Andrew Fuerste-Henry 2020-09-01 14:01:42 UTC
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
Comment 21 Jonathan Druart 2020-09-01 14:09:46 UTC
I think you are missing a DB update.
Comment 22 Andrew Fuerste-Henry 2020-09-01 14:20:28 UTC
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>
Comment 23 Andrew Fuerste-Henry 2020-09-01 14:21:10 UTC
Whoops, yes. Sorry about that!
Comment 24 Jonathan Druart 2020-09-01 14:28:38 UTC
(In reply to Andrew Fuerste-Henry from comment #23)
> Whoops, yes. Sorry about that!

haha, no worries. Thanks Andrew!