Bug 18279 - C4::Items - Remove GetLostItems
Summary: C4::Items - Remove GetLostItems
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 18278 18295
Blocks: 18252
  Show dependency treegraph
 
Reported: 2017-03-15 21:22 UTC by Jonathan Druart
Modified: 2018-06-04 20:10 UTC (History)
3 users (show)

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


Attachments
Bug 18279: Remove C4::Items::GetLostItems (9.22 KB, patch)
2017-03-15 21:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18279: Remove C4::Items::GetLostItems (9.44 KB, patch)
2017-03-16 19:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18279: Remove C4::Items::GetLostItems (9.49 KB, patch)
2017-06-02 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18279: [QA Follow-up] Correct @EXPORT (658 bytes, patch)
2017-06-02 08:28 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 Jonathan Druart 2017-03-15 21:22:26 UTC

    
Comment 1 Jonathan Druart 2017-03-15 21:30:43 UTC
Created attachment 61152 [details] [review]
Bug 18279: Remove C4::Items::GetLostItems

The JOIN done by this subroutine are not always useful (depending on
item-level_itypes). They also search with LIKE when it is not needed.

Since we have now Koha::Items, we can replace this subroutine with a
call to Koha::Items->search with the correct parameters.

A change in previous behaviours can happen: If a items.itemlost contains
a value that is not defined as a LOST authorised value, the item will
not be displayed. I think it's the expected behaviour, even if it should
not happen in correctly configured installations.

Test plan:
To test with item-level_itypes set to item and biblio:
List the lost items you have on your system, using the different
filters available.
The result table should contain the correct item's info.
Comment 2 Marc Véron 2017-03-16 08:41:35 UTC
DOes not apply: Merge conflict in C4/Items.pm
Comment 3 Jonathan Druart 2017-03-16 16:32:37 UTC
Forgot to add the dependency, sorry.
Comment 4 Marc Véron 2017-03-16 19:30:08 UTC
Created attachment 61191 [details] [review]
Bug 18279: Remove C4::Items::GetLostItems

The JOIN done by this subroutine are not always useful (depending on
item-level_itypes). They also search with LIKE when it is not needed.

Since we have now Koha::Items, we can replace this subroutine with a
call to Koha::Items->search with the correct parameters.

A change in previous behaviours can happen: If a items.itemlost contains
a value that is not defined as a LOST authorised value, the item will
not be displayed. I think it's the expected behaviour, even if it should
not happen in correctly configured installations.

Test plan:
To test with item-level_itypes set to item and biblio:
List the lost items you have on your system, using the different
filters available.
The result table should contain the correct item's info.

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

https://bugs.koha-community.org/show_bug.cgi?id=18079
Comment 5 Marcel de Rooy 2017-06-02 08:26:10 UTC
Created attachment 63919 [details] [review]
Bug 18279: Remove C4::Items::GetLostItems

The JOIN done by this subroutine are not always useful (depending on
item-level_itypes). They also search with LIKE when it is not needed.

Since we have now Koha::Items, we can replace this subroutine with a
call to Koha::Items->search with the correct parameters.

A change in previous behaviours can happen: If a items.itemlost contains
a value that is not defined as a LOST authorised value, the item will
not be displayed. I think it's the expected behaviour, even if it should
not happen in correctly configured installations.

Test plan:
To test with item-level_itypes set to item and biblio:
List the lost items you have on your system, using the different
filters available.
The result table should contain the correct item's info.

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-06-02 08:28:25 UTC
Created attachment 63920 [details] [review]
Bug 18279: [QA Follow-up] Correct @EXPORT

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-06-02 08:34:44 UTC
QA Comment:
Looks good to me.

In the lost script, this construction is little bit strange:
my $loststatusfilter = $params->{'loststatusfilter'} || undef;

Normally you would expect something as:
my $loststatusfilter = $params->{'loststatusfilter'} // '';

In this particular case, it might have a weird side effect. If you add a LOST status for value 0 in auth values, and you select that lost value on the report form, it will return you the lost items instead of the items with value 0.
But adding a LOST auth value for 0 is not really useful. Selecting the not lost items on the lost form makes no sense too.
Comment 8 Jonathan Druart 2017-06-05 14:56:36 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 9 Fridolin Somers 2017-06-13 07:25:26 UTC
Enhancement not pushed to 17.05.x