Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches
Summary: Add ability to search for patrons by date of birth from checkout and patron q...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 17375
Blocks: 17374 19398
  Show dependency treegraph
 
Reported: 2015-09-22 14:52 UTC by Kyle M Hall
Modified: 2017-10-02 17:22 UTC (History)
12 users (show)

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


Attachments
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (6.81 KB, patch)
2016-05-15 15:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (6.81 KB, patch)
2016-05-15 15:25 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - (followup) Refactor t/db_dependent/Utils/Datatables_Members.t to use TestBuilder (9.52 KB, patch)
2016-05-15 15:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (7.41 KB, patch)
2016-05-20 13:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (7.41 KB, patch)
2016-05-20 13:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - (followup) Refactor t/db_dependent/Utils/Datatables_Members.t to use TestBuilder (9.52 KB, patch)
2016-05-20 13:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (7.47 KB, patch)
2016-06-03 08:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14874 - (followup) Refactor t/db_dependent/Utils/Datatables_Members.t to use TestBuilder (9.57 KB, patch)
2016-06-03 08:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14874: Tidy previous patch (2.38 KB, patch)
2016-06-06 13:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (7.46 KB, patch)
2016-06-30 15:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - (followup) Refactor t/db_dependent/Utils/Datatables_Members.t to use TestBuilder (9.57 KB, patch)
2016-06-30 15:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874: Tidy previous patch (2.38 KB, patch)
2016-06-30 15:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 (QA Followup) Add dateofbirth column to checkout search results Reintroduce $termsearchfieldstype for resetting search type for non-date terms (2.84 KB, patch)
2016-06-30 15:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - Use a syspref to determine the 'standard' search fileds (6.58 KB, patch)
2016-09-15 16:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (18.60 KB, patch)
2016-09-29 00:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 (QA Followup) (1.05 KB, patch)
2016-09-29 10:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 (QA Followup) (920 bytes, patch)
2016-09-29 10:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (18.78 KB, patch)
2016-09-29 11:28 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14874 (QA Followup) (920 bytes, patch)
2016-09-29 11:29 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches (19.61 KB, patch)
2016-10-04 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14874: Set default DefaultPatronSearchFields (998 bytes, patch)
2016-10-04 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14874: (followup) Fix typo introduced in sysprefs.sql (1.88 KB, patch)
2016-10-31 19:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2015-09-22 14:52:30 UTC
Here's the general idea:

1) Modify C4::Utils::DataTables::Members::search to autodetect if the searchmember parameter is a date
    a) Build detection regex based on dateformat syspref
2) If searchmember is a valid date, convert to iso, set $searchfieldstype to 'dateofbirth'
Comment 1 Nick Clemens 2016-05-15 15:25:33 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2016-05-15 15:25:59 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2016-05-15 15:26:03 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2016-05-17 13:36:58 UTC
> 4 - Search combined with other terms, results should be returned as
> expected

This step didn't work for me. I found a date of birth which should have returned two results, "2006-08-09" and searched using the circulation quick search for that date of birth plus the first name of one of the results, "2006-08-09 john." No results were found.

