From 036232ce17a3ff957cd33e77f664e02f61f2c80d Mon Sep 17 00:00:00 2001 From: Lebbeous Fogle-Weekley Date: Wed, 3 Apr 2013 16:35:51 -0400 Subject: [PATCH] Fix various Traditional and holds-go-home best-hold sort orders Use copy's call number's owning_lib instead of copy's circ_lib Should compare checkin lib to copy's (call number's) owning_lib, not hold request lib. You might think the comparison should be to acp.circ_lib, but that doesn't work with floating copies (for non-floaters, acp.circ_lib should be equal to acp.call_number.owning_lib). approx is a more correct first determinant to give the behavior sites are used to. hprox can cause copies to be too eager to go home when there are holds with that copy's circ lib as its request lib (if that's what you want, then you do pick or create a sort-order with hprox near the top). Address a problem in the copy_has_not_been_home CTE. This expression was always meant to provide a TRUE or FALSE value as its lone result, but would return NULL in cases where copies had no transit history. Use pickup_lib, not request_lib, as the determinant of nearness-to-home. request_lib was used with the thinking that an item's "owning" patrons should have their wishes favored at holds-go-home time, even if where they wanted to send the copy was not actually home, but that's neither necessarily desired nor very intuitive. Clear up holds-go-home logic with better code AND add TechRef documentation with diagram in attempt to be as clear as possible. Signed-off-by: Lebbeous Fogle-Weekley Signed-off-by: Mike Rylander --- .../lib/OpenILS/Application/Circ/Holds.pm | 4 + .../Application/Storage/Publisher/action.pm | 155 +++++++++++------- Open-ILS/src/sql/Pg/950.data.seed-values.sql | 8 +- ...ata.best-hold-order-traditional-approx.sql | 67 ++++++++ docs/TechRef/Circ/holds-go-home.txt | 93 +++++++++++ 5 files changed, 263 insertions(+), 64 deletions(-) create mode 100644 Open-ILS/src/sql/Pg/upgrade/XXXX.data.best-hold-order-traditional-approx.sql create mode 100644 docs/TechRef/Circ/holds-go-home.txt diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm index 9ba8272fa6..fc43e1d89a 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Holds.pm @@ -3007,6 +3007,10 @@ sub find_nearest_permitted_hold { my $fifo = $U->ou_ancestor_setting_value($user->ws_ou, 'circ.holds_fifo'); + # the nearest_hold API call now needs this + $copy->call_number($editor->retrieve_asset_call_number($copy->call_number)) + unless ref $copy->call_number; + # search for what should be the best holds for this copy to fulfill my $best_holds = $U->storagereq( "open-ils.storage.action.hold_request.nearest_hold.atomic", diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm index baa7098e45..f18a254623 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/action.pm @@ -17,10 +17,12 @@ use OpenILS::Application::Circ::CircCommon; use OpenILS::Application::AppUtils; my $U = "OpenILS::Application::AppUtils"; -# used in build_hold_sort_clause() +# Used in build_hold_sort_clause(). See the hash %order_by_sprintf_args in +# that sub to confirm what gets used to replace the formatters, and see +# nearest_hold() for the main body of the SQL query that these go into. my %HOLD_SORT_ORDER_BY = ( pprox => 'p.prox', - hprox => 'actor.org_unit_proximity(%d, h.request_lib)', # $cp->circ_lib + hprox => 'actor.org_unit_proximity(%d, h.pickup_lib)', # $cp->call_number->owning_lib aprox => 'COALESCE(hm.proximity, p.prox)', approx => 'action.hold_copy_calculated_proximity(h.id, %d, %d)', # $cp,$here priority => 'pgt.hold_priority', @@ -29,18 +31,20 @@ my %HOLD_SORT_ORDER_BY = ( rtime => 'h.request_time', htime => q! CASE WHEN + last_event_on_copy.place <> %d AND copy_has_not_been_home.result - THEN actor.org_unit_proximity(%d, h.request_lib) + THEN actor.org_unit_proximity(%d, h.pickup_lib) ELSE 999 END - !, + !, # $cp->call_number->owning_lib x 2 shtime => q! CASE WHEN + last_event_on_copy.place <> %d AND copy_has_not_been_home_even_to_idle.result - THEN actor.org_unit_proximity(%d, h.request_lib) + THEN actor.org_unit_proximity(%d, h.pickup_lib) ELSE 999 END - !, + !, # $cp->call_number->owning_lib x 2 ); @@ -349,10 +353,10 @@ sub build_hold_sort_clause { my ($columns, $cp, $here) = @_; my %order_by_sprintf_args = ( - hprox => [$cp->circ_lib], + hprox => [$cp->call_number->owning_lib], approx => [$cp->id, $here], - htime => [$cp->circ_lib], - shtime => [$cp->circ_lib] + htime => [$cp->call_number->owning_lib, $cp->call_number->owning_lib], + shtime => [$cp->call_number->owning_lib, $cp->call_number->owning_lib] ); my @clauses; @@ -374,16 +378,69 @@ sub build_hold_sort_clause { # more order-by clauses after that. } - my ($ctes, $joins); + my ($ctes, $joins) = ("", ""); if ($ctes_needed >= 1) { - # For our first auxiliary query, the question we seek to answer is, "has - # our copy been circulating away from home too long?" Two parts to - # answer this question. + # Each CTE serves the next. The first is one version or another + # of last_event_on_copy, which is described in holds-go-home.txt + # TechRef, but it essentially returns place and time of the most + # recent transit or circ to do with a copy, and failing that it + # returns a synthetic event that means "here" and "now". + + if ($ctes_needed == 2) { + $ctes .= sprintf(q! +, last_event_on_copy AS ( -- combined circ and transit version + SELECT * + FROM ( + ( SELECT + TRUE AS concrete, + dest AS place, + COALESCE(dest_recv_time, source_send_time) AS moment + FROM action.transit_copy + WHERE target_copy = %d + ORDER BY moment DESC LIMIT 1 + ) UNION ( + SELECT + TRUE AS concrete, + COALESCE(checkin_lib, circ_lib) AS place, + COALESCE(checkin_time, xact_start) AS moment + FROM action.circulation + WHERE target_copy = %d + ORDER BY moment DESC LIMIT 1 + ) UNION + SELECT + FALSE AS concrete, + %d AS place, + NOW() AS moment + ) x ORDER BY concrete DESC, moment DESC LIMIT 1 +) !, $cp->id, $cp->id, $cp->call_number->owning_lib); + } else { + $ctes .= sprintf(q! +, last_event_on_copy AS ( -- circ only version + SELECT * FROM ( + ( SELECT + TRUE AS concrete, + COALESCE(checkin_lib, circ_lib) AS place, + COALESCE(checkin_time, xact_start) AS moment + FROM action.circulation + WHERE target_copy = %d + ORDER BY moment DESC LIMIT 1 + ) UNION SELECT + FALSE AS concrete, + %d AS place, + NOW() AS moment + ) x ORDER BY concrete DESC, moment DESC LIMIT 1 +) !, $cp->id, $cp->call_number->owning_lib); + } + + $joins .= q! + JOIN last_event_on_copy ON (true) + !; + + # For our next auxiliary query, the question we seek to answer is, + # "has our copy been circulating away from home too long?" # - # part 1: Have their been no checkouts at the copy's circ_lib since the + # Have there been no checkouts at the copy's circ_lib since the # beginning of our go-home interval? - # part 2: Was the last transit to affect our copy before the beginning - # of our go-home interval an outbound transit? i.e. away from circ-lib # [We use sprintf because the outer function that's going to send one # big query through DBI is blind to our process of dynamically building @@ -400,56 +457,33 @@ sub build_hold_sort_clause { circ.target_copy = %d AND circ.circ_lib = %d AND circ.xact_start >= NOW() - go_home_interval.value - ) IS NULL AND ( - -- part 2 - SELECT atc.dest <> %d FROM action.transit_copy atc - JOIN go_home_interval ON (true) - WHERE - atc.id = ( - SELECT MAX(id) FROM action.transit_copy atc_inner - WHERE - atc_inner.target_copy = %d AND - atc_inner.source_send_time < NOW() - go_home_interval.value - ) - ) AS result -) !, $cp->id, $cp->circ_lib, $cp->circ_lib, $cp->id); - $joins .= " JOIN copy_has_not_been_home ON (true) "; + ) IS NULL AS result +) !, $cp->id, $cp->circ_lib); + + $joins .= q! + JOIN copy_has_not_been_home ON (true) + !; } if ($ctes_needed == 2) { - # In this auxiliary query, we ask the question, "has our copy come home - # by any means that we can determine, even if it didn't circulate once - # it came home, in the time defined by the go-home-interval?" - # answer this question. Two parts to this too (besides including the - # previous auxiliary query). + # By this query, we mean to determine that the copy hasn't landed at + # home by means of transit during the go-home interval (in addition + # to not having circulated from home in the same time frame). # - # 1: there have been no homebound transits for this copy since the - # beginning of the go-home interval. - # 2: there have been no checkins at home since the beginning of - # the go-home interval for this copy + # There have been no homebound transits that arrived for this copy + # since the beginning of the go-home interval. $ctes .= sprintf(q! , copy_has_not_been_home_even_to_idle AS ( - SELECT - copy_has_not_been_home.response AND ( - -- part 1 - SELECT MIN(atc.id) FROM action.transit_copy atc - JOIN go_home_interval ON (true) - WHERE - atc.target_copy = %d AND - atc.dest = %d AND - atc.dest_recv_time >= NOW() - go_home_interval.value - ) IS NULL AND ( - -- part 2 - SELECT MIN(circ.id) FROM action.circulation circ - JOIN go_home_interval ON (true) - WHERE - circ.target_copy = %d AND - circ.checkin_lib = %d AND - circ.checkin_time >= NOW() - go_home_interval.value - ) IS NULL - AS result -) !, $cp->id, $cp->circ_lib, $cp->id, $cp->circ_lib); + SELECT result AND NOT ( + SELECT COUNT(*)::INT::BOOL + FROM action.transit_copy atc + WHERE + atc.target_copy = %d AND + (atc.dest = %d OR atc.source = %d) AND + atc.dest_recv_time >= NOW() - (SELECT value FROM go_home_interval) + ) AS result FROM copy_has_not_been_home +) !, $cp->id, $cp->circ_lib, $cp->circ_lib); $joins .= " JOIN copy_has_not_been_home_even_to_idle ON (true) "; } @@ -464,7 +498,8 @@ sub nearest_hold { my $self = shift; my $client = shift; my $here = shift; # just the ID - my $cp = shift; # now an object, formerly just the ID + my $cp = shift; # now an object with call_number fleshed, + # formerly just copy ID my $limit = int(shift()) || 10; my $age = shift() || '0 seconds'; my $fifo = shift(); diff --git a/Open-ILS/src/sql/Pg/950.data.seed-values.sql b/Open-ILS/src/sql/Pg/950.data.seed-values.sql index d693e6ce37..a8ddf6a09f 100644 --- a/Open-ILS/src/sql/Pg/950.data.seed-values.sql +++ b/Open-ILS/src/sql/Pg/950.data.seed-values.sql @@ -12489,15 +12489,15 @@ INSERT INTO config.org_unit_setting_type ( INSERT INTO config.best_hold_order ( name, - pprox, aprox, priority, cut, depth, rtime, htime, hprox + approx, pprox, aprox, priority, cut, depth, rtime ) VALUES ( 'Traditional', - 1, 2, 3, 4, 5, 6, 7, 8 + 1, 2, 3, 4, 5, 6, 7 ); INSERT INTO config.best_hold_order ( name, - hprox, pprox, aprox, priority, cut, depth, rtime, htime + hprox, approx, pprox, aprox, priority, cut, depth, rtime ) VALUES ( 'Traditional with Holds-always-go-home', 1, 2, 3, 4, 5, 6, 7, 8 @@ -12505,7 +12505,7 @@ INSERT INTO config.best_hold_order ( INSERT INTO config.best_hold_order ( name, - htime, hprox, pprox, aprox, priority, cut, depth, rtime + htime, approx, pprox, aprox, priority, cut, depth, rtime ) VALUES ( 'Traditional with Holds-go-home', 1, 2, 3, 4, 5, 6, 7, 8 diff --git a/Open-ILS/src/sql/Pg/upgrade/XXXX.data.best-hold-order-traditional-approx.sql b/Open-ILS/src/sql/Pg/upgrade/XXXX.data.best-hold-order-traditional-approx.sql new file mode 100644 index 0000000000..ddd525d551 --- /dev/null +++ b/Open-ILS/src/sql/Pg/upgrade/XXXX.data.best-hold-order-traditional-approx.sql @@ -0,0 +1,67 @@ +BEGIN; + +-- INSERT INTO config.upgrade_log (version) VALUES ('XXXX'); + +UPDATE config.best_hold_order +SET + approx = 1, + pprox = 2, + aprox = 3, + priority = 4, + cut = 5, + depth = 6, + rtime = 7, + hprox = NULL, + htime = NULL +WHERE name = 'Traditional' AND + pprox = 1 AND + aprox = 2 AND + priority = 3 AND + cut = 4 AND + depth = 5 AND + rtime = 6 ; + +UPDATE config.best_hold_order +SET + hprox = 1, + approx = 2, + pprox = 3, + aprox = 4, + priority = 5, + cut = 6, + depth = 7, + rtime = 8, + htime = NULL +WHERE name = 'Traditional with Holds-always-go-home' AND + hprox = 1 AND + pprox = 2 AND + aprox = 3 AND + priority = 4 AND + cut = 5 AND + depth = 6 AND + rtime = 7 AND + htime = 8; + +UPDATE config.best_hold_order +SET + htime = 1, + approx = 2, + pprox = 3, + aprox = 4, + priority = 5, + cut = 6, + depth = 7, + rtime = 8, + hprox = NULL +WHERE name = 'Traditional with Holds-go-home' AND + htime = 1 AND + hprox = 2 AND + pprox = 3 AND + aprox = 4 AND + priority = 5 AND + cut = 6 AND + depth = 7 AND + rtime = 8 ; + + +COMMIT; diff --git a/docs/TechRef/Circ/holds-go-home.txt b/docs/TechRef/Circ/holds-go-home.txt new file mode 100644 index 0000000000..6816574485 --- /dev/null +++ b/docs/TechRef/Circ/holds-go-home.txt @@ -0,0 +1,93 @@ +Holds Go Home +============= + +Outline +------- + +A copy prefers to fulfill a hold near its home when: + + - The last event for a copy was NOT at home *and* ... + - The copy has not circulated from home within the defined period *and* ... + - The copy has neither departed from home by transit nor arrived at home + by transit within the defined period. + +Definitions +----------- + +In the preceding section, some terms are used that want explanation. + +_Event_ refers to either a circulation or a transit related to a copy. An +event has only two qualities we care about: moment and place. + + - When the event comes from a circulation, _moment_ is checkin_time if + we have it, otherwise xact_start. When the event comes from a transit, + moment is dest_recv_time if we have it, otherwise source_send_time. + + - When the event comes from a circulation, _place_ is checkin_lib if we + have it, otherwise circ_lib. When the event comes from a transit, + place is dest, *always*. + + - When the copy in question has neither any transit history nor any + circulation history, we produce a synthetic event with _moment_ equal + to the present time, and _place_ equal to the copy's call number's + owning_lib (see 'home' below). + +_Home_ is the value of the copy's call number's owning_lib field, which +incidentally is usually equal to the copy's circ_lib field except where +floating is in use. + +_The defined period_ is the time between the present moment (_NOW()_ +to the database) and the present moment less the value of the interval +defined in the _circ.hold_go_home_interval_ org unit setting at a scope +of the copy's _home_. E.g., if the setting contains the string "6 months", +the defined period is the last 6 months before the present moment, or +anything greater than _NOW() - '6 months'::INTERVAL_. + +Logic +----- + +............................................ + + ------- +| Event | + ------- + | + | + v + ------------------------- ----------------------- +| Was last event at home? | -----Yes.-----> | Don't try to go home. | + ------------------------- ----------------------- + | ^ ^ + | No. | | + v | | + ------------------------------------------------ | | +| Did copy circ from home during defined period? |--Yes.--/ | + ------------------------------------------------ | + | | + | No. | + | | + v | + ------------------------------------------------------ | +| Did copy leave or arrive home during defined period? |--Yes.---/ + ------------------------------------------------------ + | + | No. + | + v + ---------- +| Go home. | + ---------- + +............................................ + + +Implications in Best-Hold Selection Sort Order +---------------------------------------------- + +The calculations described above are all embodied in the best-hold selection +sort order determinant _shtime_. + +_htime_ is a simpler version of the same, with all reference to transits +removed, considering only circulations. This means events become thin +circulations, the "Did a transit bring copy home..." step in the flow chart +goes away, etc. etc. -- 2.43.2