Bug 19014 - Patrons should not get an 'on_reserve' notification if the due date is far into the future
Summary: Patrons should not get an 'on_reserve' notification if the due date is far in...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
: 20360 (view as bug list)
Depends on: 15705
Blocks: 25758
  Show dependency treegraph
 
Reported: 2017-08-01 11:27 UTC by Ray Delahunty
Modified: 2020-11-13 13:03 UTC (History)
25 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
These patches alter the behaviour of 'CanBookBerenewed' to return a status of 'too_soon' before any other statuses if the book is not eligible for renewal yet. This is done to prevent sending messages to patrons about other changes in status when it is still too soon to renew the item
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 19014: Do not sent on_reserve notification if the checkout is due in the future (2.31 KB, patch)
2017-08-07 17:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Alternate patch] Bug 19014: Do not try to autorenew an item too early (2.83 KB, patch)
2019-03-25 03:48 UTC, David Cook
Details | Diff | Splinter Review
Bug 19014: [alternate-alternate] Only process issues for autorenewal if on or after soonest renewal date (2.42 KB, patch)
2019-06-11 16:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: Only process issues for autorenewal if on or after soonest renewal date (2.39 KB, patch)
2019-11-12 19:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: Only process issues for autorenewal if on or after soonest renewal date (2.43 KB, patch)
2019-11-27 16:08 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 19014: Only process issues for autorenewal if on or after soonest renewal date (2.48 KB, patch)
2019-11-27 16:15 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 19014: Unit tests (1.80 KB, patch)
2019-12-30 19:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: Return auto_too_soon before on_reserve (10.80 KB, patch)
2019-12-30 19:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: on_reserve blocks auto_renew (3.03 KB, patch)
2020-01-15 02:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: Unit tests (1.87 KB, patch)
2020-01-15 14:50 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 19014: Return auto_too_soon before on_reserve (10.86 KB, patch)
2020-01-15 14:50 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 19014: on_reserve blocks auto_renew (3.10 KB, patch)
2020-01-15 14:50 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 19014: Unit tests (1.89 KB, patch)
2020-02-11 13:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: Return auto_too_soon before on_reserve (10.17 KB, patch)
2020-02-11 13:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: on_reserve blocks auto_renew (3.09 KB, patch)
2020-02-11 13:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19014: Unit tests (1.96 KB, patch)
2020-02-11 16:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19014: Return auto_too_soon before on_reserve (10.29 KB, patch)
2020-02-11 16:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19014: on_reserve blocks auto_renew (3.22 KB, patch)
2020-02-11 16:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19014: (QA follow-up) Update test for AddReserve change (1.69 KB, patch)
2020-02-11 16:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 19014: [19.11.x] Prevent auto_renew notices form sending if too early to renew (12.52 KB, patch)
2020-03-05 12:03 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ray Delahunty 2017-08-01 11:27:35 UTC
Current functionality in 17.05:
All copies of an item with auto-renewal enabled, reserved at the title level.
Due dates of copies on loan include today, and various dates into the future, some many weeks ahead.
When the auto-renewal job runs,  Koha is sending an AUTO_RENEWAL email to ALL the users warning them ALL of renewal failure, using the [% ELSIF checkout.auto_renew_error == 'on_reserve' %]

Desired functionality in 17.05:
When the auto-renewal job runs,  Koha should only send an AUTO_RENEWAL email to the user (or possibly users) whose copy is due back today, on the day the auto-renewal job runs. Other people who have copies on loan but which are not yet due back yet should not be contacted by the AUTO_RENEWAL email. Users contacted in this way are being very puzzled and confused.

Could this be achieved by an edit to the condition in the template toolkit? So rather this:
[% ELSIF checkout.auto_renew_error == 'on_reserve' %]
This item has been reserved by another reader.
… we have something like this:
[% ELSIF checkout.auto_renew_error == 'on_reserve'  && checkout.date_due %==today %]
This item has been reserved by another reader.

Would that work? Is a second condition permissible? How should it be worded?

Ray Delahunty
University of the Arts London
Comment 1 Jonathan Druart 2017-08-07 17:14:57 UTC
Created attachment 65590 [details] [review]
Bug 19014: Do not sent on_reserve notification if the checkout is due in the future

If holds exist on an item that have been checked out and marked as auto
renewal, we do not want to notify patrons with a on_reserve notification
if the checkout is due in the future.
These on_reserve notifications must only be sent if the checkout is due
today or in the past.

Test plan:
- Check an item in and mark it as auto renewal, use a due date in the
past to ease the testing
- Place a hold in this item
- Execute the cronjob
=> Without this patch, the patron will get notified even if the checkout
is due in the future
=> With this patch applied the patron will only get notified the day the
checkout is due or if the due date is passed
Comment 2 Jonathan Druart 2017-08-07 17:19:59 UTC
Hi Ray,
What you describe sounds like a bug. Could you take a look at this patch and tell me if it could meet your needs?
Comment 3 Ray Delahunty 2017-08-08 07:16:28 UTC
Yes, I think that is a perfect plan. My suggestion about the edit to the template toolkit was just a suggestion. We have a workaround in place in 16.05 (which has the same problem) and will have to retain it when we upgrade to 17.05 in a week or so. We have repurposed ODUE to be the renewal failed advisory, and use RENEWAL as the renewal success advisory, and have turned off the AUTO_RENEWAL notice. That works OK, but we do hope to see the functionality working as you describe so we can restart using AUTO_RENEWAL. We are also funding the change of that email into a digest one, to further improve the auto-renewal functionality.

Ray Delahunty
University of the Arts London
Comment 4 Katrin Fischer 2017-08-15 20:48:32 UTC
It seems a bit harsh to send the notice the day the item is due, as this means the patron will only have 'that day' to return the book safely. 

