I can't confirm on master. Which version did you test with Emmi?

Oh, and maybe check the logs - there could be a more meaningful error there.

(In reply to Katrin Fischer from comment #1)
> I can't confirm on master. Which version did you test with Emmi?

I tested on both master and 21.11 which we use. 

It seems this happens because in batchMod.pl line 269 we don't pass biblionumber as parameter to Koha::UI::Form::Builder::Item->new and thus Koha fails to fetch biblios marc record. 

However when I tested this on "clean" master database where default framework has no values in it error wasn't raised.

Hi Emmi,
yes, that's it:

* Update default framework 952 and add a default value
  Example: Add REF as default for 952$8 with standard sample data
* Collect some barcodes
* Go to Tools > Batch item modification
* Enter barcodes and check checkbox for using default values
* Submit form and boom!

Can't call method "field" on an undefined value at /kohadevbox/koha/Koha/UI/Form/Builder/Item.pm line 165

Created attachment 138124 [details] [review]
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500

When one tries to batch mod items and has "Use default values"
checkbox checked on and their default framework contains
default values, Koha dies on error 500. In logs it reads:

Can't call method "field" on an undefined value at
.../Koha/Koha/UI/Form/Builder/Item.pm line 164.

This happens because in batchMod.pl line 269 we don't pass
biblionumber as parameter to Koha::UI::Form::Builder::Item->new
and thus Koha fails to fetch biblios marc record.

To test:
1. Update default framework 952 and add a default value
* Example: Add REF as default for 952$8 with standard sample data
2. Find some item(s) to modify
3. Provide barcodes of those item(s) and check checkbox "Use default values"
4. Hit "Continue"
=> error 500 is raised
5. Apply patch.
6. Repeat steps 3. and 4.

Sponsored-by: Koha-Suomi Oy

Thx for the patch!

I cannot reproduce the problematic behaviour either in a sandbox nor on my local dev install with the steps from comment 4. In all cases, I am brought to the batch item modification page without problem and am able to launch the batch modification.

I do get a 500 error when I click on the "View detail of the enqueued job" link to see the background job.

I tried with and without the patch and get the same behaviour...

(In reply to Caroline Cyr La Rose from comment #7)
> I cannot reproduce the problematic behaviour either in a sandbox nor on my
> local dev install with the steps from comment 4. In all cases, I am brought
> to the batch item modification page without problem and am able to launch
> the batch modification.
> 
> I do get a 500 error when I click on the "View detail of the enqueued job"
> link to see the background job.
> 
> I tried with and without the patch and get the same behaviour...

Hmm, it could also be that error is actually raised when subfield has authorised value linked to it? However at least for me saving authorised value to subfield is currently broken on master (see bug 31238) so it might be little tricky to reproduce.

I'm unable to reproduce 500 error by clicking "View detail of the enqueued job". Do you know what logs say when this happens?

I can no longer replicate the problem with the test plan that worked before :(

(In reply to Katrin Fischer from comment #9)
> I can no longer replicate the problem with the test plan that worked before
> :(

Hmm, how about if you save authorised value for 952$8?

No 500 error is produced. Instead, it says "The job has been enqueued! It will be processed as soon as possible."

It seems this patch reawakens bug 21141. I'll rework this if needed since it might be that some other patch already fixed this.

Well this is confusing. I was able to repeat this bug again by adding a value in syspref "itemcallnumber" e.g. 084a. This triggers if-statement in Koha/Koha/UI/Form/Builder/Item.pm starting from line 151 which eventually leads Koha to crash on line 165. It seems that having a default value in default framework had nothing to with this at all since I too am unable to trigger this bug again with just default values. 

Here's an updated test plan:

To test:
1. Confirm you have/set value to syspref "itemcallnumber" e.g. 084a
2. Find some item(s) to modify
3. Provide barcodes of those item(s) and check checkbox "Use default values"
4. Hit "Continue"
=> error 500 is raised

Created attachment 140518 [details] [review]
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500

When one tries to batch mod items and has "Use default values"
checkbox checked on and they have value in syspref "itemcallnumber",
Koha dies on error 500. In logs it reads:

Can't call method "field" on an undefined value at
.../Koha/Koha/UI/Form/Builder/Item.pm line 164.

This happens because in batchMod.pl line 269 we don't pass
biblionumber as parameter to Koha::UI::Form::Builder::Item->new
and thus Koha fails to fetch biblios marc record.

To test:
1. Confirm you have/set value to syspref "itemcallnumber" e.g. 084a
2. Find some item(s) to modify
3. Provide barcodes of those item(s) and check checkbox "Use default values"
4. Hit "Continue"
=> error 500 is raised
5. Apply patch.
6. Repeat steps 3. and 4.
=> no error is raised

Sponsored-by: Koha-Suomi Oy

Created attachment 140602 [details] [review]
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500

When one tries to batch mod items and has "Use default values"
checkbox checked on and they have value in syspref "itemcallnumber",
Koha dies on error 500. In logs it reads:

Can't call method "field" on an undefined value at
.../Koha/Koha/UI/Form/Builder/Item.pm line 164.

This happens because in batchMod.pl line 269 we don't pass
biblionumber as parameter to Koha::UI::Form::Builder::Item->new
and thus Koha fails to fetch biblios marc record.

To test:
1. Confirm you have/set value to syspref "itemcallnumber" e.g. 084a
2. Find some item(s) to modify
3. Provide barcodes of those item(s) and check checkbox "Use default values"
4. Hit "Continue"
=> error 500 is raised
5. Apply patch.
6. Repeat steps 3. and 4.
=> no error is raised

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

QA: Looking here

=head3 new

    my $form = Koha::UI::Form::Builder::Item->new(
        {
            biblionumber => $biblionumber,
            item => $current_item,
        }
    );

Constructor.
biblionumber should be passed if we are creating a new item.
For edition, an hashref representing the item to edit item must be passed.

Hmm. Why did the author not just pick the biblionumber of the item?

$biblionumber = Koha::Items->find($itemnumbers[0])->unblessed->{biblionumber} if $itemnumbers[0];

Emmi, you should probably not change this parameter. It is passed in, although I guess normally you will have items from several biblio records.

Dont worry, I will adjust it now.

We are really wasting resources here. 

First we do:
my %itemdata = map { lc($_->{barcode}) => $_->{itemnumber} } @{ Koha::Items->search({ barcode => { -in => \@barcodelist } }, { columns => [ 'itemnumber', 'barcode' ] } )->unblessed };

Lets discard that info.
And later do:

sub build_table {
    my ( $self, $params ) = @_;
    my %itemnumbers_to_idx = map { $self->{itemnumbers}->[$_] => $_ } 0..$#{$self->{itemnumbers}};
    my $items = Koha::Items->search( { itemnumber => $self->{itemnumbers} } );

We are looking up all items again ;)

Created attachment 140694 [details] [review]
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500

When one tries to batch mod items and has "Use default values"
checkbox checked on and they have value in syspref "itemcallnumber",
Koha dies on error 500. In logs it reads:

Can't call method "field" on an undefined value at
.../Koha/Koha/UI/Form/Builder/Item.pm line 164.

This happens because in batchMod.pl line 269 we don't pass
biblionumber as parameter to Koha::UI::Form::Builder::Item->new
and thus Koha fails to fetch biblios marc record.

To test:
1. Confirm you have/set value to syspref "itemcallnumber" e.g. 084a
2. Find some item(s) to modify
3. Provide barcodes of those item(s) and check checkbox "Use default values"
4. Hit "Continue"
=> error 500 is raised
5. Apply patch.
6. Repeat steps 3. and 4.
=> no error is raised

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Created attachment 140695 [details] [review]
Bug 31154: (QA follow-up) Fix UI form builder

The change in batchMod.pl is not needed when we fix the builder.
But we could at least add a comment there!

Note that passing the biblionumber of the first item does not
make sense since we are modifying items from several biblio
records normally. We most probably do not want this MARC
record's itemcallnumber (via the syspref) in all our items.

Test plan:
Test batchMod with and without the Populate fields checkbox.
Fill syspref 'itemcallnumber' with e.g. 084a.
Check Populate, and try again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Jonathan, left comment17 and comment19 for your consideration :)

Pushed to master for 22.11.

Nice work everyone, thanks!

(In reply to Marcel de Rooy from comment #22)
> Jonathan, left comment17 and comment19 for your consideration :)

Why didn't you add that on the original bug report? :)

(In reply to Marcel de Rooy from comment #17)
> =head3 new
> 
>     my $form = Koha::UI::Form::Builder::Item->new(
>         {
>             biblionumber => $biblionumber,
>             item => $current_item,
>         }
>     );
> 
> Constructor.
> biblionumber should be passed if we are creating a new item.
> For edition, an hashref representing the item to edit item must be passed.
> 
> Hmm. Why did the author not just pick the biblionumber of the item?

The item is not created yet, and so does not belongs to the biblio yet. But I agree we could add it to the item hashref, remove the param and use item->{biblionumber} instead.

(In reply to Marcel de Rooy from comment #19)
> We are really wasting resources here. 
> 
> First we do:
> my %itemdata = map { lc($_->{barcode}) => $_->{itemnumber} } @{
> Koha::Items->search({ barcode => { -in => \@barcodelist } }, { columns => [
> 'itemnumber', 'barcode' ] } )->unblessed };
> 
> Lets discard that info.
> And later do:
> 
> sub build_table {
>     my ( $self, $params ) = @_;
>     my %itemnumbers_to_idx = map { $self->{itemnumbers}->[$_] => $_ }
> 0..$#{$self->{itemnumbers}};
>     my $items = Koha::Items->search( { itemnumber => $self->{itemnumbers} }
> );
> 
> We are looking up all items again ;)

From controller we are only fetching itemnumber and barcode. That's extra processing for the "max items to display" feature. From the module we are fetching everything. I am expecting the query from the controller to be very fast, using DBMS indexes.
What did we do before 28445 and friends?

This changes forgot to add unit tests btw.

(In reply to Jonathan Druart from comment #25)
> This changes forgot to add unit tests btw.

We have t/db_dependent/Koha/UI/Form/Builder/Item.t, but generate_subfield_form is not tested directly.
Cannot request imo to add tests for trivial changes where the original author did not add tests himself.

Created attachment 141973 [details] [review]
Bug 31154: Add test

Created attachment 141974 [details] [review]
Bug 31154: Avoid unecessary loop

We don't need to loop over the subfield and enter this block if there is
no record.

This is how I think we should fix this problem. Feel free to ignore the second patch, but first patch is test that can be pushed.

Thanks. Looking here now

Created attachment 142051 [details] [review]
Bug 31154: Add test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 142052 [details] [review]
Bug 31154: Avoid unecessary loop

We don't need to loop over the subfield and enter this block if there is
no record.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Tomás: Last two patches please.

Follow-ups pushed. Thanks

Backported to 22.05.x for upcoming 22.05.07 release

applied to 21.11 for 21.11.14

21.05.x doesn't seem affected.

Did I check right? from comment 20

>To test:
>1. Confirm you have/set value to syspref "itemcallnumber" e.g. 084a
>2. Find some item(s) to modify
>3. Provide barcodes of those item(s) and check checkbox "Use default values"
I picked items that already had barcodes and checked "Populate fields with default values from default framework"
from sample data: 39999000012972 and 39999000012996
>4. Hit "Continue"
>=> error 500 is raised
No error. (no patches applied of course)


For now, unless I made a mistake in testing:
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.

*** Bug 32446 has been marked as a duplicate of this bug. ***