Bug 29554 - neworderempty.pl may create records with biblioitems.itemtype NULL
Summary: neworderempty.pl may create records with biblioitems.itemtype NULL
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-11-22 10:59 UTC by Marcel de Rooy
Modified: 2023-12-28 20:44 UTC (History)
5 users (show)

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


Attachments
Bug 29554: Do not hide display of itemtypes on neworderempty (1.20 KB, patch)
2021-11-22 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29554: Do not hide display of itemtypes on neworderempty (1.21 KB, patch)
2022-08-15 10:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29554: Do not hide display of itemtypes on neworderempty (1.55 KB, patch)
2022-09-16 06:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29554: Do not hide display of itemtypes on neworderempty (1.56 KB, patch)
2022-09-16 10:36 UTC, David Nind
Details | Diff | Splinter Review
Bug 29554: Do not hide display of itemtypes on neworderempty (1.62 KB, patch)
2022-10-07 10:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29554: Do not hide display of itemtypes on neworderempty (1.68 KB, patch)
2022-10-14 11:43 UTC, Slava Shishkin
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2021-11-22 10:59:29 UTC
neworderempty contains:

# Get the item types list, but only if item_level_itype is YES. Otherwise, it will be in the item, no need to display it in the biblio
my @itemtypes;
@itemtypes = Koha::ItemTypes->search unless C4::Context->preference('item-level_itypes');

I would argue that we should always fetch the itemtypes here.
Comment 1 Katrin Fischer 2021-11-22 11:49: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.
Comment 2 Marcel de Rooy 2021-11-22 12:52:39 UTC
(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.
Comment 3 Marcel de Rooy 2021-11-22 12:53:10 UTC
        - 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:
Comment 4 Marcel de Rooy 2021-11-22 13:03:15 UTC
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>
Comment 5 Katrin Fischer 2021-11-22 14:55:23 UTC
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?
Comment 6 Marcel de Rooy 2021-11-22 15:14:02 UTC
(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.
Comment 7 David Nind 2022-05-31 22:34:23 UTC
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).
Comment 8 Marcel de Rooy 2022-08-15 10:44:45 UTC
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>
Comment 9 Marcel de Rooy 2022-08-15 10:45:42 UTC
(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..
Comment 10 Marcel de Rooy 2022-09-15 05:44:00 UTC
Could you retry, David ?
Comment 11 David Nind 2022-09-15 08:34:08 UTC
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).
Comment 12 Marcel de Rooy 2022-09-15 09:33:22 UTC
(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.
Comment 13 Marcel de Rooy 2022-09-16 06:29:17 UTC
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.
Comment 14 Marcel de Rooy 2022-09-16 06:30:27 UTC
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>
Comment 15 David Nind 2022-09-16 10:36:36 UTC
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>
Comment 16 David Nind 2022-09-16 10:45:17 UTC
(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.
Comment 17 Martin Renvoize 2022-10-07 10:17:00 UTC
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>
Comment 18 Tomás Cohen Arazi 2022-10-13 14:26:35 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 19 Slava Shishkin 2022-10-14 11:43:57 UTC
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>
Comment 20 Lucas Gass 2022-12-05 20:10:43 UTC
Backported to 22.05.x for upcoming 22.05.08
Comment 21 Arthur Suzuki 2022-12-16 08:19:57 UTC
applied to 21.11 for 21.11.15
Comment 22 wainuiwitikapark 2023-03-15 00:54:28 UTC
Not backported to 21.05.x