Bug 21842 - always test Koha::ItemType in defined
Summary: always test Koha::ItemType in defined
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 17843
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-15 08:34 UTC by Fridolin Somers
Modified: 2020-11-30 21:45 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21842: test Koha::ItemType in defined in patron summary print (1.59 KB, patch)
2018-11-15 10:55 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21842: test Koha::ItemType in defined in transfers to receive (1.63 KB, patch)
2018-11-15 10:55 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21842: test Koha::ItemType in defined in order recieve items table (1.83 KB, patch)
2018-11-15 10:55 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21842: test Koha::ItemType in defined in patron summary print (1.57 KB, patch)
2018-11-27 08:08 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21842: test Koha::ItemType in defined in transfers to receive (1.60 KB, patch)
2018-11-27 08:09 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21842: test Koha::ItemType in defined in order recieve items table (1.81 KB, patch)
2018-11-27 08:09 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2018-11-15 08:34:34 UTC
Like Bug 19194 some other code calls $itemtype->value without testing the object is defined.
Itemtype should always be defined but its better not fall into error page for that.
Comment 1 Fridolin Somers 2018-11-15 09:23:45 UTC
I've looked with :
$ git grep --name-only '$itemtype->description'
acqui/basketgroup.pl
acqui/orderreceive.pl
admin/itemtypes.pl
catalogue/getitem-ajax.pl
circ/transferstoreceive.pl
labels/label-item-search.pl
members/summary-print.pl
opac/opac-shelves.pl
virtualshelves/shelves.pl

No problem '$itemtype->imageurl' and '$itemtype->notforloan' calls.
Comment 2 Fridolin Somers 2018-11-15 10:55:05 UTC
Created attachment 82354 [details] [review]
Bug 21842: test Koha::ItemType in defined in patron summary print

Like Bug 19194 calls on $itemtype->value must test the object is defined.
This patch fixes members/summary-print.pl

Test plan :
1) Define system preference item-level_itypes on "specific item"
2) Define in framework a subfield of items field : mapped on items.itype and not mandatory
3) Create an item without value on this subfield
4) Go on a patron page
5) Checkout this item
6) Click on "Print" > "Print summary"
7) Without patch you get the error :
Can't call method "description" on an undefined value
8) With patch you see the itemtype description
Comment 3 Fridolin Somers 2018-11-15 10:55:19 UTC
Created attachment 82355 [details] [review]
Bug 21842: test Koha::ItemType in defined in transfers to receive

Like Bug 19194 calls on $itemtype->value must test the object is defined.
This patch fixes circ/transferstoreceive.pl

Test plan :
1) Define system preference item-level_itypes on "specific item"
2) Define in framework a subfield of items field : mapped on items.itype and not mandatory
3) Create an item without value on this subfield
4) Create a transfers to receive on this item
5) Go to Circulation > Transfers to receive
6) Without patch you get the error :
Can't call method "description" on an undefined value
7) With patch you see the itemtype description
Comment 4 Fridolin Somers 2018-11-15 10:55:56 UTC
Created attachment 82356 [details] [review]
Bug 21842: test Koha::ItemType in defined in order recieve items table

Like Bug 19194 calls on $itemtype->value must test the object is defined.
This patch catalogue/getitem-ajax.pl

Test plan :
1) Define system preference item-level_itypes on "specific item"
2) Define in framework a subfield of items field : mapped on items.itype and not mandatory
3) Create an item without value on this subfield
4) Go to Acquisitions -> Find a vendor or make a new one -> create a new basket
5) Add the record from Step 1 to your basket
6) Close the basket
7) Go back to the vendor and click "Receive shipments"
8) Put in an invoice number, click Next
9) Click the "Receive" link for your item
10) Click on "Edit" in items table
11) You see item editor in a new tab
12) Change something and save
13) Without patch your change is not update
    The ajax call is with HTTP 500 error containing :
    Can't call method "description" on an undefined value
14) With patch items table is updated
Comment 5 Mark Tompsett 2018-11-22 03:38:15 UTC
Shouldn't the values be set to SOMETHING even if itemtype is undefined?

$foo->{key} = defined $bar ? $bar->baz : q{};
and
$foo{key} = defined $bar ? $bar->baz : q{};

Feel free to switch back to needs sign off after answering/fixing this.
Comment 6 Fridolin Somers 2018-11-27 08:08:47 UTC
Created attachment 82656 [details] [review]
Bug 21842: test Koha::ItemType in defined in patron summary print

