Bug 5911 - Transport Cost Matrix of transporting an item between branches
Summary: Transport Cost Matrix of transporting an item between branches
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: 3.10
Hardware: All All
: P3 enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 9922
  Show dependency treegraph
 
Reported: 2011-03-21 00:14 UTC by Srdjan Jankovic
Modified: 2014-05-26 21:04 UTC (History)
12 users (show)

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


Attachments
patch (63.51 KB, patch)
2011-03-31 00:42 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (64.45 KB, patch)
2011-07-21 04:02 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (65.50 KB, patch)
2012-02-22 08:37 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (64.19 KB, patch)
2012-05-15 00:21 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
p (64.34 KB, patch)
2012-05-16 02:45 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (64.56 KB, patch)
2012-05-17 00:45 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (66.38 KB, patch)
2012-05-24 06:13 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED OFF] Bug 5911 - Transport Cost Matrix (66.71 KB, patch)
2012-05-30 13:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_5911: make perlcdritic happy, shut up warnings (1.25 KB, patch)
2012-06-12 02:18 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5911: Followup - make perlcdritic happy, shut up warnings (1.33 KB, patch)
2012-06-22 12:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_5911: removed duplicate _flush_preferences(), and replaced with clear_syspref_cache() (1.23 KB, patch)
2012-07-10 03:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 5911 - Transport Cost Matrix (66.77 KB, patch)
2012-07-17 06:28 UTC, Mason James
Details | Diff | Splinter Review
bug_5911: Make tt_valid.t happy (1.48 KB, patch)
2012-07-18 06:09 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5911: Make tt_valid.t happy (1.54 KB, patch)
2012-07-23 10:23 UTC, Paul Poulain
Details | Diff | Splinter Review
bug_5911: Javascript and error messages corrections (2.86 KB, patch)
2012-07-23 23:57 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PATCH] bug_5911: Transport Cost Matrix (66.33 KB, patch)
2012-07-24 00:56 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5911: Transport Cost Matrix (66.27 KB, patch)
2012-07-26 00:11 UTC, Chris Cormack
Details | Diff | Splinter Review
bug_5911: Transport Cost Matrix (66.33 KB, patch)
2012-07-26 12:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Patch for bug 5911 (66.38 KB, patch)
2012-08-23 11:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
bug_5911 follow-up: removed debug() calls and ;; (2.44 KB, patch)
2012-08-24 00:51 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
bug_5911: Transport Cost Matrix (66.35 KB, patch)
2012-09-05 00:54 UTC, Srdjan Jankovic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2011-03-21 00:14:37 UTC
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.
Comment 1 Srdjan Jankovic 2011-03-31 00:42:07 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2011-07-20 23:12:40 UTC
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
Comment 3 Brendan Gallagher 2011-07-21 00:26:25 UTC
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.
Comment 4 Srdjan Jankovic 2011-07-21 04:02:28 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2012-02-17 14:39:09 UTC
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
Comment 6 Srdjan Jankovic 2012-02-22 08:37:51 UTC Comment hidden (obsolete)
Comment 7 Srdjan Jankovic 2012-05-15 00:21:23 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-05-15 14:03:53 UTC
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.
Comment 9 Kyle M Hall 2012-05-15 14:05:35 UTC
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?
Comment 10 Srdjan Jankovic 2012-05-16 02:45:13 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2012-05-16 05:06:12 UTC
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.
Comment 12 Srdjan Jankovic 2012-05-16 05:16:12 UTC
Then I'll move it to the sysprefs. Can you please tell me where exactly is that link so I can pinch it?
Comment 13 Katrin Fischer 2012-05-16 07:14:47 UTC
Hi Srdjan,
there is one on administration > patron attributes if ExtendedPatronAttributes is turned off.
Comment 14 Srdjan Jankovic 2012-05-17 00:45:02 UTC Comment hidden (obsolete)
Comment 15 Melia Meggs 2012-05-18 18:25:07 UTC
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.
Comment 16 Melia Meggs 2012-05-18 19:02:44 UTC
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?
Comment 17 Melia Meggs 2012-05-18 19:05:34 UTC
Sorry, one more question - are we supposed to be testing holds queue jump here as well?  Or is that separate?
Comment 18 Srdjan Jankovic 2012-05-21 01:14:43 UTC
(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.
Comment 19 Kyle M Hall 2012-05-21 15:05:04 UTC
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.
Comment 20 Srdjan Jankovic 2012-05-22 05:32:47 UTC
This is most inconvenient, because it passes here.
Can you please tell me how you run it?
Comment 21 Kyle M Hall 2012-05-22 12:29:19 UTC
(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.
Comment 22 Srdjan Jankovic 2012-05-24 06:13:21 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2012-05-30 13:52:55 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-05-30 13:54:56 UTC
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.
Comment 25 Paul Poulain 2012-06-09 13:06:11 UTC
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)
Comment 26 Srdjan Jankovic 2012-06-12 02:18:55 UTC Comment hidden (obsolete)
Comment 27 Srdjan Jankovic 2012-06-12 02:33:31 UTC
(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.
Comment 28 Melia Meggs 2012-06-20 22:14:01 UTC
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?
Comment 29 Srdjan Jankovic 2012-06-20 23:24:04 UTC
First one is OK as is, other one needs sign-off and then both need qa together I suppose.
Comment 30 Kyle M Hall 2012-06-22 12:28:05 UTC Comment hidden (obsolete)
Comment 31 Paul Poulain 2012-07-06 13:51:15 UTC
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)
Comment 32 Srdjan Jankovic 2012-07-10 03:36:32 UTC Comment hidden (obsolete)
Comment 33 Mason James 2012-07-17 06:28:18 UTC Comment hidden (obsolete)
Comment 34 Mason James 2012-07-17 06:33:50 UTC
(In reply to comment #33)
> Created attachment 10882 [details] [review]
> Bug 5911 - Transport Cost Matrix

i rebased this patch off master 39701b866676845e86cec0e1d5ca40a6b159288a
Comment 35 Mason James 2012-07-17 06:37:54 UTC
(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
Comment 36 Srdjan Jankovic 2012-07-18 06:09:54 UTC Comment hidden (obsolete)
Comment 37 Paul Poulain 2012-07-23 10:23:16 UTC Comment hidden (obsolete)
Comment 38 Paul Poulain 2012-07-23 10:38:47 UTC
QA comment: no more comments
Comment 39 Paul Poulain 2012-07-23 10:41:55 UTC
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...
Comment 40 Paul Poulain 2012-07-23 10:45:18 UTC
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
Comment 41 Srdjan Jankovic 2012-07-23 23:57:14 UTC Comment hidden (obsolete)
Comment 42 Srdjan Jankovic 2012-07-24 00:56:05 UTC Comment hidden (obsolete)
Comment 43 Kyle M Hall 2012-07-24 18:20:03 UTC
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
Comment 44 Srdjan Jankovic 2012-07-25 03:03:58 UTC
$ 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?
Comment 45 Kyle M Hall 2012-07-25 11:16:27 UTC
> 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'
Comment 46 Chris Cormack 2012-07-26 00:11:36 UTC Comment hidden (obsolete)
Comment 47 Chris Cormack 2012-07-26 00:12:29 UTC
(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.
Comment 48 Srdjan Jankovic 2012-07-26 00:16:09 UTC
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
Comment 49 Kyle M Hall 2012-07-26 12:51:37 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2012-08-23 11:44:38 UTC
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
Comment 51 Marcel de Rooy 2012-08-23 11:48:32 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2012-08-23 11:50:18 UTC
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.
Comment 53 Srdjan Jankovic 2012-08-24 00:51:53 UTC Comment hidden (obsolete)
Comment 54 Paul Poulain 2012-09-04 16:01:33 UTC
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)
Comment 55 Srdjan Jankovic 2012-09-05 00:54:46 UTC
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>
Comment 56 Srdjan Jankovic 2012-09-05 00:57:22 UTC
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.
Comment 57 Paul Poulain 2012-09-05 15:37:40 UTC
I mark passed QA, and will do as extensive tests as possible in the next days, before pushing
Comment 58 Paul Poulain 2012-09-12 13:30:56 UTC
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 !
Comment 59 Chris Cormack 2012-09-12 17:23:35 UTC
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
Comment 60 Liz Rea 2013-03-24 00:48:26 UTC
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.
Comment 61 Nicole C. Engard 2013-03-25 16:16:33 UTC
(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
Comment 62 Liz Rea 2013-03-26 02:43:12 UTC
Thanks Nicole, it was my weekend and I just didn't get to it. :)