Bug 20702 - Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref
Summary: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref
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: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
: 9566 (view as bug list)
Depends on:
Blocks: 22140 27683
  Show dependency treegraph
 
Reported: 2018-05-03 10:59 UTC by Marcel de Rooy
Modified: 2021-02-11 13:42 UTC (History)
6 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:


Attachments
Bug 20702: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref (3.46 KB, patch)
2018-05-03 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20702: Add unit tests for GetHostItemsInfo (2.50 KB, patch)
2018-05-03 12:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20702: Add unit tests for GetHostItemsInfo (2.52 KB, patch)
2018-05-08 22:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20702: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref (3.48 KB, patch)
2018-05-08 22:39 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 20702: Add unit tests for GetHostItemsInfo (2.57 KB, patch)
2018-06-19 21:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20702: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref (3.53 KB, patch)
2018-06-19 21:42 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-05-03 10:59:58 UTC
As a follow-up of bug 20697 restricting the call to GetAnalyticsCount when the preference is enabled, and resolving an associated Zebra error.

GetHostItemsInfo is part of the easy analytics feature from bug 5528. We can either bind all calls of this sub to the preference, or return an empty list when the pref is disabled.
Comment 1 Marcel de Rooy 2018-05-03 12:16:45 UTC
Created attachment 75007 [details] [review]
Bug 20702: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref

Triggered by the finding on bug 20697.
The three calls of GetHostItemsInfo should be controlled by the pref. This
patch makes the sub return an empty list when the pref is disabled.

The patch simplifies the sub by merging the two identical foreach loops
depending on the field number in MARC21/UNIMARC.

Will add a unit test on a follow-up patch.

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2018-05-03 12:16:49 UTC
Created attachment 75008 [details] [review]
Bug 20702: Add unit tests for GetHostItemsInfo

Unfortunately, we did not have unit tests yet.
Adding some simple tests that test basic operation of the sub when the
pref is enabled/disabled and for MARC21/UNIMARC.

Test plan:
[1] Run t/db_dependent/Items/GetHostItemsInfo.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2018-05-03 12:24:12 UTC
Marked as normal in view of its follow-up character (on bug 20697). Moreover, I think we can call it erroneous when we do not use easy analytics and this routine should return records. Note that we should have a performance gain when we do not use easy analytics too.
Comment 4 Mark Tompsett 2018-05-08 22:35:55 UTC
Comment on attachment 75007 [details] [review]
Bug 20702: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref

Review of attachment 75007 [details] [review]:
-----------------------------------------------------------------

::: C4/Items.pm
@@ +1226,5 @@
> +    } elsif( C4::Context->preference('marcflavour') eq 'UNIMARC') {
> +        @fields = $record->field('461');
> +    }
> +
> +    foreach my $hostfield ( @fields ) {

I like this optimization. :)
Comment 5 Mark Tompsett 2018-05-08 22:39:11 UTC
Created attachment 75194 [details] [review]
Bug 20702: Add unit tests for GetHostItemsInfo

Unfortunately, we did not have unit tests yet.
Adding some simple tests that test basic operation of the sub when the
pref is enabled/disabled and for MARC21/UNIMARC.

Test plan:
[1] Run t/db_dependent/Items/GetHostItemsInfo.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Mark Tompsett 2018-05-08 22:39:14 UTC
Created attachment 75195 [details] [review]
Bug 20702: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref

Triggered by the finding on bug 20697.
The three calls of GetHostItemsInfo should be controlled by the pref. This
patch makes the sub return an empty list when the pref is disabled.

The patch simplifies the sub by merging the two identical foreach loops
depending on the field number in MARC21/UNIMARC.

Will add a unit test on a follow-up patch.

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 Marcel de Rooy 2018-05-09 10:58:35 UTC
(In reply to M. Tompsett from comment #6)
> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Thx
Comment 8 Katrin Fischer 2018-05-28 21:05:29 UTC
Hi Marcel,
I have problems testing this on master, see bug 20829. Maybe you can take a look and confirm (or not) the problem?
Comment 9 Marcel de Rooy 2018-05-31 07:23:42 UTC
(In reply to Katrin Fischer from comment #8)
> Hi Marcel,
> I have problems testing this on master, see bug 20829. Maybe you can take a
> look and confirm (or not) the problem?

Please apply 20829 and test again ;)
Comment 10 Katrin Fischer 2018-06-19 21:42:36 UTC
Created attachment 76197 [details] [review]
Bug 20702: Add unit tests for GetHostItemsInfo

Unfortunately, we did not have unit tests yet.
Adding some simple tests that test basic operation of the sub when the
pref is enabled/disabled and for MARC21/UNIMARC.

Test plan:
[1] Run t/db_dependent/Items/GetHostItemsInfo.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2018-06-19 21:42:40 UTC
Created attachment 76198 [details] [review]
Bug 20702: Bind results of GetHostItemsInfo to the EasyAnalyticalRecords pref

Triggered by the finding on bug 20697.
The three calls of GetHostItemsInfo should be controlled by the pref. This
patch makes the sub return an empty list when the pref is disabled.

The patch simplifies the sub by merging the two identical foreach loops
depending on the field number in MARC21/UNIMARC.

Will add a unit test on a follow-up patch.

Test plan:
See next patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Nick Clemens 2018-06-22 13:51:08 UTC
Awesome work all!

Pushed to master for 18.11.x
Comment 13 Martin Renvoize 2018-06-27 08:54:50 UTC
Pushed to 18.05.x for 18.05.02
Comment 14 Fridolin Somers 2018-06-28 08:10:50 UTC
*** Bug 9566 has been marked as a duplicate of this bug. ***
Comment 15 Fridolin Somers 2018-06-28 08:23:28 UTC
Pushed to 17.11.x for 17.11.08

I hope Bug 20697 will be pushed soon to be in same release