On Sat, Sep 11, 2010 at 2:36 PM, Thomas Berezansky <[email protected]> wrote:
Uploaded a new copy of the patch to launchpad with an upgrade script and
other changes.
In regards to the "fail because we didn't find a complete matchpoint" part
and possibly needing to do something special for it.....what kind of special
thing has been done for the existing code? I don't recall seeing any
conditions wherein you couldn't just outright delete the current default
matchpoint and get the same error because something didn't match your more
specific ones. I think this comes down to being considered a configuration
error.
That's a definition we can accept, but it will be easier to break
things now that instead of deleting the "global" default matchpoint
you'd now need only edit it. As far as not being able to delete a
special one, there is a mechanism in AutoGrid (IIRC) which allows us
to protect any objects in the UI with a numeric pkey value below a
specified threshold. My thought was somehow requiring the needed
fields on that same matchpoint ... I'm not certain of the best
implementation path for that though. This is one of the "doesn't need
full resolution" points, in any case, because "don't do that" is a
simple educational approach for the time being.
For the circ_modifier piece, I don't recall intending to change that. I do
recall having noticed I had stripped part of that line off on accident when
passing through the file, but apparently didn't just use undo (I think I had
just typed some further changes) and thus didn't put it back correctly. The
new copy of the patch corrects that change.
Thanks!
As for the hold_ratio_fallthrough.....my working copy of the SQL is
different, and I recall fixing that oversight before doing my last set of
tests. I apparently didn't notice that my patch was before that fix. This is
probably also why the circulate.pm code changes aren't there.
That is also probably why I have a circ_matchpoint2.patch sitting here next
to the one I apparently uploaded. *facepalm*
Easy to do.
Still, the new copy should contain the changed code and the changes to that
check.
For the hold_ratio block(s), there is at least one processing change there,
in that hold_ratio wasn't being filled twice if both checks were set. I
swapped it back to two blocks and added a check to hopefully not do the
function call if we already filled the data in.
Right on. Some more notes:
I'd like berick to look at the changes to the AutoFieldWidget files
and EditPane. Bill, when you have a chance?
-----------------------------------------------------
- fieldOrder="['org_unit', 'active', 'grp',
'circ_modifier', 'marc_type', 'marc_form', 'marc_vr_format']"
+ fieldOrder="['id', 'active', 'grp', 'org_unit',
'copy_circ_lib', 'copy_owning_lib', 'is_renewal', 'juvenile_flag',
'circ_modifier', 'marc_type', 'marc_form', 'marc_vr_format',
'ref_flag', 'usr_age_lower_bound', 'usr_age_upper_bound', 'circulate',
'duration_rule', 'recurring_fine_rule', 'max_fine_rule',
'available_copy_hold_ratio', 'total_copy_hold_ratio', 'script_test']"
I'm unsure of the purpose of this change. Is it to expose the
"inheritiness" of not-fully-filled matchpoints? How wide/cluttered
does the grid act now for "normal" resolutions? IIRC, the reason for
the formerly short list of fields in the grid was to show just the
inclusion tests so you could find the rule (always fully fleshed) that
you need by just the selection criteria. Certainly, adding the
now-NULLable fields is a good idea, but I'm not sure that we want all
the fields. In particular, "id" and "script_test" needn't be here,
for sure. This is also one of those "doesn't need final resolution,
just want to have the discussion" things.
-----------------------------------------------------
Now it may seem that I'm just digging for nits ... I'm not. Just
something that jumped out at me. You don't need to worry about this,
I'm partly just making a note to myself if this is the version that
gets applied...
In the perl, you don't need to say {flesh=>0}, that's the default.
-----------------------------------------------------
@@ -401,8 +448,10 @@
-- Fail if the available copy-hold ratio is too low
IF circ_test.available_copy_hold_ratio IS NOT NULL THEN
- SELECT INTO hold_ratio * FROM
action.copy_related_hold_stats(match_item);
- IF hold_ratio.available_copy_ratio IS NOT NULL AND
hold_ratio.available_copy_ratio < circ_test.available_copy_hold_ratio
THEN
+ IF hold_ratio.hold_count IS NULL THEN
+ SELECT INTO hold_ratio * FROM
action.copy_related_hold_stats(match_item);
+ END IF;
+ IF circ_test.available_copy_hold_ratio IS NOT NULL AND
hold_ratio.available_copy_ratio IS NOT NULL AND
hold_ratio.available_copy_ratio < circ_test.available_copy_hold_ratio
THEN
result.fail_part :=
'config.circ_matrix_test.available_copy_hold_ratio';
result.success := FALSE;
done := TRUE;
The second instance of "circ_test.available_copy_hold_ratio IS NOT
NULL" is unnecessary.
-----------------------------------------------------
- RETURN matchpoint;
+ -- This is a successful matchpoint finding IF we have the
circulate button AND the rules
+ IF matchpoint.circulate IS NOT NULL AND matchpoint.duration_rule
IS NOT NULL AND matchpoint.recurring_fine_rule IS NOT NULL AND
matchpoint.max_fine_rule IS NOT NULL THEN
+ result.success = true;
+ result.matchpoint = matchpoint;
+ ELSE
+ result.success = false;
+ END IF;
+ -- For debug purposes, include the full considered row list regardless.
+ result.buildrows = row_list;
+ RETURN result;
I'd like to add the matchpoint, even if incomplete, to the failure
case, so we can see what it was when we decided it was not complete
enough.
My testing process was two fold. First I created a few additional users and
stuck them in different groups, made note of their IDs. Then I created some
items, made note of their IDs. Then I created a pile of deactivated rules,
some complete, some not. I then queried the DB via the
action.item_user_circ_test and action.item_user_renew_test functions that
the code would normally call for user/item/org unit combinations with only
the default rule active, only the complete rules active, and then with the
complete rules and varying sets of incomplete rules active, and checked to
make sure I got the results I was expecting. I did do at least three tests
with incomplete rules only, picking those that would generate complete as
well as incomplete results to make sure failure was happening properly.
Great, that sounds perfect for a pre-commit sanity check version of
testing. Do you happen to have a self-contained script (ideally
targetting an empty, or known-state, db) you used in this testing? If
we have that on hand, any changes can be tested against a known set of
"used to pass" tests, at least manually, without you (or me) having to
do it all over again.
Thanks, as dbs said, for sticking with it!
--miker
I then re-ran a small subset of those tests from the staff client by
checking things out and made sure it was returning the correct information
there.
Thomas Berezansky
Merrimack Valley Library Consortium
Quoting Mike Rylander <[email protected]>:
On Fri, Sep 10, 2010 at 11:03 PM, Thomas Berezansky <[email protected]>
wrote:
I believe I have completed work on this particular project. For those
interested here but not following launchpad, I submitted a patch there:
https://bugs.launchpad.net/evergreen/+bug/635463
Thanks, Thomas.
Right off the bat I want to make it clear that because this is a big
feature addition, and as it stands contains large schema changes
(which I'd also like to discuss more -- there are parts that surprised
me), I can't see this getting into 2.0.
With that out of the way, here are some points that jumped out at me
looking at the patch (relevant sections pasted in-line), in no
particular order:
------------------------------------------------
** It looks like the circ mod test for inclusion now falls through **
@@ -182,15 +188,11 @@
CASE WHEN m.usr_age_upper_bound IS NOT NULL
THEN 0.5 ELSE 0 END DESC LOOP
IF current_mp.circ_modifier IS NOT NULL THEN
- CONTINUE WHEN current_mp.circ_modifier <>
item_object.circ_modifier OR item_object.circ_modifier IS NULL;
+ CONTINUE WHEN current_mp.circ_modifier <>
item_object.circ_modifier;
END IF;
IF current_mp.marc_type IS NOT NULL THEN
- IF item_object.circ_as_type IS NOT NULL THEN
- CONTINUE WHEN current_mp.marc_type <>
item_object.circ_as_type;
- ELSE
- CONTINUE WHEN current_mp.marc_type <>
rec_descriptor.item_type;
- END IF;
+ CONTINUE WHEN current_mp.marc_type <>
COALESCE(item_object.circ_as_type, rec_descriptor.item_type);
END IF;
IF current_mp.marc_form IS NOT NULL THEN
This wasn't discussed, and seems surprising to me. If the potential
matchpoint has a circ mod and the object either has none or it's is
different from what the matchpoint wants, then we should move on.
However, I like the COALESCE optimization of the marc_type bit! :)
It may be that you intended to simplify the test without changing the
semantics, but because of how NULL acts in a <> comparison (or any
comparison for that matter) it changed the behavior. Consider this
simplified example:
=# select case when 1 <> null then true else false end;
case
------
f
(1 row)
The equivalent of the above (the change in your patch) would /not/
trigger the CONTINUE (which it should), where as the equivalent of the
below (the original code) does, obviously.
=# select case when 1 <> null or null is null then true else false end;
case
------
t
(1 row)
------------------------------------------------
** Concern about conditional ordering and logic when breaking out of
the <<usergroup>> loop **
+ EXIT usergroup WHEN full_depth = false
+ AND matchpoint.circulate IS NOT NULL
+ AND matchpoint.duration_rule IS NOT NULL
+ AND matchpoint.recurring_fine_rule IS NOT
NULL
+ AND matchpoint.max_fine_rule IS NOT NULL
+ AND hold_ratio_fallthrough OR (
+ matchpoint.total_copy_hold_ratio IS NOT
NULL
+ AND
matchpoint.available_copy_hold_ratio IS NOT NULL
+ );
The OR'd parts there really need to be grouped together. But I'm also
concerned that it's not doing what is intended. IIUC, we want to
force looking for non-NULL ratios when hold_ratio_fallthrough is
enabled, else we just take what we have when we've filled in the four
required bits. That WHEN clause reads to me like, "we're done if: we
have all required parts AND ( either we have ratios OR we need to find
ratios )". Perhaps:
EXIT usergroup WHEN (
full_depth = false
AND matchpoint.circulate IS NOT NULL
AND matchpoint.duration_rule IS NOT NULL
AND matchpoint.recurring_fine_rule IS NOT NULL
AND matchpoint.max_fine_rule IS NOT NULL
) AND (
NOT hold_ratio_fallthrough
OR (matchpoint.total_copy_hold_ratio IS NOT NULL AND
matchpoint.available_copy_hold_ratio IS NOT NULL)
);
------------------------------------------------
** Result set coming from action.find_circ_matrix_matchpoint **
- RETURN matchpoint;
+ -- This is a successful matchpoint finding IF we have the
circulate button AND the rules
+ IF matchpoint.circulate IS NOT NULL AND matchpoint.duration_rule
IS NOT NULL AND matchpoint.recurring_fine_rule IS NOT NULL AND
matchpoint.max_fine_rule IS NOT NULL THEN
+ result.success = true;
+ result.matchpoint = matchpoint;
+ ELSE
+ result.success = false;
+ END IF;
+ -- For debug purposes, include the full considered row list
regardless.
+ result.buildrows = row_list;
+ RETURN result;
The fact that we failed to find a complete constructed matchpoint is
certainly a failure case insofar as the database call is concerned.
However, we shouldn't simply fail to work. If there is no ultimate
"outer" matchpoint with all the information we need, the we need to
specify that somewhere. Either OU settings or global settings are
candidates for ultimate fall-back values. Another option, which
requires more magic in configuration but will require no changes to
the backend code you've presented, is to make a specific matchpoint
(with an id of, perhaps, 0) special and require it be tied to the
top-most OU, the top-most user group, and force it to have values for
the four fields we need.
------------------------------------------------
** Refactoring that complicates the code **
@@ -388,21 +435,16 @@
RETURN NEXT result;
END IF;
- -- Fail if the total copy-hold ratio is too low
- IF circ_test.total_copy_hold_ratio IS NOT NULL THEN
+ -- Fail if the total/available copy-hold ratio is too low
+ IF circ_test.total_copy_hold_ratio IS NOT NULL OR
circ_test.available_copy_hold_ratio IS NOT NULL THEN
SELECT INTO hold_ratio * FROM
action.copy_related_hold_stats(match_item);
- IF hold_ratio.total_copy_ratio IS NOT NULL AND
hold_ratio.total_copy_ratio < circ_test.total_copy_hold_ratio THEN
+ IF circ_test.total_copy_hold_ratio IS NOT NULL AND
hold_ratio.total_copy_ratio IS NOT NULL AND
hold_ratio.total_copy_ratio < circ_test.total_copy_hold_ratio THEN
result.fail_part :=
'config.circ_matrix_test.total_copy_hold_ratio';
result.success := FALSE;
done := TRUE;
RETURN NEXT result;
END IF;
- END IF;
-
- -- Fail if the available copy-hold ratio is too low
- IF circ_test.available_copy_hold_ratio IS NOT NULL THEN
- SELECT INTO hold_ratio * FROM
action.copy_related_hold_stats(match_item);
- IF hold_ratio.available_copy_ratio IS NOT NULL AND
hold_ratio.available_copy_ratio < circ_test.available_copy_hold_ratio
THEN
+ IF circ_test.available_copy_hold_ratio IS NOT NULL AND
hold_ratio.available_copy_ratio IS NOT NULL AND
hold_ratio.available_copy_ratio < circ_test.available_copy_hold_ratio
THEN
result.fail_part :=
'config.circ_matrix_test.available_copy_hold_ratio';
result.success := FALSE;
done := TRUE;
I don't see any functional change here, but it's certainly more
complicated to read and therefore maintain. I'd see this go back to
two blocks.
------------------------------------------------
** No changes to Circulate.pm included **
Certainly, because the return type of action.item_user_circ_test
changed, the Perl will need to lean how to use it. A simple diff
oversight, I'm sure.
------------------------------------------------
** No upgrade script **
That'll be critical to applying the change to trunk.
---------------------------------
Finally, will you describe your testing methodology? Did you set up
uninherited (fully filled) matchpoints on old and new DBs and confirm
that they work the same? Do you have some sample inherited
matchpoints that show the different features of the new design,
including the switch for ratio inheritance?
Note, not everything I mention above needs to have a complete
resolution before this goes into trunk. Getting it in to trunk will
mean more folks can poke at it, so I don't want to stall that any
longer than we must, but at least some discussion (if not significant
code changes) are warranted before commit, IMO.
Thanks again, Thomas. I'm very excited about this!
--miker
Thomas Berezansky
Merrimack Valley Library Consortium
Quoting Mike Rylander <[email protected]>:
There as been a /lot/ of discussion surrounding this functionality on
IRC over the last week, just to let everyone here know that it's not
being ignored.
More below.
On Tue, Sep 7, 2010 at 4:29 PM, Thomas Berezansky <[email protected]>
wrote:
My previous email included a patch that was, unfortunately, just the
tip
of
the iceberg, so to speak. I have since figured out a lot of what needs
to
be
done to do this on the backend.
However, in looking at what needs to be done, I came up with several
questions as to how to handle things.
Currently the circulation flag, duration rule, recurring fine rule, and
max
fine rule are on my list of things to have "fall through".
I am unsure as to whether or not to have the total copy hold ratio or
the
available copy hold ratio values fall through or not. They are not
"endpoint" data, but at the same time may be considered to be an
important
part of the checks.
In that regard, I see three logical options:
1 - Don't have them fall through. If they aren't set on the first
matchpoint
found, they aren't set.
2 - Have them fall through until we run out of rules or they are
filled,
like the four values above.
3 - Have them fall through, but only until we have filled the other
four
values. Once the other four values are filled we officially stop
checking
them.
I am partial to 1 or 3 in this case.
After discussion, option 3 seems sane and has gained general
consensus. One point I brought up was, how do you tell the system
that you do /not/ want to use an inherited test value and also don't
want to supply a value? IOW, what if you don't want to test a
copy-hold ratio and yet some broader rule does set a value for one of
these tests?
The answer is that a value of 0 (zero) acts the same as "don't test"
since all possible values will pass.
The other main issue I am seeing right now is the circ mod test. In
particular, the current system uses the matchpoint ID for the check. As
my
changes make it so that there is more than one matchpoint ID possibly
involved there are several options:
1 - Negate the check when there is more than one matchpoint used in the
final result
2 - Use only the first matchpoint found for the check
3 - Have it use all matchpoints that were considered for the check
(were
valid and before we filled in our values)
4 - Have it use all matchpoints that contributed to the check (were
valid,
before we filled in our values, AND set at least one value)
I am partial to 3 in this case, as that would allow for creating a
matchpoint that would be specifically for triggering that limit,
without
changing any of the resulting rules.
This question has not been addressed yet. I think that without
broader input, and in fact outside requests, we need to sit on this
one. I understand the argument for the flexibility of 3, but these
matchpoints and rulesets can already be very complicated, and the
potential for SAAD is high. Consider:
* matchpoint 1 (closest match) supplies all but max fine rule, and
has a circ-mod test for "book" that limits the user to 10 items out
* matchpoint 2 (fall-through by group) supplies the max fine rule,
and also has a circ-mod test for "book", but sets the limit at 5 items
out
Which do we honor? And, like the question about copy-hold ratios, how
do we say "do NOT test per-circ-mod items out at all"? There are no
magic values here, and the benefit of flexibility IMO is outweighed by
the complexity of getting it right from a user perspective.
Any thoughts?
All that said, I think this is some awesome work. Thanks, Thomas!
--miker
Thomas Berezansky
Merrimack Valley Library Consortium
Quoting Thomas Berezansky <[email protected]>:
Theory:
Make the things returned (circulate, duration_rule,
recurring_fine_rule,
max_fine_rule) "fall through" when set to NULL.
The attached patch attempts to make this happen on the back end, but
provides no front end interface changes for configuring it.
IT HAS NOT BEEN TESTED, mainly because I don't want to screw with our
test system right now when others may be trying to test existing
functionality.
It also adds in the ability to pass in one more (optional) boolean
parameter to the function to return the entire list of rules used to
create the final result, intended for "debug/test" front end
functionality
to show what rules were considered in the fall-through checking.
Pros:
You can override a subset of those fields with a specific rule while
allowing broader rules to fill in the holes.
This may result in less duplication of information across rules,
making
things easier to maintain.
Thus, this may result in less rules in general, and thus less
processing
time on sorting them overall.
Cons:
Manually figuring out the specifics of what will happen will take more
time/effort.
Changing a single rule may have a greater unintended effect on other
rules.
Staff would need training for when to have a rule fall through and
when
to set it.
More time to return from the DB for any rule that is "falling through"
to
broader rules.
Examples for the following org tree:
CONS
-SYSA
--LIBC
--LIBD
-SYSB
--LIBE
--LIBF
Implementing the following "business" rules:
At the CONS level:
By default, everything circulates, uses DFLT_DUR duration, DFLT_RFINE
recurring fine, and DFLT_MFINE max fine.
Circ Modifier "book" uses the duration BOOK_DUR
Reference flagged materials don't circulate
At the SYSA level there are no special rules.
At the SYSB level the max fine should be SYSB_MFINE.
At the LIBC level the recurring fine is LIBC_RFINE
At the LIBD level circ modifier "book" uses the DFLT_DUR duration
instead
of "BOOK_DUR"
At the LIBE level reference flagged materials circulate.
At the LIBF level there are no special rules.
The current method would require the following circ rules to implement
those business rules:
CIRC_LIB CIRC_MOD REFERENCE CIRC? DURATION_RULE RECURRING_FINE
MAX_FINE
CONS NULL NULL TRUE DFLT_DUR DFLT_RFINE
DFLT_MFINE
CONS NULL TRUE FALSE DFLT_DUR DFLT_RFINE
DFLT_MFINE
CONS book NULL TRUE BOOK_DUR DFLT_RFINE
DFLT_MFINE
CONS book TRUE FALSE BOOK_DUR DFLT_RFINE
DFLT_MFINE
SYSB NULL NULL TRUE DFLT_DUR DFLT_RFINE
SYSB_MFINE
SYSB NULL TRUE FALSE DFLT_DUR DFLT_RFINE
SYSB_MFINE
SYSB book NULL TRUE BOOK_DUR DFLT_RFINE
SYSB_MFINE
SYSB book TRUE FALSE BOOK_DUR DFLT_RFINE
SYSB_MFINE
LIBC NULL NULL TRUE DFLT_DUR LIBC_RFINE
DFLT_MFINE
LIBC NULL TRUE FALSE DFLT_DUR LIBC_RFINE
DFLT_MFINE
LIBC book NULL TRUE BOOK_DUR LIBC_RFINE
DFLT_MFINE
LIBC book TRUE FALSE BOOK_DUR LIBC_RFINE
DFLT_MFINE
LIBD book NULL TRUE DFLT_DUR DFLT_RFINE
DFLT_MFINE
LIBD book TRUE FALSE DFLT_DUR DFLT_RFINE
DFLT_MFINE
LIBE NULL NULL TRUE DFLT_DUR DFLT_RFINE
SYSB_MFINE
LIBE book NULL TRUE BOOK_DUR DFLT_RFINE
SYSB_MFINE
16 circ rules total.
The new method would require the following circ rules to implement
those
business rules:
CIRC_LIB CIRC_MOD REFERENCE CIRC? DURATION_RULE RECURRING_FINE
MAX_FINE
CONS NULL NULL TRUE DFLT_DUR DFLT_RFINE
DFLT_MFINE
CONS book NULL NULL BOOK_DUR NULL NULL
CONS NULL TRUE FALSE NULL NULL NULL
SYSB NULL NULL NULL NULL NULL
SYSB_MFINE
LIBC NULL NULL NULL NULL LIBC_RFINE NULL
LIBD book NULL NULL DFLT_DUR NULL NULL
LIBE NULL TRUE TRUE NULL NULL NULL
7 circ rules total.
Starting with the above, lets assume that SYSA wants to change their
recurring fine to SYSA_RFINE.
LIBC's recurring fine is to be unchanged.
The current method requires the following changes:
ADD the following entries:
CIRC_LIB CIRC_MOD REFERENCE CIRC? DURATION_RULE RECURRING_FINE
MAX_FINE
SYSA NULL NULL TRUE DFLT_DUR SYSA_RFINE
DFLT_MFINE
SYSA NULL TRUE FALSE DFLT_DUR SYSA_RFINE
DFLT_MFINE
SYSA book NULL TRUE BOOK_DUR SYSA_RFINE
DFLT_MFINE
SYSA book TRUE FALSE BOOK_DUR SYSA_RFINE
DFLT_MFINE
UPDATE the LIBD entries:
CIRC_LIB CIRC_MOD REFERENCE CIRC? DURATION_RULE RECURRING_FINE
MAX_FINE
LIBD book NULL TRUE DFLT_DUR SYSA_RFINE
DFLT_MFINE
LIBD book TRUE FALSE DFLT_DUR SYSA_RFINE
DFLT_MFINE
4 rules added, 2 changed, total is now 20 rules.
The new method would require the following changes:
ADD the following entry:
CIRC_LIB CIRC_MOD REFERENCE CIRC? DURATION_RULE RECURRING_FINE
MAX_FINE
SYSA NULL NULL NULL NULL SYSA_RFINE NULL
1 rule added, 0 changed, total is now 8 rules.
Thomas Berezansky
Merrimack Valley Library Consortium
--
Mike Rylander
| VP, Research and Design
| Equinox Software, Inc. / The Evergreen Experts
| phone: 1-877-OPEN-ILS (673-6457)
| email: [email protected]
| web: http://www.esilibrary.com
--
Mike Rylander
| VP, Research and Design
| Equinox Software, Inc. / The Evergreen Experts
| phone: 1-877-OPEN-ILS (673-6457)
| email: [email protected]
| web: http://www.esilibrary.com
--
Mike Rylander
| VP, Research and Design
| Equinox Software, Inc. / The Evergreen Experts
| phone: 1-877-OPEN-ILS (673-6457)
| email: [email protected]
| web: http://www.esilibrary.com
