| Time |
Nick |
Message |
| 07:37 |
|
collum joined #evergreen |
| 08:43 |
|
mmorgan joined #evergreen |
| 10:27 |
|
Llewellyn joined #evergreen |
| 10:46 |
Llewellyn |
I'm confused by something I'm seeing this morning. We had a patron complain about an autorenewal failure notice on an item where the duration rule does not permit autorenewals or renewals. I was looking into our definition and found that it's using CircIsOpen as its validator. This is the validator assigned to the autorenewal action by default. |
| 10:46 |
Llewellyn |
However, there is another validator called CircIsAutoRenewable that is not being used anywhere. It calls the same subroutine as CircIsOpen with an additional check to see whether the patron is barred. This validator also does not look at the duration rule to confirm whether autorenewals are allowed. Also, the CircIsOpen validator contains |
| 10:46 |
Llewellyn |
a logger statement implying that the intended use is for autorenewals, even though this validator is used in courtesy notices. |
| 10:46 |
Llewellyn |
Does anyone know why it's like this? |
| 10:46 |
Llewellyn |
I would think that checking the duration rule would help reduce the overhead that comes with enabling autorenewals. |
| 10:50 |
|
Dyrcona joined #evergreen |
| 10:51 |
Bmagic |
Llewellyn: you might be looking for the logic from OpenILS::Application::Trigger::Reactor::Circ::AutoRenew |
| 10:52 |
mmorgan |
Llewellyn: Does bug 1835353 fit what you're describing? |
| 10:52 |
pinesol |
Launchpad bug 1835353 in Checkbox Provider - Base "Need a job for NVLink" [High,Fix released] https://launchpad.net/bugs/1835353 - Assigned to Ray Chen (ray.chen) |
| 10:52 |
mmorgan |
Nope, not that one. |
| 10:52 |
mmorgan |
bug 1835953 |
| 10:52 |
pinesol |
Launchpad bug 1835953 in Evergreen 3.13 "Autorenewals should not be attempted on circs where auto_renewal_remaining is NULL" [Medium,Confirmed] https://launchpad.net/bugs/1835953 |
| 10:55 |
Llewellyn |
mmorgan yes that's the one |
| 11:09 |
mmorgan |
Llewellyn: We have been using the custom filter approach to prevent the events from being created in the first place. |
| 11:09 |
|
Christineb joined #evergreen |
| 11:09 |
Llewellyn |
decent strategy, but I do think there should be something concrete in place. |
| 11:14 |
mmorgan |
Agree. Would a unique hook be a better approach? Prevent the events from being created vs. marking them invalid during processing? |
| 11:15 |
Llewellyn |
I've never messed with hooks so I'm not sure how complex that would be. My idea was for the validator to look up the duration rule attached to the circ and mark it invalid if autorenewal is null or 0. |
| 11:24 |
mmorgan |
Llewellyn: That makes sense, and validators do look to be commonly used for situations like this. I always prefer to avoid creating events that just end up invalid, but it sounds like your approach would get the job done, and more reliably than relying on a NULL being carefully maintained in the duration rule. |
| 11:24 |
Bmagic |
The proposed patch is the asnwer right? |
| 11:25 |
Llewellyn |
tbh I do not know what is happening in that LP bug |
| 11:26 |
Bmagic |
Dyrcona's patch changes the tables in the database such that none of the columns having to do with max_autorenewals can be null |
| 11:26 |
Bmagic |
and/or related autorenewals_remaining columns |
| 11:27 |
Bmagic |
The XXXX upgrade script is a good summary |
| 11:28 |
Bmagic |
barring that, mmorgan's fix is reasonable as a stop-gap, until that code gets merged into main. Feeding the cron job a custom json file that tweaks the query such that circulations that have a null value are omitted from processing |
| 11:29 |
Llewellyn |
How is that going to stop notices? |
| 11:30 |
mmorgan |
Bmagic: Dyrcona's comment #15 states that the patch would prevent the abilty to have NULLs and use the NULL filter approach to prevent a notice, so it isn't a solution to the initial bug report. |
| 11:32 |
Llewellyn |
so it breaks your workaround? |
| 11:33 |
mmorgan |
Llewellyn: Yes, exactly. I'm not actually sure what issue with NULLs Dyrcona was experiencing that his patch addresses. |
| 11:35 |
Bmagic |
It seems like the patch should include a check on auto_renewal_remaining > 0 before it starts to process the circ |
| 11:35 |
Llewellyn |
but then it wouldn't generate autorenewal failure notices when we actually need them |
| 11:36 |
Bmagic |
once the database ensures that the column can never be null, then the perl can rely on an integer to apper in the circ |
| 11:37 |
Bmagic |
I see, so we want* it to be null to signal that the circulation isn't included. And when it's non-null, we can tick down the number from 2->1->0 and at zero the patron would be alerted to the failure |
| 11:38 |
Bmagic |
Then, yeah, I don't understand the solution |
| 11:39 |
Bmagic |
there is a way that a circ can be connected to the rule that produced it, and that rule could be checked for autorenewal? If the rule doesn't turn on the autorenewal feature, then we can ignore the circulation? |
| 11:41 |
Llewellyn |
circ has a "duration rule" but I don't think its actually linked to anything |
| 11:41 |
Llewellyn |
would have to look it up by name I guess |
| 11:42 |
mmorgan |
Oh, right. It's just a text field :-( |
| 11:44 |
Bmagic |
I don't think we want the circulation to be hard linked to the circ rule. Probably because circ rules evolve? Or we want to be able to delete rules without getting tangled up on the foreign keys? |
| 11:47 |
mmorgan |
That makes sense. |
| 11:53 |
Llewellyn |
in that case maybe circs should have a boolean "can autorenew?" type column |
| 12:00 |
Bmagic |
The null possibility serves dual purpose: if it's null then it's not a circulation that can autorenew, if it's non-null then it is an autorenewal circ and should be included. However, it seems like there's a side-affect of that column being allowed to be null. Not related to this fuctionality? |
| 12:00 |
Bmagic |
and due to that wrinkle, it seems that we need another* column on action.circulation, a boolean flag. Yes, I agree, but this should all make it's way onto the bug discussion |
| 12:01 |
mmorgan |
Or, to avoid schema changes, maybe look back to see if the original circ in the chain has auto_renewal_remaining = 0? |
| 12:01 |
Bmagic |
I bet that would be slow |
| 12:03 |
Dyrcona |
auto_renewal_remaining can be null, something that I consider to be a bug. |
| 12:04 |
* Dyrcona |
reads scrollback. I'm busy doing things that I didn't expect to do today and almost forgot to even log in to IRC. |
| 12:08 |
Dyrcona |
NULLs are overused in Evergreen in my opinion, particularly on numeric fields. The renewal remaining column does not allow null and neither should auto_renewal_remaining. The bug was created I think because having null in auto_renewal_remaining null causes log noise when the default processor is used for autorenew events. It looks like a serious error, but it isn't that serious. The initial bug wants to eliminate log noise. |
| 12:09 |
Dyrcona |
There are many opinions and debates about using null in databases, particularly on integer fields. I'm inclined to agree with the side that says integers should not be nullable. |
| 12:11 |
mmorgan |
Dyrcona: The initial intent of the bug report was to ensure a way to prevent autorenewal attempts on circs that are not renewable in the first place. |
| 12:11 |
Dyrcona |
I'm actually kind of done with that bug, so whatever anyone else wants to do I don't care any more. |
| 12:11 |
Dyrcona |
mmorgan: Fine. I don't care any more. My head if full of other junk at the moment. |
| 12:12 |
Dyrcona |
In fact, I don't have time for IRC. Peace out, y'all. |
| 12:14 |
* mmorgan |
will add a link to this discussion to the bug, and edit the description to try and better convey the issue. |
| 12:14 |
* mmorgan |
would also like the discussion to continue! |
| 12:14 |
|
jihpringle joined #evergreen |
| 12:22 |
* Bmagic |
would like to continue the discussion as well |
| 13:27 |
|
Llewellyn joined #evergreen |
| 13:40 |
csharp_ |
one could also theoretically create an action_trigger_filter.json entry that filters out circs with autorenew =0 or null |
| 13:41 |
csharp_ |
too many moving parts in A/T :-/ |
| 14:07 |
|
jihpringle joined #evergreen |
| 14:20 |
mmorgan |
csharp_: Unfortunately, that doesn't distinguish between the circs that have run out of autorenewals vs. the circs that were never autorenewable at all. |
| 16:31 |
csharp_ |
mmorgan: hmm - we don't use autorenew, so I'm unfamiliar with how to distinguish one from the other |
| 16:32 |
csharp_ |
I want us to use autorenew, but our directors flat out rejected the idea when the feature became available :-/ |
| 16:32 |
csharp_ |
I think with the push for fines-free libraries it might be more acceptable to them though |
| 16:50 |
|
jihpringle joined #evergreen |
| 17:19 |
|
mmorgan left #evergreen |
| 18:14 |
|
jihpringle joined #evergreen |
| 19:59 |
|
JBoyer joined #evergreen |
