Bug 9575 - Serious Holds Problem with IndependantBranches
Summary: Serious Holds Problem with IndependantBranches
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-08 16:14 UTC by Kyle M Hall
Modified: 2013-12-05 20:09 UTC (History)
5 users (show)

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


Attachments
Bug 9575 - Serious Holds Problem with IndependantBranches (3.50 KB, patch)
2013-02-08 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9575 - Serious Holds Problem with IndependantBranches (3.51 KB, patch)
2013-03-12 03:42 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 9575 - Serious Holds Problem with IndependantBranches [Alternative] (1.84 KB, patch)
2013-03-12 18:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9575 - Serious Holds Problem with IndependantBranches (1.90 KB, patch)
2013-03-25 17:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9575 - Serious Holds Problem with IndependantBranches (2.27 KB, patch)
2013-03-25 17:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9575 - Serious Holds Problem with IndependantBranches (2.58 KB, patch)
2013-05-09 16:19 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 9575 - Serious Holds Problem with IndependantBranches (2.78 KB, patch)
2013-05-14 06:17 UTC, Katrin Fischer
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 2013-02-08 16:14:05 UTC
If IndependantBranches is enabled, all holds for an item display the currently logged in library no matter what, and modifying the holds will set all pickup locations for all the holds to that logged in library!
Comment 1 Kyle M Hall 2013-02-08 16:19:58 UTC Comment hidden (obsolete)
Comment 2 Liz Rea 2013-03-12 03:42:17 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-03-12 10:58:55 UTC
QA comment:

Why do you modify 2 routines when you can just modify the pl file? To me, Getbranches($foo, $branch) is not consistent.

I think you can do something like
if ( C4::Context->preference('IndependantBranches') ) {
  $reserve{'branchloop'} = GetBranchDetail($res->{'branchcode'});
} else {
  $reserve{'branchloop'} = GetBranchesLoop($res->{'branchcode'});
}

Feel free to discuss if you disagree :)
Comment 4 Kyle M Hall 2013-03-12 18:31:45 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-03-12 18:32:55 UTC
(In reply to comment #3)
> QA comment:
> 
> Why do you modify 2 routines when you can just modify the pl file?

Only because I hadn't thought of that! Here's an alternative with that implementation.
Comment 6 Jonathan Druart 2013-03-13 08:32:46 UTC
Thanks Kyle.
I switch back to Needs Signed Off, because this alternative patch entirely differs from your first one.
Comment 7 Liz Rea 2013-03-14 07:39:50 UTC
Hi Kyle,

I with the alternative patch I seem to get empty pick up location drop downs without independant branches on. I didn't go any further than that.

Liz
Comment 8 Kyle M Hall 2013-03-25 17:19:15 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2013-03-25 17:25:25 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2013-05-09 16:19:18 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2013-05-12 18:06:42 UTC
Kyle,

I am sorry, but this doesn't work for me. I created 3 different holds with 3 different pickup libraries away from my logged in library. I turned on independentbranches and like you described, suddenly all holds showed my current branch as pickup library. But after applying the patch and refreshing the page several times, this is still the case.
Comment 12 Owen Leonard 2013-05-13 12:43:36 UTC
(In reply to comment #11)
> But after applying the patch and
> refreshing the page several times, this is still the case.

After you applied the patch did you start with a new set of three holds with different pickup locations?
Comment 13 Katrin Fischer 2013-05-14 06:08:07 UTC
Hi Kyle and Owen,

I skipped 5) - modifying the holds, because the pull downs were wrong and it seemed obvious that modifying would ruing the pickup location. But now I have problems reproducing the original problem with the wrong pull downs, I will try to investigate more later.
Comment 14 Katrin Fischer 2013-05-14 06:12:24 UTC
Ok, I think I have resolved it - the problem occurs not if your user is a superlibrarian. It does occur, when you don't have superlibrarian permissions.
Comment 15 Katrin Fischer 2013-05-14 06:17:17 UTC
Created attachment 18123 [details] [review]
[PASSED QA] Bug 9575 - Serious Holds Problem with IndependantBranches

If IndependantBranches is enabled, all holds for an item display the
currently logged in library no matter what, and modifying the holds will
set all pickup locations for all the holds to that logged in library!

Test plan:
 1) Disable IndependantBranches
 2) Place 3 holds on a record with 3 different pickup locations
 3) Enable IndependantBranches
 4) Refresh the holds page for that record, all the holds show now
    show a pickup location of the logged in branch
 5) Modify the order of the holds
 6) Disable IndependantBranches
 7) Refresh the holds, verify the holds all now have a pickup
    location of the logged in branch
 8) Apply the patch
 9) Repeat steps 1-3
10) Refresh the holds page for that record, all the holds should
    now show the correct pickup branch, but it cannot be changed
    as IndependantBranches is enabled.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
In my tests the above test plan only worked out if my user
had no superlibrarian permission. But then behaviour for
independentbranches off/on was all correct.
All tests and QA script pass.
Comment 16 Jared Camins-Esakov 2013-05-14 12:29:58 UTC
This patch has been pushed to master and 3.12.x.