Bug 19943 - Koha::Biblio - Remove GetBiblioItemData
Summary: Koha::Biblio - Remove GetBiblioItemData
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 17628 20499 20825 20889
  Show dependency treegraph
 
Reported: 2018-01-09 18:37 UTC by Jonathan Druart
Modified: 2019-10-14 19:57 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued (3.37 KB, patch)
2018-01-09 18:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences (4.15 KB, patch)
2018-01-09 18:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Remove C4::Biblio::GetBiblioItemData (1.87 KB, patch)
2018-01-09 18:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued (3.42 KB, patch)
2018-03-09 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences (4.24 KB, patch)
2018-03-09 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19943: Remove C4::Biblio::GetBiblioItemData (1.92 KB, patch)
2018-03-09 12:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued (3.48 KB, patch)
2018-03-09 13:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences (4.31 KB, patch)
2018-03-09 13:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19943: Remove C4::Biblio::GetBiblioItemData (1.98 KB, patch)
2018-03-09 13:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19943: Gentle error handling for bookcount.pl (3.72 KB, patch)
2018-03-09 13:58 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued (3.55 KB, patch)
2018-03-23 15:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Koha::Biblio - Replace GetBiblioItemData with Koha::Biblio->biblioitem (4.41 KB, patch)
2018-03-23 15:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Remove C4::Biblio::GetBiblioItemData (1.93 KB, patch)
2018-03-23 15:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Gentle error handling for bookcount.pl (3.79 KB, patch)
2018-03-23 15:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Fix NoIssuesChargeGuarantees.t - create the biblioitem entry (914 bytes, patch)
2018-03-23 16:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Fix dateexpiry.t - create the biblioitem entry (2.14 KB, patch)
2018-03-23 16:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Fix SwitchOnSiteCheckouts.t - create the biblioitem entry (949 bytes, patch)
2018-03-23 16:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19943: Fix Borrower_PrevCheckout.t - create the biblioitem entry (1.33 KB, patch)
2018-03-23 16:23 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-01-09 18:37:19 UTC

    
Comment 1 Jonathan Druart 2018-01-09 18:47:46 UTC
Created attachment 70384 [details] [review]
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued

Just a preliminary step to clean the code a bit in CanBookBeIssued.
The effective item type is already set from GetItem and we do not need
to deal with that again.
Comment 2 Jonathan Druart 2018-01-09 18:47:49 UTC
Created attachment 70385 [details] [review]
Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences

The biblioitem's info can be retrieved with Koha::Biblio->biblioitem

Test plan:
1. Use the age restriction to restrict checkouts for a given patron
2. Check some items of a biblio out, go to "Items" tab, then "View
item's checkout history" link. Compare views with and without patches
Comment 3 Jonathan Druart 2018-01-09 18:47:52 UTC
Created attachment 70386 [details] [review]
Bug 19943: Remove C4::Biblio::GetBiblioItemData

It is no longer used.
Comment 4 Mark Tompsett 2018-01-18 01:31:37 UTC
Comment on attachment 70385 [details] [review]
Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences

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

::: circ/bookcount.pl
@@ +39,5 @@
>  my $biblionumber = $input->param('biblionumber');
>  
>  my $idata = itemdatanum($itm);
> +my $biblio = Koha::Biblios->find( $biblionumber );
> +die "No valid biblionumber passed" unless $biblio; # FIXME A bit rude!

I believe if you purposefully call bookcount.pl directly so as to trigger this, GetBiblioItemData wouldn't die this way in the old code. Also, you can only call this from an existing catalogue/moredetail.tt page.
Perhaps the uglier:
$data = $biblio ? $biblio->unblessed : {};

::: t/db_dependent/Biblio.t
@@ +198,5 @@
>      is( $marc->subfield( $title_field, $title_subfield ), $title, );
>  
> +    my $biblioitem = Koha::Biblioitems->find( $biblioitemnumber );
> +    is( $biblioitem->_result->biblio->title, $title, # Should be $biblioitem->biblio instead, but not needed elsewhere for now
> +        'Do not know if this makes sense - compare result of previous two GetBiblioData tests.');

