https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Spotted in 20.11.06 - haven't confirmed in master yet. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #3 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #4 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122424 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122424&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 122461 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122461&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122424|0 |1 is obsolete| | --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 122603 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122603&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #8 from David Nind <david@davidnind.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122772|0 |1 is obsolete| | --- Comment #11 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 122799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122799&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #13 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #15 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #17 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #21 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #24 from 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 --- Comment #25 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |21.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com Version(s)|21.11.00 |21.11.00,21.05.03 released in| | Status|Pushed to master |Pushed to stable --- Comment #27 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to 21.05.x for 21.05.03 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |RESOLVED Resolution|--- |FIXED CC| |victor@tuxayo.net --- Comment #29 from Victor Grousset/tuxayo <victor@tuxayo.net> --- Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed. -- You are receiving this mail because: You are watching all bug changes.