When adding a new library the pick-up location will always saves as "Yes", even when no was selected.
Spotted in 20.11.06 - haven't confirmed in master yet.
commit f3e4b5bbb6b02d7bb6e89d84c0379666eb1e704f Bug 16154: CGI->multi_param - Force scalar context 124 my $library = Koha::Library->new( 125 { branchcode => $branchcode, 126 ( map { $_ => scalar $input->param($_) || undef } @fields ) 127 } 128 ); Branches.pickup_location has a default = 1 in DB. We should not set to undef if 0.
This is (very?) bad. When a new library is created, all the non-filled (empty string) fields that are "text" at DB level will be set to NULL. BUT after they are edited, they are set to an empty string.
Created attachment 122424 [details] [review] Bug 28567: Fix 0 vs "" vs undef on the admin library form There are two things here: * Branches.pickup_location has a default = 1 in DB, we should not set to undef if 0 or it will be set to 1 when stored. * The other fields are all text (varchar, mediumtext or longtext) and can be NULL. They are correct set to NULL when a new library is created but set to an empty string when the library is modified. That's not consistent Test plan: 0. Don't apply the patch 1. Create a new library, set pickup location to "No" 2. Save => Pickup location is set to YES => In DB notice that the different values you didn't fill in are set to NULL 3. Edit the library 4. Save => In DB notice that the different values you didn't fill in are now set to an empty string 5. Apply the patch, restart_all 6. Run the updatedatabase script => In DB all the empty string values are set to NULL 7. Repeat 1 to 4 and confirm that everything is now working as expected
Created attachment 122461 [details] Bug 28567 - Screenshot - no change I didn't notice any change after apply the patch (updatedatabase, flush_memcached, restart_all): - Still sets to Yes - Displays Address with empty information - see screenshot - Editing removes this information and can change pickup location to No
Created attachment 122603 [details] [review] Bug 28567: Fix 0 vs "" vs undef on the admin library form There are two things here: * Branches.pickup_location has a default = 1 in DB, we should not set to undef if 0 or it will be set to 1 when stored. * The other fields are all text (varchar, mediumtext or longtext) and can be NULL. They are correct set to NULL when a new library is created but set to an empty string when the library is modified. That's not consistent Test plan: 0. Don't apply the patch 1. Create a new library, set pickup location to "No" 2. Save => Pickup location is set to YES => In DB notice that the different values you didn't fill in are set to NULL 3. Edit the library 4. Save => In DB notice that the different values you didn't fill in are now set to an empty string 5. Apply the patch, restart_all 6. Run the updatedatabase script => In DB all the empty string values are set to NULL 7. Repeat 1 to 4 and confirm that everything is now working as expected
Hi David, there was an issue in the DB rev (because of branches.issuing). Sorry about that. (In reply to David Nind from comment #5) > I didn't notice any change after apply the patch (updatedatabase, > flush_memcached, restart_all): > - Still sets to Yes Now you should be able to create a library with "pickup location" set to "No"
With the updated patch, if I create a new library with 'Yes' the 'Ph:', 'Fax:' etc still show when no details are entered. You have to edit again to remove. 'No' works as expected.
Created attachment 122772 [details] [review] Bug 28567: Fix 0 vs "" vs undef on the admin library form There are two things here: * Branches.pickup_location has a default = 1 in DB, we should not set to undef if 0 or it will be set to 1 when stored. * The other fields are all text (varchar, mediumtext or longtext) and can be NULL. They are correct set to NULL when a new library is created but set to an empty string when the library is modified. That's not consistent Test plan: 0. Don't apply the patch 1. Create a new library, set pickup location to "No" 2. Save => Pickup location is set to YES => In DB notice that the different values you didn't fill in are set to NULL 3. Edit the library 4. Save => In DB notice that the different values you didn't fill in are now set to an empty string 5. Apply the patch, restart_all 6. Run the updatedatabase script => In DB all the empty string values are set to NULL 7. Repeat 1 to 4 and confirm that everything is now working as expected
(In reply to David Nind from comment #8) > With the updated patch, if I create a new library with 'Yes' the 'Ph:', > 'Fax:' etc still show when no details are entered. You have to edit again to > remove. > > 'No' works as expected. Oops, yes! Sorry about that. Should be fixed now.
Created attachment 122799 [details] [review] Bug 28567: Fix 0 vs "" vs undef on the admin library form There are two things here: * Branches.pickup_location has a default = 1 in DB, we should not set to undef if 0 or it will be set to 1 when stored. * The other fields are all text (varchar, mediumtext or longtext) and can be NULL. They are correct set to NULL when a new library is created but set to an empty string when the library is modified. That's not consistent Test plan: 0. Don't apply the patch 1. Create a new library, set pickup location to "No" 2. Save => Pickup location is set to YES => In DB notice that the different values you didn't fill in are set to NULL 3. Edit the library 4. Save => In DB notice that the different values you didn't fill in are now set to an empty string 5. Apply the patch, restart_all 6. Run the updatedatabase script => In DB all the empty string values are set to NULL 7. Repeat 1 to 4 and confirm that everything is now working as expected Signed-off-by: Owen Leonard <oleonard@myacpl.org>
(In reply to Jonathan Druart from comment #3) > This is (very?) bad. > > When a new library is created, all the non-filled (empty string) fields that > are "text" at DB level will be set to NULL. > BUT after they are edited, they are set to an empty string. Not sure if this is very bad btw. It is inconsistent, yes. This report is doing two separate things? + $_ eq 'pickup_location' # Don't fallback to undef/NULL, default is 1 in DB + ? ( $_ => scalar $input->param($_) ) + : ( $_ => scalar $input->param($_) || undef ) This kind of construction in scripts like admin/branches will make our life not easier? Should this be a change done by the store sub of Library instead ? Note that if we adjust a library somewhere else too we now need to duplicate these lines again. And we could test it ?
(In reply to Marcel de Rooy from comment #12) > (In reply to Jonathan Druart from comment #3) > > This is (very?) bad. > > > > When a new library is created, all the non-filled (empty string) fields that > > are "text" at DB level will be set to NULL. > > BUT after they are edited, they are set to an empty string. > > Not sure if this is very bad btw. It is inconsistent, yes. > This report is doing two separate things? Separate but linked, the title of the patch is "0 vs '' vs undef". I think they should all be fixed here. > + $_ eq 'pickup_location' # Don't fallback to > undef/NULL, default is 1 in DB > + ? ( $_ => scalar $input->param($_) ) > + : ( $_ => scalar $input->param($_) || undef ) > > This kind of construction in scripts like admin/branches will make our life > not easier? That's usual per code, I don't see any problems with that. > Should this be a change done by the store sub of Library instead? No, that's controller job. > Note that if we adjust a library somewhere else too we now need to duplicate > these lines again. > And we could test it ? See above.
(In reply to Jonathan Druart from comment #13) > > Should this be a change done by the store sub of Library instead? > > No, that's controller job. Well, that is a clear expression but not an answer ;) Why not? Have a look at Koha::Object->store and look whats happening there for instance? Or what are we doing in Koha::Patron->store? Probably the list could be longer..
(In reply to Marcel de Rooy from comment #14) > (In reply to Jonathan Druart from comment #13) > > > Should this be a change done by the store sub of Library instead? > > > > No, that's controller job. > > Well, that is a clear expression but not an answer ;) > Why not? > > Have a look at Koha::Object->store and look whats happening there for > instance? > Or what are we doing in Koha::Patron->store? > Probably the list could be longer.. Because they have a TEXT datatype, that can be NULL OR "". IMO we should not force NULL, a text can be "". However here, the controller assumes that if the user didn't fill in the inputs then the values are undef/NULL. pickup_location is a boolean and store must explode if we are passing undef. Either it's not passed and it fallback to the default value in DB, or it's a boolean, or it's an invalid value and we expect the insert to fail. What we are doing in some of the modules is wrong, because of legacy code that has been moved. From Koha::Patron->store: 205 $self->relationship(undef) # We do not want to store an empty string in this field 206 if defined $self->relationship 207 and $self->relationship eq ""; Typically this is wrong :) 215 # Add expiration date if it isn't already there 216 unless ( $self->dateexpiry ) { 217 $self->dateexpiry( $self->category->get_expiry_date ); 218 } This is correct, if the value does not exist we calculate it. Do you agree with that?
This kind of thing in the template is confusing: [% IF !library || library.pickup_location == 1 %] We should take care of such a thing elsewhere. This is 'hidden business logic'. The html select does not make things easier in this case. If our "business logic" or "data model" says that pickup location defaults to True, I would say make that happen (otherwise NULL simply means zero or false). Perhaps via Object->new already? We can't leave this to store here, since the html select already forces us to make a decision. TEXT fields which are set back to NULL, often generate lots of uninitialized warnings all over. So I would not mind setting them to empty string. Doing the opposite in this dbrev is arguable. The principal discussion here exceeds the scope of this report. But It is not clear where the division is between controller (caller script) and controlling object module. What should we do in e.g. new and store, and what not? In this case I would opt for the Koha::Objects instead of copying the same code in controller scripts. Object:new # Remove the arguments which exist, are not defined but NOT NULL to use the default value my $columns_info = $schema->resultset( $class->_type )->result_source->columns_info; for my $column_name ( keys %$attributes ) { my $c_info = $columns_info->{$column_name}; next if $c_info->{is_nullable}; next if not exists $attributes->{$column_name} or defined $attributes->{$column_name}; delete $attributes->{$column_name}; } Why not set $attributes->{$column_name} = $c_info->{default_value} if defined and delete only if not? Object:store } else { # If cannot be null, get the default value # What if cannot be null and does not have a default value? Possible? $self->_result()->set_column($col => $columns_info->{$col}->{default_value}); Note: I saw a lot of date fields NOT NULL but without a SQL default.
(In reply to Marcel de Rooy from comment #16) > This kind of thing in the template is confusing: [% IF !library || > library.pickup_location == 1 %] > We should take care of such a thing elsewhere. This is 'hidden business > logic'. The html select does not make things easier in this case. > If our "business logic" or "data model" says that pickup location defaults > to True, I would say make that happen (otherwise NULL simply means zero or > false). Perhaps via Object->new already? We can't leave this to store here, > since the html select already forces us to make a decision. > > TEXT fields which are set back to NULL, often generate lots of uninitialized > warnings all over. So I would not mind setting them to empty string. > Doing the opposite in this dbrev is arguable. Yes, I've decided to keep the existing behaviour. > The principal discussion here exceeds the scope of this report. But It is > not clear where the division is between controller (caller script) and > controlling object module. What should we do in e.g. new and store, and what > not? The module must let you insert "" or undef. No fallback possible there. And must explode if you try to insert an invalid value, like if you assign "" to a boolean. > In this case I would opt for the Koha::Objects instead of copying the same > code in controller scripts. > > Object:new > # Remove the arguments which exist, are not defined but NOT NULL to > use the default value > my $columns_info = $schema->resultset( $class->_type > )->result_source->columns_info; > for my $column_name ( keys %$attributes ) { > my $c_info = $columns_info->{$column_name}; > next if $c_info->{is_nullable}; > next if not exists $attributes->{$column_name} or defined > $attributes->{$column_name}; > delete $attributes->{$column_name}; > } > Why not set $attributes->{$column_name} = $c_info->{default_value} if > defined and delete only if not? We just ignore if set. This is completely different code (we are dealing with NOT NULL values). What's the question exactly, what do you suggest? Can we discuss it somewhere else? :D > Object:store > } else { > # If cannot be null, get the default value > # What if cannot be null and does not have a default > value? Possible? > $self->_result()->set_column($col => > $columns_info->{$col}->{default_value}); > Note: I saw a lot of date fields NOT NULL but without a SQL default. Yes indeed, the "Possible?" could be removed.
(In reply to Jonathan Druart from comment #17) > > In this case I would opt for the Koha::Objects instead of copying the same > > code in controller scripts. > > > > Object:new > > # Remove the arguments which exist, are not defined but NOT NULL to > > use the default value > > my $columns_info = $schema->resultset( $class->_type > > )->result_source->columns_info; > > for my $column_name ( keys %$attributes ) { > > my $c_info = $columns_info->{$column_name}; > > next if $c_info->{is_nullable}; > > next if not exists $attributes->{$column_name} or defined > > $attributes->{$column_name}; > > delete $attributes->{$column_name}; > > } > > Why not set $attributes->{$column_name} = $c_info->{default_value} if > > defined and delete only if not? > > We just ignore if set. This is completely different code (we are dealing > with NOT NULL values). What's the question exactly, what do you suggest? Can > we discuss it somewhere else? :D This code comes into play since admin/branches calls: my $library = Koha::Library->new If Koha::Object->new here passed the default SQL value of 1, we could eliminate the mentioned confusing construction. Actually in the above comment even a reference to default value is made, but the code does not do it.
(In reply to Marcel de Rooy from comment #18) > (In reply to Jonathan Druart from comment #17) > > > In this case I would opt for the Koha::Objects instead of copying the same > > > code in controller scripts. > > > > > > Object:new > > > # Remove the arguments which exist, are not defined but NOT NULL to > > > use the default value > > > my $columns_info = $schema->resultset( $class->_type > > > )->result_source->columns_info; > > > for my $column_name ( keys %$attributes ) { > > > my $c_info = $columns_info->{$column_name}; > > > next if $c_info->{is_nullable}; > > > next if not exists $attributes->{$column_name} or defined > > > $attributes->{$column_name}; > > > delete $attributes->{$column_name}; > > > } > > > Why not set $attributes->{$column_name} = $c_info->{default_value} if > > > defined and delete only if not? > > > > We just ignore if set. This is completely different code (we are dealing > > with NOT NULL values). What's the question exactly, what do you suggest? Can > > we discuss it somewhere else? :D > > This code comes into play since admin/branches calls: > my $library = Koha::Library->new > If Koha::Object->new here passed the default SQL value of 1, we could > eliminate the mentioned confusing construction. > Actually in the above comment even a reference to default value is made, but > the code does not do it. I am sorry, Marcel, but I still don't get it. Can you be more explicit, maybe provide a patch? Which "confusing construction" are you talking about? The map introduced by this patch? It's just simple and easy Perl code. In Koha::Object->new we are making the DBMS use the default value (so defined in the DB schema) by *removing* the key of the attributes that is "NOT NULL" *but* not defined! Based on that, what I understand from your suggestion would be to remove 'pickup_location' from the constructor if it's equal to '1'. But that would be very bad (what if the default value is modified later?), so I guess you had something else in mind :)
(In reply to Jonathan Druart from comment #19) > > Which "confusing construction" are you talking about? The map introduced by > this patch? It's just simple and easy Perl code. Comment16 This kind of thing in the template is confusing: [% IF !library || library.pickup_location == 1 %] Actually, the longer I look at it, the better I could live with it ;) > In Koha::Object->new we are making the DBMS use the default value (so > defined in the DB schema) by *removing* the key of the attributes that is > "NOT NULL" *but* not defined! Yes, but we do not really use the default value. Thats my point. I would suggest to return 1 for pickup_location here. See comment16: Why not set $attributes->{$column_name} = $c_info->{default_value} if defined and delete only if not? > Based on that, what I understand from your suggestion would be to remove > 'pickup_location' from the constructor if it's equal to '1'. But that would > be very bad (what if the default value is modified later?), so I guess you > had something else in mind :) No thats not my idea.
(In reply to Marcel de Rooy from comment #20) > Yes, but we do not really use the default value. Thats my point. I would > suggest to return 1 for pickup_location here. > See comment16: > Why not set $attributes->{$column_name} = $c_info->{default_value} if > defined and delete only if not? Because we are in ->new, not in store :)
(In reply to Jonathan Druart from comment #21) > (In reply to Marcel de Rooy from comment #20) > > Yes, but we do not really use the default value. Thats my point. I would > > suggest to return 1 for pickup_location here. > > See comment16: > > Why not set $attributes->{$column_name} = $c_info->{default_value} if > > defined and delete only if not? > > Because we are in ->new, not in store :) Discussed on IRC. Lets stick to what DBIx does. ie not use defaults for new.
Created attachment 123247 [details] [review] Bug 28567: Fix 0 vs "" vs undef on the admin library form There are two things here: * Branches.pickup_location has a default = 1 in DB, we should not set to undef if 0 or it will be set to 1 when stored. * The other fields are all text (varchar, mediumtext or longtext) and can be NULL. They are correct set to NULL when a new library is created but set to an empty string when the library is modified. That's not consistent Test plan: 0. Don't apply the patch 1. Create a new library, set pickup location to "No" 2. Save => Pickup location is set to YES => In DB notice that the different values you didn't fill in are set to NULL 3. Edit the library 4. Save => In DB notice that the different values you didn't fill in are now set to an empty string 5. Apply the patch, restart_all 6. Run the updatedatabase script => In DB all the empty string values are set to NULL 7. Repeat 1 to 4 and confirm that everything is now working as expected Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
As noted before, I would have split this report in two. And I do not really see the need for replacing the empty strings here. But the bug is resolved.
(In reply to Marcel de Rooy from comment #24) > As noted before, I would have split this report in two. And I do not really > see the need for replacing the empty strings here. > But the bug is resolved. Thanks for your time QAing this one, Marcel. I added the DBrev to have all the values identical before and after this patch.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.03
Pushed to 20.11.x for 20.11.09
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
*** Bug 27197 has been marked as a duplicate of this bug. ***