| Summary: | Library name linking wrong if current location is different | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Nicole C. Engard <nengard> |
| Component: | Templates | Assignee: | Owen Leonard <oleonard> |
| Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | chris, koha.sekjal, paul.poulain |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: | Proposed fix | ||
|
Description
Nicole C. Engard
2011-07-18 18:26:46 UTC
Created attachment 4668 [details] [review] Proposed fix GetItemsInfo in Items.pm includes this join: LEFT JOIN branches ON items.homebranch = branches.branchcode This means that the branch URL (from the branches table) comes out as the URL for items.homebranch, thus the URL in the holdings output is the item's home branch even though the display might be showing a different current location. This patch changes the join to use items.holdingbranch. The join was originally added to fix Bug 3702, and based on the description of that feature I'm assuming this change is not harmful to other usages. However, it does make the assumption that the item's current (holding) branch is the branch we want to see information about. I'm extremely leery about making a change in a major C4 subroutine to fix such a minor display issue, but in all likelihood, it's *probably* safe, since the only information being pulled in from the branches table is branchurl.
However, I think a further improvement can be made. The query only selects branchurl, none of the other branch information. Later in the subroutine, another query is done EACH ITEM RETURNED by the query (which is all the items for the biblio). Here's the code:
#get branch information.....
my $bsth = $dbh->prepare(
"SELECT * FROM branches WHERE branchcode = ?
"
);
$bsth->execute( $data->{'holdingbranch'} );
if ( my $bdata = $bsth->fetchrow_hashref ) {
$data->{'branchname'} = $bdata->{'branchname'};
}
This is using holdingbranch, which explains why the branchname is indeed the holdingbranch.
Improvement: Remove this extra check altogether, and just pull in the branchname from the original query, along side branchurl. This will reduce the number of queries MySQL must perform, improving (however marginally) the performance of this subroutine. Since it's a major workhorse of a subroutine (called many time all over the codebase), the net effect could be substantial.
(In reply to comment #2) > I'm extremely leery about making a change in a major C4 subroutine to fix such > a minor display issue, but in all likelihood, it's *probably* safe, since the > only information being pulled in from the branches table is branchurl. Ian: my proposal would be to mark "passed QA" and open another bug to improve the sub behaviour. ( Nicole: +++ for the screencast. Would be perfect if you could speak a little bit more slowly for poor ppl not speaking english natively ;-) ) GetItemsInfo is used in the following scripts: ./C4/XSLT.pm ./C4/Reserves.pm ./reports/reservereport.pl ./labels/label-item-search.pl ./virtualshelves/sendshelf.pl ./catalogue/moredetail.pl ./catalogue/detailprint.pl ./catalogue/detail.pl ./basket/basket.pl ./basket/sendbasket.pl ./serials/routing-preview.pl ./misc/migration_tools/rebuild_zebra.pl ./opac/opac-reserve.pl ./opac/opac-sendbasket.pl ./opac/opac-detail.pl ./opac/opac-ISBDdetail.pl ./opac/opac-sendshelf.pl Of those, only ./reports/reservereport.pl uses the branchname info, so I think that that whole section of GetItemsInfo can be safely removed. I'm going to mark this patch as Passed QA, and we can move the discussion of improving GetItemsInfo to another report. Pushed, please test well |