|
Description
Marcel de Rooy
2018-05-03 10:59:58 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> 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> 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 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. :) 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> 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> (In reply to M. Tompsett from comment #6) > Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Thx Hi Marcel, I have problems testing this on master, see bug 20829. Maybe you can take a look and confirm (or not) the problem? (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 ;) 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> 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> Awesome work all! Pushed to master for 18.11.x Pushed to 18.05.x for 18.05.02 *** Bug 9566 has been marked as a duplicate of this bug. *** Pushed to 17.11.x for 17.11.08 I hope Bug 20697 will be pushed soon to be in same release |