Is that how it's supposed to work or do I misunderstand this step?
Comment 5 Nick Clemens 2016-05-20 13:51:22 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2016-05-20 13:51:50 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2016-05-20 13:51:53 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2016-05-20 13:52:42 UTC
(In reply to Owen Leonard from comment #4)
> > 4 - Search combined with other terms, results should be returned as
> > expected
> Is that how it's supposed to work or do I misunderstand this step?

Fixed it, I wasn't resetting the fields type after hitting a date
Comment 9 PTFS Europe Sandboxes 2016-06-03 08:36:16 UTC
Patch tested with a sandbox, by Eleftheria <koseoglo@lib.auth.gr>
Comment 10 PTFS Europe Sandboxes 2016-06-03 08:36:44 UTC Comment hidden (obsolete)
Comment 11 PTFS Europe Sandboxes 2016-06-03 08:36:47 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-06-06 13:23:12 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-06-06 13:25:53 UTC
Could you detail the use case?
I am wondering if this behavior is not too arbitrary: Why date of birth and not another date field?
There is a "Date of birth" option in the field list, so it's possible to search by dob.
Comment 14 Nick Clemens 2016-06-06 13:46:41 UTC
(In reply to Jonathan Druart from comment #13)
> Could you detail the use case?
> I am wondering if this behavior is not too arbitrary: Why date of birth and
> not another date field?
> There is a "Date of birth" option in the field list, so it's possible to
> search by dob.

The use case for the sponsors is in differentiating patrons with multiple names.
So if they look up 'John Doe' and get 4 results they can quickly do 'John Doe 1984-01-01' to get the correct patron without having to change to the patron search screen
Comment 15 Jonathan Druart 2016-06-06 14:03:00 UTC
(In reply to Nick Clemens from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > Could you detail the use case?
> > I am wondering if this behavior is not too arbitrary: Why date of birth and
> > not another date field?
> > There is a "Date of birth" option in the field list, so it's possible to
> > search by dob.
> 
> The use case for the sponsors is in differentiating patrons with multiple
> names.
> So if they look up 'John Doe' and get 4 results they can quickly do 'John
> Doe 1984-01-01' to get the correct patron without having to change to the
> patron search screen

In the patron search, the dob is displayed so I guess it's easier to click on the result than starting another search.
So it could work for the patron results returned by the "check out" tab but in this case, why not adding a new dob column to the table?
Comment 16 Mike Kerth 2016-06-15 21:15:16 UTC
(In reply to Jonathan Druart from comment #13)
> Could you detail the use case?
> I am wondering if this behavior is not too arbitrary: Why date of birth and
> not another date field?
> There is a "Date of birth" option in the field list, so it's possible to
> search by dob.

Our use case would be to ask patrons for their DOB instead of their name when we have to look them up manually to save time. For instance if a patron called over the phone to renew their checkouts and their name was Jonh Deduaknouski we could ask them DOB instead of having them tediously spell out their name. 

Same for when patrons inside the building don't have their library card. If we have their picture on file it would be much quicker to ask for DOB instead of verifying how they spell their last name, or potentially with a name like John Smith that may have multiple results and we will have to ask their DOB anyway.
Comment 17 Jonathan Druart 2016-06-18 12:39:05 UTC
Hi Mike,
Thanks for your answer.
If you are on /cgi-bin/koha/members/members-home.pl there is a patron search form in the header where you can specify you want to search on dob ("Search fields: Date of birth").
You could pre-select this field using some JavaScript code.
Would not that fit your needs?
Comment 18 Katrin Fischer 2016-06-19 14:17:51 UTC
Hm, this seems a bit like 'magic' behaviour - so it would be something that has to be explained in the manual, as it's not something you might try out on your own ever. So I am not totally persuaded about the usefulness, but also not set against it.

But apart from that, I am mostly worried about how this will impact the performance of the patron search - will it make things slower?

Apart from this:

1) if you do a search from the check out tab, you end up with a results table that doesn't show the date of birth - I think if it's searchable, it should also show.

2) Patron search page: a combined search for birthdate and part of the name doesn't always seem to work? (comment#14)
With the sample data: 
19.01.1931 Cherry - no result
Cherry 19.01.1931 - result

Can you please take a look?
Comment 19 Nick Clemens 2016-06-20 15:08:39 UTC
(In reply to Katrin Fischer from comment #18)
> But apart from that, I am mostly worried about how this will impact the
> performance of the patron search - will it make things slower?

I wouldn't think so, the search itself is unaffected, there is a check on each search term to see if it is a date time, I don't think you would notice any drain unless you had MANY MANY search terms

> 1) if you do a search from the check out tab, you end up with a results
> table that doesn't show the date of birth - I think if it's searchable, it
> should also show.

Will provde on a followup shortly

> 2) Patron search page: a combined search for birthdate and part of the name
> doesn't always seem to work? (comment#14)
 
Jonathan's followup removed the variable termsearchfieldtype - this was being used to force a search by DOB if the term was a date and use whatever was passed in otherwise. Another option would be to just concat DOB to the fields being searched - do you have a preference Jonathan?
Comment 20 Martin Renvoize 2016-06-21 05:59:01 UTC
Performance wise,  I tend to like to see a benchmark script rather than conjecture to prove the point..  hint hint,  it should be super trivial to knock one up.
Comment 21 Nick Clemens 2016-06-23 17:39:37 UTC
(In reply to Martin Renvoize from comment #20)
> Performance wise,  I tend to like to see a benchmark script rather than
> conjecture to prove the point

I didn't mean for it to sound like conjecture, in detail what happens is:

1 eval call to Koha::DateUtils::output_pref is added for each search term

If a term is a datetime then the searchfieldtype is changed to 'dateofbirth'

The only additional processing is the eval - at 1 per term there would have to be many terms to add significant processing time
Comment 22 Nick Clemens 2016-06-30 15:19:26 UTC
Created attachment 52998 [details] [review]
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches

This patch alters C4::Utils::DataTables::Members::search to autodetect
if the searchmember parameter is a date and search by DOB if so

To test:
1 - Apply patch
2 - Search for members by DOB formatted as per dateformat syspref or
mysql format
3 - Results should be returned as expected
4 - Search combined with other terms, results should be returned as
expected
5 - Search for patrons without using dates, results should be as
expected
6 - Search using search fields 'Date of birth' from expanded menu on
patron search - results should be returned as before (Note dying on
        incorrect format is an existing bug)
7 - prove t/db_dependent/Utils/Datatables_Members.t

Sponsored by Carnegie Stout Library (https://www.dubuque.lib.ia.us/)

Signed-off-by: Eleftheria <koseoglo@lib.auth.gr>
Comment 23 Nick Clemens 2016-06-30 15:19:35 UTC
Created attachment 52999 [details] [review]
Bug 14874 - (followup) Refactor t/db_dependent/Utils/Datatables_Members.t to use TestBuilder

To test:
1 - Apply other patches (not this one)
2 - Prove t/db_dependent/Utils/Datatables_Members.t
3 - SHould pass
4 - Apply this patch
5 - prove t/db_dependent/Utils/Datatables_Members.t
6 - All tests should continue to pass

Signed-off-by: Eleftheria <koseoglo@lib.auth.gr>
Comment 24 Nick Clemens 2016-06-30 15:19:40 UTC
Created attachment 53000 [details] [review]
Bug 14874: Tidy previous patch

remove debug statements
remove useless add of new var $termsearchfieldstype
rename term_is_dt with term_dt (term_is_dt is not a boolean but a dt)
Comment 25 Nick Clemens 2016-06-30 15:19:46 UTC
Created attachment 53001 [details] [review]
Bug 14874 (QA Followup)     Add dateofbirth column to checkout search results     Reintroduce $termsearchfieldstype for resetting search type for     non-date terms
Comment 26 Jonathan Druart 2016-07-02 13:34:48 UTC
(In reply to Jonathan Druart from comment #13)
> I am wondering if this behavior is not too arbitrary: Why date of birth and
> not another date field?

I'd like to get another QA opinion for this point.
Comment 27 Martin Renvoize 2016-07-04 06:43:21 UTC
Cheers for the explaination regarding performance.  I also see that the DateTime identification code is s simple regex so I'm happy on that front.

Martin
Comment 28 Martin Renvoize 2016-07-04 06:45:41 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Jonathan Druart from comment #13)
> > I am wondering if this behavior is not too arbitrary: Why date of birth and
> > not another date field?
> 
> I'd like to get another QA opinion for this point.

I must admit,  I'm not sure about the usefulness of this either.  The fields we search by default are somewhat arbitrary (but work reasonably well).  Adding this may day a precident for adding others so I think it's depict you've asked for such justifications.   Hmmm. Me ponders
Comment 29 Nick Clemens 2016-09-15 16:22:18 UTC
Created attachment 55589 [details] [review]
Bug 14874 - Use a syspref to determine the 'standard' search fileds

This patch adds a new syspef 'DefaultPatronSearchFields' which defines
the fields that should be used when searching for a patron if none are
defined.

To test:
1 - Aply patch, updatedatabase
2 - Ensure patron search has not changed
3 - Add dateofbirth to new pref
4 - Ensure things work wll
5 - Experiment with adding and removing other fields from borrowers
table
6 - prove t/db_dependent/Utils/Datatables_Members.t
Comment 30 Nick Clemens 2016-09-29 00:27:10 UTC
Created attachment 55899 [details] [review]
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches

This patch adds a new syspef 'DefaultPatronSearchFields' which defines
the fields that should be used when searching for a patron if none are
defined.

To test:
1 - Aply patch, updatedatabase
2 - Ensure patron search has not changed
3 - Add dateofbirth to new pref
4 - Ensure things work wll
5 - Experiment with adding and removing other fields from borrowers
table
6 - prove t/db_dependent/Utils/Datatables_Members.t
Comment 31 Nick Clemens 2016-09-29 00:27:59 UTC
Rebased and squashed
Comment 32 Marc Véron 2016-09-29 09:05:03 UTC
Updating database did not work because 
/installer/data/mysql/atomicupdate/bug14874-addDefaultPatronSearchFieldspreference
should be:
/installer/data/mysql/atomicupdate/bug14874-addDefaultPatronSearchFieldspreference.sql

After fixing
- test runs fine
- I can search a patron by date of birth (with dateofbirth added to 'DefaultPatronSearchFields')

- It works with different date formats (sypref dateformat)

- Standard fields work with name + date (e.g. meier 12.01.1995) 
- ...with name + day (e.g. meier 12)
- ...with name + month (e.g.meier 01)
- ...with name + year (e.g.meier 1995)
- It does not work with name + partial date (e.g. meier 12.01)

- I can trigger an 'Internal Server Error' by selecting "Date of birth" as search field and searching for an invalid date (e.g. 12.01 instead of 12.01.1995)

- E.g. with zipcode added to syspref, I can find a patron by zipcode using searchfields = standard, however I can not select zipcode as search field. Would be nice to have (in a separate bug/enhancement based on this bug).
Comment 33 Nick Clemens 2016-09-29 10:40:57 UTC
(In reply to Marc Véron from comment #32)
> Updating database did not work because 
> /installer/data/mysql/atomicupdate/bug14874-
> addDefaultPatronSearchFieldspreference
> should be:
> /installer/data/mysql/atomicupdate/bug14874-
> addDefaultPatronSearchFieldspreference.sql

Will fix

> 
> After fixing
> - It does not work with name + partial date (e.g. meier 12.01)

Nope and I don't think it should. We only convert date to SQL format if it is a full date - guessing on partial dates would be tricky and likely not work. Should work with '12 01' so each part of date can be searched as its own term


> - I can trigger an 'Internal Server Error' by selecting "Date of birth" as
> search field and searching for an invalid date (e.g. 12.01 instead of
> 12.01.1995)
This is true on master:
67 if ( $searchfieldstype eq "dateofbirth" ) {
68     $searchmember = output_pref({dt => dt_from_string($searchmember), dateformat => 'iso', dateonly => 1});
69 }

We don't catch any errors or validate data - likely we should use datepicker in this scenario - file a bug please

> - E.g. with zipcode added to syspref, I can find a patron by zipcode using
> searchfields = standard, however I can not select zipcode as search field.
> Would be nice to have (in a separate bug/enhancement based on this bug).

Hmm..the only problem is determining if we want all patron fields listed, or only the 'useful' ones - file a bug though and see opinions
Comment 34 Nick Clemens 2016-09-29 10:41:40 UTC
Created attachment 55912 [details] [review]
Bug 14874 (QA Followup)

Fix atomicupdate file name
Comment 35 Nick Clemens 2016-09-29 10:42:49 UTC
Created attachment 55913 [details] [review]
Bug 14874 (QA Followup)

Fix atomicupdate file name
Comment 36 Marc Véron 2016-09-29 11:28:40 UTC
Created attachment 55919 [details] [review]
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches

This patch adds a new syspef 'DefaultPatronSearchFields' which defines
the fields that should be used when searching for a patron if none are
defined.

To test:
1 - Aply patch, updatedatabase
2 - Ensure patron search has not changed
3 - Add dateofbirth to new pref
4 - Ensure things work wll
5 - Experiment with adding and removing other fields from borrowers
table
6 - prove t/db_dependent/Utils/Datatables_Members.t

Tested together with followup. Works as described.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 37 Marc Véron 2016-09-29 11:29:13 UTC
Created attachment 55920 [details] [review]
Bug 14874 (QA Followup)

    Fix atomicupdate file name
Comment 38 Marc Véron 2016-09-29 11:52:49 UTC
Bugs added for comment #33. Signing off.
Comment 39 Jonathan Druart 2016-10-04 12:04:09 UTC
Created attachment 56027 [details] [review]
Bug 14874 - Add ability to search for patrons by date of birth from checkout and patron quick searches

This patch adds a new syspef 'DefaultPatronSearchFields' which defines
the fields that should be used when searching for a patron if none are
defined.

To test:
1 - Aply patch, updatedatabase
2 - Ensure patron search has not changed
3 - Add dateofbirth to new pref
4 - Ensure things work wll
5 - Experiment with adding and removing other fields from borrowers
table
6 - prove t/db_dependent/Utils/Datatables_Members.t

Tested together with followup. Works as described.
Signed-off-by: Marc Véron <veron@veron.ch>

Bug 14874 (QA Followup)

    Fix atomicupdate file name

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Jonathan Druart 2016-10-04 12:04:14 UTC
Created attachment 56028 [details] [review]
Bug 14874: Set default DefaultPatronSearchFields

just in case...

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2016-10-04 12:04:58 UTC
Great Nick, this version is much more better :)
The type of the pref should not be "multi" but it does not seem to hurt and bug 17374 has been opened.
Comment 42 Jonathan Druart 2016-10-04 12:06:06 UTC
I have also rebased the patch on top of bug 17375, as it is a bug fix.
Second patch as been squashed.
Comment 43 Jonathan Druart 2016-10-04 12:08:27 UTC
I'd like to see a patch on bug 17374 in order to have them in the same release.
Comment 44 Kyle M Hall 2016-10-21 17:18:23 UTC
Patches appear to need rebasing.
Comment 45 Kyle M Hall 2016-10-27 13:07:12 UTC
(In reply to Kyle M Hall from comment #44)
> Patches appear to need rebasing.

My bad, missed the dependancy. Patches do apply cleanly.
Comment 46 Kyle M Hall 2016-10-27 14:04:40 UTC
Pushed to master for 16.11, thanks Nick!
Comment 47 Tomás Cohen Arazi 2016-10-31 19:21:05 UTC
Created attachment 57009 [details] [review]
Bug 14874: (followup) Fix typo introduced in sysprefs.sql

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 48 Jonathan Druart 2016-11-01 09:53:21 UTC
(In reply to Tomás Cohen Arazi from comment #47)
> Created attachment 57009 [details] [review] [review]
> Bug 14874: (followup) Fix typo introduced in sysprefs.sql
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

I have just added a check to the QA script:
  commit b8479e5189f992c62a828fd88e1a7cc2e1206835
    Lines should not terminate by semicolons in sysprefs.sql
Comment 49 Kyle M Hall 2016-11-01 16:23:14 UTC
Followup pushed to master, thanks Tomas!