Bug 28567 - Pick-up location is not saved correctly when creating a new library
Summary: Pick-up location is not saved correctly when creating a new library
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
: 27197 (view as bug list)
Depends on: 7534 16154
Blocks:
  Show dependency treegraph
 
Reported: 2021-06-15 15:04 UTC by Katrin Fischer
Modified: 2021-08-26 13:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This fixes an issue when adding a new library - the pick-up location was always saving as "Yes", even when no was selected.
Version(s) released in:
21.11.00,21.05.03,20.11.09


Attachments
Bug 28567: Fix 0 vs "" vs undef on the admin library form (3.80 KB, patch)
2021-06-25 10:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28567 - Screenshot - no change (10.00 KB, image/png)
2021-06-26 00:56 UTC, David Nind
Details
Bug 28567: Fix 0 vs "" vs undef on the admin library form (3.79 KB, patch)
2021-07-06 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28567: Fix 0 vs "" vs undef on the admin library form (3.79 KB, patch)
2021-07-12 08:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28567: Fix 0 vs "" vs undef on the admin library form (3.83 KB, patch)
2021-07-13 11:20 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 28567: Fix 0 vs "" vs undef on the admin library form (3.93 KB, patch)
2021-07-28 12:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2021-06-15 15:04:37 UTC
When adding a new library the pick-up location will always saves as "Yes", even when no was selected.
Comment 1 Katrin Fischer 2021-06-15 15:06:24 UTC
Spotted in 20.11.06 - haven't confirmed in master yet.
Comment 2 Jonathan Druart 2021-06-25 10:29:28 UTC
  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.
Comment 3 Jonathan Druart 2021-06-25 10:35:28 UTC
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.
Comment 4 Jonathan Druart 2021-06-25 10:49:27 UTC
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
Comment 5 David Nind 2021-06-26 00:56:00 UTC
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
Comment 6 Jonathan Druart 2021-07-06 08:33:29 UTC
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
Comment 7 Jonathan Druart 2021-07-06 08:35:27 UTC
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"
Comment 8 David Nind 2021-07-09 21:59:06 UTC
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.
Comment 9 Jonathan Druart 2021-07-12 08:23:14 UTC
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
Comment 10 Jonathan Druart 2021-07-12 08:23:36 UTC
(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.
Comment 11 Owen Leonard 2021-07-13 11:20:05 UTC
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>
Comment 12 Marcel de Rooy 2021-07-16 06:19:57 UTC
(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 ?
Comment 13 Jonathan Druart 2021-07-16 06:38:20 UTC
(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.
Comment 14 Marcel de Rooy 2021-07-16 07:19:57 UTC
(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..
Comment 15 Jonathan Druart 2021-07-16 08:17:53 UTC
(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?
Comment 16 Marcel de Rooy 2021-07-16 09:53:11 UTC
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.
Comment 17 Jonathan Druart 2021-07-16 10:26:24 UTC
(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.
Comment 18 Marcel de Rooy 2021-07-26 10:00:07 UTC
(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.
Comment 19 Jonathan Druart 2021-07-28 08:43:37 UTC
(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 :)
Comment 20 Marcel de Rooy 2021-07-28 11:14:56 UTC
(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.
Comment 21 Jonathan Druart 2021-07-28 11:45:35 UTC
(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 :)
Comment 22 Marcel de Rooy 2021-07-28 11:57:49 UTC
(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.
Comment 23 Marcel de Rooy 2021-07-28 12:01:26 UTC
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>
Comment 24 Marcel de Rooy 2021-07-28 12:02:42 UTC
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.
Comment 25 Jonathan Druart 2021-07-28 12:40:01 UTC
(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.
Comment 26 Jonathan Druart 2021-07-30 14:02:30 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 27 Kyle M Hall 2021-08-06 12:17:52 UTC
Pushed to 21.05.x for 21.05.03
Comment 28 Fridolin Somers 2021-08-11 19:18:45 UTC
Pushed to 20.11.x for 20.11.09
Comment 29 Victor Grousset/tuxayo 2021-08-16 20:09:24 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.
Comment 30 Jonathan Druart 2021-08-26 13:23:56 UTC
*** Bug 27197 has been marked as a duplicate of this bug. ***