Bug 6739 - expired patrons not blocked from opac actions
Summary: expired patrons not blocked from opac actions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact: Katrin Fischer
URL:
Keywords:
: 6348 6785 (view as bug list)
Depends on:
Blocks: 10932
  Show dependency treegraph
 
Reported: 2011-08-17 10:58 UTC by Nicole C. Engard
Modified: 2015-06-04 23:23 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Ability to block expired patrons placing OPAC reserves or holds on a system and category levels (24.72 KB, patch)
2012-02-23 00:40 UTC, Chris Hall
Details | Diff | Splinter Review
Ability to block expired patrons placing OPAC reserves or holds on a system and category levels (23.62 KB, patch)
2012-04-11 05:57 UTC, Chris Hall
Details | Diff | Splinter Review
Bug 6739 - added the ability to configure whether an expired patron can perform opac actions such as renew and reserve on both a syspref and patron-category level (38.07 KB, patch)
2013-03-12 12:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (45.18 KB, patch)
2013-03-12 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (45.88 KB, patch)
2013-03-12 13:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (46.04 KB, patch)
2013-03-12 13:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (6.29 KB, patch)
2013-03-12 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (45.67 KB, patch)
2013-03-12 18:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (6.29 KB, patch)
2013-03-12 18:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (revised) (26.69 KB, patch)
2013-04-11 09:25 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (6.33 KB, patch)
2013-04-11 09:38 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (4.59 KB, patch)
2013-04-15 16:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (9.72 KB, patch)
2013-04-15 16:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (10.07 KB, patch)
2013-04-15 16:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
[Signed off] Bug 6739 - expired patrons not blocked from opac actions (26.75 KB, patch)
2013-04-18 00:09 UTC, Melia Meggs
Details | Diff | Splinter Review
[Signed off] Bug 6739 - expired patrons not blocked from opac actions - Followup (10.14 KB, patch)
2013-04-18 00:09 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (27.07 KB, patch)
2013-05-02 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (10.13 KB, patch)
2013-05-02 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (11.26 KB, patch)
2013-05-02 18:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions (26.97 KB, patch)
2013-05-09 13:51 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup (10.23 KB, patch)
2013-05-09 13:52 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (11.37 KB, patch)
2013-05-09 13:53 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions (26.50 KB, patch)
2013-07-11 09:42 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup (10.18 KB, patch)
2013-07-11 09:42 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (11.33 KB, patch)
2013-07-11 09:43 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (4.56 KB, patch)
2013-07-11 10:11 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (4.04 KB, patch)
2013-07-11 10:46 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (4.13 KB, patch)
2013-07-11 10:48 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (4.60 KB, patch)
2013-07-15 15:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (4.18 KB, patch)
2013-07-15 15:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions (26.61 KB, patch)
2013-07-31 09:26 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 1 (10.18 KB, patch)
2013-07-31 09:26 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (11.33 KB, patch)
2013-07-31 09:27 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (4.72 KB, patch)
2013-07-31 09:28 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (4.19 KB, patch)
2013-07-31 09:29 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions (26.76 KB, patch)
2013-08-27 08:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 1 (10.24 KB, patch)
2013-08-27 08:58 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (6.77 KB, patch)
2013-08-27 09:03 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (5.27 KB, patch)
2013-08-27 09:03 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (4.27 KB, patch)
2013-08-27 09:04 UTC, Fridolin Somers
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions (27.32 KB, patch)
2013-09-22 14:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup (10.30 KB, patch)
2013-09-22 14:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (6.83 KB, patch)
2013-09-22 14:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (5.33 KB, patch)
2013-09-22 14:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (6.21 KB, patch)
2013-09-22 14:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 5 (4.04 KB, patch)
2013-09-22 14:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (27.31 KB, patch)
2014-01-15 11:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (10.29 KB, patch)
2014-01-15 11:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (6.92 KB, patch)
2014-01-15 11:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (5.32 KB, patch)
2014-01-15 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (6.21 KB, patch)
2014-01-15 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 5 (4.11 KB, patch)
2014-01-15 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739: FIX conflicts with some others bugs. (5.53 KB, patch)
2014-01-15 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739: template changes for the bootstrap theme (6.91 KB, patch)
2014-01-15 11:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (27.32 KB, patch)
2014-02-10 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (10.29 KB, patch)
2014-02-10 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (6.92 KB, patch)
2014-02-10 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (5.32 KB, patch)
2014-02-10 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (6.21 KB, patch)
2014-02-10 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 5 (4.11 KB, patch)
2014-02-10 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739: FIX conflicts with some others bugs. (5.57 KB, patch)
2014-02-10 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739: template changes for the bootstrap theme (6.91 KB, patch)
2014-02-10 08:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 5 (4.15 KB, patch)
2014-02-13 22:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6739: FIX conflicts with some others bugs. (5.62 KB, patch)
2014-02-13 22:30 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6739: template changes for the bootstrap theme (6.96 KB, patch)
2014-02-13 22:31 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions (27.32 KB, patch)
2014-03-25 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup (10.29 KB, patch)
2014-03-25 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (6.97 KB, patch)
2014-03-25 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (5.32 KB, patch)
2014-03-25 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (6.21 KB, patch)
2014-03-25 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739 - expired patrons not blocked from opac actions - Followup 5 (4.11 KB, patch)
2014-03-25 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739: FIX conflicts with some others bugs. (5.53 KB, patch)
2014-03-25 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 6739: template changes for the bootstrap theme (6.91 KB, patch)
2014-03-25 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions (27.31 KB, patch)
2014-04-06 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup (10.29 KB, patch)
2014-04-06 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 2 (6.99 KB, patch)
2014-04-06 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 3 (5.32 KB, patch)
2014-04-06 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 4 (6.35 KB, patch)
2014-04-06 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 5 (4.16 KB, patch)
2014-04-06 20:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739: FIX conflicts with some others bugs. (5.59 KB, patch)
2014-04-06 20:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 6739: Template changes for the bootstrap theme (9.26 KB, patch)
2014-04-06 20:11 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-08-17 10:58:48 UTC
A patron with an expired card can still renew books and place holds via the opac.  Since the staff can't do these thing without renewing the patron's card shouldn't the patron be blocked in the OPAC as well?

