Bug 18262

Summary: Koha::Biblios - Remove GetBiblioData - part 1
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: fridolin.somers, m.de.rooy, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 17628, 19576, 21141    
Attachments: Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
[SIGNED-OFF] Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
Bug 18262: Fix test in MultiplePerRecord
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
Bug 18262: Fix test in MultiplePerRecord
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1
Bug 18262: Fix test in MultiplePerRecord
Bug 18262: Fix C4::XISBN

Description Jonathan Druart 2017-03-13 20:55:17 UTC

    
Comment 1 Jonathan Druart 2017-03-13 21:02:11 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2017-03-14 07:09:51 UTC
Small typo found:
opac/opac-addbybiblionumber.pl line 145

my $biblio = Koha::Biblios->fin( $biblionumber );
should be:
my $biblio = Koha::Biblios->find( $biblionumber );
Comment 3 Jonathan Druart 2017-03-14 12:49:26 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2017-05-31 12:13:44 UTC
Jonathan I'll prioritize testing of this patch if you can rebase it.
Comment 5 Jonathan Druart 2017-05-31 12:41:00 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2017-06-09 13:48:14 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2017-06-09 14:50:54 UTC
I ran into a couple of errors:

Global symbol "$bib" requires explicit package name at /home/vagrant/kohaclone/acqui/basketgroup.pl line 199.

syntax error at /home/vagrant/kohaclone/opac/opac-addbybiblionumber.pl line 151, near "}"
syntax error at /home/vagrant/kohaclone/opac/opac-addbybiblionumber.pl line 164, near "}"
Comment 8 Jonathan Druart 2017-06-09 15:09:36 UTC
Created attachment 64148 [details] [review]
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1

Most of the time C4::Biblio::GetBiblioData is used to retrieve the title
and/or the author of a bibliographic record.

This patch replaces the easy occurrences of GetBiblioData, the ones
where the 2 joins are needed, but only data from biblio and biblioitems
table are.

Test plan:
It will be hard to test everything, I'd suggest a QAer to review this
patch and confirm that the difference occurrences of GetBiblioData have
been correctly replaced by calling Koha::Biblios->find or
$biblio->bibioitem
Comment 9 Owen Leonard 2017-06-09 15:16:34 UTC
Created attachment 64149 [details] [review]
[SIGNED-OFF] Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1

Most of the time C4::Biblio::GetBiblioData is used to retrieve the title
and/or the author of a bibliographic record.

This patch replaces the easy occurrences of GetBiblioData, the ones
where the 2 joins are needed, but only data from biblio and biblioitems
table are.

Test plan:
It will be hard to test everything, I'd suggest a QAer to review this
patch and confirm that the difference occurrences of GetBiblioData have
been correctly replaced by calling Koha::Biblios->find or
$biblio->bibioitem

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Tomás Cohen Arazi 2017-06-27 19:07:37 UTC
Overall, it looks great. I would love to see (at some point) the Koha::Biblio objects passed to the template instead of explicitly extracting each attribute, but that's a whole other problem.

I've found a test to fail with this patch applied, and pass without it:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Reserves/MultiplePerRecord.tt/db_dependent/Reserves/MultiplePerRecord.t .. 1/38 Can't call method "agerestriction" on an undefined value at /home/vagrant/kohaclone/C4/Reserves.pm line 378.
# Looks like your test exited with 255 just after 33.
t/db_dependent/Reserves/MultiplePerRecord.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 5/38 subtests 

Test Summary Report
-------------------
t/db_dependent/Reserves/MultiplePerRecord.t (Wstat: 65280 Tests: 33 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 38 tests but ran 33.
Files=1, Tests=33,  2 wallclock secs ( 0.02 usr  0.01 sys +  1.53 cusr  0.30 csys =  1.86 CPU)
Result: FAIL

I'm trying to check what's going but wanted to note it.
Comment 11 Jonathan Druart 2017-07-05 19:53:14 UTC
Created attachment 64827 [details] [review]
Bug 18262: Fix test in MultiplePerRecord

The biblioitem entry must be added
Comment 12 Jonathan Druart 2017-07-05 19:54:21 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> Overall, it looks great. I would love to see (at some point) the
> Koha::Biblio objects passed to the template instead of explicitly extracting
> each attribute, but that's a whole other problem.

What I did on bug 18789 for Koha::Patron, I'd love to do it for Koha::Biblio too.

> I've found a test to fail with this patch applied, and pass without it:

Fixed!
Comment 13 Jonathan Druart 2017-07-10 19:02:34 UTC
Created attachment 64977 [details] [review]
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1

Most of the time C4::Biblio::GetBiblioData is used to retrieve the title
and/or the author of a bibliographic record.

This patch replaces the easy occurrences of GetBiblioData, the ones
where the 2 joins are needed, but only data from biblio and biblioitems
table are.

Test plan:
It will be hard to test everything, I'd suggest a QAer to review this
patch and confirm that the difference occurrences of GetBiblioData have
been correctly replaced by calling Koha::Biblios->find or
$biblio->bibioitem

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Jonathan Druart 2017-07-10 19:02:38 UTC
Created attachment 64978 [details] [review]
Bug 18262: Fix test in MultiplePerRecord

The biblioitem entry must be added
Comment 15 Jonathan Druart 2017-07-10 19:03:09 UTC
Simple rebase.
Comment 16 Marcel de Rooy 2017-07-14 09:55:27 UTC
You remove this test in Acquisition.pm:
if (    $biblioitem->{isbn}
-                    and $order->{isbn}
-                    and $biblioitem->{isbn} eq $order->{isbn} )
-                {
Looks fine to me, but noting it for the record.

opac-search.pl
use C4::Biblio; # Unused here?
No: I see e.g. GetMarcBiblio
Comment 17 Marcel de Rooy 2017-07-14 09:55:59 UTC
Created attachment 65045 [details] [review]
Bug 18262: Koha::Biblio - Remove GetBiblioData - part 1

Most of the time C4::Biblio::GetBiblioData is used to retrieve the title
and/or the author of a bibliographic record.

This patch replaces the easy occurrences of GetBiblioData, the ones
where the 2 joins are needed, but only data from biblio and biblioitems
table are.

Test plan:
It will be hard to test everything, I'd suggest a QAer to review this
patch and confirm that the difference occurrences of GetBiblioData have
been correctly replaced by calling Koha::Biblios->find or
$biblio->bibioitem

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-07-14 09:56:03 UTC
Created attachment 65046 [details] [review]
Bug 18262: Fix test in MultiplePerRecord

The biblioitem entry must be added

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Jonathan Druart 2017-07-14 15:32:38 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 20 Jonathan Druart 2017-07-17 20:19:21 UTC
Created attachment 65085 [details] [review]
Bug 18262: Fix C4::XISBN

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2017-07-17 20:20:11 UTC
The last patch has been pushed to master!
Comment 22 Fridolin Somers 2017-07-18 14:39:05 UTC
Enhancement not pushed to 17.05.x