Bug 14651 - Koha::Item::effective_itemtype should fallback to biblio-level if no item-level itemtype
Summary: Koha::Item::effective_itemtype should fallback to biblio-level if no item-lev...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-05 18:23 UTC by Tomás Cohen Arazi
Modified: 2016-06-21 21:40 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14651: (regression test) fallback to bib-level if itype is undef (1.72 KB, patch)
2015-08-18 19:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14651: Koha::Item->effective_itemtype fallback to bib-level (1.30 KB, patch)
2015-08-18 19:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14651: (regression test) fallback to bib-level if itype is undef (2.24 KB, patch)
2015-08-28 14:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14651: Koha::Item->effective_itemtype fallback to bib-level (1.49 KB, patch)
2015-08-28 14:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14651: (regression test) fallback to bib-level if itype is undef (2.30 KB, patch)
2015-09-03 03:46 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 14651: Koha::Item->effective_itemtype fallback to bib-level (1.54 KB, patch)
2015-09-03 03:46 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 14651: (regression test) fallback to bib-level if itype is undef (2.40 KB, patch)
2015-09-03 07:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14651: Koha::Item->effective_itemtype fallback to bib-level (1.64 KB, patch)
2015-09-03 07:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2015-08-05 18:23:06 UTC
I fill this bug to start a conversation about this issue.
It seems the codebase has several ways of determining the itemtype. For example, C4::Circulation::AddReturn used the biblio-level itemtype (until bug 14598) and C4::Circulation::CanBookBeIssued just sticks to item-level itemtype.

At first sight, it seems clear to me that we should start moving into using Koha::Item objects, that even provide an ->effective_itemtype method!

Having talked about this on IRC there seems to be a consensus that if the item doesn't have an itemtype, then there should be a fallback to biblio-level itemtype.

This means we should add that check to the ->effective_itemtype method. But it would mean including Koha::Biblios.

Thoughts?
Comment 1 Tomás Cohen Arazi 2015-08-18 19:41:12 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2015-08-18 19:41:16 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2015-08-25 12:41:44 UTC
(In reply to Tomás Cohen Arazi from comment #0)
> At first sight, it seems clear to me that we should start moving into using
> Koha::Item objects, that even provide an ->effective_itemtype method!
> 
> Having talked about this on IRC there seems to be a consensus that if the
> item doesn't have an itemtype, then there should be a fallback to
> biblio-level itemtype.
> 
> This means we should add that check to the ->effective_itemtype method. But
> it would mean including Koha::Biblios.

The patches work fine, but aren't we masquerading a problem that should be addressed instead of hidden ?
Comment 4 Tomás Cohen Arazi 2015-08-28 14:19:30 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2015-08-28 14:19:36 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2015-09-03 03:46:10 UTC Comment hidden (obsolete)
Comment 7 Chris Cormack 2015-09-03 03:46:20 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2015-09-03 07:33:20 UTC
Created attachment 42270 [details] [review]
Bug 14651: (regression test) fallback to bib-level if itype is undef

Koha::Item->effective_itemtype should fallback to biblio-level itemtype
even if item-level item types are set, in the case the item has no itemtype
set (bad migration, bad old code).

To test:
- Run
  $ prove t/db_dependent/Items.t
=> FAIL: Koha::Item->effective_itemtype doesn't work properly

Edit: Added a test for a warning when falling back as per QA request
and because it made a lot of sense :-D

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2015-09-03 07:33:24 UTC
Created attachment 42271 [details] [review]
Bug 14651: Koha::Item->effective_itemtype fallback to bib-level

In some situations (bad migrations, old bugs that introduced bad data,
people having bib-level itypes for ages and switching to item level...)
the user ends with undex itype values for items.

The current code, if the user has item_level-itype set, just returns
undef. It should fallback to bib-level. In order to avoid hiding the problem
a warning is raised.

To test:
- Run the regression tets:
  $ prove t/db_dependent/Items.t
=> FAIL: tests fail.
- Apply the patch
- Run the tests again
=> SUCCESS: The tests now pass.
- Sign off :-D

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Tomás Cohen Arazi 2015-09-07 14:30:24 UTC
Patches pushed to master.