Evergreen

Bug #1198465
Comment #20

Comment 20 for bug 1198465

Revision history for this message

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

#20

Hello Kathy,

First, let me clarify how I produced the one issue you ask about. In brief, I did everything the same as you, except I only set the "Prohibit negative balance on bills (DEFAULT)" setting. I am not sure if that's the key, but that's the only thing I see that I did differently. For the record, I also had failures when testing lost-item-returned behavior (it never generated a negative balance even when the settings were false).

In reality, though, these particular testing failures were just two among several factors in why this got pushed back. I'm wearing two different hats here, and am really trying hard to fulfill my duties to each.

As RM, my job is to make sure the release is smooth and timely. Excluding things is no fun for anyone, but the only other option is to keep pushing back deadlines (which directly hurts timeliness) or push in code which may need more development time (which can hurt both smoothness and timeliness). The fact that I find excluding code so distasteful leads me to wear my reviewer/committer hat far more than I really have time to during these crunch times.

As a reviewer/committer, I am just one of many. None of the other committers fully took this on (at least not publicly), so I did my best to give it a fair review. In my initial review on 2/14, I thought we might get away with simply renaming some of the conventions in this code to preserve (at least for the future) the concept of voiding, but as I tried out some of those ideas, I began to feel that we were definitely losing something of value by combining voids/credits into a single implementation, particularly when considering how using this code as "credit" code would necessarily rewrite past void history. In other words, if voids and credits are different, we should keep them separate, and any attempt to combine them is going to sacrifice something from one or both. Now, if the code had been completely seamless and worked perfectly, I would have been more inclined to overlook my misgivings. Instead, I felt in the end that the problems I experienced were at least in part a consequence of these underlying design issues.

This branch seems to imply from the start that the current 'voided' logic is fundamentally flawed, but that is only true if you try to make it do more than it was meant to do. My overall recommendation (as an EG dev, not an RM) for this code would be:

1) Scale back the scope of this change and restore the 'voided' pieces as they were before.
2) Keep the new payment type, but rename it something along the lines of 'account_credit' (still wishing for a better name). (We might also consider renaming 'credit_payment' to 'patron_credit_payment' for clarity.)
3) Rather than attempt to redo all the void logic, make the minimal amount of strategic changes necessary to branch and apply credits in situations where voiding would produce a negative balance. I'd think this would allow us to preserve a significant portion of the work done for this branch, but have it applied in a less pervasive way.

This is all just my opinion of a direction I would be comfortable committing to master. The fewer changes we can make, and the less we try to do too much, the fewer bugs we will surface and the less risk this code will carry. If others feel strongly that another direction is better, then all interested should continue working out what that direction should be.

In the end, it is easy to believe that the review for this bug resulted in the feature being delayed, but in fact, we are really instead that much closer to getting this in. Perhaps it goes without saying, but this feature now has a better chance of being in 2.7 than any other feature not yet committed, so I hope that reality can temper some of the disappointment we all feel at this time.

Sincerely,
Dan

Hello Kathy,

First, let me clarify how I produced the one issue you ask about.  In brief, I did everything the same as you, except I only set the "Prohibit negative balance on bills (DEFAULT)" setting.  I am not sure if that's the key, but that's the only thing I see that I did differently.  For the record, I also had failures when testing lost-item-returned behavior (it never generated a negative balance even when the settings were false).

In reality, though, these particular testing failures were just two among several factors in why this got pushed back.  I'm wearing two different hats here, and am really trying hard to fulfill my duties to each.

As RM, my job is to make sure the release is smooth and timely.  Excluding things is no fun for anyone, but the only other option is to keep pushing back deadlines (which directly hurts timeliness) or push in code which may need more development time (which can hurt both smoothness and timeliness).  The fact that I find excluding code so distasteful leads me to wear my reviewer/committer hat far more than I really have time to during these crunch times.

As a reviewer/committer, I am just one of many.  None of the other committers fully took this on (at least not publicly), so I did my best to give it a fair review.  In my initial review on 2/14, I thought we might get away with simply renaming some of the conventions in this code to preserve (at least for the future) the concept of voiding, but as I tried out some of those ideas, I began to feel that we were definitely losing something of value by combining voids/credits into a single implementation, particularly when considering how using this code as "credit" code would necessarily rewrite past void history.  In other words, if voids and credits are different, we should keep them separate, and any attempt to combine them is going to sacrifice something from one or both.  Now, if the code had been completely seamless and worked perfectly, I would have been more inclined to overlook my misgivings.  Instead, I felt in the end that the problems I experienced were at least in part a consequence of these underlying design issues.

This branch seems to imply from the start that the current 'voided' logic is fundamentally flawed, but that is only true if you try to make it do more than it was meant to do.  My overall recommendation (as an EG dev, not an RM) for this code would be:

1) Scale back the scope of this change and restore the 'voided' pieces as they were before.
2) Keep the new payment type, but rename it something along the lines of 'account_credit' (still wishing for a better name).  (We might also consider renaming 'credit_payment' to 'patron_credit_payment' for clarity.)
3) Rather than attempt to redo all the void logic, make the minimal amount of strategic changes necessary to branch and apply credits in situations where voiding would produce a negative balance.  I'd think this would allow us to preserve a significant portion of the work done for this branch, but have it applied in a less pervasive way.

This is all just my opinion of a direction I would be comfortable committing to master.  The fewer changes we can make, and the less we try to do too much, the fewer bugs we will surface and the less risk this code will carry.  If others feel strongly that another direction is better, then all interested should continue working out what that direction should be.

In the end, it is easy to believe that the review for this bug resulted in the feature being delayed, but in fact, we are really instead that much closer to getting this in.  Perhaps it goes without saying, but this feature now has a better chance of being in 2.7 than any other feature not yet committed, so I hope that reality can temper some of the disappointment we all feel at this time.

Sincerely,
Dan