This test was proving that the GetBiblioItemData function worked by confirming that the title set in the Biblio was the one retrieved in this function. Given that this bug patch set removes the function, I'm not sure this test is needed at all. I'd recommend removing this one test.
Comment 5 Jonathan Druart 2018-01-22 15:23:36 UTC
(In reply to M. Tompsett from comment #4)
> Comment on attachment 70385 [details] [review] [review]
> Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences
> 
> Review of attachment 70385 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: circ/bookcount.pl
> @@ +39,5 @@
> >  my $biblionumber = $input->param('biblionumber');
> >  
> >  my $idata = itemdatanum($itm);
> > +my $biblio = Koha::Biblios->find( $biblionumber );
> > +die "No valid biblionumber passed" unless $biblio; # FIXME A bit rude!
> 
> I believe if you purposefully call bookcount.pl directly so as to trigger
> this, GetBiblioItemData wouldn't die this way in the old code. Also, you can
> only call this from an existing catalogue/moredetail.tt page.
> Perhaps the uglier:
> $data = $biblio ? $biblio->unblessed : {};

Nope, I am waiting for output_and_exit_if_error from bug 18403 to deal with that.

> ::: t/db_dependent/Biblio.t
> @@ +198,5 @@
> >      is( $marc->subfield( $title_field, $title_subfield ), $title, );
> >  
> > +    my $biblioitem = Koha::Biblioitems->find( $biblioitemnumber );
> > +    is( $biblioitem->_result->biblio->title, $title, # Should be $biblioitem->biblio instead, but not needed elsewhere for now
> > +        'Do not know if this makes sense - compare result of previous two GetBiblioData tests.');
> 
> This test was proving that the GetBiblioItemData function worked by
> confirming that the title set in the Biblio was the one retrieved in this
> function. Given that this bug patch set removes the function, I'm not sure
> this test is needed at all. I'd recommend removing this one test.

I added a comment to tell that, so I think it's ok.
Comment 6 Kyle M Hall 2018-03-09 12:38:17 UTC
Created attachment 72568 [details] [review]
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued

Just a preliminary step to clean the code a bit in CanBookBeIssued.
The effective item type is already set from GetItem and we do not need
to deal with that again.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall 2018-03-09 12:39:15 UTC
Created attachment 72569 [details] [review]
Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences

The biblioitem's info can be retrieved with Koha::Biblio->biblioitem

Test plan:
1. Use the age restriction to restrict checkouts for a given patron
2. Check some items of a biblio out, go to "Items" tab, then "View
item's checkout history" link. Compare views with and without patches

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2018-03-09 12:39:47 UTC
Created attachment 72570 [details] [review]
Bug 19943: Remove C4::Biblio::GetBiblioItemData

It is no longer used.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Tomás Cohen Arazi 2018-03-09 13:57:46 UTC
Created attachment 72593 [details] [review]
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued

Just a preliminary step to clean the code a bit in CanBookBeIssued.
The effective item type is already set from GetItem and we do not need
to deal with that again.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2018-03-09 13:57:56 UTC
Created attachment 72594 [details] [review]
Bug 19943: Koha::GetBiblioItemData - Replace existing occurrences

The biblioitem's info can be retrieved with Koha::Biblio->biblioitem

Test plan:
1. Use the age restriction to restrict checkouts for a given patron
2. Check some items of a biblio out, go to "Items" tab, then "View
item's checkout history" link. Compare views with and without patches

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2018-03-09 13:58:03 UTC
Created attachment 72595 [details] [review]
Bug 19943: Remove C4::Biblio::GetBiblioItemData

It is no longer used.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2018-03-09 13:58:09 UTC
Created attachment 72596 [details] [review]
Bug 19943: Gentle error handling for bookcount.pl

This patch tests the required parameters and redirects with a 400 HTTP
code if parameters are invalid.

It also removes the need for the passed biblioitemnumber which is not
used at all.

A (now) useless sub is removed too.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Jonathan Druart 2018-03-23 15:32:45 UTC
Created attachment 73189 [details] [review]
Bug 19943: Remove itemtype vs itype confusion in CanBookBeIssued

Just a preliminary step to clean the code a bit in CanBookBeIssued.
The effective item type is already set from GetItem and we do not need
to deal with that again.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2018-03-23 15:32:49 UTC
Created attachment 73190 [details] [review]
Bug 19943: Koha::Biblio - Replace GetBiblioItemData with Koha::Biblio->biblioitem

The biblioitem's info can be retrieved with Koha::Biblio->biblioitem

Test plan:
1. Use the age restriction to restrict checkouts for a given patron
2. Check some items of a biblio out, go to "Items" tab, then "View
item's checkout history" link. Compare views with and without patches

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2018-03-23 15:32:53 UTC
Created attachment 73191 [details] [review]
Bug 19943: Remove C4::Biblio::GetBiblioItemData

It is no longer used.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2018-03-23 15:32:56 UTC
Created attachment 73192 [details] [review]
Bug 19943: Gentle error handling for bookcount.pl

This patch tests the required parameters and redirects with a 400 HTTP
code if parameters are invalid.

It also removes the need for the passed biblioitemnumber which is not
used at all.

A (now) useless sub is removed too.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2018-03-23 15:36:34 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 18 Jonathan Druart 2018-03-23 16:01:47 UTC
Created attachment 73194 [details] [review]
Bug 19943: Fix NoIssuesChargeGuarantees.t - create the biblioitem entry
Comment 19 Jonathan Druart 2018-03-23 16:19:13 UTC
Created attachment 73198 [details] [review]
Bug 19943: Fix dateexpiry.t - create the biblioitem entry
Comment 20 Jonathan Druart 2018-03-23 16:21:14 UTC
Created attachment 73199 [details] [review]
Bug 19943: Fix SwitchOnSiteCheckouts.t - create the biblioitem entry
Comment 21 Jonathan Druart 2018-03-23 16:23:37 UTC
Created attachment 73200 [details] [review]
Bug 19943: Fix Borrower_PrevCheckout.t - create the biblioitem entry
Comment 22 Jonathan Druart 2018-03-23 16:24:47 UTC
Last 4 patches pushed to master.
Comment 23 Nick Clemens 2018-03-26 12:12:44 UTC
Enhancement, not backported for 17.11