Bug 31154 - Batch item modification fails when "Use default values" is checked
Summary: Batch item modification fails when "Use default values" is checked
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Emmi Takkinen
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-14 06:23 UTC by Emmi Takkinen
Modified: 2022-11-24 04:38 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.07, 21.11.14


Attachments
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500 (1.88 KB, patch)
2022-07-26 10:34 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500 (1.86 KB, patch)
2022-09-13 09:21 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500 (1.91 KB, patch)
2022-09-13 18:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 31154: Pass biblionumber to Koha::UI::Form::Builder::Item->new from batchMod.pl to prevent error 500 (1.94 KB, patch)
2022-09-16 09:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31154: (QA follow-up) Fix UI form builder (3.21 KB, patch)
2022-09-16 09:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31154: Add test (1.70 KB, patch)
2022-10-17 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31154: Avoid unecessary loop (1.37 KB, patch)
2022-10-17 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31154: Add test (1.79 KB, patch)
2022-10-18 11:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31154: Avoid unecessary loop (1.46 KB, patch)
2022-10-18 11:41 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 Emmi Takkinen 2022-07-14 06:23:17 UTC
When one tries to batch mod items and has "Use default values" checkbox checked on, 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.

To test:
1. Find item(s) to modify 
2. Provide barcodes of item(s) to modify and check checkbox "Use default values"
3. Hit "Continue"
=> error 500 is raised
Comment 1 Katrin Fischer 2022-07-15 09:21:07 UTC
I can't confirm on master. Which version did you test with Emmi?
Comment 2 Katrin Fischer 2022-07-15 09:21:40 UTC
Oh, and maybe check the logs - there could be a more meaningful error there.
Comment 3 Emmi Takkinen 2022-07-25 07:14:19 UTC
(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.
Comment 4 Katrin Fischer 2022-07-25 21:26:28 UTC
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
Comment 5 Emmi Takkinen 2022-07-26 10:34:26 UTC
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
Comment 6 Katrin Fischer 2022-07-26 12:23:43 UTC
Thx for the patch!
Comment 7 Caroline Cyr La Rose 2022-07-26 14:08:10 UTC
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...
Comment 8 Emmi Takkinen 2022-07-27 05:33:51 UTC
(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?
Comment 9 Katrin Fischer 2022-07-30 21:51:47 UTC
I can no longer replicate the problem with the test plan that worked before :(
Comment 10 Emmi Takkinen 2022-08-01 05:23:03 UTC
(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?
Comment 11 Rachael 2022-08-02 15:05:22 UTC
No 500 error is produced. Instead, it says "The job has been enqueued! It will be processed as soon as possible."
Comment 12 Emmi Takkinen 2022-09-13 05:47:50 UTC
It seems this patch reawakens bug 21141. I'll rework this if needed since it might be that some other patch already fixed this.
Comment 13 Emmi Takkinen 2022-09-13 08:15:15 UTC
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
Comment 14 Emmi Takkinen 2022-09-13 09:21:40 UTC
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
Comment 15 David Nind 2022-09-13 18:40:24 UTC
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>
Comment 16 Marcel de Rooy 2022-09-16 08:23:48 UTC
QA: Looking here
Comment 17 Marcel de Rooy 2022-09-16 08:30:45 UTC
=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?
Comment 18 Marcel de Rooy 2022-09-16 08:38:26 UTC
$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.
Comment 19 Marcel de Rooy 2022-09-16 08:45:13 UTC
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 ;)
Comment 20 Marcel de Rooy 2022-09-16 09:19:40 UTC
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>
Comment 21 Marcel de Rooy 2022-09-16 09:19:44 UTC
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>
Comment 22 Marcel de Rooy 2022-09-16 09:21:12 UTC
Jonathan, left comment17 and comment19 for your consideration :)
Comment 23 Tomás Cohen Arazi 2022-09-16 13:34:31 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 24 Jonathan Druart 2022-10-17 08:00:53 UTC
(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?
Comment 25 Jonathan Druart 2022-10-17 08:01:19 UTC
This changes forgot to add unit tests btw.
Comment 26 Marcel de Rooy 2022-10-17 09:22:34 UTC
(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.
Comment 27 Jonathan Druart 2022-10-17 14:02:25 UTC
Created attachment 141973 [details] [review]
Bug 31154: Add test
Comment 28 Jonathan Druart 2022-10-17 14:02:30 UTC
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.
Comment 29 Jonathan Druart 2022-10-17 14:03:25 UTC
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.
Comment 30 Marcel de Rooy 2022-10-18 11:35:48 UTC
Thanks. Looking here now
Comment 31 Marcel de Rooy 2022-10-18 11:41:01 UTC
Created attachment 142051 [details] [review]
Bug 31154: Add test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-10-18 11:41:06 UTC
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>
Comment 33 Marcel de Rooy 2022-10-18 11:41:56 UTC
Tomás: Last two patches please.
Comment 34 Tomás Cohen Arazi 2022-10-21 13:17:10 UTC Comment hidden (obsolete)
Comment 35 Tomás Cohen Arazi 2022-10-21 15:28:00 UTC
Follow-ups pushed. Thanks
Comment 36 Lucas Gass 2022-10-31 21:42:10 UTC
Backported to 22.05.x for upcoming 22.05.07 release
Comment 37 Arthur Suzuki 2022-11-14 14:22:45 UTC
applied to 21.11 for 21.11.14
Comment 38 Victor Grousset/tuxayo 2022-11-24 04:38:01 UTC
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.