Bug 17781 - Improper branchcode set during renewal
Summary: Improper branchcode set during renewal
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Kyle M Hall (khall)
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 18572
  Show dependency treegraph
 
Reported: 2016-12-15 11:56 UTC by Kyle M Hall (khall)
Modified: 2017-12-07 22:20 UTC (History)
4 users (show)

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


Attachments
Bug 17781 - Improper branchcode set during renewal (2.60 KB, patch)
2016-12-15 12:03 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 17781 - Improper branchcode set during renewal (2.70 KB, patch)
2016-12-19 16:35 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 17781 - Improper branchcode set during renewal (5.20 KB, patch)
2016-12-22 14:01 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 17781 - Improper branchcode set during renewal (5.27 KB, patch)
2016-12-27 15:16 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 Kyle M Hall (khall) 2016-12-15 11:56:54 UTC
For no discernable reason, when AddIssue calls AddRenewal, it passes the branchcode generated from _GetCircControlBranch. Assume _GetCircControlBranch is set to return items.homebranch. So:
1) If an item owned by LibraryA is checked out at LibraryB, the statistic line branchcode will be LibraryB
2) If an item is renewed via the ajax datatables renewal function, the statistic line branchcode will be LibraryB
3) If an item is renewed via scanning the item into the checkout again, the statistic line branchcode will be *LibraryA*

This is clearly improper behavior. The renewal is taking place at LibraryB, so the branchcode passed to AddRenewal should be LibraryB, the logged in library. This also jives with the documentation for the subroutine.
Comment 1 Kyle M Hall (khall) 2016-12-15 12:03:46 UTC
Created attachment 58201 [details] [review]
Bug 17781 - Improper branchcode set during renewal

For no discernable reason, when AddIssue calls AddRenewal, it passes the
branchcode generated from _GetCircControlBranch. Assume
_GetCircControlBranch is set to return items.homebranch. So:
1) If an item owned by LibraryA is checked out at LibraryB, the
  statistic line branchcode will be LibraryB
2) If an item is renewed via the ajax datatables renewal function, the
  statistic line branchcode will be LibraryB the
3) If an item is renewed via scanning the item into the checkout again,
  statistic line branchcode will be *LibraryA*

This is clearly improper behavior. The renewal is taking place at
LibraryB, so the branchcode passed to AddRenewal should be LibraryB,
the logged in library. This also jives with the documentation for
the subroutine.

Test Plan:
1) Set CircControl to "the library the item is from" aka ( ItemHomeLibrary )
2) Set HomeOrHoldingBranch to 'The library the items is from" ( aka homebranch )
3) Create item with homebranch of LibraryA and holdingbranch of LibraryB
4) Set the logged in library to LibraryB
4) Check the item out to a patron at LibraryB
5) Note the statistics line has a branchcode of LibraryB
6) Check the item out again to trigger a renewal, renew the item
7) Note the statistic line has a branchcode of LibraryA!
8) Apply this patch
9) Repeat step 6
10) Note the statistics line has a branchcode of LibraryB!
Comment 2 Kyle M Hall (khall) 2016-12-19 16:35:39 UTC
Created attachment 58275 [details] [review]
Bug 17781 - Improper branchcode set during renewal

For no discernable reason, when AddIssue calls AddRenewal, it passes the
branchcode generated from _GetCircControlBranch. Assume
_GetCircControlBranch is set to return items.homebranch. So:
1) If an item owned by LibraryA is checked out at LibraryB, the
  statistic line branchcode will be LibraryB
2) If an item is renewed via the ajax datatables renewal function, the
  statistic line branchcode will be LibraryB the
3) If an item is renewed via scanning the item into the checkout again,
  statistic line branchcode will be *LibraryA*

This is clearly improper behavior. The renewal is taking place at
LibraryB, so the branchcode passed to AddRenewal should be LibraryB,
the logged in library. This also jives with the documentation for
the subroutine.

