Description
Marcel de Rooy
2021-11-22 10:59:29 UTC
I think this might be used for the itemtype pull down visible in the new order form when item level itype is set to biblio. It's not visible otherwise, which would explain the comment. (In reply to Katrin Fischer from comment #1) > I think this might be used for the itemtype pull down visible in the new > order form when item level itype is set to biblio. It's not visible > otherwise, which would explain the comment. Sure. But thats arguable. I dont like creating records with bibliotems.itemtype NULL even when the item level itype is item. Note that 942c is mandatory in the editor too. These NULL itemtypes make Koha crash on other code locations. Since we often assume that the itemtype should be found. - tagfield: "942" tagsubfield: "c" liblibrarian: "Koha item type" libopac: "Koha item type" repeatable: 0 mandatory: 1 kohafield: biblioitems.itemtype tab: 9 authorised_value: itemtypes authtypecode: "" value_builder: "" isurl: hidden: 0 frameworkcode: "" seealso: "" link: "" defaultvalue: Created attachment 127927 [details] [review] Bug 29554: Do not hide display of itemtypes on neworderempty Hiding them may lead to biblioitems.itemtype==NULL. We do not want that, since 942$c is mandatory. Test plan: Create basket, create order from new empty record. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> We never set the itemtype on biblio level and haven't had any issues with that in 18.11 - if there are issues now, that would be quite bad. But should this bug report then not be about making the field visible independent from the item-level_itype setting? (In reply to Katrin Fischer from comment #5) > We never set the itemtype on biblio level and haven't had any issues with > that in 18.11 - if there are issues now, that would be quite bad. > > But should this bug report then not be about making the field visible > independent from the item-level_itype setting? Yeah the bug title is a bit cryptic, but describes the problem. And the commit title tells you the fix. The patch no longer applies 8-(... Also, just checking my understanding what is required here for testing. Is it that when adding to a basket "From a new (empty) record", that for the catalog details section the item type should be shown regardless of the setting for the item-level_itypes system preference? This is so that the record created has an item type, and it is not empty (which can cause issues elsewhere in Koha if it is NULL). Created attachment 139117 [details] [review] Bug 29554: Do not hide display of itemtypes on neworderempty Hiding them may lead to biblioitems.itemtype==NULL. We do not want that, since 942$c is mandatory. Test plan: Create basket, create order from new empty record. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to David Nind from comment #7) > The patch no longer applies 8-(... > > Also, just checking my understanding what is required here for testing. > > Is it that when adding to a basket "From a new (empty) record", that for the > catalog details section the item type should be shown regardless of the > setting for the item-level_itypes system preference? > > This is so that the record created has an item type, and it is not empty > (which can cause issues elsewhere in Koha if it is NULL). Yes that sounds fine. Have another try please.. Could you retry, David ? After applying the patch (and regardless of whether item-level_itypes is set to specific item or bibliographic record) I am getting this error when adding to a basket from a new (empty) record: Template process failed: undef error - The method Koha::ItemTypes->itemtype is not covered by tests! Trace begun at /kohadevbox/koha/Koha/Objects.pm line 572 Koha::Objects::AUTOLOAD('Koha::ItemTypes=HASH(0x55b3275f1d40)') called at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt line 242 eval {...} at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt line 242 eval {...} at /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt line 250 eval {...} at /kohadevbox/koha/ ... I have cleared the browser cache, plus a flush_memcached and restart_all (using KTD). (In reply to David Nind from comment #11) > After applying the patch (and regardless of whether item-level_itypes is set > to specific item or bibliographic record) I am getting this error when > adding to a basket from a new (empty) record: > > Template process failed: undef error - The method Koha::ItemTypes->itemtype > is not covered by tests! > > Trace begun at /kohadevbox/koha/Koha/Objects.pm line 572 > Koha::Objects::AUTOLOAD('Koha::ItemTypes=HASH(0x55b3275f1d40)') called at > /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty. > tt line 242 > eval {...} at > /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty. > tt line 242 > eval {...} at > /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty. > tt line 250 > eval {...} at /kohadevbox/koha/ > ... > > I have cleared the browser cache, plus a flush_memcached and restart_all > (using KTD). Thx. Will have a look. Sorry David, that was a dumb omission related to the change of ->search to scalar context. We need ->as_list currently. I updated the test plan to include choosing receiving/cataloging to make more clear what this patch is about. Created attachment 140688 [details] [review] Bug 29554: Do not hide display of itemtypes on neworderempty Hiding the list may lead to biblioitems.itemtype being NULL. We do not want that, since 942$c is mandatory. Test plan: Create basket and set 'create items when' to receiving or cataloging. Create order from new empty record. Check biblioitems.itemtype of created biblio. Without this patch, it would have been NULL like this: select biblionumber,title from biblio; | 7 | Test 3 | select biblionumber,itemtype from biblioitems; | 7 | NULL | Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 140701 [details] [review] Bug 29554: Do not hide display of itemtypes on neworderempty Hiding the list may lead to biblioitems.itemtype being NULL. We do not want that, since 942$c is mandatory. Test plan: Create basket and set 'create items when' to receiving or cataloging. Create order from new empty record. Check biblioitems.itemtype of created biblio. Without this patch, it would have been NULL like this: select biblionumber,title from biblio; | 7 | Test 3 | select biblionumber,itemtype from biblioitems; | 7 | NULL | Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> (In reply to Marcel de Rooy from comment #13) > Sorry David, that was a dumb omission related to the change of ->search to > scalar context. We need ->as_list currently. > I updated the test plan to include choosing receiving/cataloging to make > more clear what this patch is about. Thanks for fixing and the updated test plan! Now signed off now. Created attachment 141488 [details] [review] Bug 29554: Do not hide display of itemtypes on neworderempty Hiding the list may lead to biblioitems.itemtype being NULL. We do not want that, since 942$c is mandatory. Test plan: Create basket and set 'create items when' to receiving or cataloging. Create order from new empty record. Check biblioitems.itemtype of created biblio. Without this patch, it would have been NULL like this: select biblionumber,title from biblio; | 7 | Test 3 | select biblionumber,itemtype from biblioitems; | 7 | NULL | Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Pushed to master for 22.11. Nice work everyone, thanks! Created attachment 141873 [details] [review] Bug 29554: Do not hide display of itemtypes on neworderempty Hiding the list may lead to biblioitems.itemtype being NULL. We do not want that, since 942$c is mandatory. Test plan: Create basket and set 'create items when' to receiving or cataloging. Create order from new empty record. Check biblioitems.itemtype of created biblio. Without this patch, it would have been NULL like this: select biblionumber,title from biblio; | 7 | Test 3 | select biblionumber,itemtype from biblioitems; | 7 | NULL | Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Backported to 22.05.x for upcoming 22.05.08 applied to 21.11 for 21.11.15 Not backported to 21.05.x |