Bug 24379 - Patron login attempts happen to be NULL instead of 0
Summary: Patron login attempts happen to be NULL instead of 0
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25664
  Show dependency treegraph
 
Reported: 2020-01-08 19:50 UTC by Kelly McElligott
Modified: 2020-08-05 00:10 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03, 19.11.09


Attachments
Bug 24379: Make login_attempts not nullable (4.96 KB, patch)
2020-01-08 21:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24379: Fix the test (1.25 KB, patch)
2020-01-09 12:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24379: Don't drop default of 0 for login attempts (1.46 KB, patch)
2020-06-03 12:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24379: Make login_attempts not nullable (6.56 KB, patch)
2020-07-13 17:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24379: Make login_attempts not nullable (6.62 KB, patch)
2020-07-13 17:49 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 24379: Make login_attempts not nullable (6.68 KB, patch)
2020-07-15 06:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24379: Fix failing tests (2.64 KB, patch)
2020-07-20 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2020-01-08 19:50:12 UTC
When a patron registers for a new account, their loginattempt value is NULL and not zero.  This is a change since 18.11.

Since this value is NULL, a patron cannot reset their password due to this NULL value.
Comment 1 Nick Clemens 2020-01-08 21:41:42 UTC
Created attachment 97058 [details] [review]
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!
Comment 2 Nick Clemens 2020-01-08 21:42:12 UTC
Tests don't pass, something is missing
Comment 3 Jonathan Druart 2020-01-09 12:57:39 UTC
Created attachment 97073 [details] [review]
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.
Comment 4 Katrin Fischer 2020-04-05 11:51:51 UTC
Is this ready for testing?
Comment 5 Lucas Gass 2020-05-05 22:09:03 UTC
I cannot recreate this in master. When I create a new patron (either from the staff client or selfreg via OPAC) login_attempts are initially set to 0 and I am able to reset my password via OpacResetPassword.
Comment 6 Katrin Fischer 2020-05-05 22:20:00 UTC
I think it might have been fixed already:

kohastructure.sql:
`login_attempts` int(4) default 0, -- number of failed login attemps

updatedatabase:
14478 $DBversion = '16.12.00.037'; - with default 0

14666 $DBversion = '17.06.00.009'; - is a little weird:
14668     $dbh->do(q{
14669         ALTER TABLE borrowers MODIFY COLUMN login_attempts int(4) AFTER lang;
14670     });
14671     $dbh->do(q{
14672         ALTER TABLE deletedborrowers MODIFY COLUMN login_attempts int(4) AFTER lang;
14673     });

Is it possible the 17.06 change broke the default 0 for updated installations? That could explain, why you don't see the issue on master.
Comment 7 Katrin Fischer 2020-05-05 22:20:38 UTC
Forget the first line... I think it's possibly not fixed, started the comment before looking at updatedatabase.
Comment 8 Kelly McElligott 2020-06-02 16:50:57 UTC
This is currently still the case on 19.11.  When creating a new patron, the login attempts are set to NULL making the password recovery not work on the OPAC.
Comment 9 Jonathan Druart 2020-06-03 09:47:14 UTC
I have tried on both master and 19.11.x and login_attempts contains "0"
Comment 10 Nick Clemens 2020-06-03 12:18:43 UTC
Created attachment 105522 [details] [review]
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)
Comment 11 David Cook 2020-07-09 05:16:15 UTC
(In reply to Jonathan Druart from comment #9)
> I have tried on both master and 19.11.x and login_attempts contains "0"

I have tried on 19.11.05 and login_attempts is null.
Comment 12 David Cook 2020-07-09 05:23:03 UTC
No change to login_attempts between 19.11.05 and 19.11.07...

No change to login_attempts between 19.11.05 and origin/19.11.x...

Ah, but this is an upgraded database.

I see in kohastructure.sql that login_attempts has "default 0" but in my actual upgraded database it has "default null". 

In 16.12.00.037, there is the following (for Bug 18314 - Add FailedLoginAttempts and borrowers.login_attempts):
ALTER TABLE borrowers ADD COLUMN login_attempts INT(4) DEFAULT 0 AFTER lastseen

In 17.06.00.009, the DEFAULT 0 was removed (for Bug 19344 -  Reorder lang and login_attempts in the [deleted]borrowers tables):
ALTER TABLE borrowers MODIFY COLUMN login_attempts int(4) AFTER lang;

So that explains that.

New databases will be fine, but upgraded databases will not be.
Comment 13 David Cook 2020-07-09 05:28:01 UTC
At a glance, these patches look good.

They're a bit of a pain to test though.
Comment 14 David Cook 2020-07-09 05:29:34 UTC
Sorry, Nick, but can't apply the patch:

error: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql).
error: could not build fake ancestor
Patch failed at 0001 Bug 24379: Make login_attempts not nullable
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-24379-Make-loginattempts-not-nullable-I8ewqG.patch
Comment 15 Nick Clemens 2020-07-13 17:26:23 UTC
Created attachment 106846 [details] [review]
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)
Comment 16 Lucas Gass 2020-07-13 17:49:37 UTC
Created attachment 106847 [details] [review]
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 <lucas@bywatersolutions.com>
Comment 17 Katrin Fischer 2020-07-15 05:59:31 UTC
I wondered if we should have a statement for changing the NULL to 0 on existing patrons, but it seems to automatically happen in my testing.
Comment 18 Katrin Fischer 2020-07-15 06:01:03 UTC
Created attachment 106890 [details] [review]
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 <lucas@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Jonathan Druart 2020-07-20 12:07:53 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 20 Jonathan Druart 2020-07-20 13:17:02 UTC
Created attachment 107099 [details] [review]
Bug 24379: Fix failing tests

kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/Koha/Patrons/Import.t
t/db_dependent/Koha/Patrons/Import.t .. 2/159
 #   Failed test 'No warning raised by import_patrons'
 #   at t/db_dependent/Koha/Patrons/Import.t line 171.
 # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2.
 # found warning: Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3a> line 2.
 # didn't expect to find a warning
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_2> line 2.
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_2> line 2.
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3> line 2.
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_3> line 2.
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_4> line 2.
Argument "" isn't numeric in numeric eq (==) at /usr/share/perl5/DBIx/Class/Row.pm line 1018, <$handle_4> line 2.
t/db_dependent/Koha/Patrons/Import.t .. 152/159 # Looks like you failed 1 test of 159.
t/db_dependent/Koha/Patrons/Import.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/159 subtests

Test Summary Report
-------------------
t/db_dependent/Koha/Patrons/Import.t (Wstat: 256 Tests: 159 Failed: 1)
  Failed test:  39
  Non-zero exit status: 1
Files=1, Tests=159,  3 wallclock secs ( 0.04 usr  0.01 sys +  2.11 cusr  0.24 csys =  2.40 CPU)
Result: FAIL
Comment 21 Jonathan Druart 2020-07-20 15:49:26 UTC
Follow-up pushed to master for 20.11
Comment 22 Aleisha Amohia 2020-08-03 22:26:55 UTC
not backporting to 19.11.x because of the db updates.
Comment 23 Nick Clemens 2020-08-04 12:03:55 UTC
(In reply to Aleisha Amohia from comment #22)
> not backporting to 19.11.x because of the db updates.

Hi Aleisha,

The patches here fix a problem caused by an upgrade in 17.06.00.009

We need the DB updates to restore the intended settings and prevent password reset problems for upgraded databases.

Would you reconsider this one?

-Nick
Comment 24 Aleisha Amohia 2020-08-05 00:10:20 UTC
backported to 19.11.x for 19.11.09