[Koha-bugs] [Bug 28567] Pick-up location is not saved correctly when creating a new library

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28567

--- Comment #17 from Jonathan Druart <jonathan.druart+koha at 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.