Evergreen

Bug #1198465
Comment #28

Comment 28 for bug 1198465

Revision history for this message

Dan Wells (dbw2) wrote on 2014-02-26:

#28

Okay all, pushed a branch for anyone interested in checking it out:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1198465-conditional-negative-balances

Things worth mentioning:

1) I did a bunch or rebasing of the original branch to hopefully make it easier to digest. I kept Jason's authorship intact on the first group of commits, as there are no substantive changes, but there are significant omissions (to restore the old void code), and a few things renamed (e.g. void_payment -> adjustment_payment). Because of these changes, these commits have no sign-offs other than mine.
2) After that, I have added some commits which mostly move stuff up or down the function stack in order to better bifurcate the void vs "adjustment" logic. There are handful of bug fixes here and there, and some behavior is changed slightly.
3) The behavior changes are for a variety of reasons:
    a) The relationship between the interval and the general "prohibit" setting wasn't clear to me. In particular, which setting takes control if both are set? Because of this, the code currently treats the "generic" setting as the on/off switch to allow for adjustments, THEN considers the interval as a limit on these adjustments. This seemed more natural to me, but upon further review, I think the old code expected you to set one or the other, not both. I think the best thing to do here would be to follow precedent if there are similar overlapping settings already existing.
    b) I have not (yet) enabled the idea of void in every possible case. This is meant to favor simpler behavioral expectations over situational accuracy. Basically, you have three choices:
        1. No setting set = current true-void behavior, with one exception*
        2. "prohibit" setting set = "adjustment" behavior, even if no payments have been made
        3. "prohibit + interval" settings set = void behavior inside interval, adjustment behavior outside
    c) * The one place where the code now always "adjusts" (never voids) is for overdue->lost behavior. Overdue fines are adjusted to zero (if the option is on), not voided. I think this makes sense logically (they still happened, we are just not charging for them), and it makes it much easier to accurately reinstate these fines if the lost item is returned (if *that* option is on).
4) There are almost certainly bugs. This branch has been tested a fair amount, but also changed a fair amount while testing, so new bugs may have crept in in addition to those not yet tested for.
5) There are cases where the "right" thing isn't obvious, or where the system already had bugs. I think we should bring those up as we find them (I'm not recalling every detail right now), but we should also be willing to punt some things down the road, especially if they always existed.

Known TODOs:

1) There are still places where the code and interface say "void" where it isn't really a void anymore. These should be touched up.
2) We should consider adding an "Adjust to zero" right-click option to the billing interface to allow for manual application of this code. As it stands, adjustments only result from automated processes (e.g. triggered by checkin, mark lost, etc.).
3) Permissions are not quite right. It makes sense to me to only check permissions in methods exposed by the API. This aligns with the current void permissions in master, and would better allow for the desirable case of having voids/adjustments always happen via automated processes but still limit them in manual processes for certain users.

Hopefully from all this it is quite clear that this is a work in progress! I am sure I forgot to mention something. Testing, questions, concerns, and feedback are all welcome and encouraged. Thanks!

Okay all, pushed a branch for anyone interested in checking it out:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1198465-conditional-negative-balances

Things worth mentioning:

1) I did a bunch or rebasing of the original branch to hopefully make it easier to digest.  I kept Jason's authorship intact on the first group of commits, as there are no substantive changes, but there are significant omissions (to restore the old void code), and a few things renamed (e.g. void_payment -> adjustment_payment).  Because of these changes, these commits have no sign-offs other than mine.
2) After that, I have added some commits which mostly move stuff up or down the function stack in order to better bifurcate the void vs "adjustment" logic.  There are handful of bug fixes here and there, and some behavior is changed slightly.
3) The behavior changes are for a variety of reasons:
    a) The relationship between the interval and the general "prohibit" setting wasn't clear to me.  In particular, which setting takes control if both are set?  Because of this, the code currently treats the "generic" setting as the on/off switch to allow for adjustments, THEN considers the interval as a limit on these adjustments.  This seemed more natural to me, but upon further review, I think the old code expected you to set one or the other, not both.  I think the best thing to do here would be to follow precedent if there are similar overlapping settings already existing.
    b) I have not (yet) enabled the idea of void in every possible case.  This is meant to favor simpler behavioral expectations over situational accuracy.  Basically, you have three choices:
        1. No setting set = current true-void behavior, with one exception*
        2. "prohibit" setting set = "adjustment" behavior, even if no payments have been made
        3. "prohibit + interval" settings set = void behavior inside interval, adjustment behavior outside
    c) * The one place where the code now always "adjusts" (never voids) is for overdue->lost behavior.  Overdue fines are adjusted to zero (if the option is on), not voided.  I think this makes sense logically (they still happened, we are just not charging for them), and it makes it much easier to accurately reinstate these fines if the lost item is returned (if *that* option is on).
4) There are almost certainly bugs.  This branch has been tested a fair amount, but also changed a fair amount while testing, so new bugs may have crept in in addition to those not yet tested for.
5) There are cases where the "right" thing isn't obvious, or where the system already had bugs.  I think we should bring those up as we find them (I'm not recalling every detail right now), but we should also be willing to punt some things down the road, especially if they always existed.

Known TODOs:

1) There are still places where the code and interface say "void" where it isn't really a void anymore.  These should be touched up.
2) We should consider adding an "Adjust to zero" right-click option to the billing interface to allow for manual application of this code.  As it stands, adjustments only result from automated processes (e.g. triggered by checkin, mark lost, etc.).
3) Permissions are not quite right.  It makes sense to me to only check permissions in methods exposed by the API.  This aligns with the current void permissions in master, and would better allow for the desirable case of having voids/adjustments always happen via automated processes but still limit them in manual processes for certain users.

Hopefully from all this it is quite clear that this is a work in progress!  I am sure I forgot to mention something.  Testing, questions, concerns, and feedback are all welcome and encouraged.  Thanks!