Also, there should be some kind of note to the patron that the account has expired and they need to see a librarian.
Comment 1 Chris Cormack 2011-08-25 21:43:52 UTC
Working on this
Comment 2 Chris Cormack 2011-08-25 21:56:02 UTC
Work to be done
* Add note on opac-user if users card has expired
* Block renewals and holds
Comment 3 Chris Cormack 2011-08-25 22:56:22 UTC
The note already shows

I just marked a patron as restricted, and got

Please note: Your account has been frozen. Usually the reason for freezing an account is old overdues or damage fees.If your account page shows your account to be clear, please contact the library.
Comment 4 Chris Cormack 2011-08-25 22:56:43 UTC
Will work on a patch to make it block holds and renewals too
Comment 5 Chris Cormack 2011-08-25 23:03:47 UTC
Using the text of the note for expired also
Comment 6 Chris Cormack 2011-08-25 23:12:13 UTC
You can place a hold in the staff client without renewing the expiry, ditto renewals.
You just cant issue a new item to them if their card has expired.

Is there a system preference I need to set to make them be blocked Nicole?
Comment 7 Nicole C. Engard 2011-08-26 00:02:05 UTC
Chris,

There is an override renewals limits preference (AllowRenewalLimitOverride), but it's specifically associated with the limit on number of renewals - not the fact that the patron is expired.  

Basically, what I think is being asked for here is if a patron is expired their account acts as if it's restricted and renewals, holds, checkouts are all blocked until the card is renewed by a librarian.  This block should happen in both the OPAC and the staff client - otherwise, why have an expiration date?

Make sense?

Nicole
Comment 8 Chris Cormack 2011-08-26 00:10:43 UTC
Makes sense

But I have an expired patron, and their renewals and holds are NOT blocked in the staff client.

So I was wondering how/what I had to set to make that blocking happen in the staff.
Comment 9 Nicole C. Engard 2011-09-06 19:32:23 UTC
Chris,

Are you asking me a question still?

Nicole
Comment 10 Chris Cormack 2011-09-06 21:14:08 UTC
Yes, im trying to recreate the problem

"But I have an expired patron, and their renewals and holds are NOT blocked in
the staff client.

So I was wondering how/what I had to set to make that blocking happen in the
staff.?"
Comment 11 Nicole C. Engard 2011-09-07 16:03:17 UTC
OH! I see - it sounded like you were repeating my question.  But i see what you're saying.  I think that if they're expired they should be blocked all around - in the staff and the opac until the librarian addresses the expiration message. 

Right now only checkouts appear to be blocked, which is silly if you're not blocking the other actions.

I'm up for others to chime in with their opinions.

Nicole
Comment 12 Nicole C. Engard 2011-09-07 16:08:33 UTC
*** Bug 6785 has been marked as a duplicate of this bug. ***
Comment 13 Katrin Fischer 2011-09-07 16:35:34 UTC
I think renewals should not be possible for expired patrons. But perhaps allow holds so that they have a reason to go into the library and have their patron cards renewed? Might be more consistent though to block everything.
Comment 14 Ian Walls 2011-12-06 13:35:14 UTC
This is going to need to be one of those system-preferenced kind of things.  There is no way every library is going to agree whether expired patrons should be any to do X but not Y and Z.

What we really need is patron permissions (CanBorrow, CanOPACRenew, CanOPACPlaceHold etc) on an individual basis, with defaults set by patron type like we do for message preferences, but with hooks/triggers that change this permissions under certain conditions (expired account, fines too high, too many overdues, etc).
Comment 15 Chris Hall 2012-02-23 00:40:51 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2012-03-01 07:05:20 UTC
Hi Chris H.,

