Bug 23217

Summary: Batch patron modification shows database errors when no Attribute provided
Product: Koha Reporter: David Cook <dcook>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.03
Bug Depends on: 19673    
Bug Blocks:    
Attachments: Bug 23217: Remove SQL error when batch patron mod and not attribute selected
Bug 23217: Remove SQL error when batch patron mod and not attribute selected
Bug 23217: Remove SQL error when batch patron mod and not attribute selected

Description David Cook 2019-06-26 07:01:41 UTC
I think that I've found a regression caused by https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19673. 

I'm investigating at the moment. I have the default SHOW_BCODE attribute category set up, and when running with the Environmental Variable DEBUG=1 in Apache and using Chrome on Windows 10, I'm getting database errors like this:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`kohadb`.`borrower_attributes`, CONSTRAINT `borrower_attributes_ibfk_2` FOREIGN KEY (`code`) REFERENCES `borrower_attribute_types` (`code`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO borrower_attributes SET attribute = ?, code = ?, borrowernumber = ?" with ParamValues: 0=undef, 1="", 2="1"] at /koha/lib/C4/Members/Attributes.pm line 294.

Note this is a modified Koha so the line number may not be relevant. It points to the final line of this function:

sub UpdateBorrowerAttribute {
    my ( $borrowernumber, $attribute ) = @_;

    DeleteBorrowerAttribute $borrowernumber, $attribute;

    my $dbh = C4::Context->dbh;
    my $query = "INSERT INTO borrower_attributes SET attribute = ?, code = ?, borrowernumber = ?";
    my @params = ( $attribute->{attribute}, $attribute->{code}, $borrowernumber );
    my $sth = $dbh->prepare( $query );

    $sth->execute( @params );
}
Comment 1 David Cook 2019-06-26 07:08:51 UTC
I find it odd that no one else has encountered this over the last year, but I'm guessing it might have to do with most people not running with RaiseError turned on (by DEBUG=1).

I'm not sure what the best fix would be. 

One option is to maybe change the Javascript in modborrowers.tt.

Another would be to update UpdateBorrowerAttribute() to look up the borrower_attribute_type/code before trying to do an insert, although that could have performance impacts on batch operations.
Comment 2 David Cook 2019-06-26 07:10:25 UTC
(In reply to David Cook from comment #1)
> I find it odd that no one else has encountered this over the last year, but
> I'm guessing it might have to do with most people not running with
> RaiseError turned on (by DEBUG=1).
> 

I can confirm that turning off DEBUG=1 makes the errors disappear, and turning it back on makes the errors re-appear.
Comment 3 David Cook 2019-06-26 07:20:13 UTC
(In reply to David Cook from comment #1)
> Another would be to update UpdateBorrowerAttribute() to look up the
> borrower_attribute_type/code before trying to do an insert, although that
> could have performance impacts on batch operations.

Oh actually in tools/modborrower.pl, we're already doing the following:

my $attr_type = C4::Members::AttributeTypes->fetch( $_ );

We should just do some better validations there. 

Oh... $attr_type should be null, but then we do the following:

++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $borrower_categorycode;

That doesn't make any errors, so I wonder if that's an autovivification issue... I'll take a quick look...
Comment 4 David Cook 2019-06-26 07:29:27 UTC
(In reply to David Cook from comment #3)
> That doesn't make any errors, so I wonder if that's an autovivification
> issue... I'll take a quick look...

Yep. That's exactly it. 

So need to first check the value of $attr_type when returned by C4::Members::AttributeTypes->fetch(). 

Just need to make sure the counter is still incremented correctly...
Comment 5 Jonathan Druart 2019-06-29 15:56:33 UTC
So were so close to the patch!
Comment 6 Jonathan Druart 2019-06-29 16:00:36 UTC
(In reply to Jonathan Druart from comment #5)
> So were so close to the patch!

*You* were so close to the patch!
Comment 7 Jonathan Druart 2019-06-29 16:01:08 UTC
Created attachment 91131 [details] [review]
Bug 23217: Remove SQL error when batch patron mod and not attribute selected

On the batch patron modification tool, if no patron attribute is selected then
an UPDATE query will be executed anyway. Indeed the form will send a
an empty "patron_attributes" parameter. We need to handle it.

Test plan:
Go to the patron modification tool
Enter a cardnumber
Change a field (like city)
Do not change anything in the patron's attributes
Save
=> Without this patch a SQL error is generated:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`borrower_attributes`, CONSTRAINT `borrower_attributes_ibfk_2` FOREIGN KEY (`code`) REFERENCES `borrower_attribute_types` (`code`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO borrower_attributes SET attribute = ?, code = ?, borrowernumber = ?" with ParamValues: 0=undef, 1="", 2="51"] at /home/vagrant/kohaclone/C4/Members/Attributes.pm line 287.

=> With this patch applied you will not see it

You should also test that you can modify patron's attributes ;)
Comment 8 David Cook 2019-07-01 00:05:27 UTC
Thanks, Jonathan. I'm working from home with a sick child today, but I'll take a look at signing off your patch when I'm back in the office (with a full size keyboard instead of a tiny netbook keyboard).
Comment 9 Hayley Pelham 2019-07-26 02:16:26 UTC
Created attachment 91785 [details] [review]
Bug 23217: Remove SQL error when batch patron mod and not attribute selected

On the batch patron modification tool, if no patron attribute is selected then
an UPDATE query will be executed anyway. Indeed the form will send a
an empty "patron_attributes" parameter. We need to handle it.

Test plan:
Go to the patron modification tool
Enter a cardnumber
Change a field (like city)
Do not change anything in the patron's attributes
Save
=> Without this patch a SQL error is generated:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`borrower_attributes`, CONSTRAINT `borrower_attributes_ibfk_2` FOREIGN KEY (`code`) REFERENCES `borrower_attribute_types` (`code`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO borrower_attributes SET attribute = ?, code = ?, borrowernumber = ?" with ParamValues: 0=undef, 1="", 2="51"] at /home/vagrant/kohaclone/C4/Members/Attributes.pm line 287.

=> With this patch applied you will not see it

You should also test that you can modify patron's attributes ;)

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 10 Nick Clemens 2019-07-26 12:55:20 UTC
Created attachment 91808 [details] [review]
Bug 23217: Remove SQL error when batch patron mod and not attribute selected

On the batch patron modification tool, if no patron attribute is selected then
an UPDATE query will be executed anyway. Indeed the form will send a
an empty "patron_attributes" parameter. We need to handle it.

Test plan:
Go to the patron modification tool
Enter a cardnumber
Change a field (like city)
Do not change anything in the patron's attributes
Save
=> Without this patch a SQL error is generated:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`borrower_attributes`, CONSTRAINT `borrower_attributes_ibfk_2` FOREIGN KEY (`code`) REFERENCES `borrower_attribute_types` (`code`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO borrower_attributes SET attribute = ?, code = ?, borrowernumber = ?" with ParamValues: 0=undef, 1="", 2="51"] at /home/vagrant/kohaclone/C4/Members/Attributes.pm line 287.

=> With this patch applied you will not see it

You should also test that you can modify patron's attributes ;)

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 11 Martin Renvoize 2019-07-26 15:19:21 UTC
Nice work!

Pushed to master for 19.11.00
Comment 12 Fridolin Somers 2019-08-09 12:50:06 UTC
Pushed to 19.05.x for 19.05.03