Transport Cost Matrix The system shall contain a new administrative page that serves to manage the relative costs of transporting an item between branches. “Cost” does not directly correspond to currency, but rather is a measurement of the difficulty of transporting the item between the two branches. For each branch, it shall be possible to specify a numeric value indicating the 'cost' of transporting the item from that branch to every other branch. A toggle shall also be available to block all transport from any branch to any other branch. The toggle shall not change the 'cost' value associated with the transport between any two branches. Filling Holds using the Transport Cost Matrix A system preference shall be added to control whether holds are filled using the Transport Cost Matrix, or not. If the system is configured to use the Transport Cost Matrix for filling holds, then when attempting to fill a hold, the system will search for the lowest cost branch, and attempt to fill the hold with an item from that branch first. Branches of equal cost shall be selected from randomly. The branch or branches of the next highest cost shall be selected from only if all the branches in the previous group are unable to fill the hold. The system shall use the item's current holding branch when determining whether the item can fulfill a hold using the Transport Cost Matrix. This behaviour shall be overruled if the AutomaticItemReturn system preference is enabled.
Created attachment 3559 [details] [review] patch
Branch pushed new/awaiting_qa/bug_5911 http://git.koha-community.org/gitweb/?p=koha.git;a=shortlog;h=refs/heads/new/awaiting_qa/bug_5911
kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/admin/transport-cost-matrix.tt Is not found. The patch needs to be updated for the .tt version.
Created attachment 4689 [details] [review] patch
The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply". The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix. If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script. If the patch applies does not apply as I think, please rebase and resubmit, thanks
Created attachment 7799 [details] [review] patch
Created attachment 9576 [details] [review] patch
The new system preference UseTransportCostMatrix does not seem to be added to any of the system preference tabs. Also, wouldn't it be more understandable if the values want from say 0 to 100 rather than 0 to 1? You could just multiply the internal values by 100 for presentation, and divide presentation values by 100 for storage. I would also suggest changing 'No xfr' to 'Disable'. It's only one character longer and may be less confusing, especially for translators. A testing plan would also be helpful.
An addendum, why have both a system preference for the transport matrix, and a toggle checkbox for it on transport-cost-matrix.pl. Wouldn't one or the other be sufficient?
Created attachment 9600 [details] [review] p There's no actual reason for normalising costs to 1, so I've removed that. It is up to the libraries to ave them in particular range (0 to 1 or 100 or 1000). Toggle checkbox updates system preference. I left it out from the sysprefs screen for the reason not to have it in two places. I can put it there as well if you wish, but it makes sense to me to have it on that screen. Re test plan, my understanding was that Bywater would provide test plans for all holds rewrite work. I can provide you with a plan, but then we would be doing the same tests.
Hm, I am not sure about the idea to have a syspref outside of the system preference editor. While it is more convenient to toggle it directly on the feature page, it might also be a bit confusing. For other preferences like branchtransfers the page shows a message and a link to the system preference editor page where you can turn the feature on. I am not sure which is better, but the system preference editor would be more consistent with current practice.
Then I'll move it to the sysprefs. Can you please tell me where exactly is that link so I can pinch it?
Hi Srdjan, there is one on administration > patron attributes if ExtendedPatronAttributes is turned off.
Created attachment 9626 [details] [review] patch
I am not sure I understand well enough the complexities of home branch, holding branch, patron's branch, pickup branch, etc., but here is my attempt at a test plan. Community members (Ian in particular), please let me know if this sounds like the right idea. First test: 1. Go to Administration > Sys Prefs. Turn on the Transport Cost Matrix. Turn off AutomaticItemReturn. (Turning off AutomaticItemReturn means that we're going to be looking at holding branch, not home branch.) 2. Go to the "new administrative page that serves to manage the relative costs of transporting an item between branches." (I don't know where to find this page or what it looks like.) 3. Enter a numeric value for each branch (from 0 to 100). This numeric value indicates the "cost" of transporting an item from that branch to every other branch. Example: from Branch B to A - 1 from Branch C to A - 2 from Branch D to A - 2 4. Place four holds on next available item with pickup Branch A. Choose a title that has two items with current holding branch as Branch A. 5. Since the item is already there, the first hold should be filled by Branch A. 6. Since Branch A has another copy of the item, the second hold should also be filled by Branch A. 7. Since Branch A has no more copies of the item, the third hold should be filled by Branch B because it has the least cost of transport. 8. If Branch B has another copy of the item, the fourth should also be filled by Branch B. If Branch B has no more copies of the item, the fourth hold should be filled by either Branch C or D (random choice because they have equal cost of transport). ------------------- Second test: 1. Place four holds on next available item with pickup Branch A. Choose a title that has NO items with current holding branch of Branch A but one item with current holding branch of Branch B. 2. Since Branch A has no copies of this item, the first hold should be filled by Branch B because it has the least cost of transport. 3. If Branch B has no more copies of the item, the second, third and fourth holds should be filled by Branches C and D (in random order because they have equal cost of transport - how do we test if random?) ------------------- Third test: 1. Turn on AutomaticItemReturn sys pref. This means items will automatically be transferred back to their home branch when returned, so holding branch doesn't matter any more. 2. Place four holds on next available item with pickup Branch A. Choose a title that has 1 item with Branch A as home branch. 3. The first hold should be filled by Branch A. 4. Since Branch A has no more copies of the item, the second hold should be filled by Branch B. 5. If Branch B has another copy of the item, the third hold should be filled by Branch B. If Branch B has no more copies of the item, the fourth hold should be filled by either Branch C or D (random choice). ------------------- Fourth test: 1. Go back to the "new administrative page that serves to manage the relative costs of transporting an item between branches." Use the toggle to block all transport from Branch B to any other branch. (This means no items will ever be transferred from Branch B.) 2. Place four holds on next available item with pickup Branch A. Choose a title that has one item with Branch A as home branch and one item with Branch B as home branch. 3. The first hold should be filled by Branch A. 4. The second, third and fourth holds should be filled by Branches C and D (random choice). We are skipping over the item at Branch B because it cannot be transferred to Branch A for pickup (i.e. that item at Branch B can only ever fill holds with pickup branch as Branch B). -------------------- This is way, way complicated, but what we've got here is a situation in which the behavior of the system depends upon rules that are indicated by the numeric values in a matrix. Can't a computer test this better than I can? I believe we need automated testing (unit tests, at least). I would be happy to provide a sample matrix if you could include the data in the unit test which covers the code.
Actually, I see your test now, Srdjan, scrolling down to the bottom of the patch. I can't tell what you're doing with StaticHoldsQueueWeight and RandomizeHoldsQueueWeight. Would we just get rid of those if the library choses to work with the transport cost matrix? Or would they somehow work with the matrix? Or take precedence over the matrix? Also, what happens if you leave blank values in the matrix?
Sorry, one more question - are we supposed to be testing holds queue jump here as well? Or is that separate?
(In reply to comment #16) > I can't tell what you're doing with StaticHoldsQueueWeight and > RandomizeHoldsQueueWeight. Would we just get rid of those if the library > choses to work with the transport cost matrix? Or would they somehow work > with the matrix? Or take precedence over the matrix? Quite opposite. The matrix takes precedence, if the matrix is not used then we try StaticHoldsQueueWeight. > > Also, what happens if you leave blank values in the matrix? You shouldn't. An error should be thrown. I believe holds queue jump was a separate one, but I van be challenged on that. My mind is rather fried by now.
It looks like your automated test is failing in 2 places. 1..15 ok 1 - use C4::Reserves; ok 2 - use C4::HoldsQueue; ok 3 - take from homebranch (don't use cost matrix) pick up branch ok 4 - take from homebranch (don't use cost matrix) holding branch ok 5 - take from homebranch ( use cost matrix) pick up branch ok 6 - take from homebranch ( use cost matrix) holding branch ok 7 - take from holdingbranch (don't use cost matrix) pick up branch not ok 8 - take from holdingbranch (don't use cost matrix) holding branch # Failed test 'take from holdingbranch (don't use cost matrix) holding branch' # at t/db_dependent/HoldsQueue.t line 149. # got: 'CPL' # expected: 'MPL' # Wrong pick-up/hold: $VAR1 = 'MPL'; # $VAR2 = 'MPL'; # $VAR3 = [ # { # 'priority' => '1', # 'reservenotes' => undef, # 'reservedate' => '2012-05-20', # 'suspend_until' => undef, # 'expirationdate' => undef, # 'found' => undef, # 'reminderdate' => undef, # 'timestamp' => '2012-05-21 11:03:12', # 'biblionumber' => '5', # 'borrowernumber' => '19', # 'notificationdate' => undef, # 'branchcode' => 'MPL', # 'itemnumber' => undef, # 'constrainttype' => '', # 'lowestPriority' => '0', # 'suspend' => '0', # 'waitingdate' => undef, # 'cancellationdate' => undef # } # ]; # $VAR4 = [ # { # 'itemnumber' => '117', # 'biblionumber' => '5', # 'source_branchcode' => 'CPL', # 'borrowernumber' => '19', # 'item_level_request' => '0' # } # ]; # $VAR5 = [ # { # 'pickbranch' => 'MPL', # 'firstname' => 'Henry', # 'itemnumber' => '117', # 'itemcallnumber' => undef, # 'reservedate' => '2012-05-20', # 'barcode' => '463975280138', # 'phone' => '(212) 555-1212', # 'item_level_request' => '0', # 'surname' => 'Acevedo', # 'holdingbranch' => 'CPL', # 'biblionumber' => '5', # 'notes' => undef, # 'cardnumber' => '23529000035676', # 'title' => 'Test Holds Queue XXX', # 'borrowernumber' => '19' # } # ]; ok 9 - take from holdingbranch ( use cost matrix) pick up branch not ok 10 - take from holdingbranch ( use cost matrix) holding branch # Failed test 'take from holdingbranch ( use cost matrix) holding branch' # at t/db_dependent/HoldsQueue.t line 149. # got: 'UPL' # expected: 'MPL' # Wrong pick-up/hold: $VAR1 = 'MPL'; # $VAR2 = 'MPL'; # $VAR3 = [ # { # 'priority' => '1', # 'reservenotes' => undef, # 'reservedate' => '2012-05-20', # 'suspend_until' => undef, # 'expirationdate' => undef, # 'found' => undef, # 'reminderdate' => undef, # 'timestamp' => '2012-05-21 11:03:12', # 'biblionumber' => '5', # 'borrowernumber' => '19', # 'notificationdate' => undef, # 'branchcode' => 'MPL', # 'itemnumber' => undef, # 'constrainttype' => '', # 'lowestPriority' => '0', # 'suspend' => '0', # 'waitingdate' => undef, # 'cancellationdate' => undef # } # ]; # $VAR4 = [ # { # 'itemnumber' => '114', # 'biblionumber' => '5', # 'source_branchcode' => 'UPL', # 'borrowernumber' => '19', # 'item_level_request' => '0' # } # ]; # $VAR5 = [ # { # 'pickbranch' => 'MPL', # 'firstname' => 'Henry', # 'itemnumber' => '114', # 'itemcallnumber' => undef, # 'reservedate' => '2012-05-20', # 'barcode' => '463975280135', # 'phone' => '(212) 555-1212', # 'item_level_request' => '0', # 'surname' => 'Acevedo', # 'holdingbranch' => 'UPL', # 'biblionumber' => '5', # 'notes' => undef, # 'cardnumber' => '23529000035676', # 'title' => 'Test Holds Queue XXX', # 'borrowernumber' => '19' # } # ]; ok 11 - take from lowest cost branch (don't use cost matrix) pick up branch ok 12 - take from lowest cost branch (don't use cost matrix) holding branch ok 13 - take from lowest cost branch ( use cost matrix) pick up branch ok 14 - take from lowest cost branch ( use cost matrix) holding branch ok 15 - GetHoldsQueueItems # Looks like you failed 2 tests of 15.
This is most inconvenient, because it passes here. Can you please tell me how you run it?
(In reply to comment #20) > This is most inconvenient, because it passes here. > Can you please tell me how you run it? Just 'perl t/db_dependent/HoldsQueue.t' I apologize if I am doing this wrong. I have done unit testing with perl in a long time, and even then not much.
Created attachment 9752 [details] [review] patch Well done Kyle. The problem was in the AutomaticItemReturn. And the thing is that the code was ok, just test expectations were wrong... So I've added new tests, and it should be fine now.
Created attachment 9850 [details] [review] [SIGNED OFF] Bug 5911 - Transport Cost Matrix Create transport_cost table, added UseTransportCostMatrix syspref. transport_cost table contains branch to branch transfer costs. These are used for filling inter-branch hold transfers. Moved GetHoldsQueueItems() from .pl to HoldsQueue.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Automated tests passed. Manual tests passed. NOTE: This feature will not choose the correct branch if holds and/or pickup locations have changed since the last time build_holds_queue.pl has run. Perhaps C4::HoldsQueue::CreateQueue should be run each time a hold is added/modified to ensure timely correctness of the holds queue.
Correction, I wrote "correct branch" where I meant "correct item". Kyle (In reply to comment #23) > Created attachment 9850 [details] [review] > [SIGNED OFF] Bug 5911 - Transport Cost Matrix > > Create transport_cost table, added UseTransportCostMatrix syspref. > transport_cost table contains branch to branch transfer > costs. These are used for filling inter-branch hold transfers. > > Moved GetHoldsQueueItems() from .pl to HoldsQueue.pm > > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> > Automated tests passed. Manual tests passed. > > NOTE: This feature will not choose the correct branch > if holds and/or pickup locations have changed since the > last time build_holds_queue.pl has run. Perhaps > C4::HoldsQueue::CreateQueue should be run each time a > hold is added/modified to ensure timely correctness > of the holds queue.
QA comments: * perlcritic problem: C4/HoldsQueue.pm: Don't modify $_ in list functions at line 571, column 27. See page 114 of PBP. (Severity: 5) * why is this code in this patch? +sub _flush_preferences { + %sysprefs = (); +} * we're supposed not to add anything to C4::, but use Koha:: instead. You're creating HoldsQueue.pm. As we haven't defined a clear organization for Koha::, I can't say where this code should be, it's probably fair to have it in C4. Plus I don't want Koha:: be filled with C4:: like scripts, so won't reject the patch for this reason The line: # XXX GetHoldsQueueItems() does not support $itemtypeslimit! should be # FIXME GetHoldsQueueItems() does not support $itemtypeslimit! Just a question, not related to QA: if the syspref is OFF, how are things sorted ? as before ? (if yes, good point you've set the value to 0 by default, it means there won't be any change for users)
Created attachment 10241 [details] [review] bug_5911: make perlcdritic happy, shut up warnings
(In reply to comment #25) > QA comments: > * perlcritic problem: > C4/HoldsQueue.pm: Don't modify $_ in list functions at line 571, column 27. > See page 114 of PBP. (Severity: 5) perlcritic can be such a pain... > * why is this code in this patch? > +sub _flush_preferences { > + %sysprefs = (); > +} It is a poor man's sysprefs flush as per the comment above the code: # FIXME: running this under mod_perl will require a means of # flushing the caching mechanism I did not want to remove the comment because: a) this solution was created in anger, so may not fit all b) I did not address mod_perl/plack/ any other persistant env, just needed a way to change sysprefs for the test > > * we're supposed not to add anything to C4::, but use Koha:: instead. You're > creating HoldsQueue.pm. As we haven't defined a clear organization for > Koha::, I can't say where this code should be, it's probably fair to have it > in C4. Plus I don't want Koha:: be filled with C4:: like scripts, so won't > reject the patch for this reason When I started this, I did not even know of Koha namespace. Now I know, but not sure how to put things there :) > > The line: > # XXX GetHoldsQueueItems() does not support $itemtypeslimit! > > should be > # FIXME GetHoldsQueueItems() does not support $itemtypeslimit! I was not sure if that needs addressing, just wanted to add a note. I can change this if required. > > Just a question, not related to QA: if the syspref is OFF, how are things > sorted ? as before ? (if yes, good point you've set the value to 0 by > default, it means there won't be any change for users) Yes, if off it behaves as before. Default is 0. t/db_dependent/HoldsQueue.t tests both cases.
What is the next step here? Is the first patch staying as is, or does it require changes? Does the patch that stops the perlcritic warnings need to be signed off now?
First one is OK as is, other one needs sign-off and then both need qa together I suppose.
Created attachment 10431 [details] [review] [SIGNED-OFF] Bug 5911: Followup - make perlcdritic happy, shut up warnings Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
QA comments: 1- Your patch contains: +sub _flush_preferences { + %sysprefs = (); +} Look here =head2 clear_syspref_cache C4::Context->clear_syspref_cache(); cleans the internal cache of sysprefs. Please call this method if you update the systempreferences table. Otherwise, your new changes will not be seen by this process. =cut sub clear_syspref_cache { %sysprefs = (); } => _flush_preferences is a duplicate ;-) Please provide another follow-up, switch back to signed-off & I'll pass QA, I don't see any other problem (about C4/HoldQueue.pm, even if we've now defined a structure, I agree it's a little bit early to ask for rewriting)
Created attachment 10733 [details] [review] bug_5911: removed duplicate _flush_preferences(), and replaced with clear_syspref_cache()
Created attachment 10882 [details] [review] Bug 5911 - Transport Cost Matrix Create transport_cost table, added UseTransportCostMatrix syspref. transport_cost table contains branch to branch transfer costs. These are used for filling inter-branch hold transfers. Moved GetHoldsQueueItems() from .pl to HoldsQueue.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Automated tests passed. Manual tests passed. NOTE: This feature will not choose the correct branch if holds and/or pickup locations have changed since the last time build_holds_queue.pl has run. Perhaps C4::HoldsQueue::CreateQueue should be run each time a hold is added/modified to ensure timely correctness of the holds queue.
(In reply to comment #33) > Created attachment 10882 [details] [review] > Bug 5911 - Transport Cost Matrix i rebased this patch off master 39701b866676845e86cec0e1d5ca40a6b159288a
(In reply to comment #34) > (In reply to comment #33) > > Created attachment 10882 [details] [review] > > Bug 5911 - Transport Cost Matrix > > i rebased this patch off master 39701b866676845e86cec0e1d5ca40a6b159288a and a final small issue... transport-cost-matrix.tt fails the xt/tt_valid.t test $ prove xt/tt_valid.t xt/tt_valid.t .. 1/2 # Failed test 'TT syntax: not using TT directive within HTML tag' # at xt/tt_valid.t line 55. # Files list: # intranet-tmpl/prog/en/modules/admin/transport-cost-matrix.tt: 104
Created attachment 10919 [details] [review] bug_5911: Make tt_valid.t happy
Created attachment 11075 [details] [review] bug_5911: Make tt_valid.t happy Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
QA comment: no more comments
I tried to test this new feature, but can't make it work: When I reach the page /cgi-bin/koha/admin/transport-cost-matrix.pl I just get red cells, and the documentation says: "Red cells signify no transfer allowed" One comment : if cell is red, I should not be able to enter a value isn't it ? I can, but it's not saved when I click on "Save". Is it the intended behaviour ? (Sounds strange) I tried to get rid of the red cells, but couldn't. * the UseBranchTransferLimits doesn't change anything (I tried with both possible status) * Even if limits are enabled, I have added most transfer limits as enabled on /cgi-bin/koha/admin/branch_transfer_limits.pl, but everything is still red More explanations welcomed...
Other comment, that is a QA comment one: when I try to save the matrix, I get lot of: Invalid B8 -> B5 cost - nust be a number in 0 to 1 range, disablig at /home/paul/koha.dev/koha-community/C4/HoldsQueue.pm line 98. in my logs (for most cells, I let everything empty, should not get warning for empty cells, only for cells with a value) (the warn is confusing, because it says it nust (must ?) be a number in 0 to 1 range where the doc says "Costs are decimal values 0 to some arbitrarymax value (1 or 100), 0 being minimum (no) cost." marking failed QA for now. Srdjan, you can squash the 4 previous commits in one and provide a follow-up in a separate path, thx
Created attachment 11095 [details] [review] bug_5911: Javascript and error messages corrections
Created attachment 11096 [details] [review] [PATCH] bug_5911: Transport Cost Matrix I am deeply ashamed but I did not know how to squash those previous commits. I can list what I tried to provide some entertainment if you wish. Instead, I am providing a full patch. If anyone can help with some instructions I would appreciate. So the latest fix is "Javascript and error messages corrections", and one after is the full patch
Applying: Bug 5911 - Transport Cost Matrix error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt:53 error: koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt: patch does not apply Patch failed at 0001 Bug 5911 - Transport Cost Matrix
$ git checkout kc/master ... HEAD is now at 30ee49d... Bug 8376: New script to export borrowers misc/export_borrowers.pl $ git am 0001-bug_5911-Transport-Cost-Matrix.patch Applying: bug_5911: Transport Cost Matrix /home/srdjan/koha/dev/.git/rebase-apply/patch:131: trailing whitespace. /home/srdjan/koha/dev/.git/rebase-apply/patch:301: trailing whitespace. my $request_query = "SELECT biblionumber, borrowernumber, itemnumber, priority, reserves.branchcode, /home/srdjan/koha/dev/.git/rebase-apply/patch:375: trailing whitespace. return [ grep { /home/srdjan/koha/dev/.git/rebase-apply/patch:398: trailing whitespace. my %specific_items_requested = map { $_->{itemnumber} => 1 } /home/srdjan/koha/dev/.git/rebase-apply/patch:410: trailing whitespace. warning: squelched 13 whitespace errors warning: 18 lines add whitespace errors. So it went fine here. can you please tell me how you tried to apply it?
> So it went fine here. can you please tell me how you tried to apply it? I used 'git qa 5911' which is the same as 'git fetch origin && git checkout origin/master -b bug5911-qa && git bz apply 5911'
Created attachment 11140 [details] [review] bug_5911: Transport Cost Matrix Create transport_cost table, added UseTransportCostMatrix syspref. transport_cost table contains branch to branch transfer costs. These are used for filling inter-branch hold transfers. Moved GetHoldsQueueItems() from .pl to HoldsQueue.pm
(In reply to comment #45) > > So it went fine here. can you please tell me how you tried to apply it? > > I used 'git qa 5911' which is the same as 'git fetch origin && git checkout > origin/master -b bug5911-qa && git bz apply 5911' And does your git bz do a 3 way merge, if not it will report these errors. When a 3 way merge will either merge cleanly, or actually tell you where the error is. What I do is git bz apply, if it fails it leaves the patch in /tmp git am --abort To clean out the apply in process git am -iu3 /tmp/path_to_patch (git bz will tell you this path) Then if it results in conflicts (which this one doesnt, it merges cleanly) It will tell you the conflicts and you can paste that in. However what has occured here, is that srdjan forgot to obsolete the old patches, so you were trying to apply ones that are now out of date in and in the wrong order. git bz doesn't understand stuff like that. Ill tidy up the attachments, and then you should be good to go.
srdjan@ozzy:~/koha/dev (wr76711_patch)$ git checkout kc/master Note: checking out 'kc/master'. You are in 'detached HEAD' state. You can look around, make experimental changes and commit them, and you can discard any commits you make in this state without impacting any branches by performing another checkout. If you want to create a new branch to retain commits you create, you may do so (now or later) by using -b with the checkout command again. Example: git checkout -b new_branch_name HEAD is now at de07f00... Bug 7420 tiny bugfix srdjan@ozzy:~/koha/dev ((de07f00...))$ git bz apply 5911 Bug 5911 - Transport Cost Matrix of transporting an item between branches bug_5911: Transport Cost Matrix Apply? [yn] y Applying: bug_5911: Transport Cost Matrix srdjan@ozzy:~/koha/dev ((ff727ee...))$ So I'm not sure where to go from there
Created attachment 11144 [details] [review] bug_5911: Transport Cost Matrix Create transport_cost table, added UseTransportCostMatrix syspref. transport_cost table contains branch to branch transfer costs. These are used for filling inter-branch hold transfers. Moved GetHoldsQueueItems() from .pl to HoldsQueue.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
QA Comment: Larger patch with greater potential impact :) Have looked at the code only; depend on signoff for test. Code looks good to me. Although some vital code moved from pl to pm and has been changed, it seems that not using the matrix will not change current behavior. Passed QA
Created attachment 11777 [details] [review] Patch for bug 5911 Conflict resolved for updatedatabase
Additional note: Code may have a followup to clear some old debug statement (in the module the debug routine is commented, but still called couple of times). Also saw a double ;; somewhere.
Created attachment 11820 [details] [review] bug_5911 follow-up: removed debug() calls and ;;
Sorry, but patch does not apply anymore: CONFLICT (content): Merge conflict in misc/cronjobs/holds/build_holds_queue.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl The build_holds_queue.pl seems easy to do, but I don't want to do something wrong. Please provide a rebased fix (and back to passed QA)
Created attachment 11971 [details] [review] bug_5911: Transport Cost Matrix Create transport_cost table, added UseTransportCostMatrix syspref. transport_cost table contains branch to branch transfer costs. These are used for filling inter-branch hold transfers. Moved GetHoldsQueueItems() from .pl to HoldsQueue.pm Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
The conflict was with bug 4870. It was not too hard to merge, but setting to "Pushed for QA" in case you want to check.
I mark passed QA, and will do as extensive tests as possible in the next days, before pushing
Patch pushed to master I tried to find things that could have been broken by this feature (testing with syspref=off, without hold_queue at all), and could not find anything wrong. I can edit the admin/transport-matrix.pl correctly. I haven't tested the rest of the feature work perfectly, but no regression => patch pushed !
DBD::mysql::db do failed: Can't create table 'koha.transport_cost' (errno: 150) at ./installer/data/mysql/updatedatabase.pl line 5703. (150 is foreign key constraints) This might be specific to the jenkins machine, but just noting why it didnt update properly
I believe this patch actually does cause a regression - the hold queue report now selects items based only on the homebranch, and never for holding branch. Will file a new bug.
(In reply to comment #60) > I believe this patch actually does cause a regression - the hold queue > report now selects items based only on the homebranch, and never for holding > branch. > > Will file a new bug. I didn't see a bug from you Liz, but Kyle has patched this here: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9922
Thanks Nicole, it was my weekend and I just didn't get to it. :)