I was looking at your patch and I am a bit worried about the logic of the configuration options. 
The patron category now has a pull down with 3 settings: block, don't block, use syspref. And the system preference is block or don't block. 

When I am in the system preference editor, I have no way to find out, if my patron categories are affected by my changes. And when I am looking at patron categories I don't know what 'use syspref' will do. This seems confusing to me. Perhaps we could do it the other way around matching the behaviour of enhancedmessaging a bit?

System preference: Block, Don't block, Configure on category type level
Settings on patron category only show up when system preference is set to allow configuraton on category tyle level.

For testing I fixed 2 small conflicts in your patch, be careful to keep the check on existence of dateexpiry when you rebase your patch. This was a recent change. Also the version number in updatedatabase should be XXX - this will make it easier for RM and is easier to test. With XXX you only have to update the kohaversion to the next higher number and the database update will happen.
Comment 17 Jared Camins-Esakov 2012-03-11 22:13:43 UTC
I can't figure out how to resolve the conflict in opac/opac-user.pl, so I am marking this "Patch doesn't apply."
Comment 18 Chris Hall 2012-04-11 05:57:01 UTC Comment hidden (obsolete)
Comment 19 Jared Camins-Esakov 2012-05-13 11:14:09 UTC
I tried setting this BlockExpiredPatronOpacActions to "block," but it doesn't seem to have any effect. I'm still able to place holds in the OPAC.
Comment 20 Kyle M Hall 2013-03-12 12:38:42 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-03-12 13:47:10 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-03-12 13:50:27 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-03-12 13:55:51 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2013-03-12 14:01:29 UTC
This should probably have another signer before getting a signed-off status.
Comment 25 Owen Leonard 2013-03-12 14:40:14 UTC
After applying the patch I no longer see the logged-in user's name at the top of the screen ("Welcome, ").

This kind of construction is a violation of coding guidelines because it breaks the translator:

 <option value="1"  [% IF ( BlockExpiredPatronOpacActions == 1   ) %] selected="selected" [% END %]> Block </option>

http://wiki.koha-community.org/wiki/Coding_Guidelines#HTML1:_Template_Toolkit_markup_inside_HTML

In opac-user.tt:

"Please contact the library if you wish to renew your subscription."

I don't think we use the term "subscription" anywhere in Koha to refer to the patron's account. I see two variations in the template right now: "card has expired" and "account has expired." We should pick one of these for consistency's sake: "renew your card" or "renew your account."

There are a couple of instances of this HTML typo in opac-user.tt:

<string>Please note:</string>

I assume that should be "strong."

Also in opac-user.tt:

You're renew failed

That should be "Your."

In opac-renew.pl:

print $query->redirect("/cgi-bin/koha/opac-user.pl?renew_error=$errorstring");

You can't build an error string (in English) in the script and pass it to the template for display. This is untranslatable. It's also bad practice to pass unsanitized data directly to the template .

Also, this patch has a huge amount of whitespace changes that appear to be unrelated to functional changes. I don't know if that is another cause to fail the patch, but it sure makes it harder to understand.
Comment 26 Kyle M Hall 2013-03-12 17:03:23 UTC
> Also, this patch has a huge amount of whitespace changes that appear to be
> unrelated to functional changes. I don't know if that is another cause to
> fail the patch, but it sure makes it harder to understand.

I'll take care of those changes. I had to make a lot of whitespace changes due to the fact that the code layout was a bit on the side of madness. "git diff -w" will hide changes that are just changes in indentation. I hope that is not cause alone for failing the patch. It's quite onerous to have to fix whitespace to understand the functioning of a script, undo those fixes, then make edits, then redo the fixes in a second patch.
Comment 27 Kyle M Hall 2013-03-12 17:47:01 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2013-03-12 18:25:52 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-03-12 18:26:17 UTC Comment hidden (obsolete)
Comment 30 Pascale Nalon 2013-03-18 13:28:30 UTC
Something went wrong !
CONFLICT (content)
Comment 31 Fridolin Somers 2013-04-11 09:25:35 UTC Comment hidden (obsolete)
Comment 32 Fridolin Somers 2013-04-11 09:38:19 UTC Comment hidden (obsolete)
Comment 33 Fridolin Somers 2013-04-11 09:42:48 UTC
I noticed that some changes in opac-user.pl seems to be not directly related to this bug purpose.
Some lines concerns OPACDisplayRequestPriority and NotifyBorrowerDeparture syspref behaviors.
Could this be explained is patch comment ?
Comment 34 Melia Meggs 2013-04-12 20:25:44 UTC
Hello,

I applied the revised and followup patches together, but even with "Block expired patrons" set to "Block," I was still able to renew from the OPAC as an expired patron.  

