Description
Jonathan Druart
2017-03-13 20:55:17 UTC
Created attachment 61054 [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 Small typo found: opac/opac-addbybiblionumber.pl line 145 my $biblio = Koha::Biblios->fin( $biblionumber ); should be: my $biblio = Koha::Biblios->find( $biblionumber ); Created attachment 61058 [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 Jonathan I'll prioritize testing of this patch if you can rebase it. Created attachment 63863 [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 Created attachment 64133 [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 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 "}" 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 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> 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. Created attachment 64827 [details] [review] Bug 18262: Fix test in MultiplePerRecord The biblioitem entry must be added (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! 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> Created attachment 64978 [details] [review] Bug 18262: Fix test in MultiplePerRecord The biblioitem entry must be added Simple rebase. 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 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> 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> Pushed to master for 17.11, thanks to everybody involved! Created attachment 65085 [details] [review] Bug 18262: Fix C4::XISBN Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> The last patch has been pushed to master! Enhancement not pushed to 17.05.x |