From 692a5f5f292cc39a28681a6e66e293e5fd0b9254 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 8 Jan 2020 20:59:38 +0000 Subject: [PATCH] Bug 24379: Make login_attempts not nullable While the column defaults to 0 in Koha::Object->store we set to NULL if NULLABLE When trying to reset a patrons password we check that the account is not administratively locked: login_attempts != -1 This query does not return rows where login_attempts IS NULL. It will return accounts where login_attempts = 0 Let's default to 0 like we intend To test: 1 - Create a new patron 2 - Note their login_attempts is NULL SELECT login_attempts FROM borrowers ORDER BY borrowernumber DESC LIMIT 1 3 - Enable OpacResetPassword 4 - Attempt to reset password before logging in, you cannot 5 - Apply patch, updatedatabase, restart_all, update schema 6 - Create another patron 7 - Their login attempts should be 0 8 - Attempt to reset password, it works! Bug 24379: Fix the test First we create a patron using TestBuilder to get a hashref of valid info. Then we delete it and create a new patron using Koha::Patron->new Once stored, we should call discard_changes to make the calculated values available in the currenct object. Bug 24379: Don't drop default of 0 for login attempts When moving the column we drop the default, this means that DBs upgraded form earlier versions get the wrong values set To test: 1 - Checkout 16.11.x 2 - Reset all 3 - Checkout master 4 - updatedatabase 5 - SHOW CREATE TABLE borrowers; 6 - Note the column login_attempts defaults to NULL 7 - Apply patch(es) 8 - Repeat 9 - Now it defaults ot 0 (and has NOT NULL if applied all) Signed-off-by: Lucas Gass Signed-off-by: Katrin Fischer Signed-off-by: Jonathan Druart (cherry picked from commit 7bb31cffae27dc398a7b4a4decf3623ec65478ac) Signed-off-by: Lucas Gass (cherry picked from commit 6d675b61ad2dfd8fa6cfd0f58a9ebb8d534bd358) Signed-off-by: Aleisha Amohia (cherry picked from commit f4899bafe35ad8e0c2e9af55068e6ee7f4a7e36e) Signed-off-by: Victor Grousset/tuxayo --- .../make_login_attempts_not_nullable.perl | 9 ++++++++ installer/data/mysql/kohastructure.sql | 4 ++-- installer/data/mysql/updatedatabase.pl | 4 ++-- t/db_dependent/Koha/Patron.t | 21 ++++++++++++++++++- 4 files changed, 33 insertions(+), 5 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/make_login_attempts_not_nullable.perl diff --git a/installer/data/mysql/atomicupdate/make_login_attempts_not_nullable.perl b/installer/data/mysql/atomicupdate/make_login_attempts_not_nullable.perl new file mode 100644 index 0000000000..deaf78c564 --- /dev/null +++ b/installer/data/mysql/atomicupdate/make_login_attempts_not_nullable.perl @@ -0,0 +1,9 @@ +$DBversion = 'XXX'; +if( CheckVersion( $DBversion ) ) { + $dbh->do( "UPDATE borrowers SET login_attempts=0 WHERE login_attempts IS NULL" ); + $dbh->do( "ALTER TABLE borrowers MODIFY COLUMN login_attempts int(4) NOT NULL DEFAULT 0" ); + $dbh->do( "UPDATE deletedborrowers SET login_attempts=0 WHERE login_attempts IS NULL" ); + $dbh->do( "ALTER TABLE deletedborrowers MODIFY COLUMN login_attempts int(4) NOT NULL DEFAULT 0" ); + print "Upgrade to $DBversion done (Bug 24379 - Set login_attempts NOT NULL)\n"; + SetVersion( $DBversion ); +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 6f25a2953b..201f5c23c4 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -607,7 +607,7 @@ CREATE TABLE `deletedborrowers` ( -- stores data related to the patrons/borrower `updated_on` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, -- time of last change could be useful for synchronization with external systems (among others) `lastseen` datetime default NULL, -- last time a patron has been seen (connected at the OPAC or staff interface) `lang` varchar(25) NOT NULL default 'default', -- lang to use to send notices to this patron - `login_attempts` int(4) default 0, -- number of failed login attemps + `login_attempts` int(4) NOT NULL default 0, -- number of failed login attemps `overdrive_auth_token` MEDIUMTEXT default NULL, -- persist OverDrive auth token `anonymized` TINYINT(1) NOT NULL DEFAULT 0, -- flag for data anonymization KEY borrowernumber (borrowernumber), @@ -1642,7 +1642,7 @@ CREATE TABLE `borrowers` ( -- this table includes information about your patrons `updated_on` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, -- time of last change could be useful for synchronization with external systems (among others) `lastseen` datetime default NULL, -- last time a patron has been seen (connected at the OPAC or staff interface) `lang` varchar(25) NOT NULL default 'default', -- lang to use to send notices to this patron - `login_attempts` int(4) default 0, -- number of failed login attemps + `login_attempts` int(4) NOT NULL default 0, -- number of failed login attemps `overdrive_auth_token` MEDIUMTEXT default NULL, -- persist OverDrive auth token `anonymized` TINYINT(1) NOT NULL DEFAULT 0, -- flag for data anonymization UNIQUE KEY `cardnumber` (`cardnumber`), diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 6cfc3086a7..18d49b4712 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -14666,10 +14666,10 @@ if( CheckVersion( $DBversion ) ) { $DBversion = '17.06.00.009'; if( CheckVersion( $DBversion ) ) { $dbh->do(q{ - ALTER TABLE borrowers MODIFY COLUMN login_attempts int(4) AFTER lang; + ALTER TABLE borrowers MODIFY COLUMN login_attempts int(4) DEFAULT 0 AFTER lang; }); $dbh->do(q{ - ALTER TABLE deletedborrowers MODIFY COLUMN login_attempts int(4) AFTER lang; + ALTER TABLE deletedborrowers MODIFY COLUMN login_attempts int(4) DEFAULT 0 AFTER lang; }); SetVersion( $DBversion ); diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index c9f4e07d37..87f2c7f33e 100644 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 1; +use Test::More tests => 2; use Test::Exception; use Koha::Database; @@ -54,3 +54,22 @@ subtest 'is_superlibrarian() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'login_attempts tests' => sub { + plan tests => 1; + + $schema->storage->txn_begin; + + my $patron = $builder->build_object( + { + class => 'Koha::Patrons', + } + ); + my $patron_info = $patron->unblessed; + $patron->delete; + delete $patron_info->{login_attempts}; + my $new_patron = Koha::Patron->new($patron_info)->store; + is( $new_patron->discard_changes->login_attempts, 0, "login_attempts defaults to 0 as expected"); + + $schema->storage->txn_rollback; +}; -- 2.39.5