I also discovered some strange behavior with renewing patrons while testing, but I'm not sure if it's related to this patch.  Here's what happened:  first I created a test patron and gave him an expiry date in the past.  Then after going to the OPAC to test placing a hold, I realized that I would need to check something out to my patron so that I could test renewing an item from the OPAC.  I couldn't check out to the expired test patron, so I tried to renew him.  When I tried to renew the patron, things got weird.  I got a message saying "Patron renewed until" with no date, but the warning "Patron expired" didn't go away.  I then tried actually changing the expiry date from the patron record, but I still couldn't renew him.  I had to switch back to master to renew my patron.  Not sure if that is a consequence of this patch or not, but renewing the patron did seem to work fine in master.

Thanks,

Melia
Comment 35 Katrin Fischer 2013-04-13 23:08:10 UTC
*** Bug 6348 has been marked as a duplicate of this bug. ***
Comment 36 Kyle M Hall 2013-04-15 16:26:20 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2013-04-15 16:27:03 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2013-04-15 16:54:11 UTC Comment hidden (obsolete)
Comment 39 David Cook 2013-04-16 09:34:48 UTC
(In reply to comment #14)
> This is going to need to be one of those system-preferenced kind of things. 
> There is no way every library is going to agree whether expired patrons
> should be any to do X but not Y and Z.
> 
> What we really need is patron permissions (CanBorrow, CanOPACRenew,
> CanOPACPlaceHold etc) on an individual basis, with defaults set by patron
> type like we do for message preferences, but with hooks/triggers that change
> this permissions under certain conditions (expired account, fines too high,
> too many overdues, etc).

Maybe I'm overlooking something obvious, but why are we letting an expired patron do "anything". This seems like a huge patch for something that should be straightforward, no?

When logging into the OPAC, shouldn't we just check to see if the patron is expired, and if so...throw up a page that says "You're expired. Consult the staff at your library to be renewed" or something along those lines.
Comment 40 Katrin Fischer 2013-04-16 09:51:47 UTC
As an example: I think in order to make them renew their account it can be good to allow holds. You see a book you want to get, place a hold on it, go to the library... renew your account. But that's something not every library might want to do this way, so it needs options.

I would argue that if a patron is restricted they should not be able to do anything, at least for something like lost cards. But that's another discussion probably. :)
Comment 41 David Cook 2013-04-16 23:22:17 UTC
(In reply to comment #40)
> As an example: I think in order to make them renew their account it can be
> good to allow holds. You see a book you want to get, place a hold on it, go
> to the library... renew your account. But that's something not every library
> might want to do this way, so it needs options.
> 
> I would argue that if a patron is restricted they should not be able to do
> anything, at least for something like lost cards. But that's another
> discussion probably. :)

Mmm, I see what you're saying now. That is rather obvious now that I think about it ;).
Comment 42 Melia Meggs 2013-04-18 00:09:12 UTC Comment hidden (obsolete)
Comment 43 Melia Meggs 2013-04-18 00:09:29 UTC Comment hidden (obsolete)
Comment 44 Melia Meggs 2013-04-18 00:11:37 UTC
Ok, I followed the test plan again with the new follow up patch, and it all seems to be working perfectly now.  Thanks, Kyle!

Could we please get one more signoff on this patch?

Thanks,

Melia
Comment 45 Owen Leonard 2013-04-26 17:26:19 UTC
(In reply to comment #42)
>  1) Apply patch
>  2) Run updatedatabase.pl
>  3) Pick a patron, note the patron's category
>  5) Issue an item to this patron
>  4) Edit that category, set "Block expired patrons" to "Block"
>  5) Verify the patron cannot renew or place holds in the opac

At this step in the test plan I would expect that the summary of checkouts on opac-user.pl not show me checkboxes for performing renewals, or any of the other controls related to renewals.

If I check a box and try to renew, I get a message that says " Please note: Your account renewal failed because of the following: "
Comment 46 Kyle M Hall 2013-05-02 18:18:38 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2013-05-02 18:18:52 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2013-05-02 18:19:06 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2013-05-02 18:23:06 UTC
Comment on attachment 17909 [details] [review]
Bug 6739 - expired patrons not blocked from opac actions - Followup 2

Review of attachment 17909 [details] [review]:
-----------------------------------------------------------------

Most of the changes in opac-user.tt are whitespace cleanup. I've marked out the actual changes.

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt
@@ +70,3 @@
>              });
> +
> +            [% IF ( canrenew && !userdebarred && OpacRenewalAllowed && !( borrower.is_expired && borrower.BlockExpiredPatronOpacActions ) ) %]

This line fixes the renewal links bug.

@@ +132,4 @@
>                  <string>Please note:</string>
>                  <span>
>                      Your account renewal failed because of the following:
> +                    [% FOREACH error IN RENEW_ERROR.split('\|') %]

The pipe character needed escaping, otherwise it was split into an array of single characters.

@@ +205,5 @@
> +                        [% END %]
> +
> +                        <th>Call No.</th>
> +
> +                        [% IF OpacRenewalAllowed && !( borrower.is_expired && borrower.BlockExpiredPatronOpacActions  ) %]

