Bug 29106 - Can we get rid of Koha::Item->effective_itemtype
Summary: Can we get rid of Koha::Item->effective_itemtype
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-24 11:20 UTC by Tomás Cohen Arazi
Modified: 2021-10-08 11:00 UTC (History)
6 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-09-24 11:20:28 UTC
I file this bug to gather some ideas.

Do we still need that pattern here?

Right now we send a warning to the logs each time an item doesn't have an item type set.
Comment 1 Marcel de Rooy 2021-09-27 06:23:06 UTC
sub effective_itemtype {
    my ( $self ) = @_;

    my $pref = C4::Context->preference('item-level_itypes');
    if ( $pref && $self->itype() ) {
        return $self->itype();
    } else {
        warn "item-level_itypes set but no itemtype set for item (".$self->itemnumber.")"
          if $pref;
        return $self->biblioitemnumber()->itemtype();
    }
}

This sub c/should be moved out of the DBIx schema file to Koha::Item ?
There is no need to warn each time imo.
Comment 2 Kyle M Hall 2021-09-27 10:55:04 UTC
(In reply to Marcel de Rooy from comment #1)
> sub effective_itemtype {
>     my ( $self ) = @_;
> 
>     my $pref = C4::Context->preference('item-level_itypes');
>     if ( $pref && $self->itype() ) {
>         return $self->itype();
>     } else {
>         warn "item-level_itypes set but no itemtype set for item
> (".$self->itemnumber.")"
>           if $pref;
>         return $self->biblioitemnumber()->itemtype();
>     }
> }
> 
> This sub c/should be moved out of the DBIx schema file to Koha::Item ?
> There is no need to warn each time imo.

I think it should be moved to the object. That method was added to the DBIC class before Koha::Object(s) existed!
Comment 3 Tomás Cohen Arazi 2021-09-27 11:11:05 UTC
When I said we send a warning to the logs, I meant we are telling the users their data is wrong. It implies that the user needs to fix that.
Implicitly, we are telling people they need to set a value there.

Can't we force that somehow? And set the 'effective item type' on a DBRev instead? It feels like it was a trasitional step, for a transition that never took place.
Comment 4 Kyle M Hall 2021-09-27 11:28:41 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> When I said we send a warning to the logs, I meant we are telling the users
> their data is wrong. It implies that the user needs to fix that.
> Implicitly, we are telling people they need to set a value there.
> 
> Can't we force that somehow? And set the 'effective item type' on a DBRev
> instead? It feels like it was a trasitional step, for a transition that
> never took place.

It would be better to have a Koha data linter tool for sure. Maybe we should move this check to the about page, considering we have other checks of this nature there.

The reason the warning is there is because itemtype is required for Koha to operate correctly. It can be set at either the item or the bib level. That's why that warning ended up where it is. However, unless somebody is routinely combing through the logs that warning will go unnoticed. It seems more sensible to remove the warning there and add a check to the about page.
Comment 5 Marcel de Rooy 2021-09-27 11:33:22 UTC
IIRC items without itemtype come from Acquisition
Comment 6 Katrin Fischer 2021-09-27 19:50:31 UTC
(In reply to Marcel de Rooy from comment #5)
> IIRC items without itemtype come from Acquisition

Yes, they often do. And they won't cause an issue until you try to check them out. We have moved to setting itemtype mandatory for items in our libraries to avoid this from happening - at least on the non-ACQ frameworks.

I am not sure about the consequences of removing the warn - so not going to comment on that. But having a way to tell users that their data needs fixing is probably a good thing, given that Koha explodes badly when those items slip through in some areas.
Comment 7 Jonathan Druart 2021-09-28 07:59:20 UTC
(In reply to Katrin Fischer from comment #6)
> I am not sure about the consequences of removing the warn - so not going to
> comment on that. But having a way to tell users that their data needs fixing
> is probably a good thing, given that Koha explodes badly when those items
> slip through in some areas.

misc/maintenance/search_for_data_inconsistencies.pl is catching that.
Comment 8 Jonathan Druart 2021-09-28 08:04:08 UTC
If you want to get rid of this method you should get rid of item-level_itypes first :D

Long standing discussion
https://lists.koha-community.org/pipermail/koha-devel/2015-December/042114.html
Comment 9 Katrin Fischer 2021-09-28 08:07:52 UTC
(In reply to Jonathan Druart from comment #8)
> If you want to get rid of this method you should get rid of
> item-level_itypes first :D
> 
> Long standing discussion
> https://lists.koha-community.org/pipermail/koha-devel/2015-December/042114.
> html

Yes, long standing :) I am for killing the system preference, but we do require teh item type on record level for a number of features by now, so not sure we could get rid of all the effective itemtype handling.
Comment 10 Jonathan Druart 2021-09-28 08:17:33 UTC
The idea was to have biblioitems.itemtype always populated and accurate (done when upgrading and when a new item is created). No need to check anything else.
Comment 11 Katrin Fischer 2021-09-28 08:29:23 UTC
I am not sure we can have it always accurate - I know libraries use this a lot for records without items (articles, electronic resources, series records, set records, etc.), so manual interaction will always be required there to make it accurate.

We could add an automatism to update the record level itemtype when all items have the same itemtype - but this might not be what some libraries want (it would fit ours). But if the itemtypes for the items are different, this would not work. 
Use case in academics: one item has a not for loan item type to always remain in the library for reference, while additional copies can be checked out

I think especially in some French/UNIMARC libraries, there were different authorised valued used for bibioitems (document type) and items - something we would need to clarify is not an intended workflow and maybe help provide workarounds for.
Comment 12 Katrin Fischer 2021-09-28 08:30:22 UTC
Hm adding to that: as long we can't "save" 942$c from being overwritten by the import, enforcing the itemtype on record level is quite an unsolvable issue for us.
Comment 13 Katrin Fischer 2021-09-28 08:31:26 UTC
> misc/maintenance/search_for_data_inconsistencies.pl is catching that.

I know, but I think we need something more low level, that doesn't require server side interaction for libraries.
Comment 14 Marcel de Rooy 2021-09-28 11:19:14 UTC
HEA figures for item-level_itypes show that there is a very convincing number of libraries to remove the pref.

28 NULL
16 (empty?)
239 0
11386 1

Btw the discussion for an default itemtype might be a topic on itself?
Comment 15 Marcel de Rooy 2021-10-08 11:00:12 UTC
Bug 10385 ?