Bug 6271 - GetItemIssue(s) : renewals in both items and issues tables
Summary: GetItemIssue(s) : renewals in both items and issues tables
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-27 13:31 UTC by Fridolin Somers
Modified: 2020-11-30 21:45 UTC (History)
5 users (show)

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


Attachments
Proposed patch (5.66 KB, patch)
2011-08-30 15:59 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2011-04-27 13:31:53 UTC
In Circulation.pm : 
GetItemIssue returns informations about issues and items using a LEFT JOIN : 
    SELECT * FROM issues LEFT JOIN items ON issues.itemnumber=items.itemnumber

The bug is that the fields "renewals" exists in both issues and items table.
With SELECT *, only one of the two fields will be retrieved (mostly issues.renewals).

There are other queries using JOIN between issues and items.

Two solutions : 
 - Not use SELECT * and define an alias for items.renewals, in each queries.
 - rename items.renewals in items.itemrenewals, in database model.

Regards,
Comment 1 Colin Campbell 2011-04-27 13:50:20 UTC
SELECT * is generally frowned on in db circles and issues.renewals and items.renewals describe the contents of these fields accurately. If one changes name theres not really a good way of recalling which one has been aliased (or why if dealing only with that table)
Comment 2 Fridolin Somers 2011-04-27 13:59:00 UTC
(In reply to comment #1)
> SELECT * is generally frowned on in db circles and issues.renewals and
> items.renewals describe the contents of these fields accurately. If one changes
> name theres not really a good way of recalling which one has been aliased (or
> why if dealing only with that table)

I agree,
the LEFT JOIN is only to get some fields from items.
We should change 
SELECT * in :
Comment 3 Fridolin Somers 2011-04-27 14:01:03 UTC
SELECT 
  issues.*,
  items.homebranch,
  items.holdingbranch,
  items.notforlaon,
  ...

(end of previous comment)
Comment 4 Fridolin Somers 2011-08-30 15:59:12 UTC
Created attachment 5225 [details] [review]
Proposed patch
Comment 5 Fridolin Somers 2011-08-30 16:03:57 UTC
Patch solves the problem by always adding "items.renewals AS totalrenewals" in queries where issues and items are joined.
This behavior comes from Member.pm : GetPendingIssues and GetAllIssues

Please test.

Reads,
Comment 6 dev_patches 2011-09-02 09:43:48 UTC
GBSD#4 taking that.
Comment 7 Paul Poulain 2011-10-06 15:14:13 UTC
QA comment :
  * OK, problem is real, but where can I check the differences ? (please, a test plan !)
  * There's another problem (that existed before this patch SELECT * FROM issues LEFT JOIN biblioitems LEFT JOIN items will return all xml, that is useless & require memory & CPU. I suggest opening another bug for this (Fridolyn, would you please ?)
  * why didn't you use the solution you've suggested in comment 3 ?

Marking failed QA until you've answered those concerns
Comment 8 Jonathan Druart 2015-02-20 11:07:42 UTC
Is the issue still exist?
Can we have an example?
Comment 9 Owen Leonard 2016-08-12 13:25:57 UTC
This issue still exists in that the query in Circulation.pm is still the same, and there are still renewals columns in both items and issues. I think we still have yet to see a test plan which shows how the bug manifests.
Comment 10 Jonathan Druart 2016-09-30 09:23:22 UTC
GetItemIssue has been fixed by
  commit 5b4752e027ce32492166c7018d44d4a687afae36
    Bug 8365: FIX Return issue values instead of item values

GetItemIssues is still buggy, but the only place 'renewals' is used is in catalogue/issuehistory.tt:
  [% IF ( issue.renewals ) %]

And it happens only if the itemnumber parameter is passed to the script. However this parameter is never passed from Koha.
That means we could remove the subroutine GetItemIssues as well as the related code in the pl and tt.

Would you agree to remove it?
Comment 11 Fridolin Somers 2016-11-24 07:54:08 UTC
(In reply to Jonathan Druart from comment #10)
> Would you agree to remove it?
Yep
Comment 12 Fridolin Somers 2020-03-24 15:30:47 UTC
Obsolete since Bug 17679 and Bug 17680