Here is another actual change.
Comment 50 Owen Leonard 2013-05-09 13:51:01 UTC Comment hidden (obsolete)
Comment 51 Owen Leonard 2013-05-09 13:52:00 UTC Comment hidden (obsolete)
Comment 52 Owen Leonard 2013-05-09 13:53:18 UTC Comment hidden (obsolete)
Comment 53 Katrin Fischer 2013-05-12 18:44:17 UTC
Comment on attachment 18041 [details] [review]
[SIGNED-OFF] Bug 6739 - expired patrons not blocked from opac actions

Review of attachment 18041 [details] [review]:
-----------------------------------------------------------------

Hi Chris H. and Kyle,

I have taken a look at those patches and found some problems, the most severe is in the database update (see splinter review comments below).
Please fix and resubmit.

And a little note for a possible enhancement:
The new option doesn't show up on the patron categories summary table in administration.

::: installer/data/mysql/kohastructure.sql
@@ +441,4 @@
>    `reservefee` decimal(28,6) default NULL, -- cost to place holds
>    `hidelostitems` tinyint(1) NOT NULL default '0', -- are lost items shown to this category (1 for yes, 0 for no)
>    `category_type` varchar(1) NOT NULL default 'A', -- type of Koha patron (Adult, Child, Professional, Organizational, Statistical, Staff)
> +  `BlockExpiredPatronOpacActions` tinyint(1) NOT NULL default '-1', -- wheither or not a patron of this categori can renew books or place holds once their card has expired. 0 means they can, 1 means they cannot, -1 means use syspref BLockExpiredPAtronOpacACtions

Tiny typo: categori

::: installer/data/mysql/updatedatabase.pl
@@ +6845,5 @@
> +            NULL,
> +            'YesNo'
> +        )
> +    ");
> +    $dbh->do("ALTER TABLE `categories` ADD COLUMN `BlockExpiredPatronOpacActions` TINYINT(1) DEFAULT 0 NOT NULL");

The default value for new patron categories is -1 (use system preference), but for existing installations we create the column with default 0. I checked that the follow ups are not fixing this. Failing QA.

Also: It would be a bit safer to make sure the column is added after category_type like in kohastructure.sql.
Comment 54 Fridolin Somers 2013-07-11 09:42:09 UTC Comment hidden (obsolete)
Comment 55 Fridolin Somers 2013-07-11 09:42:52 UTC Comment hidden (obsolete)
Comment 56 Fridolin Somers 2013-07-11 09:43:42 UTC Comment hidden (obsolete)
Comment 57 Fridolin Somers 2013-07-11 10:11:34 UTC Comment hidden (obsolete)
Comment 58 Fridolin Somers 2013-07-11 10:12:23 UTC
Followup 3 needs signoff
Comment 59 Fridolin Somers 2013-07-11 10:46:03 UTC Comment hidden (obsolete)
Comment 60 Fridolin Somers 2013-07-11 10:48:20 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2013-07-15 15:58:36 UTC Comment hidden (obsolete)
Comment 62 Kyle M Hall 2013-07-15 15:58:56 UTC Comment hidden (obsolete)
Comment 63 Katrin Fischer 2013-07-29 20:14:59 UTC
I am sorry, I know this has gone rounds for a while, but I can't get follow up 3 to apply after fixing the merge conflict in the first patch. Can someone please provide rebased patches? Thx!
Comment 64 Katrin Fischer 2013-07-29 20:15:27 UTC
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 6739 - expired patrons not blocked from opac actions - Followup 3
fatal: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Comment 65 Fridolin Somers 2013-07-31 09:26:15 UTC Comment hidden (obsolete)
Comment 66 Fridolin Somers 2013-07-31 09:26:54 UTC Comment hidden (obsolete)
Comment 67 Fridolin Somers 2013-07-31 09:27:30 UTC Comment hidden (obsolete)
Comment 68 Fridolin Somers 2013-07-31 09:28:59 UTC Comment hidden (obsolete)
Comment 69 Fridolin Somers 2013-07-31 09:29:45 UTC Comment hidden (obsolete)
Comment 70 Katrin Fischer 2013-08-26 06:45:42 UTC
Patches no longer apply, I fixed the conflict in sysprefs.sql in the first patch, but got stuck on opac.tt in the third. Can you please rebase?
Comment 71 Fridolin Somers 2013-08-27 08:57:45 UTC Comment hidden (obsolete)
Comment 72 Fridolin Somers 2013-08-27 08:58:35 UTC Comment hidden (obsolete)
Comment 73 Fridolin Somers 2013-08-27 09:03:32 UTC Comment hidden (obsolete)
Comment 74 Fridolin Somers 2013-08-27 09:03:57 UTC Comment hidden (obsolete)
Comment 75 Fridolin Somers 2013-08-27 09:04:24 UTC Comment hidden (obsolete)
Comment 76 Katrin Fischer 2013-09-22 13:07:12 UTC
Working on this now...
Comment 77 Katrin Fischer 2013-09-22 13:27:45 UTC
Happy to say that it applied cleanly and passed the automatic tests :)
Continueing testing...
Comment 78 Katrin Fischer 2013-09-22 14:52:08 UTC Comment hidden (obsolete)
Comment 79 Katrin Fischer 2013-09-22 14:52:21 UTC Comment hidden (obsolete)
Comment 80 Katrin Fischer 2013-09-22 14:52:32 UTC Comment hidden (obsolete)
Comment 81 Katrin Fischer 2013-09-22 14:52:42 UTC Comment hidden (obsolete)
Comment 82 Katrin Fischer 2013-09-22 14:52:53 UTC Comment hidden (obsolete)
Comment 83 Katrin Fischer 2013-09-22 14:54:05 UTC Comment hidden (obsolete)
Comment 84 Katrin Fischer 2013-09-22 14:59:27 UTC
This is something that is quite annoying for some libraries and I feel like I have tested thoroughly (which probably means I missed something). So some automated regression tests would be very much appreciated.
Comment 85 Jonathan Druart 2013-11-20 14:28:16 UTC
Galen,
Are you waiting for something to push these patches ?
Comment 86 Galen Charlton 2014-01-07 03:34:56 UTC
Comment on attachment 21312 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions

