Bug 20855 - transferstoreceive.pl returns internal server error
Summary: transferstoreceive.pl returns internal server error
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All Linux
: P2 normal (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-01 12:06 UTC by lulihua
Modified: 2021-12-13 21:10 UTC (History)
4 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:


Attachments
Don't invoke _description_ on an undef itemtype (2.58 KB, patch)
2018-08-06 20:49 UTC, Paul Hoffman
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description lulihua 2018-06-01 12:06:53 UTC
/cgi-bin/koha/circ/transferstoreceive.pl returns internal server error on some of branches, but works fine on other branches.
All these branches have some configuration.
Comment 1 Jonathan Druart 2018-06-01 14:44:47 UTC
Could you give us a step by step plan to recreate the problem?

Can you have a look at the Koha log file, you will find a more meaningful error.
Comment 2 Jonathan Druart 2018-06-01 14:46:39 UTC
Personal note:

 98             $getransf{'itemtype'} = $itemtype->description; # FIXME Should not it be translated_description?

may cause problem if the item type is not defined in the itemtypes table.
Comment 3 lulihua 2018-06-03 03:30:50 UTC
Many thanks to Jonathan
Based on your hint, we found that quite a few of items in our branches with 'itype.items' as null. Eliminating all the nulls, /cgi-bin/koha/circ/transferstoreceive.pl works fine for all branches.
Comment 4 Paul Hoffman 2018-08-06 20:49:49 UTC
Created attachment 77499 [details] [review]
Don't invoke _description_ on an undef itemtype
Comment 5 Paul Hoffman 2018-08-06 20:53:46 UTC
Sorry that my comment on the attachment was rather cryptic; this is only my first or second bug comment.

I've upped the priority because the bug makes it impossible to view any list of holds (using waitingreserves.pl) that includes an item with NULL itype.
Comment 6 Jonathan Druart 2018-08-06 21:46:14 UTC
Paul, Do you agree that it's a data inconsistency you should fix on your side?
See bug 21150.
Comment 7 Paul Hoffman 2018-08-07 13:10:50 UTC
I don't consider it a case of bad data -- as much as I would like to -- because itype (952 $y) is optional in the default framework.  See installer/data/mysql/en/marcflavour/marc21/mandatory/marc21_framework_DEFAULT.sql, which sets marc_subfield_structure.mandatory to 0:

  89 INSERT IGNORE INTO `marc_subfield_structure` (`tagfield`, `tagsubfield`, `liblibrarian`, `libopac`, `repeatable`, `mandatory`, `kohafield`, `tab`, `authorised_valu     e`, `authtypecode`, `value_builder`, `isurl`, `hidden`, `frameworkcode`, `seealso`, `link`, `defaultvalue`) VALUES                                                 
  [...]
 123         ('952', 'y', 'Koha item type', 'Koha item type', 0, 0, 'items.itype', 10, 'itemtypes', '', '', NULL, 0, '', '', '', NULL),
Comment 8 Jonathan Druart 2018-08-07 13:31:22 UTC
Hi Paul,

Yes I know, that's the problem. There are several places in the code where we assumed items.itype or biblioitems.itemtype are set to an itemtype defined in the Koha administration module.
But nothing validate that (no foreign key, no mandatory field, etc.)

What I am suggesting with bug 21150 is to provide a script to catch the different situations where it can causes problems.

Marking items.itype mandatory is wrong if item-level_itypes is set to biblio.
Comment 9 Paul Hoffman 2018-08-07 13:52:23 UTC
Well, bug 21150 presents a way to clean up bad data, but I consider it a bug that the bad data is allowed to exist in the first place.  And, worse, that the bad data (while it exists) breaks waitingreserves.pl and (apprently) other scripts.

An easy solution is for us is to update our frameworks so that they all require 952 $y -- we have item-level_itypes set to 1 ("Use the item type of the <specific item> as the authoritative item type").

But that seems like a band-aid solution.  Frankly, my patch also strikes me as a band-aid solution, but at least it prevents waitingreserves.pl et al. from breaking.

Here are some possible solutions that come to mind, with the pros and cons as I see them:

(1) Check item-level_itypes at item creation time; if it is 1, require the subfield that maps to items.itype even if the framework's marc_subfield_structure.mandatory is 0.

Pro: Prevents bad data from being created.

Con: What happens when item-level_itypes is changed to 0 (biblio)?  And then back again?  Which it shouldn't, I know, but...

(2) When item-level_itypes is changed, update marc_subfield_structure.mandatory to 1 for all frameworks.

Pro: Prevents waitingreserves.pl et al. from breaking.

Con: Not reversible.

(3) At item creation time, allow NULL items.itype if the framework allows it.  But when displaying items don't assume that they have an itype.  That's what my patch does (or attempts to do!).

Pro: Prevents waitingreserves.pl et al. from breaking.

Con: Allows for bad data.  But run search_for_data_inconsistencies.pl to fix that.

(4) Something else that's a lot more complicated.  :-(

Pro: Presumably we can find The Right Way to fix this.

Con: My head hurts trying to imagine how.

Paul.
Comment 10 Katrin Fischer 2018-08-08 12:06:46 UTC
Hi Paul, I think making items.itype manatory in your configuration would be a good first step. We also plan to do this for our libraries, but there is currently bug 14662.
Comment 11 Katrin Fischer 2020-04-05 02:04:20 UTC
Should we mark this WONTFIX as we have agreed that the data inconsistency should be fixed instead?
Comment 12 Jonathan Druart 2020-04-06 09:17:20 UTC
(In reply to Katrin Fischer from comment #11)
> Should we mark this WONTFIX as we have agreed that the data inconsistency
> should be fixed instead?

Yes, done.
Comment 13 Alexis Ripetti 2020-11-30 16:17:01 UTC
Hi, 
I reopen this bug because one of our client also encountered this problem and I think this needs to be patched because data inconsistency should not get a software error.
This is not the first time that this probleme is reported and this will not be the last.
Comment 14 Jonathan Druart 2020-11-30 16:31:40 UTC
Alexis, this has been discussed a lot of time already.
We provided maintenance scripts to deal with such inconsistencies, you should tell your support team to use them.

This patch won't help much anyway, there are ton of other places where the error will appear.