Circulation auto renewal remaining and renewal remaining should not be nullable

Adding a link to relevant IRC discussion:

http://irc.evergreen-ils.org/evergreen/2019-07-09#i_411573

Adding the NULL check to the filter is most efficient way to handle this. Maybe this is a documentation / sample data issue?

A secondary check in the code to mark such events invalid probably wouldn't hurt, though.

I'd like to suggest a slight tweak for the second case (above) where auto_renewal_remaining = 0. We should teach the AutoRenew reactor to bypass the actual circulation back-end call, but still generate the notification.

As I understand it, autorenew batches can take some time to process. This would allow the server to avoid a lot of unnecessary work processing items that are known in advance to be non-auto-renewable.

Perhaps we should have a dedicated hook for autorenewals which includes the NULL check?

Regarding the second case, I like the idea of teaching the reactor to bypass the autorenew attempt and just send a notification when auto_renewal_remaining = 0

My thoughts as of right now:

1. Modify the validator to look for auto_renewal_remaining is not null.

2. Modify the AutoRenew reactor to check for auto_renewal_remaining = 0, bypass the checkout, and generate the MAX_RENEWALS_REACHED event on its own.

An alternative to #1 in my previous comment occurred to me. We could also check for auto_renewal_remaining of NULL in the reactor and set the reason to something like autorenewal not allowed. We could even make a new event for this.

Doing this will notify the patron that the item can't be autorenewed. This might be preferable to just setting the event state to invalid and then not notifying the patron.

What's the consensus?

I am necro-bumping this bug because I stand by my previous comment even more today. We use the autorenew events as both autorenewal and courtesy notifications for those things that can't be renewed.

I think we should either make the renewal_remaining and auto_renewal_remaining columns "NOT NULL DEFAULT 0" or add code to the AutoRenew.pm handler function to set the variables to 0 when the values from the database are null.

Nothing like having a 50% failure in the autorenew process to get one to pay attention to what's going on.

I have assigned this bug to myself and changed the summary/title to reflect what I think the real problem is. I consider this an architecture implementation bug. In my opinion and the opinion of many others, numeric fields should not be nullable, unless null conveys some useful information that zero does not also convey. In the case of the renewal remaining fields in circulation, null is equivalent to zero.

I'll have a patch commit to fix the circulation table and cir rule tables (if needed) later this week.