Comment 9 for bug 793550

Hello Jason,

I think this code does the job, thanks! I would like to make one more suggestion which affects the usability of the helper function. The comments say it returns 0 or 1, and that's definitely what it should do, but it looks like the function will currently return 0, 1, 't', or 'f'. I think we're better off if we move the $U->is_true() inside the helper function, ideally inside the logic branch where it's needed. What do you think?

Also, and this is just a stylistic preference, so please take it or leave it, but I'd probably skip the ternary operator and $close variable altogether (since we don't use it again) and just do the test directly, something like (untested!):

if (!$circ or OpenILS::Application::Circ::CircCommon->can_close_circ($e, $circ )) {
        $trans = $e->retrieve_money_billable_transaction($transid);
...(go on and close it up)...

Again, I think the first part is important, but the second part is just a fresh look impression. I have no idea if Perl::Critic would like one or the other :)

I am also happy to toss up a branch if you've moved on from this.

Thanks again,
Dan