Bug 17933 - Internal software error when searching patron without birth date
Summary: Internal software error when searching patron without birth date
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff Client (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: Dobrica Pavlinusic
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-18 14:55 UTC by Dobrica Pavlinusic
Modified: 2017-02-19 19:55 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17933 - Internal software error when searching patron without birth date (2.06 KB, patch)
2017-01-18 17:14 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 17933 - Internal software error when searching patron without birth date (2.11 KB, patch)
2017-01-18 23:24 UTC, Gracie
Details | Diff | Splinter Review
Bug 17933: Add test and return unless defined dob (1.89 KB, patch)
2017-01-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17933: Do not instanciate a patron if not needed (921 bytes, patch)
2017-01-19 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17933: Do not instanciate a patron if not needed (975 bytes, patch)
2017-02-01 12:50 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 17933 - Internal software error when searching patron without birth date (2.11 KB, patch)
2017-02-01 12:51 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 17933: Add test and return unless defined dob (1.94 KB, patch)
2017-02-01 12:52 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 17933: Do not instanciate a patron if not needed (975 bytes, patch)
2017-02-01 12:52 UTC, Dobrica Pavlinusic
Details | Diff | Splinter Review
Bug 17933 - Internal software error when searching patron without birth date (2.17 KB, patch)
2017-02-16 16:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17933: Add test and return unless defined dob (2.00 KB, patch)
2017-02-16 16:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17933: Do not instanciate a patron if not needed (1.01 KB, patch)
2017-02-16 16:40 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Dobrica Pavlinusic 2017-01-18 14:55:07 UTC
It seems that Bug 17557 reintroduced problem with invalid birth dates fixed by Bug 17375.
Comment 1 Dobrica Pavlinusic 2017-01-18 15:01:40 UTC
Our current problem are patrons without date of birth, which isn't covered by tests
Comment 2 Dobrica Pavlinusic 2017-01-18 17:14:54 UTC
Created attachment 59172 [details] [review]
Bug 17933 - Internal software error when searching patron without birth date

When patrons don't have date of birth (which is not required) patron
search results on moremember page produce internal server error since we
can't convert MySQL invalid date 0000-00-00 to datetime object and
call strfdate on it.

Additionally, since we assign dates to template variables and after
than assign whole $data hash to template, later assigment overrides
previous one, so we see birth date field even for patrons which don't
have one.

This patch fixes both of those problems.

Test:
1. edit patron and remove it's birth date
2. try to search for it, and verify server error
3. apply patch
4. repeat search for patron and verify that it works and doesn't
   have enpty birth date field
Comment 3 Gracie 2017-01-18 22:28:14 UTC
I followed your test plan and I cannot reproduce the bug.  Can someone else test to confirm that this a valid bug?
Comment 4 Katrin Fischer 2017-01-18 22:43:11 UTC
I found a way to reproduce it. It happens when 0000-00-00 is in the dateofbirth field in the borrowers table, not with NULL or ''.
But the only way I managed to put the invalid date there was SQL.
Dobrica, could this be old data we are dealing with here?
Comment 5 Gracie 2017-01-18 23:24:28 UTC
Created attachment 59194 [details] [review]
Bug 17933 - Internal software error when searching patron without birth date

When patrons don't have date of birth (which is not required) patron
search results on moremember page produce internal server error since we
can't convert MySQL invalid date 0000-00-00 to datetime object and
call strfdate on it.

Additionally, since we assign dates to template variables and after
than assign whole $data hash to template, later assigment overrides
previous one, so we see birth date field even for patrons which don't
have one.

This patch fixes both of those problems.

Test:
1. edit patron and remove it's birth date
2. try to search for it, and verify server error
3. apply patch
4. repeat search for patron and verify that it works and doesn't
   have enpty birth date field

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>
Comment 6 Jonathan Druart 2017-01-19 10:21:13 UTC
Created attachment 59214 [details] [review]
Bug 17933: Add test and return unless defined dob

Without this patch get_age return actually 0 instead of
Comment 7 Jonathan Druart 2017-01-19 10:21:40 UTC
Created attachment 59215 [details] [review]
Bug 17933: Do not instanciate a patron if not needed
Comment 8 Jonathan Druart 2017-01-19 10:25:16 UTC
Hi Dobrica!
Since 3.21.00.023 you should not have any 0000-00-00 values in the borrowers table.
We should no longer deal with these values in our code.
However undef must be handled in the ->get_age method.
I also agree with the change in the pl script.

Note that these 2 last patches will break again 0000-00-00, will you agree with that?

Do you know why you still get these invalid values in DB?
Comment 9 Jonathan Druart 2017-01-19 10:25:52 UTC
Lowering the severity as it should not happen with valid data.
Comment 10 Dobrica Pavlinusic 2017-01-30 15:17:24 UTC
Most of our invalid birth dates in DB are result of external bulk script to open accounts at beginning of each academic year since source data sometimes doesn't have birth dates, so this is our local problem, and not Koha related.

However, it doesn't explain all entries, since some of them are created after bulk script, using Koha's replication from LDAP. We have following mapping in koha-conf.xml:

<dateofbirth is="hredupersondateofbirth" ></dateofbirth>

and if LDAP entry doesn't have hredupersondateofbirth we are also getting 0000-00-00 for dateofbirth in Koha mosty because of


 $data = $mapping{$key}->{content} || '';    # default or failsafe ''

from ldap_entry_2_hash in C4/Auth_with_ldap.pm.

Sould I fix ldap_entry_2_hash as separate bug?
Comment 11 Dobrica Pavlinusic 2017-01-30 15:57:52 UTC
On closer review, our script is using C4::Auth_with_ldap::ldap_entry_2_hash so I opened Bug 18016 to track this issue.
Comment 12 Dobrica Pavlinusic 2017-02-01 12:50:08 UTC
Created attachment 59733 [details] [review]
Bug 17933: Do not instanciate a patron if not needed

Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Comment 13 Dobrica Pavlinusic 2017-02-01 12:51:53 UTC
Created attachment 59734 [details] [review]
Bug 17933 - Internal software error when searching patron without birth date

When patrons don't have date of birth (which is not required) patron
search results on moremember page produce internal server error since we
can't convert MySQL invalid date 0000-00-00 to datetime object and
call strfdate on it.

Additionally, since we assign dates to template variables and after
than assign whole $data hash to template, later assigment overrides
previous one, so we see birth date field even for patrons which don't
have one.

This patch fixes both of those problems.

Test:
1. edit patron and remove it's birth date
2. try to search for it, and verify server error
3. apply patch
4. repeat search for patron and verify that it works and doesn't
   have enpty birth date field

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>
Comment 14 Dobrica Pavlinusic 2017-02-01 12:52:04 UTC
Created attachment 59735 [details] [review]
Bug 17933: Add test and return unless defined dob

Without this patch get_age return actually 0 instead of

Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Comment 15 Dobrica Pavlinusic 2017-02-01 12:52:09 UTC
Created attachment 59736 [details] [review]
Bug 17933: Do not instanciate a patron if not needed

Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Comment 16 Nick Clemens 2017-02-16 16:40:23 UTC
Created attachment 60356 [details] [review]
Bug 17933 - Internal software error when searching patron without birth date

When patrons don't have date of birth (which is not required) patron
search results on moremember page produce internal server error since we
can't convert MySQL invalid date 0000-00-00 to datetime object and
call strfdate on it.

Additionally, since we assign dates to template variables and after
than assign whole $data hash to template, later assigment overrides
previous one, so we see birth date field even for patrons which don't
have one.

This patch fixes both of those problems.

Test:
1. edit patron and remove it's birth date
2. try to search for it, and verify server error
3. apply patch
4. repeat search for patron and verify that it works and doesn't
   have enpty birth date field

Signed-off-by: Grace McKenzie <grace.mcky@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2017-02-16 16:40:29 UTC
Created attachment 60357 [details] [review]
Bug 17933: Add test and return unless defined dob

Without this patch get_age return actually 0 instead of

Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2017-02-16 16:40:35 UTC
Created attachment 60358 [details] [review]
Bug 17933: Do not instanciate a patron if not needed

Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Kyle M Hall 2017-02-17 11:22:28 UTC
Pushed to master for 17.05, thank Dobrica, Jonathan!
Comment 20 Katrin Fischer 2017-02-19 19:54:04 UTC
Patch doesn't apply cleanly on 16.11.x - please rebase!
Comment 21 Katrin Fischer 2017-02-19 19:55:02 UTC
Hm, looking at the dependencies more closely - as bug 17557 is not part of 16.11.x this doesn't seem to be needed.