Review of attachment 21312 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-user.pl
@@ +65,4 @@
>      }
>  );
>  
> +my $OPACDisplayRequestPriority = (C4::Context->preference("OPACDisplayRequestPriority")) ? 1 : 0;

This is a regression on bug 7398; OPACDisplayRequestPriority no longer exists.

@@ -110,4 @@
>      $canrenew = 0;
>      $template->param(
>          renewal_blocked_fines => sprintf( '%.02f', $no_renewal_amt ),
> -        renewal_blocked_fines_amountoutstanding => sprintf( '%.02f', $borr->{amountoutstanding} ),

Removing this would cause a regression of bug 8408.

@@ -170,5 @@
>                  $charges += $ac->{'amountoutstanding'}
>                    if $ac->{'accounttype'} eq 'F';
>                  $charges += $ac->{'amountoutstanding'}
> -                  if $ac->{'accounttype'} eq 'FU';
> -                $charges += $ac->{'amountoutstanding'}

Removing this would cause a regression on bug 8079.
Comment 87 Galen Charlton 2014-01-07 03:36:42 UTC
Marked as failed QA on account of the three regressions that this patch series would cause.  Overall, looks reasonable, and I expect it to pass into master once the regressions have been fixed.
Comment 88 Jonathan Druart 2014-01-15 11:00:31 UTC
Comment on attachment 21312 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions

Review of attachment 21312 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-user.pl
@@ +157,4 @@
>  if ($issues){
>      foreach my $issue ( sort { $b->{date_due}->datetime() cmp $a->{date_due}->datetime() } @{$issues} ) {
>          # check for reserves
> +        my ( $restype, $res, undef ) = CheckReserves( $issue->{'itemnumber'} );

This is a regression of bug 9367.
Comment 89 Jonathan Druart 2014-01-15 11:04:03 UTC Comment hidden (obsolete)
Comment 90 Jonathan Druart 2014-01-15 11:04:48 UTC Comment hidden (obsolete)
Comment 91 Jonathan Druart 2014-01-15 11:05:57 UTC Comment hidden (obsolete)
Comment 92 Jonathan Druart 2014-01-15 11:06:11 UTC Comment hidden (obsolete)
Comment 93 Jonathan Druart 2014-01-15 11:06:20 UTC Comment hidden (obsolete)
Comment 94 Jonathan Druart 2014-01-15 11:06:38 UTC Comment hidden (obsolete)
Comment 95 Jonathan Druart 2014-01-15 11:06:46 UTC Comment hidden (obsolete)
Comment 96 Jonathan Druart 2014-01-15 11:06:54 UTC Comment hidden (obsolete)
Comment 97 Jonathan Druart 2014-02-10 08:45:13 UTC Comment hidden (obsolete)
Comment 98 Jonathan Druart 2014-02-10 08:45:22 UTC Comment hidden (obsolete)
Comment 99 Jonathan Druart 2014-02-10 08:45:36 UTC Comment hidden (obsolete)
Comment 100 Jonathan Druart 2014-02-10 08:45:59 UTC Comment hidden (obsolete)
Comment 101 Jonathan Druart 2014-02-10 08:46:08 UTC Comment hidden (obsolete)
Comment 102 Jonathan Druart 2014-02-10 08:46:18 UTC Comment hidden (obsolete)
Comment 103 Jonathan Druart 2014-02-10 08:46:27 UTC Comment hidden (obsolete)
Comment 104 Jonathan Druart 2014-02-10 08:46:35 UTC Comment hidden (obsolete)
Comment 105 Chris Cormack 2014-02-13 22:30:22 UTC Comment hidden (obsolete)
Comment 106 Chris Cormack 2014-02-13 22:30:44 UTC Comment hidden (obsolete)
Comment 107 Chris Cormack 2014-02-13 22:31:10 UTC Comment hidden (obsolete)
Comment 108 Katrin Fischer 2014-03-22 10:28:33 UTC
I am sorry, there is a conflict I can't resolve in this patch series:

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 6739 - expired patrons not blocked from opac actions - Followup 2
fatal: sha1 information is lacking or useless (koha-tmpl/opac-tmpl/prog/en/modules/opac-user.tt).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 6739 - expired patrons not blocked from opac actions - Followup 2
Comment 109 Jonathan Druart 2014-03-25 09:40:14 UTC Comment hidden (obsolete)
Comment 110 Jonathan Druart 2014-03-25 09:40:24 UTC Comment hidden (obsolete)
Comment 111 Jonathan Druart 2014-03-25 09:40:31 UTC Comment hidden (obsolete)
Comment 112 Jonathan Druart 2014-03-25 09:40:38 UTC Comment hidden (obsolete)
Comment 113 Jonathan Druart 2014-03-25 09:40:45 UTC Comment hidden (obsolete)
Comment 114 Jonathan Druart 2014-03-25 09:40:52 UTC Comment hidden (obsolete)
Comment 115 Jonathan Druart 2014-03-25 09:40:58 UTC Comment hidden (obsolete)
Comment 116 Jonathan Druart 2014-03-25 09:41:05 UTC Comment hidden (obsolete)
Comment 117 Jonathan Druart 2014-03-25 09:41:47 UTC
(In reply to Katrin Fischer from comment #108)
> I am sorry, there is a conflict I can't resolve in this patch series:

Patches rebased against master.
Comment 118 Katrin Fischer 2014-03-31 20:37:51 UTC
Applies now and passes QA script, more testing later.
Comment 119 Katrin Fischer 2014-04-01 05:46:05 UTC
Testing this again, please have a little patience.
Comment 120 Katrin Fischer 2014-04-06 17:54:50 UTC
Back to this...
Comment 121 Katrin Fischer 2014-04-06 20:10:26 UTC
Created attachment 26836 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions

Test Plan:
 1) Apply patch
 2) Run updatedatabase.pl
 3) Pick a patron, note the patron's category
 5) Issue an item to this patron
 4) Edit that category, set "Block expired patrons" to "Block"
 5) Verify the patron cannot renew or place holds in the opac
 6) Edit the category again, set "Block expired patrons" to "Don't block"
 7) Verify the patron *can* renew and place holds in the opac
 8) Edit the category again, set "Block expired patrons" to "Follow system preference BlockExpiredPatronOpacActions"
 9) Set the system preference BlockExpiredPatronOpacActions to "Block"
