Failure reason autorenewal notifications to patrons should not include the event code

[Michele Morgan]

When a patron is notified of a failed autorenewal attempt, the item data in the email message looks like this example.

    Title: Harry Potter and the prisoner of Azkaban
    Author: Rowling, J. K.
    Library: Danvers
    Status: **NOT RENEWED, PLEASE RETURN**
    Reason: COPY_NEEDED_FOR_HOLD : Copy is needed to fulfill a hold
    Due Date: 07-23-2019

    Title: Call the midwife Season six
    Author: Agutter, Jenny
    Library: Danvers
    Status: **NOT RENEWED, PLEASE RETURN**
    Reason: MAX_RENEWALS_REACHED : Circulation has no more renewals remaining
    Due Date: 07-23-2019

Patrons don't need to see the failure code (COPY_NEEDED_FOR_HOLD, MAX_RENEWALS_REACHED, etc.) for failed renewals, just the description.

[Jason Stephenson (jstephenson)]

I have made a "fix" for this and loaded it on our testing server, I'm sharing it here so that Michele and others may review it:

user/dyrcona/lp1842431-autorenew-split-event-textcode-description

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1842431-autorenew-split-event-textcode-description

I'm not trying to compete with Michele, it's just that we had a branch ready to share.

[Michele Morgan (mmorgan)]

Thanks for sharing your branch Jason! This is what I've been working with:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mmorgan/lp1842431_fix_renewal_failure_reason

[% udata.reason %] is what gets used in the stock AutorenewNotify action trigger template, so that's where I made the change.

Adding a pullrequest tag. Comments welcome.

[Jason Stephenson (jstephenson)]

Michele, while I like the simplicity of your fix, I wonder if someone would want to display the text code in a patron notice.

I made my change to follow the principle of least surprise, i.e. the current behavior of the default template is unchanged. I added separate fields for the textcode and the description in case someone would want to display either of those individually. I'll admit that my branch is probably overkill, but I'll leave it to others to decide which fix is better, of if some combination of the two should be done, i.e. removing textcode from the reason but adding it as its own field.

Anyway, I give my signoff to your change if that's the direction that others want to go.

[Michele Morgan (mmorgan)]

Maybe both changes should go in. In that way, the stock notices would give a friendlier message, but the option to include the text code would be there, too.

I added Jason's changes to this branch:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mmorgan/lp1842431_fix_renewal_failure_reason_take_two

[Jason Stephenson (jstephenson)]

I have rebased Michelle's latest branch on master and added my signoff to her first commit here:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dyrcona/lp1842431_fix_renewal_failure_reason_take_two-signoff

I say this is ready to go, but would like another set of eyes before pushing it.

[Bill Erickson (berick)]

Moving from Wishlist to Low-priority bug fix. I see this as a usability bug. Input welcome.

[Jason Boyer (jboyer)]

This change looks good to me, and I'm signing off on Michele and Jason's commits, but I'd like to suggest description be dropped because I think it can lead to confusion. Rather than type out all of that rationale twice, here's the commit message from the tip of my signoff branch:

This branch brings about a good change, but I think having reason
and description be identical in the case of a failure and blank /
'SUCCESS' in the case of, well, success, is redundant and potentially
confusing. Also, if it's not used in the default template I doubt
anyone ever realizes it's there. :) Template editors can use
is_renewed to decide if they want to display success as a result
and that way the capitalization won't look like THE EIGHTIES have come
back in fashion again. :D

It's at https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads / working/user/jboyer/lp1842431_signoff_plus

[Jason Stephenson (jstephenson)]

Jason, thanks for the signoff and the suggestion of removing the description field from the data. I agree that it is redundant. I simply combined my branch with Michelle's.

I have pushed this fix to master, rel_3_4, and rel_3_5.

Thanks, everyone!

[Jason Boyer (jboyer)]

I should have thought of this earlier, but I'm going to throw a release note in there real quick since the format of reason will be changing and some people (like me) may have taken steps to fix this already in the templates themselves...