But as the automatic renewal will be tried daily depending on 'No renewal before' and (I tihnk) 'No automatic renewal after' I am not sure how to avoid spamming the patrons with lots of emails. I guess ideally one email would be sent the first time the renewal fails?
Comment 5 Caroline Cyr La Rose 2017-11-03 19:09:06 UTC
Hi Jonathan,

I'm having trouble testing this. Which cron job are we supposed to run? Is it automatic_renewals.pl --send-notices?

Furthermore, I had an item due in the future (in december) and I still received a notice saying the auto renewal failed because I had fines (which was true). But why would it try to renew something due in december?
Comment 6 Jonathan Druart 2017-11-29 16:13:12 UTC
(In reply to Caroline Cyr La Rose from comment #5)
> Hi Jonathan,
> 
> I'm having trouble testing this. Which cron job are we supposed to run? Is
> it automatic_renewals.pl --send-notices?

Yes

> Furthermore, I had an item due in the future (in december) and I still
> received a notice saying the auto renewal failed because I had fines (which
> was true). But why would it try to renew something due in december?

You need to tune that using the 'No automatic renewal before/after' settings.
Comment 7 Nick Clemens 2018-03-29 12:17:21 UTC
We have several libraries with different opinions on this:
Some want the emails to be sent the day the item becomes non-renewable
Some want the notices sent on the day the item would be renewed

This patch seems to send the notice on the due date of the item

Could this be done as a switch in the cronjob? (or a syspref?)
Comment 8 Jonathan Druart 2018-03-29 15:01:07 UTC
(In reply to Nick Clemens from comment #7)
> We have several libraries with different opinions on this:
> Some want the emails to be sent the day the item becomes non-renewable
> Some want the notices sent on the day the item would be renewed
> 
> This patch seems to send the notice on the due date of the item
> 
> Could this be done as a switch in the cronjob? (or a syspref?)

I think what you describe can be considered as a new enh, whereas this patch fixes a much bigger issue.
Comment 9 Katrin Fischer 2018-04-01 15:58:46 UTC
I am not sure all libraries can generally agree to send the notice on date due, because then the user has no way to return the item on time usually. 

I think there are separate issues thinking about the notice:

- Don't send a notice every day the item is not returned, sent a notice once or at least not too often
- Be able to inform the patrons on time before the due date so they are able to return the item on time
- Don't inform the user weeks ahead 

Could it help to set the "No renewal before" to a lower number of days? This would prevent auto renewals from happening and together with "No automatic renewal after" could limit the timeframe for sending the notice to a few days.
Comment 10 Ray Delahunty 2018-05-16 08:39:15 UTC
We have a test system at 17.11.04 and are investigating whether this problem / dilemma can be solved with the code, which I understand will solve the problem in this bug and the one in 20360 (and which is a candidate for 18.05) can possibly be added to our 17.11 version. 
We have a workaround where we disabled AUTO_RENEWALS notices and use ODUE to notify of auto-renewal failure, and RENEWALS to notify of auto-renewal success. We are today undoing this workaround and if the code can work in 17.11 we plan to address Katrin’s concerns expressed above by a mix of changes to ‘No renewal after’, ‘No renewal before’, choice of time for the auto-renewal job to run, and generous use of a grace period. These Circulation & Fine Rules changes do not solve every problem, but if they work, they will offer some improvement. If the code does not work, I think we may have to reinstate the workaround, and wait for Koha 18.05.

Test plan.
We set ‘No automatic renewal before’ to 2 days. We set ‘No automatic renewal after’ to 1 day. We set the time of the auto-renewal job to 7am. We set the RenewalBasePeriod to “old due date”. We set the Grace Period to 3 days. Assuming the new code solves both this bug and the one in 20360, this is what we would expect: 

Situation 1. 
Item is reserved / on hold and due date on Thursday. Auto-renewal job runs at 7am Wednesday, and AUTO_RENEWAL notice is sent advising of renewal failure. User has two remaining loan days and the three grace period days before fines start. If the reservation / hold is cancelled after the job has run or is fulfilled by another copy after 7am on Wednesday but before 7am Thursday, the second run of the auto-renewal job will result in the item being renewed, with the new date based on the old date (so the early renewal does not affect the loan period).

Situation 2.
Item is not reserved / on hold or otherwise blocked from renewal (say, due to max renewals count being reached) and it is due on Thursday. Auto-renewal job runs at 7am Wednesday, and AUTO_RENEWAL notice is sent advising of renewal success, with the new due date based on the old date. The early renewal does not badly affect the current holder- other than appearing to be an ‘early renewal’. But it does mean that if someone else places a reservation / hold on the title on the Wednesday or Thursday they will have a long wait than if the renewal were close to the original due date. 

We can play about with the settings in Circulation & Fine Rules to get the “best possible outcome” of minimal affect for maximum users (maybe set the ‘No Auto renewal before’ back to 3 days?) but whatever we do, it won’t be perfect as there are potentially problems with whatever settings we choose.
Comment 11 Jonathan Druart 2018-05-17 16:38:56 UTC
I will not have time to get back to this before the release.
Comment 12 Michal Denar 2018-09-19 09:57:12 UTC
Hi,
any progress with this issue?

Thank you.


Mike
Comment 13 Jonathan Druart 2018-09-19 20:31:30 UTC
(In reply to Michal Denar from comment #12)
> any progress with this issue?

Could you test the patch and give us your feedback?
Comment 14 David Cook 2018-11-21 00:26:36 UTC
I've run into this same problem, but my thinking was that C4::Circulation::CanBookBeRenewed() should check for "too_soon" before "on_reserve".

In fact, shouldn't reserves be the last thing checked when determining whether or not a book can be renewed?

Here's the scenario I see:

1. Patron A autorenews Book A on Wednesday morning
2. Patron B reserves Book A on Wednesday afternoon
3. Patron A receives autorenewal failure for Book A on Thursday morning

Here's some SQL I've used to find instances of that scenario happening (the search strings will depend on the content of your notices):

select
m.borrowernumber,
m.message_id, mq.message_id, m.time_queued, mq.time_queued,m.content, mq.content
from message_queue m
join message_queue mq ON
m.borrowernumber = mq.borrowernumber
AND substring(m.content,locate('The following item',m.content),30) = substring(mq.content,locate('The following item',mq.content),30)
AND date(date_add(m.time_queued, INTERVAL 1 DAY)) = date(mq.time_queued)
where m.letter_code = 'AUTO_RENEWALS'
and mq.content like '%item is on hold%'
ORDER BY m.borrowernumber
Comment 15 David Cook 2018-12-11 04:03:46 UTC
I think the proper solution might be to refactor C4::Circulation::CanBookBeRenewed and then maybe make a separate function C4::Circulation::CanBookBeAutoRenewed and make that configurable, since it seems that different libraries have different needs/wants for autorenewal.
Comment 16 David Cook 2018-12-11 04:04:04 UTC
(In reply to David Cook from comment #15)
> I think the proper solution might be to refactor
> C4::Circulation::CanBookBeRenewed and then maybe make a separate function
> C4::Circulation::CanBookBeAutoRenewed and make that configurable, since it
> seems that different libraries have different needs/wants for autorenewal.

That said, I don't have the resources to do this at present.
Comment 17 Holger Meißner 2018-12-11 14:13:28 UTC
(In reply to David Cook from comment #15)
> I think the proper solution might be to refactor
> C4::Circulation::CanBookBeRenewed and then maybe make a separate function
> C4::Circulation::CanBookBeAutoRenewed and make that configurable, since it
> seems that different libraries have different needs/wants for autorenewal.

That's also the first idea I had. Yet, there would be duplicate code and we shouldn't do that unless we have to.

Second idea: Don't prioritize errors in CanBookBeRenewed.
We already combine different errors in $error, e.g. if both "too_soon" and "auto_renew" apply, then it's "auto_too_soon". Take this one step further and return a complete list of basic errors instead. That list could be evaluated as needed, e.g. the cronjob could decide not to send a notice, because it's too soon and at the same time opac could decide to display a hold.
I don't know whether the loss in performance would be noticable.

Third idea: Tell CanBookBeRenewed the context we're in.
Give CanBookBeRenewed an additional parameter, so it can return different errors depending on that parameter, e.g. CanBookBeRenewed(123,123,0,X) where X might be something like "auto", "staff" or "opac".
Similar to the first idea, but the code stays in one function.

Any other ideas?

Maybe I'll find time to code in 2019, but I can't promise yet.
Comment 18 Daniel Gaghan 2018-12-27 19:52:40 UTC
Having read over the comments here, there's nothing I can add besides saying that Pueblo City County Library District is hoping for a fix for this in 2019.
Comment 19 David Cook 2019-01-25 04:26:10 UTC
In 2019, I'm still having librarians getting in touch over irate patrons getting misleading notifications.

And now it's not just holds. It's also notifications about reaching the limit of renewals, even though they won't need to autorenew for another 3 weeks. 

It seems like most people aren't affected by the problems with how autornewals works because they don't have notices set up, but the people who do get notices are plagued by notices that don't make sense.

I think it's clear that we need to do "something".

But how do we reconcile the people who like the current functionality and the people who hate the current functionality?

Looking at Jonathan's patch, it doesn't go far enough. 

Maybe the easiest thing to do is refactor CanBookBeRenewed to have the "auto_too_soon" code put into a function... and then add that function wrapped in a system preference in automatic_renewals.pl, so that people can toggle between the preferred behaviours.

I don't like it... I think everyone should agree with me about what I think is logical (hahahaha), but that might be the most feasible path forward.
Comment 20 David Cook 2019-02-11 06:45:39 UTC
Actually, it looks like there's already a function in C4::Circulation that could handle this scenario: GetSoonestRenewDate.

Personally, I have a non-Koha loan that was due back on February 15th, but today on February 11th I got an email saying that my loan had been automatically renewed until sometime in March. 

That's a reasonable way of expecting automatic renewals to work I think. That it wouldn't try the automatic renewal until the soonest renew date.

I'm going to do a local patch to try this idea out.
Comment 21 Ray Delahunty 2019-03-19 14:41:26 UTC
Almost 3 years after going live with auto-renewals, including our workaround to deal with this bug we have this very day found that the RenewalPeriodBase was set wrongly (left set the "current date", which was sensible for a manual renewal environment, but is I think NOT sensible for an auto-renewal one). This variable has had an impact on renewal timings and renewal outcomes. Our calendar has Sa/Su as closed days, items being 7 day loans, the No Renewal Before set to 1, and  auto-renewal running at 10pm Mo-Su. These variables coupled with the wrong RenewalBasePeriod has seen the loan period on renewal drop a day, until the Monday is reached and then the Sunday closure is seen and items renew every Monday for 7 days from then on. So many variables interact with auto-renewal functionality it is challenging to get then all "right". We are about to look at 18.11 to see how many bug fixes have made it there, up from our 17.05 version. But if unexpected timing puzzles are happening, it might be worthwhile checking the RenewalBasePeriod setting to see if it is set correctly.
Comment 22 Liz Rea 2019-03-19 15:21:15 UTC
I'd like to test what's here, but don't want to waste the time if it's just going to be superceded. If no one is going to submit a counterpatch in the very near term, this looks like it will be an improvement and the patch probably needs attention so as not to waste Jonathan's work.

For the record, it still applies and I plan to test it in the very near term, unless someone says "no here is my go at fixing this."

Liz
Comment 23 David Cook 2019-03-25 02:33:54 UTC
(In reply to Liz Rea from comment #22)
> I'd like to test what's here, but don't want to waste the time if it's just
> going to be superceded. If no one is going to submit a counterpatch in the
> very near term, this looks like it will be an improvement and the patch
> probably needs attention so as not to waste Jonathan's work.
> 
> For the record, it still applies and I plan to test it in the very near
> term, unless someone says "no here is my go at fixing this."
> 
> Liz

Thanks, Liz. I do have a counterpatch which I've been using locally for one client... I'll post it here. 

It's actually not too dissimilar from Jonathan's patch.
Comment 24 David Cook 2019-03-25 03:48:49 UTC
Created attachment 86960 [details] [review]
[Alternate patch] Bug 19014: Do not try to autorenew an item too early

There are a number of conditions which can cause autorenewals
to send out confusing notifications to patrons far before the
due date of an item. These may be exceeding the max number of
renewals or checkouts, a hold on the item, etc.

If an item has a hold, we don't want to tell the patron they
can't autorenew the item until the earliest renewal date or the
due date of the item.

Test plan:

1) Check out an item with a due date 2 business days in the future
2) Place a hold on this item
3) Execute the cronjob
- Without the patch, an email notification is generated for the patron
- With the patch, no email notification is generated
4) Set norenewalbefore in the Circulation and Fines rules to 2
5) Execute the cronjob
- Without the patch, an email notification is generated for the person
- With the patch, an email notification is generated for the person
6) Set norenewalbefore in the Circulation and Fines rules to 3
7) Execute the cronjob
- Without the patch, an email notification is generated for the patron
- With the patch, no email notification is generated
8) Clear norenewalbefore in the Circulation and Fines rules
9) Change item due date (in database) to a date/time prior to now
10) Execute the cronjob
- Without the patch, an email notification is generated for the person
- With the patch, an email notification is generated for the person
Comment 25 David Cook 2019-03-25 03:49:38 UTC
(In reply to David Cook from comment #24)
> Created attachment 86960 [details] [review] [review]
> [Alternate patch] Bug 19014: Do not try to autorenew an item too early
> 

I think this emulates the behaviour of other current LMSes/LSPs that use automatic renewal, and is what patrons would expect.
Comment 26 Andrew Fuerste-Henry 2019-03-25 14:09:31 UTC
Apologies for pushing for greater complexity on this, but I'm not sure about this approach. If we make auto-renew notices only send on the day the item is renewed or on the day it's due, we've not solved the problem so much as inverted it -- some libraries will like it, some libraries will be ambivalent, and some libraries will feel strongly that we're sending notices too late.

I'd like to see this either as a switch (Send notice on status change / Send notice at due date & renewal) or as a configurable preference like a No Notice Before value.
Comment 27 Joy Nelson 2019-03-25 15:59:05 UTC
It seems logical that the autorenewal process should only be looking at issues at the time specified for autorenewal.  i.e. Only try to renew items on the 'No renewals before" duedate value.
Comment 28 David Cook 2019-03-26 00:49:23 UTC
(In reply to Andrew from comment #26)
> Apologies for pushing for greater complexity on this, but I'm not sure about
> this approach. If we make auto-renew notices only send on the day the item
> is renewed or on the day it's due, we've not solved the problem so much as
> inverted it -- some libraries will like it, some libraries will be
> ambivalent, and some libraries will feel strongly that we're sending notices
> too late.
> 
> I'd like to see this either as a switch (Send notice on status change / Send
> notice at due date & renewal) or as a configurable preference like a No
> Notice Before value.

Andrew, I don't understand what you're trying to say here. The auto-renew notices will be queued up when the renewal is processed. I don't see any other logical way of handling the notices. Are you meaning you want to change the timing of the renewals (and thus the notices associated with them)? 

If so, my patch allows you to use the "No Renewal Before" rule in Circulation and Fines Rules to specify that no renewal can happen until N days before the due date. 

My local public library (which doesn't use Koha) seems to have set "4 days before the due date", which means that if there are no restrictions on the item, it will autorenew 4 days before the due date and it will promptly send me an email notification to indicate this.

I can't really see any other logical way of it working?
Comment 29 David Cook 2019-03-26 00:50:20 UTC
(In reply to Joy Nelson from comment #27)
> It seems logical that the autorenewal process should only be looking at
> issues at the time specified for autorenewal.  i.e. Only try to renew items
> on the 'No renewals before" duedate value.

I agree. I think it's both logical and it seems to be the way that other library systems do it as well, which suggests to me that is what patrons would expect as well.
Comment 30 Andrew Fuerste-Henry 2019-03-26 13:15:38 UTC
Sorry, David, my response was based on some misunderstanding of your patch. I thought you were tying the notice solely to the due date. If it's sending renewal failure notices at the renewal attempt as determined by the No Renewal Before value, then that seems reasonable. Libraries that want to give their patrons some padding between renewal attempt and due date can set No Renewal Before to a larger number. Thanks for clarifying!
Comment 31 Björn Nylén 2019-03-27 07:49:51 UTC
From our perspective Davids patch is the logical way it should function. Don't do anything until it's time to renew. 

However it does change the behaviour and I guess some libraries might like to keep sending early notices if no further renewal will be possible. Perhaps it's possible to control it with a command line option (eg '--early-notice') to skip the no-renwal-before check?(with the risk of asking for a more complex solution)
Comment 32 Ray Delahunty 2019-03-27 12:48:15 UTC
(In reply to Andrew from comment #26)

Are we focussing too much on manipulating when the auto-renewal takes place as being the driver of limiting how many people receive renewal-failure emails? I think we need a better driver. Auto-renewing an item earlier than the moment it is due back penalises other users who may want the item. If it has already auto-renewed (early) and they place a reservation / hold on it, they will have to wait longer for it. That is why we have opted for the (admittedly harsh) situation where we only warn users that items have failed auto-renewal when they are overdue. (We use a No Renewal Before value set to zero, but we do use a long grace period.)

We have multiple copies of the same item (with various loan periods) out on loan. If a reservation is placed we want to send renewal-failure notification only to users whose copy is due “soon”, and not to people who have, say, just taken a long-term loan copy out. Native Koha auto-renewal functionality currently sees every holder of the item emailed.  But the first copy returned satisfies the reservation. Why pester everyone- we want to better target the email.

In comment 26 Andrew mentions what I think could be the ideal solution- a No Notice Before parameter. Rather than this being a system preference I see it better as an additional column in the Circulation & Fine rules. This would be configurable line by line (user category / item type) and allow the Koha library to email as many or as few current holders of the non-renewed items as they wished.  Libraries that wanted to be more lenient that us could increase the No Renewal Before to 1, 2, or 3, say, but it would be the wise choice of a No Notice Before value that would be the best driver in targeting users whose items are likely to fulfil the reservation / hold.
Comment 33 David Cook 2019-04-01 00:04:55 UTC
(In reply to Björn Nylén from comment #31)
> From our perspective Davids patch is the logical way it should function.
> Don't do anything until it's time to renew. 
> 
> However it does change the behaviour and I guess some libraries might like
> to keep sending early notices if no further renewal will be possible.
> Perhaps it's possible to control it with a command line option (eg
> '--early-notice') to skip the no-renwal-before check?(with the risk of
> asking for a more complex solution)

Oh that's a good point. In fact, locally, I have wrapped it in a system preference, so that it only applies to one instance of Koha. 

With Koha, when changing things, we typically make the new behaviour opt-in, so we'd either want to use a system preference or a command line option like "--no-early-notices" or something like that.
Comment 34 David Cook 2019-04-01 00:10:46 UTC
(In reply to Ray Delahunty from comment #32)
> (In reply to Andrew from comment #26)
> 
> Are we focussing too much on manipulating when the auto-renewal takes place
> as being the driver of limiting how many people receive renewal-failure
> emails? I think we need a better driver. Auto-renewing an item earlier than
> the moment it is due back penalises other users who may want the item. 

No it doesn't. Other users can place a hold on the item. 

> If it
> has already auto-renewed (early) and they place a reservation / hold on it,
> they will have to wait longer for it. 

The same thing could be achieved by a person manually renewing it. I don't see how this is relevant?

> That is why we have opted for the
> (admittedly harsh) situation where we only warn users that items have failed
> auto-renewal when they are overdue. (We use a No Renewal Before value set to
> zero, but we do use a long grace period.)
> 

I think this is what most people do with Koha at the moment.

> We have multiple copies of the same item (with various loan periods) out on
> loan. If a reservation is placed we want to send renewal-failure
> notification only to users whose copy is due “soon”, and not to people who
> have, say, just taken a long-term loan copy out. Native Koha auto-renewal
> functionality currently sees every holder of the item emailed.  But the
> first copy returned satisfies the reservation. Why pester everyone- we want
> to better target the email.
> 

Fair enough although I think that's a separate issue to this one. 

> In comment 26 Andrew mentions what I think could be the ideal solution- a No
> Notice Before parameter. Rather than this being a system preference I see it
> better as an additional column in the Circulation & Fine rules. This would
> be configurable line by line (user category / item type) and allow the Koha
> library to email as many or as few current holders of the non-renewed items
> as they wished.  Libraries that wanted to be more lenient that us could
> increase the No Renewal Before to 1, 2, or 3, say, but it would be the wise
> choice of a No Notice Before value that would be the best driver in
> targeting users whose items are likely to fulfil the reservation / hold.

I'm not interested in this idea, but you are free to pursue that idea by yourself or pay someone else to pursue it. 

I've provided my alternate patch as it's how me and my clients think the auto-renewal should work, and we've already implemented it locally and are satisfied with that. I've uploaded it here as I think other people would benefit from it as well. That's the limit to my contribution on this particular bug report though.
Comment 35 Cori Lynn Arnold 2019-04-23 16:30:02 UTC
So, I was asked nicely to wade in on this as a librarian.

I think that:

1. Koha should not notify patrons that a book is eligible to be renewed in the _way_ far future, as the original text says.

2. There should be a configurable time (lets say 3 days for the sake of argument) where patrons are notified that material:
     a. Has been auto-renewed and the new due date is YYYYMMDD
     b. Cannot be auto-renewed and must be returned on date XX-XX-XX
Comment 36 Nick Clemens 2019-06-11 16:35:04 UTC
Created attachment 90509 [details] [review]
Bug 19014: [alternate-alternate] Only process issues for autorenewal if on or after soonest renewal date

This patch check the soonest renewal date and skips processing auto renewals if before this date
This ensures patrons won't get notifications far in advance if an item is on hold or has other issues

To test:
 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' unset
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - perl misc/cronjobs/automatic_renewals.pl --send-notices
 5 - Patron is not notified
 6 - Set 'No renewal before' to 1
 7 - run cronjob
 8 - Patron is notified with error 'on_reserve'
 9 - Set 'No renewal before' to 3
10 - UPDATE issues SET auto_renew_error = NULL;
11 - run cronjob
12 - Patron is notified with error 'on_reserve'
13 - APPLY PATCH
14 - Unset 'No renewal before'
15 - UPDATE issues SET auto_renew_error = NULL;
16 - run cronjob
17 - Patron is not notified
18 - UPDATE issues SET auto_renew_error = NULL;
19 - Set 'No renewal before' to 1
20 - run cronjob
21 - Patron is notified with error 'on_reserve'
22 - Set 'No renewal before' to 3
23 - UPDATE issues SET auto_renew_error = NULL;
24 - run cronjob
25 - Patron is not notified
Comment 37 Nick Clemens 2019-06-11 16:36:57 UTC
I tested David's patch with our educators, I don't think it quite does what it meant to do.  I attached an updated version of David's patch that simplifies the logic, I believe this fulfills the intent
Comment 38 David Cook 2019-06-13 01:15:43 UTC
(In reply to Nick Clemens from comment #37)
> I tested David's patch with our educators, I don't think it quite does what
> it meant to do.  I attached an updated version of David's patch that
> simplifies the logic, I believe this fulfills the intent

Could you elaborate on that, Nick?

I think my patch does what I mean it to do, but maybe I'm not clearly communicating what I mean it to do?

If you look at Circulation.pm (I think), I'm basically just copying what already happens if a person tries to perform a renewal manually. So I'm trying to harmonize the human and machine methods of renewing, and that takes into account NoRenewalBefore from the circulation rules.
Comment 39 Andrew Fuerste-Henry 2019-06-13 19:38:26 UTC
Hi David! Nick could give you better details, but we got stuck at one point in your test plan when, with the patch applied, the system wouldn't renew or apply an auto-renew error to a checkout that was within the window defined by No Renewal Before but was not yet overdue.
Comment 40 David Cook 2019-06-14 00:05:39 UTC
(In reply to Andrew from comment #39)
> Hi David! Nick could give you better details, but we got stuck at one point
> in your test plan when, with the patch applied, the system wouldn't renew or
> apply an auto-renew error to a checkout that was within the window defined
> by No Renewal Before but was not yet overdue.

Thanks for the details, Andrew. I'll take another look when I have some time. Cheers!
Comment 41 David Cook 2019-11-11 02:33:59 UTC
I'm probably not going to work on this for a long time.

I have a local fix which seems to be working for my libraries. I'll keep this on my todo list, but have no plans to work on it currently.
Comment 42 Nick Clemens 2019-11-12 19:10:23 UTC
Created attachment 95348 [details] [review]
Bug 19014: Only process issues for autorenewal if on or after soonest renewal date

This patch check the soonest renewal date and skips processing auto renewals if before this date
This ensures patrons won't get notifications far in advance if an item is on hold or has other issues

To test:
 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' unset
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - perl misc/cronjobs/automatic_renewals.pl --send-notices
 5 - Patron is not notified
 6 - Set 'No renewal before' to 1
 7 - run cronjob
 8 - Patron is notified with error 'on_reserve'
 9 - Set 'No renewal before' to 3
10 - UPDATE issues SET auto_renew_error = NULL;
11 - run cronjob
12 - Patron is notified with error 'on_reserve'
13 - APPLY PATCH
14 - Unset 'No renewal before'
15 - UPDATE issues SET auto_renew_error = NULL;
16 - run cronjob
17 - Patron is not notified
18 - UPDATE issues SET auto_renew_error = NULL;
19 - Set 'No renewal before' to 1
20 - run cronjob
21 - Patron is notified with error 'on_reserve'
22 - Set 'No renewal before' to 3
23 - UPDATE issues SET auto_renew_error = NULL;
24 - run cronjob
25 - Patron is not notified
Comment 43 Andrew Fuerste-Henry 2019-11-25 18:22:35 UTC
New test plan:

To test:
 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' set to 1
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - Patron is notified with error 'on_reserve'
 5 - Set 'No renewal before' to 3
 6 - UPDATE issues SET auto_renew_error = NULL;
7 - run cronjob
8 - Patron is notified with error 'on_reserve'
9 - APPLY PATCH
10 - Set 'No renewal before' to 1
11 - UPDATE issues SET auto_renew_error = NULL;
12 - run cronjob
13 - Patron is not notified
14 - UPDATE issues SET auto_renew_error = NULL;
15 - Set 'No renewal before' to 3
16 - UPDATE issues SET auto_renew_error = NULL;
17 - run cronjob
18 - Patron is notified with error 'on_reserve'
Comment 44 Andrew Fuerste-Henry 2019-11-25 19:30:07 UTC
Failing QA.
Testing with an item due 4 days in the future, No Renewal Before set to 1, issue still gets assessed by the auto-renew cron. Gets an auto_renew_error of auto_too_soon when no hold is placed on the item. Gets an error of on_reserve when there is a hold.
Comment 45 Nick Clemens 2019-11-26 16:27:05 UTC
Please retest
Comment 46 ByWater Sandboxes 2019-11-27 16:08:55 UTC
Created attachment 95851 [details] [review]
Bug 19014: Only process issues for autorenewal if on or after soonest renewal date

This patch check the soonest renewal date and skips processing auto renewals if before this date
This ensures patrons won't get notifications far in advance if an item is on hold or has other issues

To test:
 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' unset
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - perl misc/cronjobs/automatic_renewals.pl --send-notices
 5 - Patron is not notified
 6 - Set 'No renewal before' to 1
 7 - run cronjob
 8 - Patron is notified with error 'on_reserve'
 9 - Set 'No renewal before' to 3
10 - UPDATE issues SET auto_renew_error = NULL;
11 - run cronjob
12 - Patron is notified with error 'on_reserve'
13 - APPLY PATCH
14 - Unset 'No renewal before'
15 - UPDATE issues SET auto_renew_error = NULL;
16 - run cronjob
17 - Patron is not notified
18 - UPDATE issues SET auto_renew_error = NULL;
19 - Set 'No renewal before' to 1
20 - run cronjob
21 - Patron is notified with error 'on_reserve'
22 - Set 'No renewal before' to 3
23 - UPDATE issues SET auto_renew_error = NULL;
24 - run cronjob
25 - Patron is not notified

Signed-off-by: 

Signed-off-by:
Comment 47 ByWater Sandboxes 2019-11-27 16:15:01 UTC
Created attachment 95852 [details] [review]
Bug 19014: Only process issues for autorenewal if on or after soonest renewal date

This patch check the soonest renewal date and skips processing auto renewals if before this date
This ensures patrons won't get notifications far in advance if an item is on hold or has other issues

To test:
 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' unset
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - perl misc/cronjobs/automatic_renewals.pl --send-notices
 5 - Patron is not notified
 6 - Set 'No renewal before' to 1
 7 - run cronjob
 8 - Patron is notified with error 'on_reserve'
 9 - Set 'No renewal before' to 3
10 - UPDATE issues SET auto_renew_error = NULL;
11 - run cronjob
12 - Patron is notified with error 'on_reserve'
13 - APPLY PATCH
14 - Unset 'No renewal before'
15 - UPDATE issues SET auto_renew_error = NULL;
16 - run cronjob
17 - Patron is not notified
18 - UPDATE issues SET auto_renew_error = NULL;
19 - Set 'No renewal before' to 1
20 - run cronjob
21 - Patron is notified with error 'on_reserve'
22 - Set 'No renewal before' to 3
23 - UPDATE issues SET auto_renew_error = NULL;
24 - run cronjob
25 - Patron is not notified

Signed-off-by: 

Signed-off-by: 

Signed-off-by: Hasina Akhter <hasinaa@pascolibraries.org>
Comment 48 Andrew Fuerste-Henry 2019-11-27 16:17:02 UTC
Apologies for some messy signing-off :) Signed off on behalf of Hasina.
Comment 49 Marcel de Rooy 2019-12-13 10:53:28 UTC
It would be nice if some of the people involved earlier could add a comment on the last signed off patch. It is actually the third try to solve this, if I am reading well.
Comment 50 David Cook 2019-12-16 05:39:03 UTC
(In reply to Marcel de Rooy from comment #49)
> It would be nice if some of the people involved earlier could add a comment
> on the last signed off patch. It is actually the third try to solve this, if
> I am reading well.

The code looks good to me. I give it a thumbs up/+1.
Comment 51 Jonathan Druart 2019-12-27 15:13:36 UTC
Nick, do not we want to truncate to days before the date comparison?
Comment 52 Nick Clemens 2019-12-30 19:36:03 UTC
Created attachment 96688 [details] [review]
Bug 19014: Unit tests
Comment 53 Nick Clemens 2019-12-30 19:36:08 UTC
Created attachment 96689 [details] [review]
Bug 19014: Return auto_too_soon before on_reserve

When calling CanBookBeRenewed we do check for 'too_soon', however, a reserve
makes the return 'on_reserve' before other statuses. It seems the patron specific reasons
should be returned first. This patch places the 'on_reserve' check after too_soon and others

To test:
 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' unset
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - perl misc/cronjobs/automatic_renewals.pl --send-notices
 5 - Patron is not notified
 6 - Set 'No renewal before' to 1
 7 - run cronjob
 8 - Patron is notified with error 'on_reserve'
 9 - Set 'No renewal before' to 3
10 - UPDATE issues SET auto_renew_error = NULL;
11 - run cronjob
12 - Patron is notified with error 'on_reserve'
13 - APPLY PATCH
14 - Unset 'No renewal before'
15 - UPDATE issues SET auto_renew_error = NULL;
16 - run cronjob
17 - Patron is not notified
18 - UPDATE issues SET auto_renew_error = NULL;
19 - Set 'No renewal before' to 1
20 - run cronjob
21 - Patron is notified with error 'on_reserve'
22 - Set 'No renewal before' to 3
23 - UPDATE issues SET auto_renew_error = NULL;
24 - run cronjob
25 - Patron is not notified
Comment 54 Nick Clemens 2019-12-30 19:37:23 UTC
(In reply to Jonathan Druart from comment #51)
> Nick, do not we want to truncate to days before the date comparison?

Yes, we already have this code in C4::Circulation:CanBookBeRenewed - I submit a new patch to make use of that code and cover it with tests rather than add logic to the cron
Comment 55 Andrew Fuerste-Henry 2019-12-31 15:14:24 UTC
*** Bug 20360 has been marked as a duplicate of this bug. ***
Comment 56 Andrew Fuerste-Henry 2020-01-10 21:15:03 UTC
Stalling out on signing this off while holds are broken, but reading your test plan for the 100th time. In steps 19-21 we have an item due in 2 days and no renewal before set to 1 day. If our goal is to not send a notice outside of the no renewal before period, shouldn't our patrons *not* get a notice in this situation?
Comment 57 Andrew Fuerste-Henry 2020-01-14 18:58:14 UTC
With this patch applied renewal is not blocked by a hold.
Comment 58 Nick Clemens 2020-01-15 02:07:54 UTC
Created attachment 97374 [details] [review]
Bug 19014: on_reserve blocks auto_renew
Comment 59 ByWater Sandboxes 2020-01-15 14:50:16 UTC
Created attachment 97395 [details] [review]
Bug 19014: Unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 60 ByWater Sandboxes 2020-01-15 14:50:19 UTC
Created attachment 97396 [details] [review]
Bug 19014: Return auto_too_soon before on_reserve

When calling CanBookBeRenewed we do check for 'too_soon', however, a reserve
makes the return 'on_reserve' before other statuses. It seems the patron specific reasons
should be returned first. This patch places the 'on_reserve' check after too_soon and others

To test:
 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' unset
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - perl misc/cronjobs/automatic_renewals.pl --send-notices
 5 - Patron is not notified
 6 - Set 'No renewal before' to 1
 7 - run cronjob
 8 - Patron is notified with error 'on_reserve'
 9 - Set 'No renewal before' to 3
10 - UPDATE issues SET auto_renew_error = NULL;
11 - run cronjob
12 - Patron is notified with error 'on_reserve'
13 - APPLY PATCH
14 - Unset 'No renewal before'
15 - UPDATE issues SET auto_renew_error = NULL;
16 - run cronjob
17 - Patron is not notified
18 - UPDATE issues SET auto_renew_error = NULL;
19 - Set 'No renewal before' to 1
20 - run cronjob
21 - Patron is notified with error 'on_reserve'
22 - Set 'No renewal before' to 3
23 - UPDATE issues SET auto_renew_error = NULL;
24 - run cronjob
25 - Patron is not notified

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 61 ByWater Sandboxes 2020-01-15 14:50:23 UTC
Created attachment 97397 [details] [review]
Bug 19014: on_reserve blocks auto_renew

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 62 Andrew Fuerste-Henry 2020-01-15 14:53:11 UTC
Modified test plan:

 1 - Checkout an item to a patron with a due date 2 days in the future and 'No renewal before' unset
 2 - Place a hold on that item
 3 - Define an AUTO_RENEWALS circulation notice with below at a minimum
     [% checkout.auto_renew_error %]
 4 - perl misc/cronjobs/automatic_renewals.pl --send-notices
 5 - Patron is not notified
 6 - Set 'No renewal before' to 1
 7 - run cronjob
 8 - Patron is notified with error 'on_reserve'
 9 - Set 'No renewal before' to 3
10 - UPDATE issues SET auto_renew_error = NULL;
11 - run cronjob
12 - Patron is notified with error 'on_reserve'
13 - APPLY PATCH
14 - Unset 'No renewal before'
15 - UPDATE issues SET auto_renew_error = NULL;
16 - run cronjob
17 - Patron is not notified
18 - UPDATE issues SET auto_renew_error = NULL;
19 - Set 'No renewal before' to 2
20 - run cronjob
21 - Patron is notified with error 'on_reserve'
22 - Set 'No renewal before' to 3
23 - UPDATE issues SET auto_renew_error = NULL;
24 - run cronjob
25 - Patron is notified with error 'on_reserve'
Comment 63 Martin Renvoize 2020-02-11 08:16:37 UTC
Sorry Nick, this one needs a rebase.  The coding looks solid to me, and it seems to appease the many.

I've opened a new bug to take into account Ray's last comments regarding enhanced control of reserve targets.

Once rebased, I'll give this a once over for QA.
Comment 64 Nick Clemens 2020-02-11 13:18:47 UTC
Created attachment 98704 [details] [review]
Bug 19014: Unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 65 Nick Clemens 2020-02-11 13:18:51 UTC
Created attachment 98705 [details] [review]
Bug 19014: Return auto_too_soon before on_reserve
Comment 66 Nick Clemens 2020-02-11 13:18:55 UTC
Created attachment 98706 [details] [review]
Bug 19014: on_reserve blocks auto_renew
Comment 67 Martin Renvoize 2020-02-11 16:41:19 UTC
Created attachment 98717 [details] [review]
Bug 19014: Unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 68 Martin Renvoize 2020-02-11 16:41:23 UTC
Created attachment 98718 [details] [review]
Bug 19014: Return auto_too_soon before on_reserve

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 69 Martin Renvoize 2020-02-11 16:41:28 UTC
Created attachment 98719 [details] [review]
Bug 19014: on_reserve blocks auto_renew

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 70 Martin Renvoize 2020-02-11 16:41:32 UTC
Created attachment 98720 [details] [review]
Bug 19014: (QA follow-up) Update test for AddReserve change

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 71 Martin Renvoize 2020-02-11 16:43:38 UTC
Works well, passes tests and qa scripts.

I can't find any regressions and the code reads well.

Passing QA
Comment 72 Martin Renvoize 2020-02-12 16:09:59 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 73 Joy Nelson 2020-03-05 00:53:53 UTC
Does not apply to 19.11.x branch.  please rebase if needed in 19.11.x branch
Comment 74 Nick Clemens 2020-03-05 12:03:52 UTC
Created attachment 100180 [details] [review]
Bug 19014: [19.11.x] Prevent auto_renew notices form sending if too early to renew

Bug 19014: Unit tests

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 19014: Return auto_too_soon before on_reserve

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Bug 19014: on_reserve blocks auto_renew

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Joy Nelson 2020-04-28 21:43:37 UTC
backported to 19.11.x branch for 19.11.06
Comment 76 Lucas Gass 2020-05-11 20:52:27 UTC
Doesn't apply cleanly to 19.05.x branch. No backport
Comment 77 Jonathan Druart 2020-08-26 09:08:33 UTC
What has been done here is not correct. The side-effects it introduced are quite big. Please see bug 25758 that will need lot of testing.

I am not sure bug 25758 will be safe to backport and maybe we should consider reverting this patchset for stable branches.
Comment 78 Martin Renvoize 2020-09-04 08:52:14 UTC
I've played with this a bit more now and am inclined to agree with Jonathan regarding reverting this from stables and waiting for the enhancement to surface with the 20.11 release with the followup bug hopefully resolving the issues this introduced.
Comment 79 Nick Clemens 2020-09-04 11:26:46 UTC
(In reply to Martin Renvoize from comment #78)
> I've played with this a bit more now and am inclined to agree with Jonathan
> regarding reverting this from stables and waiting for the enhancement to
> surface with the 20.11 release with the followup bug hopefully resolving the
> issues this introduced.

I disagree, reverting this will just trade one issue for another. This patchset tackles the problem of too many incorrect emails - the other the problem of incorrect display. I am going to work on a patch for bug 25758