10) Verify the patron cannot renew or place holds in the opac
11) Set the system preference BlockExpiredPatronOpacActions to "Don't block"
12) Verify the patron *can* renew and place holds in the opac

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Melia Meggs <melia@bywatersolutions.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Testing notes on last patch in series.
Comment 122 Katrin Fischer 2014-04-06 20:10:34 UTC
Created attachment 26837 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup

Signed-off-by: Melia Meggs <melia@bywatersolutions.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 123 Katrin Fischer 2014-04-06 20:10:39 UTC
Created attachment 26838 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 2

* Hide renewal column, buttons and links
* Fixes minor bug where upper-right table links would show even if renewal impossible
* Cleans up some of the template formatting ( whitespace changes only )

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 124 Katrin Fischer 2014-04-06 20:10:45 UTC
Created attachment 26839 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 3

Installer fixes :
- in kohastructure.sql, category.BlockExpiredPatronOpacActions default value is -1, sets the same in updatedatabase.pl
- in syspref comment, replaces "opac actions such as placing a hold or reserve" by "opac actions such as placing holds or renrw books"
- A 'YesNo' does not have 'yes' as value in database, it is '1'.
- corrects small typo "categori" and syspref name case

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 125 Katrin Fischer 2014-04-06 20:10:51 UTC
Created attachment 26840 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 4

Some corrections :
- opac-reserve.tt : opening <p> instead of closing
- opac-user.tt : warnexpired was in database format, adds the use of KohaDates template plugin
- opac-user.tt : duplicated TT test : [% IF ( BORROWER_INF.warnexpired ) %] and [% ELSIF ( BORROWER_INF.warnexpired ) %], maybe a merge error
- opac-user.tt : <string> instead of <strong>, maybe for HTML 6 :-)
- opac-user.pl : adding dateformat var to template is already done by Auth.pm

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.

Testing notes:
- Database update
  * Changes to kohastructure match changes done by the updatedatabase
    statement. Feature is activated by default. Fixing 'yes' to be '1'
    in a follow up.
  * Ran database update succesfully.
  * Note: Patrons are now blocked by default in new installations
    AND in updated installations.
- System preference
  * Verified system preference shows up correctly.
