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'
Created attachment 51503 [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/)
Created attachment 51504 [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/)
Created attachment 51505 [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
> 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?
Created attachment 51652 [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/)
Created attachment 51653 [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/)
Created attachment 51654 [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
(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
Patch tested with a sandbox, by Eleftheria <koseoglo@lib.auth.gr>
Created attachment 51971 [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>
Created attachment 51972 [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>
Created attachment 52079 [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)
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.
(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 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?
(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.
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?
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?
(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?
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.
(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
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>
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>
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)
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
(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.
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
(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
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
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
Rebased and squashed
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).
(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
Created attachment 55912 [details] [review] Bug 14874 (QA Followup) Fix atomicupdate file name
Created attachment 55913 [details] [review] Bug 14874 (QA Followup) Fix atomicupdate file name
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>
Created attachment 55920 [details] [review] Bug 14874 (QA Followup) Fix atomicupdate file name
Bugs added for comment #33. Signing off.
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>
Created attachment 56028 [details] [review] Bug 14874: Set default DefaultPatronSearchFields just in case... Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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.
I have also rebased the patch on top of bug 17375, as it is a bug fix. Second patch as been squashed.
I'd like to see a patch on bug 17374 in order to have them in the same release.
Patches appear to need rebasing.
(In reply to Kyle M Hall from comment #44) > Patches appear to need rebasing. My bad, missed the dependancy. Patches do apply cleanly.
Pushed to master for 16.11, thanks Nick!
Created attachment 57009 [details] [review] Bug 14874: (followup) Fix typo introduced in sysprefs.sql Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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
Followup pushed to master, thanks Tomas!