Relevant versions: OS: Ubuntu 22.04 Koha: 23.11.04-4 MySQL: 8.0.36-0ubuntu0.22.04.1 When using Auth_with_ldap, new borrowers that are created via LDAP have an updated_on date of 0000-00-00 00:00:00. Guessing this may be related to comments in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18016 May also be a side effect of explicitly declaring a default_value of \"current_timestamp" for updated_on in Koha/Schema/Result/Borrower.pm. (Guessing no one else is still using LDAP to create accounts)
I've been looking into this with Martin since we've been experiencing the same issue but with Shibboleth. We believe we've got a fix and currently working on it to submit soon.
We've narrowed the same issue down in Shibboleth and likely have the fix for this case too... we need to refresh the new patron object after it's creation in auth_with_ldap before passing it back to auth.pm else the internal representation of the data isn't up to date with whats in the database and we try to reset those fields at update later. What I don't fully understand is why the internal \'current_timestamp' is somehow getting converted to 0000-00-00 for insert later from SCALAR dump.. but I'll leave someone wiser than me to dig there if they're keen to.
Created attachment 173373 [details] [review] Bug 36822: Shibboleth - Add tests prove t/db_dependent/Auth_with_shibboleth.t
Created attachment 173374 [details] [review] Bug 36822: Shibboleth - Add discard_changes Test plan: 1) Apply only Shibboleth tests patch 2) Run that test file: $ prove t/db_dependent/Auth_with_shibboleth.t 3) Verify tests fail with the updated_on error message. 4) Apply this patch. Repeat 2) Verify tests now pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173375 [details] [review] Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file Without this rearrangement, C4::Context->config('useldapserver') is always 0. It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up
Created attachment 173376 [details] [review] Bug 36822: LDAP - Add test Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173377 [details] [review] Bug 36822: LDAP - Add discard_changes Test plan: 1) Apply only LDAP rearrangement patch 2) Run that test file (to prove the rearrangement does not break tests): $ prove t/db_dependent/Auth_with_ldap.t 3) Verify tests pass 4) Apply LDAP test patch on top 5) Run the test file. Notice it fails. 6) Apply this patch. Run LDAP tests again. Verify tests pass.
Created attachment 173385 [details] [review] Bug 36822: Shibboleth - Add tests prove t/db_dependent/Auth_with_shibboleth.t
Created attachment 173386 [details] [review] Bug 36822: Shibboleth - Add discard_changes Test plan: 1) Apply only Shibboleth tests patch 2) Run that test file: $ prove t/db_dependent/Auth_with_shibboleth.t 3) Verify tests fail with the updated_on error message. 4) Apply this patch. Repeat 2) Verify tests now pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173387 [details] [review] Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file Without this rearrangement, C4::Context->config('useldapserver') is always 0. It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up
Created attachment 173388 [details] [review] Bug 36822: LDAP - Add test
Created attachment 173389 [details] [review] Bug 36822: LDAP - Add discard_changes Test plan: 1) Apply only LDAP rearrangement patch 2) Run that test file (to prove the rearrangement does not break tests): $ prove t/db_dependent/Auth_with_ldap.t 3) Verify tests pass 4) Apply LDAP test patch on top 5) Run the test file. Notice it fails. 6) Apply this patch. Run LDAP tests again. Verify tests pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173391 [details] [review] Bug 36822: Shibboleth - Add tests prove t/db_dependent/Auth_with_shibboleth.t
Created attachment 173392 [details] [review] Bug 36822: Shibboleth - Add discard_changes Test plan: 1) Apply only Shibboleth tests patch 2) Run that test file: $ prove t/db_dependent/Auth_with_shibboleth.t 3) Verify tests fail with the updated_on error message. 4) Apply this patch. Repeat 2) Verify tests now pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173393 [details] [review] Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file Without this rearrangement, C4::Context->config('useldapserver') is always 0. It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up
Created attachment 173394 [details] [review] Bug 36822: LDAP - Add test
Created attachment 173395 [details] [review] Bug 36822: LDAP - Add discard_changes Test plan: 1) Apply only LDAP rearrangement patch 2) Run that test file (to prove the rearrangement does not break tests): $ prove t/db_dependent/Auth_with_ldap.t 3) Verify tests pass 4) Apply LDAP test patch on top 5) Run the test file. Notice it fails. 6) Apply this patch. Run LDAP tests again. Verify tests pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173402 [details] [review] Bug 36822: Shibboleth - Add tests prove t/db_dependent/Auth_with_shibboleth.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173403 [details] [review] Bug 36822: Shibboleth - Add discard_changes Test plan: 1) Apply only Shibboleth tests patch 2) Run that test file: $ prove t/db_dependent/Auth_with_shibboleth.t 3) Verify tests fail with the updated_on error message. 4) Apply this patch. Repeat 2) Verify tests now pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173404 [details] [review] Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file Without this rearrangement, C4::Context->config('useldapserver') is always 0. It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173405 [details] [review] Bug 36822: LDAP - Add test Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173406 [details] [review] Bug 36822: LDAP - Add discard_changes Test plan: 1) Apply only LDAP rearrangement patch 2) Run that test file (to prove the rearrangement does not break tests): $ prove t/db_dependent/Auth_with_ldap.t 3) Verify tests pass 4) Apply LDAP test patch on top 5) Run the test file. Notice it fails. 6) Apply this patch. Run LDAP tests again. Verify tests pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173646 [details] [review] Bug 36822: atomicupdate Fix any existing updated_on issues
Really keen to pass QA on this but a few QA tool warnings to address first please: WARN C4/Auth.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 387, now: 388) OK C4/Auth_with_ldap.pm OK C4/Auth_with_shibboleth.pm OK installer/data/mysql/atomicupdate/bug_36822.pl WARN t/db_dependent/Auth_with_ldap.t WARN tidiness The file is less tidy than before (bad/messy lines before: 104, now: 105) WARN t/db_dependent/Auth_with_shibboleth.t WARN tidiness The file is less tidy than before (bad/messy lines before: 109, now: 116) Processing additional checks OK! * Was this bug sponsored by anyone? Running tests (2) * Proving /kohadevbox/koha/t/db_dependent/Auth_with_shibboleth.t OK! * Proving /kohadevbox/koha/t/db_dependent/Auth_with_ldap.t OK!
Created attachment 173903 [details] [review] Bug 36822: Shibboleth - Add tests prove t/db_dependent/Auth_with_shibboleth.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173904 [details] [review] Bug 36822: Shibboleth - Add discard_changes Test plan: 1) Apply only Shibboleth tests patch 2) Run that test file: $ prove t/db_dependent/Auth_with_shibboleth.t 3) Verify tests fail with the updated_on error message. 4) Apply this patch. Repeat 2) Verify tests now pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173905 [details] [review] Bug 36822: LDAP - Rearrange Auth_with_ldap.pm so it picks up on mock config from test file Without this rearrangement, C4::Context->config('useldapserver') is always 0. It seems that because its inside a BEGIN block, the mock coming from .t file does not get picked up Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173906 [details] [review] Bug 36822: LDAP - Add test Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173907 [details] [review] Bug 36822: LDAP - Add discard_changes Test plan: 1) Apply only LDAP rearrangement patch 2) Run that test file (to prove the rearrangement does not break tests): $ prove t/db_dependent/Auth_with_ldap.t 3) Verify tests pass 4) Apply LDAP test patch on top 5) Run the test file. Notice it fails. 6) Apply this patch. Run LDAP tests again. Verify tests pass. Co-authored-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 173908 [details] [review] Bug 36822: atomicupdate Fix any existing updated_on issues
Created attachment 173909 [details] [review] Bug 36822: (QA follow-up): Fix now time in tests This is to prevent the scenario of tests failing if they take long to run, e.g.: # Failed test 'updated_on correctly saved on newly created user' # at t/db_dependent/Auth_with_shibboleth.t line 317. # Structures begin differing at: # ->[0] = '2024-11-04 11:20:13' # ->[0] = '2024-11-04 11:20:14
(In reply to Aleisha Amohia from comment #24) > Really keen to pass QA on this but a few QA tool warnings to address first > please: Thank you Aleisha, I've squashed perltidy tidyness to the tests files patches to address the warnings of Auth_with_ldap.t and Auth_with_shibboleth.t I've also submitted a follow-up to prevent tests from failing if enough time has gone by between user being created and the test running (stumbled upon this while revisiting). I've spent an unreasonable amount of time trying fix the tidy warning of 1 untidy line in C4/Auth.pm and I can't fix it barring tidying the whole file. I'm happy to have someone else fix this tidy warning for me on C4/Auth.pm but I also don't think this should be reason enough to block these patches.
Let's not hold this one up just for the Tidy challenges..
Pushed for 24.11! Well done everyone, thank you!
This affects our libraries as early as 23.05 so have added the relevant keywords
This causes an issue with MySQL 8 that doesn't even allow comparisons to an invalid date: 11:19:21 koha_1 | Upgrade to 24.06.00.056 [10:19:04]: Bug 36822 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36822) - Sanitize borrowers.updated_on 11:19:21 koha_1 | ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::st execute failed: Incorrect TIMESTAMP value: '0000-00-00 00:00:00' at /kohadevbox/koha/C4/Installer.pm line 742 We have discussed several solutions. I guess we can assume a database that strict doesn't have invalid dates, so a try-catch was what we thought would work best.
Created attachment 174523 [details] [review] Bug 36822: (follow-up) Catch MySQL 8 failure in update We add a try/catch around the call to fix 0000-00-00 dates in the database. MySQL 8.0 throws an error on the WHERE clause for such a search. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 174525 [details] [review] Bug 36822: (follow-up) Catch MySQL 8 failure in update We add a CAST to the fetch of 0000-00-00 dates in the database. This prevents an error in MySQL 8.0 throws htat aborts the update. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Follow-up "Catch MySQL 8 failur in update" picked for main.
Backported to 24.05.x for upcoming 24.05.06