Negative balance settings used in combination with one another should interact differently

Bug #1479110 reported by Kathy Lussier on 2015-07-28
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned

Bug Description

Evergreen version: master

 With the current "prohibit negative balances" code, if a library wants to only allow negative balances for 30 days after payment is made, they must enable the "Prohibit Negative Balances" setting and then set the interval setting to 30 days.

 I think most users will think "prohibit" means prohibit in all cases and that the interval settings would be used when the negative balances weren't prohibited. It would be more intuitive if the interval settings worked when the prohibit settings were either Unset or set to "false."

In either case, we should probably provide some guidance on what needs to be done in the description for the interval settings.

Dan Wells (dbw2) wrote :

I've looked this over, and I think it is more or less a two line change. I plan to go ahead and push a branch soon, but I'm not sure we really want to go that way. IMHO, it potentially makes it equally confusing from the opposite perspective (i.e. setting the prohibit setting to 'false' to try and stop the behavior, but it doesn't).

The real problem is that Evergreen doesn't have a decent way to manage settings. It isn't practical to treat every setting as fully independent (and we don't), but they sure look that way in our rudimentary interface. I think the goal of having one or just a few 'master' switches for optional features is sensible and desirable from the code side, but does get confusing from the setting interface side.

Does anyone have good thoughts on how to generally make clear "these switches enable behavior x" while "these switches tweak/modify behavior x"? I think that is the idea behind the current setup. Ideally they'd all be in a nice single configuration dialog for the feature, but we're far away from that.

Dan Wells (dbw2) wrote :

Okay, still wrapping my brain around all this. Here is the original spec by Jason:

"In the case where multiple settings from the above are activated, the more specific setting will take precedence. For example, if a library has set both the default and a setting for lost materials and a bill for lost materials is being handled, then the setting for lost materials will be used. Similarly, the settings that prohibit negative balances will take precedence over the interval settings for negative balances, so if both are set, then no negative balance is allowed."

The last sentence is the kicker, and probably where we've been inconsistent through the code changes. This document considers a "true" setting overriding the interval, but what about false? In other words, does the interval setting *only* matter when the other is unset?

I'm going to try to boil this down "graphically".

Kathy Lussier (klussier) wrote :

IMO the interval setting would work both when the prohibit setting is unset or set to false. When you're using the interval settings, you aren't prohibiting negative balances, you're just telling the system when they can be applied. It therefore makes intuitive sense (to me) that the interval settings could be used in both instances.

Maybe we could change the label for the prohibit settings to say "Always Prohibit Negative Balances" to make it clearer for the user?

Jason Stephenson (jstephenson) wrote :

If the label is changed, then it would make sense to make the settings mutually exclusive, or at least so the interval settings do not apply it the generic prohibit setting is applied.

Dan Wells (dbw2) wrote :

Okay, all, here is a document which outlines the options on the table. I'm only showing the "overdue" decision tree, but the "lost" tree would be identical:

https://docs.google.com/document/d/1SXJkRVYFHE6Vh5vblcqe-Hcn69pRXvbJraXE-lNE9rM/edit?usp=sharing

(There are certainly more options if we wish to slice and dice.)

I think all three make enough sense to be workable, and that whatever we go with could be made more clear with language tweaks. My take is that the strength of the current design is in cordoning off the new behavior. It is a "two-phase" model, so we only need to check at most two settings to answer the question, "Does this library ever allow negative balances?", which is the most common question we need an answer to in the code. Both of the other models require us to check all four settings to know for sure. At the evaluation stage, both of the first two models can successfully short-circuit after one test by using 'false', while model three *must* always check every layer to get to the only "do not prohibit" case. On the other hand, model three has the potential to be simpler due to binary vs. ternary logic if we can convince people that unset==false (and at the cost of nuance for nested settings).

Does this help? Can anyone point out a similar setting group which we could model? Being consistent would be a big factor if we can find a precedent.

Thanks!

Rogan Hamby (rogan-hamby) wrote :

I'm a bit torn. Honestly, the spec option number 2 in Dan's graphical represenation is the least intuitive to me. Option 3 is simpler than 1 and I want to prefer it but .... option 1, how it currently works, is probably how I would guess it works and more intuitive. I can't pull out of memory an example but I want to say that it's closer to how other options have worked elsewhere in Evergreen (though I can't think of the actual example off hand).

Kathy Lussier (klussier) wrote :

Thanks for weighing in Rogan! It looks like we have two people in favor of keeping the behavior as is. Unless somebody else weighs in later this morning with another opinion, I can create a branch this afternoon that modifies the descriptions of the settings to make it clear that the "prohibit" setting must be enabled in order for the interval settings to work.

Mike Rylander (mrylander) wrote :

First, I think all of the "in interval"s need to be changed to "outside interval"s. The intent of the interval, IIRC, was to stop allowing refunds after a certain amount of time had passed. IOW, for a while you can get a full refund (even if that leads to a negative balance) but if you don't return the item inside the window defined by the interval then you only get your outstanding balance waived.

Assuming that is correct, I believe 3 is the best option, and I vote for it strongly. It uses the least settings, assumes "truthiness" of the settings are what matters (for boolean settings), and is the most straightforward "stacking" of options.

Michele Morgan (mmorgan) wrote :

I am inclined to echo Rogan's comments. While option 3 put forth in the doc from comment #5 is simpler, logically, the current behavior is equally as intuitive as other ou setting relationships already in Evergreen, and exhaustively tested. I would tend to say keeping the current behavior with modified descriptions for the settings, making it clear how they interact would be the most expeditious approach in moving things forward.

Dan Wells (dbw2) wrote :

Yes, the document interval direction was just misworded. It works as expected.

The main thing I can't get over in #3 is that a person can set "Prohibit negative balance on bills for overdue materials" to 'false' and still have them prohibited, even though it's the most specific setting. In other words, the goal was to allow 'unset' to mean 'revert to default' for the nested settings, which I think is typical.

There is no difference in the number of settings. A branch is duplicated in #1 just to make it linear.

I've gone ahead and added a #4, which is #1 reformatted without the duplication, but shown in two phases instead. Seems a little more clear, to me anyway :)

Also, here are some thoughts I had about possible rewording:

Enable negative balance prohibition on bills for overdue materials
(instead of 'Prohibit negative balance on bills for overdue materials')

Allow negative balances within interval for overdues
(instead of 'Negative Balance Interval for Overdues')

The 'enable' makes the wording of the first a little softer, and better highlights the behavior instead of the action. That said, I'm not sure it helps much, and it seems a little more clumsy. On the other hand, I think the reword of the second helps quite a lot to express the gist of how it works right now.

Kathy Lussier (klussier) on 2015-08-06
Changed in evergreen:
assignee: nobody → Kathy Lussier (klussier)
Kathy Lussier (klussier) wrote :
tags: added: pullrequest
Changed in evergreen:
assignee: Kathy Lussier (klussier) → nobody
Changed in evergreen:
milestone: none → 2.9-beta
Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Jason Stephenson (jstephenson) wrote :

Pushed to master. Thanks, Kathy!

Changed in evergreen:
status: New → Fix Committed
Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers