Bug 6600 - Library name linking wrong if current location is different
Summary: Library name linking wrong if current location is different
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-18 18:26 UTC by Nicole C. Engard
Modified: 2013-12-05 20:01 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed fix (1.62 KB, patch)
2011-07-19 13:20 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-07-18 18:26:46 UTC
When you have a URL for a branch it shows in the holdings table in the OPAC.  However it looks like the link is pulling from the home branch and the text is pulling from the current branch.  Both should be pulling from the 'current branch'

See here for an explanation: http://screencast.com/t/27tYlkbW
Comment 1 Owen Leonard 2011-07-19 13:20:56 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.
Comment 2 Ian Walls 2011-09-01 16:29:32 UTC
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.
Comment 3 Paul Poulain 2011-10-06 14:36:48 UTC
(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 ;-) )
Comment 4 Ian Walls 2011-10-06 15:45:49 UTC
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.
Comment 5 Chris Cormack 2011-10-06 22:39:45 UTC
Pushed, please test well