Bug 18279

Summary: C4::Items - Remove GetLostItems
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, m.de.rooy, veron
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 18278, 18295    
Bug Blocks: 18252    
Attachments: Bug 18279: Remove C4::Items::GetLostItems
Bug 18279: Remove C4::Items::GetLostItems
Bug 18279: Remove C4::Items::GetLostItems
Bug 18279: [QA Follow-up] Correct @EXPORT

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