- Category configuration
  * Add new patron category
  * Edit existing patron category
  * Delete patron category
  * Check patron category summary table.
  => Verified all actions work as expected.
  => Verified chosen value for BlockExpiredPatronOpacActions'
     is always displayed and saved correctly.
  * Note: The new value is missing from the summary table.
  * Note: The new value is also not shown when deleting a patron category.
- Check functionality
  * Renew and place a hold for an NOT EXPIRED patron with
    a) category: use syspref (default)
       syspref: block  (default)
    b) category: use syspref (default)
       syspref: don't block
    c) category: block
       syspref: don't block
    d) category: block
       syspref: block
    e) category: don't block
       sypref: block
    * Verified renewals and placing holds were never blocked.
    * Also verified that the warning from NotifyBorrowerDeparture
      still shows up correctly.
  * Renew and place a hold for an EXPIRED patron with
    a) category: use syspref (default)
       syspref: block  (default)
       => OK, both actions are blocked.
    b) category: use syspref (default)
       syspref: don't block
       => OK, both actions possible.
    c) category: block
       syspref: don't block
       => OK, both actions are blocked.
    d) category: block
       syspref: block
       => OK, both actions are blocked.
    e) category: don't block
       sypref: block
       => OK, both actions possible.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
2014-04-06 Update: Will repeat and amend above test plan on last patch in this series.
Comment 126 Katrin Fischer 2014-04-06 20:10:56 UTC
Created attachment 26841 [details] [review]
[PASSED QA] Bug 6739 - expired patrons not blocked from opac actions - Followup 5

- Fixes a typo in one of the OPAC messages: "you account"
- Fixes the updatedatabase script to use 1 instead of yes for the
  system preference default matching the sysprefs.sql file.
- Fixes capitalization "Don't Block" in system preference
  description.
- Adding . to the end of the warning, when renewals are blocked.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 127 Katrin Fischer 2014-04-06 20:11:02 UTC
Created attachment 26842 [details] [review]
[PASSED QA] Bug 6739: FIX conflicts with some others bugs.

see comments 86 and 88. This patch fixes a bad conflict resolution in a
previous patch.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 128 Katrin Fischer 2014-04-06 20:11:09 UTC
Created attachment 26843 [details] [review]
[PASSED QA] Bug 6739: Template changes for the bootstrap theme

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes QA script, t, xt and t/db_dependent/Members*.

Testing notes:
    - Database update
      * Changes to kohastructure match changes done by the updatedatabase
        statement. Feature is activated by default!
      * Ran database update succesfully.
      * Note: Expired patrons are now blocked by default in new
        AND in updated installations.
    - System preference
      * Verified system preference shows up correctly.
    - Category configuration
      * Add new patron category
      * Edit existing patron category
      * Delete patron category
      * Check patron category summary table.
      => Verified all actions work as expected.
      => Verified chosen value for BlockExpiredPatronOpacActions
         is always displayed and saved correctly.
      * Note: The new value is missing from the summary table.
      * Note: The new value is also not shown when deleting a patron
        category.
    - Check functionality
      * Renew and place a hold for an NOT EXPIRED patron with
        a) category: use syspref (default)
           syspref: block  (default)
        b) category: use syspref (default)
           syspref: don't block
        c) category: block
           syspref: don't block
        d) category: block
           syspref: block
        e) category: don't block
           sypref: block
        f) category: don't block
           syspref: don't block
        Holds and renewals were always possible.
      * Also verified that the warning from NotifyBorrowerDeparture
        still shows up correctly.
      * Verified holds and renewals are still blocked, when the
        user has a restriction with a date in the future.
      * Renew and place a hold for an EXPIRED patron with
        a) category: use syspref (default)
           syspref: block  (default)
           Holds and renewals blocked.
        b) category: use syspref (default)
           syspref: don't block
           Holds an renewals possible.
        c) category: block
           syspref: don't block
           Holds and renewals blocked.
        d) category: block
           syspref: block
           Holds and renewals blocked.
        e) category: don't block
           sypref: block
           Holds and renewals possible.
        f) category: don't block
           syspref: don't block
           Holds and renewals possible.
Comment 129 Katrin Fischer 2014-04-06 20:13:20 UTC
There are a few more things to note:

- Changing the existing behaviour by activating the feature by default might
  be debatable. I am going to leave this as it is and it could be changed easily
  if we get some opionions on this before release.
- The changes to GetMemberDetails are currently not covered by unit tests. All
  existing tests in for Members* pass, so I am going to pass this on the
  condition, that we get some more to cover the changes.

Enhancements:
- Show the new value on the patron category summary table
- Show the new field when deleting a patron category
Comment 130 Galen Charlton 2014-05-05 02:24:33 UTC
Pushed to master.  Thanks, Chris, Kyle, Fridolin, Katrin, and Jonathan!

Note that I have elected to have the feature be OFF by default for upgrades.  It will be on by default for new installations.
Comment 131 Fridolin Somers 2014-10-29 10:41:20 UTC
Patch too complex and big to backport to 3.14.x