Like Bug 19194 calls on $itemtype->value must test the object is defined.
This patch fixes members/summary-print.pl

Test plan :
1) Define system preference item-level_itypes on "specific item"
2) Define in framework a subfield of items field : mapped on items.itype and not mandatory
3) Create an item without value on this subfield
4) Go on a patron page
5) Checkout this item
6) Click on "Print" > "Print summary"
7) Without patch you get the error :
Can't call method "description" on an undefined value
8) With patch you see the itemtype description
Comment 7 Fridolin Somers 2018-11-27 08:09:06 UTC
Created attachment 82657 [details] [review]
Bug 21842: test Koha::ItemType in defined in transfers to receive

Like Bug 19194 calls on $itemtype->value must test the object is defined.
This patch fixes circ/transferstoreceive.pl

Test plan :
1) Define system preference item-level_itypes on "specific item"
2) Define in framework a subfield of items field : mapped on items.itype and not mandatory
3) Create an item without value on this subfield
4) Create a transfers to receive on this item
5) Go to Circulation > Transfers to receive
6) Without patch you get the error :
Can't call method "description" on an undefined value
7) With patch you see the itemtype description
Comment 8 Fridolin Somers 2018-11-27 08:09:35 UTC
Created attachment 82658 [details] [review]
Bug 21842: test Koha::ItemType in defined in order recieve items table

Like Bug 19194 calls on $itemtype->value must test the object is defined.
This patch catalogue/getitem-ajax.pl

Test plan :
1) Define system preference item-level_itypes on "specific item"
2) Define in framework a subfield of items field : mapped on items.itype and not mandatory
3) Create an item without value on this subfield
4) Go to Acquisitions -> Find a vendor or make a new one -> create a new basket
5) Add the record from Step 1 to your basket
6) Close the basket
7) Go back to the vendor and click "Receive shipments"
8) Put in an invoice number, click Next
9) Click the "Receive" link for your item
10) Click on "Edit" in items table
11) You see item editor in a new tab
12) Change something and save
13) Without patch your change is not update
    The ajax call is with HTTP 500 error containing :
    Can't call method "description" on an undefined value
14) With patch items table is updated
Comment 9 Fridolin Somers 2018-11-27 08:10:22 UTC
(In reply to M. Tompsett from comment #5)
> Shouldn't the values be set to SOMETHING even if itemtype is undefined?
> 
> $foo->{key} = defined $bar ? $bar->baz : q{};
> and
> $foo{key} = defined $bar ? $bar->baz : q{};
> 
> Feel free to switch back to needs sign off after answering/fixing this.

Very good point.
I have changed the patches.
Comment 10 Mark Tompsett 2018-11-29 04:55:34 UTC
(In reply to Fridolin SOMERS from comment #6)
> 2) Define in framework a subfield of items field : mapped on items.itype and
> not mandatory

Isn't this 952$y by default when you create a new framework from Default?
(See the Advanced Constraints) or is this that mapped thing elsewhere?
Comment 11 Fridolin Somers 2018-11-29 10:22:07 UTC
(In reply to M. Tompsett from comment #10)
> (In reply to Fridolin SOMERS from comment #6)
> > 2) Define in framework a subfield of items field : mapped on items.itype and
> > not mandatory
> 
> Isn't this 952$y by default when you create a new framework from Default?
> (See the Advanced Constraints) or is this that mapped thing elsewhere?

Indeed it is 952$y :
y 	Koha item type 	Tab:10, | Koha field: items.itype, Not repeatable, Not mandatory, | Auth value:itemtypes,
Comment 12 Jonathan Druart 2018-12-17 16:13:58 UTC
As I have explained on the list in a previous thread (http://lists.koha-community.org/pipermail/koha-devel/2017-July/043841.html) I am against the idea of testing for data inconsistencies at code level.

I have worked on providing a script to catch such inconsistencies in the DB (see bug 21150), the idea is more to not hide them and compel people to run the script to make sure they have correct data, and that Koha will behave correctly.
Comment 13 Fridolin Somers 2019-07-17 07:39:51 UTC
Comment on attachment 82656 [details] [review]
Bug 21842: test Koha::ItemType in defined in patron summary print

Patch for patron summary print is obsolete.
members/summary-print.pl does not contain $checkout->{itemtype_description} anymore
Comment 14 Jonathan Druart 2019-12-11 13:45:14 UTC
Marking as won't fix regarding to comment 12.