Test Plan:
1) Set CircControl to "the library the item is from" aka ( ItemHomeLibrary )
2) Set HomeOrHoldingBranch to 'The library the items is from" ( aka homebranch )
3) Create item with homebranch of LibraryA and holdingbranch of LibraryB
4) Set the logged in library to LibraryB
4) Check the item out to a patron at LibraryB
5) Note the statistics line has a branchcode of LibraryB
6) Check the item out again to trigger a renewal, renew the item
7) Note the statistic line has a branchcode of LibraryA!
8) Apply this patch
9) Repeat step 6
10) Note the statistics line has a branchcode of LibraryB!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: David Kuhn <kuhn@monterey.org>
Comment 3 Jonathan Druart 2016-12-20 16:33:51 UTC
Kyle, that changes will have more effects than just the statistic line. It will also modifies the branchcode condition for the item circulation alert (see C4::Circulation::AddRenewal:2991).
I don't think it is what we want.

Moreover we will need tests to cover this change.
Comment 4 Kyle M Hall (khall) 2016-12-22 14:01:46 UTC
Created attachment 58388 [details] [review]
Bug 17781 - Improper branchcode set during renewal

For no discernable reason, when AddIssue calls AddRenewal, it passes the
branchcode generated from _GetCircControlBranch. Assume
_GetCircControlBranch is set to return items.homebranch. So:
1) If an item owned by LibraryA is checked out at LibraryB, the
  statistic line branchcode will be LibraryB
2) If an item is renewed via the ajax datatables renewal function, the
  statistic line branchcode will be LibraryB the
3) If an item is renewed via scanning the item into the checkout again,
  statistic line branchcode will be *LibraryA*

This is clearly improper behavior. The renewal is taking place at
LibraryB, so the branchcode passed to AddRenewal should be LibraryB,
the logged in library. This also jives with the documentation for
the subroutine.

Test Plan:
1) Set CircControl to "the library the item is from" aka ( ItemHomeLibrary )
2) Set HomeOrHoldingBranch to 'The library the items is from" ( aka homebranch )
3) Create item with homebranch of LibraryA and holdingbranch of LibraryB
4) Set the logged in library to LibraryB
4) Check the item out to a patron at LibraryB
5) Note the statistics line has a branchcode of LibraryB
6) Check the item out again to trigger a renewal, renew the item
7) Note the statistic line has a branchcode of LibraryA!
8) Apply this patch
9) Repeat step 6
10) Note the statistics line has a branchcode of LibraryB!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: David Kuhn <kuhn@monterey.org>
Comment 5 Jonathan Druart 2016-12-27 15:16:27 UTC
Created attachment 58436 [details] [review]
Bug 17781 - Improper branchcode set during renewal

For no discernable reason, when AddIssue calls AddRenewal, it passes the
branchcode generated from _GetCircControlBranch. Assume
_GetCircControlBranch is set to return items.homebranch. So:
1) If an item owned by LibraryA is checked out at LibraryB, the
  statistic line branchcode will be LibraryB
2) If an item is renewed via the ajax datatables renewal function, the
  statistic line branchcode will be LibraryB the
3) If an item is renewed via scanning the item into the checkout again,
  statistic line branchcode will be *LibraryA*

This is clearly improper behavior. The renewal is taking place at
LibraryB, so the branchcode passed to AddRenewal should be LibraryB,
the logged in library. This also jives with the documentation for
the subroutine.

Test Plan:
1) Set CircControl to "the library the item is from" aka ( ItemHomeLibrary )
2) Set HomeOrHoldingBranch to 'The library the items is from" ( aka homebranch )
3) Create item with homebranch of LibraryA and holdingbranch of LibraryB
4) Set the logged in library to LibraryB
4) Check the item out to a patron at LibraryB
5) Note the statistics line has a branchcode of LibraryB
6) Check the item out again to trigger a renewal, renew the item
7) Note the statistic line has a branchcode of LibraryA!
8) Apply this patch
9) Repeat step 6
10) Note the statistics line has a branchcode of LibraryB!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: David Kuhn <kuhn@monterey.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Kyle M Hall (khall) 2016-12-28 13:35:56 UTC
Pushed to master for 17.05!
Comment 7 Katrin Fischer 2017-01-04 20:58:45 UTC
This patch has been pushed to 16.11.x and will be in 16.11.02.
Comment 8 Mason James 2017-01-27 02:39:59 UTC
Pushed to 16.05.x, for 16.05.08 release