Void options in the billing UI allow negative balances to occur when they should be prohibited

[Kathy Lussier]

Evergreen version: 2.9RC

In https://bugs.launchpad.net/evergreen/+bug/1479107 and IRC chats, we reached a consensus to replace the manual void option with an adjust to zero option. However, in the rush to get the adjust to zero option into 2.9 in time for beta, it looks like we forgot to remove the manual void option.

The problem is that this option allows staff to inadvertently create negative balances, even if the "Prohibit Negative Balances" settings has been enabled for their site.

A working branch is forthcoming that removes these options from the UI in the patron bills interface. It also adds the adjust to zero option in the "Actions for Selected Transactions" menu.

There is still an option available in the bill details interface to void selected billings that should be replaced with an option to adjust the selected billings to zero. The code to do so is a bit above my head.

I don't know if we should try to get that code in here or if we should merge this fix as is and address the other UI in another bug report.

[Kathy Lussier (klussier)]

Working branch is available at:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp1494544-remove-void-balance-references-from-bills-ui

I'll add a pullrequest for this branch since we can address the other UI in another bug report.

[Ben Shum (bshum)]

Marking confirmed and targeting towards 2.9.0 for final review and inclusion.

Tested and the above commit works for me, though I also wonder about the Void Selected Billings option in the full details summary area. Will look further into that before committing this tomorrow.

[Dan Wells (dbw2)]

This was not an oversight, and I don't think this should be committed. Adjust to zero is not a replacement for libraries who still wish to manually void bills, which includes at least us. Instead, these options were meant to coexist, and are both designed to be controlled by permissions. In this case, if you do not want users to use the void option, taking away the "VOID_BILLING" permission should prevent it.

I know there is still some polish which needs to be applied to make the interface nicer, and I don't recall if the ADJUST perm ever got into the seed file. I think ideally the options should be disabled or hidden if you don't have the permission, and that should make everyone happy. I have an unfinished branch which started down that road, so I'll see about getting that finished up soon.

[Kathy Lussier (klussier)]

Hi Dan,

When I created this branch, I was going by the consensus that we reached in IRC. I didn't see any indication that there was a change in that consensus.

In my testing last night, I found that removing the "VOID_BILLING" permission prevented the user from adjusting the bill, so I'm thinking the new adjust permission hadn't made it in yet.

My preference is that lack of the void permission would hide the option to void a bill. If people see an option or see that it is disabled , they will want to click it. Right now, they can click the option to void the bill, and, IIRC, don't get a message saying they don't have permission until the last click in the process. This is very frustrating for users.

Is there any chance these changes could be made before the .0 release? When our users move to 2.9, they will be expecting that, once they enable that option, there will be no situations where a negative balance will be produced. As of now, this is not the case without taking away a permission that is there only means to adjusting a balance to zero.

Kathy

[Dan Wells (dbw2)]

Hello Kathy,

Looking back at that IRC conversation, I have to apologize to being sloppy in at least one thing I said, and maybe more. When I said at one point that the new menu option would "replace" the other, I really meant "replace based on configuration", e.g. settings and permissions. The goal all along has been to get these features to happily coexist as much as reasonable possible, and I think we're almost there.

Second, are you sure the VOID_BILLING permission affects adjustment? I don't think I am seeing the same thing. A user needs to have "ADJUST_BILLING" rights to do an adjustment, and since nobody has that yet out-of-the-box, effectively only superusers can adjust in current master. Putting the perm in place is a very easy fix which can certainly happen by .0, though we'll also need an upgrade note to highlight that this permission may need to be applied to groups locally, dependent upon local policies/needs.

Dan

[Kathy Lussier (klussier)]

Reporting back that the "ADJUST_BILLING" permission is what gives the user the right to adjust to zero. However, it looks like it didn't make it into the seed data. Sorry for the confusion.

Also adding an official +1 to the idea raised in IRC to have the void options display dependent on the state of the prohibit negative balance settings:

http://irc.evergreen-ils.org/evergreen/2015-09-11#i_202912

[Jason Stephenson (jstephenson)]

Changing importance to critical as this is a release blocker for 2.9.0.

[Dan Wells (dbw2)]

Here is the first and most basic step needed to make the current code fully functional. It simply adds the missing permission to the seed data. I'm continuing to work on UI bits to make the interface side nicer, but in my opinion we shouldn't wait on that to get this part in.

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

working/user/dbwells/lp1494544_add_missing_adjust_perm

[Jason Stephenson (jstephenson)]

I disagree with Dan in comment #8. I think the interface should be complete before 2.9.0 is released.

That said, I am willing to delay the release, so no big rush.

[Kathy Lussier (klussier)]

I've signed off on the code to add the permission.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp1494544_add_missing_adjust_perm

I'm also in agreement with Jason in comment 9.

[Dan Wells (dbw2)]

After much deliberation and experimentation, here is a slightly tweaked version of the discussed changes. From the commit:

   1) Adds "Adjust to Zero" to the "Actions" menu. It was previously only
    on the right-click popup.
    2) Shows or hides the void/adjustment controls based on permissions.
    3) Generates a new force-verified warning if you attempt to void, but
    there is also some chance that doing so would be contrary to your
    negative balance policy settings.

In working through the plan, I ran into two issues. First and most important, I think there are negative security implication for hiding UI elements if the user can still run the underlying API calls. Basically, an administrator is not likely to realize access is not blocked once the UI is hidden, and is therefore less likely to adjust the necessary permissions. Second, there are many supported configurations where negative balances are partially allowed (via interval, or via lost vs. overdue distinctions). We would essentially need to run the entire void/adjust logic stack just to display the interface, and doing so does not seem like a viable option.

Instead, I have opted to do some basic settings checking, then display a new warning (with checkbox confirmation) if it is *possible* that voiding a given bill (or billing) would violate local negative balance policies. I think this serves the purpose while keeping the code and performance reasonable.

In summary:

-User permissions will now show/hide the interface commands.
-If the user has VOID_BILLING permission and tries to void, library settings may now trigger a situationally dependent warning box.

Branch is here (rebased, 2 commits now):
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbwells/lp1494544_better_void_adjust_perms_and_ui

working/user/dbwells/lp1494544_better_void_adjust_perms_and_ui

Thanks!

[Kathy Lussier (klussier)]

Thanks Dan! I tested the branch and find that it works as you described in your comments. I have a couple of follow-up comments:

1.I think the warning message could be clearer for staff to describe the exact problem that might occur if they proceed with the void. mmorgan suggested in IRC that the warning be "Voiding these bills may produce a negative balance. Are you sure you wish to continue?" I prefer this wording.

2. If an account doesn't have the void permission, the void option is removed from the Full Details view of a bill, which is a good thing. However, there still isn't a corresponding "adjust" option there that will allow them to remove a portion of the overdue fine rather than the entire fine. Staff will miss having the ability to adjust just a portion of the fine.

[Jason Stephenson (jstephenson)]

After discussion in IRC, http://irc.evergreen-ils.org/evergreen/2015-09-16#i_203606, and some private messages on the side, I've decided this can go in.

I'll push it to master for a 2.9.0 release later today.

Thanks Dan, Kathy, and everyone who participated in the discussion on IRC.